From a5d7ec7edfd675379a8cd1ef6ba6d926c47d3e04 Mon Sep 17 00:00:00 2001 From: Omri Luzon Date: Mon, 10 Jan 2022 17:30:46 +0200 Subject: [PATCH 1/9] feat(eslint-plugin): issue 4395 Empty object as optional chaining --- .../docs/rules/prefer-optional-chain.md | 7 ++ .../src/rules/prefer-optional-chain.ts | 37 +++++++++++ .../tests/rules/prefer-optional-chain.test.ts | 65 +++++++++++++++++++ 3 files changed, 109 insertions(+) diff --git a/packages/eslint-plugin/docs/rules/prefer-optional-chain.md b/packages/eslint-plugin/docs/rules/prefer-optional-chain.md index 179ba988733..8e3bbb242c4 100644 --- a/packages/eslint-plugin/docs/rules/prefer-optional-chain.md +++ b/packages/eslint-plugin/docs/rules/prefer-optional-chain.md @@ -20,6 +20,10 @@ function myFunc(foo: T | null) { function myFunc(foo: T | null) { return foo && foo.a && foo.a.b && foo.a.b.c; } +// or +function myFunc(foo: T | null) { + return (((foo || {}).a || {}).b || {}).c; +} function myFunc(foo: T | null) { return foo?.['a']?.b?.c; @@ -55,6 +59,9 @@ foo && foo.a && foo.a.b && foo.a.b.c; foo && foo['a'] && foo['a'].b && foo['a'].b.c; foo && foo.a && foo.a.b && foo.a.b.method && foo.a.b.method(); +(((foo || {}).a || {}).b {}).c; +(((foo || {})['a'] || {}).b {}).c; + // this rule also supports converting chained strict nullish checks: foo && foo.a != null && diff --git a/packages/eslint-plugin/src/rules/prefer-optional-chain.ts b/packages/eslint-plugin/src/rules/prefer-optional-chain.ts index 84c2e15e397..46df13215b6 100644 --- a/packages/eslint-plugin/src/rules/prefer-optional-chain.ts +++ b/packages/eslint-plugin/src/rules/prefer-optional-chain.ts @@ -52,6 +52,43 @@ export default util.createRule({ create(context) { const sourceCode = context.getSourceCode(); return { + 'LogicalExpression[operator=||]'(node: TSESTree.LogicalExpression): void { + const rightNode = node.right; + const parentNode = node.parent; + const isRightNodeAnEmptyObjectLiteral = + rightNode.type === AST_NODE_TYPES.ObjectExpression && + rightNode.properties.length === 0; + if ( + !isRightNodeAnEmptyObjectLiteral || + !parentNode || + parentNode.type !== AST_NODE_TYPES.MemberExpression || + parentNode.optional + ) { + return; + } + context.report({ + node: parentNode, + messageId: 'optionalChainSuggest', + suggest: [ + { + messageId: 'optionalChainSuggest', + fix: (fixer): TSESLint.RuleFix => { + const leftNodeText = context.getSourceCode().getText(node.left); + const propertyToBeOptionalText = context + .getSourceCode() + .getText(parentNode.property); + const maybeWrapped = parentNode.computed + ? `[${propertyToBeOptionalText}]` + : propertyToBeOptionalText; + return fixer.replaceTextRange( + parentNode.range, + `${leftNodeText}?.${maybeWrapped}`, + ); + }, + }, + ], + }); + }, [[ 'LogicalExpression[operator="&&"] > Identifier', 'LogicalExpression[operator="&&"] > MemberExpression', diff --git a/packages/eslint-plugin/tests/rules/prefer-optional-chain.test.ts b/packages/eslint-plugin/tests/rules/prefer-optional-chain.test.ts index feb7f9bd524..34a7abb46c7 100644 --- a/packages/eslint-plugin/tests/rules/prefer-optional-chain.test.ts +++ b/packages/eslint-plugin/tests/rules/prefer-optional-chain.test.ts @@ -146,6 +146,10 @@ const baseCases = [ ruleTester.run('prefer-optional-chain', rule, { valid: [ + 'foo || {};', + 'foo || ({} as any);', + '(foo || {})?.bar;', // This might seem stupid, but I'm not sure if we want to show suggestion for it + '(foo || { bar: 1 }).bar;', 'foo && bar;', 'foo && foo;', 'foo || bar;', @@ -501,5 +505,66 @@ foo?.bar(/* comment */a, }, }, }, + { + code: 'const foo = (bar || {}).baz;', + errors: [ + { + messageId: 'optionalChainSuggest', + column: 13, + endColumn: 28, + suggestions: [ + { + messageId: 'optionalChainSuggest', + output: 'const foo = bar?.baz;', + }, + ], + }, + ], + }, + { + code: '(foo.bar || {})[baz];', + errors: [ + { + messageId: 'optionalChainSuggest', + column: 1, + endColumn: 21, + suggestions: [ + { + messageId: 'optionalChainSuggest', + output: 'foo.bar?.[baz];', + }, + ], + }, + ], + }, + { + // Currently it does not suggest a fix for nested optional with empty object + // It shows 2 suggestions, one for the outer object and one for the inner object + code: '((foo1 || {}).foo2 || {}).foo3;', + errors: [ + { + messageId: 'optionalChainSuggest', + column: 1, + endColumn: 31, + suggestions: [ + { + messageId: 'optionalChainSuggest', + output: '(foo1 || {}).foo2?.foo3;', + }, + ], + }, + { + messageId: 'optionalChainSuggest', + column: 2, + endColumn: 19, + suggestions: [ + { + messageId: 'optionalChainSuggest', + output: '(foo1?.foo2 || {}).foo3;', + }, + ], + }, + ], + }, ], }); From a5823d3b6edae464415eb4ead570e8a6a0ff24de Mon Sep 17 00:00:00 2001 From: Omri Luzon Date: Tue, 11 Jan 2022 12:19:36 +0200 Subject: [PATCH 2/9] feat(eslint-plugin): issue 4395 Empty object as optional chaining --- .../src/rules/prefer-optional-chain.ts | 11 +++++-- .../tests/rules/prefer-optional-chain.test.ts | 29 +++++++++++++++++++ 2 files changed, 37 insertions(+), 3 deletions(-) diff --git a/packages/eslint-plugin/src/rules/prefer-optional-chain.ts b/packages/eslint-plugin/src/rules/prefer-optional-chain.ts index 46df13215b6..d2028051824 100644 --- a/packages/eslint-plugin/src/rules/prefer-optional-chain.ts +++ b/packages/eslint-plugin/src/rules/prefer-optional-chain.ts @@ -53,6 +53,7 @@ export default util.createRule({ const sourceCode = context.getSourceCode(); return { 'LogicalExpression[operator=||]'(node: TSESTree.LogicalExpression): void { + const leftNode = node.left; const rightNode = node.right; const parentNode = node.parent; const isRightNodeAnEmptyObjectLiteral = @@ -73,16 +74,20 @@ export default util.createRule({ { messageId: 'optionalChainSuggest', fix: (fixer): TSESLint.RuleFix => { - const leftNodeText = context.getSourceCode().getText(node.left); + const leftNodeText = context.getSourceCode().getText(leftNode); + const maybeWrappedLeftNode = + leftNode.type === AST_NODE_TYPES.LogicalExpression + ? `(${leftNodeText})` + : leftNodeText; const propertyToBeOptionalText = context .getSourceCode() .getText(parentNode.property); - const maybeWrapped = parentNode.computed + const maybeWrappedProperty = parentNode.computed ? `[${propertyToBeOptionalText}]` : propertyToBeOptionalText; return fixer.replaceTextRange( parentNode.range, - `${leftNodeText}?.${maybeWrapped}`, + `${maybeWrappedLeftNode}?.${maybeWrappedProperty}`, ); }, }, diff --git a/packages/eslint-plugin/tests/rules/prefer-optional-chain.test.ts b/packages/eslint-plugin/tests/rules/prefer-optional-chain.test.ts index 34a7abb46c7..1bf9dc6c250 100644 --- a/packages/eslint-plugin/tests/rules/prefer-optional-chain.test.ts +++ b/packages/eslint-plugin/tests/rules/prefer-optional-chain.test.ts @@ -150,6 +150,7 @@ ruleTester.run('prefer-optional-chain', rule, { 'foo || ({} as any);', '(foo || {})?.bar;', // This might seem stupid, but I'm not sure if we want to show suggestion for it '(foo || { bar: 1 }).bar;', + '(undefined && (foo || {})).bar;', 'foo && bar;', 'foo && foo;', 'foo || bar;', @@ -566,5 +567,33 @@ foo?.bar(/* comment */a, }, ], }, + { + code: '(foo || undefined || {}).bar;', + errors: [ + { + messageId: 'optionalChainSuggest', + suggestions: [ + { + messageId: 'optionalChainSuggest', + output: '(foo || undefined)?.bar;', + }, + ], + }, + ], + }, + { + code: noFormat`(undefined && foo || {}).bar;`, + errors: [ + { + messageId: 'optionalChainSuggest', + suggestions: [ + { + messageId: 'optionalChainSuggest', + output: '(undefined && foo)?.bar;', + }, + ], + }, + ], + }, ], }); From aac5eb14e47a2ff30925dbe96d33653c7304eb9f Mon Sep 17 00:00:00 2001 From: Omri Luzon Date: Wed, 19 Jan 2022 21:38:29 +0200 Subject: [PATCH 3/9] fix(eslint-plugin): cr comment const sourceCode already available in the upper scope --- packages/eslint-plugin/src/rules/prefer-optional-chain.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/eslint-plugin/src/rules/prefer-optional-chain.ts b/packages/eslint-plugin/src/rules/prefer-optional-chain.ts index 2cc1f2a37bc..2179af1109b 100644 --- a/packages/eslint-plugin/src/rules/prefer-optional-chain.ts +++ b/packages/eslint-plugin/src/rules/prefer-optional-chain.ts @@ -70,14 +70,14 @@ export default util.createRule({ { messageId: 'optionalChainSuggest', fix: (fixer): TSESLint.RuleFix => { - const leftNodeText = context.getSourceCode().getText(leftNode); + const leftNodeText = sourceCode.getText(leftNode); const maybeWrappedLeftNode = leftNode.type === AST_NODE_TYPES.LogicalExpression ? `(${leftNodeText})` : leftNodeText; - const propertyToBeOptionalText = context - .getSourceCode() - .getText(parentNode.property); + const propertyToBeOptionalText = sourceCode.getText( + parentNode.property, + ); const maybeWrappedProperty = parentNode.computed ? `[${propertyToBeOptionalText}]` : propertyToBeOptionalText; From 155e936278b59383663bf3352525fb3852838e2f Mon Sep 17 00:00:00 2001 From: Omri Luzon Date: Tue, 25 Jan 2022 00:18:27 +0200 Subject: [PATCH 4/9] fix(eslint-plugin): more UT and handle ternary - Wrap left node if it's ternary or await expressions - Early exit if left node can't be a nullish object - Add More UT cases --- .../src/rules/prefer-optional-chain.ts | 11 +- .../tests/rules/prefer-optional-chain.test.ts | 160 +++++++++++++++++- 2 files changed, 169 insertions(+), 2 deletions(-) diff --git a/packages/eslint-plugin/src/rules/prefer-optional-chain.ts b/packages/eslint-plugin/src/rules/prefer-optional-chain.ts index 2179af1109b..6fa09753506 100644 --- a/packages/eslint-plugin/src/rules/prefer-optional-chain.ts +++ b/packages/eslint-plugin/src/rules/prefer-optional-chain.ts @@ -55,7 +55,12 @@ export default util.createRule({ const isRightNodeAnEmptyObjectLiteral = rightNode.type === AST_NODE_TYPES.ObjectExpression && rightNode.properties.length === 0; + // Ignore nodes that evaluate to numbers or booleans + const canLeftNodeBeANullishObject = + leftNode.type !== AST_NODE_TYPES.BinaryExpression && + leftNode.type !== AST_NODE_TYPES.UnaryExpression; if ( + !canLeftNodeBeANullishObject || !isRightNodeAnEmptyObjectLiteral || !parentNode || parentNode.type !== AST_NODE_TYPES.MemberExpression || @@ -71,8 +76,12 @@ export default util.createRule({ messageId: 'optionalChainSuggest', fix: (fixer): TSESLint.RuleFix => { const leftNodeText = sourceCode.getText(leftNode); + // Any node that is made of an operator with higher or equal precedence, + // is reasonable to sometimes evaluate as falsy or an object (maybe not bitwise/binary operators) const maybeWrappedLeftNode = - leftNode.type === AST_NODE_TYPES.LogicalExpression + leftNode.type === AST_NODE_TYPES.LogicalExpression || + leftNode.type === AST_NODE_TYPES.ConditionalExpression || + leftNode.type === AST_NODE_TYPES.AwaitExpression ? `(${leftNodeText})` : leftNodeText; const propertyToBeOptionalText = sourceCode.getText( diff --git a/packages/eslint-plugin/tests/rules/prefer-optional-chain.test.ts b/packages/eslint-plugin/tests/rules/prefer-optional-chain.test.ts index caf7c3da51d..f5d2d2de6e4 100644 --- a/packages/eslint-plugin/tests/rules/prefer-optional-chain.test.ts +++ b/packages/eslint-plugin/tests/rules/prefer-optional-chain.test.ts @@ -148,9 +148,21 @@ ruleTester.run('prefer-optional-chain', rule, { valid: [ 'foo || {};', 'foo || ({} as any);', - '(foo || {})?.bar;', // This might seem stupid, but I'm not sure if we want to show suggestion for it + '(foo || {})?.bar;', '(foo || { bar: 1 }).bar;', '(undefined && (foo || {})).bar;', + 'foo ||= bar;', + 'foo ||= bar || {};', + 'foo ||= bar?.baz;', + 'foo ||= bar?.baz || {};', + 'foo ||= bar?.baz?.buzz;', + '(foo1 ? foo2 : foo3 || {}).foo4;', + '(1 > 2 || {}).bar;', + '(foo = 2 || {}).bar;', + '(foo == 2 || {}).bar;', + '(foo instanceof Number || {}).bar;', + '(typeof a || {}).bar;', + 'func(foo || {}).bar;', 'foo && bar;', 'foo && foo;', 'foo || bar;', @@ -506,6 +518,86 @@ foo?.bar(/* comment */a, }, }, }, + { + code: '(foo || {}).bar;', + errors: [ + { + messageId: 'optionalChainSuggest', + column: 1, + endColumn: 16, + suggestions: [ + { + messageId: 'optionalChainSuggest', + output: 'foo?.bar;', + }, + ], + }, + ], + }, + { + code: noFormat`(foo || ({})).bar;`, + errors: [ + { + messageId: 'optionalChainSuggest', + column: 1, + endColumn: 18, + suggestions: [ + { + messageId: 'optionalChainSuggest', + output: 'foo?.bar;', + }, + ], + }, + ], + }, + { + code: noFormat`(await foo || {}).bar;`, + errors: [ + { + messageId: 'optionalChainSuggest', + column: 1, + endColumn: 22, + suggestions: [ + { + messageId: 'optionalChainSuggest', + output: '(await foo)?.bar;', + }, + ], + }, + ], + }, + { + code: '(foo1?.foo2 || {}).foo3;', + errors: [ + { + messageId: 'optionalChainSuggest', + column: 1, + endColumn: 24, + suggestions: [ + { + messageId: 'optionalChainSuggest', + output: 'foo1?.foo2?.foo3;', + }, + ], + }, + ], + }, + { + code: '((() => foo())() || {}).bar;', + errors: [ + { + messageId: 'optionalChainSuggest', + column: 1, + endColumn: 28, + suggestions: [ + { + messageId: 'optionalChainSuggest', + output: '(() => foo())()?.bar;', + }, + ], + }, + ], + }, { code: 'const foo = (bar || {}).baz;', errors: [ @@ -581,11 +673,77 @@ foo?.bar(/* comment */a, }, ], }, + { + code: '(foo() || bar || {}).baz;', + errors: [ + { + messageId: 'optionalChainSuggest', + column: 1, + endColumn: 25, + suggestions: [ + { + messageId: 'optionalChainSuggest', + output: '(foo() || bar)?.baz;', + }, + ], + }, + ], + }, + { + code: '((foo1 ? foo2 : foo3) || {}).foo4;', + errors: [ + { + messageId: 'optionalChainSuggest', + column: 1, + endColumn: 34, + suggestions: [ + { + messageId: 'optionalChainSuggest', + output: '(foo1 ? foo2 : foo3)?.foo4;', + }, + ], + }, + ], + }, + { + code: noFormat`if (foo) { (foo || {}).bar; }`, + errors: [ + { + messageId: 'optionalChainSuggest', + column: 12, + endColumn: 27, + suggestions: [ + { + messageId: 'optionalChainSuggest', + output: noFormat`if (foo) { foo?.bar; }`, + }, + ], + }, + ], + }, + { + code: noFormat`if ((foo || {}).bar) { foo.bar; }`, + errors: [ + { + messageId: 'optionalChainSuggest', + column: 5, + endColumn: 20, + suggestions: [ + { + messageId: 'optionalChainSuggest', + output: noFormat`if (foo?.bar) { foo.bar; }`, + }, + ], + }, + ], + }, { code: noFormat`(undefined && foo || {}).bar;`, errors: [ { messageId: 'optionalChainSuggest', + column: 1, + endColumn: 29, suggestions: [ { messageId: 'optionalChainSuggest', From 7f907c5ffe3b09b1e5f3307a5457f8af45da5695 Mon Sep 17 00:00:00 2001 From: Omri Luzon Date: Tue, 25 Jan 2022 00:25:29 +0200 Subject: [PATCH 5/9] fix(eslint-plugin): remove comment --- .../eslint-plugin/tests/rules/prefer-optional-chain.test.ts | 2 -- 1 file changed, 2 deletions(-) diff --git a/packages/eslint-plugin/tests/rules/prefer-optional-chain.test.ts b/packages/eslint-plugin/tests/rules/prefer-optional-chain.test.ts index f5d2d2de6e4..42d2693c83f 100644 --- a/packages/eslint-plugin/tests/rules/prefer-optional-chain.test.ts +++ b/packages/eslint-plugin/tests/rules/prefer-optional-chain.test.ts @@ -631,8 +631,6 @@ foo?.bar(/* comment */a, ], }, { - // Currently it does not suggest a fix for nested optional with empty object - // It shows 2 suggestions, one for the outer object and one for the inner object code: '((foo1 || {}).foo2 || {}).foo3;', errors: [ { From d9c81204b3a7b76aa9ded53a7d55f0195f0f00af Mon Sep 17 00:00:00 2001 From: Omri Luzon Date: Wed, 26 Jan 2022 08:27:52 +0200 Subject: [PATCH 6/9] fix(eslint-plugin): prefer optional chaining over `?? {}).` --- .../src/rules/prefer-optional-chain.ts | 4 +- .../tests/rules/prefer-optional-chain.test.ts | 237 ++++++++++++++++++ 2 files changed, 240 insertions(+), 1 deletion(-) diff --git a/packages/eslint-plugin/src/rules/prefer-optional-chain.ts b/packages/eslint-plugin/src/rules/prefer-optional-chain.ts index 6fa09753506..3dc296efa3a 100644 --- a/packages/eslint-plugin/src/rules/prefer-optional-chain.ts +++ b/packages/eslint-plugin/src/rules/prefer-optional-chain.ts @@ -48,7 +48,9 @@ export default util.createRule({ create(context) { const sourceCode = context.getSourceCode(); return { - 'LogicalExpression[operator=||]'(node: TSESTree.LogicalExpression): void { + 'LogicalExpression[operator=||],LogicalExpression[operator=??]'( + node: TSESTree.LogicalExpression, + ): void { const leftNode = node.left; const rightNode = node.right; const parentNode = node.parent; diff --git a/packages/eslint-plugin/tests/rules/prefer-optional-chain.test.ts b/packages/eslint-plugin/tests/rules/prefer-optional-chain.test.ts index 42d2693c83f..1d2e2dad5b0 100644 --- a/packages/eslint-plugin/tests/rules/prefer-optional-chain.test.ts +++ b/packages/eslint-plugin/tests/rules/prefer-optional-chain.test.ts @@ -163,6 +163,10 @@ ruleTester.run('prefer-optional-chain', rule, { '(foo instanceof Number || {}).bar;', '(typeof a || {}).bar;', 'func(foo || {}).bar;', + 'foo ?? {};', + '(foo ?? {})?.bar;', + 'foo ||= bar ?? {};', + '(1 > 2 ?? {}).bar;', 'foo && bar;', 'foo && foo;', 'foo || bar;', @@ -751,5 +755,238 @@ foo?.bar(/* comment */a, }, ], }, + { + code: '(foo ?? {}).bar;', + errors: [ + { + messageId: 'optionalChainSuggest', + column: 1, + endColumn: 16, + suggestions: [ + { + messageId: 'optionalChainSuggest', + output: 'foo?.bar;', + }, + ], + }, + ], + }, + { + code: noFormat`(foo ?? ({})).bar;`, + errors: [ + { + messageId: 'optionalChainSuggest', + column: 1, + endColumn: 18, + suggestions: [ + { + messageId: 'optionalChainSuggest', + output: 'foo?.bar;', + }, + ], + }, + ], + }, + { + code: noFormat`(await foo ?? {}).bar;`, + errors: [ + { + messageId: 'optionalChainSuggest', + column: 1, + endColumn: 22, + suggestions: [ + { + messageId: 'optionalChainSuggest', + output: '(await foo)?.bar;', + }, + ], + }, + ], + }, + { + code: '(foo1?.foo2 ?? {}).foo3;', + errors: [ + { + messageId: 'optionalChainSuggest', + column: 1, + endColumn: 24, + suggestions: [ + { + messageId: 'optionalChainSuggest', + output: 'foo1?.foo2?.foo3;', + }, + ], + }, + ], + }, + { + code: '((() => foo())() ?? {}).bar;', + errors: [ + { + messageId: 'optionalChainSuggest', + column: 1, + endColumn: 28, + suggestions: [ + { + messageId: 'optionalChainSuggest', + output: '(() => foo())()?.bar;', + }, + ], + }, + ], + }, + { + code: 'const foo = (bar ?? {}).baz;', + errors: [ + { + messageId: 'optionalChainSuggest', + column: 13, + endColumn: 28, + suggestions: [ + { + messageId: 'optionalChainSuggest', + output: 'const foo = bar?.baz;', + }, + ], + }, + ], + }, + { + code: '(foo.bar ?? {})[baz];', + errors: [ + { + messageId: 'optionalChainSuggest', + column: 1, + endColumn: 21, + suggestions: [ + { + messageId: 'optionalChainSuggest', + output: 'foo.bar?.[baz];', + }, + ], + }, + ], + }, + { + code: '((foo1 ?? {}).foo2 ?? {}).foo3;', + errors: [ + { + messageId: 'optionalChainSuggest', + column: 1, + endColumn: 31, + suggestions: [ + { + messageId: 'optionalChainSuggest', + output: '(foo1 ?? {}).foo2?.foo3;', + }, + ], + }, + { + messageId: 'optionalChainSuggest', + column: 2, + endColumn: 19, + suggestions: [ + { + messageId: 'optionalChainSuggest', + output: '(foo1?.foo2 ?? {}).foo3;', + }, + ], + }, + ], + }, + { + code: '(foo ?? undefined ?? {}).bar;', + errors: [ + { + messageId: 'optionalChainSuggest', + suggestions: [ + { + messageId: 'optionalChainSuggest', + output: '(foo ?? undefined)?.bar;', + }, + ], + }, + ], + }, + { + code: '(foo() ?? bar ?? {}).baz;', + errors: [ + { + messageId: 'optionalChainSuggest', + column: 1, + endColumn: 25, + suggestions: [ + { + messageId: 'optionalChainSuggest', + output: '(foo() ?? bar)?.baz;', + }, + ], + }, + ], + }, + { + code: '((foo1 ? foo2 : foo3) ?? {}).foo4;', + errors: [ + { + messageId: 'optionalChainSuggest', + column: 1, + endColumn: 34, + suggestions: [ + { + messageId: 'optionalChainSuggest', + output: '(foo1 ? foo2 : foo3)?.foo4;', + }, + ], + }, + ], + }, + { + code: noFormat`if (foo) { (foo ?? {}).bar; }`, + errors: [ + { + messageId: 'optionalChainSuggest', + column: 12, + endColumn: 27, + suggestions: [ + { + messageId: 'optionalChainSuggest', + output: noFormat`if (foo) { foo?.bar; }`, + }, + ], + }, + ], + }, + { + code: noFormat`if ((foo ?? {}).bar) { foo.bar; }`, + errors: [ + { + messageId: 'optionalChainSuggest', + column: 5, + endColumn: 20, + suggestions: [ + { + messageId: 'optionalChainSuggest', + output: noFormat`if (foo?.bar) { foo.bar; }`, + }, + ], + }, + ], + }, + { + code: noFormat`(undefined && foo ?? {}).bar;`, + errors: [ + { + messageId: 'optionalChainSuggest', + column: 1, + endColumn: 29, + suggestions: [ + { + messageId: 'optionalChainSuggest', + output: '(undefined && foo)?.bar;', + }, + ], + }, + ], + }, ], }); From 914d2f9e76715b3819138a3a9891fafb1e610a33 Mon Sep 17 00:00:00 2001 From: Omri Luzon Date: Wed, 9 Feb 2022 23:29:25 +0200 Subject: [PATCH 7/9] Legit nitpick Co-authored-by: Josh Goldberg --- packages/eslint-plugin/src/rules/prefer-optional-chain.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/eslint-plugin/src/rules/prefer-optional-chain.ts b/packages/eslint-plugin/src/rules/prefer-optional-chain.ts index 3dc296efa3a..617ecc872ef 100644 --- a/packages/eslint-plugin/src/rules/prefer-optional-chain.ts +++ b/packages/eslint-plugin/src/rules/prefer-optional-chain.ts @@ -48,7 +48,7 @@ export default util.createRule({ create(context) { const sourceCode = context.getSourceCode(); return { - 'LogicalExpression[operator=||],LogicalExpression[operator=??]'( + 'LogicalExpression[operator="||"], LogicalExpression[operator="??"]'( node: TSESTree.LogicalExpression, ): void { const leftNode = node.left; From d1650b023c2e10e65b028af702094e6edb3dd3ea Mon Sep 17 00:00:00 2001 From: Omri Luzon Date: Thu, 10 Feb 2022 23:51:53 +0200 Subject: [PATCH 8/9] fix(eslint-plugin): prefer optional chaining Catch more cases --- .../src/rules/prefer-optional-chain.ts | 9 +- .../tests/rules/prefer-optional-chain.test.ts | 133 +++++++++++++++++- 2 files changed, 131 insertions(+), 11 deletions(-) diff --git a/packages/eslint-plugin/src/rules/prefer-optional-chain.ts b/packages/eslint-plugin/src/rules/prefer-optional-chain.ts index 617ecc872ef..b554af530a8 100644 --- a/packages/eslint-plugin/src/rules/prefer-optional-chain.ts +++ b/packages/eslint-plugin/src/rules/prefer-optional-chain.ts @@ -57,12 +57,7 @@ export default util.createRule({ const isRightNodeAnEmptyObjectLiteral = rightNode.type === AST_NODE_TYPES.ObjectExpression && rightNode.properties.length === 0; - // Ignore nodes that evaluate to numbers or booleans - const canLeftNodeBeANullishObject = - leftNode.type !== AST_NODE_TYPES.BinaryExpression && - leftNode.type !== AST_NODE_TYPES.UnaryExpression; if ( - !canLeftNodeBeANullishObject || !isRightNodeAnEmptyObjectLiteral || !parentNode || parentNode.type !== AST_NODE_TYPES.MemberExpression || @@ -79,8 +74,10 @@ export default util.createRule({ fix: (fixer): TSESLint.RuleFix => { const leftNodeText = sourceCode.getText(leftNode); // Any node that is made of an operator with higher or equal precedence, - // is reasonable to sometimes evaluate as falsy or an object (maybe not bitwise/binary operators) const maybeWrappedLeftNode = + leftNode.type === AST_NODE_TYPES.BinaryExpression || + leftNode.type === AST_NODE_TYPES.TSAsExpression || + leftNode.type === AST_NODE_TYPES.UnaryExpression || leftNode.type === AST_NODE_TYPES.LogicalExpression || leftNode.type === AST_NODE_TYPES.ConditionalExpression || leftNode.type === AST_NODE_TYPES.AwaitExpression diff --git a/packages/eslint-plugin/tests/rules/prefer-optional-chain.test.ts b/packages/eslint-plugin/tests/rules/prefer-optional-chain.test.ts index 1d2e2dad5b0..bf5aebadc16 100644 --- a/packages/eslint-plugin/tests/rules/prefer-optional-chain.test.ts +++ b/packages/eslint-plugin/tests/rules/prefer-optional-chain.test.ts @@ -157,16 +157,11 @@ ruleTester.run('prefer-optional-chain', rule, { 'foo ||= bar?.baz || {};', 'foo ||= bar?.baz?.buzz;', '(foo1 ? foo2 : foo3 || {}).foo4;', - '(1 > 2 || {}).bar;', '(foo = 2 || {}).bar;', - '(foo == 2 || {}).bar;', - '(foo instanceof Number || {}).bar;', - '(typeof a || {}).bar;', 'func(foo || {}).bar;', 'foo ?? {};', '(foo ?? {})?.bar;', 'foo ||= bar ?? {};', - '(1 > 2 ?? {}).bar;', 'foo && bar;', 'foo && foo;', 'foo || bar;', @@ -988,5 +983,133 @@ foo?.bar(/* comment */a, }, ], }, + { + code: noFormat`(a > b || {}).bar;`, + errors: [ + { + messageId: 'optionalChainSuggest', + column: 1, + endColumn: 18, + suggestions: [ + { + messageId: 'optionalChainSuggest', + output: '(a > b)?.bar;', + }, + ], + }, + ], + }, + { + code: noFormat`(((typeof x) as string) || {}).bar;`, + errors: [ + { + messageId: 'optionalChainSuggest', + column: 1, + endColumn: 35, + suggestions: [ + { + messageId: 'optionalChainSuggest', + output: noFormat`((typeof x) as string)?.bar;`, + }, + ], + }, + ], + }, + { + code: '(void foo() || {}).bar;', + errors: [ + { + messageId: 'optionalChainSuggest', + column: 1, + endColumn: 23, + suggestions: [ + { + messageId: 'optionalChainSuggest', + output: '(void foo())?.bar;', + }, + ], + }, + ], + }, + { + code: '((a ? b : c) || {}).bar;', + errors: [ + { + messageId: 'optionalChainSuggest', + column: 1, + endColumn: 24, + suggestions: [ + { + messageId: 'optionalChainSuggest', + output: '(a ? b : c)?.bar;', + }, + ], + }, + ], + }, + { + code: noFormat`((a instanceof Error) || {}).bar;`, + errors: [ + { + messageId: 'optionalChainSuggest', + column: 1, + endColumn: 33, + suggestions: [ + { + messageId: 'optionalChainSuggest', + output: '(a instanceof Error)?.bar;', + }, + ], + }, + ], + }, + { + code: noFormat`((a << b) || {}).bar;`, + errors: [ + { + messageId: 'optionalChainSuggest', + column: 1, + endColumn: 21, + suggestions: [ + { + messageId: 'optionalChainSuggest', + output: '(a << b)?.bar;', + }, + ], + }, + ], + }, + { + code: noFormat`((foo ** 2) || {}).bar;`, + errors: [ + { + messageId: 'optionalChainSuggest', + column: 1, + endColumn: 23, + suggestions: [ + { + messageId: 'optionalChainSuggest', + output: '(foo ** 2)?.bar;', + }, + ], + }, + ], + }, + { + code: '(foo ** 2 || {}).bar;', + errors: [ + { + messageId: 'optionalChainSuggest', + column: 1, + // endColumn: 23, + suggestions: [ + { + messageId: 'optionalChainSuggest', + output: '(foo ** 2)?.bar;', + }, + ], + }, + ], + }, ], }); From 44236bbb293ec202afbc16180dd82e9a0f4250fe Mon Sep 17 00:00:00 2001 From: Omri Luzon Date: Fri, 18 Mar 2022 22:29:54 +0200 Subject: [PATCH 9/9] feat(eslint-plugin): cr util.getOperatorPrecedence * Use util.getOperatorPrecedence instead of hard coding the precedence --- .../src/rules/prefer-optional-chain.ts | 33 ++++++++++++------ packages/eslint-plugin/src/util/index.ts | 1 + .../tests/rules/prefer-optional-chain.test.ts | 34 ++++++++++++++++++- 3 files changed, 57 insertions(+), 11 deletions(-) diff --git a/packages/eslint-plugin/src/rules/prefer-optional-chain.ts b/packages/eslint-plugin/src/rules/prefer-optional-chain.ts index b554af530a8..b3815e09ef9 100644 --- a/packages/eslint-plugin/src/rules/prefer-optional-chain.ts +++ b/packages/eslint-plugin/src/rules/prefer-optional-chain.ts @@ -1,5 +1,7 @@ -import { AST_NODE_TYPES, TSESTree, TSESLint } from '@typescript-eslint/utils'; +import * as ts from 'typescript'; import * as util from '../util'; +import { AST_NODE_TYPES, TSESTree, TSESLint } from '@typescript-eslint/utils'; +import { isBinaryExpression } from 'tsutils'; type ValidChainTarget = | TSESTree.BinaryExpression @@ -47,6 +49,8 @@ export default util.createRule({ defaultOptions: [], create(context) { const sourceCode = context.getSourceCode(); + const parserServices = util.getParserServices(context, true); + return { 'LogicalExpression[operator="||"], LogicalExpression[operator="??"]'( node: TSESTree.LogicalExpression, @@ -65,6 +69,21 @@ export default util.createRule({ ) { return; } + + function isLeftSideLowerPrecedence(): boolean { + const logicalTsNode = parserServices.esTreeNodeToTSNodeMap.get(node); + + const leftTsNode = parserServices.esTreeNodeToTSNodeMap.get(leftNode); + const operator = isBinaryExpression(logicalTsNode) + ? logicalTsNode.operatorToken.kind + : ts.SyntaxKind.Unknown; + const leftPrecedence = util.getOperatorPrecedence( + leftTsNode.kind, + operator, + ); + + return leftPrecedence < util.OperatorPrecedence.LeftHandSide; + } context.report({ node: parentNode, messageId: 'optionalChainSuggest', @@ -74,15 +93,9 @@ export default util.createRule({ fix: (fixer): TSESLint.RuleFix => { const leftNodeText = sourceCode.getText(leftNode); // Any node that is made of an operator with higher or equal precedence, - const maybeWrappedLeftNode = - leftNode.type === AST_NODE_TYPES.BinaryExpression || - leftNode.type === AST_NODE_TYPES.TSAsExpression || - leftNode.type === AST_NODE_TYPES.UnaryExpression || - leftNode.type === AST_NODE_TYPES.LogicalExpression || - leftNode.type === AST_NODE_TYPES.ConditionalExpression || - leftNode.type === AST_NODE_TYPES.AwaitExpression - ? `(${leftNodeText})` - : leftNodeText; + const maybeWrappedLeftNode = isLeftSideLowerPrecedence() + ? `(${leftNodeText})` + : leftNodeText; const propertyToBeOptionalText = sourceCode.getText( parentNode.property, ); diff --git a/packages/eslint-plugin/src/util/index.ts b/packages/eslint-plugin/src/util/index.ts index a83198807a6..b2932466388 100644 --- a/packages/eslint-plugin/src/util/index.ts +++ b/packages/eslint-plugin/src/util/index.ts @@ -4,6 +4,7 @@ export * from './astUtils'; export * from './collectUnusedVariables'; export * from './createRule'; export * from './getFunctionHeadLoc'; +export * from './getOperatorPrecedence'; export * from './getThisExpression'; export * from './getWrappingFixer'; export * from './misc'; diff --git a/packages/eslint-plugin/tests/rules/prefer-optional-chain.test.ts b/packages/eslint-plugin/tests/rules/prefer-optional-chain.test.ts index bf5aebadc16..2b004e9d7a9 100644 --- a/packages/eslint-plugin/tests/rules/prefer-optional-chain.test.ts +++ b/packages/eslint-plugin/tests/rules/prefer-optional-chain.test.ts @@ -1101,7 +1101,7 @@ foo?.bar(/* comment */a, { messageId: 'optionalChainSuggest', column: 1, - // endColumn: 23, + endColumn: 21, suggestions: [ { messageId: 'optionalChainSuggest', @@ -1111,5 +1111,37 @@ foo?.bar(/* comment */a, }, ], }, + { + code: '(foo++ || {}).bar;', + errors: [ + { + messageId: 'optionalChainSuggest', + column: 1, + endColumn: 18, + suggestions: [ + { + messageId: 'optionalChainSuggest', + output: '(foo++)?.bar;', + }, + ], + }, + ], + }, + { + code: '(+foo || {}).bar;', + errors: [ + { + messageId: 'optionalChainSuggest', + column: 1, + endColumn: 17, + suggestions: [ + { + messageId: 'optionalChainSuggest', + output: '(+foo)?.bar;', + }, + ], + }, + ], + }, ], });