fix(rome_js_analyze): fix for unused flag on functional types on ts

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

@@ -1,16 +1,15 @@
-use crate::semantic_services::Semantic;
 use crate::JsRuleAction;
+use crate::{semantic_services::Semantic, utils::rename::RenameSymbolExtensions};
 use rome_analyze::{context::RuleContext, declare_rule, ActionCategory, Rule, RuleDiagnostic};
 use rome_console::markup;
 use rome_diagnostics::Applicability;
-use rome_js_factory::make::{ident, js_identifier_binding};
 use rome_js_semantic::{ReferencesExtensions, SemanticScopeExtensions};
 use rome_js_syntax::{
-    JsClassExpression, JsConstructorParameterList, JsConstructorParameters, JsFunctionDeclaration,
-    JsFunctionExpression, JsIdentifierBinding, JsParameterList, JsParameters, JsSyntaxKind,
-    JsVariableDeclarator, TsDeclareStatement, TsPropertyParameter,
+    binding_ext::{AnyJsBindingDeclaration, AnyJsIdentifierBinding, JsAnyParameterParentFunction},
+    JsClassExpression, JsFunctionDeclaration, JsFunctionExpression, JsSyntaxKind, JsSyntaxNode,
+    JsVariableDeclarator, TsPropertyParameter,
 };
-use rome_rowan::{AstNode, BatchMutationExt, SyntaxNodeCast};
+use rome_rowan::{AstNode, BatchMutationExt};
 
 declare_rule! {
     /// Disallow unused variables.
@@ -106,83 +105,167 @@ declare_rule! {
     }
 }
 
+/// Suggestion if the bindnig is unused
+#[derive(Copy, Clone)]
+pub enum SuggestedFix {
+    /// No suggestion will be given
+    NoSuggestion,
+    /// Suggest to prefix the name of the binding with underscore
+    PrefixUnderscore,
+}
+
+fn is_function_that_is_ok_parameter_not_be_used(
+    parent_function: Option<JsAnyParameterParentFunction>,
+) -> bool {
+    matches!(
+        parent_function,
+        Some(
+            // bindings in signatures are ok to not be used
+            JsAnyParameterParentFunction::TsMethodSignatureClassMember(_)
+            | JsAnyParameterParentFunction::TsCallSignatureTypeMember(_)
+            | JsAnyParameterParentFunction::TsConstructSignatureTypeMember(_)
+            | JsAnyParameterParentFunction::TsConstructorSignatureClassMember(_)
+            | JsAnyParameterParentFunction::TsMethodSignatureTypeMember(_)
+            | JsAnyParameterParentFunction::TsSetterSignatureClassMember(_)
+            | JsAnyParameterParentFunction::TsSetterSignatureTypeMember(_)
+            // bindings in function types are ok to not be used
+            | JsAnyParameterParentFunction::TsFunctionType(_)
+            // binding in declare are ok to not be used
+            | JsAnyParameterParentFunction::TsDeclareFunctionDeclaration(_)
+        )
+    )
+}
+
+fn is_property_parameter_ok_not_be_used(parameter: TsPropertyParameter) -> Option<bool> {
+    for modifier in parameter.modifiers() {
+        if let Some(modifier) = modifier.as_ts_accessibility_modifier() {
+            match modifier.modifier_token().ok()?.kind() {
+                // modifiers that are ok to not be used
+                JsSyntaxKind::PRIVATE_KW | JsSyntaxKind::PUBLIC_KW => return Some(true),
+                _ => {}
+            }
+        }
+    }
+
+    Some(false)
+}
+
+fn is_is_ambient_context(node: &JsSyntaxNode) -> bool {
+    node.ancestors()
+        .any(|x| x.kind() == JsSyntaxKind::TS_DECLARE_STATEMENT)
+}
+
+fn suggestion_for_binding(binding: &AnyJsIdentifierBinding) -> Option<SuggestedFix> {
+    if binding.is_under_object_pattern_binding()? {
+        Some(SuggestedFix::NoSuggestion)
+    } else {
+        Some(SuggestedFix::PrefixUnderscore)
+    }
+}
+
 // It is ok in some Typescripts constructs for a parameter to be unused.
