Skip to content

Commit 69493d2

Browse files
committedAug 31, 2024·
refactor(linter/oxc): improve diagnostic for no-accumulating-spread 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.
1 parent 024b585 commit 69493d2

File tree

2 files changed

+83
-60
lines changed

2 files changed

+83
-60
lines changed
 

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

+13-4
Original file line numberDiff line numberDiff line change
@@ -44,10 +44,15 @@ fn reduce_unknown(spread_span: Span, reduce_span: Span) -> OxcDiagnostic {
4444
])
4545
}
4646

47-
fn loop_spread_diagnostic(spread_span: Span, loop_span: Span) -> OxcDiagnostic {
47+
fn loop_spread_diagnostic(
48+
accumulator_decl_span: Span,
49+
spread_span: Span,
50+
loop_span: Span,
51+
) -> OxcDiagnostic {
4852
OxcDiagnostic::warn("Do not spread accumulators in loops")
4953
.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.")
5054
.with_labels([
55+
accumulator_decl_span.label("From this accumulator"),
5156
spread_span.label("From this spread"),
5257
loop_span.label("For this loop")
5358
])
@@ -191,9 +196,9 @@ fn check_loop_usage<'a>(
191196
return;
192197
}
193198

194-
if !matches!(declarator.kind(), AstKind::VariableDeclarator(_)) {
199+
let AstKind::VariableDeclarator(declarator) = declarator.kind() else {
195200
return;
196-
}
201+
};
197202

198203
let Some(write_reference) =
199204
ctx.semantic().symbol_references(referenced_symbol_id).find(|r| r.is_write())
@@ -229,7 +234,11 @@ fn check_loop_usage<'a>(
229234
if !parent.kind().span().contains_inclusive(declaration.span)
230235
&& parent.kind().span().contains_inclusive(spread_span)
231236
{
232-
ctx.diagnostic(loop_spread_diagnostic(spread_span, loop_span));
237+
ctx.diagnostic(loop_spread_diagnostic(
238+
declarator.id.span(),
239+
spread_span,
240+
loop_span,
241+
));
233242
}
234243
}
235244
}

‎crates/oxc_linter/src/snapshots/no_accumulating_spread.snap

+70-56
Original file line numberDiff line numberDiff line change
@@ -315,141 +315,155 @@ source: crates/oxc_linter/src/tester.rs
315315
Using spreads within accumulators leads to `O(n^2)` time complexity.
316316

317317
oxc(no-accumulating-spread): Do not spread accumulators in loops
318-
╭─[no_accumulating_spread.tsx:1:15]
318+
╭─[no_accumulating_spread.tsx:1:5]
319319
1let foo = []; for (let i = 0; i < 10; i++) { foo = [...foo, i]; }
320-
· ─┬─ ───┬──
321-
· │ ╰── From this spread
322-
· ╰── For this loop
320+
· ─┬─ ─┬─ ───┬──
321+
· │ │ ╰── From this spread
322+
· │ ╰── For this loop
323+
· ╰── From this accumulator
323324
╰────
324325
help: Consider using `Object.assign()` or `Array.prototype.push()` to mutate the accumulator instead.
325326
Using spreads within accumulators leads to `O(n^2)` time complexity.
326327

327328
oxc(no-accumulating-spread): Do not spread accumulators in loops
328-
╭─[no_accumulating_spread.tsx:1:15]
329+
╭─[no_accumulating_spread.tsx:1:5]
329330
1let foo = []; for (const i = 0; i < 10; i++) { foo = [...foo, i]; }
330-
· ─┬─ ───┬──
331-
· │ ╰── From this spread
332-
· ╰── For this loop
331+
· ─┬─ ─┬─ ───┬──
332+
· │ │ ╰── From this spread
333+
· │ ╰── For this loop
334+
· ╰── From this accumulator
333335
╰────
334336
help: Consider using `Object.assign()` or `Array.prototype.push()` to mutate the accumulator instead.
335337
Using spreads within accumulators leads to `O(n^2)` time complexity.
336338

337339
oxc(no-accumulating-spread): Do not spread accumulators in loops
338-
╭─[no_accumulating_spread.tsx:1:15]
340+
╭─[no_accumulating_spread.tsx:1:5]
339341
1let foo = []; for (let i in [1,2,3]) { foo = [...foo, i]; }
340-
· ─┬─ ───┬──
341-
· │ ╰── From this spread
342-
· ╰── For this loop
342+
· ─┬─ ─┬─ ───┬──
343+
· │ │ ╰── From this spread
344+
· │ ╰── For this loop
345+
· ╰── From this accumulator
343346
╰────
344347
help: Consider using `Object.assign()` or `Array.prototype.push()` to mutate the accumulator instead.
345348
Using spreads within accumulators leads to `O(n^2)` time complexity.
346349

