From 99eb2ed9f20fa6708066ca407dd02e60f916fd80 Mon Sep 17 00:00:00 2001 From: Lucas Weng Date: Fri, 26 Jan 2024 23:13:52 +0800 Subject: [PATCH] fix(lint/useOptionalChain): unsafe fix incorrectly discards conditions in logical and chains --- CHANGELOG.md | 7 ++++ .../complexity/use_optional_chain.rs | 35 ++++++++++++++++--- .../useOptionalChain/logicalAndCases6.js | 5 ++- .../useOptionalChain/logicalAndCases6.js.snap | 27 ++++++++++++++ .../src/content/docs/internals/changelog.mdx | 7 ++++ 5 files changed, 76 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9b21a710a685..0b848c52df76 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -72,6 +72,13 @@ Read our [guidelines for writing a good changelog entry](https://github.com/biom - Fix [#1640](https://github.com/biomejs/biome/issues/1640). [useEnumInitializers](https://biomejs.dev/linter/rules/use-enum-initializers) code action now generates valid code when last member has a comment but no comma. Contributed by @kalleep +- Fix [#1656](https://github.com/biomejs/biome/issues/1656). [useOptionalChain](https://biomejs.dev/linter/rules/use-optional-chain/) code action now correctly handles logical and chains where methods with the same name are invoked with different arguments: + ```diff + - tags·&&·tags.includes('a')·&&·tags.includes('b') + + tags?.includes('a') && tags.includes('b') + ``` + Contributed by @lucasweng + ### Parser ## 1.5.3 (2024-01-22) diff --git a/crates/biome_js_analyze/src/analyzers/complexity/use_optional_chain.rs b/crates/biome_js_analyze/src/analyzers/complexity/use_optional_chain.rs index 5380405ffd32..3a1741874683 100644 --- a/crates/biome_js_analyze/src/analyzers/complexity/use_optional_chain.rs +++ b/crates/biome_js_analyze/src/analyzers/complexity/use_optional_chain.rs @@ -399,13 +399,27 @@ impl LogicalAndChain { Ordering::Greater => LogicalAndChainOrdering::SubChain, }; for (main_expression, branch_expression) in self.buf.iter().zip(&other.buf) { - let (main_expression, branch_expression) = match (&main_expression, &branch_expression) - { + let ( + main_expression, + branch_expression, + main_call_expression_arguments, + branch_call_expression_arguments, + ) = match (&main_expression, &branch_expression) { ( AnyJsExpression::JsCallExpression(main_expression), AnyJsExpression::JsCallExpression(branch_expression), - ) => (main_expression.callee()?, branch_expression.callee()?), - _ => (main_expression.clone(), branch_expression.clone()), + ) => ( + main_expression.callee()?, + branch_expression.callee()?, + Some(main_expression.arguments()?), + Some(branch_expression.arguments()?), + ), + _ => ( + main_expression.clone(), + branch_expression.clone(), + None, + None, + ), }; let (main_value_token, branch_value_token) = match (main_expression, branch_expression) { @@ -453,6 +467,19 @@ impl LogicalAndChain { }; if main_value_token.token_text_trimmed() != branch_value_token.token_text_trimmed() { return Ok(LogicalAndChainOrdering::Different); + } else if main_call_expression_arguments.is_some() + && branch_call_expression_arguments.is_some() + { + // When comparing chains of call expressions with the same name, + // e.g., `foo.bar('a') && foo.bar('b')`, + // they should be considered different even if `main_value_token` + // and `branch_value_token` are the same. + // Therefore, we need to check their arguments here. + if main_call_expression_arguments.unwrap().args().text() + != branch_call_expression_arguments.unwrap().args().text() + { + return Ok(LogicalAndChainOrdering::Different); + } } } Ok(chain_ordering) diff --git a/crates/biome_js_analyze/tests/specs/complexity/useOptionalChain/logicalAndCases6.js b/crates/biome_js_analyze/tests/specs/complexity/useOptionalChain/logicalAndCases6.js index d007a2cb11bf..edfbf605ac57 100644 --- a/crates/biome_js_analyze/tests/specs/complexity/useOptionalChain/logicalAndCases6.js +++ b/crates/biome_js_analyze/tests/specs/complexity/useOptionalChain/logicalAndCases6.js @@ -61,4 +61,7 @@ foo.bar && foo.bar?.(); foo && foo.bar && /*0*/foo/*1*/./*2*/bar/*3*/./*4*/baz/*5*/; foo && foo[bar] && /*0*/foo/*1*/[/*2*/bar/*3*/]/*4*/[/*5*/baz/*6*/]/*7*/; -foo && foo[bar] && /*0*/foo/*1*/?./*2*/[/*3*/bar/*4*/]/*5*/?./*6*/[/*7*/baz/*8*/]/*9*/; \ No newline at end of file +foo && foo[bar] && /*0*/foo/*1*/?./*2*/[/*3*/bar/*4*/]/*5*/?./*6*/[/*7*/baz/*8*/]/*9*/; + +// call expressions with the same member name but different arguments +foo && foo.bar('a') && foo.bar('b') diff --git a/crates/biome_js_analyze/tests/specs/complexity/useOptionalChain/logicalAndCases6.js.snap b/crates/biome_js_analyze/tests/specs/complexity/useOptionalChain/logicalAndCases6.js.snap index f2d58ca979ff..12c9c959dcab 100644 --- a/crates/biome_js_analyze/tests/specs/complexity/useOptionalChain/logicalAndCases6.js.snap +++ b/crates/biome_js_analyze/tests/specs/complexity/useOptionalChain/logicalAndCases6.js.snap @@ -68,6 +68,10 @@ foo && foo.bar && /*0*/foo/*1*/./*2*/bar/*3*/./*4*/baz/*5*/; foo && foo[bar] && /*0*/foo/*1*/[/*2*/bar/*3*/]/*4*/[/*5*/baz/*6*/]/*7*/; foo && foo[bar] && /*0*/foo/*1*/?./*2*/[/*3*/bar/*4*/]/*5*/?./*6*/[/*7*/baz/*8*/]/*9*/; + +// call expressions with the same member name but different arguments +foo && foo.bar('a') && foo.bar('b') + ``` # Diagnostics @@ -858,6 +862,8 @@ logicalAndCases6.js:64:1 lint/complexity/useOptionalChain FIXABLE ━━━━ 63 │ > 64 │ foo && foo[bar] && /*0*/foo/*1*/?./*2*/[/*3*/bar/*4*/]/*5*/?./*6*/[/*7*/baz/*8*/]/*9*/; │ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + 65 │ + 66 │ // call expressions with the same member name but different arguments i Unsafe fix: Change to an optional chain. @@ -866,4 +872,25 @@ logicalAndCases6.js:64:1 lint/complexity/useOptionalChain FIXABLE ━━━━ ``` +``` +logicalAndCases6.js:67:1 lint/complexity/useOptionalChain FIXABLE ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + + ! Change to an optional chain. + + 66 │ // call expressions with the same member name but different arguments + > 67 │ foo && foo.bar('a') && foo.bar('b') + │ ^^^^^^^^^^^^^^^^^^^ + 68 │ + + i Unsafe fix: Change to an optional chain. + + 65 65 │ + 66 66 │ // call expressions with the same member name but different arguments + 67 │ - foo·&&·foo.bar('a')·&&·foo.bar('b') + 67 │ + foo?.bar('a')·&&·foo.bar('b') + 68 68 │ + + +``` + diff --git a/website/src/content/docs/internals/changelog.mdx b/website/src/content/docs/internals/changelog.mdx index 9bb2ef04b9bf..c6be5bf13bdc 100644 --- a/website/src/content/docs/internals/changelog.mdx +++ b/website/src/content/docs/internals/changelog.mdx @@ -78,6 +78,13 @@ Read our [guidelines for writing a good changelog entry](https://github.com/biom - Fix [#1640](https://github.com/biomejs/biome/issues/1640). [useEnumInitializers](https://biomejs.dev/linter/rules/use-enum-initializers) code action now generates valid code when last member has a comment but no comma. Contributed by @kalleep +- Fix [#1656](https://github.com/biomejs/biome/issues/1656). [useOptionalChain](https://biomejs.dev/linter/rules/use-optional-chain/) code action now correctly handles logical and chains where methods with the same name are invoked with different arguments: + ```diff + - tags·&&·tags.includes('a')·&&·tags.includes('b') + + tags?.includes('a') && tags.includes('b') + ``` + Contributed by @lucasweng + ### Parser ## 1.5.3 (2024-01-22)