Skip to content

Commit

Permalink
refactor(linter/oxc): improve diagnostic for no-accumulating-spread…
Browse files Browse the repository at this point in the history 
… in loops (#5374)

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 report **where** the accumulator is defined.
since this is constant for `Array.prototype.reduce`, it is not necessary.
however for loops, it makes sense to add this span to clearly show the user where the accumator is defined.
  • Loading branch information
@camc314
camc314 committed Aug 31, 2024
1 parent 024b585 commit 69493d2
Show file tree
Hide file tree
Showing 2 changed files with 83 additions and 60 deletions.
17 changes: 13 additions & 4 deletions crates/oxc_linter/src/rules/oxc/no_accumulating_spread.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -44,10 +44,15 @@ fn reduce_unknown(spread_span: Span, reduce_span: Span) -> OxcDiagnostic {
])
}

fn loop_spread_diagnostic(spread_span: Span, loop_span: Span) -> OxcDiagnostic {
fn loop_spread_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_labels([
accumulator_decl_span.label("From this accumulator"),
spread_span.label("From this spread"),
loop_span.label("For this loop")
])
Expand Down Expand Up @@ -191,9 +196,9 @@ fn check_loop_usage<'a>(
return;
}

if !matches!(declarator.kind(), AstKind::VariableDeclarator(_)) {
let AstKind::VariableDeclarator(declarator) = declarator.kind() else {
return;
}
};

let Some(write_reference) =
ctx.semantic().symbol_references(referenced_symbol_id).find(|r| r.is_write())
Expand Down Expand Up @@ -229,7 +234,11 @@ 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(spread_span, loop_span));
ctx.diagnostic(loop_spread_diagnostic(
declarator.id.span(),
spread_span,
loop_span,
));
}
}
}
Expand Down
126 changes: 70 additions & 56 deletions crates/oxc_linter/src/snapshots/no_accumulating_spread.snap
View file
Original file line number Diff line number Diff line change
Expand Up @@ -315,141 +315,155 @@ source: crates/oxc_linter/src/tester.rs
Using spreads within accumulators leads to `O(n^2)` time complexity.