347350
oxc(no-accumulating-spread): Do not spread accumulators in loops
348-
╭─[no_accumulating_spread.tsx:1:15]
351+
╭─[no_accumulating_spread.tsx:1:5]
349352
1let foo = []; for (const i in [1,2,3]) { foo = [...foo, i]; }
350-
· ─┬─ ───┬──
351-
· │ ╰── From this spread
352-
· ╰── For this loop
353+
· ─┬─ ─┬─ ───┬──
354+
· │ │ ╰── From this spread
355+
· │ ╰── For this loop
356+
· ╰── From this accumulator
353357
╰────
354358
help: Consider using `Object.assign()` or `Array.prototype.push()` to mutate the accumulator instead.
355359
Using spreads within accumulators leads to `O(n^2)` time complexity.
356360

357361
oxc(no-accumulating-spread): Do not spread accumulators in loops
358-
╭─[no_accumulating_spread.tsx:1:15]
362+
╭─[no_accumulating_spread.tsx:1:5]
359363
1let foo = []; for (let i of [1,2,3]) { foo = [...foo, i]; }
360-
· ─┬─ ───┬──
361-
· │ ╰── From this spread
362-
· ╰── For this loop
364+
· ─┬─ ─┬─ ───┬──
365+
· │ │ ╰── From this spread
366+
· │ ╰── For this loop
367+
· ╰── From this accumulator
363368
╰────
364369
help: Consider using `Object.assign()` or `Array.prototype.push()` to mutate the accumulator instead.
365370
Using spreads within accumulators leads to `O(n^2)` time complexity.
366371

367372
oxc(no-accumulating-spread): Do not spread accumulators in loops
368-
╭─[no_accumulating_spread.tsx:1:15]
373+
╭─[no_accumulating_spread.tsx:1:5]
369374
1let foo = []; for (const i of [1,2,3]) { foo = [...foo, i]; }
370-
· ─┬─ ───┬──
371-
· │ ╰── From this spread
372-
· ╰── For this loop
375+
· ─┬─ ─┬─ ───┬──
376+
· │ │ ╰── From this spread
377+
· │ ╰── For this loop
378+
· ╰── From this accumulator
373379
╰────
374380
help: Consider using `Object.assign()` or `Array.prototype.push()` to mutate the accumulator instead.
375381
Using spreads within accumulators leads to `O(n^2)` time complexity.
376382

377383
oxc(no-accumulating-spread): Do not spread accumulators in loops
378-
╭─[no_accumulating_spread.tsx:1:15]
384+
╭─[no_accumulating_spread.tsx:1:5]
379385
1let foo = []; while (foo.length < 10) { foo = [...foo, foo.length]; }
380-
· ──┬── ───┬──
381-
· │ ╰── From this spread
382-
· ╰── For this loop
386+
· ─┬─ ──┬── ───┬──
387+
· │ │ ╰── From this spread
388+
· │ ╰── For this loop
389+
· ╰── From this accumulator
383390
╰────
384391
help: Consider using `Object.assign()` or `Array.prototype.push()` to mutate the accumulator instead.
385392
Using spreads within accumulators leads to `O(n^2)` time complexity.
386393

387394
oxc(no-accumulating-spread): Do not spread accumulators in loops
388-
╭─[no_accumulating_spread.tsx:1:15]
395+
╭─[no_accumulating_spread.tsx:1:5]
389396
1let foo = {}; for (let i = 0; i < 10; i++) { foo = { ...foo, [i]: i }; }
390-
· ─┬─ ───┬──
391-
· │ ╰── From this spread
392-
· ╰── For this loop
397+
· ─┬─ ─┬─ ───┬──
398+
· │ │ ╰── From this spread
399+
· │ ╰── For this loop
400+
· ╰── From this accumulator
393401
╰────
394402
help: Consider using `Object.assign()` or `Array.prototype.push()` to mutate the accumulator instead.
395403
Using spreads within accumulators leads to `O(n^2)` time complexity.
396404

