Skip to content

Commit b103737

Browse files
camc314camc314
committed
feat(linter): improve no-accumulating-spread (#5302)
VSCode has a couple violations. examples: ``` x oxc(no-accumulating-spread): Do not spread accumulators in loops ,-[src/vs/workbench/services/textMate/common/TMGrammarFactory.ts:65:5] 64 | let injections: string[] = []; 65 | for (let i = 1; i <= scopeParts.length; i++) { : ^|^ : `-- For this loop 66 | const subScopeName = scopeParts.slice(0, i).join('.'); 67 | injections = [...injections, ...(this._injections[subScopeName] || [])]; : ^^^^^^|^^^^^^ : `-- From this spread 68 | } `---- 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. x oxc(no-accumulating-spread): Do not spread accumulators in loops ,-[src/vs/base/common/actions.ts:205:3] 204 | let out: IAction[] = []; 205 | for (const list of actionLists) { : ^|^ : `-- For this loop 206 | if (!list.length) { 207 | // skip 208 | } else if (out.length) { 209 | out = [...out, new Separator(), ...list]; : ^^^|^^ : `-- From this spread 210 | } else { `---- 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. help: It looks like you're spreading an `Array`. Consider using the `Array.push` or `Array.concat` methods to mutate the accumulator instead. Using spreads within accumulators leads to `O(n^2)` time complexity. x oxc(no-accumulating-spread): Do not spread accumulators in loops ,-[src/vs/workbench/contrib/extensions/browser/extensionsActions.ts:302:3] 301 | let actions: IAction[] = []; 302 | for (const visibleActions of actionsGroups) { : ^|^ : `-- For this loop 303 | if (visibleActions.length) { 304 | actions = [...actions, ...visibleActions, new Separator()]; : ^^^^^|^^^^ : `-- From this spread 305 | } `---- 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. x oxc(no-accumulating-spread): Do not spread accumulators in loops ,-[src/vs/workbench/contrib/extensions/browser/extensionsActions.ts:1141:3] 1140 | let actions: IAction[] = []; 1141 | for (const menuActions of menuActionGroups) { : ^|^ : `-- For this loop 1142 | actions = [...actions, ...menuActions, new Separator()]; : ^^^^^|^^^^ : `-- From this spread 1143 | } `---- 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. x oxc(no-accumulating-spread): Do not spread accumulators in loops ,-[src/vs/workbench/contrib/extensions/browser/extensionsViews.ts:334:4] 333 | let actions: IAction[] = []; 334 | for (const menuActions of groups) { : ^|^ : `-- For this loop 335 | actions = [...actions, ...menuActions, new Separator()]; : ^^^^^|^^^^ : `-- From this spread 336 | } `---- 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. ```
1 parent da8aa18 commit b103737

File tree

2 files changed

+317
-52
lines changed

2 files changed

+317
-52
lines changed

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

+177-52
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,14 @@
11
use oxc_ast::{
2-
ast::{Argument, BindingPatternKind, CallExpression, Expression},
2+
ast::{
3+
Argument, BindingPatternKind, CallExpression, Expression, ForInStatement, ForOfStatement,
4+
ForStatement, VariableDeclarationKind,
5+
},
36
AstKind,
47
};
58
use oxc_diagnostics::OxcDiagnostic;
69
use oxc_macros::declare_oxc_lint;
7-
use oxc_semantic::SymbolId;
8-
use oxc_span::Span;
10+
use oxc_semantic::{AstNodeId, SymbolId};
11+
use oxc_span::{GetSpan, Span};
912

1013
use crate::{
1114
ast_util::{call_expr_method_callee_info, is_method_call},
@@ -14,30 +17,39 @@ use crate::{
1417
AstNode,
1518
};
1619

17-
fn likely_array(span0: Span, span1: Span) -> OxcDiagnostic {
20+
fn reduce_likely_array_spread_diagnostic(spread_span: Span, reduce_span: Span) -> OxcDiagnostic {
1821
OxcDiagnostic::warn("Do not spread accumulators in Array.prototype.reduce()")
1922
.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.")
2023
.with_labels([
21-
span0.label("From this spread"),
22-
span1.label("For this reduce")
24+
spread_span.label("From this spread"),
25+
reduce_span.label("For this reduce")
2326
])
2427
}
2528

26-
fn likely_object(span0: Span, span1: Span) -> OxcDiagnostic {
29+
fn reduce_likely_object_spread_diagnostic(spread_span: Span, reduce_span: Span) -> OxcDiagnostic {
2730
OxcDiagnostic::warn("Do not spread accumulators in Array.prototype.reduce()")
2831
.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.")
2932
.with_labels([
30-
span0.label("From this spread"),
31-
span1.label("For this reduce")
33+
spread_span.label("From this spread"),
34+
reduce_span.label("For this reduce")
3235
])
3336
}
3437

35-
fn unknown(span0: Span, span1: Span) -> OxcDiagnostic {
38+
fn reduce_unknown(spread_span: Span, reduce_span: Span) -> OxcDiagnostic {
3639
OxcDiagnostic::warn("Do not spread accumulators in Array.prototype.reduce()")
3740
.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.")
3841
.with_labels([
39-
span0.label("From this spread"),
40-
span1.label("For this reduce")
42+
spread_span.label("From this spread"),
43+
reduce_span.label("For this reduce")
44+
])
45+
}
46+
47+
fn loop_spread_diagnostic(spread_span: Span, loop_span: Span) -> OxcDiagnostic {
48+
OxcDiagnostic::warn("Do not spread accumulators in loops")
49+
.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.")
50+
.with_labels([
51+
spread_span.label("From this spread"),
52+
loop_span.label("For this loop")
4153
])
4254
}
4355

@@ -46,7 +58,7 @@ pub struct NoAccumulatingSpread;
4658

4759
declare_oxc_lint!(
4860
/// ### What it does
49-
/// Prevents using object or array spreads on accumulators in `Array.prototype.reduce()`.
61+
/// Prevents using object or array spreads on accumulators in `Array.prototype.reduce()` and in loops.
5062
///
5163
/// ### Why is this bad?
5264
/// Object and array spreads create a new object or array on each iteration.
@@ -75,12 +87,16 @@ declare_oxc_lint!(
7587
/// acc[el] = { ...obj[el] }
7688
/// return acc
7789
/// }, {})
90+
///
91+
/// let foo = []; for (let i = 0; i < 10; i++) { foo.push(i); }
7892
/// ```
7993
///
8094
/// Fail
8195
/// ```javascript
8296
/// arr.reduce((acc, x) => ({ ...acc, [x]: fn(x) }), {})
8397
/// Object.keys(obj).reduce((acc, el) => ({ ...acc, [el]: fn(el) }), {})
98+
///
99+
/// let foo = []; for (let i = 0; i < 10; i++) { foo = [...foo, i]; }
84100
/// ```
85101
NoAccumulatingSpread,
86102
perf,
@@ -96,7 +112,6 @@ impl Rule for NoAccumulatingSpread {
96112
return;
97113
};
98114

99-
let nodes = ctx.semantic().nodes();
100115
let symbols = ctx.semantic().symbols();
101116

102117
// get the AST node + symbol id of the declaration of the identifier
@@ -111,63 +126,142 @@ impl Rule for NoAccumulatingSpread {
111126
let Some(declaration) = ctx.semantic().nodes().parent_node(declaration_id) else {
112127
return;
113128
};
114-
let AstKind::FormalParameters(params) = declaration.kind() else {
115-
return;
116-
};
117129

118-
// We're only looking for the first parameter, since that's where acc is.
119-
// Skip non-parameter or non-first-parameter declarations.
120-
let first_param_symbol_id =
121-
params.items.first().and_then(|item| get_identifier_symbol_id(&item.pattern.kind));
122-
if !first_param_symbol_id.is_some_and(|id| id == referenced_symbol_id) {
123-
return;
124-
}
130+
check_reduce_usage(declaration, referenced_symbol_id, spread.span, ctx);
131+
check_loop_usage(
132+
declaration,
133+
ctx.semantic().nodes().get_node(declaration_id),
134+
referenced_symbol_id,
135+
node.id(),
136+
spread.span,
137+
ctx,
138+
);
139+
}
140+
}
125141

126-
// invalid number of parameters to reduce callback
127-
let params_count = params.parameters_count();
128-
if params_count != 2 {
142+
fn check_reduce_usage<'a>(
143+
declaration: &AstNode<'a>,
144+
referenced_symbol_id: SymbolId,
145+
spread_span: Span,
146+
ctx: &LintContext<'a>,
147+
) {
148+
let AstKind::FormalParameters(params) = declaration.kind() else {
149+
return;
150+
};
151+
152+
// We're only looking for the first parameter, since that's where acc is.
153+
// Skip non-parameter or non-first-parameter declarations.
154+
let first_param_symbol_id =
155+
params.items.first().and_then(|item| get_identifier_symbol_id(&item.pattern.kind));
156+
if !first_param_symbol_id.is_some_and(|id| id == referenced_symbol_id) {
157+
return;
158+
}
159+
160+
// invalid number of parameters to reduce callback
161+
let params_count = params.parameters_count();
162+
if params_count != 2 {
163+
return;
164+
}
165+
166+
// Check if the declaration resides within a call to reduce()
167+
for parent in ctx.nodes().iter_parents(declaration.id()) {
168+
if let AstKind::CallExpression(call_expr) = parent.kind() {
169+
if is_method_call(call_expr, None, Some(&["reduce", "reduceRight"]), Some(1), Some(2)) {
170+
ctx.diagnostic(get_reduce_diagnostic(call_expr, spread_span));
171+
}
129172
return;
130173
}
174+
}
175+
}
176+
177+
fn check_loop_usage<'a>(
178+
declaration_node: &AstNode<'a>,
179+
declarator: &AstNode<'a>,
180+
referenced_symbol_id: SymbolId,
181+
spread_node_id: AstNodeId,
182+
spread_span: Span,
183+
ctx: &LintContext<'a>,
184+
) {
185+
let AstKind::VariableDeclaration(declaration) = declaration_node.kind() else {
186+
return;
187+
};
188+
189+
if !matches!(declaration.kind, VariableDeclarationKind::Let) {
190+
return;
191+
}
192+
193+
if !matches!(declarator.kind(), AstKind::VariableDeclarator(_)) {
194+
return;
195+
}
196+
197+
let Some(write_reference) =
198+
ctx.semantic().symbol_references(referenced_symbol_id).find(|r| r.is_write())
199+
else {
200+
return;
201+
};
202+
203+
let Some(assignment_target) = ctx.nodes().parent_node(write_reference.node_id()) else {
204+
return;
205+
};
206+
207+
let AstKind::SimpleAssignmentTarget(_) = assignment_target.kind() else { return };
131208

132-
// Check if the declaration resides within a call to reduce()
133-
for parent in nodes.iter_parents(declaration.id()) {
134-
if let AstKind::CallExpression(call_expr) = parent.kind() {
135-
if is_method_call(
136-
call_expr,
137-
None,
138-
Some(&["reduce", "reduceRight"]),
139-
Some(1),
140-
Some(2),
141-
) {
142-
ctx.diagnostic(get_diagnostic(call_expr, spread.span));
143-
}
144-
return;
209+
let Some(assignment_expr) = ctx.nodes().parent_node(assignment_target.id()) else { return };
210+
if !matches!(assignment_expr.kind(), AstKind::AssignmentTarget(_)) {
211+
return;
212+
}
213+
let Some(assignment) = ctx.nodes().parent_node(assignment_expr.id()) else { return };
214+
let AstKind::AssignmentExpression(assignment_expression) = assignment.kind() else {
215+
return;
216+
};
217+
218+
match assignment_expression.right.get_inner_expression() {
219+
Expression::ArrayExpression(array_expr)
220+
if array_expr.span.contains_inclusive(spread_span) => {}
221+
Expression::ObjectExpression(object_expr)
222+
if object_expr.span.contains_inclusive(spread_span) => {}
223+
_ => return,
224+
}
225+
226+
for parent in ctx.nodes().iter_parents(spread_node_id) {
227+
if let Some(loop_span) = get_loop_span(parent.kind()) {
228+
if !parent.kind().span().contains_inclusive(declaration.span)
229+
&& parent.kind().span().contains_inclusive(spread_span)
230+
{
231+
ctx.diagnostic(loop_spread_diagnostic(spread_span, loop_span));
145232
}
146233
}
147234
}
148235
}
149236

150-
fn get_diagnostic<'a>(call_expr: &'a CallExpression<'a>, spread_span: Span) -> OxcDiagnostic {
237+
fn get_loop_span(ast_kind: AstKind) -> Option<Span> {
238+
match ast_kind {
239+
AstKind::ForStatement(ForStatement { span, .. })
240+
| AstKind::ForOfStatement(ForOfStatement { span, .. })
241+
| AstKind::ForInStatement(ForInStatement { span, .. }) => Some(Span::sized(span.start, 3)),
242+
AstKind::WhileStatement(while_stmt) => Some(Span::sized(while_stmt.span.start, 5)),
243+
AstKind::DoWhileStatement(do_stmt) => Some(Span::sized(do_stmt.span.start, 2)),
244+
_ => None,
245+
}
246+
}
247+
248+
fn get_reduce_diagnostic<'a>(
249+
call_expr: &'a CallExpression<'a>,
250+
spread_span: Span,
251+
) -> OxcDiagnostic {
151252
// unwrap is safe because we already checked that this is a reduce call
152253
let (reduce_call_span, _) = call_expr_method_callee_info(call_expr).unwrap();
153254

154255
if let Some(second_arg) = call_expr.arguments.get(1).and_then(Argument::as_expression) {
155-
let second_arg = second_arg.without_parenthesized();
156-
let second_arg =
157-
if let Expression::TSAsExpression(as_expr) = second_arg.without_parenthesized() {
158-
as_expr.expression.without_parenthesized()
159-
} else {
160-
second_arg
161-
};
162-
256+
let second_arg = second_arg.get_inner_expression();
163257
if matches!(second_arg, Expression::ObjectExpression(_)) {
164-
return likely_object(spread_span, reduce_call_span);
258+
return reduce_likely_object_spread_diagnostic(spread_span, reduce_call_span);
165259
} else if matches!(second_arg, Expression::ArrayExpression(_)) {
166-
return likely_array(spread_span, reduce_call_span);
260+
return reduce_likely_array_spread_diagnostic(spread_span, reduce_call_span);
167261
}
168262
}
169263

170-
unknown(spread_span, reduce_call_span)
264+
reduce_unknown(spread_span, reduce_call_span)
171265
}
172266

173267
fn get_identifier_symbol_id(ident: &BindingPatternKind<'_>) -> Option<SymbolId> {
@@ -249,6 +343,21 @@ fn test() {
249343
"foo.reduce((acc) => [...acc], [])",
250344
// Wrong number of arguments to known method (reduce can have 1 or 2 args, but not more)
251345
"foo.reduce((acc, bar) => [...acc, bar], [], 123)",
346+
// loops, array case
347+
"let foo = []; for (let i = 0; i < 10; i++) { foo.push(i); }",
348+
"let foo = []; for (const i = 0; i < 10; i++) { foo.push(i); }",
349+
"let foo = []; for (let i in [1,2,3]) { foo.push(i); }",
350+
"let foo = []; for (const i in [1,2,3]) { foo.push(i); }",
351+
"let foo = []; for (let i of [1,2,3]) { foo.push(i); }",
352+
"let foo = []; while (foo.length < 10) { foo.push(foo.length); }",
353+
// loops, object case
354+
"let foo = {}; for (let i = 0; i < 10; i++) { foo[i] = i; }",
355+
"let foo = {}; for (const i = 0; i < 10; i++) { foo[i] = i; }",
356+
"let foo = {}; for (let i in [1,2,3]) { foo[i] = i; }",
357+
"let foo = {}; for (const i in [1,2,3]) { foo[i] = i; }",
358+
"let foo = {}; for (let i of [1,2,3]) { foo[i] = i; }",
359+
"let foo = {}; for (const i of [1,2,3]) { foo[i] = i; }",
360+
"let foo = {}; while (Object.keys(foo).length < 10) { foo[Object.keys(foo).length] = Object.keys(foo).length; }",
252361
];
253362

254363
let fail = vec![
@@ -297,6 +406,22 @@ fn test() {
297406
// Object - Body return with item spread
298407
"foo.reduce((acc, bar) => {return {...acc, ...bar};}, {})",
299408
"foo.reduceRight((acc, bar) => {return {...acc, ...bar};}, {})",
409+
// loops, array case
410+
"let foo = []; for (let i = 0; i < 10; i++) { foo = [...foo, i]; }",
411+
"let foo = []; for (const i = 0; i < 10; i++) { foo = [...foo, i]; }",
412+
"let foo = []; for (let i in [1,2,3]) { foo = [...foo, i]; }",
413+
"let foo = []; for (const i in [1,2,3]) { foo = [...foo, i]; }",
414+
"let foo = []; for (let i of [1,2,3]) { foo = [...foo, i]; }",
415+
"let foo = []; for (const i of [1,2,3]) { foo = [...foo, i]; }",
416+
"let foo = []; while (foo.length < 10) { foo = [...foo, foo.length]; }",
417+
// loops, object case
418+
"let foo = {}; for (let i = 0; i < 10; i++) { foo = { ...foo, [i]: i }; }",
419+
"let foo = {}; for (const i = 0; i < 10; i++) { foo = { ...foo, [i]: i }; }",
420+
"let foo = {}; for (let i in [1,2,3]) { foo = { ...foo, [i]: i }; }",
421+
"let foo = {}; for (const i in [1,2,3]) { foo = { ...foo, [i]: i }; }",
422+
"let foo = {}; for (let i of [1,2,3]) { foo = { ...foo, [i]: i }; }",
423+
"let foo = {}; for (const i of [1,2,3]) { foo = { ...foo, [i]: i }; }",
424+
"let foo = {}; while (Object.keys(foo).length < 10) { foo = { ...foo, [Object.keys(foo).length]: Object.keys(foo).length }; }",
300425
];
301426

302427
Tester::new(NoAccumulatingSpread::NAME, pass, fail).test_and_snapshot();

0 commit comments

Comments
 (0)