Skip to content

Commit

Permalink
feat(linter): improve no-accumulating-spread
Browse files Browse the repository at this point in the history
  • Loading branch information
@camc314
camc314 committed Aug 28, 2024
1 parent 7fa2fa3 commit cc648de
Show file tree
Hide file tree
Showing 2 changed files with 562 additions and 52 deletions.
218 changes: 166 additions & 52 deletions crates/oxc_linter/src/rules/oxc/no_accumulating_spread.rs
View file
Original file line number Diff line number Diff line change
@@ -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},
Expand All @@ -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")
])
}

Expand All @@ -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.
Expand Down Expand Up @@ -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,
Expand All @@ -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
Expand All @@ -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> {
Expand Down Expand Up @@ -249,6 +343,16 @@ 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 (let 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 (let i in [1,2,3]) { foo[i] = i; }",
"let foo = {}; for (let 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![
Expand Down Expand Up @@ -297,6 +401,16 @@ 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 (let i in [1,2,3]) { foo = [...foo, i]; }",
"let foo = []; for (let 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 (let i in [1,2,3]) { foo = { ...foo, [i]: i }; }",
"let foo = {}; for (let 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();
Expand Down
Loading

0 comments on commit cc648de

Please sign in to comment.