397405
oxc(no-accumulating-spread): Do not spread accumulators in loops
398-
╭─[no_accumulating_spread.tsx:1:15]
406+
╭─[no_accumulating_spread.tsx:1:5]
399407
1let foo = {}; for (const i = 0; i < 10; i++) { foo = { ...foo, [i]: i }; }
400-
· ─┬─ ───┬──
401-
· │ ╰── From this spread
402-
· ╰── For this loop
408+
· ─┬─ ─┬─ ───┬──
409+
· │ │ ╰── From this spread
410+
· │ ╰── For this loop
411+
· ╰── From this accumulator
403412
╰────
404413
help: Consider using `Object.assign()` or `Array.prototype.push()` to mutate the accumulator instead.
405414
Using spreads within accumulators leads to `O(n^2)` time complexity.
406415

407416
oxc(no-accumulating-spread): Do not spread accumulators in loops
408-
╭─[no_accumulating_spread.tsx:1:15]
417+
╭─[no_accumulating_spread.tsx:1:5]
409418
1let foo = {}; for (let i in [1,2,3]) { foo = { ...foo, [i]: i }; }
410-
· ─┬─ ───┬──
411-
· │ ╰── From this spread
412-
· ╰── For this loop
419+
· ─┬─ ─┬─ ───┬──
420+
· │ │ ╰── From this spread
421+
· │ ╰── For this loop
422+
· ╰── From this accumulator
413423
╰────
414424
help: Consider using `Object.assign()` or `Array.prototype.push()` to mutate the accumulator instead.
415425
Using spreads within accumulators leads to `O(n^2)` time complexity.
416426

417427
oxc(no-accumulating-spread): Do not spread accumulators in loops
418-
╭─[no_accumulating_spread.tsx:1:15]
428+
╭─[no_accumulating_spread.tsx:1:5]
419429
1let foo = {}; for (const i in [1,2,3]) { foo = { ...foo, [i]: i }; }
420-
· ─┬─ ───┬──
421-
· │ ╰── From this spread
422-
· ╰── For this loop
430+
· ─┬─ ─┬─ ───┬──
431+
· │ │ ╰── From this spread
432+
· │ ╰── For this loop
433+
· ╰── From this accumulator
423434
╰────
424435
help: Consider using `Object.assign()` or `Array.prototype.push()` to mutate the accumulator instead.
425436
Using spreads within accumulators leads to `O(n^2)` time complexity.
426437

427438
oxc(no-accumulating-spread): Do not spread accumulators in loops
428-
╭─[no_accumulating_spread.tsx:1:15]
439+
╭─[no_accumulating_spread.tsx:1:5]
429440
1let foo = {}; for (let i of [1,2,3]) { foo = { ...foo, [i]: i }; }
430-
· ─┬─ ───┬──
431-
· │ ╰── From this spread
432-
· ╰── For this loop
441+
· ─┬─ ─┬─ ───┬──
442+
· │ │ ╰── From this spread
443+
· │ ╰── For this loop
444+
· ╰── From this accumulator
433445
╰────
434446
help: Consider using `Object.assign()` or `Array.prototype.push()` to mutate the accumulator instead.
435447
Using spreads within accumulators leads to `O(n^2)` time complexity.
436448

437449
oxc(no-accumulating-spread): Do not spread accumulators in loops
438-
╭─[no_accumulating_spread.tsx:1:15]
450+
╭─[no_accumulating_spread.tsx:1:5]
439451
1let foo = {}; for (const i of [1,2,3]) { foo = { ...foo, [i]: i }; }
440-
· ─┬─ ───┬──
441-
· │ ╰── From this spread
442-
· ╰── For this loop
452+
· ─┬─ ─┬─ ───┬──
453+
· │ │ ╰── From this spread
454+
· │ ╰── For this loop
455+
· ╰── From this accumulator
443456
╰────
444457
help: Consider using `Object.assign()` or `Array.prototype.push()` to mutate the accumulator instead.
445458
Using spreads within accumulators leads to `O(n^2)` time complexity.
446459

447460
oxc(no-accumulating-spread): Do not spread accumulators in loops
448-
╭─[no_accumulating_spread.tsx:1:15]
461+
╭─[no_accumulating_spread.tsx:1:5]
449462
1let foo = {}; while (Object.keys(foo).length < 10) { foo = { ...foo, [Object.keys(foo).length]: Object.keys(foo).length }; }
450-
· ──┬── ───┬──
451-
· │ ╰── From this spread
452-
· ╰── For this loop
463+
· ─┬─ ──┬── ───┬──
464+
· │ │ ╰── From this spread
465+
· │ ╰── For this loop
466+
· ╰── From this accumulator
453467
╰────
454468
help: Consider using `Object.assign()` or `Array.prototype.push()` to mutate the accumulator instead.
455469
Using spreads within accumulators leads to `O(n^2)` time complexity.

0 commit comments

Comments
 (0)
Please sign in to comment.