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.

Sign up for GitHub

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

Jump to bottom

fix(eslint-plugin): [prefer-optional-chain] fix ThisExpression and PrivateIdentifier errors #6028

Merged
merged 25 commits into from Jan 31, 2023
Merged

fix(eslint-plugin): [prefer-optional-chain] fix ThisExpression and PrivateIdentifier errors #6028

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
71cb502
prevent thrown error from default case
Nov 17, 2022
5acc1af
try
Nov 17, 2022
a3b6eda
Revert "try"
Nov 17, 2022
a68b13a
More cases and format
Nov 18, 2022
7e2185e
Update packages/eslint-plugin/tests/rules/prefer-optional-chain.test.ts
omril1 Nov 18, 2022
1a30e94
Merge remote-tracking branch 'upstream/main' into fix/issue6024-fix-p…
Nov 24, 2022
21cfbd2
TSAsExpression,AwaitExpression,NewExpression
Nov 25, 2022
0ae6c45
Merge remote-tracking branch 'upstream/main' into fix/issue6024-fix-p…
Nov 25, 2022
ecfacff
Merge branch 'main' into fix/issue6024-fix-prefer-optional-chain-priv…
Dec 5, 2022
3ce9b0a
Merge branch 'main' into fix/issue6024-fix-prefer-optional-chain-priv…
Dec 17, 2022
c3c458e
CR: remove throws and simplify switch cases
Dec 21, 2022
9c15f28
Merge branch 'main' into fix/issue6024-fix-prefer-optional-chain-priv…
Dec 21, 2022
ab70949
support private identifiers
Dec 24, 2022
e58dab9
Merge remote-tracking branch 'upstream/main' into fix/issue6024-fix-p…
Dec 24, 2022
12effef
coverage
Dec 24, 2022
0afc516
over covered case
Dec 24, 2022
8efa51d
Clearly, we had some missing coverage 😄
Dec 24, 2022
57b86c1
this got uglier
Dec 24, 2022
99528f0
fix coverage
Dec 25, 2022
cbb5168
Merge branch 'main' into fix/issue6024-fix-prefer-optional-chain-priv…
Jan 1, 2023
b6d265c
Merge branch 'main' into fix/issue6024-fix-prefer-optional-chain-priv…
Jan 2, 2023
54d4664
Merge branch 'main' into fix/issue6024-fix-prefer-optional-chain-priv…
Jan 13, 2023
ac8b6a2
Merge remote-tracking branch 'upstream/main' into fix/issue6024-fix-p…
Jan 30, 2023
a2a6314
Merge branch 'main' into fix/issue6024-fix-prefer-optional-chain-priv…
Jan 30, 2023
37716b2
Merge branch 'main' into fix/issue6024-fix-prefer-optional-chain-priv…
Jan 31, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
25 changes: 22 additions & 3 deletions packages/eslint-plugin/src/rules/prefer-optional-chain.ts
View file
Open in desktop
Expand Up @@ -368,6 +368,15 @@ export default util.createRule({
* Gets a normalized representation of the given MemberExpression
*/
function getMemberExpressionText(node: TSESTree.MemberExpression): string {
if (
[
AST_NODE_TYPES.TSAsExpression,
AST_NODE_TYPES.AwaitExpression,
AST_NODE_TYPES.NewExpression,
].includes(node.object.type)
) {
return '';
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This feels a bit like a hack since typescript doesn't seem to think they are assignable to MemberExpression
if (node.object.type === AST_NODE_TYPES.TSAsExpression) {} shows a typescript error, but with Array#includes it does not

Copy link
Member

Choose a reason for hiding this comment

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

It is a bit of a hack 😄 but the root cause is a bug in our typings, I think. A node's member can be an await or new. [typescript-eslint playground]

const example = {} as any;

(await example).prop;
(await new example).prop;

For now, you can use a // @ts-expect-error and link to #6239 #6192

let objectText: string;

// cases should match the list in ALLOWED_MEMBER_OBJECT_TYPES
Expand Down Expand Up @@ -425,9 +434,19 @@ export default util.createRule({

/* istanbul ignore next */
omril1 marked this conversation as resolved.
Show resolved Hide resolved
default:
throw new Error(
`Unexpected member property type: ${node.object.type}`,
);
if (
![
AST_NODE_TYPES.Identifier,
AST_NODE_TYPES.ThisExpression,
AST_NODE_TYPES.CallExpression,
AST_NODE_TYPES.MemberExpression,
].includes(node.object.type)
) {
throw new Error(
`Unexpected member property type: ${node.object.type}`,
);
}
omril1 marked this conversation as resolved.
Show resolved Hide resolved
propertyText = sourceCode.getText(node.property);
}

return `${objectText}${node.optional ? '?.' : '.'}${propertyText}`;
Expand Down
12 changes: 12 additions & 0 deletions packages/eslint-plugin/tests/rules/prefer-optional-chain.test.ts
View file
Open in desktop
Expand Up @@ -194,6 +194,18 @@ ruleTester.run('prefer-optional-chain', rule, {
'match && match$1 !== undefined;',
'foo !== null && foo !== undefined;',
"x['y'] !== undefined && x['y'] !== null;",
// private properties
'this.#a && this.#b;',
Copy link
Member

Choose a reason for hiding this comment

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

This should still report a complaint:

class Violation {
  #property = 0;

  test() {
    const self = Math.random() ? this : undefined;

    self && self#.property;
  }
}

I suspect the issue is returning '' instead of the actual node text above.

Copy link
Member

Choose a reason for hiding this comment

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

bump on this 😇

Copy link
Contributor Author

@omril1 omril1 Dec 24, 2022 •
edited

Choose a reason for hiding this comment

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

The highlighted line has two different properties (#a and #b) and the example seems to be covered by this, am I missing something?

typescript-eslint/packages/eslint-plugin/tests/rules/prefer-optional-chain.test.ts

Lines 141 to 144 in 0afc516

{
code: 'foo && foo.#bar',
output: 'foo?.#bar',
},

Copy link
Member

Choose a reason for hiding this comment

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

Ah sorry you're right, I missed that they're the same property. But this actually brings up that there's a bug in this PR for private identifiers: microsoft/TypeScript#42734 blocks them from being used.

https://deploy-preview-6028--typescript-eslint.netlify.app/play#ts=4.9.3&sourceType=module&code=MYGwhgzhAEBqCWB7cAXJA7aBvAUNaAxAA4BOiRApiSgJ7QC80ADANw57QoUQoAUAlNg75gidD2gQKIAGYNoAWTAoAFgDoSYdABNEAWwHQA-JxXwYALmgBXHRRnx0FbW2GTpMo2uJlK1Gmz4AL44IUA&eslintrc=N4KABGBEBOCuA2BTAzpAXGUEKQAIBcBPABxQGNoBLY-AWhXkoDt8B6Y6RAM0WloHsalfkwCG8WmQAWo5uii9o-aJHBgAviHVA&tsconfig=N4KABGBEDGD2C2AHAlgGwKYCcDyiAuysAdgM6QBcYoEEkJemy0eAcgK6qoDCAFutAGsylBm3TgwAXxCSgA

class Violation {
  #property = 0;

  test() {
    const self = Math.random() ? this : undefined;

    self && self.#property;
  }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So do I revert it for now? What do you suggest?

Copy link
Member

Choose a reason for hiding this comment

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

Well, it shouldn't throw an error - that's definitely a bug. But it also shouldn't complain. The rule should effectively ignore this case for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Kept the private properties in computed nodes

'!this.#a || !this.#b;',
'a.#foo && a.#foo.bar;',
'!a.#foo || !a.#foo.bar;',
'a.#foo?.bar;',
'!a.#foo?.bar;',
Copy link
Member

@Josh-Cena Josh-Cena Nov 17, 2022 •
edited

Choose a reason for hiding this comment

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

More test cases to consider:

foo().#a
a.b.#a
new A().#b
(await a).#b

I have no idea why this rule is so strict about what expressions it can lint, but you can in fact use private properties on all expressions, just that some may have nonsensical behaviors (e.g. (typeof a).#a). It shouldn't crash the linter anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can I fix the rule to not intentionally throw? this seems irrational to me.
It's better to not show a suggestion than to just break and do nothing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would love to change this rule to not intentionally throw, or at least catch the error.
The rule logic now has many combinations that could be missed and better be gracefully ignored.

@Josh-Cena @JoshuaKGoldberg @bradzacher WDYT?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah as Brad said in #6028 (comment), I think the rule should no longer throw. Throwing is useful for us finding missing combinations but is inconvenient for users.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, so I misunderstood his comment

'!foo().#a || a;',
'!a.b.#a || a;',
'!new A().#b || a;',
'!(await a).#b || a;',
"!(foo as any).bar || 'anything';",
// currently do not handle complex computed properties
'foo && foo[bar as string] && foo[bar as string].baz;',
'foo && foo[1 + 2] && foo[1 + 2].baz;',
Expand Down