diff --git a/crates/oxc_ast/src/ast_impl/js.rs b/crates/oxc_ast/src/ast_impl/js.rs index 759cb224a631b..c113adaa2a236 100644 --- a/crates/oxc_ast/src/ast_impl/js.rs +++ b/crates/oxc_ast/src/ast_impl/js.rs @@ -316,6 +316,14 @@ impl<'a> ArrayExpressionElement<'a> { } } +impl<'a> ObjectPropertyKind<'a> { + /// Returns `true` if this object property is a [spread](SpreadElement). + #[inline] + pub fn is_spread(&self) -> bool { + matches!(self, Self::SpreadProperty(_)) + } +} + impl<'a> PropertyKey<'a> { #[allow(missing_docs)] pub fn static_name(&self) -> Option> { diff --git a/crates/oxc_linter/src/ast_util.rs b/crates/oxc_linter/src/ast_util.rs index 66321cb364531..86355df6ae96c 100644 --- a/crates/oxc_linter/src/ast_util.rs +++ b/crates/oxc_linter/src/ast_util.rs @@ -253,6 +253,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, 's>( @@ -424,3 +425,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 721fd6f6b38e0..5690e29651978 100644 --- a/crates/oxc_linter/src/rules.rs +++ b/crates/oxc_linter/src/rules.rs @@ -413,6 +413,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; @@ -765,6 +766,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 0000000000000..63c77c01bceb0 --- /dev/null +++ b/crates/oxc_linter/src/rules/oxc/no_map_spread.rs @@ -0,0 +1,863 @@ +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::{LabeledSpan, OxcDiagnostic}; +use oxc_macros::declare_oxc_lint; +use oxc_span::{GetSpan, Span}; + +use crate::{ + ast_util::{is_method_call, leftmost_identifier_reference}, + context::LintContext, + fixer::{RuleFix, RuleFixer}, + rule::Rule, + utils::default_true, + AstNode, +}; + +fn no_map_spread_diagnostic( + map_call: Span, + spread: &Spread<'_, '_>, + returned_span: Option, +) -> OxcDiagnostic { + let spans = spread.spread_spans(); + assert!(!spans.is_empty()); + let mut spread_labels = spread.spread_spans().into_iter(); + let first_message = if spans.len() == 1 { + "It should be mutated in place" + } else { + "They should be mutated in place" + }; + let first = spread_labels.next().unwrap().label(first_message); + let others = spread_labels.map(LabeledSpan::from); + + let returned_label = returned_span + .filter(|span| !span.contains_inclusive(spread.span())) + .map(|span| span.label("Map returns the spread here")); + + let diagnostic = + match spread { + // Obj + Spread::Object(_) => OxcDiagnostic::warn( + "Spreading to modify object properties in `map` calls is inefficient", + ) + .with_labels([map_call.label("This map call spreads an object"), first]) + .with_help("Consider using `Object.assign` instead"), + // Array + Spread::Array(_) => OxcDiagnostic::warn( + "Spreading to modify array elements in `map` calls is inefficient", + ) + .with_labels([map_call.label("This map call spreads an array"), first]) + .with_help("Consider using `Array.prototype.concat` or `Array.prototype.push` instead"), + }; + + diagnostic.and_labels(others).and_labels(returned_label) +} + +#[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, + /// Ignore maps on arrays passed as parameters to a function. + /// + /// This option is enabled by default to better avoid false positives. It + /// comes at the cost of potentially missing spreads that are inefficient. + /// We recommend turning this off in your `.oxlintrc.json` files. + /// + /// ### Example + /// + /// Examples of **incorrect** code for this rule when `ignoreArgs` is `true`: + /// ```ts + /// /* "oxc/no-map-spread": ["error", { "ignoreArgs": true }] */ + /// function foo(arr) { + /// let arr2 = arr.filter(x => x.a > 0); + /// return arr2.map(x => ({ ...x })); + /// } + /// ``` + /// + /// Examples of **correct** code for this rule when `ignoreArgs` is `true`: + /// ```ts + /// /* "oxc/no-map-spread": ["error", { "ignoreArgs": true }] */ + /// function foo(arr) { + /// return arr.map(x => ({ ...x })); + /// } + /// ``` + /// + /// Default: `true` + #[serde(default = "default_true")] + ignore_args: bool, + // todo: ignore_arrays? +} + +// NOTE: not boxing the config for now because of how small it is. If we add +// more than 16 bytes of options, we need to add a box back. +#[derive(Debug, Default, Clone)] +pub struct NoMapSpread(NoMapSpreadConfig); +impl Deref for NoMapSpread { + type Target = NoMapSpreadConfig; + + fn deref(&self) -> &Self::Target { + &self.0 + } +} + +impl From for NoMapSpread { + fn from(config: NoMapSpreadConfig) -> Self { + Self(config) + } +} + +impl Default for NoMapSpreadConfig { + fn default() -> Self { + Self { ignore_rereads: true, ignore_args: true } + } +} + +declare_oxc_lint!( + /// ### What it does + /// + /// Disallow the use of object or array spreads in + /// [`Array.prototype.map`](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/map) + /// and + /// [`Array.prototype.flatMap`](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/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 = []; + /// public 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] + /// ``` + /// + /// ### Automatic Fixing + /// This rule can automatically fix violations caused by object spreads, but + /// does not fix arrays. Object spreads will get replaced with + /// [`Object.assign`](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/assign). Array fixing may be added in the future. + /// + /// Object expressions with a single element (the spread) are not fixed. + /// ```js + /// arr.map(x => ({ ...x })) // not fixed + /// ``` + /// + /// A `fix` is available (using `--fix`) for objects with "normal" elements before the + /// spread. Since `Object.apply` mutates the first argument, and a new + /// object will be created with those elements, the spread identifier will + /// not be mutated. In effect, the spread semantics are preserved + /// ```js + /// // before + /// arr.map(({ x, y }) => ({ x, ...y })) + /// + /// // after + /// arr.map(({ x, y }) => (Object.assign({ x }, y))) + /// ``` + /// + /// A suggestion (using `--fix-suggestions`) is provided when a spread is + /// the first property in an object. This fix mutates the spread identifier, + /// meaning it could have unintended side effects. + /// ```js + /// // before + /// arr.map(({ x, y }) => ({ ...x, y })) + /// arr.map(({ x, y }) => ({ ...x, y })) + /// + /// // after + /// arr.map(({ x, y }) => (Object.assign(x, { y }))) + /// arr.map(({ x, y }) => (Object.assign(x, y))) + /// ``` + /// + /// ### 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 + conditional_fix_suggestion +); + +const MAP_FN_NAMES: [&str; 2] = ["map", "flatMap"]; + +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; + }; + + // object and array literals with spread properties + let mut spreads = Vec::new(); + // Look for return statements that contain an object or array spread. + let visitor = 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) }); + }); + let returned_span = visitor.and_then(|v| v.return_span); + if spreads.is_empty() { + return; + } + + match leftmost_identifier_reference(&call_expr.callee) { + Ok(ident) => { + if let Some(ref_id) = ident.reference_id() { + if self.is_ignored_map_call(ctx, ident.name.as_str(), ref_id, call_expr.span) { + return; + } + } + } + // Mapped class properties likely have their elements spread to + // avoid side effects on the class instance. + Err(Expression::ThisExpression(_)) => { + return; + } + Err(_) => {} + } + + 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 { + let diagnostic = no_map_spread_diagnostic(map_call_site, &spread, returned_span); + if let Some(obj) = spread.as_object() { + debug_assert!(!obj.properties.is_empty()); + if obj.properties.first().is_some_and(ObjectPropertyKind::is_spread) { + ctx.diagnostic_with_suggestion(diagnostic, |fixer| { + fix_spread_to_object_assign(fixer, obj) + }); + } else { + ctx.diagnostic_with_fix(diagnostic, |fixer| { + fix_spread_to_object_assign(fixer, obj) + }); + } + } else { + ctx.diagnostic(diagnostic); + } + } + } +} + +impl NoMapSpread { + /// Spreads returned from `map` (et. al.) are not violations if + /// 1. `map` is a custom user-defined function + /// 2. the array being mapped is read after mapping and `ignore_rereads` is + /// enabled. + fn is_ignored_map_call( + &self, + ctx: &LintContext<'_>, + name: &str, + reference_id: ReferenceId, + call_site: Span, + ) -> bool { + let Some(symbol_id) = ctx.symbols().get_reference(reference_id).symbol_id() else { + return false; + }; + // Call is to a self-defined `map` call. + if MAP_FN_NAMES.contains(&name) { + // TODO: use symbolflags to check if function. Note that + // SymbolFlags::ArrowFunction was removed. + // TODO: ignore `const map = Array.prototype.map` + return true; + } + + if self.ignore_args { + let declaration = ctx.nodes().get_node(ctx.symbols().get_declaration(symbol_id)); + if matches!(declaration.kind(), AstKind::FormalParameter(_)) { + return true; + } + } + + self.ignore_rereads && has_reads_after(ctx, reference_id, symbol_id, call_site) + } +} + +/// Check if a symbol has any read references that occur after `span`. `span` is +/// the location of the reference identified by `reference_id`. +fn has_reads_after( + ctx: &LintContext<'_>, + reference_id: ReferenceId, + symbol_id: SymbolId, + span: Span, +) -> bool { + let symbols = ctx.symbols(); + let nodes = ctx.nodes(); + symbols + // skip the reference within the spread itself + .get_resolved_reference_ids(symbol_id) + .iter() + .filter(|id| **id != reference_id) + .map(|id| 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| nodes.get_node(r.node_id())) // + .any(|node| node.span().end > span.end) +} + +fn get_map_callback<'a, 'b>(call_expr: &'b CallExpression<'a>) -> Option<&'b Expression<'a>> { + if !is_method_call(call_expr, None, Some(&MAP_FN_NAMES), Some(1), Some(1)) { + return None; + } + + let arg = call_expr.arguments.first()?.as_expression()?.get_inner_expression(); + match arg { + Expression::ArrowFunctionExpression(_) | Expression::FunctionExpression(_) => Some(arg), + _ => None, + } +} + +fn fix_spread_to_object_assign<'a>( + fixer: RuleFixer<'_, 'a>, + obj: &ObjectExpression<'a>, +) -> RuleFix<'a> { + use oxc_allocator::{Allocator, CloneIn}; + use oxc_ast::AstBuilder; + use oxc_codegen::CodegenOptions; + use oxc_span::SPAN; + + if obj.properties.len() <= 1 { + return fixer.noop(); + } + + let alloc = Allocator::default(); + let ast = AstBuilder::new(&alloc); + + // almost always overshoots, but will not re-alloc, so it's more performant + // than creating an empty vec. + // let mut args = ast.vec_with_capacity::(obj.properties.len()); + let mut curr_obj_properties = ast.vec::(); + let mut codegen = + fixer.codegen().with_options(CodegenOptions { minify: true, ..Default::default() }); + let mut is_first = true; + codegen.print_str("Object.assign("); + + for prop in &obj.properties { + match prop { + ObjectPropertyKind::ObjectProperty(_) => { + curr_obj_properties.push(prop.clone_in(&alloc)); + } + ObjectPropertyKind::SpreadProperty(spread) => { + if !curr_obj_properties.is_empty() { + let properties = std::mem::replace(&mut curr_obj_properties, ast.vec()); + let obj_arg = ast.expression_object(SPAN, properties, None); + if is_first { + is_first = false; + } else { + codegen.print_str(", "); + } + codegen.print_expression(&obj_arg); + } + if is_first { + is_first = false; + } else { + codegen.print_str(", "); + } + codegen.print_expression(&spread.argument); + } + } + } + + if !curr_obj_properties.is_empty() { + if !is_first { + codegen.print_str(", "); + } + let properties = std::mem::replace(&mut curr_obj_properties, ast.vec()); + let obj_arg = ast.expression_object(SPAN, properties, None); + codegen.print_expression(&obj_arg); + } + codegen.print_ascii_byte(b')'); + + // TODO: expand replaced span to outer paren parent to replace wrapped + // parenthesis in implicit arrow returns. Blocked by AST node IDs not yet in + // each node. + fixer.replace(obj.span, codegen.into_source_text()) +} + +enum Spread<'a, 'b> { + Object(&'b ObjectExpression<'a>), + Array(&'b ArrayExpression<'a>), +} + +impl<'a, 'b> Spread<'a, 'b> { + #[inline] + fn as_object(&self) -> Option<&'b ObjectExpression<'a>> { + match self { + Spread::Object(obj) => Some(obj), + Spread::Array(_) => None, + } + } + + fn spread_spans(&self) -> Vec { + match self { + Spread::Object(obj) => obj + .properties + .iter() + .filter_map(|prop| match prop { + ObjectPropertyKind::SpreadProperty(spread) => Some(spread.span()), + ObjectPropertyKind::ObjectProperty(_) => None, + }) + .collect(), + Spread::Array(arr) => arr + .elements + .iter() + .filter_map(|elem| match elem { + ArrayExpressionElement::SpreadElement(spread) => Some(spread.span()), + _ => None, + }) + .collect(), + } + } +} + +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> { + ctx: &'ctx LintContext<'a>, + cb: F, + cb_scope_id: ScopeId, + is_in_return: bool, + /// Span covering returned expression. [`None`] when not in a return + /// statement or no value is being returned (e.g. `return;`, but not `return + /// undefined;`). + return_span: Option, +} + +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) -> Option { + let (mut visitor, body) = match map_cb { + Expression::ArrowFunctionExpression(f) => { + let v = Self { + ctx, + cb, + cb_scope_id: f.scope_id.get().unwrap(), + is_in_return: f.expression, + return_span: f.expression.then(|| f.body.span()), + }; + (v, f.body.as_ref()) + } + Expression::FunctionExpression(f) => { + let v = Self { + ctx, + cb, + cb_scope_id: f.scope_id.get().unwrap(), + is_in_return: false, + return_span: None, + }; + let body = f.body.as_ref().map(AsRef::as_ref)?; + (v, body) + } + _ => unreachable!(), + }; + + visitor.visit_function_body(body); + Some(visitor) + } + + 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(stmt) = kind { + self.is_in_return = true; + self.return_span = stmt.argument.as_ref().map(GetSpan::span); + } + } + + #[inline] + fn leave_node(&mut self, kind: AstKind<'a>) { + if let AstKind::ReturnStatement(_) = kind { + self.is_in_return = false; + // NOTE: do not clear `return_span` here. We want to keep the last + // encountered `return` for reporting. + } + } + + fn visit_expression(&mut self, expr: &Expression<'a>) { + if !self.is_in_return { + return; + } + match expr.get_inner_expression() { + // base cases + Expression::ObjectExpression(obj) => self.visit_object_expression(obj), + Expression::ArrayExpression(arr) => self.visit_array_expression(arr), + // recursive cases + Expression::ConditionalExpression(cond) => { + self.visit_expression(&cond.consequent); + self.visit_expression(&cond.alternate); + } + Expression::SequenceExpression(expr) => { + if let Some(last) = expr.expressions.last() { + self.visit_expression(last); + } + } + Expression::LogicalExpression(expr) => self.visit_logical_expression(expr), + + // check if identifier is a reference to a spread-initialized + // variable declared within the map callback. + Expression::Identifier(ident) => { + 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; + } + + // walk the declaration + 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, + ), + // class members are ignored + ("this.foo.map(x => ({ ...x }))", None), + ("this.#foo.map(x => ({ ...x }))", None), + // indirection + ( + // Spread does not occur within `map` + "let extras = { ...foo, ...bar }; let a = b.map(x => extras)", + None, + ), + // user-defined map functions are ignored + ("function map(f) {}; map(x => ({ ...x }))", None), + ("function flatMap(f) {}; flatMap(x => ({ ...x }))", None), + // ignoreArgs + ("function foo(a) { return a.map(x => ({ ...(x ?? y) })) }", None), + ("const foo = a => a.map(x => ({ ...(x ?? y) }))", None), + ]; + + let fail = vec![ + // basic objects + ("let a = b.map(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), + // basic arrays + ("let a = b.map(x => [ ...x ])", None), + ("let a = b.map(x => [ ...x, ...y ])", None), + ("let a = b.map(x => { return [ ...x ] })", None), + // map (et. al.) variations + ("a.map?.(x => ({ ...x }))", None), + ("let a = b.flatMap(x => ({ ...x }))", None), + // rereads + ( + "let a = b.map(x => ({ ...x })); console.log(b)", + Some(json!([{ "ignoreRereads": false }])), + ), + ("let b = []; console.log(b); let a = b.map(x => ({ ...x }));", None), + // indirection + ("let a = b.map(x => { let x2 = { ...x }; return x2; })", None), + ("let a = b.map(x => { let x2 = { ...x }; let x3 = x2; return x3; })", None), + ( + "let a = b.map(x => { + let y = { ...x }; + if (y.foo) { + return y; + } else { + return x; + } + })", + None, + ), + // conditionals and other "outer" expressions + ("let a = b.map(x => someCond ? { ...x, foo: true } : { ...x, foo: false })", None), + ("let a = b.map( ({ x, y }) => ({ ...(cond ? x : y) }) )", None), + ("const b = a.map((x, i) => y ? { ...x, i } : x)", None), + ("const b = a.map((x, i) => y ? x : { ...x, i })", None), + ("let a = b.map(x => (0, { ...x }))", None), + ("let a = b.map(({ x, y }) => (x ?? { ...y }))", None), + ("let a = b.map((x => ({ ...x }))) as MyCustomMapper", None), + // make sure reported spans are nice + // map call variations + ("foo().map(x => ({ ...x }))", None), + ("foo[1].map(x => ({ ...x }))", None), + ("foo?.bar?.map(x => ({ ...x }))", None), + ("(foo ?? bar).map(x => ({ ...x }))", None), + ("obj.#foo.map(x => ({ ...x }))", None), + // spread variations + ("a.map(x => ({ ...x.y }))", None), + ("a.map(x => ({ ...x[y] }))", None), + ("a.map(x => ({ ...(x ?? y) }))", None), + // ignoreArgs + ( + "function foo(a) { return a.map(x => ({ ...(x ?? y) })) }", + Some(json!([{ "ignoreArgs": false }])), + ), + ("const foo = a => a.map(x => ({ ...(x ?? y) }))", Some(json!([{ "ignoreArgs": false }]))), + ]; + + let fix = vec![ + // single spreads cannot be fixed with `Object.assign`. We'll assume the + // spread is happening intentionally, to force a shallow clone. Maybe we + // shouldn't even report these cases? + ("let a = b.map(x => ({ ...x }))", "let a = b.map(x => ({ ...x }))"), + // two + ( + "let a = b.map(({ x, y }) => ({ ...x, ...y }))", + "let a = b.map(({ x, y }) => (Object.assign(x, y)))", + ), + ( + "let a = b.map(({ x, y }) => { return { ...x, ...y }; })", + "let a = b.map(({ x, y }) => { return Object.assign(x, y); })", + ), + ( + "let a = b.map(({ x, y }) => ({ x, ...y }))", + "let a = b.map(({ x, y }) => (Object.assign({x}, y)))", + ), + ( + "let a = b.map(({ x, y }) => ({ ...x, y }))", + "let a = b.map(({ x, y }) => (Object.assign(x, {y})))", + ), + // three + ( + "let a = b.map(({ x, y, z }) => ({ ...x, y, z }))", + "let a = b.map(({ x, y, z }) => (Object.assign(x, {y,z})))", + ), + ( + "let a = b.map(({ x, y, z }) => ({ x, ...y, z }))", + "let a = b.map(({ x, y, z }) => (Object.assign({x}, y, {z})))", + ), + ( + "let a = b.map(({ x, y, z }) => ({ x, y, ...z }))", + "let a = b.map(({ x, y, z }) => (Object.assign({x,y}, z)))", + ), + ]; + + Tester::new(NoMapSpread::NAME, pass, fail).expect_fix(fix).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 0000000000000..3097c715d2602 --- /dev/null +++ b/crates/oxc_linter/src/snapshots/no_map_spread.snap @@ -0,0 +1,302 @@ +--- +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:11] + 1 │ let a = b.map(x => ({ ...x })) + · ─┬─ ──┬─ + · │ ╰── It should 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:11] + 1 │ let a = b.map(x => ({ ...x, ...y })) + · ─┬─ ──┬─ ──── + · │ ╰── They should 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:23] + 1 │ let b = []; let a = b.map(x => ({ ...x })) + · ─┬─ ──┬─ + · │ ╰── It should 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:11] + 1 │ let a = b.map(x => { return { ...x } }) + · ─┬─ ──┬─ + · │ ╰── It should 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:11] + 1 │ let a = b.map(x => [ ...x ]) + · ─┬─ ──┬─ + · │ ╰── It should 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:11] + 1 │ let a = b.map(x => [ ...x, ...y ]) + · ─┬─ ──┬─ ──── + · │ ╰── They should 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:11] + 1 │ let a = b.map(x => { return [ ...x ] }) + · ─┬─ ──┬─ + · │ ╰── It should 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:3] + 1 │ a.map?.(x => ({ ...x })) + · ─┬─ ──┬─ + · │ ╰── It should 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:11] + 1 │ let a = b.flatMap(x => ({ ...x })) + · ───┬─── ──┬─ + · │ ╰── It should 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:11] + 1 │ let a = b.map(x => ({ ...x })); console.log(b) + · ─┬─ ──┬─ + · │ ╰── It should 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:39] + 1 │ let b = []; console.log(b); let a = b.map(x => ({ ...x })); + · ─┬─ ──┬─ + · │ ╰── It should 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:11] + 1 │ let a = b.map(x => { let x2 = { ...x }; return x2; }) + · ─┬─ ──┬─ ─┬ + · │ │ ╰── Map returns the spread here + · │ ╰── It should 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:11] + 1 │ let a = b.map(x => { let x2 = { ...x }; let x3 = x2; return x3; }) + · ─┬─ ──┬─ ─┬ + · │ │ ╰── Map returns the spread here + · │ ╰── It should 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:11] + 1 │ let a = b.map(x => { + · ─┬─ + · ╰── This map call spreads an object + 2 │ let y = { ...x }; + · ──┬─ + · ╰── It should be mutated in place + 3 │ if (y.foo) { + ╰──── + ╭─[no_map_spread.tsx:6:21] + 5 │ } else { + 6 │ return x; + · ┬ + · ╰── Map returns the spread here + 7 │ } + ╰──── + 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:11] + 1 │ let a = b.map(x => someCond ? { ...x, foo: true } : { ...x, foo: false }) + · ─┬─ ──┬─ + · │ ╰── It should 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:11] + 1 │ let a = b.map(x => someCond ? { ...x, foo: true } : { ...x, foo: false }) + · ─┬─ ──┬─ + · │ ╰── It should 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:11] + 1 │ let a = b.map( ({ x, y }) => ({ ...(cond ? x : y) }) ) + · ─┬─ ────────┬──────── + · │ ╰── It should 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:13] + 1 │ const b = a.map((x, i) => y ? { ...x, i } : x) + · ─┬─ ──┬─ + · │ ╰── It should 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:13] + 1 │ const b = a.map((x, i) => y ? x : { ...x, i }) + · ─┬─ ──┬─ + · │ ╰── It should 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:11] + 1 │ let a = b.map(x => (0, { ...x })) + · ─┬─ ──┬─ + · │ ╰── It should 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:11] + 1 │ let a = b.map(({ x, y }) => (x ?? { ...y })) + · ─┬─ ──┬─ + · │ ╰── It should 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:11] + 1 │ let a = b.map((x => ({ ...x }))) as MyCustomMapper + · ─┬─ ──┬─ + · │ ╰── It should 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:7] + 1 │ foo().map(x => ({ ...x })) + · ─┬─ ──┬─ + · │ ╰── It should 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:8] + 1 │ foo[1].map(x => ({ ...x })) + · ─┬─ ──┬─ + · │ ╰── It should 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:11] + 1 │ foo?.bar?.map(x => ({ ...x })) + · ─┬─ ──┬─ + · │ ╰── It should 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:14] + 1 │ (foo ?? bar).map(x => ({ ...x })) + · ─┬─ ──┬─ + · │ ╰── It should 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:10] + 1 │ obj.#foo.map(x => ({ ...x })) + · ─┬─ ──┬─ + · │ ╰── It should 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:3] + 1 │ a.map(x => ({ ...x.y })) + · ─┬─ ───┬── + · │ ╰── It should 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:3] + 1 │ a.map(x => ({ ...x[y] })) + · ─┬─ ───┬─── + · │ ╰── It should 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:3] + 1 │ a.map(x => ({ ...(x ?? y) })) + · ─┬─ ─────┬───── + · │ ╰── It should 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:28] + 1 │ function foo(a) { return a.map(x => ({ ...(x ?? y) })) } + · ─┬─ ─────┬───── + · │ ╰── It should 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:20] + 1 │ const foo = a => a.map(x => ({ ...(x ?? y) })) + · ─┬─ ─────┬───── + · │ ╰── It should be mutated in place + · ╰── This map call spreads an object + ╰──── + help: Consider using `Object.assign` instead