From 3e694573dfb9dda15f72ab22c3dc4253b641d844 Mon Sep 17 00:00:00 2001 From: Don Isaac Date: Fri, 7 Jun 2024 23:38:38 -0400 Subject: [PATCH] up(linter): add fixer for no-useless-spread (#3583) Only fixes array spreads for now. --------- Co-authored-by: Boshen --- crates/oxc_ast/src/span.rs | 6 + crates/oxc_linter/src/context.rs | 13 +- .../unicorn/no_useless_fallback_in_spread.rs | 5 +- .../src/rules/unicorn/no_useless_spread.rs | 148 ++++++++++++------ crates/oxc_linter/src/tester.rs | 36 ++++- 5 files changed, 152 insertions(+), 56 deletions(-) diff --git a/crates/oxc_ast/src/span.rs b/crates/oxc_ast/src/span.rs index c08683f2c..98371ce85 100644 --- a/crates/oxc_ast/src/span.rs +++ b/crates/oxc_ast/src/span.rs @@ -347,6 +347,12 @@ impl<'a> GetSpan for Argument<'a> { } } +impl<'a> GetSpan for ArrayExpression<'a> { + fn span(&self) -> Span { + self.span + } +} + impl<'a> GetSpan for ArrayExpressionElement<'a> { fn span(&self) -> Span { match self { diff --git a/crates/oxc_linter/src/context.rs b/crates/oxc_linter/src/context.rs index be6ba77fc..5c53c13a3 100644 --- a/crates/oxc_linter/src/context.rs +++ b/crates/oxc_linter/src/context.rs @@ -3,7 +3,7 @@ use std::{cell::RefCell, path::Path, rc::Rc, sync::Arc}; use oxc_codegen::{Codegen, CodegenOptions}; use oxc_diagnostics::{OxcDiagnostic, Severity}; use oxc_semantic::{AstNodes, JSDocFinder, ScopeTree, Semantic, SymbolTable}; -use oxc_span::SourceType; +use oxc_span::{SourceType, Span}; use crate::{ disable_directives::{DisableDirectives, DisableDirectivesBuilder}, @@ -81,10 +81,17 @@ impl<'a> LintContext<'a> { &self.disable_directives } + /// Source code of the file being linted. pub fn source_text(&self) -> &'a str { self.semantic().source_text() } + /// Get a snippet of source text covered by the given [`Span`]. For details, + /// see [`Span::source_text`]. + pub fn source_range(&self, span: Span) -> &'a str { + span.source_text(self.semantic().source_text()) + } + pub fn source_type(&self) -> &SourceType { self.semantic().source_type() } @@ -132,10 +139,14 @@ impl<'a> LintContext<'a> { } } + /// Report a lint rule violation. + /// + /// Use [`LintContext::diagnostic_with_fix`] to provide an automatic fix. pub fn diagnostic(&self, diagnostic: OxcDiagnostic) { self.add_diagnostic(Message::new(diagnostic, None)); } + /// Report a lint rule violation and provide an automatic fix. pub fn diagnostic_with_fix Fix<'a>>(&self, diagnostic: OxcDiagnostic, fix: F) { if self.fix { self.add_diagnostic(Message::new(diagnostic, Some(fix()))); diff --git a/crates/oxc_linter/src/rules/unicorn/no_useless_fallback_in_spread.rs b/crates/oxc_linter/src/rules/unicorn/no_useless_fallback_in_spread.rs index e2862f272..32ca2b1e0 100644 --- a/crates/oxc_linter/src/rules/unicorn/no_useless_fallback_in_spread.rs +++ b/crates/oxc_linter/src/rules/unicorn/no_useless_fallback_in_spread.rs @@ -81,7 +81,7 @@ impl Rule for NoUselessFallbackInSpread { if can_fix(&logical_expression.left) { ctx.diagnostic_with_fix(diagnostic, || { - let left_text = logical_expression.left.span().source_text(ctx.source_text()); + let left_text = ctx.source_range(logical_expression.left.span()); Fix::new(format!("...{left_text}"), spread_element.span) }); } else { @@ -96,6 +96,7 @@ fn can_fix(left: &Expression<'_>) -> bool { Expression::Identifier(ident) => !BANNED_IDENTIFIERS.contains(&ident.name.as_str()), Expression::LogicalExpression(expr) => can_fix(&expr.left), Expression::ObjectExpression(_) + | Expression::ChainExpression(_) | Expression::CallExpression(_) | Expression::StaticMemberExpression(_) | Expression::ComputedMemberExpression(_) => true, @@ -158,7 +159,9 @@ fn test() { let fix = vec![ // (r"const object = {...(foo || {})}", r"const object = {...foo}"), + (r"const object = {...(foo?.bar || {})}", r"const object = {...foo?.bar}"), (r"const object = {...(foo() || {})}", r"const object = {...foo()}"), + (r"const object = {...(foo?.bar() || {})}", r"const object = {...foo?.bar()}"), (r"const object = {...((foo && {}) || {})}", "const object = {...(foo && {})}"), (r"const object = {...(0 || {})}", r"const object = {...(0 || {})}"), (r"const object = {...(NaN || {})}", r"const object = {...(NaN || {})}"), diff --git a/crates/oxc_linter/src/rules/unicorn/no_useless_spread.rs b/crates/oxc_linter/src/rules/unicorn/no_useless_spread.rs index c1e3e550d..86bd33140 100644 --- a/crates/oxc_linter/src/rules/unicorn/no_useless_spread.rs +++ b/crates/oxc_linter/src/rules/unicorn/no_useless_spread.rs @@ -1,7 +1,7 @@ use oxc_ast::{ ast::{ match_expression, Argument, ArrayExpression, ArrayExpressionElement, CallExpression, - Expression, + Expression, SpreadElement, }, AstKind, }; @@ -15,42 +15,43 @@ use crate::{ get_new_expr_ident_name, is_method_call, is_new_expression, outermost_paren_parent, }, context::LintContext, + fixer::Fix, rule::Rule, AstNode, }; -fn spread_in_list(span0: Span, x1: &str) -> OxcDiagnostic { +fn spread_in_list(span: Span, x1: &str) -> OxcDiagnostic { OxcDiagnostic::warn(format!("eslint-plugin-unicorn(no-useless-spread): Using a spread operator here creates a new {x1} unnecessarily.")) .with_help("Consider removing the spread operator.") - .with_labels([span0.into()]) + .with_labels([span.into()]) } -fn spread_in_arguments(span0: Span) -> OxcDiagnostic { - OxcDiagnostic::warn("eslint-plugin-unicorn(no-useless-spread): Using a spread operator here creates a new array unnecessarily.").with_help("This function accepts a rest parameter, it's unnecessary to create a new array and then spread it. Instead, supply the arguments directly.\nFor example, replace `foo(...[1, 2, 3])` with `foo(1, 2, 3)`.").with_labels([span0.into()]) +fn spread_in_arguments(span: Span) -> OxcDiagnostic { + OxcDiagnostic::warn("eslint-plugin-unicorn(no-useless-spread): Using a spread operator here creates a new array unnecessarily.").with_help("This function accepts a rest parameter, it's unnecessary to create a new array and then spread it. Instead, supply the arguments directly.\nFor example, replace `foo(...[1, 2, 3])` with `foo(1, 2, 3)`.").with_labels([span.into()]) } -fn iterable_to_array(span0: Span, x1: &str) -> OxcDiagnostic { +fn iterable_to_array(span: Span, x1: &str) -> OxcDiagnostic { OxcDiagnostic::warn(format!("eslint-plugin-unicorn(no-useless-spread): `{x1}` accepts an iterable, so it's unnecessary to convert the iterable to an array.")) .with_help("Consider removing the spread operator.") - .with_labels([span0.into()]) + .with_labels([span.into()]) } -fn iterable_to_array_in_for_of(span0: Span) -> OxcDiagnostic { +fn iterable_to_array_in_for_of(span: Span) -> OxcDiagnostic { OxcDiagnostic::warn("eslint-plugin-unicorn(no-useless-spread): Using a spread operator here creates a new array unnecessarily.") .with_help("`for…of` can iterate over iterable, it's unnecessary to convert to an array.") - .with_labels([span0.into()]) + .with_labels([span.into()]) } -fn iterable_to_array_in_yield_star(span0: Span) -> OxcDiagnostic { +fn iterable_to_array_in_yield_star(span: Span) -> OxcDiagnostic { OxcDiagnostic::warn("eslint-plugin-unicorn(no-useless-spread): Using a spread operator here creates a new array unnecessarily.") .with_help("`yield*` can delegate to another iterable, so it's unnecessary to convert the iterable to an array.") - .with_labels([span0.into()]) + .with_labels([span.into()]) } -fn clone_array(span0: Span, x1: &str) -> OxcDiagnostic { +fn clone_array(span: Span, x1: &str) -> OxcDiagnostic { OxcDiagnostic::warn("eslint-plugin-unicorn(no-useless-spread): 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_labels([span0.into()]) + .with_labels([span.into()]) } #[derive(Debug, Default, Clone)] @@ -140,39 +141,49 @@ impl Rule for NoUselessSpread { } fn check_useless_spread_in_list<'a>(node: &AstNode<'a>, ctx: &LintContext<'a>) { - if matches!(node.kind(), AstKind::ArrayExpression(_) | AstKind::ObjectExpression(_)) { - let Some(parent) = outermost_paren_parent(node, ctx) else { + if !matches!(node.kind(), AstKind::ArrayExpression(_) | AstKind::ObjectExpression(_)) { + return; + } + let Some(parent) = outermost_paren_parent(node, ctx) else { + return; + }; + + if let AstKind::SpreadElement(spread_elem) = parent.kind() { + let Some(parent_parent) = outermost_paren_parent(parent, ctx) else { return; }; - if let AstKind::SpreadElement(spread_elem) = parent.kind() { - let Some(parent_parent) = outermost_paren_parent(parent, ctx) else { - return; - }; + let span = Span::new(spread_elem.span.start, spread_elem.span.start + 3); - let span = Span::new(spread_elem.span.start, spread_elem.span.start + 3); - - match node.kind() { - AstKind::ObjectExpression(_) => { - // { ...{ } } - if matches!(parent_parent.kind(), AstKind::ObjectExpression(_)) { - ctx.diagnostic(spread_in_list(span, "object")); + match node.kind() { + AstKind::ObjectExpression(_) => { + // { ...{ } } + if matches!(parent_parent.kind(), AstKind::ObjectExpression(_)) { + ctx.diagnostic(spread_in_list(span, "object")); + } + } + AstKind::ArrayExpression(array_expr) => match parent_parent.kind() { + // ...[ ] + AstKind::ArrayExpressionElement(_) => { + let diagnostic = spread_in_list(span, "array"); + if let Some(outer_array) = ctx.nodes().parent_kind(parent_parent.id()) { + ctx.diagnostic_with_fix(diagnostic, || { + fix_replace(ctx, &outer_array, array_expr) + }); + } else { + ctx.diagnostic(diagnostic); } } - AstKind::ArrayExpression(_) => match parent_parent.kind() { - // ...[ ] - AstKind::ArrayExpressionElement(_) => { - ctx.diagnostic(spread_in_list(span, "array")); - } - // foo(...[ ]) - AstKind::Argument(_) => { - ctx.diagnostic(spread_in_arguments(span)); - } - _ => {} - }, - _ => { - unreachable!() + // foo(...[ ]) + AstKind::Argument(_) => { + ctx.diagnostic_with_fix(spread_in_arguments(span), || { + fix_by_removing_spread(ctx, array_expr, spread_elem) + }); } + _ => {} + }, + _ => { + unreachable!() } } } @@ -247,10 +258,10 @@ fn check_useless_iterable_to_array<'a>( { return; } - ctx.diagnostic(iterable_to_array( - span, - get_new_expr_ident_name(new_expr).unwrap_or("unknown"), - )); + ctx.diagnostic_with_fix( + iterable_to_array(span, get_new_expr_ident_name(new_expr).unwrap_or("unknown")), + || fix_by_removing_spread(ctx, &new_expr.arguments[0], spread_elem), + ); } AstKind::CallExpression(call_expr) => { if !((is_method_call( @@ -289,10 +300,13 @@ fn check_useless_iterable_to_array<'a>( return; } - ctx.diagnostic(iterable_to_array( - span, - &get_method_name(call_expr).unwrap_or_else(|| "unknown".into()), - )); + ctx.diagnostic_with_fix( + iterable_to_array( + span, + &get_method_name(call_expr).unwrap_or_else(|| "unknown".into()), + ), + || fix_by_removing_spread(ctx, array_expr, spread_elem), + ); } _ => {} } @@ -349,7 +363,9 @@ fn check_useless_array_clone<'a>(array_expr: &ArrayExpression<'a>, ctx: &LintCon }, ); - ctx.diagnostic(clone_array(span, &method)); + ctx.diagnostic_with_fix(clone_array(span, &method), || { + fix_by_removing_spread(ctx, array_expr, spread_elem) + }); } if let Expression::AwaitExpression(await_expr) = &spread_elem.argument { @@ -366,11 +382,32 @@ fn check_useless_array_clone<'a>(array_expr: &ArrayExpression<'a>, ctx: &LintCon let method_name = call_expr.callee.get_member_expr().unwrap().static_property_name().unwrap(); - ctx.diagnostic(clone_array(span, &format!("Promise.{method_name}"))); + ctx.diagnostic_with_fix(clone_array(span, &format!("Promise.{method_name}")), || { + fix_by_removing_spread(ctx, array_expr, spread_elem) + }); } } } +fn fix_replace<'a, T: GetSpan, U: GetSpan>( + ctx: &LintContext<'a>, + target: &T, + replacement: &U, +) -> Fix<'a> { + let replacement = ctx.source_range(replacement.span()); + Fix::new(replacement, target.span()) +} + +/// Creates a fix that replaces `[...spread]` with `spread` +fn fix_by_removing_spread<'a, S: GetSpan>( + ctx: &LintContext<'a>, + iterable: &S, + spread: &SpreadElement<'a>, +) -> Fix<'a> { + Fix::new(ctx.source_range(spread.argument.span()), iterable.span()) +} + +/// Checks if `node` is `[...(expr)]` fn is_single_array_spread(node: &ArrayExpression) -> bool { node.elements.len() == 1 && matches!(node.elements[0], ArrayExpressionElement::SpreadElement(_)) } @@ -569,5 +606,16 @@ fn test() { ", ]; - Tester::new(NoUselessSpread::NAME, pass, fail).test_and_snapshot(); + let fix = vec![ + ("[...[1,2,3]]", "[1,2,3]"), + ("[...[...[1,2,3]]]", "[...[1,2,3]]"), + ("[...foo.map(x => !!x)]", "foo.map(x => !!x)"), + (r"[...await Promise.all(foo)]", r"await Promise.all(foo)"), + (r"const promise = Promise.any([...iterable])", r"const promise = Promise.any(iterable)"), + (r"[...Array.from(iterable)]", r"Array.from(iterable)"), + (r"new Map([...iterable])", r"new Map(iterable)"), + (r"new Map([ ...((iterable)) ])", r"new Map(((iterable)))"), + // (r"new Map(...[...iterable])", r"new Map(iterable)"), + ]; + Tester::new(NoUselessSpread::NAME, pass, fail).expect_fix(fix).test_and_snapshot(); } diff --git a/crates/oxc_linter/src/tester.rs b/crates/oxc_linter/src/tester.rs index 468432a71..873277d13 100644 --- a/crates/oxc_linter/src/tester.rs +++ b/crates/oxc_linter/src/tester.rs @@ -73,16 +73,19 @@ pub struct ExpectFix { expected: String, rule_config: Option, } + impl> From<(S, S, Option)> for ExpectFix { fn from(value: (S, S, Option)) -> Self { Self { source: value.0.into(), expected: value.1.into(), rule_config: value.2 } } } + impl> From<(S, S)> for ExpectFix { fn from(value: (S, S)) -> Self { Self { source: value.0.into(), expected: value.1.into(), rule_config: None } } } + pub struct Tester { rule_name: &'static str, rule_path: PathBuf, @@ -156,6 +159,28 @@ impl Tester { self } + /// Add cases that should fix problems found in the source code. + /// + /// These cases will fail if no fixes are produced or if the fixed source + /// code does not match the expected result. + /// + /// ``` + /// use oxc_linter::tester::Tester; + /// + /// let pass = vec![ + /// ("let x = 1", None) + /// ]; + /// let fail = vec![]; + /// // You can omit the rule_config if you never use it, + /// //otherwise its an Option + /// let fix = vec![ + /// // source, expected, rule_config? + /// ("let x = 1", "let x = 1", None) + /// ]; + /// + /// // the first argument is normally `MyRuleStruct::NAME`. + /// Tester::new("no-undef", pass, fail).expect_fix(fix).test(); + /// ``` pub fn expect_fix>(mut self, expect_fix: Vec) -> Self { self.expect_fix = expect_fix.into_iter().map(std::convert::Into::into).collect::>(); self @@ -199,10 +224,13 @@ impl Tester { for fix in self.expect_fix.clone() { let ExpectFix { source, expected, rule_config: config } = fix; let result = self.run(&source, config, &None, None, true); - if let TestResult::Fixed(fixed_str) = result { - assert_eq!(expected, fixed_str); - } else { - unreachable!() + match result { + TestResult::Fixed(fixed_str) => assert_eq!( + expected, fixed_str, + r#"Expected "{source}" to be fixed into "{expected}""# + ), + TestResult::Passed => panic!("Expected a fix, but test passed: {source}"), + TestResult::Failed => panic!("Expected a fix, but test failed: {source}"), } } }