feat(linter): improve no-accumulating-spread

crates/oxc_linter/src/rules/oxc/no_accumulating_spread.rs

@@ -1,11 +1,14 @@
 use oxc_ast::{
-    ast::{Argument, BindingPatternKind, CallExpression, Expression},
+    ast::{
+        Argument, BindingPatternKind, CallExpression, Expression, ForInStatement, ForOfStatement,
+        ForStatement, VariableDeclarationKind,
+    },
     AstKind,
 };
 use oxc_diagnostics::OxcDiagnostic;
 use oxc_macros::declare_oxc_lint;
-use oxc_semantic::SymbolId;
-use oxc_span::Span;
+use oxc_semantic::{AstNodeId, SymbolId};
+use oxc_span::{GetSpan, Span};
 
 use crate::{
     ast_util::{call_expr_method_callee_info, is_method_call},
@@ -14,30 +17,39 @@ use crate::{
     AstNode,
 };
 
-fn likely_array(span0: Span, span1: Span) -> OxcDiagnostic {
+fn reduce_likely_array_spread_diagnostic(spread_span: Span, reduce_span: Span) -> OxcDiagnostic {
     OxcDiagnostic::warn("Do not spread accumulators in Array.prototype.reduce()")
         .with_help("It looks like you're spreading an `Array`. Consider using the `Array.push` or `Array.concat` methods to mutate the accumulator instead.\nUsing spreads within accumulators leads to `O(n^2)` time complexity.")
         .with_labels([
-            span0.label("From this spread"),
-            span1.label("For this reduce")
+            spread_span.label("From this spread"),
+            reduce_span.label("For this reduce")
         ])
 }
 
-fn likely_object(span0: Span, span1: Span) -> OxcDiagnostic {
+fn reduce_likely_object_spread_diagnostic(spread_span: Span, reduce_span: Span) -> OxcDiagnostic {
     OxcDiagnostic::warn("Do not spread accumulators in Array.prototype.reduce()")
         .with_help("It looks like you're spreading an `Object`. Consider using the `Object.assign` or assignment operators to mutate the accumulator instead.\nUsing spreads within accumulators leads to `O(n^2)` time complexity.")
         .with_labels([
-            span0.label("From this spread"),
-            span1.label("For this reduce")
+            spread_span.label("From this spread"),
+            reduce_span.label("For this reduce")
         ])
 }
 
-fn unknown(span0: Span, span1: Span) -> OxcDiagnostic {
+fn reduce_unknown(spread_span: Span, reduce_span: Span) -> OxcDiagnostic {
     OxcDiagnostic::warn("Do not spread accumulators in Array.prototype.reduce()")
         .with_help("Consider using `Object.assign()` or `Array.prototype.push()` to mutate the accumulator instead.\nUsing spreads within accumulators leads to `O(n^2)` time complexity.")
         .with_labels([
-            span0.label("From this spread"),
-            span1.label("For this reduce")
+            spread_span.label("From this spread"),
+            reduce_span.label("For this reduce")
+        ])
+}
+
+fn loop_spread_diagnostic(spread_span: Span, loop_span: Span) -> OxcDiagnostic {
+    OxcDiagnostic::warn("Do not spread accumulators in loops")
+        .with_help("Consider using `Object.assign()` or `Array.prototype.push()` to mutate the accumulator instead.\nUsing spreads within accumulators leads to `O(n^2)` time complexity.")
+        .with_labels([
+            spread_span.label("From this spread"),
+            loop_span.label("For this loop")
         ])
 }
 
@@ -46,7 +58,7 @@ pub struct NoAccumulatingSpread;
 
 declare_oxc_lint!(
     /// ### What it does
-    /// Prevents using object or array spreads on accumulators in `Array.prototype.reduce()`.
+    /// Prevents using object or array spreads on accumulators in `Array.prototype.reduce()` and in loops.
     ///
     /// ### Why is this bad?
     /// Object and array spreads create a new object or array on each iteration.
@@ -75,12 +87,16 @@ declare_oxc_lint!(
     ///   acc[el] = { ...obj[el] }
     ///   return acc
     /// }, {})
+    ///
+    /// let foo = []; for (let i = 0; i < 10; i++) { foo.push(i); }
     /// ```
     ///
     /// Fail
     /// ```javascript
     /// arr.reduce((acc, x) => ({ ...acc, [x]: fn(x) }), {})
     /// Object.keys(obj).reduce((acc, el) => ({ ...acc, [el]: fn(el) }), {})
+    ///
+    /// let foo = []; for (let i = 0; i < 10; i++) { foo = [...foo, i]; }
     /// ```
     NoAccumulatingSpread,
     perf,
@@ -96,7 +112,6 @@ impl Rule for NoAccumulatingSpread {
             return;
         };
 
-        let nodes = ctx.semantic().nodes();
         let symbols = ctx.semantic().symbols();
 
         // get the AST node + symbol id of the declaration of the identifier
@@ -111,63 +126,142 @@ impl Rule for NoAccumulatingSpread {
         let Some(declaration) = ctx.semantic().nodes().parent_node(declaration_id) else {
             return;
         };
-        let AstKind::FormalParameters(params) = declaration.kind() else {
-            return;
-        };
 
-        // We're only looking for the first parameter, since that's where acc is.
-        // Skip non-parameter or non-first-parameter declarations.
-        let first_param_symbol_id =
-            params.items.first().and_then(|item| get_identifier_symbol_id(&item.pattern.kind));
-        if !first_param_symbol_id.is_some_and(|id| id == referenced_symbol_id) {
-            return;
-        }
+        check_reduce_usage(declaration, referenced_symbol_id, spread.span, ctx);
+        check_loop_usage(
+            declaration,
+            ctx.semantic().nodes().get_node(declaration_id),
+            referenced_symbol_id,
+            node.id(),
+            spread.span,
+            ctx,
+        );
+    }
+}
 
-        // invalid number of parameters to reduce callback
-        let params_count = params.parameters_count();
-        if params_count != 2 {
+fn check_reduce_usage<'a>(
+    declaration: &AstNode<'a>,
+    referenced_symbol_id: SymbolId,
+    spread_span: Span,
+    ctx: &LintContext<'a>,
+) {
+    let AstKind::FormalParameters(params) = declaration.kind() else {
+        return;
+    };
+
+    // We're only looking for the first parameter, since that's where acc is.
+    // Skip non-parameter or non-first-parameter declarations.
+    let first_param_symbol_id =
+        params.items.first().and_then(|item| get_identifier_symbol_id(&item.pattern.kind));
+    if !first_param_symbol_id.is_some_and(|id| id == referenced_symbol_id) {
+        return;
+    }
+
+    // invalid number of parameters to reduce callback
+    let params_count = params.parameters_count();
+    if params_count != 2 {
+        return;
+    }
+
+    // Check if the declaration resides within a call to reduce()
+    for parent in ctx.nodes().iter_parents(declaration.id()) {
+        if let AstKind::CallExpression(call_expr) = parent.kind() {
+            if is_method_call(call_expr, None, Some(&["reduce", "reduceRight"]), Some(1), Some(2)) {
+                ctx.diagnostic(get_reduce_diagnostic(call_expr, spread_span));
+            }
             return;
         }
+    }
+}
+
+fn check_loop_usage<'a>(
+    declaration_node: &AstNode<'a>,
+    declarator: &AstNode<'a>,
+    referenced_symbol_id: SymbolId,
+    spread_node_id: AstNodeId,
+    spread_span: Span,
+    ctx: &LintContext<'a>,
+) {
+    let AstKind::VariableDeclaration(declaration) = declaration_node.kind() else {
+        return;
+    };
+
+    if !matches!(declaration.kind, VariableDeclarationKind::Let) {
+        return;
+    }
+
+    if !matches!(declarator.kind(), AstKind::VariableDeclarator(_)) {
+        return;
+    }
+
+    let Some(write_reference) =
+        ctx.semantic().symbol_references(referenced_symbol_id).find(|r| r.is_write())
+    else {
+        return;
+    };
+
+    let Some(assignment_target) = ctx.nodes().parent_node(write_reference.node_id()) else {
+        return;
+    };
+
+    let AstKind::SimpleAssignmentTarget(_) = assignment_target.kind() else { return };
 
-        // Check if the declaration resides within a call to reduce()
-        for parent in nodes.iter_parents(declaration.id()) {
-            if let AstKind::CallExpression(call_expr) = parent.kind() {
-                if is_method_call(
-                    call_expr,
-                    None,
-                    Some(&["reduce", "reduceRight"]),
-                    Some(1),
-                    Some(2),
-                ) {
-                    ctx.diagnostic(get_diagnostic(call_expr, spread.span));
-                }
-                return;
+    let Some(assignment_expr) = ctx.nodes().parent_node(assignment_target.id()) else { return };
+    if !matches!(assignment_expr.kind(), AstKind::AssignmentTarget(_)) {
+        return;
+    }
+    let Some(assignment) = ctx.nodes().parent_node(assignment_expr.id()) else { return };
+    let AstKind::AssignmentExpression(assignment_expression) = assignment.kind() else {
+        return;
+    };
+
+    match assignment_expression.right.get_inner_expression() {
+        Expression::ArrayExpression(array_expr)
+            if array_expr.span.contains_inclusive(spread_span) => {}
+        Expression::ObjectExpression(object_expr)
+            if object_expr.span.contains_inclusive(spread_span) => {}
+        _ => return,
+    }
+
+    for parent in ctx.nodes().iter_parents(spread_node_id) {
+        if let Some(loop_span) = get_loop_span(parent.kind()) {
+            if !parent.kind().span().contains_inclusive(declaration.span)
+                && parent.kind().span().contains_inclusive(spread_span)
+            {
+                ctx.diagnostic(loop_spread_diagnostic(spread_span, loop_span));
             }
         }
     }
 }
 
-fn get_diagnostic<'a>(call_expr: &'a CallExpression<'a>, spread_span: Span) -> OxcDiagnostic {
+fn get_loop_span(ast_kind: AstKind) -> Option<Span> {
+    match ast_kind {
+        AstKind::ForStatement(ForStatement { span, .. })
+        | AstKind::ForOfStatement(ForOfStatement { span, .. })
+        | AstKind::ForInStatement(ForInStatement { span, .. }) => Some(Span::sized(span.start, 3)),
+        AstKind::WhileStatement(while_stmt) => Some(Span::sized(while_stmt.span.start, 5)),
+        AstKind::DoWhileStatement(do_stmt) => Some(Span::sized(do_stmt.span.start, 2)),
+        _ => None,
+    }
+}
+
+fn get_reduce_diagnostic<'a>(
+    call_expr: &'a CallExpression<'a>,
+    spread_span: Span,
+) -> OxcDiagnostic {
     // unwrap is safe because we already checked that this is a reduce call
     let (reduce_call_span, _) = call_expr_method_callee_info(call_expr).unwrap();
 
     if let Some(second_arg) = call_expr.arguments.get(1).and_then(Argument::as_expression) {
-        let second_arg = second_arg.without_parenthesized();
-        let second_arg =
-            if let Expression::TSAsExpression(as_expr) = second_arg.without_parenthesized() {
-                as_expr.expression.without_parenthesized()
-            } else {
-                second_arg
-            };
-
+        let second_arg = second_arg.get_inner_expression();
         if matches!(second_arg, Expression::ObjectExpression(_)) {
-            return likely_object(spread_span, reduce_call_span);
+            return reduce_likely_object_spread_diagnostic(spread_span, reduce_call_span);
         } else if matches!(second_arg, Expression::ArrayExpression(_)) {
-            return likely_array(spread_span, reduce_call_span);
+            return reduce_likely_array_spread_diagnostic(spread_span, reduce_call_span);
         }
     }
 
-    unknown(spread_span, reduce_call_span)
+    reduce_unknown(spread_span, reduce_call_span)
 }
 
 fn get_identifier_symbol_id(ident: &BindingPatternKind<'_>) -> Option<SymbolId> {
@@ -249,6 +343,21 @@ fn test() {
         "foo.reduce((acc) => [...acc], [])",
         // Wrong number of arguments to known method (reduce can have 1 or 2 args, but not more)
         "foo.reduce((acc, bar) => [...acc, bar], [], 123)",
+        // loops, array case
+        "let foo = []; for (let i = 0; i < 10; i++) { foo.push(i); }",
+        "let foo = []; for (const i = 0; i < 10; i++) { foo.push(i); }",
+        "let foo = []; for (let i in [1,2,3]) { foo.push(i); }",
+        "let foo = []; for (const i in [1,2,3]) { foo.push(i); }",
+        "let foo = []; for (let i of [1,2,3]) { foo.push(i); }",
+        "let foo = []; while (foo.length < 10) { foo.push(foo.length); }",
+        // loops, object case
+        "let foo = {}; for (let i = 0; i < 10; i++) { foo[i] = i; }",
+        "let foo = {}; for (const i = 0; i < 10; i++) { foo[i] = i; }",
+        "let foo = {}; for (let i in [1,2,3]) { foo[i] = i; }",
+        "let foo = {}; for (const i in [1,2,3]) { foo[i] = i; }",
+        "let foo = {}; for (let i of [1,2,3]) { foo[i] = i; }",
+        "let foo = {}; for (const i of [1,2,3]) { foo[i] = i; }",
+        "let foo = {}; while (Object.keys(foo).length < 10) { foo[Object.keys(foo).length] = Object.keys(foo).length; }",        
     ];
 
     let fail = vec![
@@ -297,6 +406,22 @@ fn test() {
         // Object - Body return with item spread
         "foo.reduce((acc, bar) => {return {...acc, ...bar};}, {})",
         "foo.reduceRight((acc, bar) => {return {...acc, ...bar};}, {})",
+        // loops, array case
+        "let foo = []; for (let i = 0; i < 10; i++) { foo = [...foo, i]; }",
+        "let foo = []; for (const i = 0; i < 10; i++) { foo = [...foo, i]; }",
+        "let foo = []; for (let i in [1,2,3]) { foo = [...foo, i]; }",
+        "let foo = []; for (const i in [1,2,3]) { foo = [...foo, i]; }",
+        "let foo = []; for (let i of [1,2,3]) { foo = [...foo, i]; }",
+        "let foo = []; for (const i of [1,2,3]) { foo = [...foo, i]; }",
+        "let foo = []; while (foo.length < 10) { foo = [...foo, foo.length]; }",
+        // loops, object case
+        "let foo = {}; for (let i = 0; i < 10; i++) { foo = { ...foo, [i]: i }; }",
+        "let foo = {}; for (const i = 0; i < 10; i++) { foo = { ...foo, [i]: i }; }",
+        "let foo = {}; for (let i in [1,2,3]) { foo = { ...foo, [i]: i }; }",
+        "let foo = {}; for (const i in [1,2,3]) { foo = { ...foo, [i]: i }; }",
+        "let foo = {}; for (let i of [1,2,3]) { foo = { ...foo, [i]: i }; }",
+        "let foo = {}; for (const i of [1,2,3]) { foo = { ...foo, [i]: i }; }",
+        "let foo = {}; while (Object.keys(foo).length < 10) { foo = { ...foo, [Object.keys(foo).length]: Object.keys(foo).length }; }",
     ];
 
     Tester::new(NoAccumulatingSpread::NAME, pass, fail).test_and_snapshot();