fix(linter/consistent-function-scoping): descriptive diagnostic labels

crates/oxc_linter/src/rules/unicorn/consistent_function_scoping.rs

@@ -1,14 +1,11 @@
 use rustc_hash::FxHashSet;
 
-use oxc_ast::{
-    AstKind,
-    ast::{Function, IdentifierReference, JSXElementName, ThisExpression},
-};
+use oxc_ast::{AstKind, ast::*};
 use oxc_ast_visit::{Visit, walk};
 use oxc_diagnostics::OxcDiagnostic;
 use oxc_macros::declare_oxc_lint;
-use oxc_semantic::{ReferenceId, ScopeFlags};
-use oxc_span::{GetSpan, Span};
+use oxc_semantic::{ReferenceId, ScopeFlags, ScopeId, SymbolId};
+use oxc_span::{Atom, GetSpan, Span};
 
 use crate::{
     AstNode,
@@ -18,10 +15,41 @@ use crate::{
     utils::is_react_hook,
 };
 
-fn consistent_function_scoping(span: Span) -> OxcDiagnostic {
-    OxcDiagnostic::warn("Function does not capture any variables from the outer scope.")
-        .with_help("Move this function to the outer scope.")
-        .with_label(span)
+fn consistent_function_scoping(
+    fn_span: Span,
+    parent_scope_span: Option<Span>,
+    parent_scope_kind: Option<&'static str>,
+    function_name: Option<Atom<'_>>,
+) -> OxcDiagnostic {
+    debug_assert!(Some(fn_span) != parent_scope_span);
+    let function_label = if let Some(name) = function_name {
+        format!("Function `{name}` does not capture any variables from its parent scope")
+    } else {
+        "This function does not use any variables from its parent scope".into()
+    };
+
+    let d = OxcDiagnostic::warn(function_label).with_help(match function_name {
+        Some(name) => {
+            format!("Move `{name}` to the outer scope to avoid recreating it on every call.")
+        }
+        None => {
+            "Move this function to the outer scope to avoid recreating it on every call.".into()
+        }
+    });
+
+    match parent_scope_span {
+        Some(parent) => d.with_labels([
+            parent.label("Outer scope where this function is defined"),
+            fn_span.primary_label(if let Some(parent_scope_kind) = parent_scope_kind {
+                format!(
+                    "This function does not use any variables from the parent {parent_scope_kind}"
+                )
+            } else {
+                "This function does not use any variables from here".into()
+            }),
+        ]),
+        None => d.with_label(fn_span),
+    }
 }
 
 #[derive(Debug, Default, Clone)]
@@ -148,7 +176,7 @@ impl Rule for ConsistentFunctionScoping {
     }
 
     fn run<'a>(&self, node: &AstNode<'a>, ctx: &LintContext<'a>) {
-        let (function_declaration_symbol_id, function_body, reporter_span) =
+        let (function_declaration_symbol_id, function_name, function_body, reporter_span) =
             match node.kind() {
                 AstKind::Function(function) => {
                     if function.is_typescript_syntax() {
@@ -176,14 +204,20 @@ impl Rule for ConsistentFunctionScoping {
                     if let Some(binding_ident) = get_function_like_declaration(node, ctx) {
                         (
                             binding_ident.symbol_id(),
+                            Some(binding_ident.name),
                             function_body,
                             function.id.as_ref().map_or(
                                 Span::sized(function.span.start, 8),
                                 |func_binding_ident| func_binding_ident.span,
                             ),
                         )
                     } else if let Some(function_id) = &function.id {
-                        (function_id.symbol_id(), function_body, function_id.span())
+                        (
+                            function_id.symbol_id(),
+                            Some(function_id.name),
+                            function_body,
+                            function_id.span(),
+                        )
                     } else {
                         return;
                     }
@@ -193,7 +227,12 @@ impl Rule for ConsistentFunctionScoping {
                         return;
                     };
 
-                    (binding_ident.symbol_id(), &arrow_function.body, binding_ident.span())
+                    (
+                        binding_ident.symbol_id(),
+                        Some(binding_ident.name),
+                        &arrow_function.body,
+                        binding_ident.span(),
+                    )
                 }
                 _ => return,
             };
@@ -249,7 +288,16 @@ impl Rule for ConsistentFunctionScoping {
             }
         }
 
-        ctx.diagnostic(consistent_function_scoping(reporter_span));
+        let (maybe_parent_scope_span, maybe_parent_scope_type) =
+            get_short_span_for_fn_scope(ctx, function_declaration_symbol_id, scope)
+                .map_or((None, None), |(span, rtype)| (Some(span), Some(rtype)));
+
+        ctx.diagnostic(consistent_function_scoping(
+            reporter_span,
+            maybe_parent_scope_span,
+            maybe_parent_scope_type,
+            function_name,
+        ));
     }
 }
 
@@ -311,6 +359,64 @@ fn is_in_react_hook<'a>(node: &AstNode<'a>, ctx: &LintContext<'a>) -> bool {
     false
 }
 
