feat(rome_js_analyze): improvements to noUnusedVariables to consider …

…catch, typescript and export (#3004)

* improvements to noUnusedVariables to consider catch, typescript and export.

crates/rome_cli/tests/main.rs

@@ -41,9 +41,10 @@ if(a != 0) {}
 
 const APPLY_SUGGESTED_BEFORE: &str = "let a = 4;
 debugger;
+console.log(a);
 ";
 
-const APPLY_SUGGESTED_AFTER: &str = "let a = 4;\n";
+const APPLY_SUGGESTED_AFTER: &str = "let a = 4;\nconsole.log(a);\n";
 
 const CUSTOM_FORMAT_BEFORE: &str = r#"
 function f() {

crates/rome_cli/tests/snapshots/apply_suggested.snap

@@ -1,12 +1,12 @@
 ---
 source: crates/rome_cli/tests/snap_test.rs
-assertion_line: 137
 expression: content
 ---
 ## `fix.js`
 
 ```js
 let a = 4;
+console.log(a);
 
 ```
 

crates/rome_cli/tests/snapshots/apply_suggested_error.snap

@@ -1,13 +1,13 @@
 ---
 source: crates/rome_cli/tests/snap_test.rs
-assertion_line: 107
 expression: content
 ---
 ## `fix.js`
 
 ```js
 let a = 4;
 debugger;
+console.log(a);
 
 ```
 

crates/rome_js_analyze/src/semantic_analyzers/js/no_unused_variables.rs

@@ -6,7 +6,8 @@ use rome_console::markup;
 use rome_diagnostics::Applicability;
 use rome_js_semantic::{AllReferencesExtensions, SemanticScopeExtensions};
 use rome_js_syntax::{
-    JsFormalParameter, JsFunctionDeclaration, JsIdentifierBinding, JsSyntaxKind,
+    JsCatchDeclaration, JsConstructorParameterList, JsConstructorParameters, JsFormalParameter,
+    JsFunctionDeclaration, JsIdentifierBinding, JsParameterList, JsParameters, JsSyntaxKind,
     JsVariableDeclarator,
 };
 use rome_rowan::{AstNode, BatchMutationExt};
@@ -71,6 +72,43 @@ declare_rule! {
     }
 }
 
+// It is ok in some Typescripts constructs for a parameter to be unused.
+fn is_typescript_unused_ok(binding: &JsIdentifierBinding) -> Option<()> {
+    match binding.syntax().parent()?.kind() {
+        JsSyntaxKind::JS_FORMAL_PARAMETER => {
+            let parameter = binding.parent::<JsFormalParameter>()?;
+            match parameter.syntax().parent()?.kind() {
+                JsSyntaxKind::JS_PARAMETER_LIST => {
+                    let parameters = parameter
+                        .parent::<JsParameterList>()?
+                        .parent::<JsParameters>()?;
+                    match parameters.syntax().parent()?.kind() {
+                        JsSyntaxKind::TS_METHOD_SIGNATURE_CLASS_MEMBER
+                        | JsSyntaxKind::TS_CALL_SIGNATURE_TYPE_MEMBER
+                        | JsSyntaxKind::TS_METHOD_SIGNATURE_TYPE_MEMBER => Some(()),
+                        _ => None,
+                    }
+                }
+                JsSyntaxKind::JS_CONSTRUCTOR_PARAMETER_LIST => {
+                    let parameters = parameter
+                        .parent::<JsConstructorParameterList>()?
+                        .parent::<JsConstructorParameters>()?;
+                    match parameters.syntax().parent()?.kind() {
+                        JsSyntaxKind::TS_CONSTRUCT_SIGNATURE_TYPE_MEMBER
+                        | JsSyntaxKind::TS_CONSTRUCTOR_SIGNATURE_CLASS_MEMBER => Some(()),
+                        _ => None,
+                    }
+                }
+                JsSyntaxKind::TS_SETTER_SIGNATURE_TYPE_MEMBER
+                | JsSyntaxKind::TS_SETTER_SIGNATURE_CLASS_MEMBER => Some(()),
+                _ => None,
+            }
+        }
+        JsSyntaxKind::TS_INDEX_SIGNATURE_PARAMETER => Some(()),
+        _ => None,
+    }
+}
+
 impl Rule for NoUnusedVariables {
     const CATEGORY: RuleCategory = RuleCategory::Lint;
 
@@ -82,6 +120,14 @@ impl Rule for NoUnusedVariables {
         let binding = ctx.query();
         let model = ctx.model();
 
+        if is_typescript_unused_ok(binding).is_some() {
+            return None;
+        }
+
+        if model.is_exported(binding) {
+            return None;
+        }
+
         let all_references = binding.all_references(model);
 
         if all_references.count() == 0 {
@@ -158,26 +204,36 @@ impl Rule for NoUnusedVariables {
 
         let mut batch = root.begin();
 
-        // If this is a function, remove the whole declaration
-        if let Some(declaration) = binding.parent::<JsFunctionDeclaration>() {
-            batch.remove_node(declaration)
+        // Try to remove node
+        let removed = if let Some(declaration) = binding.parent::<JsFunctionDeclaration>() {
+            batch.remove_node(declaration);
+            true
         } else if let Some(variable_declarator) = binding.parent::<JsVariableDeclarator>() {
-            batch.remove_js_variable_declarator(&variable_declarator);
+            batch.remove_js_variable_declarator(&variable_declarator)
         } else if let Some(formal_parameter) = binding.parent::<JsFormalParameter>() {
-            batch.remove_js_formal_parameter(&formal_parameter);
-        }
-
-        let symbol_type = match binding.syntax().parent().unwrap().kind() {
-            JsSyntaxKind::JS_FORMAL_PARAMETER => "parameter",
-            JsSyntaxKind::JS_FUNCTION_DECLARATION => "function",
-            _ => "variable",
+            batch.remove_js_formal_parameter(&formal_parameter)
+        } else if let Some(catch_declaration) = binding.parent::<JsCatchDeclaration>() {
+            batch.remove_node(catch_declaration);
+            true
+        } else {
+            false
         };
 
-        Some(JsRuleAction {
-            category: ActionCategory::QuickFix,
-            applicability: Applicability::Unspecified,
-            message: markup! { "Remove this " {symbol_type} "." }.to_owned(),
-            mutation: batch,
-        })
+        if removed {
+            let symbol_type = match binding.syntax().parent().unwrap().kind() {
+                JsSyntaxKind::JS_FORMAL_PARAMETER => "parameter",
+                JsSyntaxKind::JS_FUNCTION_DECLARATION => "function",
+                _ => "variable",
+            };
+
+            Some(JsRuleAction {
+                category: ActionCategory::QuickFix,
+                applicability: Applicability::MaybeIncorrect,
+                message: markup! { "Remove this " {symbol_type} "." }.to_owned(),
+                mutation: batch,
+            })
+        } else {
+            None
+        }
     }
 }

crates/rome_js_analyze/src/utils/batch.rs

@@ -1,5 +1,6 @@
 use rome_js_syntax::{
-    JsAnyFormalParameter, JsAnyParameter, JsFormalParameter, JsLanguage, JsParameterList,
+    JsAnyConstructorParameter, JsAnyFormalParameter, JsAnyParameter, JsConstructorParameterList,
+    JsFormalParameter, JsLanguage, JsParameterList, JsSyntaxKind, JsSyntaxNode,
     JsVariableDeclaration, JsVariableDeclarator, JsVariableDeclaratorList, JsVariableStatement,
 };
 use rome_rowan::{AstNode, AstSeparatedList, BatchMutation};
@@ -8,15 +9,99 @@ pub trait JsBatchMutation {
     /// Removes the declarator, and:
     /// 1 - removes the statement if the declaration only has one declarator;
     /// 2 - removes commas around the declarator to keep the list valid.
-    fn remove_js_variable_declarator(&mut self, declarator: &JsVariableDeclarator);
+    fn remove_js_variable_declarator(&mut self, declarator: &JsVariableDeclarator) -> bool;
 
     /// Removes the parameter, and:
     /// 1 - removes commas around the parameter to keep the list valid.
-    fn remove_js_formal_parameter(&mut self, parameter: &JsFormalParameter);
+    fn remove_js_formal_parameter(&mut self, parameter: &JsFormalParameter) -> bool;
+}
+
+fn remove_js_formal_parameter_from_js_parameter_list(
+    batch: &mut BatchMutation<JsLanguage>,
+    parameter: &JsFormalParameter,
+    list: JsSyntaxNode,
+) -> Option<bool> {
+    let list = JsParameterList::cast(list)?;
+    let mut elements = list.elements();
+
+    // Find the parameter we want to remove
+    // remove its trailing comma, if there is one
+    let mut previous_element = None;
+    for element in elements.by_ref() {
+        if let Ok(node) = element.node() {
+            match node {
+                JsAnyParameter::JsAnyFormalParameter(JsAnyFormalParameter::JsFormalParameter(
+                    node,
+                )) if node == parameter => {
+                    batch.remove_node(node.clone());
+                    if let Some(comma) = element.trailing_separator().ok().flatten() {
+                        batch.remove_token(comma.clone());
+                    }
+                    break;
+                }
+                _ => {}
+            }
+        }
+        previous_element = Some(element);
+    }
+
+    // if it is the last parameter of the list
+    // removes the comma before this element
+    if elements.next().is_none() {
+        if let Some(element) = previous_element {
+            if let Some(comma) = element.trailing_separator().ok().flatten() {
+                batch.remove_token(comma.clone());
+            }
+        }
+    }
+
+    Some(true)
+}
+
+fn remove_js_formal_parameter_from_js_constructor_parameter_list(
+    batch: &mut BatchMutation<JsLanguage>,
+    parameter: &JsFormalParameter,
+    list: JsSyntaxNode,
+) -> Option<bool> {
+    let list = JsConstructorParameterList::cast(list)?;
+    let mut elements = list.elements();
+
+    // Find the parameter we want to remove
+    // remove its trailing comma, if there is one
+    let mut previous_element = None;
+    for element in elements.by_ref() {
+        if let Ok(node) = element.node() {
+            match node {
+                JsAnyConstructorParameter::JsAnyFormalParameter(
+                    JsAnyFormalParameter::JsFormalParameter(node),
+                ) if node == parameter => {
+                    batch.remove_node(node.clone());
+                    if let Some(comma) = element.trailing_separator().ok().flatten() {
+                        batch.remove_token(comma.clone());
+                    }
+                    break;
+                }
+                _ => {}
+            }
+        }
+        previous_element = Some(element);
+    }
+
+    // if it is the last parameter of the list
+    // removes the comma before this element
+    if elements.next().is_none() {
+        if let Some(element) = previous_element {
+            if let Some(comma) = element.trailing_separator().ok().flatten() {
+                batch.remove_token(comma.clone());
+            }
+        }
+    }
+
+    Some(true)
 }
 
 impl JsBatchMutation for BatchMutation<JsLanguage> {
-    fn remove_js_variable_declarator(&mut self, declarator: &JsVariableDeclarator) {
+    fn remove_js_variable_declarator(&mut self, declarator: &JsVariableDeclarator) -> bool {
         declarator
             .parent::<JsVariableDeclaratorList>()
             .and_then(|list| {
@@ -61,47 +146,27 @@ impl JsBatchMutation for BatchMutation<JsLanguage> {
                     }
                 }
 
-                Some(())
-            });
+                Some(true)
+            })
+            .unwrap_or(false)
     }
 
-    fn remove_js_formal_parameter(&mut self, parameter: &JsFormalParameter) {
-        parameter.parent::<JsParameterList>().map(|list| {
-            let mut elements = list.elements();
-
-            // Find the parameter we want to remove
-            // remove its trailing comma, if there is one
-            let mut previous_element = None;
-            for element in elements.by_ref() {
-                if let Ok(node) = element.node() {
-                    match node {
-                        JsAnyParameter::JsAnyFormalParameter(
-                            JsAnyFormalParameter::JsFormalParameter(node),
-                        ) if node == parameter => {
-                            self.remove_node(node.clone());
-                            if let Some(comma) = element.trailing_separator().ok().flatten() {
-                                self.remove_token(comma.clone());
-                            }
-                            break;
-                        }
-                        _ => {}
-                    }
+    fn remove_js_formal_parameter(&mut self, parameter: &JsFormalParameter) -> bool {
+        parameter
+            .syntax()
+            .parent()
+            .and_then(|parent| match parent.kind() {
+                JsSyntaxKind::JS_PARAMETER_LIST => {
+                    remove_js_formal_parameter_from_js_parameter_list(self, parameter, parent)
                 }
-                previous_element = Some(element);
-            }
-
-            // if it is the last parameter of the list
-            // removes the comma before this element
-            if elements.next().is_none() {
-                if let Some(element) = previous_element {
-                    if let Some(comma) = element.trailing_separator().ok().flatten() {
-                        self.remove_token(comma.clone());
-                    }
+                JsSyntaxKind::JS_CONSTRUCTOR_PARAMETER_LIST => {
+                    remove_js_formal_parameter_from_js_constructor_parameter_list(
+                        self, parameter, parent,
+                    )
                 }
-            }
-
-            Some(())
-        });
+                _ => None,
+            })
+            .unwrap_or(false)
     }
 }
 
@@ -128,7 +193,7 @@ mod tests {
             "let b, c;",
     }
 
-    // Remove JsFormalParameter
+    // Remove JsFormalParameter from functions
     assert_remove_ok! {
         ok_remove_formal_parameter_single,
             "function f(a) {}",
@@ -146,4 +211,23 @@ mod tests {
             "function f(b, a, c) {}",
             "function f(b, c) {}",
     }
+
+    // Remove JsFormalParameter from class constructors
+    assert_remove_ok! {
+        ok_remove_formal_parameter_from_class_constructor_single,
+            "class A { constructor(a) {} }",
+            "class A { constructor() {} }",
+        ok_remove_formal_parameter_from_class_constructor_first,
+            "class A { constructor(a, b) {} }",
+            "class A { constructor(b) {} }",
+        ok_remove_formal_parameter_from_class_constructor_second,
+            "class A { constructor(b, a) {} }",
+            "class A { constructor(b) {} }",
+        ok_remove_formal_parameter_from_class_constructor_second_trailing_comma,
+            "class A { constructor(b, a,) {} }",
+            "class A { constructor(b) {} }",
+        ok_remove_formal_parameter_from_class_constructor_middle,
+            "class A { constructor(b, a, c) {} }",
+            "class A { constructor(b, c) {} }",
+    }
 }

crates/rome_js_analyze/src/utils/tests.rs

@@ -57,11 +57,14 @@ pub fn assert_remove_ok(before: &str, expected: &str) {
 
     let mut batch = r.tree().begin();
 
-    if let Some(parameter) = binding_a.parent::<JsFormalParameter>() {
-        batch.remove_js_formal_parameter(&parameter);
+    let r = if let Some(parameter) = binding_a.parent::<JsFormalParameter>() {
+        batch.remove_js_formal_parameter(&parameter)
     } else if let Some(declarator) = binding_a.parent::<JsVariableDeclarator>() {
-        batch.remove_js_variable_declarator(&declarator);
-    }
+        batch.remove_js_variable_declarator(&declarator)
+    } else {
+        panic!("Don't know how to remove this node: {:?}", binding_a);
+    };
+    assert!(r);
     let root = batch.commit();
 
     let after = root.to_string();