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 22 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
51 changes: 34 additions & 17 deletions packages/eslint-plugin/src/rules/prefer-optional-chain.ts
View file
Open in desktop
Expand Up @@ -10,6 +10,7 @@ type ValidChainTarget =
| TSESTree.CallExpression
| TSESTree.ChainExpression
| TSESTree.Identifier
| TSESTree.PrivateIdentifier
| TSESTree.MemberExpression
| TSESTree.ThisExpression
| TSESTree.MetaProperty;
Expand Down Expand Up @@ -164,7 +165,9 @@ export default util.createRule({
break;
}

let invalidOptionallyChainedPrivateProperty;
({
invalidOptionallyChainedPrivateProperty,
expressionCount,
previousLeftText,
optionallyChainedCode,
Expand All @@ -178,6 +181,9 @@ export default util.createRule({
previous,
current,
));
if (invalidOptionallyChainedPrivateProperty) {
return;
}
}

reportIfMoreThanOne({
Expand Down Expand Up @@ -243,7 +249,9 @@ export default util.createRule({
break;
}

let invalidOptionallyChainedPrivateProperty;
({
invalidOptionallyChainedPrivateProperty,
expressionCount,
previousLeftText,
optionallyChainedCode,
Expand All @@ -257,6 +265,9 @@ export default util.createRule({
previous,
current,
));
if (invalidOptionallyChainedPrivateProperty) {
return;
}
}

reportIfMoreThanOne({
Expand Down Expand Up @@ -343,7 +354,10 @@ export default util.createRule({
return `${calleeText}${argumentsText}`;
}

if (node.type === AST_NODE_TYPES.Identifier) {
if (
node.type === AST_NODE_TYPES.Identifier ||
node.type === AST_NODE_TYPES.PrivateIdentifier
) {
return node.name;
}

Expand Down Expand Up @@ -381,23 +395,20 @@ export default util.createRule({

// cases should match the list in ALLOWED_MEMBER_OBJECT_TYPES
switch (node.object.type) {
case AST_NODE_TYPES.CallExpression:
case AST_NODE_TYPES.Identifier:
objectText = getText(node.object);
break;

case AST_NODE_TYPES.MemberExpression:
objectText = getMemberExpressionText(node.object);
break;

case AST_NODE_TYPES.CallExpression:
case AST_NODE_TYPES.Identifier:
case AST_NODE_TYPES.MetaProperty:
case AST_NODE_TYPES.ThisExpression:
objectText = getText(node.object);
break;

/* istanbul ignore next */
default:
throw new Error(`Unexpected member object type: ${node.object.type}`);
return '';
}

let propertyText: string;
Expand All @@ -420,9 +431,7 @@ export default util.createRule({

/* istanbul ignore next */
default:
throw new Error(
`Unexpected member property type: ${node.object.type}`,
);
return '';
}

return `${objectText}${node.optional ? '?.' : ''}[${propertyText}]`;
Expand All @@ -432,12 +441,12 @@ export default util.createRule({
case AST_NODE_TYPES.Identifier:
propertyText = getText(node.property);
break;
case AST_NODE_TYPES.PrivateIdentifier:
propertyText = '#' + getText(node.property);
break;

/* istanbul ignore next */
default:
throw new Error(
`Unexpected member property type: ${node.object.type}`,
);
propertyText = sourceCode.getText(node.property);
}

return `${objectText}${node.optional ? '?.' : '.'}${propertyText}`;
Expand All @@ -461,6 +470,7 @@ const ALLOWED_COMPUTED_PROP_TYPES: ReadonlySet<AST_NODE_TYPES> = new Set([
]);
const ALLOWED_NON_COMPUTED_PROP_TYPES: ReadonlySet<AST_NODE_TYPES> = new Set([
AST_NODE_TYPES.Identifier,
AST_NODE_TYPES.PrivateIdentifier,
]);

interface ReportIfMoreThanOneOptions {
Expand Down Expand Up @@ -490,10 +500,9 @@ function reportIfMoreThanOne({
shouldHandleChainedAnds &&
previous.right.type === AST_NODE_TYPES.BinaryExpression
) {
const rightText = sourceCode.getText(previous.right.right);
// case like foo && foo.bar !== someValue
optionallyChainedCode += ` ${
previous.right.operator
} ${sourceCode.getText(previous.right.right)}`;
optionallyChainedCode += ` ${previous.right.operator} ${rightText}`;
}

context.report({
Expand All @@ -515,6 +524,7 @@ function reportIfMoreThanOne({
}

interface NormalizedPattern {
invalidOptionallyChainedPrivateProperty: boolean;
expressionCount: number;
previousLeftText: string;
optionallyChainedCode: string;
Expand All @@ -531,6 +541,7 @@ function normalizeRepeatingPatterns(
current: TSESTree.Node,
): NormalizedPattern {
const leftText = previousLeftText;
let invalidOptionallyChainedPrivateProperty = false;
// omit weird doubled up expression that make no sense like foo.bar && foo.bar
if (rightText !== previousLeftText) {
expressionCount += 1;
Expand Down Expand Up @@ -566,6 +577,11 @@ function normalizeRepeatingPatterns(
diff === '?.buzz'
*/
const diff = rightText.replace(leftText, '');
if (diff.startsWith('.#')) {
// Do not handle direct optional chaining on private properties because of a typescript bug (https://github.com/microsoft/TypeScript/issues/42734)
// We still allow in computed properties
invalidOptionallyChainedPrivateProperty = true;
}
if (diff.startsWith('?')) {
// item was "pre optional chained"
optionallyChainedCode += diff;
Expand All @@ -581,6 +597,7 @@ function normalizeRepeatingPatterns(
util.NullThrowsReasons.MissingParent,
);
return {
invalidOptionallyChainedPrivateProperty,
expressionCount,
previousLeftText,
optionallyChainedCode,
Expand Down
28 changes: 27 additions & 1 deletion packages/eslint-plugin/tests/rules/prefer-optional-chain.test.ts
View file
Open in desktop
Expand Up @@ -133,6 +133,19 @@ const baseCases = [
code: 'foo.bar && foo.bar?.() && foo.bar?.().baz',
output: 'foo.bar?.()?.baz',
},
// private properties
{
code: 'foo.#bar && foo.#bar.baz',
output: 'foo.#bar?.baz',
},
{
code: 'foo && foo[bar.#baz]',
output: 'foo?.[bar.#baz]',
},
{
code: 'foo[bar.#baz] && foo[bar.#baz].buzz',
output: 'foo[bar.#baz]?.buzz',
},
].map(
c =>
({
Expand Down Expand Up @@ -194,6 +207,16 @@ 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?.bar;',
'!a.#foo?.bar;',
'!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 All @@ -217,6 +240,10 @@ ruleTester.run('prefer-optional-chain', rule, {
'!import.meta && !import.meta.foo;',
'new.target || new.target.length;',
'!new.target || true;',
// Do not handle direct optional chaining on private properties because of a typescript bug (https://github.com/microsoft/TypeScript/issues/42734)
// We still allow in computed properties
'foo && foo.#bar;',
'!foo || !foo.#bar;',
],
invalid: [
...baseCases,
Expand Down Expand Up @@ -1384,7 +1411,6 @@ foo?.bar(/* comment */a,
},
],
},

{
code: noFormat`import.meta && import.meta?.() && import.meta?.().baz;`,
output: null,
Expand Down