feat(linter/oxc): differentiate between array/object in `no-accumulat…

…ing-spread` loop diagnostic (#5375) when reporting diagnotics for code such as ```ts let foo = {}; for (let i = 0; i < 10; i++) { foo = { ...foo, [i]: i }; } ``` we do not currently differentiate the diagnostic message between object/array. this PR changes this to help the user fix ths issue
oxc-project · Aug 31, 2024 · 8d781e7 · 8d781e7
1 parent 69493d2
commit 8d781e7
Show file tree

Hide file tree

Showing 2 changed files with 49 additions and 22 deletions.
diff --git a/crates/oxc_linter/src/rules/oxc/no_accumulating_spread.rs b/crates/oxc_linter/src/rules/oxc/no_accumulating_spread.rs
@@ -44,13 +44,26 @@ fn reduce_unknown(spread_span: Span, reduce_span: Span) -> OxcDiagnostic {
         ])
 }
 
-fn loop_spread_diagnostic(
+fn loop_spread_likely_object_diagnostic(
     accumulator_decl_span: Span,
     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_help("Consider using `Object.assign()` to mutate the accumulator instead.\nUsing spreads within accumulators leads to `O(n^2)` time complexity.")
+        .with_labels([
+            accumulator_decl_span.label("From this accumulator"),
+            spread_span.label("From this spread"),
+            loop_span.label("For this loop")
+        ])
+}
+fn loop_spread_likely_array_diagnostic(
+    accumulator_decl_span: Span,
+    spread_span: Span,
+    loop_span: Span,
+) -> OxcDiagnostic {
+    OxcDiagnostic::warn("Do not spread accumulators in loops")
+        .with_help("Consider using `Array.prototype.push()` to mutate the accumulator instead.\nUsing spreads within accumulators leads to `O(n^2)` time complexity.")
         .with_labels([
             accumulator_decl_span.label("From this accumulator"),
             spread_span.label("From this spread"),
@@ -221,7 +234,8 @@ fn check_loop_usage<'a>(
         return;
     };
 
-    match assignment_expression.right.get_inner_expression() {
+    let assignment_expression_right_inner_expr = assignment_expression.right.get_inner_expression();
+    match assignment_expression_right_inner_expr {
         Expression::ArrayExpression(array_expr)
             if array_expr.span.contains_inclusive(spread_span) => {}
         Expression::ObjectExpression(object_expr)
@@ -234,11 +248,24 @@ fn check_loop_usage<'a>(
             if !parent.kind().span().contains_inclusive(declaration.span)
                 && parent.kind().span().contains_inclusive(spread_span)
             {
-                ctx.diagnostic(loop_spread_diagnostic(
-                    declarator.id.span(),
-                    spread_span,
-                    loop_span,
-                ));
+                match assignment_expression_right_inner_expr {
+                    Expression::ArrayExpression(_) => {
+                        ctx.diagnostic(loop_spread_likely_array_diagnostic(
+                            declarator.id.span(),
+                            spread_span,
+                            loop_span,
+                        ));
+                    }
+                    Expression::ObjectExpression(_) => {
+                        ctx.diagnostic(loop_spread_likely_object_diagnostic(
+                            declarator.id.span(),
+                            spread_span,
+                            loop_span,
+                        ));
+                    }
+                    // we check above that the expression is either an array or object expression
+                    _ => unreachable!(),
+                }
             }
         }
     }

diff --git a/crates/oxc_linter/src/snapshots/no_accumulating_spread.snap b/crates/oxc_linter/src/snapshots/no_accumulating_spread.snap
@@ -322,7 +322,7 @@ source: crates/oxc_linter/src/tester.rs
    ·      │         ╰── For this loop
    ·      ╰── From this accumulator
    ╰────
-  help: Consider using `Object.assign()` or `Array.prototype.push()` to mutate the accumulator instead.
+  help: Consider using `Array.prototype.push()` to mutate the accumulator instead.
         Using spreads within accumulators leads to `O(n^2)` time complexity.
 
   ⚠ oxc(no-accumulating-spread): Do not spread accumulators in loops
@@ -333,7 +333,7 @@ source: crates/oxc_linter/src/tester.rs
    ·      │         ╰── For this loop
    ·      ╰── From this accumulator
    ╰────
-  help: Consider using `Object.assign()` or `Array.prototype.push()` to mutate the accumulator instead.
+  help: Consider using `Array.prototype.push()` to mutate the accumulator instead.
         Using spreads within accumulators leads to `O(n^2)` time complexity.
 
   ⚠ oxc(no-accumulating-spread): Do not spread accumulators in loops
@@ -344,7 +344,7 @@ source: crates/oxc_linter/src/tester.rs
    ·      │         ╰── For this loop
    ·      ╰── From this accumulator
    ╰────
-  help: Consider using `Object.assign()` or `Array.prototype.push()` to mutate the accumulator instead.
+  help: Consider using `Array.prototype.push()` to mutate the accumulator instead.
         Using spreads within accumulators leads to `O(n^2)` time complexity.
 
   ⚠ oxc(no-accumulating-spread): Do not spread accumulators in loops
@@ -355,7 +355,7 @@ source: crates/oxc_linter/src/tester.rs
    ·      │         ╰── For this loop
    ·      ╰── From this accumulator
    ╰────
-  help: Consider using `Object.assign()` or `Array.prototype.push()` to mutate the accumulator instead.
+  help: Consider using `Array.prototype.push()` to mutate the accumulator instead.
         Using spreads within accumulators leads to `O(n^2)` time complexity.
 
   ⚠ oxc(no-accumulating-spread): Do not spread accumulators in loops
@@ -366,7 +366,7 @@ source: crates/oxc_linter/src/tester.rs
    ·      │         ╰── For this loop
    ·      ╰── From this accumulator
    ╰────
-  help: Consider using `Object.assign()` or `Array.prototype.push()` to mutate the accumulator instead.
+  help: Consider using `Array.prototype.push()` to mutate the accumulator instead.
         Using spreads within accumulators leads to `O(n^2)` time complexity.
 
   ⚠ oxc(no-accumulating-spread): Do not spread accumulators in loops
@@ -377,7 +377,7 @@ source: crates/oxc_linter/src/tester.rs
    ·      │         ╰── For this loop
    ·      ╰── From this accumulator
    ╰────
-  help: Consider using `Object.assign()` or `Array.prototype.push()` to mutate the accumulator instead.
+  help: Consider using `Array.prototype.push()` to mutate the accumulator instead.
         Using spreads within accumulators leads to `O(n^2)` time complexity.
 
   ⚠ oxc(no-accumulating-spread): Do not spread accumulators in loops
@@ -388,7 +388,7 @@ source: crates/oxc_linter/src/tester.rs
    ·      │          ╰── For this loop
    ·      ╰── From this accumulator
    ╰────
-  help: Consider using `Object.assign()` or `Array.prototype.push()` to mutate the accumulator instead.
+  help: Consider using `Array.prototype.push()` to mutate the accumulator instead.
         Using spreads within accumulators leads to `O(n^2)` time complexity.
 
   ⚠ oxc(no-accumulating-spread): Do not spread accumulators in loops
@@ -399,7 +399,7 @@ source: crates/oxc_linter/src/tester.rs
    ·      │         ╰── For this loop
    ·      ╰── From this accumulator
    ╰────
-  help: Consider using `Object.assign()` or `Array.prototype.push()` to mutate the accumulator instead.
+  help: Consider using `Object.assign()` to mutate the accumulator instead.
         Using spreads within accumulators leads to `O(n^2)` time complexity.
 
   ⚠ oxc(no-accumulating-spread): Do not spread accumulators in loops
@@ -410,7 +410,7 @@ source: crates/oxc_linter/src/tester.rs
    ·      │         ╰── For this loop
    ·      ╰── From this accumulator
    ╰────
-  help: Consider using `Object.assign()` or `Array.prototype.push()` to mutate the accumulator instead.
+  help: Consider using `Object.assign()` to mutate the accumulator instead.
         Using spreads within accumulators leads to `O(n^2)` time complexity.
 
   ⚠ oxc(no-accumulating-spread): Do not spread accumulators in loops
@@ -421,7 +421,7 @@ source: crates/oxc_linter/src/tester.rs
    ·      │         ╰── For this loop
    ·      ╰── From this accumulator
    ╰────
-  help: Consider using `Object.assign()` or `Array.prototype.push()` to mutate the accumulator instead.
+  help: Consider using `Object.assign()` to mutate the accumulator instead.
         Using spreads within accumulators leads to `O(n^2)` time complexity.
 
   ⚠ oxc(no-accumulating-spread): Do not spread accumulators in loops
@@ -432,7 +432,7 @@ source: crates/oxc_linter/src/tester.rs
    ·      │         ╰── For this loop
    ·      ╰── From this accumulator
    ╰────
-  help: Consider using `Object.assign()` or `Array.prototype.push()` to mutate the accumulator instead.
+  help: Consider using `Object.assign()` to mutate the accumulator instead.
         Using spreads within accumulators leads to `O(n^2)` time complexity.
 
   ⚠ oxc(no-accumulating-spread): Do not spread accumulators in loops
@@ -443,7 +443,7 @@ source: crates/oxc_linter/src/tester.rs
    ·      │         ╰── For this loop
    ·      ╰── From this accumulator
    ╰────
-  help: Consider using `Object.assign()` or `Array.prototype.push()` to mutate the accumulator instead.
+  help: Consider using `Object.assign()` to mutate the accumulator instead.
         Using spreads within accumulators leads to `O(n^2)` time complexity.
 
   ⚠ oxc(no-accumulating-spread): Do not spread accumulators in loops
@@ -454,7 +454,7 @@ source: crates/oxc_linter/src/tester.rs
    ·      │         ╰── For this loop
    ·      ╰── From this accumulator
    ╰────
-  help: Consider using `Object.assign()` or `Array.prototype.push()` to mutate the accumulator instead.
+  help: Consider using `Object.assign()` to mutate the accumulator instead.
         Using spreads within accumulators leads to `O(n^2)` time complexity.
 
   ⚠ oxc(no-accumulating-spread): Do not spread accumulators in loops
@@ -465,5 +465,5 @@ source: crates/oxc_linter/src/tester.rs
    ·      │          ╰── For this loop
    ·      ╰── From this accumulator
    ╰────
-  help: Consider using `Object.assign()` or `Array.prototype.push()` to mutate the accumulator instead.
+  help: Consider using `Object.assign()` to mutate the accumulator instead.
         Using spreads within accumulators leads to `O(n^2)` time complexity.