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

[djcsdy]

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]

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.

[bradzacher]

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.

[bradzacher]

nitpick prefer comparisons against null

Suggested change

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

[djcsdy]

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.

[bradzacher]

@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.

[codecov]

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%)	⬆️

[djcsdy]

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.

[bradzacher]

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

[eugene-stativka]

@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?

[bradzacher]

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