feat(rome_js_analyze): noUnusedVariable rule (#2987)

* no unused variables lint rule

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

@@ -1,16 +1,16 @@
-use crate::{semantic_services::Semantic, JsRuleAction};
+use crate::{semantic_services::Semantic, utils::batch::JsBatchMutation, JsRuleAction};
 use rome_analyze::{
     context::RuleContext, declare_rule, ActionCategory, Rule, RuleCategory, RuleDiagnostic,
 };
 use rome_console::markup;
 use rome_diagnostics::Applicability;
 use rome_js_semantic::{AllReferencesExtensions, Reference};
 use rome_js_syntax::{
-    JsAnyExpression, JsAnyLiteralExpression, JsAnyRoot, JsIdentifierBinding,
-    JsIdentifierExpression, JsLanguage, JsStringLiteralExpression, JsVariableDeclaration,
-    JsVariableDeclarator, JsVariableDeclaratorList, JsVariableStatement,
+    JsAnyExpression, JsAnyLiteralExpression, JsIdentifierBinding, JsIdentifierExpression,
+    JsStringLiteralExpression, JsVariableDeclaration, JsVariableDeclarator,
+    JsVariableDeclaratorList,
 };
-use rome_rowan::{AstNode, AstSeparatedList, BatchMutation, BatchMutationExt, SyntaxNodeCast};
+use rome_rowan::{AstNode, BatchMutationExt, SyntaxNodeCast};
 
 declare_rule! {
     /// Disallow the use of constants which its value is the upper-case version of its name.
@@ -53,53 +53,6 @@ fn is_id_and_string_literal_inner_text_equal(
     }
 }
 
-/// Removes the declarator, and:
-/// 1 - removes the statement if the declaration only has one declarator;
-/// 2 - removes commas around the declarator to keep the declaration list valid.
-fn remove_declarator(
-    batch: &mut BatchMutation<JsLanguage, JsAnyRoot>,
-    declarator: &JsVariableDeclarator,
-) -> Option<()> {
-    let list = declarator.parent::<JsVariableDeclaratorList>()?;
-    let declaration = list.parent::<JsVariableDeclaration>()?;
-
-    if list.syntax_list().len() == 1 {
-        let statement = declaration.parent::<JsVariableStatement>()?;
-        batch.remove_node(statement);
-    } else {
-        let mut elements = list.elements();
-
-        // Find the declarator 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() {
-                if node == declarator {
-                    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 declarator of the list
-        // removes the comma before this element
-        let is_last = elements.next().is_none();
-        if is_last {
-            if let Some(element) = previous_element {
-                if let Some(comma) = element.trailing_separator().ok().flatten() {
-                    batch.remove_token(comma.clone());
-                }
-            }
-        }
-    }
-
-    Some(())
-}
-
 pub struct State {
     literal: JsStringLiteralExpression,
     references: Vec<Reference>,
@@ -161,7 +114,7 @@ impl Rule for NoShoutyConstants {
 
         let mut batch = root.begin();
 
-        remove_declarator(&mut batch, ctx.query());
+        batch.remove_js_variable_declarator(ctx.query());
 
         for reference in state.references.iter() {
             let node = reference

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

@@ -0,0 +1,183 @@
+use crate::{semantic_services::Semantic, utils::batch::JsBatchMutation, JsRuleAction};
+use rome_analyze::{
+    context::RuleContext, declare_rule, ActionCategory, Rule, RuleCategory, RuleDiagnostic,
+};
+use rome_console::markup;
+use rome_diagnostics::Applicability;
+use rome_js_semantic::{AllReferencesExtensions, SemanticScopeExtensions};
+use rome_js_syntax::{
+    JsFormalParameter, JsFunctionDeclaration, JsIdentifierBinding, JsSyntaxKind,
+    JsVariableDeclarator,
+};
+use rome_rowan::{AstNode, BatchMutationExt};
+
+declare_rule! {
+    /// Disallow unused variables.
+    ///
+    /// ## Examples
+    ///
+    /// ### Invalid
+    ///
+    /// ```js,expect_diagnostic
+    /// const a = 4;
+    /// ```
+    ///
+    /// ```js,expect_diagnostic
+    /// let a = 4;
+    /// ```
+    ///
+    /// ```js,expect_diagnostic
+    /// function foo() {
+    /// };
+    /// ```
+    ///
+    /// ```js,expect_diagnostic
+    /// function foo(myVar) {
+    ///     console.log('foo');
+    /// }
+    /// foo();
+    /// ```
+    ///
+    /// ```js,expect_diagnostic
+    /// const foo = () => {
+    /// };
+    /// ```
+    ///
+    /// ```js,expect_diagnostic
+    /// function foo() {
+    ///     foo();
+    /// }
+    /// ```
+    ///
+    /// ```js,expect_diagnostic
+    /// const foo = () => {
+    ///     foo();
+    ///     console.log(this);
+    /// };
+    /// ```
+    ///
+    /// # Valid
+    ///
+    /// ```js
+    /// function foo(b) {
+    ///     console.log(b)
+    /// };
+    /// foo();
+    /// ```
+    pub(crate) NoUnusedVariables {
+        version: "0.9.0",
+        name: "noUnusedVariables",
+        recommended: true,
+    }
+}
+
+impl Rule for NoUnusedVariables {
+    const CATEGORY: RuleCategory = RuleCategory::Lint;
+
+    type Query = Semantic<JsIdentifierBinding>;
+    type State = ();
+    type Signals = Option<Self::State>;
+
+    fn run(ctx: &RuleContext<Self>) -> Option<Self::State> {
+        let binding = ctx.query();
+        let model = ctx.model();
+
+        let all_references = binding.all_references(model);
+
+        if all_references.count() == 0 {
+            Some(())
+        } else {
+            // We need to check if all uses of this binding are somehow recursive
+
+            let function_declaration_scope = binding
+                .parent::<JsFunctionDeclaration>()
+                .map(|declaration| declaration.scope(model));
+
+            let declarator = binding.parent::<JsVariableDeclarator>();
+
+            let mut references_outside = 0;
+            for r in binding.all_references(model) {
+                let reference_scope = r.scope();
+
+                // If this binding is a function, and all its references are "inside" this
+                // function, we can safely say that this function is not used
+                if function_declaration_scope
+                    .as_ref()
+                    .map(|s| s.is_ancestor_of(&reference_scope))
+                    .unwrap_or(false)
+                {
+                    continue;
+                }
+
+                // Another possibility is if all its references are "inside" the same declaration
+                if let Some(declarator) = declarator.as_ref() {
+                    let node = declarator.syntax();
+                    if r.node().ancestors().any(|n| n == *node) {
+                        continue;
+                    }
+                }
+
+                references_outside += 1;
+                break;
+            }
+
+            if references_outside == 0 {
+                Some(())
+            } else {
+                None
+            }
+        }
+    }
+
+    fn diagnostic(ctx: &RuleContext<Self>, _: &Self::State) -> Option<RuleDiagnostic> {
+        let binding = ctx.query();
+
+        let symbol_type = match binding.syntax().parent().unwrap().kind() {
+            JsSyntaxKind::JS_FORMAL_PARAMETER => "parameter",
+            JsSyntaxKind::JS_FUNCTION_DECLARATION => "function",
+            _ => "variable",
+        };
+
+        let diag = RuleDiagnostic::warning(
+            binding.syntax().text_trimmed_range(),
+            markup! {
+                "This " {symbol_type} " is unused."
+            },
+        );
+
+        let diag = diag.footer_note(
+            markup! {"Unused variables usually are result of incomplete refactoring, typos and other source of bugs."},
+        );
+
+        Some(diag)
+    }
+
+    fn action(ctx: &RuleContext<Self>, _: &Self::State) -> Option<JsRuleAction> {
+        let root = ctx.root();
+        let binding = ctx.query();
+
+        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)
+        } else if let Some(variable_declarator) = binding.parent::<JsVariableDeclarator>() {
+            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",
+        };
+
+        Some(JsRuleAction {
+            category: ActionCategory::QuickFix,
+            applicability: Applicability::Unspecified,
+            message: markup! { "Remove this " {symbol_type} "." }.to_owned(),
+            mutation: batch,
+        })
+    }
+}

crates/rome_js_analyze/src/utils.rs

@@ -3,6 +3,7 @@ use rome_js_syntax::{JsAnyRoot, JsAnyStatement, JsLanguage, JsModuleItemList, Js
 use rome_rowan::{AstNode, BatchMutation};
 use std::borrow::Cow;
 
+pub mod batch;
 pub mod rename;
 #[cfg(test)]
 pub mod tests;