-fn is_typescript_unused_ok(binding: &JsIdentifierBinding) -> Option<()> {
-    let parent = binding.syntax().parent()?;
-    match parent.kind() {
-        JsSyntaxKind::JS_FORMAL_PARAMETER | JsSyntaxKind::JS_REST_PARAMETER => {
-            let parameter = binding.syntax().parent()?;
-            let parent = parameter.parent()?;
-            match parent.kind() {
-                // example: abstract f(a: number);
-                JsSyntaxKind::JS_PARAMETER_LIST => {
-                    let parameters = JsParameterList::cast(parent)?.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
-                        | JsSyntaxKind::TS_FUNCTION_TYPE
-                        | JsSyntaxKind::TS_DECLARE_FUNCTION_DECLARATION => Some(()),
-                        _ => None,
-                    }
-                }
-                // example: constructor(a: number);
-                JsSyntaxKind::JS_CONSTRUCTOR_PARAMETER_LIST => {
-                    let parameters = JsConstructorParameterList::cast(parent)?
-                        .parent::<JsConstructorParameters>()?;
-                    match parameters.syntax().parent()?.kind() {
-                        JsSyntaxKind::TS_CONSTRUCT_SIGNATURE_TYPE_MEMBER
-                        | JsSyntaxKind::TS_CONSTRUCTOR_SIGNATURE_CLASS_MEMBER => Some(()),
-                        _ => None,
-                    }
-                }
-                // example: abstract set a(a: number);
-                // We don't need get because getter do not have parameters
-                JsSyntaxKind::TS_SETTER_SIGNATURE_TYPE_MEMBER
-                | JsSyntaxKind::TS_SETTER_SIGNATURE_CLASS_MEMBER => Some(()),
-                // example: constructor(a, private b, public c) {}
-                JsSyntaxKind::TS_PROPERTY_PARAMETER => {
-                    if let Some(parent) = parent.cast::<TsPropertyParameter>() {
-                        for modifier in parent.modifiers().into_iter() {
-                            if let Some(modifier) = modifier.as_ts_accessibility_modifier() {
-                                match modifier.modifier_token().ok()?.kind() {
-                                    JsSyntaxKind::PRIVATE_KW | JsSyntaxKind::PUBLIC_KW => {
-                                        return Some(())
-                                    }
-                                    _ => {}
-                                }
-                            }
-                        }
-                    }
+// Returning None means is ok to be unused
+fn suggested_fix_if_unused(binding: &AnyJsIdentifierBinding) -> Option<SuggestedFix> {
+    match binding.declaration()? {
+        // ok to not be used
+        AnyJsBindingDeclaration::TsIndexSignatureParameter(_)
+        | AnyJsBindingDeclaration::TsDeclareFunctionDeclaration(_)
+        | AnyJsBindingDeclaration::JsClassExpression(_)
+        | AnyJsBindingDeclaration::JsFunctionExpression(_) => None,
 
-                    None
-                }
-                _ => None,
+        // Some parameters are ok to not be used
+        AnyJsBindingDeclaration::TsPropertyParameter(parameter) => {
+            let is_binding_ok =
+                is_function_that_is_ok_parameter_not_be_used(parameter.parent_function())
+                    || is_property_parameter_ok_not_be_used(parameter)?;
+            if !is_binding_ok {
+                suggestion_for_binding(binding)
+            } else {
+                None
             }
         }
-        // example: [key: string]: string;
-        JsSyntaxKind::TS_INDEX_SIGNATURE_PARAMETER => Some(()),
-        // example: declare function notUsedParameters(a);
-        JsSyntaxKind::TS_DECLARE_FUNCTION_DECLARATION => Some(()),
-        _ => {
-            // Anything below a declare
-            parent
-                .ancestors()
-                .any(|x| TsDeclareStatement::can_cast(x.kind()))
-                .then_some(())
+        AnyJsBindingDeclaration::JsFormalParameter(parameter) => {
+            let is_binding_ok =
+                is_function_that_is_ok_parameter_not_be_used(parameter.parent_function());
+            if !is_binding_ok {
+                suggestion_for_binding(binding)
+            } else {
+                None
+            }
+        }
+        AnyJsBindingDeclaration::JsRestParameter(parameter) => {
+            let is_binding_ok =
+                is_function_that_is_ok_parameter_not_be_used(parameter.parent_function());
+            if !is_binding_ok {
+                suggestion_for_binding(binding)
+            } else {
+                None
+            }
+        }
+
+        // declarations need to be check if they are under `declare`
+        node @ AnyJsBindingDeclaration::JsVariableDeclarator(_) => {
+            let is_binding_ok = is_is_ambient_context(&node.syntax().clone());
+            if !is_binding_ok {
+                suggestion_for_binding(binding)
+            } else {
+                None
+            }
+        }
+        node @ AnyJsBindingDeclaration::TsTypeAliasDeclaration(_)
+        | node @ AnyJsBindingDeclaration::JsClassDeclaration(_)
+        | node @ AnyJsBindingDeclaration::JsFunctionDeclaration(_)
+        | node @ AnyJsBindingDeclaration::TsInterfaceDeclaration(_)
+        | node @ AnyJsBindingDeclaration::TsEnumDeclaration(_)
+        | node @ AnyJsBindingDeclaration::TsModuleDeclaration(_)
+        | node @ AnyJsBindingDeclaration::TsImportEqualsDeclaration(_) => {
+            if is_is_ambient_context(&node.syntax().clone()) {
+                None
+            } else {
+                Some(SuggestedFix::NoSuggestion)
+            }
+        }
+
+        // Bindings under unknown parameter are never ok to be unused
+        AnyJsBindingDeclaration::JsBogusParameter(_) => Some(SuggestedFix::NoSuggestion),
+
+        // Bindings under catch are never ok to be unused
+        AnyJsBindingDeclaration::JsCatchDeclaration(_) => Some(SuggestedFix::PrefixUnderscore),
+
+        // Imports are never ok to be unused
+        AnyJsBindingDeclaration::JsImportDefaultClause(_)
+        | AnyJsBindingDeclaration::JsImportNamespaceClause(_)
+        | AnyJsBindingDeclaration::JsShorthandNamedImportSpecifier(_)
+        | AnyJsBindingDeclaration::JsNamedImportSpecifier(_)
+        | AnyJsBindingDeclaration::JsBogusNamedImportSpecifier(_)
+        | AnyJsBindingDeclaration::JsDefaultImportSpecifier(_)
+        | AnyJsBindingDeclaration::JsNamespaceImportSpecifier(_) => {
+            Some(SuggestedFix::NoSuggestion)
+        }
+
+        // exports with binding are ok to be unused
+        AnyJsBindingDeclaration::JsClassExportDefaultDeclaration(_)
+        | AnyJsBindingDeclaration::JsFunctionExportDefaultDeclaration(_)
+        | AnyJsBindingDeclaration::TsDeclareFunctionExportDefaultDeclaration(_) => {
+            Some(SuggestedFix::NoSuggestion)
         }
     }
 }
 
 impl Rule for NoUnusedVariables {
-    type Query = Semantic<JsIdentifierBinding>;
-    type State = ();
+    type Query = Semantic<AnyJsIdentifierBinding>;
+    type State = SuggestedFix;
     type Signals = Option<Self::State>;
     type Options = ();
 
     fn run(ctx: &RuleContext<Self>) -> Option<Self::State> {
         let binding = ctx.query();
-        let name = binding.name_token().ok()?;
+
+        let name = match binding {
+            AnyJsIdentifierBinding::JsIdentifierBinding(binding) => binding.name_token().ok()?,
+            AnyJsIdentifierBinding::TsIdentifierBinding(binding) => binding.name_token().ok()?,
+        };
+
         let name = name.token_text_trimmed();
         let name = name.text();
 
@@ -199,9 +282,9 @@ impl Rule for NoUnusedVariables {
             return None;
         }
 
-        if is_typescript_unused_ok(binding).is_some() {
+        let Some(suggestion) = suggested_fix_if_unused(binding) else {
             return None;
-        }
+        };
 
         let model = ctx.model();
         if model.is_exported(binding) {
@@ -210,7 +293,7 @@ impl Rule for NoUnusedVariables {
 
         let all_references = binding.all_references(model);
         if all_references.count() == 0 {
-            Some(())
+            Some(suggestion)
         } else {
             // We need to check if all uses of this binding are somehow recursive
 
@@ -247,7 +330,7 @@ impl Rule for NoUnusedVariables {
             }
 
             if references_outside == 0 {
-                Some(())
+                Some(suggestion)
             } else {
                 None
             }
@@ -281,23 +364,35 @@ impl Rule for NoUnusedVariables {
         Some(diag)
     }
 
-    fn action(ctx: &RuleContext<Self>, _: &Self::State) -> Option<JsRuleAction> {
-        let node = ctx.query();
-        let mut mutation = ctx.root().begin();
-        let name = node.name_token().ok()?;
-        let name_trimmed = name.text_trimmed();
-        let new_text_trimmed = format!("_{}", name_trimmed);
-        mutation.replace_node(
-            node.clone(),
-            js_identifier_binding(ident(&new_text_trimmed)),
-        );
+    fn action(ctx: &RuleContext<Self>, suggestion: &Self::State) -> Option<JsRuleAction> {
+        match suggestion {
+            SuggestedFix::NoSuggestion => None,
+            SuggestedFix::PrefixUnderscore => {
+                let binding = ctx.query();
+                let mut mutation = ctx.root().begin();
+
+                let name = match binding {
+                    AnyJsIdentifierBinding::JsIdentifierBinding(binding) => {
+                        binding.name_token().ok()?
+                    }
+                    AnyJsIdentifierBinding::TsIdentifierBinding(binding) => {
+                        binding.name_token().ok()?
+                    }
+                };
+                let name_trimmed = name.text_trimmed();
+                let new_name = format!("_{}", name_trimmed);
 
-        Some(JsRuleAction {
-            mutation,
-            category: ActionCategory::QuickFix,
-            applicability: Applicability::MaybeIncorrect,
-            message: markup! { "If this is intentional, prepend "<Emphasis>{name_trimmed}</Emphasis>" with an underscore." }
-                .to_owned(),
-        })
+                let model = ctx.model();
+                mutation.rename_node_declaration(model, binding.clone(), &new_name);
+
+                Some(JsRuleAction {
+                    mutation,
+                    category: ActionCategory::QuickFix,
+                    applicability: Applicability::MaybeIncorrect,
+                    message: markup! { "If this is intentional, prepend "<Emphasis>{name_trimmed}</Emphasis>" with an underscore." }
+                        .to_owned(),
+                })
+            }
+        }
     }
 }

crates/rome_js_analyze/src/utils/rename.rs

@@ -1,7 +1,7 @@
 use rome_js_semantic::{ReferencesExtensions, SemanticModel};
 use rome_js_syntax::{
-    JsIdentifierAssignment, JsIdentifierBinding, JsLanguage, JsReferenceIdentifier, JsSyntaxKind,
-    JsSyntaxNode, JsSyntaxToken,
+    binding_ext::AnyJsIdentifierBinding, JsIdentifierAssignment, JsIdentifierBinding, JsLanguage,
+    JsReferenceIdentifier, JsSyntaxKind, JsSyntaxNode, JsSyntaxToken,
 };
 use rome_rowan::{AstNode, BatchMutation, SyntaxNodeCast, TriviaPiece};
 use serde::{Deserialize, Serialize};
@@ -28,6 +28,13 @@ impl RenamableNode for JsIdentifierAssignment {
     }
 }
 
+impl RenamableNode for AnyJsIdentifierBinding {
+    fn binding(&self, _: &SemanticModel) -> Option<JsSyntaxNode> {
+        let node = self.syntax();
+        Some(node.clone())
+    }
+}
+
 pub enum AnyJsRenamableDeclaration {
     JsIdentifierBinding(JsIdentifierBinding),
     JsReferenceIdentifier(JsReferenceIdentifier),