fix(lint/useOptionalChain): unsafe fix incorrectly discards condition…

…s in logical and chains (biomejs#1685)
lucasweng · Jan 26, 2024 · 82917c7 · 82917c7
1 parent 012ff58
commit 82917c7
Show file tree

Hide file tree

Showing 5 changed files with 76 additions and 5 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -82,6 +82,13 @@ Read our [guidelines for writing a good changelog entry](https://github.com/biom
 
 - Fix [#1653](https://github.com/biomejs/biome/issues/1653). Handle a shorthand value in `useForOf` to avoid the false-positive case. Contributed by @togami2864
 
+- 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
@@ -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
@@ -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*/;
+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
@@ -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
@@ -88,6 +88,13 @@ Read our [guidelines for writing a good changelog entry](https://github.com/biom
 
 - Fix [#1653](https://github.com/biomejs/biome/issues/1653). Handle a shorthand value in `useForOf` to avoid the false-positive case. Contributed by @togami2864
 
+- 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)