fix(rome_js_analyze): fix useConst suggestion when some bindings cann…

…ot live inside const (#4014)

* fix useConst suggestion when some bindings cannot live inside const

crates/rome_js_analyze/src/semantic_analyzers/nursery/use_const.rs

@@ -42,18 +42,18 @@ declare_rule! {
     /// }
     /// ```
     ///
-    /// ```js,expect_diagnostic
-    /// let a = 1, b = 2;
-    /// b = 3;
-    /// ```
-    ///
     /// ## Valid
     ///
     /// ```js
     /// let a = 2;
     /// a = 3;
     /// console.log(a);
     /// ```
+    ///
+    /// ```js
+    /// let a = 1, b = 2;
+    /// b = 3;
+    /// ```
     pub(crate) UseConst {
         version: "11.0.0",
         name: "useConst",
@@ -150,7 +150,10 @@ impl ConstBindings {
             declaration,
             VariableDeclaration::JsForVariableDeclaration(..)
         );
+        let mut bindings = 0;
         declaration.for_each_binding(|binding, declarator| {
+            bindings += 1;
+
             let has_initializer = declarator.initializer().is_some();
             let fix =
                 check_binding_can_be_const(&binding, in_for_in_or_of_loop, has_initializer, model);
@@ -163,7 +166,9 @@ impl ConstBindings {
                 None => state.can_fix = false,
             }
         });
-        if state.can_be_const.is_empty() {
+
+        // Only flag if all bindings can be const
+        if state.can_be_const.len() != bindings {
             None
         } else {
             Some(state)

crates/rome_js_analyze/src/utils/rename.rs

@@ -230,10 +230,7 @@ impl RenameSymbolExtensions for BatchMutation<JsLanguage> {
         node: impl RenamableNode,
         new_name: &str,
     ) -> bool {
-        let prev_binding = match node
-            .binding(model)
-            .and_then(|node| node.cast::<JsIdentifierBinding>())
-        {
+        let prev_binding = match node.binding(model).and_then(AnyJsIdentifierBinding::cast) {
             Some(prev_binding) => prev_binding,
             None => return false,
         };
@@ -294,13 +291,14 @@ impl RenameSymbolExtensions for BatchMutation<JsLanguage> {
 
         // Now it is safe to push changes to the batch mutation
         // Rename binding
+        let Ok(prev_name_token) = prev_binding.name_token() else {
+            return false;
+        };
 
         let next_name_token = token_with_new_text(&name_token, new_name);
-        let next_binding = prev_binding.clone().with_name_token(next_name_token);
-        self.replace_node(prev_binding, next_binding);
+        self.replace_token(prev_name_token, next_name_token);
 
         // Rename all references
-
         for (prev_token, next_token) in changes {
             self.replace_token(prev_token, next_token);
         }
@@ -320,6 +318,9 @@ mod tests {
         ok_rename_declaration,
             "let a;",
             "let b;",
+        ok_rename_declaration_with_multiple_declarators,
+            "let a1, a2;",
+            "let b1, b2;",
         ok_rename_declaration_inner_scope,
             "let b; if (true) { let a; }",
             "let b; if (true) { let b; }",

crates/rome_js_analyze/src/utils/tests.rs

@@ -11,29 +11,40 @@ use rome_js_syntax::{
 };
 use rome_rowan::{AstNode, BatchMutationExt, SyntaxNodeCast};
 
-/// Search and renames a binding named "a" to "b".
+/// Search and renames alls bindings where the name contains "a" replacing it to "b".
 /// Asserts the renaming worked.
 pub fn assert_rename_binding_a_to_b_ok(before: &str, expected: &str) {
     let r = rome_js_parser::parse(before, FileId::zero(), SourceType::js_module());
     let model = semantic_model(&r.tree(), SemanticModelOptions::default());
 
-    let binding_a = r
+    let bindings: Vec<JsIdentifierBinding> = r
         .syntax()
         .descendants()
-        .filter_map(|x| x.cast::<JsIdentifierBinding>())
-        .find(|x| x.text() == "a")
-        .unwrap();
+        .filter_map(JsIdentifierBinding::cast)
+        .filter(|x| x.text().contains('a'))
+        .collect();
 
     let mut batch = r.tree().begin();
-    assert!(batch.rename_node_declaration(&model, binding_a, "b"));
-    let root = batch.commit();
+    for binding in bindings {
+        let new_name = binding
+            .name_token()
+            .unwrap()
+            .text_trimmed()
+            .replace('a', "b");
+        assert!(batch.rename_node_declaration(&model, binding, &new_name));
+    }
 
+    let root = batch.commit();
     let after = root.to_string();
     assert_eq!(expected, after.as_str());
+
+    assert!(!rome_js_parser::test_utils::has_bogus_nodes_or_empty_slots(
+        &root
+    ));
 }
 
-/// Search and renames a binding named "a" to "b".
-/// Asserts the renaming to fail.
+/// Search and renames one binding named "a" to "b".
+/// Asserts the renaming fails.
 pub fn assert_rename_binding_a_to_b_nok(before: &str) {
     let r = rome_js_parser::parse(before, FileId::zero(), SourceType::js_module());
     let model = semantic_model(&r.tree(), SemanticModelOptions::default());

crates/rome_js_analyze/tests/spec_tests.rs

@@ -261,7 +261,10 @@ fn check_code_action(
     assert_eq!(new_tree.to_string(), output);
 
     if has_bogus_nodes_or_empty_slots(&new_tree) {
-        panic!("modified tree has bogus nodes or empty slots:\n{new_tree:#?}")
+        panic!(
+            "modified tree has bogus nodes or empty slots:\n{new_tree:#?} \n\n {}",
+            new_tree
+        )
     }
 
     // Checks the returned tree contains no missing children node

crates/rome_js_analyze/tests/specs/correctness/noUnusedVariables/invalidVariables.ts

@@ -12,3 +12,5 @@ const [i] = [1];
 
 var [{ j }] = [{ j: 1 }]
 var { k: [l] } = { k: [1] } 
+
+let m, n;

crates/rome_js_analyze/tests/specs/correctness/noUnusedVariables/invalidVariables.ts.snap

@@ -19,6 +19,8 @@ const [i] = [1];
 var [{ j }] = [{ j: 1 }]
 var { k: [l] } = { k: [1] } 
 
+let m, n;
+
 ```
 
 # Diagnostics
@@ -247,6 +249,7 @@ invalidVariables.ts:14:11 lint/correctness/noUnusedVariables  FIXABLE  ━━━
   > 14 │ var { k: [l] } = { k: [1] }·
        │           ^
     15 │ 
+    16 │ let m, n;
   
   i Unused variables usually are result of incomplete refactoring, typos and other source of bugs.
   
@@ -257,6 +260,55 @@ invalidVariables.ts:14:11 lint/correctness/noUnusedVariables  FIXABLE  ━━━
     14    │ - var·{·k:·[l]·}·=·{·k:·[1]·}·
        14 │ + var·{·k:·[_l]·}·=·{·k:·[1]·}·
     15 15 │   
+    16 16 │   let m, n;
+  
+
+```
+
+```
+invalidVariables.ts:16:5 lint/correctness/noUnusedVariables  FIXABLE  ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
+
+  ! This variable is unused.
+  
+    14 │ var { k: [l] } = { k: [1] }·
+    15 │ 
+  > 16 │ let m, n;
+       │     ^
+    17 │ 
+  
+  i Unused variables usually are result of incomplete refactoring, typos and other source of bugs.
+  
+  i Suggested fix: If this is intentional, prepend m with an underscore.
+  
+    14 14 │   var { k: [l] } = { k: [1] }·
+    15 15 │   
+    16    │ - let·m,·n;
+       16 │ + let·_m,·n;
+    17 17 │   
+  
+
+```
+
+```
+invalidVariables.ts:16:8 lint/correctness/noUnusedVariables  FIXABLE  ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
+
+  ! This variable is unused.
+  
+    14 │ var { k: [l] } = { k: [1] }·
+    15 │ 
+  > 16 │ let m, n;
+       │        ^
+    17 │ 
+  
+  i Unused variables usually are result of incomplete refactoring, typos and other source of bugs.
+  
+  i Suggested fix: If this is intentional, prepend n with an underscore.
+  
+    14 14 │   var { k: [l] } = { k: [1] }·
+    15 15 │   
+    16    │ - let·m,·n;
+       16 │ + let·m,·_n;
+    17 17 │   
   
 
 ```

crates/rome_js_analyze/tests/specs/correctness/noUnusedVariables/validVariables.ts → crates/rome_js_analyze/tests/specs/correctness/noUnusedVariables/validVariables.tsx

@@ -9,3 +9,7 @@ console.log(a, b, c);
 let value;
 function Button() {}
 console.log(<Button att={value}/>);
+
+// object assignment pattern
+let d, e; 
+({d, e} = {d: 1, e: 2});

crates/rome_js_analyze/tests/specs/correctness/noUnusedVariables/validVariables.tsx.snap

@@ -0,0 +1,25 @@
+---
+source: crates/rome_js_analyze/tests/spec_tests.rs
+expression: validVariables.tsx
+---
+# Input
+```js
+/* should not generate diagnostics */
+
+var a = 1;
+let b = 1;
+const c = 1;
+console.log(a, b, c);
+
+// being used inside JSX
+let value;
+function Button() {}
+console.log(<Button att={value}/>);
+
+// object assignment pattern
+let d, e; 
+({d, e} = {d: 1, e: 2});
+
+```
+
+

crates/rome_js_analyze/tests/specs/nursery/noVar/invalidScript.jsonc.snap

@@ -1,6 +1,5 @@
 ---
 source: crates/rome_js_analyze/tests/spec_tests.rs
-assertion_line: 73
 expression: invalidScript.jsonc
 ---
 # Input
@@ -88,7 +87,7 @@ var [x = -1, y] = [1,2]; y = 0;
 
 # Diagnostics
 ```
-invalidScript.jsonc:1:1 lint/nursery/noVar  FIXABLE  ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
+invalidScript.jsonc:1:1 lint/nursery/noVar ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
 
   ! Use let or const instead of var.
   
@@ -99,11 +98,6 @@ invalidScript.jsonc:1:1 lint/nursery/noVar  FIXABLE  ━━━━━━━━━
   
   i See MDN web docs for more details.
   
-  i Suggested fix: Use 'let' instead.
-  
-  - var·[x·=·-1,·y]·=·[1,2];·y·=·0;
-  + let·[x·=·-1,·y]·=·[1,2];·y·=·0;
-  
 
 ```
 
@@ -114,7 +108,7 @@ var {a: x = -1, b: y} = {a:1,b:2}; y = 0;
 
 # Diagnostics
 ```
-invalidScript.jsonc:1:1 lint/nursery/noVar  FIXABLE  ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
+invalidScript.jsonc:1:1 lint/nursery/noVar ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
 
   ! Use let or const instead of var.
   
@@ -125,11 +119,6 @@ invalidScript.jsonc:1:1 lint/nursery/noVar  FIXABLE  ━━━━━━━━━
   
   i See MDN web docs for more details.
   
-  i Suggested fix: Use 'let' instead.
-  
-  - var·{a:·x·=·-1,·b:·y}·=·{a:1,b:2};·y·=·0;
-  + let·{a:·x·=·-1,·b:·y}·=·{a:1,b:2};·y·=·0;
-  
 
 ```
 

crates/rome_js_analyze/tests/specs/nursery/useConst/invalid.jsonc

@@ -2,14 +2,10 @@
 	"let x = 1; foo(x);",
 	"for (let i in [1,2,3]) { foo(i); }",
 	"for (let x of [1,2,3]) { foo(x); }",
-	"let [x = -1, y] = [1,2]; y = 0;",
-	"let {a: x = -1, b: y} = {a:1,b:2}; y = 0;",
 	"(function() { let x = 1; foo(x); })();",
 	"(function() { for (let i in [1,2,3]) { foo(i); } })();",
 	"(function() { for (let x of [1,2,3]) { foo(x); } })();",
-	"(function() { let [x = -1, y] = [1,2]; y = 0; })();",
 	"let f = (function() { let g = x; })(); f = 1;",
-	"(function() { let {a: x = -1, b: y} = {a:1,b:2}; y = 0; })();",
 	"let x = 0; { let x = 1; foo(x); } x = 0;",
 	"for (let i = 0; i < 10; ++i) { let x = 1; foo(x); }",
 	"for (let i in [1,2,3]) { let x = 1; foo(x); }",
@@ -18,20 +14,13 @@
 	"let x; x = 0;",
 	"switch (a) { case 0: let x; x = 0; }",
 	"(function() { let x; x = 1; })();",
-	"let {a = 0, b} = obj; b = 0; foo(a, b);",
-	"let {a: {b, c}} = {a: {b: 1, c: 2}}; b = 3;",
 	"let {a: {b, c}} = {a: {b: 1, c: 2}}",
-	"let a, b; ({a = 0, b} = obj); b = 0; foo(a, b);",
 	"let {a = 0, b} = obj; foo(a, b);",
 	"let [a] = [1]",
 	"let {a} = obj",
 	"let a, b; ({a = 0, b} = obj); foo(a, b);",
-	"let {a = 0, b} = obj, c = a; b = a;",
-	"let {a = 0, b} = obj, c = a; b = a;",
 
 	// https://github.com/eslint/eslint/issues/8187
-	"let { name, ...otherStuff } = obj; otherStuff = {};",
-	"let { name, ...otherStuff } = obj; otherStuff = {};",
 	"let x; function foo() { bar(x); } x = 0;",
 
 	// https://github.com/eslint/eslint/issues/5837
@@ -48,17 +37,12 @@
 	"let predicate; [, {foo:returnType, predicate}, ...bar ] = foo();",
 	"let predicate; [, {foo:returnType, ...predicate} ] = foo();",
 	"let x = 'x', y = 'y';",
-	"let x = 'x', y = 'y'; x = 1",
+
 	"let x = 1, y = 'y'; let z = 1;",
 	"let { a, b, c} = obj; let { x, y, z} = anotherObj; x = 2;",
 	"let x = 'x', y = 'y'; function someFunc() { let a = 1, b = 2; foo(a, b) }",
 	"let someFunc = () => { let a = 1, b = 2; foo(a, b) }",
-
-	// https://github.com/eslint/eslint/issues/11699
-	"let {a, b} = c, d;",
-	"let {a, b, c} = {}, e, f;",
-	"function a() { let foo = 0, bar = 1; foo = 1; } function b() { let foo = 0, bar = 2; foo = 2; }",
-
+
 	// https://github.com/eslint/eslint/issues/13899
 	"/*eslint no-undef-init:error*/ let foo = undefined;",
 	"let a = 1; class C { static { a; } }",
@@ -74,8 +58,9 @@
 	"class C { static { let a; a = 0; console.log(a); } }",
 
 	// https://github.com/eslint/eslint/issues/16266
-	"let { itemId, list } = {}, obj = [], total = 0; total = 9; console.log(itemId, list, obj, total);",
 	"let { itemId, list } = {}, obj = []; console.log(itemId, list, obj);",
-	"let [ itemId, list ] = [], total = 0; total = 9; console.log(itemId, list, total);",
-	"let [ itemId, list ] = [], obj = []; console.log(itemId, list, obj);"
+	"let [ itemId, list ] = [], obj = []; console.log(itemId, list, obj);",
+
+	"class C { static { () => a; let a = 1; } };",
+	"let x; function foo() { bar(x); } x = 0;"
 ]