⚠ oxc(no-accumulating-spread): Do not spread accumulators in loops
╭─[no_accumulating_spread.tsx:1:15]
╭─[no_accumulating_spread.tsx:1:5]
1 │ let foo = []; for (let i = 0; i < 10; i++) { foo = [...foo, i]; }
· ─┬─ ───┬──
· │ ╰── From this spread
· ╰── For this loop
· ─┬─ ─┬─ ───┬──
· │ │ ╰── From this spread
· │ ╰── For this loop
· ╰── From this accumulator
╰────
help: Consider using `Object.assign()` or `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
╭─[no_accumulating_spread.tsx:1:15]
╭─[no_accumulating_spread.tsx:1:5]
1 │ let foo = []; for (const i = 0; i < 10; i++) { foo = [...foo, i]; }
· ─┬─ ───┬──
· │ ╰── From this spread
· ╰── For this loop
· ─┬─ ─┬─ ───┬──
· │ │ ╰── From this spread
· │ ╰── For this loop
· ╰── From this accumulator
╰────
help: Consider using `Object.assign()` or `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
╭─[no_accumulating_spread.tsx:1:15]
╭─[no_accumulating_spread.tsx:1:5]
1 │ let foo = []; for (let i in [1,2,3]) { foo = [...foo, i]; }
· ─┬─ ───┬──
· │ ╰── From this spread
· ╰── For this loop
· ─┬─ ─┬─ ───┬──
· │ │ ╰── From this spread
· │ ╰── For this loop
· ╰── From this accumulator
╰────
help: Consider using `Object.assign()` or `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
╭─[no_accumulating_spread.tsx:1:15]
╭─[no_accumulating_spread.tsx:1:5]
1 │ let foo = []; for (const i in [1,2,3]) { foo = [...foo, i]; }
· ─┬─ ───┬──
· │ ╰── From this spread
· ╰── For this loop
· ─┬─ ─┬─ ───┬──
· │ │ ╰── From this spread
· │ ╰── For this loop
· ╰── From this accumulator
╰────
help: Consider using `Object.assign()` or `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
╭─[no_accumulating_spread.tsx:1:15]
╭─[no_accumulating_spread.tsx:1:5]
1 │ let foo = []; for (let i of [1,2,3]) { foo = [...foo, i]; }
· ─┬─ ───┬──
· │ ╰── From this spread
· ╰── For this loop
· ─┬─ ─┬─ ───┬──
· │ │ ╰── From this spread
· │ ╰── For this loop
· ╰── From this accumulator
╰────
help: Consider using `Object.assign()` or `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
╭─[no_accumulating_spread.tsx:1:15]
╭─[no_accumulating_spread.tsx:1:5]
1 │ let foo = []; for (const i of [1,2,3]) { foo = [...foo, i]; }
· ─┬─ ───┬──
· │ ╰── From this spread
· ╰── For this loop
· ─┬─ ─┬─ ───┬──
· │ │ ╰── From this spread
· │ ╰── For this loop
· ╰── From this accumulator
╰────
help: Consider using `Object.assign()` or `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
╭─[no_accumulating_spread.tsx:1:15]
╭─[no_accumulating_spread.tsx:1:5]
1 │ let foo = []; while (foo.length < 10) { foo = [...foo, foo.length]; }
· ──┬── ───┬──
· │ ╰── From this spread
· ╰── For this loop
· ─┬─ ──┬── ───┬──
· │ │ ╰── From this spread
· │ ╰── For this loop
· ╰── From this accumulator
╰────
help: Consider using `Object.assign()` or `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
╭─[no_accumulating_spread.tsx:1:15]
╭─[no_accumulating_spread.tsx:1:5]
1 │ let foo = {}; for (let i = 0; i < 10; i++) { foo = { ...foo, [i]: i }; }
· ─┬─ ───┬──
· │ ╰── From this spread
· ╰── For this loop
· ─┬─ ─┬─ ───┬──
· │ │ ╰── From this spread
· │ ╰── For this loop
· ╰── From this accumulator
╰────
help: Consider using `Object.assign()` or `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
╭─[no_accumulating_spread.tsx:1:15]
╭─[no_accumulating_spread.tsx:1:5]
1 │ let foo = {}; for (const i = 0; i < 10; i++) { foo = { ...foo, [i]: i }; }
· ─┬─ ───┬──
· │ ╰── From this spread
· ╰── For this loop
· ─┬─ ─┬─ ───┬──
· │ │ ╰── From this spread
· │ ╰── For this loop
· ╰── From this accumulator
╰────
help: Consider using `Object.assign()` or `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
╭─[no_accumulating_spread.tsx:1:15]
╭─[no_accumulating_spread.tsx:1:5]
1 │ let foo = {}; for (let i in [1,2,3]) { foo = { ...foo, [i]: i }; }
· ─┬─ ───┬──
· │ ╰── From this spread
· ╰── For this loop
· ─┬─ ─┬─ ───┬──
· │ │ ╰── From this spread
· │ ╰── For this loop
· ╰── From this accumulator
╰────
help: Consider using `Object.assign()` or `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
╭─[no_accumulating_spread.tsx:1:15]
╭─[no_accumulating_spread.tsx:1:5]
1 │ let foo = {}; for (const i in [1,2,3]) { foo = { ...foo, [i]: i }; }
· ─┬─ ───┬──
· │ ╰── From this spread
· ╰── For this loop
· ─┬─ ─┬─ ───┬──
· │ │ ╰── From this spread
· │ ╰── For this loop
· ╰── From this accumulator
╰────
help: Consider using `Object.assign()` or `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
╭─[no_accumulating_spread.tsx:1:15]
╭─[no_accumulating_spread.tsx:1:5]
1 │ let foo = {}; for (let i of [1,2,3]) { foo = { ...foo, [i]: i }; }
· ─┬─ ───┬──
· │ ╰── From this spread
· ╰── For this loop
· ─┬─ ─┬─ ───┬──
· │ │ ╰── From this spread
· │ ╰── For this loop
· ╰── From this accumulator
╰────
help: Consider using `Object.assign()` or `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
╭─[no_accumulating_spread.tsx:1:15]
╭─[no_accumulating_spread.tsx:1:5]
1 │ let foo = {}; for (const i of [1,2,3]) { foo = { ...foo, [i]: i }; }
· ─┬─ ───┬──
· │ ╰── From this spread
· ╰── For this loop
· ─┬─ ─┬─ ───┬──
· │ │ ╰── From this spread
· │ ╰── For this loop
· ╰── From this accumulator
╰────
help: Consider using `Object.assign()` or `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
╭─[no_accumulating_spread.tsx:1:15]
╭─[no_accumulating_spread.tsx:1:5]
1 │ let foo = {}; while (Object.keys(foo).length < 10) { foo = { ...foo, [Object.keys(foo).length]: Object.keys(foo).length }; }
· ──┬── ───┬──
· │ ╰── From this spread
· ╰── For this loop
· ─┬─ ──┬── ───┬──
· │ │ ╰── From this spread
· │ ╰── For this loop
· ╰── From this accumulator
╰────
help: Consider using `Object.assign()` or `Array.prototype.push()` to mutate the accumulator instead.
Using spreads within accumulators leads to `O(n^2)` time complexity.

0 comments on commit 69493d2

Please sign in to comment.