+fn get_short_span_for_fn_scope(
+    ctx: &LintContext<'_>,
+    function_symbol_id: SymbolId,
+    scope_id: ScopeId,
+) -> Option<(Span, &'static str)> {
+    let scoping = ctx.scoping();
+
+    debug_assert!(!scoping.scope_flags(scope_id).contains(ScopeFlags::Top));
+
+    let scope_id =
+        match ctx.nodes().parent_kind(ctx.scoping().symbol_declaration(function_symbol_id)) {
+            Some(AstKind::AssignmentExpression(_)) => {
+                ctx.scoping().scope_parent_id(scope_id).unwrap_or(scope_id)
+            }
+            _ => scope_id,
+        };
+
+    let node_creating_parent_scope = ctx.nodes().get_node(scoping.get_node_id(scope_id));
+
+    match node_creating_parent_scope.kind() {
+        AstKind::Function(f) => f.id.as_ref().map(|id| (id.span(), "function")),
+        AstKind::ArrowFunctionExpression(_) => {
+            let parent = ctx.nodes().parent_kind(node_creating_parent_scope.id())?;
+            match parent {
+                AstKind::VariableDeclarator(v) => Some((v.id.span(), "arrow function")),
+                AstKind::AssignmentExpression(a) => Some((a.left.span(), "arrow function")),
+                _ => None,
+            }
+        }
+        AstKind::Class(c) => c.id.as_ref().map(|id| (id.span(), "class")),
+        // only cover keywords of control flow statements
+        AstKind::ForInStatement(ForInStatement { span, .. })
+        | AstKind::ForOfStatement(ForOfStatement { span, .. })
+        | AstKind::ForStatement(ForStatement { span, .. }) => {
+            Some((Span::sized(span.start, 3), "for loop"))
+        }
+        AstKind::TryStatement(TryStatement { span, .. }) => {
+            Some((Span::sized(span.start, 3), "try statement"))
+        }
+        AstKind::IfStatement(IfStatement { span, .. }) => {
+            Some((Span::sized(span.start, 2), "if statement"))
+        }
+        AstKind::DoWhileStatement(DoWhileStatement { span, .. }) => {
+            Some((Span::sized(span.start, 2), "do while statement"))
+        }
+        AstKind::SwitchStatement(SwitchStatement { span, .. }) => {
+            Some((Span::sized(span.start, 6), "switch statement"))
+        }
+        AstKind::WhileStatement(WhileStatement { span, .. }) => {
+            Some((Span::sized(span.start, 5), "while statement"))
+        }
+        AstKind::CatchClause(CatchClause { span, .. }) => {
+            Some((Span::sized(span.start, 5), "catch block"))
+        }
+        _ => None,
+    }
+}
+
 #[test]
 fn test() {
     use crate::tester::Tester;