From c304943085fb97bd84db5989e69e0e1b6688a7e8 Mon Sep 17 00:00:00 2001 From: Daniel Leite Date: Wed, 7 Dec 2022 19:33:17 +0000 Subject: [PATCH 1/3] fix useConst suggestion when some bindings cannot live inside const --- .../semantic_analyzers/nursery/use_const.rs | 7 +- crates/rome_js_analyze/src/utils/rename.rs | 15 +- crates/rome_js_analyze/src/utils/tests.rs | 29 +- crates/rome_js_analyze/tests/spec_tests.rs | 5 +- .../noUnusedVariables/invalidVariables.ts | 2 + .../invalidVariables.ts.snap | 52 ++ .../{validVariables.ts => validVariables.tsx} | 4 + .../noUnusedVariables/validVariables.tsx.snap | 25 + .../nursery/noVar/invalidScript.jsonc.snap | 15 +- .../specs/nursery/useConst/invalid.jsonc | 27 +- .../specs/nursery/useConst/invalid.jsonc.snap | 474 ++---------------- .../tests/specs/nursery/useConst/valid.jsonc | 27 +- .../specs/nursery/useConst/valid.jsonc.snap | 199 +++----- .../specs/nursery/useConst/validPartial.js | 3 + .../nursery/useConst/validPartial.js.snap | 12 + .../rome_js_semantic/src/tests/references.rs | 5 +- crates/rome_js_syntax/src/binding_ext.rs | 22 +- 17 files changed, 284 insertions(+), 639 deletions(-) rename crates/rome_js_analyze/tests/specs/correctness/noUnusedVariables/{validVariables.ts => validVariables.tsx} (74%) create mode 100644 crates/rome_js_analyze/tests/specs/correctness/noUnusedVariables/validVariables.tsx.snap create mode 100644 crates/rome_js_analyze/tests/specs/nursery/useConst/validPartial.js create mode 100644 crates/rome_js_analyze/tests/specs/nursery/useConst/validPartial.js.snap diff --git a/crates/rome_js_analyze/src/semantic_analyzers/nursery/use_const.rs b/crates/rome_js_analyze/src/semantic_analyzers/nursery/use_const.rs index 2fb9460a2da..6d60d30beba 100644 --- a/crates/rome_js_analyze/src/semantic_analyzers/nursery/use_const.rs +++ b/crates/rome_js_analyze/src/semantic_analyzers/nursery/use_const.rs @@ -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) diff --git a/crates/rome_js_analyze/src/utils/rename.rs b/crates/rome_js_analyze/src/utils/rename.rs index 087837540f7..a15345e25c1 100644 --- a/crates/rome_js_analyze/src/utils/rename.rs +++ b/crates/rome_js_analyze/src/utils/rename.rs @@ -170,10 +170,7 @@ impl RenameSymbolExtensions for BatchMutation { node: impl RenamableNode, new_name: &str, ) -> bool { - let prev_binding = match node - .binding(model) - .and_then(|node| node.cast::()) - { + let prev_binding = match node.binding(model).and_then(AnyJsIdentifierBinding::cast) { Some(prev_binding) => prev_binding, None => return false, }; @@ -234,13 +231,14 @@ impl RenameSymbolExtensions for BatchMutation { // 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); } @@ -257,6 +255,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; }", diff --git a/crates/rome_js_analyze/src/utils/tests.rs b/crates/rome_js_analyze/src/utils/tests.rs index 264465cf73a..132b4851cb2 100644 --- a/crates/rome_js_analyze/src/utils/tests.rs +++ 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 = r .syntax() .descendants() - .filter_map(|x| x.cast::()) - .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()); diff --git a/crates/rome_js_analyze/tests/spec_tests.rs b/crates/rome_js_analyze/tests/spec_tests.rs index c8f5a71ac9c..6767184f83a 100644 --- a/crates/rome_js_analyze/tests/spec_tests.rs +++ b/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 diff --git a/crates/rome_js_analyze/tests/specs/correctness/noUnusedVariables/invalidVariables.ts b/crates/rome_js_analyze/tests/specs/correctness/noUnusedVariables/invalidVariables.ts index a6f067eb8d4..7f7f7436025 100644 --- a/crates/rome_js_analyze/tests/specs/correctness/noUnusedVariables/invalidVariables.ts +++ b/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; diff --git a/crates/rome_js_analyze/tests/specs/correctness/noUnusedVariables/invalidVariables.ts.snap b/crates/rome_js_analyze/tests/specs/correctness/noUnusedVariables/invalidVariables.ts.snap index e60ff6c49e5..3ff4a6df89a 100644 --- a/crates/rome_js_analyze/tests/specs/correctness/noUnusedVariables/invalidVariables.ts.snap +++ b/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 │ ``` diff --git a/crates/rome_js_analyze/tests/specs/correctness/noUnusedVariables/validVariables.ts b/crates/rome_js_analyze/tests/specs/correctness/noUnusedVariables/validVariables.tsx similarity index 74% rename from crates/rome_js_analyze/tests/specs/correctness/noUnusedVariables/validVariables.ts rename to crates/rome_js_analyze/tests/specs/correctness/noUnusedVariables/validVariables.tsx index f3f891b97fe..29282bc47d4 100644 --- a/crates/rome_js_analyze/tests/specs/correctness/noUnusedVariables/validVariables.ts +++ b/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(