From fd1c08de9246563882319e6dc2e8f4047d5745d3 Mon Sep 17 00:00:00 2001 From: Omri Luzon Date: Sat, 4 May 2024 21:48:49 +0300 Subject: [PATCH 01/38] feat(eslint-plugin): [prefer-optional-chain] support if statement as part of chain (issue-6309) --- .../PreferOptionalChainOptions.ts | 1 + .../analyzeChain.ts | 10 +- .../src/rules/prefer-optional-chain.ts | 86 ++++++++ .../prefer-optional-chain.test.ts | 192 +++++++++++++++--- 4 files changed, 256 insertions(+), 33 deletions(-) diff --git a/packages/eslint-plugin/src/rules/prefer-optional-chain-utils/PreferOptionalChainOptions.ts b/packages/eslint-plugin/src/rules/prefer-optional-chain-utils/PreferOptionalChainOptions.ts index de1147b54447..07c01ae68605 100644 --- a/packages/eslint-plugin/src/rules/prefer-optional-chain-utils/PreferOptionalChainOptions.ts +++ b/packages/eslint-plugin/src/rules/prefer-optional-chain-utils/PreferOptionalChainOptions.ts @@ -4,6 +4,7 @@ export type PreferOptionalChainMessageIds = export interface PreferOptionalChainOptions { allowPotentiallyUnsafeFixesThatModifyTheReturnTypeIKnowWhatImDoing?: boolean; + allowSuggestingOnIfStatements?: boolean; checkAny?: boolean; checkBigInt?: boolean; checkBoolean?: boolean; diff --git a/packages/eslint-plugin/src/rules/prefer-optional-chain-utils/analyzeChain.ts b/packages/eslint-plugin/src/rules/prefer-optional-chain-utils/analyzeChain.ts index cd4894cc384c..5e9257682b34 100644 --- a/packages/eslint-plugin/src/rules/prefer-optional-chain-utils/analyzeChain.ts +++ b/packages/eslint-plugin/src/rules/prefer-optional-chain-utils/analyzeChain.ts @@ -208,9 +208,13 @@ const analyzeOrChainOperand: OperandAnalyzer = ( */ function getReportRange( chain: ValidOperand[], - boundary: TSESTree.Range, + node: TSESTree.Node, sourceCode: SourceCode, ): TSESTree.Range { + if (node.type === AST_NODE_TYPES.IfStatement) { + return node.range; + } + const boundary = node.range; const leftNode = chain[0].node; const rightNode = chain[chain.length - 1].node; let leftMost = nullThrows( @@ -324,7 +328,7 @@ function getReportDescriptor( // 6) diff(`foo.bar.baz?.bam`, `foo.bar.baz.bam()`) = `()` // 7) result = `foo?.bar?.baz?.bam?.()` - const parts = []; + const parts: FlattenedChain[] = []; for (const current of chain) { const nextOperand = flattenChainExpression( sourceCode, @@ -401,7 +405,7 @@ function getReportDescriptor( newCode = `!${newCode}`; } - const reportRange = getReportRange(chain, node.range, sourceCode); + const reportRange = getReportRange(chain, node, sourceCode); const fix: ReportFixFunction = fixer => fixer.replaceTextRange(reportRange, newCode); diff --git a/packages/eslint-plugin/src/rules/prefer-optional-chain.ts b/packages/eslint-plugin/src/rules/prefer-optional-chain.ts index ffbcd213fedd..2b25df185f70 100644 --- a/packages/eslint-plugin/src/rules/prefer-optional-chain.ts +++ b/packages/eslint-plugin/src/rules/prefer-optional-chain.ts @@ -19,6 +19,7 @@ import { analyzeChain } from './prefer-optional-chain-utils/analyzeChain'; import { checkNullishAndReport } from './prefer-optional-chain-utils/checkNullishAndReport'; import { gatherLogicalOperands, + NullishComparisonType, OperandValidity, } from './prefer-optional-chain-utils/gatherLogicalOperands'; @@ -52,6 +53,11 @@ export default createRule< description: 'Allow autofixers that will change the return type of the expression. This option is considered unsafe as it may break the build.', }, + allowSuggestingOnIfStatements: { + type: 'boolean', + description: + 'Allow suggesting optional chain on if statements with a single expression in the consequent.', + }, checkAny: { type: 'boolean', description: @@ -94,6 +100,7 @@ export default createRule< defaultOptions: [ { allowPotentiallyUnsafeFixesThatModifyTheReturnTypeIKnowWhatImDoing: false, + allowSuggestingOnIfStatements: false, checkAny: true, checkBigInt: true, checkBoolean: true, @@ -110,6 +117,85 @@ export default createRule< return { // specific handling for `(foo ?? {}).bar` / `(foo || {}).bar` + 'IfStatement[consequent.body.length=1][consequent.type=BlockStatement]': ( + node: { + consequent: { type: AST_NODE_TYPES.BlockStatement }; + } & TSESTree.IfStatement, + ): void => { + if (!options.allowSuggestingOnIfStatements) { + return; + } + if (node.alternate) { + return; + } + if ( + node.test.type !== AST_NODE_TYPES.LogicalExpression && + node.test.type !== AST_NODE_TYPES.Identifier && + node.test.type !== AST_NODE_TYPES.MemberExpression && + node.test.type !== AST_NODE_TYPES.CallExpression + ) { + return; + } + const ifBodyStatement = node.consequent.body[0]; + const hasCommentsInIfBody = + context.sourceCode.getCommentsBefore(ifBodyStatement).length || + context.sourceCode.getCommentsAfter(ifBodyStatement).length; + + if (hasCommentsInIfBody) { + return; + } + + if (ifBodyStatement.type !== AST_NODE_TYPES.ExpressionStatement) { + return; + } + const ifBodyExpression = ifBodyStatement.expression; + if (ifBodyExpression.type !== AST_NODE_TYPES.CallExpression) { + return; + } + const currentChain: ValidOperand[] = [ + { + node: node.test, + type: OperandValidity.Valid, + comparedName: node.test, + comparisonType: NullishComparisonType.Boolean, + isYoda: false, + }, + ]; + if (node.test.type === AST_NODE_TYPES.LogicalExpression) { + const { newlySeenLogicals, operands } = gatherLogicalOperands( + node.test, + parserServices, + context.sourceCode, + options, + ); + for (const logical of newlySeenLogicals) { + seenLogicals.add(logical); + } + for (const operand of operands) { + if (operand.type === OperandValidity.Invalid) { + return; + } + currentChain.push(operand); + } + } + currentChain.push({ + node: ifBodyExpression, + type: OperandValidity.Valid, + comparedName: ifBodyExpression, + comparisonType: NullishComparisonType.Boolean, + isYoda: false, + }); + analyzeChain( + context, + parserServices, + options, + node, + '&&', + currentChain, + ); + return; + }, + 'LogicalExpression[operator!="??"]'( node: TSESTree.LogicalExpression, ): void { diff --git a/packages/eslint-plugin/tests/rules/prefer-optional-chain/prefer-optional-chain.test.ts b/packages/eslint-plugin/tests/rules/prefer-optional-chain/prefer-optional-chain.test.ts index c05f6218b24c..1723e07e8113 100644 --- a/packages/eslint-plugin/tests/rules/prefer-optional-chain/prefer-optional-chain.test.ts +++ b/packages/eslint-plugin/tests/rules/prefer-optional-chain/prefer-optional-chain.test.ts @@ -665,35 +665,138 @@ describe('|| {}', () => { 'foo ?? {};', '(foo ?? {})?.bar;', 'foo ||= bar ?? {};', - // https://github.com/typescript-eslint/typescript-eslint/issues/8380 - ` - const a = null; - const b = 0; - a === undefined || b === null || b === undefined; - `, - // https://github.com/typescript-eslint/typescript-eslint/issues/8380 - ` - const a = 0; - const b = 0; - a === undefined || b === undefined || b === null; - `, - // https://github.com/typescript-eslint/typescript-eslint/issues/8380 - ` - const a = 0; - const b = 0; - b === null || a === undefined || b === undefined; - `, - // https://github.com/typescript-eslint/typescript-eslint/issues/8380 - ` - const b = 0; - b === null || b === undefined; - `, - // https://github.com/typescript-eslint/typescript-eslint/issues/8380 - ` - const a = 0; - const b = 0; - b != null && a !== null && a !== undefined; - `, + ], + }); +}); + +describe('if block with a single statment matches part of the condition', () => { + ruleTester.run('prefer-optional-chain', rule, { + invalid: [ + { + code: noFormat`if (foo) { foo.bar(); }`, + errors: [ + { + messageId: 'preferOptionalChain', + suggestions: null, + }, + ], + options: [{ allowSuggestingOnIfStatements: true }], + output: 'foo?.bar()', + }, + { + code: noFormat`if (foo) { foo(); }`, + errors: [ + { + messageId: 'preferOptionalChain', + suggestions: null, + }, + ], + options: [{ allowSuggestingOnIfStatements: true }], + output: 'foo?.()', + }, + { + code: noFormat`if (foo) { foo[bar](); }`, + errors: [ + { + messageId: 'preferOptionalChain', + suggestions: null, + }, + ], + options: [{ allowSuggestingOnIfStatements: true }], + output: 'foo?.[bar]()', + }, + { + code: noFormat`if (foo[bar]) { foo[bar].baz(); }`, + errors: [ + { + messageId: 'preferOptionalChain', + suggestions: null, + }, + ], + options: [{ allowSuggestingOnIfStatements: true }], + output: 'foo[bar]?.baz()', + }, + { + code: noFormat`if (foo.bar.baz()) { foo.bar.baz().bazz(); }`, + errors: [ + { + messageId: 'preferOptionalChain', + suggestions: null, + }, + ], + options: [{ allowSuggestingOnIfStatements: true }], + output: 'foo.bar.baz()?.bazz()', + }, + { + code: ` +declare const foo: undefined | { bar?: { baz?: { bazz: () => void } } }; +if (foo && foo.bar && foo.bar.baz) { + foo.bar.baz.bazz(); +} + `, + errors: [ + { + messageId: 'preferOptionalChain', + suggestions: null, + }, + ], + options: [{ allowSuggestingOnIfStatements: true }], + output: ` +declare const foo: undefined | { bar?: { baz?: { bazz: () => void } } }; +foo?.bar?.baz?.bazz() + `, + }, + ], + valid: [ + // Ignore no calls + { + code: noFormat`if (foo) { foo.bar; }`, + options: [{ allowSuggestingOnIfStatements: true }], + }, + { + code: noFormat`if (foo) { foo.bar?.baz; }`, + options: [{ allowSuggestingOnIfStatements: true }], + }, + // Ignore when comment exists before or after statement inside consequent + { + code: noFormat`if (foo) { /* comment */ foo.bar(); }`, + options: [{ allowSuggestingOnIfStatements: true }], + }, + { + code: noFormat`if (foo) { foo.bar(); /* comment */ }`, + options: [{ allowSuggestingOnIfStatements: true }], + }, + // Ignore when multiple statements exist - only single call expression statement allowed + { + code: ` + if (foo) { + foo.bar(); + foo.baz(); + } + `, + options: [{ allowSuggestingOnIfStatements: true }], + }, + // Ignore when else is used + { + code: ` + declare const x: null | { a: () => string }; + if (x) { + x.a(); + } else { + // do something else + } + `, + options: [{ allowSuggestingOnIfStatements: true }], + }, + { + code: ` + declare const x: null | { a: () => string }; + if (globalThis) { + x.a(); + } + `, + options: [{ allowSuggestingOnIfStatements: true }], + }, ], }); }); @@ -1708,7 +1811,7 @@ describe('hand-crafted cases', () => { 'foo === 1 && foo.toFixed();', // call arguments are considered 'foo.bar(a) && foo.bar(a, b).baz;', - // type parameters are considered + // type arguments are considered 'foo.bar() && foo.bar().baz;', // array elements are considered '[1, 2].length && [1, 2, 3].length.toFixed();', @@ -1917,6 +2020,35 @@ describe('hand-crafted cases', () => { !x || x.a; `, "typeof globalThis !== 'undefined' && globalThis.Array();", + // https://github.com/typescript-eslint/typescript-eslint/issues/8380 + ` + const a = null; + const b = 0; + a === undefined || b === null || b === undefined; + `, + // https://github.com/typescript-eslint/typescript-eslint/issues/8380 + ` + const a = 0; + const b = 0; + a === undefined || b === undefined || b === null; + `, + // https://github.com/typescript-eslint/typescript-eslint/issues/8380 + ` + const a = 0; + const b = 0; + b === null || a === undefined || b === undefined; + `, + // https://github.com/typescript-eslint/typescript-eslint/issues/8380 + ` + const b = 0; + b === null || b === undefined; + `, + // https://github.com/typescript-eslint/typescript-eslint/issues/8380 + ` + const a = 0; + const b = 0; + b != null && a !== null && a !== undefined; + `, ], }); }); From 387a3ab2db699d441663a447d194df438a90b163 Mon Sep 17 00:00:00 2001 From: Omri Luzon Date: Sat, 12 Oct 2024 14:35:16 +0300 Subject: [PATCH 02/38] Add docs mdx --- .../docs/rules/prefer-optional-chain.mdx | 24 +++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/packages/eslint-plugin/docs/rules/prefer-optional-chain.mdx b/packages/eslint-plugin/docs/rules/prefer-optional-chain.mdx index ee7aa6abf42f..a6f709a2e495 100644 --- a/packages/eslint-plugin/docs/rules/prefer-optional-chain.mdx +++ b/packages/eslint-plugin/docs/rules/prefer-optional-chain.mdx @@ -66,6 +66,30 @@ foo?.a?.b?.c?.d?.e; In the context of the descriptions below a "loose boolean" operand is any operand that implicitly coerces the value to a boolean. Specifically the argument of the not operator (`!loose`) or a bare value in a logical expression (`loose && looser`). +### allowSuggestingOnIfStatements + +When this option is `true`, the rule will analyze and suggest turning simple `if` statements with a single block expression into an optional chain call + + + + + +```ts option='{ "allowSuggestingOnIfStatements": true }' +if (foo) { + foo.bar(); +} +``` + + + + +```ts option='{ "allowSuggestingOnIfStatements": false }' +foo?.bar(); +``` + + + + ### `allowPotentiallyUnsafeFixesThatModifyTheReturnTypeIKnowWhatImDoing` When this option is `true`, the rule will provide an auto-fixer for cases where the return type of the expression would change. For example for the expression `!foo || foo.bar` the return type of the expression is `true | T`, however for the equivalent optional chain `foo?.bar` the return type of the expression is `undefined | T`. Thus changing the code from a logical expression to an optional chain expression has altered the type of the expression. From a21c19721fd213fb0b81c76ef33789a8bbcae772 Mon Sep 17 00:00:00 2001 From: Omri Luzon Date: Sat, 12 Oct 2024 15:31:18 +0300 Subject: [PATCH 03/38] Order matters + snapshots --- .../docs/rules/prefer-optional-chain.mdx | 44 ++++++++--------- .../prefer-optional-chain.shot | 47 ++++++++++++++----- .../prefer-optional-chain.shot | 6 +++ 3 files changed, 62 insertions(+), 35 deletions(-) diff --git a/packages/eslint-plugin/docs/rules/prefer-optional-chain.mdx b/packages/eslint-plugin/docs/rules/prefer-optional-chain.mdx index a6f709a2e495..2d277efeda9c 100644 --- a/packages/eslint-plugin/docs/rules/prefer-optional-chain.mdx +++ b/packages/eslint-plugin/docs/rules/prefer-optional-chain.mdx @@ -66,7 +66,28 @@ foo?.a?.b?.c?.d?.e; In the context of the descriptions below a "loose boolean" operand is any operand that implicitly coerces the value to a boolean. Specifically the argument of the not operator (`!loose`) or a bare value in a logical expression (`loose && looser`). -### allowSuggestingOnIfStatements +### `allowPotentiallyUnsafeFixesThatModifyTheReturnTypeIKnowWhatImDoing` + +When this option is `true`, the rule will provide an auto-fixer for cases where the return type of the expression would change. For example for the expression `!foo || foo.bar` the return type of the expression is `true | T`, however for the equivalent optional chain `foo?.bar` the return type of the expression is `undefined | T`. Thus changing the code from a logical expression to an optional chain expression has altered the type of the expression. + +In some cases this distinction _may_ matter - which is why these fixers are considered unsafe - they may break the build! For example in the following code: + +```ts option='{ "allowPotentiallyUnsafeFixesThatModifyTheReturnTypeIKnowWhatImDoing": true }' showPlaygroundButton +declare const foo: { bar: boolean } | null | undefined; +declare function acceptsBoolean(arg: boolean): void; + +// ✅ typechecks succesfully as the expression only returns `boolean` +acceptsBoolean(foo != null && foo.bar); + +// ❌ typechecks UNSUCCESSFULLY as the expression returns `boolean | undefined` +acceptsBoolean(foo?.bar); +``` + +This style of code isn't super common - which means having this option set to `true` _should_ be safe in most codebases. However we default it to `false` due to its unsafe nature. We have provided this option for convenience because it increases the autofix cases covered by the rule. If you set option to `true` the onus is entirely on you and your team to ensure that each fix is correct and safe and that it does not break the build. + +When this option is `false` unsafe cases will have suggestion fixers provided instead of auto-fixers - meaning you can manually apply the fix using your IDE tooling. + +### `allowSuggestingOnIfStatements` When this option is `true`, the rule will analyze and suggest turning simple `if` statements with a single block expression into an optional chain call @@ -90,27 +111,6 @@ foo?.bar(); -### `allowPotentiallyUnsafeFixesThatModifyTheReturnTypeIKnowWhatImDoing` - -When this option is `true`, the rule will provide an auto-fixer for cases where the return type of the expression would change. For example for the expression `!foo || foo.bar` the return type of the expression is `true | T`, however for the equivalent optional chain `foo?.bar` the return type of the expression is `undefined | T`. Thus changing the code from a logical expression to an optional chain expression has altered the type of the expression. - -In some cases this distinction _may_ matter - which is why these fixers are considered unsafe - they may break the build! For example in the following code: - -```ts option='{ "allowPotentiallyUnsafeFixesThatModifyTheReturnTypeIKnowWhatImDoing": true }' showPlaygroundButton -declare const foo: { bar: boolean } | null | undefined; -declare function acceptsBoolean(arg: boolean): void; - -// ✅ typechecks succesfully as the expression only returns `boolean` -acceptsBoolean(foo != null && foo.bar); - -// ❌ typechecks UNSUCCESSFULLY as the expression returns `boolean | undefined` -acceptsBoolean(foo?.bar); -``` - -This style of code isn't super common - which means having this option set to `true` _should_ be safe in most codebases. However we default it to `false` due to its unsafe nature. We have provided this option for convenience because it increases the autofix cases covered by the rule. If you set option to `true` the onus is entirely on you and your team to ensure that each fix is correct and safe and that it does not break the build. - -When this option is `false` unsafe cases will have suggestion fixers provided instead of auto-fixers - meaning you can manually apply the fix using your IDE tooling. - ### `checkAny` When this option is `true` the rule will check operands that are typed as `any` when inspecting "loose boolean" operands. diff --git a/packages/eslint-plugin/tests/docs-eslint-output-snapshots/prefer-optional-chain.shot b/packages/eslint-plugin/tests/docs-eslint-output-snapshots/prefer-optional-chain.shot index 980b3f8058df..b7181d30833c 100644 --- a/packages/eslint-plugin/tests/docs-eslint-output-snapshots/prefer-optional-chain.shot +++ b/packages/eslint-plugin/tests/docs-eslint-output-snapshots/prefer-optional-chain.shot @@ -72,6 +72,27 @@ acceptsBoolean(foo?.bar); exports[`Validating rule docs prefer-optional-chain.mdx code examples ESLint output 4`] = ` "Incorrect +Options: { "allowSuggestingOnIfStatements": true } + +if (foo) { +~~~~~~~~~~ Prefer using an optional chain expression instead, as it's more concise and easier to read. + foo.bar(); +~~~~~~~~~~~~ +} +~ +" +`; + +exports[`Validating rule docs prefer-optional-chain.mdx code examples ESLint output 5`] = ` +"Correct +Options: { "allowSuggestingOnIfStatements": false } + +foo?.bar(); +" +`; + +exports[`Validating rule docs prefer-optional-chain.mdx code examples ESLint output 6`] = ` +"Incorrect Options: { "checkAny": true } declare const thing: any; @@ -81,7 +102,7 @@ thing && thing.toString(); " `; -exports[`Validating rule docs prefer-optional-chain.mdx code examples ESLint output 5`] = ` +exports[`Validating rule docs prefer-optional-chain.mdx code examples ESLint output 7`] = ` "Correct Options: { "checkAny": false } @@ -91,7 +112,7 @@ thing && thing.toString(); " `; -exports[`Validating rule docs prefer-optional-chain.mdx code examples ESLint output 6`] = ` +exports[`Validating rule docs prefer-optional-chain.mdx code examples ESLint output 8`] = ` "Incorrect Options: { "checkUnknown": true } @@ -102,7 +123,7 @@ thing && thing.toString(); " `; -exports[`Validating rule docs prefer-optional-chain.mdx code examples ESLint output 7`] = ` +exports[`Validating rule docs prefer-optional-chain.mdx code examples ESLint output 9`] = ` "Correct Options: { "checkUnknown": false } @@ -112,7 +133,7 @@ thing && thing.toString(); " `; -exports[`Validating rule docs prefer-optional-chain.mdx code examples ESLint output 8`] = ` +exports[`Validating rule docs prefer-optional-chain.mdx code examples ESLint output 10`] = ` "Incorrect Options: { "checkString": true } @@ -123,7 +144,7 @@ thing && thing.toString(); " `; -exports[`Validating rule docs prefer-optional-chain.mdx code examples ESLint output 9`] = ` +exports[`Validating rule docs prefer-optional-chain.mdx code examples ESLint output 11`] = ` "Correct Options: { "checkString": false } @@ -133,7 +154,7 @@ thing && thing.toString(); " `; -exports[`Validating rule docs prefer-optional-chain.mdx code examples ESLint output 10`] = ` +exports[`Validating rule docs prefer-optional-chain.mdx code examples ESLint output 12`] = ` "Incorrect Options: { "checkNumber": true } @@ -144,7 +165,7 @@ thing && thing.toString(); " `; -exports[`Validating rule docs prefer-optional-chain.mdx code examples ESLint output 11`] = ` +exports[`Validating rule docs prefer-optional-chain.mdx code examples ESLint output 13`] = ` "Correct Options: { "checkNumber": false } @@ -154,7 +175,7 @@ thing && thing.toString(); " `; -exports[`Validating rule docs prefer-optional-chain.mdx code examples ESLint output 12`] = ` +exports[`Validating rule docs prefer-optional-chain.mdx code examples ESLint output 14`] = ` "Incorrect Options: { "checkBoolean": true } @@ -165,7 +186,7 @@ thing && thing.toString(); " `; -exports[`Validating rule docs prefer-optional-chain.mdx code examples ESLint output 13`] = ` +exports[`Validating rule docs prefer-optional-chain.mdx code examples ESLint output 15`] = ` "Correct Options: { "checkBoolean": false } @@ -175,7 +196,7 @@ thing && thing.toString(); " `; -exports[`Validating rule docs prefer-optional-chain.mdx code examples ESLint output 14`] = ` +exports[`Validating rule docs prefer-optional-chain.mdx code examples ESLint output 16`] = ` "Incorrect Options: { "checkBigInt": true } @@ -186,7 +207,7 @@ thing && thing.toString(); " `; -exports[`Validating rule docs prefer-optional-chain.mdx code examples ESLint output 15`] = ` +exports[`Validating rule docs prefer-optional-chain.mdx code examples ESLint output 17`] = ` "Correct Options: { "checkBigInt": false } @@ -196,7 +217,7 @@ thing && thing.toString(); " `; -exports[`Validating rule docs prefer-optional-chain.mdx code examples ESLint output 16`] = ` +exports[`Validating rule docs prefer-optional-chain.mdx code examples ESLint output 18`] = ` "Incorrect Options: { "requireNullish": true } @@ -206,7 +227,7 @@ thing1 && thing1.toString(); " `; -exports[`Validating rule docs prefer-optional-chain.mdx code examples ESLint output 17`] = ` +exports[`Validating rule docs prefer-optional-chain.mdx code examples ESLint output 19`] = ` "Correct Options: { "requireNullish": true } diff --git a/packages/eslint-plugin/tests/schema-snapshots/prefer-optional-chain.shot b/packages/eslint-plugin/tests/schema-snapshots/prefer-optional-chain.shot index d46855fc7ca8..55c41d5073b6 100644 --- a/packages/eslint-plugin/tests/schema-snapshots/prefer-optional-chain.shot +++ b/packages/eslint-plugin/tests/schema-snapshots/prefer-optional-chain.shot @@ -12,6 +12,10 @@ exports[`Rule schemas should be convertible to TS types for documentation purpos "description": "Allow autofixers that will change the return type of the expression. This option is considered unsafe as it may break the build.", "type": "boolean" }, + "allowSuggestingOnIfStatements": { + "description": "Allow suggesting optional chain on if statements with a single expression in the consequent.", + "type": "boolean" + }, "checkAny": { "description": "Check operands that are typed as \`any\` when inspecting \\"loose boolean\\" operands.", "type": "boolean" @@ -52,6 +56,8 @@ type Options = [ { /** Allow autofixers that will change the return type of the expression. This option is considered unsafe as it may break the build. */ allowPotentiallyUnsafeFixesThatModifyTheReturnTypeIKnowWhatImDoing?: boolean; + /** Allow suggesting optional chain on if statements with a single expression in the consequent. */ + allowSuggestingOnIfStatements?: boolean; /** Check operands that are typed as \`any\` when inspecting "loose boolean" operands. */ checkAny?: boolean; /** Check operands that are typed as \`bigint\` when inspecting "loose boolean" operands. */ From eaa203f9d49812fc72e9e28e03f086b2bde200d7 Mon Sep 17 00:00:00 2001 From: Omri Luzon Date: Sat, 12 Oct 2024 15:52:00 +0300 Subject: [PATCH 04/38] Handle semi colons --- .../analyzeChain.ts | 9 ++++++++ .../prefer-optional-chain.test.ts | 23 ++++++++++++++----- 2 files changed, 26 insertions(+), 6 deletions(-) diff --git a/packages/eslint-plugin/src/rules/prefer-optional-chain-utils/analyzeChain.ts b/packages/eslint-plugin/src/rules/prefer-optional-chain-utils/analyzeChain.ts index 5e9257682b34..be0955f5104d 100644 --- a/packages/eslint-plugin/src/rules/prefer-optional-chain-utils/analyzeChain.ts +++ b/packages/eslint-plugin/src/rules/prefer-optional-chain-utils/analyzeChain.ts @@ -407,6 +407,15 @@ function getReportDescriptor( const reportRange = getReportRange(chain, node, sourceCode); + if (node.type === AST_NODE_TYPES.IfStatement) { + const lastChainNode = chain.at(-1)?.node; + const chainEndedWithSemicolon = + lastChainNode && sourceCode.getTokenAfter(lastChainNode)?.value === ';'; + if (chainEndedWithSemicolon) { + newCode += ';'; + } + } + const fix: ReportFixFunction = fixer => fixer.replaceTextRange(reportRange, newCode); diff --git a/packages/eslint-plugin/tests/rules/prefer-optional-chain/prefer-optional-chain.test.ts b/packages/eslint-plugin/tests/rules/prefer-optional-chain/prefer-optional-chain.test.ts index 1723e07e8113..d015a822844e 100644 --- a/packages/eslint-plugin/tests/rules/prefer-optional-chain/prefer-optional-chain.test.ts +++ b/packages/eslint-plugin/tests/rules/prefer-optional-chain/prefer-optional-chain.test.ts @@ -673,7 +673,7 @@ describe('if block with a single statment matches part of the condition', () => ruleTester.run('prefer-optional-chain', rule, { invalid: [ { - code: noFormat`if (foo) { foo.bar(); }`, + code: noFormat`if (foo) { foo.bar() }`, // Missing semi-colon errors: [ { messageId: 'preferOptionalChain', @@ -683,6 +683,17 @@ describe('if block with a single statment matches part of the condition', () => options: [{ allowSuggestingOnIfStatements: true }], output: 'foo?.bar()', }, + { + code: noFormat`if (foo) { foo.bar(); }`, + errors: [ + { + messageId: 'preferOptionalChain', + suggestions: null, + }, + ], + options: [{ allowSuggestingOnIfStatements: true }], + output: 'foo?.bar();', + }, { code: noFormat`if (foo) { foo(); }`, errors: [ @@ -692,7 +703,7 @@ describe('if block with a single statment matches part of the condition', () => }, ], options: [{ allowSuggestingOnIfStatements: true }], - output: 'foo?.()', + output: 'foo?.();', }, { code: noFormat`if (foo) { foo[bar](); }`, @@ -703,7 +714,7 @@ describe('if block with a single statment matches part of the condition', () => }, ], options: [{ allowSuggestingOnIfStatements: true }], - output: 'foo?.[bar]()', + output: 'foo?.[bar]();', }, { code: noFormat`if (foo[bar]) { foo[bar].baz(); }`, @@ -714,7 +725,7 @@ describe('if block with a single statment matches part of the condition', () => }, ], options: [{ allowSuggestingOnIfStatements: true }], - output: 'foo[bar]?.baz()', + output: 'foo[bar]?.baz();', }, { code: noFormat`if (foo.bar.baz()) { foo.bar.baz().bazz(); }`, @@ -725,7 +736,7 @@ describe('if block with a single statment matches part of the condition', () => }, ], options: [{ allowSuggestingOnIfStatements: true }], - output: 'foo.bar.baz()?.bazz()', + output: 'foo.bar.baz()?.bazz();', }, { code: ` @@ -743,7 +754,7 @@ if (foo && foo.bar && foo.bar.baz) { options: [{ allowSuggestingOnIfStatements: true }], output: ` declare const foo: undefined | { bar?: { baz?: { bazz: () => void } } }; -foo?.bar?.baz?.bazz() +foo?.bar?.baz?.bazz(); `, }, ], From a7258b8868d65f13290ce5f4bec1656a7220bf63 Mon Sep 17 00:00:00 2001 From: Omri Luzon Date: Sat, 12 Oct 2024 16:31:22 +0300 Subject: [PATCH 05/38] Coverage --- .../src/rules/prefer-optional-chain.ts | 15 ++++-------- .../prefer-optional-chain.test.ts | 23 +++++++++++++++++++ 2 files changed, 27 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 2b25df185f70..c2235efa155b 100644 --- a/packages/eslint-plugin/src/rules/prefer-optional-chain.ts +++ b/packages/eslint-plugin/src/rules/prefer-optional-chain.ts @@ -128,15 +128,12 @@ export default createRule< if (node.alternate) { return; } - if ( - node.test.type !== AST_NODE_TYPES.LogicalExpression && - node.test.type !== AST_NODE_TYPES.Identifier && - node.test.type !== AST_NODE_TYPES.MemberExpression && - node.test.type !== AST_NODE_TYPES.CallExpression - ) { + const ifBodyStatement = node.consequent.body[0]; + + if (ifBodyStatement.type !== AST_NODE_TYPES.ExpressionStatement) { return; } - const ifBodyStatement = node.consequent.body[0]; + const hasCommentsInIfBody = context.sourceCode.getCommentsBefore(ifBodyStatement).length || context.sourceCode.getCommentsAfter(ifBodyStatement).length; @@ -144,10 +141,6 @@ export default createRule< if (hasCommentsInIfBody) { return; } - - if (ifBodyStatement.type !== AST_NODE_TYPES.ExpressionStatement) { - return; - } const ifBodyExpression = ifBodyStatement.expression; if (ifBodyExpression.type !== AST_NODE_TYPES.CallExpression) { return; diff --git a/packages/eslint-plugin/tests/rules/prefer-optional-chain/prefer-optional-chain.test.ts b/packages/eslint-plugin/tests/rules/prefer-optional-chain/prefer-optional-chain.test.ts index d015a822844e..1e8128d8da94 100644 --- a/packages/eslint-plugin/tests/rules/prefer-optional-chain/prefer-optional-chain.test.ts +++ b/packages/eslint-plugin/tests/rules/prefer-optional-chain/prefer-optional-chain.test.ts @@ -759,6 +759,11 @@ foo?.bar?.baz?.bazz(); }, ], valid: [ + // Option is disabled + { + code: noFormat`if (foo) { foo.bar(); }`, + options: [{ allowSuggestingOnIfStatements: false }], + }, // Ignore no calls { code: noFormat`if (foo) { foo.bar; }`, @@ -808,6 +813,24 @@ foo?.bar?.baz?.bazz(); `, options: [{ allowSuggestingOnIfStatements: true }], }, + { + code: ` + if (foo && typeof window === 'undefined') { + foo.bar(); + } + `, + options: [{ allowSuggestingOnIfStatements: true }], + }, + { + code: ` + if (foo) { + if (foo.bar) { + console.log(window); + } + } + `, + options: [{ allowSuggestingOnIfStatements: true }], + }, ], }); }); From 02891625ac6e09048b4a7383912c739b4772ed17 Mon Sep 17 00:00:00 2001 From: Omri Luzon Date: Sat, 12 Oct 2024 16:53:14 +0300 Subject: [PATCH 06/38] Use `lastOperand` to fix false-negative coverage report --- .../src/rules/prefer-optional-chain-utils/analyzeChain.ts | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/packages/eslint-plugin/src/rules/prefer-optional-chain-utils/analyzeChain.ts b/packages/eslint-plugin/src/rules/prefer-optional-chain-utils/analyzeChain.ts index be0955f5104d..fdd88bc06710 100644 --- a/packages/eslint-plugin/src/rules/prefer-optional-chain-utils/analyzeChain.ts +++ b/packages/eslint-plugin/src/rules/prefer-optional-chain-utils/analyzeChain.ts @@ -408,9 +408,8 @@ function getReportDescriptor( const reportRange = getReportRange(chain, node, sourceCode); if (node.type === AST_NODE_TYPES.IfStatement) { - const lastChainNode = chain.at(-1)?.node; const chainEndedWithSemicolon = - lastChainNode && sourceCode.getTokenAfter(lastChainNode)?.value === ';'; + sourceCode.getTokenAfter(lastOperand.node)?.value === ';'; if (chainEndedWithSemicolon) { newCode += ';'; } From 958d09d7bace3e8eb116331305b628682beb98ce Mon Sep 17 00:00:00 2001 From: Omri Luzon Date: Sat, 12 Oct 2024 17:44:10 +0300 Subject: [PATCH 07/38] fix comment placement --- 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 c2235efa155b..cb97a64bc939 100644 --- a/packages/eslint-plugin/src/rules/prefer-optional-chain.ts +++ b/packages/eslint-plugin/src/rules/prefer-optional-chain.ts @@ -116,7 +116,6 @@ export default createRule< const seenLogicals = new Set(); return { - // specific handling for `(foo ?? {}).bar` / `(foo || {}).bar` 'IfStatement[consequent.body.length=1][consequent.type=BlockStatement]': ( node: { consequent: { type: AST_NODE_TYPES.BlockStatement }; @@ -236,6 +235,7 @@ export default createRule< } }, + // specific handling for `(foo ?? {}).bar` / `(foo || {}).bar` 'LogicalExpression[operator="||"], LogicalExpression[operator="??"]'( node: TSESTree.LogicalExpression, ): void { From 4490802350689832eb9717138957a49c6b55fda0 Mon Sep 17 00:00:00 2001 From: Omri Luzon Date: Sat, 12 Oct 2024 20:47:40 +0300 Subject: [PATCH 08/38] Support chaining in `if` body - only for logicals that end with `CallExpression` --- .../src/rules/prefer-optional-chain.ts | 38 ++++++++++++++----- .../prefer-optional-chain.test.ts | 23 ++++++++--- 2 files changed, 45 insertions(+), 16 deletions(-) diff --git a/packages/eslint-plugin/src/rules/prefer-optional-chain.ts b/packages/eslint-plugin/src/rules/prefer-optional-chain.ts index cb97a64bc939..98059f2dbaff 100644 --- a/packages/eslint-plugin/src/rules/prefer-optional-chain.ts +++ b/packages/eslint-plugin/src/rules/prefer-optional-chain.ts @@ -141,9 +141,6 @@ export default createRule< return; } const ifBodyExpression = ifBodyStatement.expression; - if (ifBodyExpression.type !== AST_NODE_TYPES.CallExpression) { - return; - } const currentChain: ValidOperand[] = [ { node: node.test, @@ -170,13 +167,34 @@ export default createRule< currentChain.push(operand); } } - currentChain.push({ - node: ifBodyExpression, - type: OperandValidity.Valid, - comparedName: ifBodyExpression, - comparisonType: NullishComparisonType.Boolean, - isYoda: false, - }); + if (ifBodyExpression.type === AST_NODE_TYPES.LogicalExpression) { + const { newlySeenLogicals, operands } = gatherLogicalOperands( + ifBodyExpression, + parserServices, + context.sourceCode, + options, + ); + for (const logical of newlySeenLogicals) { + seenLogicals.add(logical); + } + for (const operand of operands) { + if (operand.type === OperandValidity.Invalid) { + return; + } + currentChain.push(operand); + } + } else { + if (ifBodyExpression.type !== AST_NODE_TYPES.CallExpression) { + return; + } + currentChain.push({ + node: ifBodyExpression, + type: OperandValidity.Valid, + comparedName: ifBodyExpression, + comparisonType: NullishComparisonType.Boolean, + isYoda: false, + }); + } analyzeChain( context, parserServices, diff --git a/packages/eslint-plugin/tests/rules/prefer-optional-chain/prefer-optional-chain.test.ts b/packages/eslint-plugin/tests/rules/prefer-optional-chain/prefer-optional-chain.test.ts index 1e8128d8da94..05fcad8d2e08 100644 --- a/packages/eslint-plugin/tests/rules/prefer-optional-chain/prefer-optional-chain.test.ts +++ b/packages/eslint-plugin/tests/rules/prefer-optional-chain/prefer-optional-chain.test.ts @@ -740,10 +740,10 @@ describe('if block with a single statment matches part of the condition', () => }, { code: ` -declare const foo: undefined | { bar?: { baz?: { bazz: () => void } } }; -if (foo && foo.bar && foo.bar.baz) { - foo.bar.baz.bazz(); -} + declare const foo: undefined | { bar?: { baz?: { bazz: () => void } } }; + if (foo && foo.bar && foo.bar.baz) { + foo.bar.baz.bazz(); + } `, errors: [ { @@ -753,10 +753,21 @@ if (foo && foo.bar && foo.bar.baz) { ], options: [{ allowSuggestingOnIfStatements: true }], output: ` -declare const foo: undefined | { bar?: { baz?: { bazz: () => void } } }; -foo?.bar?.baz?.bazz(); + declare const foo: undefined | { bar?: { baz?: { bazz: () => void } } }; + foo?.bar?.baz?.bazz(); `, }, + { + code: noFormat`if (foo) { foo.bar.baz && foo.bar.baz(); }`, + errors: [ + { + messageId: 'preferOptionalChain', + suggestions: null, + }, + ], + options: [{ allowSuggestingOnIfStatements: true }], + output: 'foo?.bar.baz?.();', + }, ], valid: [ // Option is disabled From f3ad6bd22f6bff610546ddbb72a62c23643c2e66 Mon Sep 17 00:00:00 2001 From: Omri Luzon Date: Sun, 13 Oct 2024 11:35:03 +0300 Subject: [PATCH 09/38] coverage --- .../prefer-optional-chain.test.ts | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/packages/eslint-plugin/tests/rules/prefer-optional-chain/prefer-optional-chain.test.ts b/packages/eslint-plugin/tests/rules/prefer-optional-chain/prefer-optional-chain.test.ts index 05fcad8d2e08..45d5dca2d31d 100644 --- a/packages/eslint-plugin/tests/rules/prefer-optional-chain/prefer-optional-chain.test.ts +++ b/packages/eslint-plugin/tests/rules/prefer-optional-chain/prefer-optional-chain.test.ts @@ -832,6 +832,22 @@ describe('if block with a single statment matches part of the condition', () => `, options: [{ allowSuggestingOnIfStatements: true }], }, + { + code: ` + if (foo) { + typeof window === 'undefined' && foo.bar(); + } + `, + options: [{ allowSuggestingOnIfStatements: true }], + }, + { + code: ` + if (foo) { + foo.bar() && typeof window === 'undefined'; + } + `, + options: [{ allowSuggestingOnIfStatements: true }], + }, { code: ` if (foo) { From ed1ccbeefb10d930caff648d05c0352ce601ece8 Mon Sep 17 00:00:00 2001 From: Omri Luzon Date: Sun, 13 Oct 2024 11:45:19 +0300 Subject: [PATCH 10/38] reformat a bit and test with `requireNullish: true` --- .../prefer-optional-chain.test.ts | 84 +++++++------------ 1 file changed, 31 insertions(+), 53 deletions(-) diff --git a/packages/eslint-plugin/tests/rules/prefer-optional-chain/prefer-optional-chain.test.ts b/packages/eslint-plugin/tests/rules/prefer-optional-chain/prefer-optional-chain.test.ts index 45d5dca2d31d..76281c8f0257 100644 --- a/packages/eslint-plugin/tests/rules/prefer-optional-chain/prefer-optional-chain.test.ts +++ b/packages/eslint-plugin/tests/rules/prefer-optional-chain/prefer-optional-chain.test.ts @@ -674,70 +674,52 @@ describe('if block with a single statment matches part of the condition', () => invalid: [ { code: noFormat`if (foo) { foo.bar() }`, // Missing semi-colon - errors: [ - { - messageId: 'preferOptionalChain', - suggestions: null, - }, - ], + errors: [{ messageId: 'preferOptionalChain', suggestions: null }], options: [{ allowSuggestingOnIfStatements: true }], output: 'foo?.bar()', }, { code: noFormat`if (foo) { foo.bar(); }`, - errors: [ - { - messageId: 'preferOptionalChain', - suggestions: null, - }, - ], + errors: [{ messageId: 'preferOptionalChain', suggestions: null }], options: [{ allowSuggestingOnIfStatements: true }], output: 'foo?.bar();', }, { code: noFormat`if (foo) { foo(); }`, - errors: [ - { - messageId: 'preferOptionalChain', - suggestions: null, - }, - ], + errors: [{ messageId: 'preferOptionalChain', suggestions: null }], options: [{ allowSuggestingOnIfStatements: true }], output: 'foo?.();', }, { code: noFormat`if (foo) { foo[bar](); }`, - errors: [ - { - messageId: 'preferOptionalChain', - suggestions: null, - }, - ], + errors: [{ messageId: 'preferOptionalChain', suggestions: null }], options: [{ allowSuggestingOnIfStatements: true }], output: 'foo?.[bar]();', }, { code: noFormat`if (foo[bar]) { foo[bar].baz(); }`, - errors: [ - { - messageId: 'preferOptionalChain', - suggestions: null, - }, - ], + errors: [{ messageId: 'preferOptionalChain', suggestions: null }], options: [{ allowSuggestingOnIfStatements: true }], output: 'foo[bar]?.baz();', }, { code: noFormat`if (foo.bar.baz()) { foo.bar.baz().bazz(); }`, - errors: [ - { - messageId: 'preferOptionalChain', - suggestions: null, - }, - ], + errors: [{ messageId: 'preferOptionalChain', suggestions: null }], options: [{ allowSuggestingOnIfStatements: true }], output: 'foo.bar.baz()?.bazz();', }, + { + code: noFormat`if (foo && foo.bar && foo.bar.baz) { foo.bar.baz.bazz(); }`, + errors: [{ messageId: 'preferOptionalChain', suggestions: null }], + options: [{ allowSuggestingOnIfStatements: true }], + output: 'foo?.bar?.baz?.bazz();', + }, + { + code: noFormat`if (foo) { foo.bar.baz && foo.bar.baz(); }`, + errors: [{ messageId: 'preferOptionalChain', suggestions: null }], + options: [{ allowSuggestingOnIfStatements: true }], + output: 'foo?.bar.baz?.();', + }, { code: ` declare const foo: undefined | { bar?: { baz?: { bazz: () => void } } }; @@ -745,29 +727,15 @@ describe('if block with a single statment matches part of the condition', () => foo.bar.baz.bazz(); } `, - errors: [ - { - messageId: 'preferOptionalChain', - suggestions: null, - }, + errors: [{ messageId: 'preferOptionalChain', suggestions: null }], + options: [ + { allowSuggestingOnIfStatements: true, requireNullish: true }, ], - options: [{ allowSuggestingOnIfStatements: true }], output: ` declare const foo: undefined | { bar?: { baz?: { bazz: () => void } } }; foo?.bar?.baz?.bazz(); `, }, - { - code: noFormat`if (foo) { foo.bar.baz && foo.bar.baz(); }`, - errors: [ - { - messageId: 'preferOptionalChain', - suggestions: null, - }, - ], - options: [{ allowSuggestingOnIfStatements: true }], - output: 'foo?.bar.baz?.();', - }, ], valid: [ // Option is disabled @@ -858,6 +826,16 @@ describe('if block with a single statment matches part of the condition', () => `, options: [{ allowSuggestingOnIfStatements: true }], }, + { + code: ` + if (foo) { + foo.bar(); + } + `, + options: [ + { allowSuggestingOnIfStatements: true, requireNullish: true }, + ], + }, ], }); }); From 36f074ca53b5903b6d25e6150551a354812d0f90 Mon Sep 17 00:00:00 2001 From: Omri Luzon Date: Sun, 13 Oct 2024 12:34:25 +0300 Subject: [PATCH 11/38] comment --- packages/eslint-plugin/src/rules/prefer-optional-chain.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/eslint-plugin/src/rules/prefer-optional-chain.ts b/packages/eslint-plugin/src/rules/prefer-optional-chain.ts index 98059f2dbaff..6596c775860f 100644 --- a/packages/eslint-plugin/src/rules/prefer-optional-chain.ts +++ b/packages/eslint-plugin/src/rules/prefer-optional-chain.ts @@ -116,6 +116,7 @@ export default createRule< const seenLogicals = new Set(); return { + // specific handling for `if (foo) { foo.bar(); }` 'IfStatement[consequent.body.length=1][consequent.type=BlockStatement]': ( node: { consequent: { type: AST_NODE_TYPES.BlockStatement }; From 2c215873a185fc9c7ff695784fe33b2bf05d51ea Mon Sep 17 00:00:00 2001 From: Omri Luzon Date: Sun, 13 Oct 2024 13:40:37 +0300 Subject: [PATCH 12/38] Handle `if` without braces --- .../src/rules/prefer-optional-chain.ts | 160 +++++++++--------- .../prefer-optional-chain.test.ts | 6 + 2 files changed, 86 insertions(+), 80 deletions(-) diff --git a/packages/eslint-plugin/src/rules/prefer-optional-chain.ts b/packages/eslint-plugin/src/rules/prefer-optional-chain.ts index 6596c775860f..6a5049f5518b 100644 --- a/packages/eslint-plugin/src/rules/prefer-optional-chain.ts +++ b/packages/eslint-plugin/src/rules/prefer-optional-chain.ts @@ -116,96 +116,96 @@ export default createRule< const seenLogicals = new Set(); return { - // specific handling for `if (foo) { foo.bar(); }` - 'IfStatement[consequent.body.length=1][consequent.type=BlockStatement]': ( - node: { - consequent: { type: AST_NODE_TYPES.BlockStatement }; - } & TSESTree.IfStatement, - ): void => { - if (!options.allowSuggestingOnIfStatements) { - return; - } - if (node.alternate) { - return; - } - const ifBodyStatement = node.consequent.body[0]; + // specific handling for `if (foo) { foo.bar(); }` / `if (foo) foo.bar();` + 'IfStatement[consequent.type=BlockStatement][consequent.body.length=1], IfStatement[consequent.type=ExpressionStatement]': + (node: TSESTree.IfStatement): void => { + if (!options.allowSuggestingOnIfStatements) { + return; + } + if (node.alternate) { + return; + } + const ifBodyStatement = + node.consequent.type === AST_NODE_TYPES.BlockStatement + ? node.consequent.body[0] + : node.consequent; - if (ifBodyStatement.type !== AST_NODE_TYPES.ExpressionStatement) { - return; - } + if (ifBodyStatement.type !== AST_NODE_TYPES.ExpressionStatement) { + return; + } - const hasCommentsInIfBody = - context.sourceCode.getCommentsBefore(ifBodyStatement).length || - context.sourceCode.getCommentsAfter(ifBodyStatement).length; + const hasCommentsInIfBody = + context.sourceCode.getCommentsBefore(ifBodyStatement).length || + context.sourceCode.getCommentsAfter(ifBodyStatement).length; - if (hasCommentsInIfBody) { - return; - } - const ifBodyExpression = ifBodyStatement.expression; - const currentChain: ValidOperand[] = [ - { - node: node.test, - type: OperandValidity.Valid, - comparedName: node.test, - comparisonType: NullishComparisonType.Boolean, - isYoda: false, - }, - ]; - if (node.test.type === AST_NODE_TYPES.LogicalExpression) { - const { newlySeenLogicals, operands } = gatherLogicalOperands( - node.test, - parserServices, - context.sourceCode, - options, - ); - for (const logical of newlySeenLogicals) { - seenLogicals.add(logical); + if (hasCommentsInIfBody) { + return; + } + const ifBodyExpression = ifBodyStatement.expression; + const currentChain: ValidOperand[] = [ + { + node: node.test, + type: OperandValidity.Valid, + comparedName: node.test, + comparisonType: NullishComparisonType.Boolean, + isYoda: false, + }, + ]; + if (node.test.type === AST_NODE_TYPES.LogicalExpression) { + const { newlySeenLogicals, operands } = gatherLogicalOperands( + node.test, + parserServices, + context.sourceCode, + options, + ); + for (const logical of newlySeenLogicals) { + seenLogicals.add(logical); + } + for (const operand of operands) { + if (operand.type === OperandValidity.Invalid) { + return; + } + currentChain.push(operand); + } } - for (const operand of operands) { - if (operand.type === OperandValidity.Invalid) { + if (ifBodyExpression.type === AST_NODE_TYPES.LogicalExpression) { + const { newlySeenLogicals, operands } = gatherLogicalOperands( + ifBodyExpression, + parserServices, + context.sourceCode, + options, + ); + for (const logical of newlySeenLogicals) { + seenLogicals.add(logical); + } + for (const operand of operands) { + if (operand.type === OperandValidity.Invalid) { + return; + } + currentChain.push(operand); + } + } else { + if (ifBodyExpression.type !== AST_NODE_TYPES.CallExpression) { return; } - currentChain.push(operand); + currentChain.push({ + node: ifBodyExpression, + type: OperandValidity.Valid, + comparedName: ifBodyExpression, + comparisonType: NullishComparisonType.Boolean, + isYoda: false, + }); } - } - if (ifBodyExpression.type === AST_NODE_TYPES.LogicalExpression) { - const { newlySeenLogicals, operands } = gatherLogicalOperands( - ifBodyExpression, + analyzeChain( + context, parserServices, - context.sourceCode, options, + node, + '&&', + currentChain, ); - for (const logical of newlySeenLogicals) { - seenLogicals.add(logical); - } - for (const operand of operands) { - if (operand.type === OperandValidity.Invalid) { - return; - } - currentChain.push(operand); - } - } else { - if (ifBodyExpression.type !== AST_NODE_TYPES.CallExpression) { - return; - } - currentChain.push({ - node: ifBodyExpression, - type: OperandValidity.Valid, - comparedName: ifBodyExpression, - comparisonType: NullishComparisonType.Boolean, - isYoda: false, - }); - } - analyzeChain( - context, - parserServices, - options, - node, - '&&', - currentChain, - ); - return; - }, + return; + }, 'LogicalExpression[operator!="??"]'( node: TSESTree.LogicalExpression, diff --git a/packages/eslint-plugin/tests/rules/prefer-optional-chain/prefer-optional-chain.test.ts b/packages/eslint-plugin/tests/rules/prefer-optional-chain/prefer-optional-chain.test.ts index 76281c8f0257..b683b5e4c886 100644 --- a/packages/eslint-plugin/tests/rules/prefer-optional-chain/prefer-optional-chain.test.ts +++ b/packages/eslint-plugin/tests/rules/prefer-optional-chain/prefer-optional-chain.test.ts @@ -684,6 +684,12 @@ describe('if block with a single statment matches part of the condition', () => options: [{ allowSuggestingOnIfStatements: true }], output: 'foo?.bar();', }, + { + code: 'if (foo) foo.bar();', // Missing curly braces + errors: [{ messageId: 'preferOptionalChain', suggestions: null }], + options: [{ allowSuggestingOnIfStatements: true }], + output: 'foo?.bar();', + }, { code: noFormat`if (foo) { foo(); }`, errors: [{ messageId: 'preferOptionalChain', suggestions: null }], From 23943b15ab82bceff04b774532c54b4e2958a71e Mon Sep 17 00:00:00 2001 From: Omri Luzon Date: Sun, 13 Oct 2024 13:42:33 +0300 Subject: [PATCH 13/38] better node type --- .../eslint-plugin/src/rules/prefer-optional-chain.ts | 10 +++++++++- 1 file changed, 9 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 6a5049f5518b..ed4f026f8fd9 100644 --- a/packages/eslint-plugin/src/rules/prefer-optional-chain.ts +++ b/packages/eslint-plugin/src/rules/prefer-optional-chain.ts @@ -118,7 +118,15 @@ export default createRule< return { // specific handling for `if (foo) { foo.bar(); }` / `if (foo) foo.bar();` 'IfStatement[consequent.type=BlockStatement][consequent.body.length=1], IfStatement[consequent.type=ExpressionStatement]': - (node: TSESTree.IfStatement): void => { + ( + node: { + consequent: { + type: + | AST_NODE_TYPES.BlockStatement + | AST_NODE_TYPES.ExpressionStatement; + }; + } & TSESTree.IfStatement, + ): void => { if (!options.allowSuggestingOnIfStatements) { return; } From 5f3f9df01efad3920a498f6f819bef8835492c0d Mon Sep 17 00:00:00 2001 From: Omri Luzon Date: Fri, 18 Oct 2024 10:37:26 +0300 Subject: [PATCH 14/38] hit the missing coverage --- .../prefer-optional-chain.test.ts | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/packages/eslint-plugin/tests/rules/prefer-optional-chain/prefer-optional-chain.test.ts b/packages/eslint-plugin/tests/rules/prefer-optional-chain/prefer-optional-chain.test.ts index b683b5e4c886..d9a2f692108a 100644 --- a/packages/eslint-plugin/tests/rules/prefer-optional-chain/prefer-optional-chain.test.ts +++ b/packages/eslint-plugin/tests/rules/prefer-optional-chain/prefer-optional-chain.test.ts @@ -672,6 +672,12 @@ describe('|| {}', () => { describe('if block with a single statment matches part of the condition', () => { ruleTester.run('prefer-optional-chain', rule, { invalid: [ + { + code: noFormat`if (foo) { foo.bar(); }`, + errors: [{ messageId: 'preferOptionalChain', suggestions: null }], + options: [{ allowSuggestingOnIfStatements: true }], + output: 'foo?.bar();', + }, { code: noFormat`if (foo) { foo.bar() }`, // Missing semi-colon errors: [{ messageId: 'preferOptionalChain', suggestions: null }], @@ -679,16 +685,16 @@ describe('if block with a single statment matches part of the condition', () => output: 'foo?.bar()', }, { - code: noFormat`if (foo) { foo.bar(); }`, + code: 'if (foo) foo.bar();', // Missing curly braces errors: [{ messageId: 'preferOptionalChain', suggestions: null }], options: [{ allowSuggestingOnIfStatements: true }], output: 'foo?.bar();', }, { - code: 'if (foo) foo.bar();', // Missing curly braces + code: noFormat`if (foo) foo.bar()`, // Missing semi-colon and curly braces errors: [{ messageId: 'preferOptionalChain', suggestions: null }], options: [{ allowSuggestingOnIfStatements: true }], - output: 'foo?.bar();', + output: 'foo?.bar()', }, { code: noFormat`if (foo) { foo(); }`, From 3aab4baa9e48a75b2149ec0c91d6621d33eb3a3d Mon Sep 17 00:00:00 2001 From: Omri Luzon Date: Fri, 25 Oct 2024 19:40:38 +0300 Subject: [PATCH 15/38] `insert option description` --- packages/eslint-plugin/docs/rules/prefer-optional-chain.mdx | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/packages/eslint-plugin/docs/rules/prefer-optional-chain.mdx b/packages/eslint-plugin/docs/rules/prefer-optional-chain.mdx index c365f92ff848..a24edf78093a 100644 --- a/packages/eslint-plugin/docs/rules/prefer-optional-chain.mdx +++ b/packages/eslint-plugin/docs/rules/prefer-optional-chain.mdx @@ -91,7 +91,9 @@ When this option is `false` unsafe cases will have suggestion fixers provided in ### `allowSuggestingOnIfStatements` -When this option is `true`, the rule will analyze and suggest turning simple `if` statements with a single block expression into an optional chain call +{/* insert option description */} + +Examples of code for this rule with `{ allowSuggestingOnIfStatements: true }`: From bd7ee281e3997ac4abd1bf49e8ca28d9102fa66b Mon Sep 17 00:00:00 2001 From: Omri Luzon Date: Tue, 5 Nov 2024 16:44:22 +0200 Subject: [PATCH 16/38] CR: change arroy function to method to reduce indent --- .../src/rules/prefer-optional-chain.ts | 173 +++++++++--------- 1 file changed, 86 insertions(+), 87 deletions(-) diff --git a/packages/eslint-plugin/src/rules/prefer-optional-chain.ts b/packages/eslint-plugin/src/rules/prefer-optional-chain.ts index ed4f026f8fd9..ec4a18946ee8 100644 --- a/packages/eslint-plugin/src/rules/prefer-optional-chain.ts +++ b/packages/eslint-plugin/src/rules/prefer-optional-chain.ts @@ -117,103 +117,102 @@ export default createRule< return { // specific handling for `if (foo) { foo.bar(); }` / `if (foo) foo.bar();` - 'IfStatement[consequent.type=BlockStatement][consequent.body.length=1], IfStatement[consequent.type=ExpressionStatement]': - ( - node: { - consequent: { - type: - | AST_NODE_TYPES.BlockStatement - | AST_NODE_TYPES.ExpressionStatement; - }; - } & TSESTree.IfStatement, - ): void => { - if (!options.allowSuggestingOnIfStatements) { - return; - } - if (node.alternate) { - return; - } - const ifBodyStatement = - node.consequent.type === AST_NODE_TYPES.BlockStatement - ? node.consequent.body[0] - : node.consequent; + 'IfStatement[consequent.type=BlockStatement][consequent.body.length=1], IfStatement[consequent.type=ExpressionStatement]'( + node: { + consequent: { + type: + | AST_NODE_TYPES.BlockStatement + | AST_NODE_TYPES.ExpressionStatement; + }; + } & TSESTree.IfStatement, + ): void { + if (!options.allowSuggestingOnIfStatements) { + return; + } + if (node.alternate) { + return; + } + const ifBodyStatement = + node.consequent.type === AST_NODE_TYPES.BlockStatement + ? node.consequent.body[0] + : node.consequent; - if (ifBodyStatement.type !== AST_NODE_TYPES.ExpressionStatement) { - return; - } + if (ifBodyStatement.type !== AST_NODE_TYPES.ExpressionStatement) { + return; + } - const hasCommentsInIfBody = - context.sourceCode.getCommentsBefore(ifBodyStatement).length || - context.sourceCode.getCommentsAfter(ifBodyStatement).length; + const hasCommentsInIfBody = + context.sourceCode.getCommentsBefore(ifBodyStatement).length || + context.sourceCode.getCommentsAfter(ifBodyStatement).length; - if (hasCommentsInIfBody) { - return; - } - const ifBodyExpression = ifBodyStatement.expression; - const currentChain: ValidOperand[] = [ - { - node: node.test, - type: OperandValidity.Valid, - comparedName: node.test, - comparisonType: NullishComparisonType.Boolean, - isYoda: false, - }, - ]; - if (node.test.type === AST_NODE_TYPES.LogicalExpression) { - const { newlySeenLogicals, operands } = gatherLogicalOperands( - node.test, - parserServices, - context.sourceCode, - options, - ); - for (const logical of newlySeenLogicals) { - seenLogicals.add(logical); - } - for (const operand of operands) { - if (operand.type === OperandValidity.Invalid) { - return; - } - currentChain.push(operand); - } + if (hasCommentsInIfBody) { + return; + } + const ifBodyExpression = ifBodyStatement.expression; + const currentChain: ValidOperand[] = [ + { + node: node.test, + type: OperandValidity.Valid, + comparedName: node.test, + comparisonType: NullishComparisonType.Boolean, + isYoda: false, + }, + ]; + if (node.test.type === AST_NODE_TYPES.LogicalExpression) { + const { newlySeenLogicals, operands } = gatherLogicalOperands( + node.test, + parserServices, + context.sourceCode, + options, + ); + for (const logical of newlySeenLogicals) { + seenLogicals.add(logical); } - if (ifBodyExpression.type === AST_NODE_TYPES.LogicalExpression) { - const { newlySeenLogicals, operands } = gatherLogicalOperands( - ifBodyExpression, - parserServices, - context.sourceCode, - options, - ); - for (const logical of newlySeenLogicals) { - seenLogicals.add(logical); - } - for (const operand of operands) { - if (operand.type === OperandValidity.Invalid) { - return; - } - currentChain.push(operand); - } - } else { - if (ifBodyExpression.type !== AST_NODE_TYPES.CallExpression) { + for (const operand of operands) { + if (operand.type === OperandValidity.Invalid) { return; } - currentChain.push({ - node: ifBodyExpression, - type: OperandValidity.Valid, - comparedName: ifBodyExpression, - comparisonType: NullishComparisonType.Boolean, - isYoda: false, - }); + currentChain.push(operand); } - analyzeChain( - context, + } + if (ifBodyExpression.type === AST_NODE_TYPES.LogicalExpression) { + const { newlySeenLogicals, operands } = gatherLogicalOperands( + ifBodyExpression, parserServices, + context.sourceCode, options, - node, - '&&', - currentChain, ); - return; - }, + for (const logical of newlySeenLogicals) { + seenLogicals.add(logical); + } + for (const operand of operands) { + if (operand.type === OperandValidity.Invalid) { + return; + } + currentChain.push(operand); + } + } else { + if (ifBodyExpression.type !== AST_NODE_TYPES.CallExpression) { + return; + } + currentChain.push({ + node: ifBodyExpression, + type: OperandValidity.Valid, + comparedName: ifBodyExpression, + comparisonType: NullishComparisonType.Boolean, + isYoda: false, + }); + } + analyzeChain( + context, + parserServices, + options, + node, + '&&', + currentChain, + ); + return; + }, 'LogicalExpression[operator!="??"]'( node: TSESTree.LogicalExpression, From 353202f6825bb29e4f0ed68e52db4e14580b11c3 Mon Sep 17 00:00:00 2001 From: Omri Luzon Date: Tue, 5 Nov 2024 16:47:41 +0200 Subject: [PATCH 17/38] CR: less specific type --- packages/eslint-plugin/src/rules/prefer-optional-chain.ts | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/packages/eslint-plugin/src/rules/prefer-optional-chain.ts b/packages/eslint-plugin/src/rules/prefer-optional-chain.ts index ec4a18946ee8..d7cb1f1e6f4d 100644 --- a/packages/eslint-plugin/src/rules/prefer-optional-chain.ts +++ b/packages/eslint-plugin/src/rules/prefer-optional-chain.ts @@ -118,13 +118,7 @@ export default createRule< return { // specific handling for `if (foo) { foo.bar(); }` / `if (foo) foo.bar();` 'IfStatement[consequent.type=BlockStatement][consequent.body.length=1], IfStatement[consequent.type=ExpressionStatement]'( - node: { - consequent: { - type: - | AST_NODE_TYPES.BlockStatement - | AST_NODE_TYPES.ExpressionStatement; - }; - } & TSESTree.IfStatement, + node: TSESTree.IfStatement, ): void { if (!options.allowSuggestingOnIfStatements) { return; From e180f60d7135ece54c505ee78060f020b8ed68ae Mon Sep 17 00:00:00 2001 From: Omri Luzon Date: Tue, 5 Nov 2024 16:48:14 +0200 Subject: [PATCH 18/38] CR: combine early returns --- packages/eslint-plugin/src/rules/prefer-optional-chain.ts | 5 +---- 1 file changed, 1 insertion(+), 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 d7cb1f1e6f4d..cbea67636c56 100644 --- a/packages/eslint-plugin/src/rules/prefer-optional-chain.ts +++ b/packages/eslint-plugin/src/rules/prefer-optional-chain.ts @@ -120,10 +120,7 @@ export default createRule< 'IfStatement[consequent.type=BlockStatement][consequent.body.length=1], IfStatement[consequent.type=ExpressionStatement]'( node: TSESTree.IfStatement, ): void { - if (!options.allowSuggestingOnIfStatements) { - return; - } - if (node.alternate) { + if (!options.allowSuggestingOnIfStatements || node.alternate) { return; } const ifBodyStatement = From 5cdeb5e06dae1e3d5b0ec13300126f2a18eac7d2 Mon Sep 17 00:00:00 2001 From: Omri Luzon Date: Tue, 5 Nov 2024 16:48:57 +0200 Subject: [PATCH 19/38] CR: remove unncessary return --- packages/eslint-plugin/src/rules/prefer-optional-chain.ts | 1 - 1 file changed, 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 cbea67636c56..75ec4b18a36e 100644 --- a/packages/eslint-plugin/src/rules/prefer-optional-chain.ts +++ b/packages/eslint-plugin/src/rules/prefer-optional-chain.ts @@ -202,7 +202,6 @@ export default createRule< '&&', currentChain, ); - return; }, 'LogicalExpression[operator!="??"]'( From b6b34912362c55527fb491734855011c83fb635a Mon Sep 17 00:00:00 2001 From: Omri Luzon Date: Tue, 5 Nov 2024 16:52:29 +0200 Subject: [PATCH 20/38] CR: rename `allowSuggestingOnIfStatements` to `allowIfStatements` --- .../docs/rules/prefer-optional-chain.mdx | 12 ++--- .../PreferOptionalChainOptions.ts | 2 +- .../src/rules/prefer-optional-chain.ts | 12 ++--- .../prefer-optional-chain.shot | 4 +- .../prefer-optional-chain.test.ts | 52 +++++++++---------- .../prefer-optional-chain.shot | 10 ++-- 6 files changed, 44 insertions(+), 48 deletions(-) diff --git a/packages/eslint-plugin/docs/rules/prefer-optional-chain.mdx b/packages/eslint-plugin/docs/rules/prefer-optional-chain.mdx index a24edf78093a..e2836da5cc33 100644 --- a/packages/eslint-plugin/docs/rules/prefer-optional-chain.mdx +++ b/packages/eslint-plugin/docs/rules/prefer-optional-chain.mdx @@ -89,26 +89,26 @@ This style of code isn't super common - which means having this option set to `t When this option is `false` unsafe cases will have suggestion fixers provided instead of auto-fixers - meaning you can manually apply the fix using your IDE tooling. -### `allowSuggestingOnIfStatements` +### `allowIfStatements` {/* insert option description */} -Examples of code for this rule with `{ allowSuggestingOnIfStatements: true }`: +Examples of code for this rule with `{ allowIfStatements: true }`: - + -```ts option='{ "allowSuggestingOnIfStatements": true }' +```ts option='{ "allowIfStatements": true }' if (foo) { foo.bar(); } ``` - + -```ts option='{ "allowSuggestingOnIfStatements": false }' +```ts option='{ "allowIfStatements": false }' foo?.bar(); ``` diff --git a/packages/eslint-plugin/src/rules/prefer-optional-chain-utils/PreferOptionalChainOptions.ts b/packages/eslint-plugin/src/rules/prefer-optional-chain-utils/PreferOptionalChainOptions.ts index 07c01ae68605..510d896bc052 100644 --- a/packages/eslint-plugin/src/rules/prefer-optional-chain-utils/PreferOptionalChainOptions.ts +++ b/packages/eslint-plugin/src/rules/prefer-optional-chain-utils/PreferOptionalChainOptions.ts @@ -3,8 +3,8 @@ export type PreferOptionalChainMessageIds = | 'preferOptionalChain'; export interface PreferOptionalChainOptions { + allowIfStatements?: boolean; allowPotentiallyUnsafeFixesThatModifyTheReturnTypeIKnowWhatImDoing?: boolean; - allowSuggestingOnIfStatements?: boolean; checkAny?: boolean; checkBigInt?: boolean; checkBoolean?: boolean; diff --git a/packages/eslint-plugin/src/rules/prefer-optional-chain.ts b/packages/eslint-plugin/src/rules/prefer-optional-chain.ts index 75ec4b18a36e..6d59b9574cd9 100644 --- a/packages/eslint-plugin/src/rules/prefer-optional-chain.ts +++ b/packages/eslint-plugin/src/rules/prefer-optional-chain.ts @@ -48,15 +48,15 @@ export default createRule< type: 'object', additionalProperties: false, properties: { - allowPotentiallyUnsafeFixesThatModifyTheReturnTypeIKnowWhatImDoing: { + allowIfStatements: { type: 'boolean', description: - 'Allow autofixers that will change the return type of the expression. This option is considered unsafe as it may break the build.', + 'Allow autofix to optional chain on if statements with a single call expression in the consequent.', }, - allowSuggestingOnIfStatements: { + allowPotentiallyUnsafeFixesThatModifyTheReturnTypeIKnowWhatImDoing: { type: 'boolean', description: - 'Allow suggesting optional chain on if statements with a single expression in the consequent.', + 'Allow autofixers that will change the return type of the expression. This option is considered unsafe as it may break the build.', }, checkAny: { type: 'boolean', @@ -99,8 +99,8 @@ export default createRule< }, defaultOptions: [ { + allowIfStatements: false, allowPotentiallyUnsafeFixesThatModifyTheReturnTypeIKnowWhatImDoing: false, - allowSuggestingOnIfStatements: false, checkAny: true, checkBigInt: true, checkBoolean: true, @@ -120,7 +120,7 @@ export default createRule< 'IfStatement[consequent.type=BlockStatement][consequent.body.length=1], IfStatement[consequent.type=ExpressionStatement]'( node: TSESTree.IfStatement, ): void { - if (!options.allowSuggestingOnIfStatements || node.alternate) { + if (!options.allowIfStatements || node.alternate) { return; } const ifBodyStatement = diff --git a/packages/eslint-plugin/tests/docs-eslint-output-snapshots/prefer-optional-chain.shot b/packages/eslint-plugin/tests/docs-eslint-output-snapshots/prefer-optional-chain.shot index b7181d30833c..b29dee9c5293 100644 --- a/packages/eslint-plugin/tests/docs-eslint-output-snapshots/prefer-optional-chain.shot +++ b/packages/eslint-plugin/tests/docs-eslint-output-snapshots/prefer-optional-chain.shot @@ -72,7 +72,7 @@ acceptsBoolean(foo?.bar); exports[`Validating rule docs prefer-optional-chain.mdx code examples ESLint output 4`] = ` "Incorrect -Options: { "allowSuggestingOnIfStatements": true } +Options: { "allowIfStatements": true } if (foo) { ~~~~~~~~~~ Prefer using an optional chain expression instead, as it's more concise and easier to read. @@ -85,7 +85,7 @@ if (foo) { exports[`Validating rule docs prefer-optional-chain.mdx code examples ESLint output 5`] = ` "Correct -Options: { "allowSuggestingOnIfStatements": false } +Options: { "allowIfStatements": false } foo?.bar(); " diff --git a/packages/eslint-plugin/tests/rules/prefer-optional-chain/prefer-optional-chain.test.ts b/packages/eslint-plugin/tests/rules/prefer-optional-chain/prefer-optional-chain.test.ts index d9a2f692108a..9e7fc304da06 100644 --- a/packages/eslint-plugin/tests/rules/prefer-optional-chain/prefer-optional-chain.test.ts +++ b/packages/eslint-plugin/tests/rules/prefer-optional-chain/prefer-optional-chain.test.ts @@ -675,61 +675,61 @@ describe('if block with a single statment matches part of the condition', () => { code: noFormat`if (foo) { foo.bar(); }`, errors: [{ messageId: 'preferOptionalChain', suggestions: null }], - options: [{ allowSuggestingOnIfStatements: true }], + options: [{ allowIfStatements: true }], output: 'foo?.bar();', }, { code: noFormat`if (foo) { foo.bar() }`, // Missing semi-colon errors: [{ messageId: 'preferOptionalChain', suggestions: null }], - options: [{ allowSuggestingOnIfStatements: true }], + options: [{ allowIfStatements: true }], output: 'foo?.bar()', }, { code: 'if (foo) foo.bar();', // Missing curly braces errors: [{ messageId: 'preferOptionalChain', suggestions: null }], - options: [{ allowSuggestingOnIfStatements: true }], + options: [{ allowIfStatements: true }], output: 'foo?.bar();', }, { code: noFormat`if (foo) foo.bar()`, // Missing semi-colon and curly braces errors: [{ messageId: 'preferOptionalChain', suggestions: null }], - options: [{ allowSuggestingOnIfStatements: true }], + options: [{ allowIfStatements: true }], output: 'foo?.bar()', }, { code: noFormat`if (foo) { foo(); }`, errors: [{ messageId: 'preferOptionalChain', suggestions: null }], - options: [{ allowSuggestingOnIfStatements: true }], + options: [{ allowIfStatements: true }], output: 'foo?.();', }, { code: noFormat`if (foo) { foo[bar](); }`, errors: [{ messageId: 'preferOptionalChain', suggestions: null }], - options: [{ allowSuggestingOnIfStatements: true }], + options: [{ allowIfStatements: true }], output: 'foo?.[bar]();', }, { code: noFormat`if (foo[bar]) { foo[bar].baz(); }`, errors: [{ messageId: 'preferOptionalChain', suggestions: null }], - options: [{ allowSuggestingOnIfStatements: true }], + options: [{ allowIfStatements: true }], output: 'foo[bar]?.baz();', }, { code: noFormat`if (foo.bar.baz()) { foo.bar.baz().bazz(); }`, errors: [{ messageId: 'preferOptionalChain', suggestions: null }], - options: [{ allowSuggestingOnIfStatements: true }], + options: [{ allowIfStatements: true }], output: 'foo.bar.baz()?.bazz();', }, { code: noFormat`if (foo && foo.bar && foo.bar.baz) { foo.bar.baz.bazz(); }`, errors: [{ messageId: 'preferOptionalChain', suggestions: null }], - options: [{ allowSuggestingOnIfStatements: true }], + options: [{ allowIfStatements: true }], output: 'foo?.bar?.baz?.bazz();', }, { code: noFormat`if (foo) { foo.bar.baz && foo.bar.baz(); }`, errors: [{ messageId: 'preferOptionalChain', suggestions: null }], - options: [{ allowSuggestingOnIfStatements: true }], + options: [{ allowIfStatements: true }], output: 'foo?.bar.baz?.();', }, { @@ -740,9 +740,7 @@ describe('if block with a single statment matches part of the condition', () => } `, errors: [{ messageId: 'preferOptionalChain', suggestions: null }], - options: [ - { allowSuggestingOnIfStatements: true, requireNullish: true }, - ], + options: [{ allowIfStatements: true, requireNullish: true }], output: ` declare const foo: undefined | { bar?: { baz?: { bazz: () => void } } }; foo?.bar?.baz?.bazz(); @@ -753,25 +751,25 @@ describe('if block with a single statment matches part of the condition', () => // Option is disabled { code: noFormat`if (foo) { foo.bar(); }`, - options: [{ allowSuggestingOnIfStatements: false }], + options: [{ allowIfStatements: false }], }, // Ignore no calls { code: noFormat`if (foo) { foo.bar; }`, - options: [{ allowSuggestingOnIfStatements: true }], + options: [{ allowIfStatements: true }], }, { code: noFormat`if (foo) { foo.bar?.baz; }`, - options: [{ allowSuggestingOnIfStatements: true }], + options: [{ allowIfStatements: true }], }, // Ignore when comment exists before or after statement inside consequent { code: noFormat`if (foo) { /* comment */ foo.bar(); }`, - options: [{ allowSuggestingOnIfStatements: true }], + options: [{ allowIfStatements: true }], }, { code: noFormat`if (foo) { foo.bar(); /* comment */ }`, - options: [{ allowSuggestingOnIfStatements: true }], + options: [{ allowIfStatements: true }], }, // Ignore when multiple statements exist - only single call expression statement allowed { @@ -781,7 +779,7 @@ describe('if block with a single statment matches part of the condition', () => foo.baz(); } `, - options: [{ allowSuggestingOnIfStatements: true }], + options: [{ allowIfStatements: true }], }, // Ignore when else is used { @@ -793,7 +791,7 @@ describe('if block with a single statment matches part of the condition', () => // do something else } `, - options: [{ allowSuggestingOnIfStatements: true }], + options: [{ allowIfStatements: true }], }, { code: ` @@ -802,7 +800,7 @@ describe('if block with a single statment matches part of the condition', () => x.a(); } `, - options: [{ allowSuggestingOnIfStatements: true }], + options: [{ allowIfStatements: true }], }, { code: ` @@ -810,7 +808,7 @@ describe('if block with a single statment matches part of the condition', () => foo.bar(); } `, - options: [{ allowSuggestingOnIfStatements: true }], + options: [{ allowIfStatements: true }], }, { code: ` @@ -818,7 +816,7 @@ describe('if block with a single statment matches part of the condition', () => typeof window === 'undefined' && foo.bar(); } `, - options: [{ allowSuggestingOnIfStatements: true }], + options: [{ allowIfStatements: true }], }, { code: ` @@ -826,7 +824,7 @@ describe('if block with a single statment matches part of the condition', () => foo.bar() && typeof window === 'undefined'; } `, - options: [{ allowSuggestingOnIfStatements: true }], + options: [{ allowIfStatements: true }], }, { code: ` @@ -836,7 +834,7 @@ describe('if block with a single statment matches part of the condition', () => } } `, - options: [{ allowSuggestingOnIfStatements: true }], + options: [{ allowIfStatements: true }], }, { code: ` @@ -844,9 +842,7 @@ describe('if block with a single statment matches part of the condition', () => foo.bar(); } `, - options: [ - { allowSuggestingOnIfStatements: true, requireNullish: true }, - ], + options: [{ allowIfStatements: true, requireNullish: true }], }, ], }); diff --git a/packages/eslint-plugin/tests/schema-snapshots/prefer-optional-chain.shot b/packages/eslint-plugin/tests/schema-snapshots/prefer-optional-chain.shot index 55c41d5073b6..b90ae147c37b 100644 --- a/packages/eslint-plugin/tests/schema-snapshots/prefer-optional-chain.shot +++ b/packages/eslint-plugin/tests/schema-snapshots/prefer-optional-chain.shot @@ -8,12 +8,12 @@ exports[`Rule schemas should be convertible to TS types for documentation purpos { "additionalProperties": false, "properties": { - "allowPotentiallyUnsafeFixesThatModifyTheReturnTypeIKnowWhatImDoing": { - "description": "Allow autofixers that will change the return type of the expression. This option is considered unsafe as it may break the build.", + "allowIfStatements": { + "description": "Allow autofix to optional chain on if statements with a single call expression in the consequent.", "type": "boolean" }, - "allowSuggestingOnIfStatements": { - "description": "Allow suggesting optional chain on if statements with a single expression in the consequent.", + "allowPotentiallyUnsafeFixesThatModifyTheReturnTypeIKnowWhatImDoing": { + "description": "Allow autofixers that will change the return type of the expression. This option is considered unsafe as it may break the build.", "type": "boolean" }, "checkAny": { @@ -57,7 +57,7 @@ type Options = [ /** Allow autofixers that will change the return type of the expression. This option is considered unsafe as it may break the build. */ allowPotentiallyUnsafeFixesThatModifyTheReturnTypeIKnowWhatImDoing?: boolean; /** Allow suggesting optional chain on if statements with a single expression in the consequent. */ - allowSuggestingOnIfStatements?: boolean; + allowIfStatements?: boolean; /** Check operands that are typed as \`any\` when inspecting "loose boolean" operands. */ checkAny?: boolean; /** Check operands that are typed as \`bigint\` when inspecting "loose boolean" operands. */ From 1beff72d4039e6ebd67caec9cf54b70bcba964d2 Mon Sep 17 00:00:00 2001 From: Omri Luzon Date: Tue, 5 Nov 2024 16:56:00 +0200 Subject: [PATCH 21/38] CR: Fix title for mdx TabItem --- packages/eslint-plugin/docs/rules/prefer-optional-chain.mdx | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/eslint-plugin/docs/rules/prefer-optional-chain.mdx b/packages/eslint-plugin/docs/rules/prefer-optional-chain.mdx index e2836da5cc33..4de1674f5c9d 100644 --- a/packages/eslint-plugin/docs/rules/prefer-optional-chain.mdx +++ b/packages/eslint-plugin/docs/rules/prefer-optional-chain.mdx @@ -97,7 +97,7 @@ Examples of code for this rule with `{ allowIfStatements: true }`: - + ```ts option='{ "allowIfStatements": true }' if (foo) { @@ -106,7 +106,7 @@ if (foo) { ``` - + ```ts option='{ "allowIfStatements": false }' foo?.bar(); From 7b1273266be49d129293ae48efb06433c44545e1 Mon Sep 17 00:00:00 2001 From: Omri Luzon Date: Tue, 5 Nov 2024 16:58:19 +0200 Subject: [PATCH 22/38] CR: Remove comments on test cases --- .../prefer-optional-chain.test.ts | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/packages/eslint-plugin/tests/rules/prefer-optional-chain/prefer-optional-chain.test.ts b/packages/eslint-plugin/tests/rules/prefer-optional-chain/prefer-optional-chain.test.ts index 9e7fc304da06..9348e7ed9920 100644 --- a/packages/eslint-plugin/tests/rules/prefer-optional-chain/prefer-optional-chain.test.ts +++ b/packages/eslint-plugin/tests/rules/prefer-optional-chain/prefer-optional-chain.test.ts @@ -679,19 +679,19 @@ describe('if block with a single statment matches part of the condition', () => output: 'foo?.bar();', }, { - code: noFormat`if (foo) { foo.bar() }`, // Missing semi-colon + code: noFormat`if (foo) { foo.bar() }`, errors: [{ messageId: 'preferOptionalChain', suggestions: null }], options: [{ allowIfStatements: true }], output: 'foo?.bar()', }, { - code: 'if (foo) foo.bar();', // Missing curly braces + code: 'if (foo) foo.bar();', errors: [{ messageId: 'preferOptionalChain', suggestions: null }], options: [{ allowIfStatements: true }], output: 'foo?.bar();', }, { - code: noFormat`if (foo) foo.bar()`, // Missing semi-colon and curly braces + code: noFormat`if (foo) foo.bar()`, errors: [{ messageId: 'preferOptionalChain', suggestions: null }], options: [{ allowIfStatements: true }], output: 'foo?.bar()', @@ -748,12 +748,10 @@ describe('if block with a single statment matches part of the condition', () => }, ], valid: [ - // Option is disabled { code: noFormat`if (foo) { foo.bar(); }`, options: [{ allowIfStatements: false }], }, - // Ignore no calls { code: noFormat`if (foo) { foo.bar; }`, options: [{ allowIfStatements: true }], @@ -762,7 +760,6 @@ describe('if block with a single statment matches part of the condition', () => code: noFormat`if (foo) { foo.bar?.baz; }`, options: [{ allowIfStatements: true }], }, - // Ignore when comment exists before or after statement inside consequent { code: noFormat`if (foo) { /* comment */ foo.bar(); }`, options: [{ allowIfStatements: true }], @@ -771,7 +768,6 @@ describe('if block with a single statment matches part of the condition', () => code: noFormat`if (foo) { foo.bar(); /* comment */ }`, options: [{ allowIfStatements: true }], }, - // Ignore when multiple statements exist - only single call expression statement allowed { code: ` if (foo) { @@ -781,7 +777,6 @@ describe('if block with a single statment matches part of the condition', () => `, options: [{ allowIfStatements: true }], }, - // Ignore when else is used { code: ` declare const x: null | { a: () => string }; From c2c6ae30f8e0c2355379e143688b20b1ef011853 Mon Sep 17 00:00:00 2001 From: Omri Luzon Date: Tue, 5 Nov 2024 22:22:21 +0200 Subject: [PATCH 23/38] fix snapshot --- .../tests/schema-snapshots/prefer-optional-chain.shot | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/eslint-plugin/tests/schema-snapshots/prefer-optional-chain.shot b/packages/eslint-plugin/tests/schema-snapshots/prefer-optional-chain.shot index b90ae147c37b..025fc570f7ed 100644 --- a/packages/eslint-plugin/tests/schema-snapshots/prefer-optional-chain.shot +++ b/packages/eslint-plugin/tests/schema-snapshots/prefer-optional-chain.shot @@ -54,10 +54,10 @@ exports[`Rule schemas should be convertible to TS types for documentation purpos type Options = [ { + /** Allow autofix to optional chain on if statements with a single call expression in the consequent. */ + allowIfStatements?: boolean; /** Allow autofixers that will change the return type of the expression. This option is considered unsafe as it may break the build. */ allowPotentiallyUnsafeFixesThatModifyTheReturnTypeIKnowWhatImDoing?: boolean; - /** Allow suggesting optional chain on if statements with a single expression in the consequent. */ - allowIfStatements?: boolean; /** Check operands that are typed as \`any\` when inspecting "loose boolean" operands. */ checkAny?: boolean; /** Check operands that are typed as \`bigint\` when inspecting "loose boolean" operands. */ From 98cb43d79545670c137a87f8b1746268c9fc7913 Mon Sep 17 00:00:00 2001 From: Omri Luzon Date: Tue, 5 Nov 2024 22:28:34 +0200 Subject: [PATCH 24/38] CR: Make the rule work with comments to some degree --- .../analyzeChain.ts | 29 +++++++++++++++-- .../src/rules/prefer-optional-chain.ts | 7 ---- .../prefer-optional-chain.test.ts | 32 ++++++++++++++----- 3 files changed, 50 insertions(+), 18 deletions(-) diff --git a/packages/eslint-plugin/src/rules/prefer-optional-chain-utils/analyzeChain.ts b/packages/eslint-plugin/src/rules/prefer-optional-chain-utils/analyzeChain.ts index 7ec1877e3696..429f22d92a25 100644 --- a/packages/eslint-plugin/src/rules/prefer-optional-chain-utils/analyzeChain.ts +++ b/packages/eslint-plugin/src/rules/prefer-optional-chain-utils/analyzeChain.ts @@ -9,7 +9,7 @@ import type { SourceCode, } from '@typescript-eslint/utils/ts-eslint'; -import { AST_NODE_TYPES } from '@typescript-eslint/utils'; +import { AST_NODE_TYPES, AST_TOKEN_TYPES } from '@typescript-eslint/utils'; import { unionTypeParts } from 'ts-api-utils'; import * as ts from 'typescript'; @@ -246,13 +246,17 @@ function getReportRange( } function getReportDescriptor( - sourceCode: SourceCode, + context: RuleContext< + PreferOptionalChainMessageIds, + [PreferOptionalChainOptions] + >, parserServices: ParserServicesWithTypeInformation, node: TSESTree.Node, operator: '&&' | '||', options: PreferOptionalChainOptions, chain: ValidOperand[], ): ReportDescriptor { + const sourceCode = context.sourceCode; const lastOperand = chain[chain.length - 1]; let useSuggestionFixer: boolean; @@ -413,6 +417,25 @@ function getReportDescriptor( if (chainEndedWithSemicolon) { newCode += ';'; } + + function getTextFromCommentsArray(comments: TSESTree.Comment[]): string { + return comments + .map(({ type, value }) => + type === AST_TOKEN_TYPES.Line ? `//${value}` : `/*${value}*/`, + ) + .join(''); + } + const commentsBefore = sourceCode.getCommentsBefore(chain[1].node); + if (commentsBefore.length > 0) { + newCode = getTextFromCommentsArray(commentsBefore) + newCode; + } + const nodeBeforeTheComment = chainEndedWithSemicolon + ? lastOperand.node.parent + : lastOperand.node; + const commentsAfter = sourceCode.getCommentsAfter(nodeBeforeTheComment); + if (commentsAfter.length > 0) { + newCode += getTextFromCommentsArray(commentsAfter); + } } const fix: ReportFixFunction = fixer => @@ -567,7 +590,7 @@ export function analyzeChain( options, subChainFlat.slice(0, -1).map(({ node }) => node), getReportDescriptor( - context.sourceCode, + context, parserServices, node, operator, diff --git a/packages/eslint-plugin/src/rules/prefer-optional-chain.ts b/packages/eslint-plugin/src/rules/prefer-optional-chain.ts index 6d59b9574cd9..f7a587c18c61 100644 --- a/packages/eslint-plugin/src/rules/prefer-optional-chain.ts +++ b/packages/eslint-plugin/src/rules/prefer-optional-chain.ts @@ -132,13 +132,6 @@ export default createRule< return; } - const hasCommentsInIfBody = - context.sourceCode.getCommentsBefore(ifBodyStatement).length || - context.sourceCode.getCommentsAfter(ifBodyStatement).length; - - if (hasCommentsInIfBody) { - return; - } const ifBodyExpression = ifBodyStatement.expression; const currentChain: ValidOperand[] = [ { diff --git a/packages/eslint-plugin/tests/rules/prefer-optional-chain/prefer-optional-chain.test.ts b/packages/eslint-plugin/tests/rules/prefer-optional-chain/prefer-optional-chain.test.ts index 9348e7ed9920..e06f438b4cb6 100644 --- a/packages/eslint-plugin/tests/rules/prefer-optional-chain/prefer-optional-chain.test.ts +++ b/packages/eslint-plugin/tests/rules/prefer-optional-chain/prefer-optional-chain.test.ts @@ -732,6 +732,30 @@ describe('if block with a single statment matches part of the condition', () => options: [{ allowIfStatements: true }], output: 'foo?.bar.baz?.();', }, + { + code: noFormat`if (foo) { /* comment */ foo.bar(); }`, + errors: [{ messageId: 'preferOptionalChain', suggestions: null }], + options: [{ allowIfStatements: true }], + output: '/* comment */foo?.bar();', + }, + { + code: noFormat`if (foo) { /* comment *//* comment */ foo.bar(); }`, + errors: [{ messageId: 'preferOptionalChain', suggestions: null }], + options: [{ allowIfStatements: true }], + output: '/* comment *//* comment */foo?.bar();', + }, + { + code: noFormat`if (foo) { foo.bar(); /* comment */ }`, + errors: [{ messageId: 'preferOptionalChain', suggestions: null }], + options: [{ allowIfStatements: true }], + output: 'foo?.bar();/* comment */', + }, + { + code: noFormat`if (foo) { foo.bar(); /* comment *//* comment */ }`, + errors: [{ messageId: 'preferOptionalChain', suggestions: null }], + options: [{ allowIfStatements: true }], + output: 'foo?.bar();/* comment *//* comment */', + }, { code: ` declare const foo: undefined | { bar?: { baz?: { bazz: () => void } } }; @@ -760,14 +784,6 @@ describe('if block with a single statment matches part of the condition', () => code: noFormat`if (foo) { foo.bar?.baz; }`, options: [{ allowIfStatements: true }], }, - { - code: noFormat`if (foo) { /* comment */ foo.bar(); }`, - options: [{ allowIfStatements: true }], - }, - { - code: noFormat`if (foo) { foo.bar(); /* comment */ }`, - options: [{ allowIfStatements: true }], - }, { code: ` if (foo) { From 1a3bee2ea71bdbde988cbac72347efb8c55069ab Mon Sep 17 00:00:00 2001 From: Omri Luzon Date: Sat, 9 Nov 2024 19:16:53 +0200 Subject: [PATCH 25/38] Remove unncessary `suggestions: null` --- .../prefer-optional-chain.test.ts | 161 +++++++++--------- 1 file changed, 80 insertions(+), 81 deletions(-) diff --git a/packages/eslint-plugin/tests/rules/prefer-optional-chain/prefer-optional-chain.test.ts b/packages/eslint-plugin/tests/rules/prefer-optional-chain/prefer-optional-chain.test.ts index e06f438b4cb6..de88341d1cbb 100644 --- a/packages/eslint-plugin/tests/rules/prefer-optional-chain/prefer-optional-chain.test.ts +++ b/packages/eslint-plugin/tests/rules/prefer-optional-chain/prefer-optional-chain.test.ts @@ -674,85 +674,85 @@ describe('if block with a single statment matches part of the condition', () => invalid: [ { code: noFormat`if (foo) { foo.bar(); }`, - errors: [{ messageId: 'preferOptionalChain', suggestions: null }], + errors: [{ messageId: 'preferOptionalChain' }], options: [{ allowIfStatements: true }], output: 'foo?.bar();', }, { code: noFormat`if (foo) { foo.bar() }`, - errors: [{ messageId: 'preferOptionalChain', suggestions: null }], + errors: [{ messageId: 'preferOptionalChain' }], options: [{ allowIfStatements: true }], output: 'foo?.bar()', }, { code: 'if (foo) foo.bar();', - errors: [{ messageId: 'preferOptionalChain', suggestions: null }], + errors: [{ messageId: 'preferOptionalChain' }], options: [{ allowIfStatements: true }], output: 'foo?.bar();', }, { code: noFormat`if (foo) foo.bar()`, - errors: [{ messageId: 'preferOptionalChain', suggestions: null }], + errors: [{ messageId: 'preferOptionalChain' }], options: [{ allowIfStatements: true }], output: 'foo?.bar()', }, { code: noFormat`if (foo) { foo(); }`, - errors: [{ messageId: 'preferOptionalChain', suggestions: null }], + errors: [{ messageId: 'preferOptionalChain' }], options: [{ allowIfStatements: true }], output: 'foo?.();', }, { code: noFormat`if (foo) { foo[bar](); }`, - errors: [{ messageId: 'preferOptionalChain', suggestions: null }], + errors: [{ messageId: 'preferOptionalChain' }], options: [{ allowIfStatements: true }], output: 'foo?.[bar]();', }, { code: noFormat`if (foo[bar]) { foo[bar].baz(); }`, - errors: [{ messageId: 'preferOptionalChain', suggestions: null }], + errors: [{ messageId: 'preferOptionalChain' }], options: [{ allowIfStatements: true }], output: 'foo[bar]?.baz();', }, { code: noFormat`if (foo.bar.baz()) { foo.bar.baz().bazz(); }`, - errors: [{ messageId: 'preferOptionalChain', suggestions: null }], + errors: [{ messageId: 'preferOptionalChain' }], options: [{ allowIfStatements: true }], output: 'foo.bar.baz()?.bazz();', }, { code: noFormat`if (foo && foo.bar && foo.bar.baz) { foo.bar.baz.bazz(); }`, - errors: [{ messageId: 'preferOptionalChain', suggestions: null }], + errors: [{ messageId: 'preferOptionalChain' }], options: [{ allowIfStatements: true }], output: 'foo?.bar?.baz?.bazz();', }, { code: noFormat`if (foo) { foo.bar.baz && foo.bar.baz(); }`, - errors: [{ messageId: 'preferOptionalChain', suggestions: null }], + errors: [{ messageId: 'preferOptionalChain' }], options: [{ allowIfStatements: true }], output: 'foo?.bar.baz?.();', }, { code: noFormat`if (foo) { /* comment */ foo.bar(); }`, - errors: [{ messageId: 'preferOptionalChain', suggestions: null }], + errors: [{ messageId: 'preferOptionalChain' }], options: [{ allowIfStatements: true }], output: '/* comment */foo?.bar();', }, { code: noFormat`if (foo) { /* comment *//* comment */ foo.bar(); }`, - errors: [{ messageId: 'preferOptionalChain', suggestions: null }], + errors: [{ messageId: 'preferOptionalChain' }], options: [{ allowIfStatements: true }], output: '/* comment *//* comment */foo?.bar();', }, { code: noFormat`if (foo) { foo.bar(); /* comment */ }`, - errors: [{ messageId: 'preferOptionalChain', suggestions: null }], + errors: [{ messageId: 'preferOptionalChain' }], options: [{ allowIfStatements: true }], output: 'foo?.bar();/* comment */', }, { code: noFormat`if (foo) { foo.bar(); /* comment *//* comment */ }`, - errors: [{ messageId: 'preferOptionalChain', suggestions: null }], + errors: [{ messageId: 'preferOptionalChain' }], options: [{ allowIfStatements: true }], output: 'foo?.bar();/* comment *//* comment */', }, @@ -763,7 +763,7 @@ describe('if block with a single statment matches part of the condition', () => foo.bar.baz.bazz(); } `, - errors: [{ messageId: 'preferOptionalChain', suggestions: null }], + errors: [{ messageId: 'preferOptionalChain' }], options: [{ allowIfStatements: true, requireNullish: true }], output: ` declare const foo: undefined | { bar?: { baz?: { bazz: () => void } } }; @@ -866,15 +866,15 @@ describe('hand-crafted cases', () => { { code: noFormat`foo && foo.bar && foo.bar.baz || baz && baz.bar && baz.bar.foo`, errors: [ - { messageId: 'preferOptionalChain', suggestions: null }, - { messageId: 'preferOptionalChain', suggestions: null }, + { messageId: 'preferOptionalChain' }, + { messageId: 'preferOptionalChain' }, ], output: 'foo?.bar?.baz || baz?.bar?.foo', }, // case with inconsistent checks should "break" the chain { code: 'foo && foo.bar != null && foo.bar.baz !== undefined && foo.bar.baz.buzz;', - errors: [{ messageId: 'preferOptionalChain', suggestions: null }], + errors: [{ messageId: 'preferOptionalChain' }], output: 'foo?.bar != null && foo.bar.baz !== undefined && foo.bar.baz.buzz;', }, @@ -885,7 +885,7 @@ describe('hand-crafted cases', () => { foo.bar.baz.qux !== undefined && foo.bar.baz.qux.buzz; `, - errors: [{ messageId: 'preferOptionalChain', suggestions: null }], + errors: [{ messageId: 'preferOptionalChain' }], output: ` foo.bar?.baz != null && foo.bar.baz.qux !== undefined && @@ -895,66 +895,66 @@ describe('hand-crafted cases', () => { // ensure essential whitespace isn't removed { code: 'foo && foo.bar(baz => );', - errors: [{ messageId: 'preferOptionalChain', suggestions: null }], + errors: [{ messageId: 'preferOptionalChain' }], filename: 'react.tsx', languageOptions: { parserOptions: { ecmaFeatures: { jsx: true } } }, output: 'foo?.bar(baz => );', }, { code: 'foo && foo.bar(baz => typeof baz);', - errors: [{ messageId: 'preferOptionalChain', suggestions: null }], + errors: [{ messageId: 'preferOptionalChain' }], output: 'foo?.bar(baz => typeof baz);', }, { code: "foo && foo['some long string'] && foo['some long string'].baz;", - errors: [{ messageId: 'preferOptionalChain', suggestions: null }], + errors: [{ messageId: 'preferOptionalChain' }], output: "foo?.['some long string']?.baz;", }, { code: 'foo && foo[`some long string`] && foo[`some long string`].baz;', - errors: [{ messageId: 'preferOptionalChain', suggestions: null }], + errors: [{ messageId: 'preferOptionalChain' }], output: 'foo?.[`some long string`]?.baz;', }, { code: 'foo && foo[`some ${long} string`] && foo[`some ${long} string`].baz;', - errors: [{ messageId: 'preferOptionalChain', suggestions: null }], + errors: [{ messageId: 'preferOptionalChain' }], output: 'foo?.[`some ${long} string`]?.baz;', }, // complex computed properties should be handled correctly { code: 'foo && foo[bar as string] && foo[bar as string].baz;', - errors: [{ messageId: 'preferOptionalChain', suggestions: null }], + errors: [{ messageId: 'preferOptionalChain' }], output: 'foo?.[bar as string]?.baz;', }, { code: 'foo && foo[1 + 2] && foo[1 + 2].baz;', - errors: [{ messageId: 'preferOptionalChain', suggestions: null }], + errors: [{ messageId: 'preferOptionalChain' }], output: 'foo?.[1 + 2]?.baz;', }, { code: 'foo && foo[typeof bar] && foo[typeof bar].baz;', - errors: [{ messageId: 'preferOptionalChain', suggestions: null }], + errors: [{ messageId: 'preferOptionalChain' }], output: 'foo?.[typeof bar]?.baz;', }, { code: 'foo && foo.bar(a) && foo.bar(a, b).baz;', - errors: [{ messageId: 'preferOptionalChain', suggestions: null }], + errors: [{ messageId: 'preferOptionalChain' }], output: 'foo?.bar(a) && foo.bar(a, b).baz;', }, { code: 'foo() && foo()(bar);', - errors: [{ messageId: 'preferOptionalChain', suggestions: null }], + errors: [{ messageId: 'preferOptionalChain' }], output: 'foo()?.(bar);', }, // type parameters are considered { code: 'foo && foo() && foo().bar;', - errors: [{ messageId: 'preferOptionalChain', suggestions: null }], + errors: [{ messageId: 'preferOptionalChain' }], output: 'foo?.()?.bar;', }, { code: 'foo && foo() && foo().bar;', - errors: [{ messageId: 'preferOptionalChain', suggestions: null }], + errors: [{ messageId: 'preferOptionalChain' }], output: 'foo?.() && foo().bar;', }, // should preserve comments in a call expression @@ -964,7 +964,7 @@ describe('hand-crafted cases', () => { // comment2 b, ); `, - errors: [{ messageId: 'preferOptionalChain', suggestions: null }], + errors: [{ messageId: 'preferOptionalChain' }], output: ` foo?.bar(/* comment */a, // comment2 @@ -975,34 +975,34 @@ describe('hand-crafted cases', () => { // these get autofixers because the trailing binary means the type doesn't matter { code: 'foo && foo.bar != null;', - errors: [{ messageId: 'preferOptionalChain', suggestions: null }], + errors: [{ messageId: 'preferOptionalChain' }], output: 'foo?.bar != null;', }, { code: 'foo && foo.bar != undefined;', - errors: [{ messageId: 'preferOptionalChain', suggestions: null }], + errors: [{ messageId: 'preferOptionalChain' }], output: 'foo?.bar != undefined;', }, { code: 'foo && foo.bar != null && baz;', - errors: [{ messageId: 'preferOptionalChain', suggestions: null }], + errors: [{ messageId: 'preferOptionalChain' }], output: 'foo?.bar != null && baz;', }, // case with this keyword at the start of expression { code: 'this.bar && this.bar.baz;', - errors: [{ messageId: 'preferOptionalChain', suggestions: null }], + errors: [{ messageId: 'preferOptionalChain' }], output: 'this.bar?.baz;', }, // other weird cases { code: 'foo && foo?.();', - errors: [{ messageId: 'preferOptionalChain', suggestions: null }], + errors: [{ messageId: 'preferOptionalChain' }], output: 'foo?.();', }, { code: 'foo.bar && foo.bar?.();', - errors: [{ messageId: 'preferOptionalChain', suggestions: null }], + errors: [{ messageId: 'preferOptionalChain' }], output: 'foo.bar?.();', }, { @@ -1012,7 +1012,6 @@ describe('hand-crafted cases', () => { column: 1, line: 1, messageId: 'preferOptionalChain', - suggestions: null, }, ], filename: 'react.tsx', @@ -1022,35 +1021,35 @@ describe('hand-crafted cases', () => { // case with this keyword at the start of expression { code: '!this.bar || !this.bar.baz;', - errors: [{ messageId: 'preferOptionalChain', suggestions: null }], + errors: [{ messageId: 'preferOptionalChain' }], output: '!this.bar?.baz;', }, { code: '!a.b || !a.b();', - errors: [{ messageId: 'preferOptionalChain', suggestions: null }], + errors: [{ messageId: 'preferOptionalChain' }], output: '!a.b?.();', }, { code: '!foo.bar || !foo.bar.baz;', - errors: [{ messageId: 'preferOptionalChain', suggestions: null }], + errors: [{ messageId: 'preferOptionalChain' }], output: '!foo.bar?.baz;', }, { code: '!foo[bar] || !foo[bar]?.[baz];', - errors: [{ messageId: 'preferOptionalChain', suggestions: null }], + errors: [{ messageId: 'preferOptionalChain' }], output: '!foo[bar]?.[baz];', }, { code: '!foo || !foo?.bar.baz;', - errors: [{ messageId: 'preferOptionalChain', suggestions: null }], + errors: [{ messageId: 'preferOptionalChain' }], output: '!foo?.bar.baz;', }, // two errors { code: '(!foo || !foo.bar || !foo.bar.baz) && (!baz || !baz.bar || !baz.bar.foo);', errors: [ - { messageId: 'preferOptionalChain', suggestions: null }, - { messageId: 'preferOptionalChain', suggestions: null }, + { messageId: 'preferOptionalChain' }, + { messageId: 'preferOptionalChain' }, ], output: '(!foo?.bar?.baz) && (!baz?.bar?.foo);', }, @@ -1083,38 +1082,38 @@ describe('hand-crafted cases', () => { }, { code: 'import.meta && import.meta?.baz;', - errors: [{ messageId: 'preferOptionalChain', suggestions: null }], + errors: [{ messageId: 'preferOptionalChain' }], output: 'import.meta?.baz;', }, { code: '!import.meta || !import.meta?.baz;', - errors: [{ messageId: 'preferOptionalChain', suggestions: null }], + errors: [{ messageId: 'preferOptionalChain' }], output: '!import.meta?.baz;', }, { code: 'import.meta && import.meta?.() && import.meta?.().baz;', - errors: [{ messageId: 'preferOptionalChain', suggestions: null }], + errors: [{ messageId: 'preferOptionalChain' }], output: 'import.meta?.()?.baz;', }, // non-null expressions { code: '!foo() || !foo().bar;', - errors: [{ messageId: 'preferOptionalChain', suggestions: null }], + errors: [{ messageId: 'preferOptionalChain' }], output: '!foo()?.bar;', }, { code: '!foo!.bar || !foo!.bar.baz;', - errors: [{ messageId: 'preferOptionalChain', suggestions: null }], + errors: [{ messageId: 'preferOptionalChain' }], output: '!foo!.bar?.baz;', }, { code: '!foo!.bar!.baz || !foo!.bar!.baz!.paz;', - errors: [{ messageId: 'preferOptionalChain', suggestions: null }], + errors: [{ messageId: 'preferOptionalChain' }], output: '!foo!.bar!.baz?.paz;', }, { code: '!foo.bar!.baz || !foo.bar!.baz!.paz;', - errors: [{ messageId: 'preferOptionalChain', suggestions: null }], + errors: [{ messageId: 'preferOptionalChain' }], output: '!foo.bar!.baz?.paz;', }, { @@ -1140,7 +1139,7 @@ describe('hand-crafted cases', () => { }, { code: 'foo != null && foo.bar != null;', - errors: [{ messageId: 'preferOptionalChain', suggestions: null }], + errors: [{ messageId: 'preferOptionalChain' }], output: 'foo?.bar != null;', }, { @@ -1169,7 +1168,7 @@ describe('hand-crafted cases', () => { declare const foo: { bar: string | null } | null; foo !== null && foo.bar != null; `, - errors: [{ messageId: 'preferOptionalChain', suggestions: null }], + errors: [{ messageId: 'preferOptionalChain' }], output: ` declare const foo: { bar: string | null } | null; foo?.bar != null; @@ -1178,61 +1177,61 @@ describe('hand-crafted cases', () => { // https://github.com/typescript-eslint/typescript-eslint/issues/6332 { code: 'unrelated != null && foo != null && foo.bar != null;', - errors: [{ messageId: 'preferOptionalChain', suggestions: null }], + errors: [{ messageId: 'preferOptionalChain' }], output: 'unrelated != null && foo?.bar != null;', }, { code: 'unrelated1 != null && unrelated2 != null && foo != null && foo.bar != null;', - errors: [{ messageId: 'preferOptionalChain', suggestions: null }], + errors: [{ messageId: 'preferOptionalChain' }], output: 'unrelated1 != null && unrelated2 != null && foo?.bar != null;', }, // https://github.com/typescript-eslint/typescript-eslint/issues/1461 { code: 'foo1 != null && foo1.bar != null && foo2 != null && foo2.bar != null;', errors: [ - { messageId: 'preferOptionalChain', suggestions: null }, - { messageId: 'preferOptionalChain', suggestions: null }, + { messageId: 'preferOptionalChain' }, + { messageId: 'preferOptionalChain' }, ], output: 'foo1?.bar != null && foo2?.bar != null;', }, { code: 'foo && foo.a && bar && bar.a;', errors: [ - { messageId: 'preferOptionalChain', suggestions: null }, - { messageId: 'preferOptionalChain', suggestions: null }, + { messageId: 'preferOptionalChain' }, + { messageId: 'preferOptionalChain' }, ], output: 'foo?.a && bar?.a;', }, // randomly placed optional chain tokens are ignored { code: 'foo.bar.baz != null && foo?.bar?.baz.bam != null;', - errors: [{ messageId: 'preferOptionalChain', suggestions: null }], + errors: [{ messageId: 'preferOptionalChain' }], output: 'foo.bar.baz?.bam != null;', }, { code: 'foo?.bar.baz != null && foo.bar?.baz.bam != null;', - errors: [{ messageId: 'preferOptionalChain', suggestions: null }], + errors: [{ messageId: 'preferOptionalChain' }], output: 'foo?.bar.baz?.bam != null;', }, { code: 'foo?.bar?.baz != null && foo.bar.baz.bam != null;', - errors: [{ messageId: 'preferOptionalChain', suggestions: null }], + errors: [{ messageId: 'preferOptionalChain' }], output: 'foo?.bar?.baz?.bam != null;', }, // randomly placed non-null assertions are retained as long as they're in an earlier operand { code: 'foo.bar.baz != null && foo!.bar!.baz.bam != null;', - errors: [{ messageId: 'preferOptionalChain', suggestions: null }], + errors: [{ messageId: 'preferOptionalChain' }], output: 'foo.bar.baz?.bam != null;', }, { code: 'foo!.bar.baz != null && foo.bar!.baz.bam != null;', - errors: [{ messageId: 'preferOptionalChain', suggestions: null }], + errors: [{ messageId: 'preferOptionalChain' }], output: 'foo!.bar.baz?.bam != null;', }, { code: 'foo!.bar!.baz != null && foo.bar.baz.bam != null;', - errors: [{ messageId: 'preferOptionalChain', suggestions: null }], + errors: [{ messageId: 'preferOptionalChain' }], output: 'foo!.bar!.baz?.bam != null;', }, // mixed binary checks are followed and flagged @@ -1250,7 +1249,7 @@ describe('hand-crafted cases', () => { a.b.c.d.e.f.g !== null && a.b.c.d.e.f.g.h; `, - errors: [{ messageId: 'preferOptionalChain', suggestions: null }], + errors: [{ messageId: 'preferOptionalChain' }], output: ` a?.b?.c?.d?.e?.f?.g?.h; `, @@ -1269,7 +1268,7 @@ describe('hand-crafted cases', () => { a.b.c.d.e.f.g === null || !a.b.c.d.e.f.g.h; `, - errors: [{ messageId: 'preferOptionalChain', suggestions: null }], + errors: [{ messageId: 'preferOptionalChain' }], output: ` !a?.b?.c?.d?.e?.f?.g?.h; `, @@ -1288,7 +1287,7 @@ describe('hand-crafted cases', () => { a.b.c.d.e.f.g === null || !a.b.c.d.e.f.g.h; `, - errors: [{ messageId: 'preferOptionalChain', suggestions: null }], + errors: [{ messageId: 'preferOptionalChain' }], output: ` !a?.b?.c?.d?.e?.f?.g?.h; `, @@ -1296,7 +1295,7 @@ describe('hand-crafted cases', () => { // yoda checks are flagged { code: 'undefined !== foo && null !== foo && null != foo.bar && foo.bar.baz;', - errors: [{ messageId: 'preferOptionalChain', suggestions: null }], + errors: [{ messageId: 'preferOptionalChain' }], output: 'foo?.bar?.baz;', }, { @@ -1306,7 +1305,7 @@ describe('hand-crafted cases', () => { null !== foo.bar && foo.bar.baz; `, - errors: [{ messageId: 'preferOptionalChain', suggestions: null }], + errors: [{ messageId: 'preferOptionalChain' }], output: ` foo?.bar?.baz; `, @@ -1318,7 +1317,7 @@ describe('hand-crafted cases', () => { null !== foo.bar && null != foo.bar.baz; `, - errors: [{ messageId: 'preferOptionalChain', suggestions: null }], + errors: [{ messageId: 'preferOptionalChain' }], output: ` null != foo?.bar?.baz; `, @@ -1428,7 +1427,7 @@ describe('hand-crafted cases', () => { undefined !== foo.bar.baz && null !== foo.bar.baz; `, - errors: [{ messageId: 'preferOptionalChain', suggestions: null }], + errors: [{ messageId: 'preferOptionalChain' }], output: ` undefined !== foo?.bar?.baz && null !== foo.bar.baz; @@ -1442,7 +1441,7 @@ describe('hand-crafted cases', () => { foo.bar.baz !== undefined && foo.bar.baz !== null; `, - errors: [{ messageId: 'preferOptionalChain', suggestions: null }], + errors: [{ messageId: 'preferOptionalChain' }], output: ` foo?.bar?.baz !== undefined && foo.bar.baz !== null; @@ -1451,7 +1450,7 @@ describe('hand-crafted cases', () => { // await { code: '(await foo).bar && (await foo).bar.baz;', - errors: [{ messageId: 'preferOptionalChain', suggestions: null }], + errors: [{ messageId: 'preferOptionalChain' }], output: '(await foo).bar?.baz;', }, // TODO - should we handle this case and expand the range, or should we leave this as is? @@ -1468,7 +1467,7 @@ describe('hand-crafted cases', () => { a.b.c.d.e.f.g == null || a.b.c.d.e.f.g.h; `, - errors: [{ messageId: 'preferOptionalChain', suggestions: null }], + errors: [{ messageId: 'preferOptionalChain' }], output: ` a?.b?.c?.d?.e?.f?.g == null || a.b.c.d.e.f.g.h; @@ -1480,7 +1479,7 @@ describe('hand-crafted cases', () => { declare const foo: { bar: number } | null | undefined; foo && foo.bar != null; `, - errors: [{ messageId: 'preferOptionalChain', suggestions: null }], + errors: [{ messageId: 'preferOptionalChain' }], output: ` declare const foo: { bar: number } | null | undefined; foo?.bar != null; @@ -1491,7 +1490,7 @@ describe('hand-crafted cases', () => { declare const foo: { bar: number } | undefined; foo && typeof foo.bar !== 'undefined'; `, - errors: [{ messageId: 'preferOptionalChain', suggestions: null }], + errors: [{ messageId: 'preferOptionalChain' }], output: ` declare const foo: { bar: number } | undefined; typeof foo?.bar !== 'undefined'; @@ -1502,7 +1501,7 @@ describe('hand-crafted cases', () => { declare const foo: { bar: number } | undefined; foo && 'undefined' !== typeof foo.bar; `, - errors: [{ messageId: 'preferOptionalChain', suggestions: null }], + errors: [{ messageId: 'preferOptionalChain' }], output: ` declare const foo: { bar: number } | undefined; 'undefined' !== typeof foo?.bar; @@ -1673,7 +1672,7 @@ describe('hand-crafted cases', () => { declare const foo: { bar: number } | null | undefined; foo != undefined && foo.bar; `, - errors: [{ messageId: 'preferOptionalChain', suggestions: null }], + errors: [{ messageId: 'preferOptionalChain' }], options: [ { allowPotentiallyUnsafeFixesThatModifyTheReturnTypeIKnowWhatImDoing: From bfbf55c7f32e7dcf7a1d08d1c653d7ff7f77eac4 Mon Sep 17 00:00:00 2001 From: Omri Luzon Date: Fri, 8 Nov 2024 08:15:52 +0200 Subject: [PATCH 26/38] better comments --- .../analyzeChain.ts | 33 ++++++---- .../prefer-optional-chain.test.ts | 60 ++++++++++++++++--- 2 files changed, 72 insertions(+), 21 deletions(-) diff --git a/packages/eslint-plugin/src/rules/prefer-optional-chain-utils/analyzeChain.ts b/packages/eslint-plugin/src/rules/prefer-optional-chain-utils/analyzeChain.ts index 429f22d92a25..9ddd0fcb3e1b 100644 --- a/packages/eslint-plugin/src/rules/prefer-optional-chain-utils/analyzeChain.ts +++ b/packages/eslint-plugin/src/rules/prefer-optional-chain-utils/analyzeChain.ts @@ -418,23 +418,30 @@ function getReportDescriptor( newCode += ';'; } - function getTextFromCommentsArray(comments: TSESTree.Comment[]): string { - return comments - .map(({ type, value }) => - type === AST_TOKEN_TYPES.Line ? `//${value}` : `/*${value}*/`, - ) - .join(''); - } - const commentsBefore = sourceCode.getCommentsBefore(chain[1].node); - if (commentsBefore.length > 0) { - newCode = getTextFromCommentsArray(commentsBefore) + newCode; - } const nodeBeforeTheComment = chainEndedWithSemicolon ? lastOperand.node.parent : lastOperand.node; + const commentsBefore = sourceCode.getCommentsBefore(chain[1].node); const commentsAfter = sourceCode.getCommentsAfter(nodeBeforeTheComment); - if (commentsAfter.length > 0) { - newCode += getTextFromCommentsArray(commentsAfter); + if (commentsBefore.length || commentsAfter.length) { + const indentationCount = node.loc.start.column; + const indentation = ' '.repeat(indentationCount); + const newLineIndentation = `\n${indentation}`; + function getTextFromCommentsArray(comments: TSESTree.Comment[]): string { + return comments + .map(({ type, value }) => + type === AST_TOKEN_TYPES.Line ? `//${value}` : `/*${value}*/`, + ) + .join(newLineIndentation); + } + if (commentsBefore.length > 0) { + const commentsText = getTextFromCommentsArray(commentsBefore); + newCode = `${commentsText}\n${indentation}${newCode}`; + } + if (commentsAfter.length > 0) { + const commentsText = getTextFromCommentsArray(commentsAfter); + newCode = `${newCode}\n${indentation}${commentsText}`; + } } } diff --git a/packages/eslint-plugin/tests/rules/prefer-optional-chain/prefer-optional-chain.test.ts b/packages/eslint-plugin/tests/rules/prefer-optional-chain/prefer-optional-chain.test.ts index de88341d1cbb..87af54263549 100644 --- a/packages/eslint-plugin/tests/rules/prefer-optional-chain/prefer-optional-chain.test.ts +++ b/packages/eslint-plugin/tests/rules/prefer-optional-chain/prefer-optional-chain.test.ts @@ -733,28 +733,72 @@ describe('if block with a single statment matches part of the condition', () => output: 'foo?.bar.baz?.();', }, { - code: noFormat`if (foo) { /* comment */ foo.bar(); }`, + code: ` + if (foo) { + // before expression + /** multi + line */ + // single-line + foo.bar(); /* after semicolon */ + // after expression + } + `, errors: [{ messageId: 'preferOptionalChain' }], options: [{ allowIfStatements: true }], - output: '/* comment */foo?.bar();', + output: ` + // before expression + /** multi + line */ + // single-line + foo?.bar(); + /* after semicolon */ + // after expression + `, }, { - code: noFormat`if (foo) { /* comment *//* comment */ foo.bar(); }`, + code: ` + if (foo) { + // comment1 + // comment2 + foo.bar(); + } + `, errors: [{ messageId: 'preferOptionalChain' }], options: [{ allowIfStatements: true }], - output: '/* comment *//* comment */foo?.bar();', + output: ` + // comment1 + // comment2 + foo?.bar(); + `, }, { - code: noFormat`if (foo) { foo.bar(); /* comment */ }`, + code: ` + if (foo) { + foo.bar(); // comment + } + `, errors: [{ messageId: 'preferOptionalChain' }], options: [{ allowIfStatements: true }], - output: 'foo?.bar();/* comment */', + output: ` + foo?.bar(); + // comment + `, }, { - code: noFormat`if (foo) { foo.bar(); /* comment *//* comment */ }`, + code: ` + if (foo) { + foo.bar(); + // comment 1 + // comment 2 + } + `, errors: [{ messageId: 'preferOptionalChain' }], options: [{ allowIfStatements: true }], - output: 'foo?.bar();/* comment *//* comment */', + output: ` + foo?.bar(); + // comment 1 + // comment 2 + `, }, { code: ` From 5d9ef8f19425182332c4a2feef5f64b72a350fd4 Mon Sep 17 00:00:00 2001 From: Omri Luzon Date: Sat, 9 Nov 2024 20:02:54 +0200 Subject: [PATCH 27/38] change to suggestion fixer when `if` contains comments between --- .../analyzeChain.ts | 1 + .../prefer-optional-chain.test.ts | 60 +++++++++++++++---- 2 files changed, 49 insertions(+), 12 deletions(-) diff --git a/packages/eslint-plugin/src/rules/prefer-optional-chain-utils/analyzeChain.ts b/packages/eslint-plugin/src/rules/prefer-optional-chain-utils/analyzeChain.ts index 9ddd0fcb3e1b..dfa6b5406c2d 100644 --- a/packages/eslint-plugin/src/rules/prefer-optional-chain-utils/analyzeChain.ts +++ b/packages/eslint-plugin/src/rules/prefer-optional-chain-utils/analyzeChain.ts @@ -424,6 +424,7 @@ function getReportDescriptor( const commentsBefore = sourceCode.getCommentsBefore(chain[1].node); const commentsAfter = sourceCode.getCommentsAfter(nodeBeforeTheComment); if (commentsBefore.length || commentsAfter.length) { + useSuggestionFixer = true; const indentationCount = node.loc.start.column; const indentation = ' '.repeat(indentationCount); const newLineIndentation = `\n${indentation}`; diff --git a/packages/eslint-plugin/tests/rules/prefer-optional-chain/prefer-optional-chain.test.ts b/packages/eslint-plugin/tests/rules/prefer-optional-chain/prefer-optional-chain.test.ts index 87af54263549..06d6974fcf7c 100644 --- a/packages/eslint-plugin/tests/rules/prefer-optional-chain/prefer-optional-chain.test.ts +++ b/packages/eslint-plugin/tests/rules/prefer-optional-chain/prefer-optional-chain.test.ts @@ -743,9 +743,13 @@ describe('if block with a single statment matches part of the condition', () => // after expression } `, - errors: [{ messageId: 'preferOptionalChain' }], - options: [{ allowIfStatements: true }], - output: ` + errors: [ + { + messageId: 'preferOptionalChain', + suggestions: [ + { + messageId: 'optionalChainSuggest', + output: ` // before expression /** multi line */ @@ -754,6 +758,11 @@ describe('if block with a single statment matches part of the condition', () => /* after semicolon */ // after expression `, + }, + ], + }, + ], + options: [{ allowIfStatements: true }], }, { code: ` @@ -763,13 +772,22 @@ describe('if block with a single statment matches part of the condition', () => foo.bar(); } `, - errors: [{ messageId: 'preferOptionalChain' }], - options: [{ allowIfStatements: true }], - output: ` + errors: [ + { + messageId: 'preferOptionalChain', + suggestions: [ + { + messageId: 'optionalChainSuggest', + output: ` // comment1 // comment2 foo?.bar(); `, + }, + ], + }, + ], + options: [{ allowIfStatements: true }], }, { code: ` @@ -777,12 +795,21 @@ describe('if block with a single statment matches part of the condition', () => foo.bar(); // comment } `, - errors: [{ messageId: 'preferOptionalChain' }], - options: [{ allowIfStatements: true }], - output: ` + errors: [ + { + messageId: 'preferOptionalChain', + suggestions: [ + { + messageId: 'optionalChainSuggest', + output: ` foo?.bar(); // comment `, + }, + ], + }, + ], + options: [{ allowIfStatements: true }], }, { code: ` @@ -792,13 +819,22 @@ describe('if block with a single statment matches part of the condition', () => // comment 2 } `, - errors: [{ messageId: 'preferOptionalChain' }], - options: [{ allowIfStatements: true }], - output: ` + errors: [ + { + messageId: 'preferOptionalChain', + suggestions: [ + { + messageId: 'optionalChainSuggest', + output: ` foo?.bar(); // comment 1 // comment 2 `, + }, + ], + }, + ], + options: [{ allowIfStatements: true }], }, { code: ` From 6465402fb148b91cd1d7630e3e2afaedd28b613e Mon Sep 17 00:00:00 2001 From: Omri Luzon Date: Fri, 15 Nov 2024 14:28:22 +0200 Subject: [PATCH 28/38] CR: add types to tests --- .../prefer-optional-chain.test.ts | 155 +++++++++++++++--- 1 file changed, 132 insertions(+), 23 deletions(-) diff --git a/packages/eslint-plugin/tests/rules/prefer-optional-chain/prefer-optional-chain.test.ts b/packages/eslint-plugin/tests/rules/prefer-optional-chain/prefer-optional-chain.test.ts index 06d6974fcf7c..1780fb93365c 100644 --- a/packages/eslint-plugin/tests/rules/prefer-optional-chain/prefer-optional-chain.test.ts +++ b/packages/eslint-plugin/tests/rules/prefer-optional-chain/prefer-optional-chain.test.ts @@ -673,67 +673,148 @@ describe('if block with a single statment matches part of the condition', () => ruleTester.run('prefer-optional-chain', rule, { invalid: [ { - code: noFormat`if (foo) { foo.bar(); }`, + code: ` + declare const foo: undefined | { bar: () => void }; + if (foo) { + foo.bar(); + } + `, errors: [{ messageId: 'preferOptionalChain' }], options: [{ allowIfStatements: true }], - output: 'foo?.bar();', + output: ` + declare const foo: undefined | { bar: () => void }; + foo?.bar(); + `, }, { - code: noFormat`if (foo) { foo.bar() }`, + // eslint-disable-next-line @typescript-eslint/internal/plugin-test-formatting + code: ` + declare const foo: undefined | { bar: () => void }; + if (foo) { + foo.bar() + } + `, errors: [{ messageId: 'preferOptionalChain' }], options: [{ allowIfStatements: true }], - output: 'foo?.bar()', + output: ` + declare const foo: undefined | { bar: () => void }; + foo?.bar() + `, }, { - code: 'if (foo) foo.bar();', + code: ` + declare const foo: undefined | { bar: () => void }; + if (foo) foo.bar(); + `, errors: [{ messageId: 'preferOptionalChain' }], options: [{ allowIfStatements: true }], - output: 'foo?.bar();', + output: ` + declare const foo: undefined | { bar: () => void }; + foo?.bar(); + `, }, { - code: noFormat`if (foo) foo.bar()`, + // eslint-disable-next-line @typescript-eslint/internal/plugin-test-formatting + code: ` + declare const foo: undefined | { bar: () => void }; + if (foo) foo.bar() + `, errors: [{ messageId: 'preferOptionalChain' }], options: [{ allowIfStatements: true }], - output: 'foo?.bar()', + output: ` + declare const foo: undefined | { bar: () => void }; + foo?.bar() + `, }, { - code: noFormat`if (foo) { foo(); }`, + code: ` + declare const foo: undefined | { bar: () => void }; + if (foo) { + foo(); + } + `, errors: [{ messageId: 'preferOptionalChain' }], options: [{ allowIfStatements: true }], - output: 'foo?.();', + output: ` + declare const foo: undefined | { bar: () => void }; + foo?.(); + `, }, { - code: noFormat`if (foo) { foo[bar](); }`, + code: ` + declare const foo: undefined | { bar: () => void }; + declare const bar: 'bar'; + if (foo) { + foo[bar](); + } + `, errors: [{ messageId: 'preferOptionalChain' }], options: [{ allowIfStatements: true }], - output: 'foo?.[bar]();', + output: ` + declare const foo: undefined | { bar: () => void }; + declare const bar: 'bar'; + foo?.[bar](); + `, }, { - code: noFormat`if (foo[bar]) { foo[bar].baz(); }`, + code: ` + declare const foo: { bar?: { baz: () => void } }; + declare const bar: 'bar'; + if (foo[bar]) { + foo[bar].baz(); + } + `, errors: [{ messageId: 'preferOptionalChain' }], options: [{ allowIfStatements: true }], - output: 'foo[bar]?.baz();', + output: ` + declare const foo: { bar?: { baz: () => void } }; + declare const bar: 'bar'; + foo[bar]?.baz(); + `, }, { - code: noFormat`if (foo.bar.baz()) { foo.bar.baz().bazz(); }`, + code: ` + declare const foo: { bar: { baz: () => undefined | { bazz: () => void } } }; + if (foo.bar.baz()) { + foo.bar.baz().bazz(); + } + `, errors: [{ messageId: 'preferOptionalChain' }], options: [{ allowIfStatements: true }], - output: 'foo.bar.baz()?.bazz();', + output: ` + declare const foo: { bar: { baz: () => undefined | { bazz: () => void } } }; + foo.bar.baz()?.bazz(); + `, }, { - code: noFormat`if (foo && foo.bar && foo.bar.baz) { foo.bar.baz.bazz(); }`, + code: ` + declare const foo: { bar: { baz: () => undefined | { bazz: () => void } } }; + if (foo && foo.bar && foo.bar.baz) { + foo.bar.baz.bazz(); + } + `, errors: [{ messageId: 'preferOptionalChain' }], options: [{ allowIfStatements: true }], - output: 'foo?.bar?.baz?.bazz();', + output: ` + declare const foo: { bar: { baz: () => undefined | { bazz: () => void } } }; + foo?.bar?.baz?.bazz(); + `, }, { - code: noFormat`if (foo) { foo.bar.baz && foo.bar.baz(); }`, + code: ` + if (foo) { + foo.bar.baz && foo.bar.baz(); + } + `, errors: [{ messageId: 'preferOptionalChain' }], options: [{ allowIfStatements: true }], - output: 'foo?.bar.baz?.();', + output: ` + foo?.bar.baz?.(); + `, }, { code: ` + declare const foo: undefined | { bar: () => void }; if (foo) { // before expression /** multi @@ -750,6 +831,7 @@ describe('if block with a single statment matches part of the condition', () => { messageId: 'optionalChainSuggest', output: ` + declare const foo: undefined | { bar: () => void }; // before expression /** multi line */ @@ -766,6 +848,7 @@ describe('if block with a single statment matches part of the condition', () => }, { code: ` + declare const foo: undefined | { bar: () => void }; if (foo) { // comment1 // comment2 @@ -779,6 +862,7 @@ describe('if block with a single statment matches part of the condition', () => { messageId: 'optionalChainSuggest', output: ` + declare const foo: undefined | { bar: () => void }; // comment1 // comment2 foo?.bar(); @@ -791,6 +875,7 @@ describe('if block with a single statment matches part of the condition', () => }, { code: ` + declare const foo: undefined | { bar: () => void }; if (foo) { foo.bar(); // comment } @@ -802,6 +887,7 @@ describe('if block with a single statment matches part of the condition', () => { messageId: 'optionalChainSuggest', output: ` + declare const foo: undefined | { bar: () => void }; foo?.bar(); // comment `, @@ -813,6 +899,7 @@ describe('if block with a single statment matches part of the condition', () => }, { code: ` + declare const foo: undefined | { bar: () => void }; if (foo) { foo.bar(); // comment 1 @@ -826,6 +913,7 @@ describe('if block with a single statment matches part of the condition', () => { messageId: 'optionalChainSuggest', output: ` + declare const foo: undefined | { bar: () => void }; foo?.bar(); // comment 1 // comment 2 @@ -853,19 +941,35 @@ describe('if block with a single statment matches part of the condition', () => ], valid: [ { - code: noFormat`if (foo) { foo.bar(); }`, + code: ` + declare const foo: undefined | { bar: () => void }; + if (foo) { + foo.bar(); + } + `, options: [{ allowIfStatements: false }], }, { - code: noFormat`if (foo) { foo.bar; }`, + code: ` + declare const foo: undefined | { bar: () => void }; + if (foo) { + foo.bar; + } + `, options: [{ allowIfStatements: true }], }, { - code: noFormat`if (foo) { foo.bar?.baz; }`, + code: ` + declare const foo: undefined | { bar: { baz: unknown } }; + if (foo) { + foo.bar?.baz; + } + `, options: [{ allowIfStatements: true }], }, { code: ` + declare const foo: undefined | { bar: VoidFunction; baz: VoidFunction }; if (foo) { foo.bar(); foo.baz(); @@ -895,6 +999,7 @@ describe('if block with a single statment matches part of the condition', () => }, { code: ` + declare const x: null | { bar: () => string }; if (foo && typeof window === 'undefined') { foo.bar(); } @@ -903,6 +1008,7 @@ describe('if block with a single statment matches part of the condition', () => }, { code: ` + declare const foo: undefined | { bar: () => void }; if (foo) { typeof window === 'undefined' && foo.bar(); } @@ -911,6 +1017,7 @@ describe('if block with a single statment matches part of the condition', () => }, { code: ` + declare const foo: undefined | { bar: () => void }; if (foo) { foo.bar() && typeof window === 'undefined'; } @@ -919,6 +1026,7 @@ describe('if block with a single statment matches part of the condition', () => }, { code: ` + declare const foo: undefined | { bar: () => void }; if (foo) { if (foo.bar) { console.log(window); @@ -929,6 +1037,7 @@ describe('if block with a single statment matches part of the condition', () => }, { code: ` + declare const foo: false | { bar: () => void }; if (foo) { foo.bar(); } From f3f98a9d43bc4dbbea5a10527a27c44b625d67bf Mon Sep 17 00:00:00 2001 From: Omri Luzon Date: Sat, 11 Jan 2025 16:00:04 +0200 Subject: [PATCH 29/38] recreate comments only if the exist in ranges that are being removed --- .../analyzeChain.ts | 42 +++++++++++- .../prefer-optional-chain.test.ts | 64 ++++++------------- 2 files changed, 62 insertions(+), 44 deletions(-) diff --git a/packages/eslint-plugin/src/rules/prefer-optional-chain-utils/analyzeChain.ts b/packages/eslint-plugin/src/rules/prefer-optional-chain-utils/analyzeChain.ts index dfa6b5406c2d..cbed00ee6274 100644 --- a/packages/eslint-plugin/src/rules/prefer-optional-chain-utils/analyzeChain.ts +++ b/packages/eslint-plugin/src/rules/prefer-optional-chain-utils/analyzeChain.ts @@ -421,9 +421,31 @@ function getReportDescriptor( const nodeBeforeTheComment = chainEndedWithSemicolon ? lastOperand.node.parent : lastOperand.node; + const commentsBefore = sourceCode.getCommentsBefore(chain[1].node); const commentsAfter = sourceCode.getCommentsAfter(nodeBeforeTheComment); - if (commentsBefore.length || commentsAfter.length) { + const tokenAfterNodeTest = sourceCode.getTokenAfter(node.test); + const tokenBeforeNodeTest = sourceCode.getTokenBefore(node.test); + const tokenAfterAfterNodeTest = + tokenAfterNodeTest && sourceCode.getTokenAfter(tokenAfterNodeTest); + const commentsBeforeNodeTest = tokenBeforeNodeTest + ? sourceCode.getCommentsBefore(tokenBeforeNodeTest) + : []; // if /* this */ (foo) + const beforeNodeTestComments = sourceCode.getCommentsBefore(node.test); // if (/* this */ foo) + const afterNodeTestComments1 = sourceCode.getCommentsAfter(node.test); // if (foo /* this */) + const afterNodeTestComments2 = tokenAfterAfterNodeTest + ? sourceCode.getCommentsBefore(tokenAfterAfterNodeTest) + : []; // if (foo) /* this */ + + const commentsToReloacte = [ + ...commentsBeforeNodeTest, + ...beforeNodeTestComments, + ...afterNodeTestComments1, + ...afterNodeTestComments2, + ]; + + if (commentsToReloacte.length) { + commentsBefore.unshift(...commentsToReloacte); useSuggestionFixer = true; const indentationCount = node.loc.start.column; const indentation = ' '.repeat(indentationCount); @@ -443,6 +465,24 @@ function getReportDescriptor( const commentsText = getTextFromCommentsArray(commentsAfter); newCode = `${newCode}\n${indentation}${commentsText}`; } + } else if (commentsBefore.length || commentsAfter.length) { + const indentationCount = node.loc.start.column; + const indentation = ' '.repeat(indentationCount); + function getTextFromCommentsArray(comments: TSESTree.Comment[]): string { + return sourceCode + .getText() + .slice(comments[0].range[0], comments[comments.length - 1].range[1]); + } + if (commentsBefore.length > 0) { + const commentsTextBeforeWithoutIndent = + getTextFromCommentsArray(commentsBefore); + newCode = `${commentsTextBeforeWithoutIndent}\n${indentation}${newCode}`; + } + if (commentsAfter.length > 0) { + const commentsTextAfterWithoutIndent = + getTextFromCommentsArray(commentsAfter); + newCode = `${newCode}\n${indentation}${commentsTextAfterWithoutIndent}`; + } } } diff --git a/packages/eslint-plugin/tests/rules/prefer-optional-chain/prefer-optional-chain.test.ts b/packages/eslint-plugin/tests/rules/prefer-optional-chain/prefer-optional-chain.test.ts index 1780fb93365c..f6279e04db21 100644 --- a/packages/eslint-plugin/tests/rules/prefer-optional-chain/prefer-optional-chain.test.ts +++ b/packages/eslint-plugin/tests/rules/prefer-optional-chain/prefer-optional-chain.test.ts @@ -824,34 +824,26 @@ describe('if block with a single statment matches part of the condition', () => // after expression } `, - errors: [ - { - messageId: 'preferOptionalChain', - suggestions: [ - { - messageId: 'optionalChainSuggest', - output: ` + errors: [{ messageId: 'preferOptionalChain' }], + options: [{ allowIfStatements: true }], + output: ` declare const foo: undefined | { bar: () => void }; // before expression - /** multi + /** multi line */ - // single-line + // single-line foo?.bar(); /* after semicolon */ - // after expression + // after expression `, - }, - ], - }, - ], - options: [{ allowIfStatements: true }], }, { + // eslint-disable-next-line @typescript-eslint/internal/plugin-test-formatting code: ` declare const foo: undefined | { bar: () => void }; - if (foo) { + if /* sneaky1 */ (/* sneaky2 */ foo /* sneaky3 */) /* sneaky4 */ { // comment1 - // comment2 + /* comment2 */ foo.bar(); } `, @@ -863,8 +855,12 @@ describe('if block with a single statment matches part of the condition', () => messageId: 'optionalChainSuggest', output: ` declare const foo: undefined | { bar: () => void }; + /* sneaky1 */ + /* sneaky2 */ + /* sneaky3 */ + /* sneaky4 */ // comment1 - // comment2 + /* comment2 */ foo?.bar(); `, }, @@ -880,22 +876,13 @@ describe('if block with a single statment matches part of the condition', () => foo.bar(); // comment } `, - errors: [ - { - messageId: 'preferOptionalChain', - suggestions: [ - { - messageId: 'optionalChainSuggest', - output: ` + errors: [{ messageId: 'preferOptionalChain' }], + options: [{ allowIfStatements: true }], + output: ` declare const foo: undefined | { bar: () => void }; foo?.bar(); // comment `, - }, - ], - }, - ], - options: [{ allowIfStatements: true }], }, { code: ` @@ -906,23 +893,14 @@ describe('if block with a single statment matches part of the condition', () => // comment 2 } `, - errors: [ - { - messageId: 'preferOptionalChain', - suggestions: [ - { - messageId: 'optionalChainSuggest', - output: ` + errors: [{ messageId: 'preferOptionalChain' }], + options: [{ allowIfStatements: true }], + output: ` declare const foo: undefined | { bar: () => void }; foo?.bar(); // comment 1 - // comment 2 + // comment 2 `, - }, - ], - }, - ], - options: [{ allowIfStatements: true }], }, { code: ` From 6671148d582297a00d093bc88322fe344c458945 Mon Sep 17 00:00:00 2001 From: Omri Luzon Date: Sat, 11 Jan 2025 16:04:34 +0200 Subject: [PATCH 30/38] CR: Fix rule descrition --- packages/eslint-plugin/src/rules/prefer-optional-chain.ts | 2 +- .../tests/schema-snapshots/prefer-optional-chain.shot | 4 ++-- 2 files changed, 3 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 f7a587c18c61..304cad6c0b31 100644 --- a/packages/eslint-plugin/src/rules/prefer-optional-chain.ts +++ b/packages/eslint-plugin/src/rules/prefer-optional-chain.ts @@ -51,7 +51,7 @@ export default createRule< allowIfStatements: { type: 'boolean', description: - 'Allow autofix to optional chain on if statements with a single call expression in the consequent.', + 'Whether to ignore `if` statements with a single expression in the consequent.', }, allowPotentiallyUnsafeFixesThatModifyTheReturnTypeIKnowWhatImDoing: { type: 'boolean', diff --git a/packages/eslint-plugin/tests/schema-snapshots/prefer-optional-chain.shot b/packages/eslint-plugin/tests/schema-snapshots/prefer-optional-chain.shot index 025fc570f7ed..cdf48c4a852a 100644 --- a/packages/eslint-plugin/tests/schema-snapshots/prefer-optional-chain.shot +++ b/packages/eslint-plugin/tests/schema-snapshots/prefer-optional-chain.shot @@ -9,7 +9,7 @@ exports[`Rule schemas should be convertible to TS types for documentation purpos "additionalProperties": false, "properties": { "allowIfStatements": { - "description": "Allow autofix to optional chain on if statements with a single call expression in the consequent.", + "description": "Whether to ignore `if` statements with a single expression in the consequent.", "type": "boolean" }, "allowPotentiallyUnsafeFixesThatModifyTheReturnTypeIKnowWhatImDoing": { @@ -54,7 +54,7 @@ exports[`Rule schemas should be convertible to TS types for documentation purpos type Options = [ { - /** Allow autofix to optional chain on if statements with a single call expression in the consequent. */ + /** Whether to ignore `if` statements with a single expression in the consequent. */ allowIfStatements?: boolean; /** Allow autofixers that will change the return type of the expression. This option is considered unsafe as it may break the build. */ allowPotentiallyUnsafeFixesThatModifyTheReturnTypeIKnowWhatImDoing?: boolean; From 310b56e6f06d0ee9cfdf9cec5cbc4121b0c85f0c Mon Sep 17 00:00:00 2001 From: Omri Luzon Date: Sat, 11 Jan 2025 16:09:11 +0200 Subject: [PATCH 31/38] CR: Fix ignored non-CallExpression --- .../src/rules/prefer-optional-chain.ts | 3 -- .../prefer-optional-chain.test.ts | 28 +++++++++++++------ 2 files changed, 19 insertions(+), 12 deletions(-) diff --git a/packages/eslint-plugin/src/rules/prefer-optional-chain.ts b/packages/eslint-plugin/src/rules/prefer-optional-chain.ts index 304cad6c0b31..501d46c82a12 100644 --- a/packages/eslint-plugin/src/rules/prefer-optional-chain.ts +++ b/packages/eslint-plugin/src/rules/prefer-optional-chain.ts @@ -176,9 +176,6 @@ export default createRule< currentChain.push(operand); } } else { - if (ifBodyExpression.type !== AST_NODE_TYPES.CallExpression) { - return; - } currentChain.push({ node: ifBodyExpression, type: OperandValidity.Valid, diff --git a/packages/eslint-plugin/tests/rules/prefer-optional-chain/prefer-optional-chain.test.ts b/packages/eslint-plugin/tests/rules/prefer-optional-chain/prefer-optional-chain.test.ts index f6279e04db21..ad3268c9b34d 100644 --- a/packages/eslint-plugin/tests/rules/prefer-optional-chain/prefer-optional-chain.test.ts +++ b/packages/eslint-plugin/tests/rules/prefer-optional-chain/prefer-optional-chain.test.ts @@ -916,34 +916,44 @@ describe('if block with a single statment matches part of the condition', () => foo?.bar?.baz?.bazz(); `, }, - ], - valid: [ { code: ` declare const foo: undefined | { bar: () => void }; if (foo) { - foo.bar(); + foo.bar; } `, - options: [{ allowIfStatements: false }], + errors: [{ messageId: 'preferOptionalChain' }], + options: [{ allowIfStatements: true }], + output: ` + declare const foo: undefined | { bar: () => void }; + foo?.bar; + `, }, { code: ` - declare const foo: undefined | { bar: () => void }; + declare const foo: undefined | { bar: { baz: unknown } }; if (foo) { - foo.bar; + foo.bar?.baz; } `, + errors: [{ messageId: 'preferOptionalChain' }], options: [{ allowIfStatements: true }], + output: ` + declare const foo: undefined | { bar: { baz: unknown } }; + foo?.bar?.baz; + `, }, + ], + valid: [ { code: ` - declare const foo: undefined | { bar: { baz: unknown } }; + declare const foo: undefined | { bar: () => void }; if (foo) { - foo.bar?.baz; + foo.bar(); } `, - options: [{ allowIfStatements: true }], + options: [{ allowIfStatements: false }], }, { code: ` From 36e306fd3611e46ff84a6a5043d9b9b8260665e3 Mon Sep 17 00:00:00 2001 From: Omri Luzon Date: Sat, 11 Jan 2025 16:11:09 +0200 Subject: [PATCH 32/38] CR: Revert change to `context.sourceCode` input --- .../src/rules/prefer-optional-chain-utils/analyzeChain.ts | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/packages/eslint-plugin/src/rules/prefer-optional-chain-utils/analyzeChain.ts b/packages/eslint-plugin/src/rules/prefer-optional-chain-utils/analyzeChain.ts index cbed00ee6274..9d6e4adbb152 100644 --- a/packages/eslint-plugin/src/rules/prefer-optional-chain-utils/analyzeChain.ts +++ b/packages/eslint-plugin/src/rules/prefer-optional-chain-utils/analyzeChain.ts @@ -246,17 +246,13 @@ function getReportRange( } function getReportDescriptor( - context: RuleContext< - PreferOptionalChainMessageIds, - [PreferOptionalChainOptions] - >, + sourceCode: SourceCode, parserServices: ParserServicesWithTypeInformation, node: TSESTree.Node, operator: '&&' | '||', options: PreferOptionalChainOptions, chain: ValidOperand[], ): ReportDescriptor { - const sourceCode = context.sourceCode; const lastOperand = chain[chain.length - 1]; let useSuggestionFixer: boolean; @@ -638,7 +634,7 @@ export function analyzeChain( options, subChainFlat.slice(0, -1).map(({ node }) => node), getReportDescriptor( - context, + context.sourceCode, parserServices, node, operator, From 6ccb9bee1b2e95b3c4f81161ec13ab3fa31a5ada Mon Sep 17 00:00:00 2001 From: Omri Luzon Date: Sat, 11 Jan 2025 16:31:31 +0200 Subject: [PATCH 33/38] fix coverage and bug with comment included twice --- .../analyzeChain.ts | 7 ++++--- .../prefer-optional-chain.test.ts | 20 +++++++++++++++++++ 2 files changed, 24 insertions(+), 3 deletions(-) diff --git a/packages/eslint-plugin/src/rules/prefer-optional-chain-utils/analyzeChain.ts b/packages/eslint-plugin/src/rules/prefer-optional-chain-utils/analyzeChain.ts index 9d6e4adbb152..7e23d7c243c5 100644 --- a/packages/eslint-plugin/src/rules/prefer-optional-chain-utils/analyzeChain.ts +++ b/packages/eslint-plugin/src/rules/prefer-optional-chain-utils/analyzeChain.ts @@ -429,9 +429,10 @@ function getReportDescriptor( : []; // if /* this */ (foo) const beforeNodeTestComments = sourceCode.getCommentsBefore(node.test); // if (/* this */ foo) const afterNodeTestComments1 = sourceCode.getCommentsAfter(node.test); // if (foo /* this */) - const afterNodeTestComments2 = tokenAfterAfterNodeTest - ? sourceCode.getCommentsBefore(tokenAfterAfterNodeTest) - : []; // if (foo) /* this */ + const afterNodeTestComments2 = + tokenAfterAfterNodeTest?.type === AST_TOKEN_TYPES.Punctuator + ? sourceCode.getCommentsBefore(tokenAfterAfterNodeTest) + : []; // if (foo) /* this */ const commentsToReloacte = [ ...commentsBeforeNodeTest, diff --git a/packages/eslint-plugin/tests/rules/prefer-optional-chain/prefer-optional-chain.test.ts b/packages/eslint-plugin/tests/rules/prefer-optional-chain/prefer-optional-chain.test.ts index ad3268c9b34d..7016c961a855 100644 --- a/packages/eslint-plugin/tests/rules/prefer-optional-chain/prefer-optional-chain.test.ts +++ b/packages/eslint-plugin/tests/rules/prefer-optional-chain/prefer-optional-chain.test.ts @@ -845,6 +845,8 @@ describe('if block with a single statment matches part of the condition', () => // comment1 /* comment2 */ foo.bar(); + // comment3 + /* comment4 */ } `, errors: [ @@ -862,6 +864,8 @@ describe('if block with a single statment matches part of the condition', () => // comment1 /* comment2 */ foo?.bar(); + // comment3 + /* comment4 */ `, }, ], @@ -869,6 +873,22 @@ describe('if block with a single statment matches part of the condition', () => ], options: [{ allowIfStatements: true }], }, + + { + // eslint-disable-next-line @typescript-eslint/internal/plugin-test-formatting + code: ` + declare const foo: undefined | { bar: () => void }; + if (foo) /* sneaky */ + foo.bar(); + `, + errors: [{ messageId: 'preferOptionalChain' }], + options: [{ allowIfStatements: true }], + output: ` + declare const foo: undefined | { bar: () => void }; + /* sneaky */ + foo?.bar(); + `, + }, { code: ` declare const foo: undefined | { bar: () => void }; From bf84196ee1ac7f37c685988722ac22f58a244809 Mon Sep 17 00:00:00 2001 From: Omri Luzon Date: Sat, 11 Jan 2025 17:00:31 +0200 Subject: [PATCH 34/38] update snapshot --- .../tests/schema-snapshots/prefer-optional-chain.shot | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/eslint-plugin/tests/schema-snapshots/prefer-optional-chain.shot b/packages/eslint-plugin/tests/schema-snapshots/prefer-optional-chain.shot index cdf48c4a852a..1a1cc308905d 100644 --- a/packages/eslint-plugin/tests/schema-snapshots/prefer-optional-chain.shot +++ b/packages/eslint-plugin/tests/schema-snapshots/prefer-optional-chain.shot @@ -9,7 +9,7 @@ exports[`Rule schemas should be convertible to TS types for documentation purpos "additionalProperties": false, "properties": { "allowIfStatements": { - "description": "Whether to ignore `if` statements with a single expression in the consequent.", + "description": "Whether to ignore \`if\` statements with a single expression in the consequent.", "type": "boolean" }, "allowPotentiallyUnsafeFixesThatModifyTheReturnTypeIKnowWhatImDoing": { @@ -54,7 +54,7 @@ exports[`Rule schemas should be convertible to TS types for documentation purpos type Options = [ { - /** Whether to ignore `if` statements with a single expression in the consequent. */ + /** Whether to ignore \`if\` statements with a single expression in the consequent. */ allowIfStatements?: boolean; /** Allow autofixers that will change the return type of the expression. This option is considered unsafe as it may break the build. */ allowPotentiallyUnsafeFixesThatModifyTheReturnTypeIKnowWhatImDoing?: boolean; From bb2b2aa261651a26102d66447e935d829cb424a7 Mon Sep 17 00:00:00 2001 From: Omri Luzon Date: Sat, 11 Jan 2025 17:43:30 +0200 Subject: [PATCH 35/38] change rule name, invert value and fix tests --- .../docs/rules/prefer-optional-chain.mdx | 52 +++++++-------- .../PreferOptionalChainOptions.ts | 2 +- .../src/rules/prefer-optional-chain.ts | 14 ++-- .../prefer-optional-chain.shot | 64 +++++++++---------- .../prefer-optional-chain.test.ts | 54 ++++++++-------- .../prefer-optional-chain.shot | 12 ++-- 6 files changed, 99 insertions(+), 99 deletions(-) diff --git a/packages/eslint-plugin/docs/rules/prefer-optional-chain.mdx b/packages/eslint-plugin/docs/rules/prefer-optional-chain.mdx index 4de1674f5c9d..815d31c3460c 100644 --- a/packages/eslint-plugin/docs/rules/prefer-optional-chain.mdx +++ b/packages/eslint-plugin/docs/rules/prefer-optional-chain.mdx @@ -89,32 +89,6 @@ This style of code isn't super common - which means having this option set to `t When this option is `false` unsafe cases will have suggestion fixers provided instead of auto-fixers - meaning you can manually apply the fix using your IDE tooling. -### `allowIfStatements` - -{/* insert option description */} - -Examples of code for this rule with `{ allowIfStatements: true }`: - - - - - -```ts option='{ "allowIfStatements": true }' -if (foo) { - foo.bar(); -} -``` - - - - -```ts option='{ "allowIfStatements": false }' -foo?.bar(); -``` - - - - ### `checkAny` {/* insert option description */} @@ -291,6 +265,32 @@ thing && thing.toString(); +### `ignoreIfStatements` + +{/* insert option description */} + +Examples of code for this rule with `{ ignoreIfStatements: false }`: + + + + + +```ts option='{ "ignoreIfStatements": false }' +if (foo) { + foo.bar(); +} +``` + + + + +```ts option='{ "ignoreIfStatements": true }' +foo?.bar(); +``` + + + + ### `requireNullish` {/* insert option description */} diff --git a/packages/eslint-plugin/src/rules/prefer-optional-chain-utils/PreferOptionalChainOptions.ts b/packages/eslint-plugin/src/rules/prefer-optional-chain-utils/PreferOptionalChainOptions.ts index 510d896bc052..6892729476c6 100644 --- a/packages/eslint-plugin/src/rules/prefer-optional-chain-utils/PreferOptionalChainOptions.ts +++ b/packages/eslint-plugin/src/rules/prefer-optional-chain-utils/PreferOptionalChainOptions.ts @@ -3,7 +3,7 @@ export type PreferOptionalChainMessageIds = | 'preferOptionalChain'; export interface PreferOptionalChainOptions { - allowIfStatements?: boolean; + ignoreIfStatements?: boolean; allowPotentiallyUnsafeFixesThatModifyTheReturnTypeIKnowWhatImDoing?: boolean; checkAny?: boolean; checkBigInt?: boolean; diff --git a/packages/eslint-plugin/src/rules/prefer-optional-chain.ts b/packages/eslint-plugin/src/rules/prefer-optional-chain.ts index 501d46c82a12..f7ea45d55ad8 100644 --- a/packages/eslint-plugin/src/rules/prefer-optional-chain.ts +++ b/packages/eslint-plugin/src/rules/prefer-optional-chain.ts @@ -48,11 +48,6 @@ export default createRule< type: 'object', additionalProperties: false, properties: { - allowIfStatements: { - type: 'boolean', - description: - 'Whether to ignore `if` statements with a single expression in the consequent.', - }, allowPotentiallyUnsafeFixesThatModifyTheReturnTypeIKnowWhatImDoing: { type: 'boolean', description: @@ -88,6 +83,11 @@ export default createRule< description: 'Check operands that are typed as `unknown` when inspecting "loose boolean" operands.', }, + ignoreIfStatements: { + type: 'boolean', + description: + 'Whether to ignore `if` statements with a single expression in the consequent.', + }, requireNullish: { type: 'boolean', description: @@ -99,7 +99,6 @@ export default createRule< }, defaultOptions: [ { - allowIfStatements: false, allowPotentiallyUnsafeFixesThatModifyTheReturnTypeIKnowWhatImDoing: false, checkAny: true, checkBigInt: true, @@ -107,6 +106,7 @@ export default createRule< checkNumber: true, checkString: true, checkUnknown: true, + ignoreIfStatements: false, requireNullish: false, }, ], @@ -120,7 +120,7 @@ export default createRule< 'IfStatement[consequent.type=BlockStatement][consequent.body.length=1], IfStatement[consequent.type=ExpressionStatement]'( node: TSESTree.IfStatement, ): void { - if (!options.allowIfStatements || node.alternate) { + if (options.ignoreIfStatements || node.alternate) { return; } const ifBodyStatement = diff --git a/packages/eslint-plugin/tests/docs-eslint-output-snapshots/prefer-optional-chain.shot b/packages/eslint-plugin/tests/docs-eslint-output-snapshots/prefer-optional-chain.shot index b29dee9c5293..ed292e5fd841 100644 --- a/packages/eslint-plugin/tests/docs-eslint-output-snapshots/prefer-optional-chain.shot +++ b/packages/eslint-plugin/tests/docs-eslint-output-snapshots/prefer-optional-chain.shot @@ -72,27 +72,6 @@ acceptsBoolean(foo?.bar); exports[`Validating rule docs prefer-optional-chain.mdx code examples ESLint output 4`] = ` "Incorrect -Options: { "allowIfStatements": true } - -if (foo) { -~~~~~~~~~~ Prefer using an optional chain expression instead, as it's more concise and easier to read. - foo.bar(); -~~~~~~~~~~~~ -} -~ -" -`; - -exports[`Validating rule docs prefer-optional-chain.mdx code examples ESLint output 5`] = ` -"Correct -Options: { "allowIfStatements": false } - -foo?.bar(); -" -`; - -exports[`Validating rule docs prefer-optional-chain.mdx code examples ESLint output 6`] = ` -"Incorrect Options: { "checkAny": true } declare const thing: any; @@ -102,7 +81,7 @@ thing && thing.toString(); " `; -exports[`Validating rule docs prefer-optional-chain.mdx code examples ESLint output 7`] = ` +exports[`Validating rule docs prefer-optional-chain.mdx code examples ESLint output 5`] = ` "Correct Options: { "checkAny": false } @@ -112,7 +91,7 @@ thing && thing.toString(); " `; -exports[`Validating rule docs prefer-optional-chain.mdx code examples ESLint output 8`] = ` +exports[`Validating rule docs prefer-optional-chain.mdx code examples ESLint output 6`] = ` "Incorrect Options: { "checkUnknown": true } @@ -123,7 +102,7 @@ thing && thing.toString(); " `; -exports[`Validating rule docs prefer-optional-chain.mdx code examples ESLint output 9`] = ` +exports[`Validating rule docs prefer-optional-chain.mdx code examples ESLint output 7`] = ` "Correct Options: { "checkUnknown": false } @@ -133,7 +112,7 @@ thing && thing.toString(); " `; -exports[`Validating rule docs prefer-optional-chain.mdx code examples ESLint output 10`] = ` +exports[`Validating rule docs prefer-optional-chain.mdx code examples ESLint output 8`] = ` "Incorrect Options: { "checkString": true } @@ -144,7 +123,7 @@ thing && thing.toString(); " `; -exports[`Validating rule docs prefer-optional-chain.mdx code examples ESLint output 11`] = ` +exports[`Validating rule docs prefer-optional-chain.mdx code examples ESLint output 9`] = ` "Correct Options: { "checkString": false } @@ -154,7 +133,7 @@ thing && thing.toString(); " `; -exports[`Validating rule docs prefer-optional-chain.mdx code examples ESLint output 12`] = ` +exports[`Validating rule docs prefer-optional-chain.mdx code examples ESLint output 10`] = ` "Incorrect Options: { "checkNumber": true } @@ -165,7 +144,7 @@ thing && thing.toString(); " `; -exports[`Validating rule docs prefer-optional-chain.mdx code examples ESLint output 13`] = ` +exports[`Validating rule docs prefer-optional-chain.mdx code examples ESLint output 11`] = ` "Correct Options: { "checkNumber": false } @@ -175,7 +154,7 @@ thing && thing.toString(); " `; -exports[`Validating rule docs prefer-optional-chain.mdx code examples ESLint output 14`] = ` +exports[`Validating rule docs prefer-optional-chain.mdx code examples ESLint output 12`] = ` "Incorrect Options: { "checkBoolean": true } @@ -186,7 +165,7 @@ thing && thing.toString(); " `; -exports[`Validating rule docs prefer-optional-chain.mdx code examples ESLint output 15`] = ` +exports[`Validating rule docs prefer-optional-chain.mdx code examples ESLint output 13`] = ` "Correct Options: { "checkBoolean": false } @@ -196,7 +175,7 @@ thing && thing.toString(); " `; -exports[`Validating rule docs prefer-optional-chain.mdx code examples ESLint output 16`] = ` +exports[`Validating rule docs prefer-optional-chain.mdx code examples ESLint output 14`] = ` "Incorrect Options: { "checkBigInt": true } @@ -207,7 +186,7 @@ thing && thing.toString(); " `; -exports[`Validating rule docs prefer-optional-chain.mdx code examples ESLint output 17`] = ` +exports[`Validating rule docs prefer-optional-chain.mdx code examples ESLint output 15`] = ` "Correct Options: { "checkBigInt": false } @@ -217,6 +196,27 @@ thing && thing.toString(); " `; +exports[`Validating rule docs prefer-optional-chain.mdx code examples ESLint output 16`] = ` +"Incorrect +Options: { "ignoreIfStatements": false } + +if (foo) { +~~~~~~~~~~ Prefer using an optional chain expression instead, as it's more concise and easier to read. + foo.bar(); +~~~~~~~~~~~~ +} +~ +" +`; + +exports[`Validating rule docs prefer-optional-chain.mdx code examples ESLint output 17`] = ` +"Correct +Options: { "ignoreIfStatements": true } + +foo?.bar(); +" +`; + exports[`Validating rule docs prefer-optional-chain.mdx code examples ESLint output 18`] = ` "Incorrect Options: { "requireNullish": true } diff --git a/packages/eslint-plugin/tests/rules/prefer-optional-chain/prefer-optional-chain.test.ts b/packages/eslint-plugin/tests/rules/prefer-optional-chain/prefer-optional-chain.test.ts index 7016c961a855..87695d909300 100644 --- a/packages/eslint-plugin/tests/rules/prefer-optional-chain/prefer-optional-chain.test.ts +++ b/packages/eslint-plugin/tests/rules/prefer-optional-chain/prefer-optional-chain.test.ts @@ -680,7 +680,7 @@ describe('if block with a single statment matches part of the condition', () => } `, errors: [{ messageId: 'preferOptionalChain' }], - options: [{ allowIfStatements: true }], + options: [{ ignoreIfStatements: false }], output: ` declare const foo: undefined | { bar: () => void }; foo?.bar(); @@ -695,7 +695,7 @@ describe('if block with a single statment matches part of the condition', () => } `, errors: [{ messageId: 'preferOptionalChain' }], - options: [{ allowIfStatements: true }], + options: [{ ignoreIfStatements: false }], output: ` declare const foo: undefined | { bar: () => void }; foo?.bar() @@ -707,7 +707,7 @@ describe('if block with a single statment matches part of the condition', () => if (foo) foo.bar(); `, errors: [{ messageId: 'preferOptionalChain' }], - options: [{ allowIfStatements: true }], + options: [{ ignoreIfStatements: false }], output: ` declare const foo: undefined | { bar: () => void }; foo?.bar(); @@ -720,7 +720,7 @@ describe('if block with a single statment matches part of the condition', () => if (foo) foo.bar() `, errors: [{ messageId: 'preferOptionalChain' }], - options: [{ allowIfStatements: true }], + options: [{ ignoreIfStatements: false }], output: ` declare const foo: undefined | { bar: () => void }; foo?.bar() @@ -734,7 +734,7 @@ describe('if block with a single statment matches part of the condition', () => } `, errors: [{ messageId: 'preferOptionalChain' }], - options: [{ allowIfStatements: true }], + options: [{ ignoreIfStatements: false }], output: ` declare const foo: undefined | { bar: () => void }; foo?.(); @@ -749,7 +749,7 @@ describe('if block with a single statment matches part of the condition', () => } `, errors: [{ messageId: 'preferOptionalChain' }], - options: [{ allowIfStatements: true }], + options: [{ ignoreIfStatements: false }], output: ` declare const foo: undefined | { bar: () => void }; declare const bar: 'bar'; @@ -765,7 +765,7 @@ describe('if block with a single statment matches part of the condition', () => } `, errors: [{ messageId: 'preferOptionalChain' }], - options: [{ allowIfStatements: true }], + options: [{ ignoreIfStatements: false }], output: ` declare const foo: { bar?: { baz: () => void } }; declare const bar: 'bar'; @@ -780,7 +780,7 @@ describe('if block with a single statment matches part of the condition', () => } `, errors: [{ messageId: 'preferOptionalChain' }], - options: [{ allowIfStatements: true }], + options: [{ ignoreIfStatements: false }], output: ` declare const foo: { bar: { baz: () => undefined | { bazz: () => void } } }; foo.bar.baz()?.bazz(); @@ -794,7 +794,7 @@ describe('if block with a single statment matches part of the condition', () => } `, errors: [{ messageId: 'preferOptionalChain' }], - options: [{ allowIfStatements: true }], + options: [{ ignoreIfStatements: false }], output: ` declare const foo: { bar: { baz: () => undefined | { bazz: () => void } } }; foo?.bar?.baz?.bazz(); @@ -807,7 +807,7 @@ describe('if block with a single statment matches part of the condition', () => } `, errors: [{ messageId: 'preferOptionalChain' }], - options: [{ allowIfStatements: true }], + options: [{ ignoreIfStatements: false }], output: ` foo?.bar.baz?.(); `, @@ -825,7 +825,7 @@ describe('if block with a single statment matches part of the condition', () => } `, errors: [{ messageId: 'preferOptionalChain' }], - options: [{ allowIfStatements: true }], + options: [{ ignoreIfStatements: false }], output: ` declare const foo: undefined | { bar: () => void }; // before expression @@ -871,7 +871,7 @@ describe('if block with a single statment matches part of the condition', () => ], }, ], - options: [{ allowIfStatements: true }], + options: [{ ignoreIfStatements: false }], }, { @@ -882,7 +882,7 @@ describe('if block with a single statment matches part of the condition', () => foo.bar(); `, errors: [{ messageId: 'preferOptionalChain' }], - options: [{ allowIfStatements: true }], + options: [{ ignoreIfStatements: false }], output: ` declare const foo: undefined | { bar: () => void }; /* sneaky */ @@ -897,7 +897,7 @@ describe('if block with a single statment matches part of the condition', () => } `, errors: [{ messageId: 'preferOptionalChain' }], - options: [{ allowIfStatements: true }], + options: [{ ignoreIfStatements: false }], output: ` declare const foo: undefined | { bar: () => void }; foo?.bar(); @@ -914,7 +914,7 @@ describe('if block with a single statment matches part of the condition', () => } `, errors: [{ messageId: 'preferOptionalChain' }], - options: [{ allowIfStatements: true }], + options: [{ ignoreIfStatements: false }], output: ` declare const foo: undefined | { bar: () => void }; foo?.bar(); @@ -930,7 +930,7 @@ describe('if block with a single statment matches part of the condition', () => } `, errors: [{ messageId: 'preferOptionalChain' }], - options: [{ allowIfStatements: true, requireNullish: true }], + options: [{ ignoreIfStatements: false, requireNullish: true }], output: ` declare const foo: undefined | { bar?: { baz?: { bazz: () => void } } }; foo?.bar?.baz?.bazz(); @@ -944,7 +944,7 @@ describe('if block with a single statment matches part of the condition', () => } `, errors: [{ messageId: 'preferOptionalChain' }], - options: [{ allowIfStatements: true }], + options: [{ ignoreIfStatements: false }], output: ` declare const foo: undefined | { bar: () => void }; foo?.bar; @@ -958,7 +958,7 @@ describe('if block with a single statment matches part of the condition', () => } `, errors: [{ messageId: 'preferOptionalChain' }], - options: [{ allowIfStatements: true }], + options: [{ ignoreIfStatements: false }], output: ` declare const foo: undefined | { bar: { baz: unknown } }; foo?.bar?.baz; @@ -973,7 +973,7 @@ describe('if block with a single statment matches part of the condition', () => foo.bar(); } `, - options: [{ allowIfStatements: false }], + options: [{ ignoreIfStatements: true }], }, { code: ` @@ -983,7 +983,7 @@ describe('if block with a single statment matches part of the condition', () => foo.baz(); } `, - options: [{ allowIfStatements: true }], + options: [{ ignoreIfStatements: false }], }, { code: ` @@ -994,7 +994,7 @@ describe('if block with a single statment matches part of the condition', () => // do something else } `, - options: [{ allowIfStatements: true }], + options: [{ ignoreIfStatements: false }], }, { code: ` @@ -1003,7 +1003,7 @@ describe('if block with a single statment matches part of the condition', () => x.a(); } `, - options: [{ allowIfStatements: true }], + options: [{ ignoreIfStatements: false }], }, { code: ` @@ -1012,7 +1012,7 @@ describe('if block with a single statment matches part of the condition', () => foo.bar(); } `, - options: [{ allowIfStatements: true }], + options: [{ ignoreIfStatements: false }], }, { code: ` @@ -1021,7 +1021,7 @@ describe('if block with a single statment matches part of the condition', () => typeof window === 'undefined' && foo.bar(); } `, - options: [{ allowIfStatements: true }], + options: [{ ignoreIfStatements: false }], }, { code: ` @@ -1030,7 +1030,7 @@ describe('if block with a single statment matches part of the condition', () => foo.bar() && typeof window === 'undefined'; } `, - options: [{ allowIfStatements: true }], + options: [{ ignoreIfStatements: false }], }, { code: ` @@ -1041,7 +1041,7 @@ describe('if block with a single statment matches part of the condition', () => } } `, - options: [{ allowIfStatements: true }], + options: [{ ignoreIfStatements: false }], }, { code: ` @@ -1050,7 +1050,7 @@ describe('if block with a single statment matches part of the condition', () => foo.bar(); } `, - options: [{ allowIfStatements: true, requireNullish: true }], + options: [{ ignoreIfStatements: false, requireNullish: true }], }, ], }); diff --git a/packages/eslint-plugin/tests/schema-snapshots/prefer-optional-chain.shot b/packages/eslint-plugin/tests/schema-snapshots/prefer-optional-chain.shot index 1a1cc308905d..f5b6bb552206 100644 --- a/packages/eslint-plugin/tests/schema-snapshots/prefer-optional-chain.shot +++ b/packages/eslint-plugin/tests/schema-snapshots/prefer-optional-chain.shot @@ -8,10 +8,6 @@ exports[`Rule schemas should be convertible to TS types for documentation purpos { "additionalProperties": false, "properties": { - "allowIfStatements": { - "description": "Whether to ignore \`if\` statements with a single expression in the consequent.", - "type": "boolean" - }, "allowPotentiallyUnsafeFixesThatModifyTheReturnTypeIKnowWhatImDoing": { "description": "Allow autofixers that will change the return type of the expression. This option is considered unsafe as it may break the build.", "type": "boolean" @@ -40,6 +36,10 @@ exports[`Rule schemas should be convertible to TS types for documentation purpos "description": "Check operands that are typed as \`unknown\` when inspecting \\"loose boolean\\" operands.", "type": "boolean" }, + "ignoreIfStatements": { + "description": "Whether to ignore \`if\` statements with a single expression in the consequent.", + "type": "boolean" + }, "requireNullish": { "description": "Skip operands that are not typed with \`null\` and/or \`undefined\` when inspecting \\"loose boolean\\" operands.", "type": "boolean" @@ -54,8 +54,6 @@ exports[`Rule schemas should be convertible to TS types for documentation purpos type Options = [ { - /** Whether to ignore \`if\` statements with a single expression in the consequent. */ - allowIfStatements?: boolean; /** Allow autofixers that will change the return type of the expression. This option is considered unsafe as it may break the build. */ allowPotentiallyUnsafeFixesThatModifyTheReturnTypeIKnowWhatImDoing?: boolean; /** Check operands that are typed as \`any\` when inspecting "loose boolean" operands. */ @@ -70,6 +68,8 @@ type Options = [ checkString?: boolean; /** Check operands that are typed as \`unknown\` when inspecting "loose boolean" operands. */ checkUnknown?: boolean; + /** Whether to ignore \`if\` statements with a single expression in the consequent. */ + ignoreIfStatements?: boolean; /** Skip operands that are not typed with \`null\` and/or \`undefined\` when inspecting "loose boolean" operands. */ requireNullish?: boolean; }, From c843a4d98ba959892f78562fdb74565147569a96 Mon Sep 17 00:00:00 2001 From: Omri Luzon Date: Sat, 11 Jan 2025 18:04:50 +0200 Subject: [PATCH 36/38] Fix internal lint error --- .../tests/lib/createProjectService.test.ts | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/packages/typescript-estree/tests/lib/createProjectService.test.ts b/packages/typescript-estree/tests/lib/createProjectService.test.ts index fa9684e6237a..8494ee14e473 100644 --- a/packages/typescript-estree/tests/lib/createProjectService.test.ts +++ b/packages/typescript-estree/tests/lib/createProjectService.test.ts @@ -26,11 +26,9 @@ jest.mock('typescript/lib/tsserverlibrary', () => ({ this.eventHandler = args[0].eventHandler; this.host = args[0].host; this.logger = args[0].logger; - if (this.eventHandler) { - this.eventHandler({ - eventName: 'projectLoadingStart', - } as ts.server.ProjectLoadingStartEvent); - } + this.eventHandler?.({ + eventName: 'projectLoadingStart', + } as ts.server.ProjectLoadingStartEvent); } }, }, From f16358f344a1c0cf2d7e4007e58c88b89e3b567f Mon Sep 17 00:00:00 2001 From: Omri Luzon Date: Sat, 18 Jan 2025 15:39:13 +0200 Subject: [PATCH 37/38] add non-null assertions because I couldn't find a way to make this code reachable for coverage --- .../prefer-optional-chain-utils/analyzeChain.ts | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/packages/eslint-plugin/src/rules/prefer-optional-chain-utils/analyzeChain.ts b/packages/eslint-plugin/src/rules/prefer-optional-chain-utils/analyzeChain.ts index 7e23d7c243c5..69f5ec2f3231 100644 --- a/packages/eslint-plugin/src/rules/prefer-optional-chain-utils/analyzeChain.ts +++ b/packages/eslint-plugin/src/rules/prefer-optional-chain-utils/analyzeChain.ts @@ -420,17 +420,19 @@ function getReportDescriptor( const commentsBefore = sourceCode.getCommentsBefore(chain[1].node); const commentsAfter = sourceCode.getCommentsAfter(nodeBeforeTheComment); - const tokenAfterNodeTest = sourceCode.getTokenAfter(node.test); - const tokenBeforeNodeTest = sourceCode.getTokenBefore(node.test); + // eslint-disable-next-line @typescript-eslint/no-non-null-assertion + const tokenAfterNodeTest = sourceCode.getTokenAfter(node.test)!; // if (foo) /* this */, there is always a token here + // eslint-disable-next-line @typescript-eslint/no-non-null-assertion + const tokenBeforeNodeTest = sourceCode.getTokenBefore(node.test)!; // if /* this */ (foo), there is always a token here const tokenAfterAfterNodeTest = - tokenAfterNodeTest && sourceCode.getTokenAfter(tokenAfterNodeTest); - const commentsBeforeNodeTest = tokenBeforeNodeTest - ? sourceCode.getCommentsBefore(tokenBeforeNodeTest) - : []; // if /* this */ (foo) + // eslint-disable-next-line @typescript-eslint/no-non-null-assertion + sourceCode.getTokenAfter(tokenAfterNodeTest)!; // if (foo) /* this */, there is always a token here + const commentsBeforeNodeTest = + sourceCode.getCommentsBefore(tokenBeforeNodeTest); // if /* this */ (foo) const beforeNodeTestComments = sourceCode.getCommentsBefore(node.test); // if (/* this */ foo) const afterNodeTestComments1 = sourceCode.getCommentsAfter(node.test); // if (foo /* this */) const afterNodeTestComments2 = - tokenAfterAfterNodeTest?.type === AST_TOKEN_TYPES.Punctuator + tokenAfterAfterNodeTest.type === AST_TOKEN_TYPES.Punctuator ? sourceCode.getCommentsBefore(tokenAfterAfterNodeTest) : []; // if (foo) /* this */ From ec6389b8e84dfb3c0eb7635a4a4f649ea8151754 Mon Sep 17 00:00:00 2001 From: Omri Luzon Date: Mon, 17 Feb 2025 18:33:58 +0200 Subject: [PATCH 38/38] Update comment --- 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 f7ea45d55ad8..bd65c769e7de 100644 --- a/packages/eslint-plugin/src/rules/prefer-optional-chain.ts +++ b/packages/eslint-plugin/src/rules/prefer-optional-chain.ts @@ -116,7 +116,7 @@ export default createRule< const seenLogicals = new Set(); return { - // specific handling for `if (foo) { foo.bar(); }` / `if (foo) foo.bar();` + // specific handling for `if (foo) { foo.bar }` / `if (foo) foo.bar` 'IfStatement[consequent.type=BlockStatement][consequent.body.length=1], IfStatement[consequent.type=ExpressionStatement]'( node: TSESTree.IfStatement, ): void {