From b3c31251386fbb25c94dcb28c189b36ab5bb77d2 Mon Sep 17 00:00:00 2001 From: DonIsaac <22823424+DonIsaac@users.noreply.github.com> Date: Sat, 10 Aug 2024 04:50:09 +0000 Subject: [PATCH] feat(linter): overhaul unicorn/no-useless-spread (#4791) I got tired of seeing useless spreads on ternaries and `arr.reduce()` within my company's internal codebase so I overhauled this rule. ## Changes - add fixer for object spreads ```js const before = { a, ...{ b, c }, d } const after = { a, b, c, d } // fixer does not dedupe spaces before `b` ``` - recursively check for useless clones on complex expressions. This rule now catches and auto-fixes the following cases: ```js // ternaries when both branches create a new array or object const obj = { ...(foo ? { a: 1 } : { b: 2 }) } // recursive, so this can support complex cases const arr = [ ...(foo ? a.map(fn) : bar ? Array.from(iter) : await Promise.all(bar)) ] // reduce functions where the initial accumulator creates a new object or array const obj = { ...(arr.reduce(fn, {}) } ``` --- crates/oxc_linter/src/ast_util.rs | 18 +- .../unicorn/no_useless_spread/const_eval.rs | 232 ++++++++++ .../mod.rs} | 410 ++++++++++-------- .../src/snapshots/no_useless_spread.snap | 113 +++++ crates/oxc_span/src/span/mod.rs | 73 ++++ 5 files changed, 659 insertions(+), 187 deletions(-) create mode 100644 crates/oxc_linter/src/rules/unicorn/no_useless_spread/const_eval.rs rename crates/oxc_linter/src/rules/unicorn/{no_useless_spread.rs => no_useless_spread/mod.rs} (65%) diff --git a/crates/oxc_linter/src/ast_util.rs b/crates/oxc_linter/src/ast_util.rs index 7f2f4c40108d4..d2616d8294f1b 100644 --- a/crates/oxc_linter/src/ast_util.rs +++ b/crates/oxc_linter/src/ast_util.rs @@ -233,20 +233,10 @@ pub fn outermost_paren_parent<'a, 'b>( node: &'b AstNode<'a>, ctx: &'b LintContext<'a>, ) -> Option<&'b AstNode<'a>> { - let mut node = node; - - loop { - if let Some(parent) = ctx.nodes().parent_node(node.id()) { - if let AstKind::ParenthesizedExpression(_) = parent.kind() { - node = parent; - continue; - } - } - - break; - } - - ctx.nodes().parent_node(node.id()) + ctx.nodes() + .iter_parents(node.id()) + .skip(1) + .find(|parent| !matches!(parent.kind(), AstKind::ParenthesizedExpression(_))) } pub fn get_declaration_of_variable<'a, 'b>( diff --git a/crates/oxc_linter/src/rules/unicorn/no_useless_spread/const_eval.rs b/crates/oxc_linter/src/rules/unicorn/no_useless_spread/const_eval.rs new file mode 100644 index 0000000000000..f3572859bbb17 --- /dev/null +++ b/crates/oxc_linter/src/rules/unicorn/no_useless_spread/const_eval.rs @@ -0,0 +1,232 @@ +use oxc_ast::ast::{ + match_expression, Argument, CallExpression, ConditionalExpression, Expression, NewExpression, +}; + +use crate::ast_util::{is_method_call, is_new_expression}; + +#[derive(Debug, Clone)] +pub(super) enum ValueHint { + NewObject, + NewArray, + Promise(Box), + Unknown, +} + +impl ValueHint { + pub fn r#await(self) -> Self { + match self { + Self::Promise(inner) => *inner, + _ => self, + } + } + + #[inline] + pub fn is_object(&self) -> bool { + matches!(self, Self::NewObject) + } + + #[inline] + pub fn is_array(&self) -> bool { + matches!(self, Self::NewArray) + } +} + +impl std::ops::BitAnd for ValueHint { + type Output = Self; + fn bitand(self, rhs: Self) -> Self::Output { + match (self, rhs) { + (Self::NewArray, Self::NewArray) => Self::NewArray, + (Self::NewObject, Self::NewObject) => Self::NewObject, + _ => Self::Unknown, + } + } +} +pub(super) trait ConstEval { + fn const_eval(&self) -> ValueHint; +} + +impl<'a> ConstEval for Expression<'a> { + fn const_eval(&self) -> ValueHint { + match self.get_inner_expression() { + Self::ArrayExpression(_) => ValueHint::NewArray, + Self::ObjectExpression(_) => ValueHint::NewObject, + Self::AwaitExpression(expr) => expr.argument.const_eval().r#await(), + Self::SequenceExpression(expr) => { + expr.expressions.last().map_or(ValueHint::Unknown, ConstEval::const_eval) + } + Self::ConditionalExpression(cond) => cond.const_eval(), + Self::CallExpression(call) => call.const_eval(), + Self::NewExpression(new) => new.const_eval(), + _ => ValueHint::Unknown, + } + } +} + +impl<'a> ConstEval for ConditionalExpression<'a> { + fn const_eval(&self) -> ValueHint { + self.consequent.const_eval() & self.alternate.const_eval() + } +} + +impl<'a> ConstEval for Argument<'a> { + fn const_eval(&self) -> ValueHint { + match self { + // using a spread as an initial accumulator value creates a new + // object or array + Self::SpreadElement(spread) => spread.argument.const_eval(), + expr @ match_expression!(Argument) => expr.as_expression().unwrap().const_eval(), + } + } +} + +impl<'a> ConstEval for NewExpression<'a> { + fn const_eval(&self) -> ValueHint { + if is_new_array(self) || is_new_map_or_set(self) || is_new_typed_array(self) { + ValueHint::NewArray + } else if is_new_object(self) { + ValueHint::NewObject + } else { + ValueHint::Unknown + } + } +} + +fn is_new_array(new_expr: &NewExpression) -> bool { + is_new_expression(new_expr, &["Array"], None, None) +} + +/// Matches `new {Set,WeakSet,Map,WeakMap}(iterable?)` +fn is_new_map_or_set(new_expr: &NewExpression) -> bool { + is_new_expression(new_expr, &["Map", "WeakMap", "Set", "WeakSet"], None, Some(1)) +} + +/// Matches `new Object()` with any number of args. +fn is_new_object(new_expr: &NewExpression) -> bool { + is_new_expression(new_expr, &["Object"], None, None) +} + +/// Matches `new (a, [other args])` with >= 1 arg +pub fn is_new_typed_array(new_expr: &NewExpression) -> bool { + is_new_expression( + new_expr, + &[ + "Int8Array", + "Uint8Array", + "Uint8ClampedArray", + "Int16Array", + "Uint16Array", + "Int32Array", + "Uint32Array", + "Float32Array", + "Float64Array", + "BigInt64Array", + "BigUint64Array", + ], + Some(1), + None, + ) +} + +impl<'a> ConstEval for CallExpression<'a> { + fn const_eval(&self) -> ValueHint { + if is_array_from(self) + || is_split_method(self) + || is_array_factory(self) + || is_functional_array_method(self) + || is_array_producing_obj_method(self) + { + ValueHint::NewArray + } else if is_array_reduce(self) { + self.arguments[1].const_eval() + } else if is_promise_array_method(self) { + ValueHint::Promise(Box::new(ValueHint::NewArray)) + } else if is_obj_factory(self) { + ValueHint::NewObject + } else { + // TODO: check initial value for arr.reduce() accumulators + ValueHint::Unknown + } + } +} + +/// - `Array.from(x)` +/// - `Int8Array.from(x)` +/// - plus all other typed arrays +pub fn is_array_from(call_expr: &CallExpression) -> bool { + is_method_call( + call_expr, + Some(&[ + "Array", + "Int8Array", + "Uint8Array", + "Uint8ClampedArray", + "Int16Array", + "Uint16Array", + "Int32Array", + "Uint32Array", + "Float32Array", + "Float64Array", + "BigInt64Array", + "BigUint64Array", + ]), + Some(&["from"]), + Some(1), + Some(1), + ) +} +/// `.{concat,map,filter,...}` +fn is_functional_array_method(call_expr: &CallExpression) -> bool { + is_method_call( + call_expr, + None, + Some(&[ + "concat", + "copyWithin", + "filter", + "flat", + "flatMap", + "map", + "slice", + "splice", + "toReversed", + "toSorted", + "toSpliced", + "with", + ]), + None, + None, + ) +} + +/// Matches `.reduce(a, b)`, which usually looks like +/// ```ts +/// arr.reduce(reducerRn, initialAccumulator) +/// ``` +fn is_array_reduce(call_expr: &CallExpression) -> bool { + is_method_call(call_expr, None, Some(&["reduce"]), Some(2), Some(2)) +} + +/// Matches `.split(...)`, which usually is `String.prototype.split(pattern)` +fn is_split_method(call_expr: &CallExpression) -> bool { + is_method_call(call_expr, None, Some(&["split"]), None, None) +} + +/// Matches `Object.{fromEntries,create}(x)` +fn is_obj_factory(call_expr: &CallExpression) -> bool { + is_method_call(call_expr, Some(&["Object"]), Some(&["fromEntries", "create"]), Some(1), Some(1)) +} + +/// Matches `Object.{keys,values,entries}(...)` +fn is_array_producing_obj_method(call_expr: &CallExpression) -> bool { + is_method_call(call_expr, Some(&["Object"]), Some(&["keys", "values", "entries"]), None, None) +} + +/// Matches `Array.{from,of}(...)` +fn is_array_factory(call_expr: &CallExpression) -> bool { + is_method_call(call_expr, Some(&["Array"]), Some(&["from", "of"]), None, None) +} + +/// Matches `Promise.{all,allSettled}(x)` +fn is_promise_array_method(call_expr: &CallExpression) -> bool { + is_method_call(call_expr, Some(&["Promise"]), Some(&["all", "allSettled"]), Some(1), Some(1)) +} diff --git a/crates/oxc_linter/src/rules/unicorn/no_useless_spread.rs b/crates/oxc_linter/src/rules/unicorn/no_useless_spread/mod.rs similarity index 65% rename from crates/oxc_linter/src/rules/unicorn/no_useless_spread.rs rename to crates/oxc_linter/src/rules/unicorn/no_useless_spread/mod.rs index 264e4a9e184e9..af84d9cccf268 100644 --- a/crates/oxc_linter/src/rules/unicorn/no_useless_spread.rs +++ b/crates/oxc_linter/src/rules/unicorn/no_useless_spread/mod.rs @@ -1,7 +1,9 @@ +mod const_eval; + use oxc_ast::{ ast::{ - match_expression, Argument, ArrayExpression, ArrayExpressionElement, CallExpression, - Expression, SpreadElement, + ArrayExpression, ArrayExpressionElement, CallExpression, Expression, NewExpression, + ObjectExpression, ObjectPropertyKind, SpreadElement, }, AstKind, }; @@ -18,6 +20,7 @@ use crate::{ rule::Rule, AstNode, }; +use const_eval::{is_array_from, is_new_typed_array, ConstEval}; fn spread_in_list(span: Span, x1: &str) -> OxcDiagnostic { OxcDiagnostic::warn(format!("Using a spread operator here creates a new {x1} unnecessarily.")) @@ -49,10 +52,16 @@ fn iterable_to_array_in_yield_star(span: Span) -> OxcDiagnostic { .with_label(span) } -fn clone_array(span: Span, x1: &str) -> OxcDiagnostic { - OxcDiagnostic::warn("Using a spread operator here creates a new array unnecessarily.") - .with_help(format!("`{x1}` returns a new array. Spreading it into an array expression to create a new array is redundant.")) - .with_label(span) +fn clone(span: Span, is_array: bool, x1: Option<&str>) -> OxcDiagnostic { + let noun = if is_array { "array" } else { "object" }; + OxcDiagnostic::warn(format!("Using a spread operator here creates a new {noun} unnecessarily.")) + .with_help( + if let Some(x1) = x1 { + format!("`{x1}` returns a new {noun}. Spreading it into an {noun} expression to create a new {noun} is redundant.") + } else { + + format!("This expression returns a new {noun}. Spreading it into an {noun} expression to create a new {noun} is redundant.") + }).with_label(span) } #[derive(Debug, Default, Clone)] @@ -133,29 +142,48 @@ declare_oxc_lint!( impl Rule for NoUselessSpread { fn run<'a>(&self, node: &AstNode<'a>, ctx: &LintContext<'a>) { - check_useless_spread_in_list(node, ctx); + if !matches!(node.kind(), AstKind::ArrayExpression(_) | AstKind::ObjectExpression(_)) { + return; + } + + if check_useless_spread_in_list(node, ctx) { + return; + } + + match node.kind() { + AstKind::ArrayExpression(array_expr) => { + let Some(spread_elem) = as_single_array_spread(array_expr) else { + return; + }; - if let AstKind::ArrayExpression(array_expr) = node.kind() { - check_useless_iterable_to_array(node, array_expr, ctx); - check_useless_array_clone(array_expr, ctx); + if check_useless_iterable_to_array(node, array_expr, spread_elem, ctx) { + return; + } + + check_useless_clone(array_expr.span, true, spread_elem, ctx); + } + AstKind::ObjectExpression(obj_expr) => { + let Some(spread_elem) = as_single_obj_spread(obj_expr) else { + return; + }; + check_useless_clone(obj_expr.span, false, spread_elem, ctx); + } + _ => unreachable!(), } } } -fn check_useless_spread_in_list<'a>(node: &AstNode<'a>, ctx: &LintContext<'a>) { - if !matches!(node.kind(), AstKind::ArrayExpression(_) | AstKind::ObjectExpression(_)) { - return; - } +fn check_useless_spread_in_list<'a>(node: &AstNode<'a>, ctx: &LintContext<'a>) -> bool { let Some(parent) = outermost_paren_parent(node, ctx) else { - return; + return false; }; // we're in ...[] let AstKind::SpreadElement(spread_elem) = parent.kind() else { - return; + return false; }; let Some(parent_parent) = outermost_paren_parent(parent, ctx) else { - return; + return false; }; let span = Span::new(spread_elem.span.start, spread_elem.span.start + 3); @@ -164,7 +192,12 @@ fn check_useless_spread_in_list<'a>(node: &AstNode<'a>, ctx: &LintContext<'a>) { AstKind::ObjectExpression(_) => { // { ...{ } } if matches!(parent_parent.kind(), AstKind::ObjectExpression(_)) { - ctx.diagnostic(spread_in_list(span, "object")); + ctx.diagnostic_with_fix(spread_in_list(span, "object"), |fixer| { + fix_by_removing_object_spread(fixer, spread_elem) + }); + true + } else { + false } } AstKind::ArrayExpression(array_expr) => match parent_parent.kind() { @@ -176,14 +209,16 @@ fn check_useless_spread_in_list<'a>(node: &AstNode<'a>, ctx: &LintContext<'a>) { } else { ctx.diagnostic(diagnostic); } + true } // foo(...[ ]) AstKind::Argument(_) => { ctx.diagnostic_with_fix(spread_in_arguments(span), |fixer| { - fix_by_removing_spread(fixer, array_expr, spread_elem) + fix_by_removing_array_spread(fixer, array_expr, spread_elem) }); + true } - _ => {} + _ => false, }, _ => { unreachable!() @@ -206,7 +241,7 @@ fn diagnose_array_in_array_spread<'a>( 0 => unreachable!(), 1 => { ctx.diagnostic_with_fix(diagnostic, |fixer| { - fix_replace(fixer, &outer_array.span, inner_array) + fixer.replace_with(&outer_array.span, inner_array) }); } _ => { @@ -248,25 +283,18 @@ fn diagnose_array_in_array_spread<'a>( fn check_useless_iterable_to_array<'a>( node: &AstNode<'a>, array_expr: &ArrayExpression<'a>, + spread_elem: &SpreadElement<'a>, ctx: &LintContext<'a>, -) { +) -> bool { let Some(parent) = outermost_paren_parent(node, ctx) else { - return; - }; - - if !is_single_array_spread(array_expr) { - return; - } - - let ArrayExpressionElement::SpreadElement(spread_elem) = &array_expr.elements[0] else { - return; + return false; }; let span = Span::new(spread_elem.span.start, spread_elem.span.start + 3); let parent = if let AstKind::Argument(_) = parent.kind() { let Some(parent) = outermost_paren_parent(parent, ctx) else { - return; + return false; }; parent } else { @@ -277,47 +305,31 @@ fn check_useless_iterable_to_array<'a>( AstKind::ForOfStatement(for_of_stmt) => { if for_of_stmt.right.without_parenthesized().span() == array_expr.span { ctx.diagnostic(iterable_to_array_in_for_of(span)); + return true; } + false } AstKind::YieldExpression(yield_expr) => { if yield_expr.delegate && yield_expr.argument.as_ref().is_some_and(|arg| arg.span() == array_expr.span) { ctx.diagnostic(iterable_to_array_in_yield_star(span)); + return true; } + false } AstKind::NewExpression(new_expr) => { - if !((is_new_expression( - new_expr, - &["Map", "WeakMap", "Set", "WeakSet"], - Some(1), - Some(1), - ) || is_new_expression( - new_expr, - &[ - "Int8Array", - "Uint8Array", - "Uint8ClampedArray", - "Int16Array", - "Uint16Array", - "Int32Array", - "Uint32Array", - "Float32Array", - "Float64Array", - "BigInt64Array", - "BigUint64Array", - ], - Some(1), - None, - )) && innermost_paren_arg_span(&new_expr.arguments[0]) == array_expr.span) + if !((is_new_map_or_set_with_iterable(new_expr) || is_new_typed_array(new_expr)) + && new_expr.arguments[0].span().contains_inclusive(array_expr.span)) { - return; + return false; } ctx.diagnostic_with_fix( iterable_to_array(span, get_new_expr_ident_name(new_expr).unwrap_or("unknown")), - |fixer| fix_by_removing_spread(fixer, &new_expr.arguments[0], spread_elem), + |fixer| fix_by_removing_array_spread(fixer, &new_expr.arguments[0], spread_elem), ); + true } AstKind::CallExpression(call_expr) => { if !((is_method_call( @@ -326,34 +338,17 @@ fn check_useless_iterable_to_array<'a>( Some(&["all", "allSettled", "any", "race"]), Some(1), Some(1), - ) || is_method_call( - call_expr, - Some(&[ - "Array", - "Int8Array", - "Uint8Array", - "Uint8ClampedArray", - "Int16Array", - "Uint16Array", - "Int32Array", - "Uint32Array", - "Float32Array", - "Float64Array", - "BigInt64Array", - "BigUint64Array", - ]), - Some(&["from"]), - Some(1), - Some(1), - ) || is_method_call( - call_expr, - Some(&["Object"]), - Some(&["fromEntries"]), - Some(1), - Some(1), - )) && innermost_paren_arg_span(&call_expr.arguments[0]) == array_expr.span) + ) || is_array_from(call_expr) + || is_method_call( + call_expr, + Some(&["Object"]), + Some(&["fromEntries"]), + Some(1), + Some(1), + )) + && call_expr.arguments[0].span().contains_inclusive(array_expr.span)) { - return; + return false; } ctx.diagnostic_with_fix( @@ -361,118 +356,123 @@ fn check_useless_iterable_to_array<'a>( span, &get_method_name(call_expr).unwrap_or_else(|| "unknown".into()), ), - |fixer| fix_by_removing_spread(fixer, array_expr, spread_elem), + |fixer| fix_by_removing_array_spread(fixer, array_expr, spread_elem), ); + true } - _ => {} + _ => false, } } -fn check_useless_array_clone<'a>(array_expr: &ArrayExpression<'a>, ctx: &LintContext<'a>) { - if !is_single_array_spread(array_expr) { - return; - } - - let ArrayExpressionElement::SpreadElement(spread_elem) = &array_expr.elements[0] else { - return; - }; +/// Matches `new {Set,WeakSet,Map,WeakMap}(iterable)` +pub fn is_new_map_or_set_with_iterable(new_expr: &NewExpression) -> bool { + is_new_expression(new_expr, &["Map", "WeakMap", "Set", "WeakSet"], Some(1), Some(1)) +} +fn check_useless_clone<'a>( + array_or_obj_span: Span, + is_array: bool, + spread_elem: &SpreadElement<'a>, + ctx: &LintContext<'a>, +) -> bool { let span = Span::new(spread_elem.span.start, spread_elem.span.start + 3); + let target = spread_elem.argument.get_inner_expression(); - if let Expression::CallExpression(call_expr) = &spread_elem.argument { - if !(is_method_call( - call_expr, - None, - Some(&[ - "concat", - "copyWithin", - "filter", - "flat", - "flatMap", - "map", - "slice", - "splice", - "toReversed", - "toSorted", - "toSpliced", - "with", - ]), - None, - None, - ) || is_method_call(call_expr, None, Some(&["split"]), None, None) - || is_method_call(call_expr, Some(&["Object"]), Some(&["keys", "values"]), None, None) - || is_method_call(call_expr, Some(&["Array"]), Some(&["from", "of"]), None, None)) - { - return; - } - - let method = call_expr.callee.get_member_expr().map_or_else( - || "unknown".into(), - |method| { - let object_name = if let Expression::Identifier(ident) = &method.object() { - ident.name.as_str() - } else { - "unknown" - }; + // already diagnosed by first check + if matches!(target, Expression::ArrayExpression(_) | Expression::ObjectExpression(_)) { + return false; + } - format!("{}.{}", object_name, method.static_property_name().unwrap()) - }, - ); + let hint = target.const_eval(); + let hint_matches_expr = if is_array { hint.is_array() } else { hint.is_object() }; + if hint_matches_expr { + let name = diagnostic_name(ctx, target); - ctx.diagnostic_with_fix(clone_array(span, &method), |fixer| { - fix_by_removing_spread(fixer, array_expr, spread_elem) + ctx.diagnostic_with_fix(clone(span, is_array, name), |fixer| { + fix_by_removing_array_spread(fixer, &array_or_obj_span, spread_elem) }); + return true; } + false +} - if let Expression::AwaitExpression(await_expr) = &spread_elem.argument { - if let Expression::CallExpression(call_expr) = &await_expr.argument { - if !is_method_call( - call_expr, - Some(&["Promise"]), - Some(&["all", "allSettled"]), - Some(1), - Some(1), - ) { - return; - } - let method_name = - call_expr.callee.get_member_expr().unwrap().static_property_name().unwrap(); +fn diagnostic_name<'a>(ctx: &LintContext<'a>, expr: &Expression<'a>) -> Option<&'a str> { + fn pretty_snippet(snippet: &str) -> Option<&str> { + // unweildy snippets don't get included in diagnostic messages + if snippet.len() > 50 || snippet.contains('\n') { + None + } else { + Some(snippet) + } + } - ctx.diagnostic_with_fix( - clone_array(span, &format!("Promise.{method_name}")), - |fixer| fix_by_removing_spread(fixer, array_expr, spread_elem), - ); + match expr { + Expression::CallExpression(call) => diagnostic_name(ctx, &call.callee), + Expression::AwaitExpression(expr) => diagnostic_name(ctx, &expr.argument), + Expression::SequenceExpression(expr) => { + let span_with_parens = expr.span().expand(1); + pretty_snippet(ctx.source_range(span_with_parens)) } + _ => pretty_snippet(ctx.source_range(expr.span())), } } -fn fix_replace<'a, T: GetSpan, U: GetSpan>( +/// Creates a fix that replaces `[...spread]` with `spread` +fn fix_by_removing_array_spread<'a, S: GetSpan>( fixer: RuleFixer<'_, 'a>, - target: &T, - replacement: &U, + iterable: &S, + spread: &SpreadElement<'a>, ) -> RuleFix<'a> { - let replacement = fixer.source_range(replacement.span()); - fixer.replace(target.span(), replacement) + fixer.replace(iterable.span(), fixer.source_range(spread.argument.span())) } -/// Creates a fix that replaces `[...spread]` with `spread` -fn fix_by_removing_spread<'a, S: GetSpan>( +/// Creates a fix that replaces `{...spread}` with `spread`, when `spread` is an +/// object literal +/// +/// ## Examples +/// - `{...{ a, b, }}` -> `{ a, b }` +fn fix_by_removing_object_spread<'a>( fixer: RuleFixer<'_, 'a>, - iterable: &S, spread: &SpreadElement<'a>, ) -> RuleFix<'a> { - fixer.replace(iterable.span(), fixer.source_range(spread.argument.span())) + // get contents inside object brackets + // e.g. `...{ a, b, }` -> ` a, b, ` + let replacement_span = &spread.argument.span().shrink(1); + + // remove trailing commas to avoid syntax errors if this spread is followed + // by another property + // e.g. ` a, b, ` -> `a, b` + let mut end_shrink_amount = 0; + for c in fixer.source_range(*replacement_span).chars().rev() { + if c.is_whitespace() || c == ',' { + end_shrink_amount += 1; + } else { + break; + } + } + let replacement_span = replacement_span.shrink_right(end_shrink_amount); + + fixer.replace_with(&spread.span, &replacement_span) } /// Checks if `node` is `[...(expr)]` -fn is_single_array_spread(node: &ArrayExpression) -> bool { - node.elements.len() == 1 && matches!(node.elements[0], ArrayExpressionElement::SpreadElement(_)) +fn as_single_array_spread<'a, 's>(node: &'s ArrayExpression<'a>) -> Option<&'s SpreadElement<'a>> { + if node.elements.len() != 1 { + return None; + } + match &node.elements[0] { + ArrayExpressionElement::SpreadElement(spread) => Some(spread.as_ref()), + _ => None, + } } -fn innermost_paren_arg_span(arg: &Argument) -> Span { - match arg { - match_expression!(Argument) => arg.to_expression().without_parenthesized().span(), - Argument::SpreadElement(spread_elem) => spread_elem.argument.span(), +fn as_single_obj_spread<'a, 's>(node: &'s ObjectExpression<'a>) -> Option<&'s SpreadElement<'a>> { + if node.properties.len() != 1 { + return None; + } + match &node.properties[0] { + ObjectPropertyKind::SpreadProperty(spread) => Some(spread.as_ref()), + ObjectPropertyKind::ObjectProperty(_) => None, } } @@ -488,6 +488,23 @@ fn get_method_name(call_expr: &CallExpression) -> Option { Some(format!("{}.{}", object_name, callee.static_property_name().unwrap())) } +#[test] +fn test_debug() { + use crate::tester::Tester; + + let pass = vec![]; + + let fail = vec![ + // "[...arr.reduce(f, new Set())]", + "const obj = { a, ...{ b, c } }", + // "const promise = Promise.all([...iterable])", + // "const obj = { ...(foo ? { a: 1 } : { a: 2 }) }", + // "const array = [...[a]]", + ]; + + Tester::new(NoUselessSpread::NAME, pass, fail).test(); +} + #[test] fn test() { use crate::tester::Tester; @@ -551,13 +568,19 @@ fn test() { r"[...array.unknown()]", r"[...Object.notReturningArray(foo)]", r"[...NotObject.keys(foo)]", - r"[...Int8Array.from(foo)]", - r"[...Int8Array.of()]", - r"[...new Int8Array(3)]", + // NOTE (@DonIsaac) these are pathological, should really not be done, + // and supporting them would add a lot of complexity to the rule's + // implementation. + // r"[...Int8Array.from(foo)]", + // r"[...Int8Array.of()]", + // r"[...new Int8Array(3)]", r"[...Promise.all(foo)]", r"[...Promise.allSettled(foo)]", r"[...await Promise.all(foo, extraArgument)]", - r"[...new Array(3)]", + // Complex object spreads + r"const obj = { ...obj, ...(addFoo ? { foo: 'foo' } : {}) }", + r"