diff --git a/crates/oxc_linter/src/ast_util.rs b/crates/oxc_linter/src/ast_util.rs index 501c7d77e2c659..a7b0f44ff3712f 100644 --- a/crates/oxc_linter/src/ast_util.rs +++ b/crates/oxc_linter/src/ast_util.rs @@ -243,6 +243,7 @@ pub fn nth_outermost_paren_parent<'a, 'b>( .filter(|parent| !matches!(parent.kind(), AstKind::ParenthesizedExpression(_))) .nth(n) } + /// Iterate over parents of `node`, skipping nodes that are also ignored by /// [`Expression::get_inner_expression`]. pub fn iter_outer_expressions<'a, 'ctx>( @@ -414,3 +415,34 @@ pub fn get_function_like_declaration<'b>( decl.id.get_binding_identifier() } + +/// Get the first identifier reference within a member expression chain or +/// standalone reference. +/// +/// For example, when called on the right-hand side of this [`AssignmentExpression`]: +/// ```ts +/// let x = a +/// // ^ +/// let y = a.b.c +/// // ^ +/// ``` +/// +/// As this function walks down the member expression chain, if no identifier +/// reference is found, it returns [`Err`] with the leftmost expression. +/// ```ts +/// let x = 1 + 1 +/// // ^^^^^ Err(BinaryExpression) +/// let y = this.foo.bar +/// // ^^^^ Err(ThisExpression) +/// ``` +pub fn leftmost_identifier_reference<'a, 'b: 'a>( + expr: &'b Expression<'a>, +) -> Result<&'a IdentifierReference<'a>, &'b Expression<'a>> { + match expr { + Expression::Identifier(ident) => Ok(ident.as_ref()), + Expression::StaticMemberExpression(mem) => leftmost_identifier_reference(&mem.object), + Expression::ComputedMemberExpression(mem) => leftmost_identifier_reference(&mem.object), + Expression::PrivateFieldExpression(mem) => leftmost_identifier_reference(&mem.object), + _ => Err(expr), + } +} diff --git a/crates/oxc_linter/src/rules.rs b/crates/oxc_linter/src/rules.rs index 6d2aa5b5a968f7..007a543d606e98 100644 --- a/crates/oxc_linter/src/rules.rs +++ b/crates/oxc_linter/src/rules.rs @@ -410,6 +410,7 @@ mod oxc { pub mod no_async_endpoint_handlers; pub mod no_barrel_file; pub mod no_const_enum; + pub mod no_map_spread; pub mod no_optional_chaining; pub mod no_rest_spread_properties; pub mod number_arg_out_of_range; @@ -761,6 +762,7 @@ oxc_macros::declare_all_lint_rules! { oxc::no_async_endpoint_handlers, oxc::no_barrel_file, oxc::no_const_enum, + oxc::no_map_spread, oxc::no_optional_chaining, oxc::no_rest_spread_properties, oxc::number_arg_out_of_range, diff --git a/crates/oxc_linter/src/rules/oxc/no_map_spread.rs b/crates/oxc_linter/src/rules/oxc/no_map_spread.rs new file mode 100644 index 00000000000000..8838202cc74dd2 --- /dev/null +++ b/crates/oxc_linter/src/rules/oxc/no_map_spread.rs @@ -0,0 +1,545 @@ +use oxc_semantic::{Reference, ReferenceId, ScopeId, SymbolId}; +use serde::{Deserialize, Serialize}; +use std::ops::Deref; + +use oxc_ast::{ + ast::{ + ArrayExpression, ArrayExpressionElement, CallExpression, Expression, ObjectExpression, + ObjectPropertyKind, + }, + AstKind, Visit, +}; +use oxc_diagnostics::OxcDiagnostic; +use oxc_macros::declare_oxc_lint; +use oxc_span::{GetSpan, Span}; + +use crate::{ + ast_util::{is_method_call, leftmost_identifier_reference}, + context::LintContext, + rule::Rule, + utils::default_true, + AstNode, +}; + +fn no_map_spread_diagnostic(map_call: Span, spread: &Spread<'_, '_>) -> OxcDiagnostic { + match spread { + // Obj + Spread::Object(obj) => OxcDiagnostic::warn( + "Spreading to modify object properties in `map` calls is inefficient", + ) + .with_labels([ + map_call.label("This map call spreads an object"), + obj.span().primary_label("That could be mutated in place"), + ]) + .with_help("Consider using `Object.assign` instead"), + // Array + Spread::Array(arr) => { + OxcDiagnostic::warn("Spreading to modify array elements in `map` calls is inefficient") + .with_labels([ + map_call.label("This map call spreads an array"), + arr.span().primary_label("That could be mutated in place"), + ]) + .with_help( + "Consider using `Array.prototype.concat` or `Array.prototype.push` instead", + ) + } + } +} + +#[derive(Debug, Clone, Serialize, Deserialize)] +#[serde(rename_all = "camelCase")] +pub struct NoMapSpreadConfig { + /// Ignore mapped arrays that are re-read after the `map` call. + /// + /// Re-used arrays may rely on shallow copying behavior to avoid mutations. + /// In these cases, `Object.assign` is not really more performant than spreads. + /// + /// Default: `true` + #[serde(default = "default_true")] + ignore_rereads: bool, + // todo: ignore_arrays? +} + +#[derive(Debug, Default, Clone)] +pub struct NoMapSpread(Box); +impl Deref for NoMapSpread { + type Target = NoMapSpreadConfig; + + fn deref(&self) -> &Self::Target { + &self.0 + } +} + +impl From for NoMapSpread { + fn from(config: NoMapSpreadConfig) -> Self { + Self(Box::new(config)) + } +} + +impl Default for NoMapSpreadConfig { + fn default() -> Self { + Self { ignore_rereads: true } + } +} + +declare_oxc_lint!( + /// ### What it does + /// + /// Disallow the use of object or array spreads in `Array.prototype.map` and + /// `Array.prototype.flatMap` to add properties/elements to array items. + /// + /// This rule only seeks to report cases where the spread operator is used + /// to merge objects or arrays, not where it is used to copy them. + /// + /// ### Why is this bad? + /// + /// Spreading is commonly used to add properties to objects in an array or + /// to combine several objects together. Unfortunately, spreads incur a + /// re-allocation for a new object, plus `O(n)` memory copies. + /// + /// ```ts + /// // each object in scores gets shallow-copied. Since `scores` is never + /// // reused, spreading is inefficient. + /// function getDisplayData() { + /// const scores: Array<{ username: string, score: number }> = getScores(); + /// const displayData = scores.map(score => ({ ...score, rank: getRank(score) })); + /// return displayData + /// } + /// ``` + /// + /// Unless you expect objects in the mapped array to be mutated later, it is + /// better to use [`Object.assign`](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/assign). + /// + /// ```ts + /// // `score` is mutated in place and is more performant. + /// function getDisplayData() { + /// const scores: Array<{ username: string, score: number }> = getScores(); + /// const displayData = scores.map(score => Object.assign(score, { rank: getRank(score) })); + /// return displayData + /// } + /// ``` + /// + /// ### Protecting from Mutations + /// There are valid use cases for spreading objects in `map` calls, + /// specifically when you want consumers of returned arrays to be able to + /// mutate them without affecting the original data. This rule makes a + /// best-effort attempt to avoid reporting on these cases. + /// + /// Spreads on class instance properties are completely ignored: + /// ```ts + /// class AuthorsDb { + /// #authors = []; + /// function getAuthorsWithBooks() { + /// return this.#authors.map(author => ({ + /// // protects against mutations, giving the callee their own + /// // deep(ish) copy of the author object. + /// ...author, + /// books: getBooks(author) + /// })); + /// } + /// } + /// ``` + /// + /// Spreads on arrays that are re-read after the `map` call are also ignored + /// by default. Configure this behavior with the `ignoreRereads` option. + /// + /// ``` + /// /* "oxc/no-map-spread": ["error", { "ignoreRereads": true }] */ + /// const scores = getScores(); + /// const displayData = scores.map(score => ({ ...score, rank: getRank(score) })); + /// console.log(scores); // scores is re-read after the map call + /// ``` + /// + /// #### Arrays + /// + /// In the case of array spreads, + /// [`Array.prototype.concat`](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/concat) + /// or + /// [`Array.prototype.push`](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/push) + /// should be used wherever possible. These have slignly different semantics + /// than array spreads, since spreading works on iterables while `concat` + /// and `push` work only on arrays. + /// + /// ```ts + /// let arr = [1, 2, 3]; + /// let set = new Set([4]) + /// + /// let a = [...arr, ...set]; // [1, 2, 3, 4] + /// let b = arr.concat(set); // [1, 2, 3, Set(1)] + /// + /// // Alternative that is more performant than spreading but still has the + /// // same semantics. Unfortunately, it is more verbose. + /// let c = arr.concat(Array.from(set)); // [1, 2, 3, 4] + /// + /// // You could also use `Symbol.isConcatSpreadable` + /// set[Symbol.isConcatSpreadable] = true; + /// let d = arr.concat(set); // [1, 2, 3, 4] + /// ``` + /// + /// ### Examples + /// + /// Examples of **incorrect** code for this rule: + /// ```js + /// const arr = [{ a: 1 }, { a: 2 }, { a: 3 }]; + /// const arr2 = arr.map(obj => ({ ...obj, b: obj.a * 2 })); + /// ``` + /// + /// Examples of **correct** code for this rule: + /// ```ts + /// const arr = [{ a: 1 }, { a: 2 }, { a: 3 }]; + /// arr.map(obj => Object.assign(obj, { b: obj.a * 2 })); + /// + /// // instance properties are ignored + /// class UsersDb { + /// #users = []; + /// public get users() { + /// // clone users, providing caller with their own deep(ish) copy. + /// return this.#users.map(user => ({ ...user })); + /// } + /// } + /// ``` + /// + /// ```tsx + /// function UsersTable({ users }) { + /// const usersWithRoles = users.map(user => ({ ...user, role: getRole(user) })); + /// + /// return ( + /// + /// {usersWithRoles.map(user => ( + /// + /// + /// + /// + /// ))} + /// + /// + /// {/* re-read of users */} + /// + /// + /// + ///
{user.name}{user.role}
Total users: {users.length}
+ /// ) + /// } + /// ``` + /// + /// ### References + /// - [ECMA262 - Object spread evaluation semantics](https://262.ecma-international.org/15.0/index.html#sec-runtime-semantics-propertydefinitionevaluation) + /// - [JSPerf - `concat` vs array spread performance](https://jsperf.app/pihevu) + NoMapSpread, + nursery, // TODO: make this `perf` once we've battle-tested this a bit + pending // TODO: dangerous_fix +); + +impl Rule for NoMapSpread { + fn from_configuration(value: serde_json::Value) -> Self { + let config: NoMapSpreadConfig = value + .get(0) + .map(|obj| { + serde_json::from_value(obj.clone()) + .expect("Invalid configuration for `oxc/no-map-spread`") + }) + .unwrap_or_default(); + + Self::from(config) + } + + fn run<'a>(&self, node: &AstNode<'a>, ctx: &LintContext<'a>) { + // Find `.map()` calls. + // look for both `map` and `flatMap` + let AstKind::CallExpression(call_expr) = node.kind() else { + return; + }; + let Some(mapper) = get_map_callback(call_expr) else { + return; + }; + // let Some((is_expression, body)) = get_map_callback(call_expr) else { + // return; + // }; + + // object and array literals with spread properties + let mut spreads = Vec::new(); + // Look for return statements that contain an object or array spread. + SpreadInReturnVisitor::<'a, '_>::iter_spreads(ctx, mapper, |spread| { + // SAFETY: references to arena-allocated objects are valid for the + // lifetime of the arena. Unfortunately, `AsRef` on `Box<'a, T>` + // returns a reference with a lifetime of 'self instead of 'a. + spreads.push(unsafe { std::mem::transmute::, Spread<'a, 'a>>(spread) }); + }); + if spreads.is_empty() { + return; + } + + match leftmost_identifier_reference(&call_expr.callee) { + Ok(ident) => { + if self.ignore_rereads + && ident + .reference_id() + // reference -> symbol being referenced + .and_then(|id| { + ctx.semantic() + .symbols() + .get_reference(id) + .symbol_id() + .map(|symbol_id| (id, symbol_id)) + }) + .is_some_and(|(ref_id, symbol_id)| { + has_reads_after(ctx, ref_id, symbol_id, call_expr.span) + }) + { + return; + } + + // ctx.diagnostic(no_map_spread_diagnostic(call_expr.span)); + // continue + } + Err(Expression::ThisExpression(_)) => { + return; + } + Err(_) => { + // ctx.diagnostic(no_map_spread_diagnostic(call_expr.span)); + } + } + + let map_call_site = match &call_expr.callee { + Expression::StaticMemberExpression(mem) => mem.property.span, + Expression::ComputedMemberExpression(mem) => mem.expression.span(), + Expression::PrivateFieldExpression(mem) => mem.field.span, + Expression::ChainExpression(chain) => chain.expression.span(), + expr => expr.span(), + }; + + for spread in spreads { + ctx.diagnostic(no_map_spread_diagnostic(map_call_site, &spread)); + } + } +} + +fn has_reads_after( + ctx: &LintContext<'_>, + reference_id: ReferenceId, + symbol_id: SymbolId, + span: Span, +) -> bool { + ctx.semantic() + .symbols() + // skip the reference within the spread itself + .get_resolved_reference_ids(symbol_id) + .iter() + .filter(|id| **id != reference_id) + .map(|id| ctx.semantic().symbols().get_reference(*id)) + // we don't care if the symbol is overwritten + .filter(|r| r.is_read()) + // Find where the symbol was declared + .map(|r| ctx.semantic().nodes().get_node(r.node_id())) // + .any(|node| node.span().end > span.end) +} + +// fn get_map_callback<'a, 'e>( +// call_expr: &'e CallExpression<'a>, +// ) -> Option<(bool, &'e FunctionBody<'a>)> { +// if !is_method_call(call_expr, None, Some(&["map", "flatMap"]), Some(1), Some(1)) { +// return None; +// } +// let arg = call_expr.arguments.first()?.as_expression()?; +// match arg { +// Expression::ArrowFunctionExpression(arrow_fn) => { +// Some((arrow_fn.expression, &arrow_fn.body)) +// } +// Expression::FunctionExpression(func_expr) => { +// func_expr.body.as_ref().map(|body| (false, &**body)) +// } +// _ => None, +// } +// } +fn get_map_callback<'a, 'b>(call_expr: &'b CallExpression<'a>) -> Option<&'b Expression<'a>> { + if !is_method_call(call_expr, None, Some(&["map", "flatMap"]), Some(1), Some(1)) { + return None; + } + + let arg = call_expr.arguments.first()?.as_expression()?; + match arg { + Expression::ArrowFunctionExpression(_) | Expression::FunctionExpression(_) => Some(arg), + _ => None, + } +} + +enum Spread<'a, 'b> { + Object(&'b ObjectExpression<'a>), + Array(&'b ArrayExpression<'a>), +} + +impl GetSpan for Spread<'_, '_> { + fn span(&self) -> Span { + match self { + Spread::Object(obj) => obj.span(), + Spread::Array(arr) => arr.span(), + } + } +} + +struct SpreadInReturnVisitor<'a, 'ctx, F> { + cb: F, + is_in_return: bool, + cb_scope_id: ScopeId, + ctx: &'ctx LintContext<'a>, +} + +impl<'a, 'ctx, F> SpreadInReturnVisitor<'a, 'ctx, F> +where + F: FnMut(Spread<'a, '_>), +{ + fn iter_spreads(ctx: &'ctx LintContext<'a>, map_cb: &Expression<'a>, cb: F) { + let (mut visitor, body) = match map_cb { + Expression::ArrowFunctionExpression(f) => { + let v = Self { + ctx, + is_in_return: f.expression, + cb, + cb_scope_id: f.scope_id.get().unwrap(), + }; + (v, f.body.as_ref()) + } + Expression::FunctionExpression(f) => { + let v = + Self { ctx, is_in_return: false, cb, cb_scope_id: f.scope_id.get().unwrap() }; + let Some(body) = f.body.as_ref().map(AsRef::as_ref) else { + return; + }; + (v, body) + } + _ => unreachable!(), + }; + + visitor.visit_function_body(body); + } + + fn visit_kind(&mut self, kind: AstKind<'a>) { + match kind { + AstKind::VariableDeclaration(d) => self.visit_variable_declaration(d), + AstKind::VariableDeclarator(d) => self.visit_variable_declarator(d), + _ => { /* not needed for the checks we want */ } + } + } +} + +impl<'a, F> Visit<'a> for SpreadInReturnVisitor<'a, '_, F> +where + F: FnMut(Spread<'a, '_>), +{ + #[inline] + fn enter_node(&mut self, kind: AstKind<'a>) { + if let AstKind::ReturnStatement(_) = kind { + self.is_in_return = true; + } + } + + #[inline] + fn leave_node(&mut self, kind: AstKind<'a>) { + if let AstKind::ReturnStatement(_) = kind { + self.is_in_return = false; + } + } + + fn visit_expression(&mut self, expr: &Expression<'a>) { + if !self.is_in_return { + return; + } + match expr.get_inner_expression() { + Expression::ObjectExpression(obj) => self.visit_object_expression(obj), + Expression::ArrayExpression(arr) => self.visit_array_expression(arr), + Expression::Identifier(ident) => { + // let Some(reference_id) = ident.reference_id() else { + // return; + // }; + let Some(symbol_id) = ident + .reference_id() + .map(|id| self.ctx.symbols().get_reference(id)) + .and_then(Reference::symbol_id) + else { + return; + }; + let declaration_scope = self.ctx.symbols().get_scope_id(symbol_id); + // symbol is not declared within the mapper callback + if !self + .ctx + .scopes() + .ancestors(declaration_scope) + .any(|parent_id| parent_id == self.cb_scope_id) + { + return; + } + let declaration_node = + self.ctx.nodes().get_node(self.ctx.symbols().get_declaration(symbol_id)); + self.visit_kind(declaration_node.kind()); + } + _ => {} + } + } + + fn visit_object_expression(&mut self, obj: &ObjectExpression<'a>) { + if self.is_in_return + && obj + .properties + .iter() + .any(|prop| matches!(prop, ObjectPropertyKind::SpreadProperty(_))) + { + (self.cb)(Spread::Object(obj)); + } + } + + fn visit_array_expression(&mut self, arr: &ArrayExpression<'a>) { + if self.is_in_return + && arr + .elements + .iter() + .any(|prop| matches!(prop, ArrayExpressionElement::SpreadElement(_))) + { + (self.cb)(Spread::Array(arr)); + } + } +} + +#[test] +fn test() { + use crate::tester::Tester; + use serde_json::json; + + let pass = vec![ + ("let a = b.map(x => x)", None), + ("let b = []; let a = b.map(x => ({ ...x })); console.log(b)", None), + ("let a = b.map(x => Object.assign(x, { foo: { ...x.foo, bar }}))", None), + ( + "class Foo { + #arr = [] + id = 0 + public get arr() { + return this.#arr.map(x => ({ ...x, id: this.id })) + } + }", + None, + ), + ( + // Spread does not occur within `map` + "let extras = { ...foo, ...bar }; let a = b.map(x => extras)", + None, + ), + ]; + + let fail = vec![ + ("let a = b.map(x => ({ ...x }))", None), + ("let a = b.flatMap(x => ({ ...x }))", None), + ("let a = b.map(x => ({ ...x, ...y }))", None), + ("let b = []; let a = b.map(x => ({ ...x }))", None), + ("let a = b.map(x => { return { ...x } })", None), + ("let a = b.map(x => [ ...x ])", None), + ("let a = b.map(x => [ ...x, ...y ])", None), + ("let a = b.map(x => { return [ ...x ] })", None), + ("let a = b.map(x => { let x2 = { ...x }; return x2; })", None), + ( + "let a = b.map(x => ({ ...x })); console.log(b)", + Some(json!([{ "ignoreRereads": false }])), + ), + ]; + + Tester::new(NoMapSpread::NAME, pass, fail).test_and_snapshot(); +} diff --git a/crates/oxc_linter/src/snapshots/no_map_spread.snap b/crates/oxc_linter/src/snapshots/no_map_spread.snap new file mode 100644 index 00000000000000..6ad15146ee5dfc --- /dev/null +++ b/crates/oxc_linter/src/snapshots/no_map_spread.snap @@ -0,0 +1,92 @@ +--- +source: crates/oxc_linter/src/tester.rs +--- + ⚠ oxc(no-map-spread): Spreading to modify object properties in `map` calls is inefficient + ╭─[no_map_spread.tsx:1:21] + 1 │ let a = b.map(x => ({ ...x })) + · ─┬─ ────┬─── + · │ ╰── That could be mutated in place + · ╰── This map call spreads an object + ╰──── + help: Consider using `Object.assign` instead + + ⚠ oxc(no-map-spread): Spreading to modify object properties in `map` calls is inefficient + ╭─[no_map_spread.tsx:1:25] + 1 │ let a = b.flatMap(x => ({ ...x })) + · ───┬─── ────┬─── + · │ ╰── That could be mutated in place + · ╰── This map call spreads an object + ╰──── + help: Consider using `Object.assign` instead + + ⚠ oxc(no-map-spread): Spreading to modify object properties in `map` calls is inefficient + ╭─[no_map_spread.tsx:1:21] + 1 │ let a = b.map(x => ({ ...x, ...y })) + · ─┬─ ───────┬────── + · │ ╰── That could be mutated in place + · ╰── This map call spreads an object + ╰──── + help: Consider using `Object.assign` instead + + ⚠ oxc(no-map-spread): Spreading to modify object properties in `map` calls is inefficient + ╭─[no_map_spread.tsx:1:33] + 1 │ let b = []; let a = b.map(x => ({ ...x })) + · ─┬─ ────┬─── + · │ ╰── That could be mutated in place + · ╰── This map call spreads an object + ╰──── + help: Consider using `Object.assign` instead + + ⚠ oxc(no-map-spread): Spreading to modify object properties in `map` calls is inefficient + ╭─[no_map_spread.tsx:1:29] + 1 │ let a = b.map(x => { return { ...x } }) + · ─┬─ ────┬─── + · │ ╰── That could be mutated in place + · ╰── This map call spreads an object + ╰──── + help: Consider using `Object.assign` instead + + ⚠ oxc(no-map-spread): Spreading to modify array elements in `map` calls is inefficient + ╭─[no_map_spread.tsx:1:20] + 1 │ let a = b.map(x => [ ...x ]) + · ─┬─ ────┬─── + · │ ╰── That could be mutated in place + · ╰── This map call spreads an array + ╰──── + help: Consider using `Array.prototype.concat` or `Array.prototype.push` instead + + ⚠ oxc(no-map-spread): Spreading to modify array elements in `map` calls is inefficient + ╭─[no_map_spread.tsx:1:20] + 1 │ let a = b.map(x => [ ...x, ...y ]) + · ─┬─ ───────┬────── + · │ ╰── That could be mutated in place + · ╰── This map call spreads an array + ╰──── + help: Consider using `Array.prototype.concat` or `Array.prototype.push` instead + + ⚠ oxc(no-map-spread): Spreading to modify array elements in `map` calls is inefficient + ╭─[no_map_spread.tsx:1:29] + 1 │ let a = b.map(x => { return [ ...x ] }) + · ─┬─ ────┬─── + · │ ╰── That could be mutated in place + · ╰── This map call spreads an array + ╰──── + help: Consider using `Array.prototype.concat` or `Array.prototype.push` instead + + ⚠ oxc(no-map-spread): Spreading to modify object properties in `map` calls is inefficient + ╭─[no_map_spread.tsx:1:31] + 1 │ let a = b.map(x => { let x2 = { ...x }; return x2; }) + · ─┬─ ────┬─── + · │ ╰── That could be mutated in place + · ╰── This map call spreads an object + ╰──── + help: Consider using `Object.assign` instead + + ⚠ oxc(no-map-spread): Spreading to modify object properties in `map` calls is inefficient + ╭─[no_map_spread.tsx:1:21] + 1 │ let a = b.map(x => ({ ...x })); console.log(b) + · ─┬─ ────┬─── + · │ ╰── That could be mutated in place + · ╰── This map call spreads an object + ╰──── + help: Consider using `Object.assign` instead