Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(eslint-plugin): [dot-notation] optionally allow square bracket notation where an index signature exists in conjunction with noPropertyAccessFromIndexSignature #3361

Merged

Conversation

@djcsdy
Copy link
Contributor

@djcsdy djcsdy commented May 8, 2021

Fixes #3104.

This PR modifies the dot-notation rule to add a new option named allowIndexSignaturePropertyAccess.

When this option is set to true, dot-notation allows the use of square bracket notation if there is a corresponding index signature defined on the type that is being dereferenced. For example:

class X {
    property: string;
    [key: string]: number;
}

const x = new X();
x["hello"] = 123; // Allowed if `allowIndexSignaturePropertyAccess: true`
x["property"] = "hello"; // Still disallowed, should use dot-notation.

If the TypeScript compiler option noPropertyAccessFromIndexSignature is set to true, then the dot-notation rule always behaves as if allowIndexSignaturePropertyAccess is set to true. The motivation for this behaviour is that noPropertyAccessFromIndexSignature causes the TypeScript compiler to require the use of square bracket notation, so typescript-eslint conflicts with the compiler if it disallows square bracket notation in this case. See #3104.

@typescript-eslint
Copy link

@typescript-eslint typescript-eslint bot commented May 8, 2021

Thanks for the PR, @djcsdy!

typescript-eslint is a 100% community driven project, and we are incredibly grateful that you are contributing to that community.

The core maintainers work on this in their personal time, so please understand that it may not be possible for them to review your work immediately.

Thanks again!


🙏 Please, if you or your company is finding typescript-eslint valuable, help us sustain the project by sponsoring it transparently on https://opencollective.com/typescript-eslint. As a thank you, your profile/company logo will be added to our main README which receives thousands of unique visitors per day.

Loading

@bradzacher bradzacher changed the title feat(eslint-plugin): optionally allow square bracket notation where an index signature exists feat(eslint-plugin): [dot-notation] optionally allow square bracket notation where an index signature exists May 15, 2021
Copy link
Member

@bradzacher bradzacher left a comment

almost there! the logic is looking pretty good so far.

One thing I think we should block is accessing a known property via the computed notation. Known properties should be accessed via dot notation.

interface SomeType {
    foo: string;
    [propName: string]: any;
}

function doStuff(value: SomeType) {
    let x = value["someProperty"]; // should NOT error
    let y = value.foo; // should NOT error
    let z = value['foo']; // should error!!!
}

I think the rule should be strict about this because the rule is intended to ensure you're using dot notation whenever you can.

Loading

objectType,
ts.IndexKind.String,
);
if (indexType != undefined) {
Copy link
Member

@bradzacher bradzacher May 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick prefer comparisons against null

Suggested change
if (indexType != undefined) {
if (indexType != null) {

Loading

Copy link
Contributor Author

@djcsdy djcsdy May 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One thing I think we should block is accessing a known property via the computed notation. Known properties should be accessed via dot notation.

This is already blocked, see the tests.

Loading

@bradzacher
Copy link
Member

@bradzacher bradzacher commented May 15, 2021

@djcsdy

Sorry, to clarify. There are two cases here.

The first is the case of a known property on a type with no index signature.
The rule handles this just fine and your logic doesn't touch this branch :)

interface Foo { prop: string }
declare const x: Foo;

x.prop // ✅ rule does not report

The second case is the case of a known property on a type with an index signature.

interface SomeType {
    foo: string;
    [propName: string]: any;
}

function doStuff(value: SomeType) {
    // should NOT error if the compiler option is on
    // your code correctly handles this ✅
    let x = value["someProperty"];

    // this case should not error ever!
    // your code correctly handles this ✅
    let y = value.foo;

    // this case SHOULD error!
    // your code does not currently error on this
    let z = value['foo'];
}

This is a bit of a trickier case to handle as you'll have to look up the property in the base type to see if it exists.

Loading

@codecov
Copy link

@codecov codecov bot commented May 15, 2021

Codecov Report

Merging #3361 (fc11f65) into master (ddfab95) will increase coverage by 0.01%.
The diff coverage is 83.33%.

@@            Coverage Diff             @@
##           master    #3361      +/-   ##
==========================================
+ Coverage   92.85%   92.87%   +0.01%     
==========================================
  Files         318      318              
  Lines       11048    11056       +8     
  Branches     3128     3132       +4     
==========================================
+ Hits        10259    10268       +9     
  Misses        352      352              
+ Partials      437      436       -1     
Flag Coverage Δ
unittest 92.87% <83.33%> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
packages/eslint-plugin/src/rules/dot-notation.ts 92.59% <83.33%> (+8.38%) ⬆️

Loading

@djcsdy
Copy link
Contributor Author

@djcsdy djcsdy commented May 15, 2021

I understood what you meant. I believe it is already handled, see this test case: https://github.com/typescript-eslint/typescript-eslint/pull/3361/files#diff-93cb24cbe290e1f30e97aaf0631ade9bd3751820d1f0bef8ecb4a0532243e2bdR302-R323

However I will double check, perhaps the different type signatures between the index type and the property are confusing matters in this test case.

Loading

@bradzacher
Copy link
Member

@bradzacher bradzacher commented May 15, 2021

I'm so sorry - I wasn't reading your code or tests correctly.
You're absolutely right!

Loading

@bradzacher bradzacher changed the title feat(eslint-plugin): [dot-notation] optionally allow square bracket notation where an index signature exists feat(eslint-plugin): [dot-notation] optionally allow square bracket notation where an index signature exists in conjunction with noPropertyAccessFromIndexSignature May 15, 2021
@bradzacher bradzacher merged commit 37ec2c2 into typescript-eslint:master May 15, 2021
9 of 10 checks passed
Loading
@eugene-stativka
Copy link

@eugene-stativka eugene-stativka commented May 26, 2021

@djcsdy @bradzacher Hey! I think there is a bug when there is an optional chaining operator:

foo?.bar['buzz'] // unexpected eslint error

Can you please have a look?

Loading

@bradzacher
Copy link
Member

@bradzacher bradzacher commented May 26, 2021

Please file an issue. PRs are not the place for discussing bugs.
https://github.com/typescript-eslint/typescript-eslint/blob/master/CONTRIBUTING.md

Loading

@typescript-eslint typescript-eslint locked as resolved and limited conversation to collaborators May 26, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants