From 7521f59c3a5f70e1c37901a8ca4bf010dbea539e Mon Sep 17 00:00:00 2001 From: keita Date: Tue, 13 Aug 2024 15:56:27 +0900 Subject: [PATCH 1/8] feat(linter): eslint-plugin-react jsx-props-no-spread-multi --- crates/oxc_linter/src/rules.rs | 2 + .../rules/react/jsx_props_no_spread_multi.rs | 95 +++++++++++++++++++ .../snapshots/jsx_props_no_spread_multi.snap | 38 ++++++++ 3 files changed, 135 insertions(+) create mode 100644 crates/oxc_linter/src/rules/react/jsx_props_no_spread_multi.rs create mode 100644 crates/oxc_linter/src/snapshots/jsx_props_no_spread_multi.snap diff --git a/crates/oxc_linter/src/rules.rs b/crates/oxc_linter/src/rules.rs index 344112dce1395..ccad7740e77d7 100644 --- a/crates/oxc_linter/src/rules.rs +++ b/crates/oxc_linter/src/rules.rs @@ -227,6 +227,7 @@ mod react { pub mod jsx_no_target_blank; pub mod jsx_no_undef; pub mod jsx_no_useless_fragment; + pub mod jsx_props_no_spread_multi; pub mod no_children_prop; pub mod no_danger; pub mod no_direct_mutation_state; @@ -733,6 +734,7 @@ oxc_macros::declare_all_lint_rules! { react::jsx_no_comment_textnodes, react::jsx_no_duplicate_props, react::jsx_no_useless_fragment, + react::jsx_props_no_spread_multi, react::jsx_no_undef, react::react_in_jsx_scope, react::no_children_prop, diff --git a/crates/oxc_linter/src/rules/react/jsx_props_no_spread_multi.rs b/crates/oxc_linter/src/rules/react/jsx_props_no_spread_multi.rs new file mode 100644 index 0000000000000..60e3e3d8ce95a --- /dev/null +++ b/crates/oxc_linter/src/rules/react/jsx_props_no_spread_multi.rs @@ -0,0 +1,95 @@ +use std::collections::HashSet; + +use oxc_ast::{ast::JSXAttributeItem, AstKind}; +use oxc_diagnostics::OxcDiagnostic; +use oxc_macros::declare_oxc_lint; +use oxc_span::Span; + +use crate::{context::LintContext, rule::Rule, AstNode}; + +fn jsx_props_no_spread_multi_diagnostic(span0: Span) -> OxcDiagnostic { + OxcDiagnostic::warn("Disallow JSX prop spreading the same identifier multiple times.") + .with_help("Remove duplicate spread attributes.") + .with_label(span0) +} + +#[derive(Debug, Default, Clone)] +pub struct JsxPropsNoSpreadMulti; + +declare_oxc_lint!( + /// ### What it does + /// Enforces that any unique expression is only spread once. + /// Generally spreading the same expression twice is an indicator of a mistake since any attribute between the spreads may be overridden when the intent was not to. + /// Even when that is not the case this will lead to unnecessary computations being performed. + /// + /// ### Example + /// ```jsx + /// // Bad + /// + /// + /// // Good + /// + /// + /// ``` + JsxPropsNoSpreadMulti, + correctness, +); + +impl Rule for JsxPropsNoSpreadMulti { + fn run<'a>(&self, node: &AstNode<'a>, ctx: &LintContext<'a>) { + if let AstKind::JSXOpeningElement(jsx_opening_el) = node.kind() { + let spread_attrs = jsx_opening_el.attributes.iter().filter_map(|attr| { + if let JSXAttributeItem::SpreadAttribute(spread_attr) = attr { + if spread_attr.argument.is_identifier_reference() { + return Some(spread_attr); + } + } + None + }); + + let mut identifier_names = HashSet::new(); + + for spread_attr in spread_attrs { + let identifier_name = + &spread_attr.argument.get_identifier_reference().unwrap().name; + if !identifier_names.insert(identifier_name) { + ctx.diagnostic(jsx_props_no_spread_multi_diagnostic(spread_attr.span)); + } + } + } + } +} + +#[test] +fn test() { + use crate::tester::Tester; + + let pass = vec![ + " + const a = {}; + + ", + " + const a = {}; + const b = {}; + + ", + ]; + + let fail = vec![ + " + const props = {}; + + ", + r#" + const props = {}; +
+ "#, + " + const props = {}; +
+ ", + ]; + + Tester::new(JsxPropsNoSpreadMulti::NAME, pass, fail).test_and_snapshot(); +} diff --git a/crates/oxc_linter/src/snapshots/jsx_props_no_spread_multi.snap b/crates/oxc_linter/src/snapshots/jsx_props_no_spread_multi.snap new file mode 100644 index 0000000000000..c9b6d3264c914 --- /dev/null +++ b/crates/oxc_linter/src/snapshots/jsx_props_no_spread_multi.snap @@ -0,0 +1,38 @@ +--- +source: crates/oxc_linter/src/tester.rs +--- + ⚠ eslint-plugin-react(jsx-props-no-spread-multi): Disallow JSX prop spreading the same identifier multiple times. + ╭─[jsx_props_no_spread_multi.tsx:3:27] + 2 │ const props = {}; + 3 │ + · ────────── + 4 │ + ╰──── + help: Remove duplicate spread attributes. + + ⚠ eslint-plugin-react(jsx-props-no-spread-multi): Disallow JSX prop spreading the same identifier multiple times. + ╭─[jsx_props_no_spread_multi.tsx:3:33] + 2 │ const props = {}; + 3 │
+ · ────────── + 4 │ + ╰──── + help: Remove duplicate spread attributes. + + ⚠ eslint-plugin-react(jsx-props-no-spread-multi): Disallow JSX prop spreading the same identifier multiple times. + ╭─[jsx_props_no_spread_multi.tsx:3:27] + 2 │ const props = {}; + 3 │
+ · ────────── + 4 │ + ╰──── + help: Remove duplicate spread attributes. + + ⚠ eslint-plugin-react(jsx-props-no-spread-multi): Disallow JSX prop spreading the same identifier multiple times. + ╭─[jsx_props_no_spread_multi.tsx:3:38] + 2 │ const props = {}; + 3 │
+ · ────────── + 4 │ + ╰──── + help: Remove duplicate spread attributes. From 274c55d71a84e892802fd29becd98ce8989b50d8 Mon Sep 17 00:00:00 2001 From: keita hino Date: Tue, 13 Aug 2024 18:15:39 +0900 Subject: [PATCH 2/8] fix: point to the first span as well --- .../rules/react/jsx_props_no_spread_multi.rs | 19 ++++++++++++------- .../snapshots/jsx_props_no_spread_multi.snap | 16 ++++++++-------- 2 files changed, 20 insertions(+), 15 deletions(-) diff --git a/crates/oxc_linter/src/rules/react/jsx_props_no_spread_multi.rs b/crates/oxc_linter/src/rules/react/jsx_props_no_spread_multi.rs index 60e3e3d8ce95a..975e33417c1bd 100644 --- a/crates/oxc_linter/src/rules/react/jsx_props_no_spread_multi.rs +++ b/crates/oxc_linter/src/rules/react/jsx_props_no_spread_multi.rs @@ -1,16 +1,16 @@ -use std::collections::HashSet; +use std::collections::HashMap; use oxc_ast::{ast::JSXAttributeItem, AstKind}; use oxc_diagnostics::OxcDiagnostic; use oxc_macros::declare_oxc_lint; -use oxc_span::Span; +use oxc_span::{Atom, Span}; use crate::{context::LintContext, rule::Rule, AstNode}; -fn jsx_props_no_spread_multi_diagnostic(span0: Span) -> OxcDiagnostic { +fn jsx_props_no_spread_multi_diagnostic(span0: Span, span1: Span) -> OxcDiagnostic { OxcDiagnostic::warn("Disallow JSX prop spreading the same identifier multiple times.") .with_help("Remove duplicate spread attributes.") - .with_label(span0) + .with_labels([span0, span1]) } #[derive(Debug, Default, Clone)] @@ -47,13 +47,18 @@ impl Rule for JsxPropsNoSpreadMulti { None }); - let mut identifier_names = HashSet::new(); + let mut identifier_names: HashMap<&Atom, Span> = HashMap::new(); for spread_attr in spread_attrs { let identifier_name = &spread_attr.argument.get_identifier_reference().unwrap().name; - if !identifier_names.insert(identifier_name) { - ctx.diagnostic(jsx_props_no_spread_multi_diagnostic(spread_attr.span)); + if let Some(first_span) = identifier_names.get(&identifier_name) { + ctx.diagnostic(jsx_props_no_spread_multi_diagnostic( + *first_span, + spread_attr.span, + )); + } else { + identifier_names.insert(identifier_name, spread_attr.span); } } } diff --git a/crates/oxc_linter/src/snapshots/jsx_props_no_spread_multi.snap b/crates/oxc_linter/src/snapshots/jsx_props_no_spread_multi.snap index c9b6d3264c914..6b3e108230b46 100644 --- a/crates/oxc_linter/src/snapshots/jsx_props_no_spread_multi.snap +++ b/crates/oxc_linter/src/snapshots/jsx_props_no_spread_multi.snap @@ -2,37 +2,37 @@ source: crates/oxc_linter/src/tester.rs --- ⚠ eslint-plugin-react(jsx-props-no-spread-multi): Disallow JSX prop spreading the same identifier multiple times. - ╭─[jsx_props_no_spread_multi.tsx:3:27] + ╭─[jsx_props_no_spread_multi.tsx:3:16] 2 │ const props = {}; 3 │ - · ────────── + · ────────── ────────── 4 │ ╰──── help: Remove duplicate spread attributes. ⚠ eslint-plugin-react(jsx-props-no-spread-multi): Disallow JSX prop spreading the same identifier multiple times. - ╭─[jsx_props_no_spread_multi.tsx:3:33] + ╭─[jsx_props_no_spread_multi.tsx:3:16] 2 │ const props = {}; 3 │
- · ────────── + · ────────── ────────── 4 │ ╰──── help: Remove duplicate spread attributes. ⚠ eslint-plugin-react(jsx-props-no-spread-multi): Disallow JSX prop spreading the same identifier multiple times. - ╭─[jsx_props_no_spread_multi.tsx:3:27] + ╭─[jsx_props_no_spread_multi.tsx:3:16] 2 │ const props = {}; 3 │
- · ────────── + · ────────── ────────── 4 │ ╰──── help: Remove duplicate spread attributes. ⚠ eslint-plugin-react(jsx-props-no-spread-multi): Disallow JSX prop spreading the same identifier multiple times. - ╭─[jsx_props_no_spread_multi.tsx:3:38] + ╭─[jsx_props_no_spread_multi.tsx:3:16] 2 │ const props = {}; 3 │
- · ────────── + · ────────── ────────── 4 │ ╰──── help: Remove duplicate spread attributes. From 7e73647ad9f4d4bf84102c400552e9521c20a290 Mon Sep 17 00:00:00 2001 From: keita Date: Wed, 14 Aug 2024 09:17:54 +0900 Subject: [PATCH 3/8] fix: Point to all spread operations --- .../rules/react/jsx_props_no_spread_multi.rs | 30 ++++++++++++------- .../snapshots/jsx_props_no_spread_multi.snap | 11 +------ 2 files changed, 20 insertions(+), 21 deletions(-) diff --git a/crates/oxc_linter/src/rules/react/jsx_props_no_spread_multi.rs b/crates/oxc_linter/src/rules/react/jsx_props_no_spread_multi.rs index 975e33417c1bd..f57a02ff0c8c3 100644 --- a/crates/oxc_linter/src/rules/react/jsx_props_no_spread_multi.rs +++ b/crates/oxc_linter/src/rules/react/jsx_props_no_spread_multi.rs @@ -7,10 +7,10 @@ use oxc_span::{Atom, Span}; use crate::{context::LintContext, rule::Rule, AstNode}; -fn jsx_props_no_spread_multi_diagnostic(span0: Span, span1: Span) -> OxcDiagnostic { +fn jsx_props_no_spread_multi_diagnostic(spans: Vec) -> OxcDiagnostic { OxcDiagnostic::warn("Disallow JSX prop spreading the same identifier multiple times.") .with_help("Remove duplicate spread attributes.") - .with_labels([span0, span1]) + .with_labels(spans) } #[derive(Debug, Default, Clone)] @@ -48,19 +48,27 @@ impl Rule for JsxPropsNoSpreadMulti { }); let mut identifier_names: HashMap<&Atom, Span> = HashMap::new(); + let mut duplicate_spreads: HashMap<&Atom, Vec> = HashMap::new(); for spread_attr in spread_attrs { - let identifier_name = - &spread_attr.argument.get_identifier_reference().unwrap().name; - if let Some(first_span) = identifier_names.get(&identifier_name) { - ctx.diagnostic(jsx_props_no_spread_multi_diagnostic( - *first_span, - spread_attr.span, - )); - } else { - identifier_names.insert(identifier_name, spread_attr.span); + if let Some(identifier_name) = + spread_attr.argument.get_identifier_reference().map(|arg| &arg.name) + { + identifier_names + .entry(identifier_name) + .and_modify(|first_span| { + duplicate_spreads + .entry(identifier_name) + .or_insert_with(|| vec![*first_span]) + .push(spread_attr.span); + }) + .or_insert(spread_attr.span); } } + + for spans in duplicate_spreads.values() { + ctx.diagnostic(jsx_props_no_spread_multi_diagnostic(spans.clone())); + } } } } diff --git a/crates/oxc_linter/src/snapshots/jsx_props_no_spread_multi.snap b/crates/oxc_linter/src/snapshots/jsx_props_no_spread_multi.snap index 6b3e108230b46..babf13361c609 100644 --- a/crates/oxc_linter/src/snapshots/jsx_props_no_spread_multi.snap +++ b/crates/oxc_linter/src/snapshots/jsx_props_no_spread_multi.snap @@ -23,16 +23,7 @@ source: crates/oxc_linter/src/tester.rs ╭─[jsx_props_no_spread_multi.tsx:3:16] 2 │ const props = {}; 3 │
- · ────────── ────────── - 4 │ - ╰──── - help: Remove duplicate spread attributes. - - ⚠ eslint-plugin-react(jsx-props-no-spread-multi): Disallow JSX prop spreading the same identifier multiple times. - ╭─[jsx_props_no_spread_multi.tsx:3:16] - 2 │ const props = {}; - 3 │
- · ────────── ────────── + · ────────── ────────── ────────── 4 │ ╰──── help: Remove duplicate spread attributes. From af2c3560b566d162346094c2b3970429dc6f52f0 Mon Sep 17 00:00:00 2001 From: keita hino Date: Wed, 14 Aug 2024 09:25:23 +0900 Subject: [PATCH 4/8] fix: Use FxHashMap --- .../oxc_linter/src/rules/react/jsx_props_no_spread_multi.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/crates/oxc_linter/src/rules/react/jsx_props_no_spread_multi.rs b/crates/oxc_linter/src/rules/react/jsx_props_no_spread_multi.rs index f57a02ff0c8c3..bfa9d14ada5bd 100644 --- a/crates/oxc_linter/src/rules/react/jsx_props_no_spread_multi.rs +++ b/crates/oxc_linter/src/rules/react/jsx_props_no_spread_multi.rs @@ -1,4 +1,4 @@ -use std::collections::HashMap; +use rustc_hash::FxHashMap; use oxc_ast::{ast::JSXAttributeItem, AstKind}; use oxc_diagnostics::OxcDiagnostic; @@ -47,8 +47,8 @@ impl Rule for JsxPropsNoSpreadMulti { None }); - let mut identifier_names: HashMap<&Atom, Span> = HashMap::new(); - let mut duplicate_spreads: HashMap<&Atom, Vec> = HashMap::new(); + let mut identifier_names: FxHashMap<&Atom, Span> = FxHashMap::default(); + let mut duplicate_spreads: FxHashMap<&Atom, Vec> = FxHashMap::default(); for spread_attr in spread_attrs { if let Some(identifier_name) = From 36f69a7a690d9af8f768619c2c7c1f06884eb118 Mon Sep 17 00:00:00 2001 From: keita hino Date: Wed, 14 Aug 2024 09:34:09 +0900 Subject: [PATCH 5/8] fix: Include the prop name in the error message --- .../src/rules/react/jsx_props_no_spread_multi.rs | 8 ++++---- .../src/snapshots/jsx_props_no_spread_multi.snap | 6 +++--- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/crates/oxc_linter/src/rules/react/jsx_props_no_spread_multi.rs b/crates/oxc_linter/src/rules/react/jsx_props_no_spread_multi.rs index bfa9d14ada5bd..a3b42ca2fe89a 100644 --- a/crates/oxc_linter/src/rules/react/jsx_props_no_spread_multi.rs +++ b/crates/oxc_linter/src/rules/react/jsx_props_no_spread_multi.rs @@ -7,9 +7,9 @@ use oxc_span::{Atom, Span}; use crate::{context::LintContext, rule::Rule, AstNode}; -fn jsx_props_no_spread_multi_diagnostic(spans: Vec) -> OxcDiagnostic { +fn jsx_props_no_spread_multi_diagnostic(spans: Vec, prop_name: &str) -> OxcDiagnostic { OxcDiagnostic::warn("Disallow JSX prop spreading the same identifier multiple times.") - .with_help("Remove duplicate spread attributes.") + .with_help(format!("Prop '{prop_name}' is spread multiple times.")) .with_labels(spans) } @@ -66,8 +66,8 @@ impl Rule for JsxPropsNoSpreadMulti { } } - for spans in duplicate_spreads.values() { - ctx.diagnostic(jsx_props_no_spread_multi_diagnostic(spans.clone())); + for (identifier_name, spans) in duplicate_spreads { + ctx.diagnostic(jsx_props_no_spread_multi_diagnostic(spans, identifier_name)); } } } diff --git a/crates/oxc_linter/src/snapshots/jsx_props_no_spread_multi.snap b/crates/oxc_linter/src/snapshots/jsx_props_no_spread_multi.snap index babf13361c609..5cbb80aae7ab2 100644 --- a/crates/oxc_linter/src/snapshots/jsx_props_no_spread_multi.snap +++ b/crates/oxc_linter/src/snapshots/jsx_props_no_spread_multi.snap @@ -8,7 +8,7 @@ source: crates/oxc_linter/src/tester.rs · ────────── ────────── 4 │ ╰──── - help: Remove duplicate spread attributes. + help: Prop 'props' is spread multiple times. ⚠ eslint-plugin-react(jsx-props-no-spread-multi): Disallow JSX prop spreading the same identifier multiple times. ╭─[jsx_props_no_spread_multi.tsx:3:16] @@ -17,7 +17,7 @@ source: crates/oxc_linter/src/tester.rs · ────────── ────────── 4 │ ╰──── - help: Remove duplicate spread attributes. + help: Prop 'props' is spread multiple times. ⚠ eslint-plugin-react(jsx-props-no-spread-multi): Disallow JSX prop spreading the same identifier multiple times. ╭─[jsx_props_no_spread_multi.tsx:3:16] @@ -26,4 +26,4 @@ source: crates/oxc_linter/src/tester.rs · ────────── ────────── ────────── 4 │ ╰──── - help: Remove duplicate spread attributes. + help: Prop 'props' is spread multiple times. From c9f84b41d8e1415022178602467c5be666a0140a Mon Sep 17 00:00:00 2001 From: keita hino Date: Wed, 14 Aug 2024 09:53:57 +0900 Subject: [PATCH 6/8] =?UTF-8?q?fix:=20Add=20a=20=E2=80=98Why=20is=20this?= =?UTF-8?q?=20bad=3F=E2=80=99=20section?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- crates/oxc_linter/src/rules/react/jsx_props_no_spread_multi.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/crates/oxc_linter/src/rules/react/jsx_props_no_spread_multi.rs b/crates/oxc_linter/src/rules/react/jsx_props_no_spread_multi.rs index a3b42ca2fe89a..cdb5e7c94cd58 100644 --- a/crates/oxc_linter/src/rules/react/jsx_props_no_spread_multi.rs +++ b/crates/oxc_linter/src/rules/react/jsx_props_no_spread_multi.rs @@ -19,6 +19,8 @@ pub struct JsxPropsNoSpreadMulti; declare_oxc_lint!( /// ### What it does /// Enforces that any unique expression is only spread once. + /// + /// ### Why is this bad? /// Generally spreading the same expression twice is an indicator of a mistake since any attribute between the spreads may be overridden when the intent was not to. /// Even when that is not the case this will lead to unnecessary computations being performed. /// From f9b691f3af3cb3a679036a42b22c01a580af0efb Mon Sep 17 00:00:00 2001 From: keita hino Date: Wed, 14 Aug 2024 09:55:59 +0900 Subject: [PATCH 7/8] fix: Add TODO comments about autofix --- crates/oxc_linter/src/rules/react/jsx_props_no_spread_multi.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/crates/oxc_linter/src/rules/react/jsx_props_no_spread_multi.rs b/crates/oxc_linter/src/rules/react/jsx_props_no_spread_multi.rs index cdb5e7c94cd58..7e9b9b63a5f91 100644 --- a/crates/oxc_linter/src/rules/react/jsx_props_no_spread_multi.rs +++ b/crates/oxc_linter/src/rules/react/jsx_props_no_spread_multi.rs @@ -35,6 +35,7 @@ declare_oxc_lint!( /// ``` JsxPropsNoSpreadMulti, correctness, + pending // TODO: add auto-fix to remove the first spread. Removing the second one would change program behavior. ); impl Rule for JsxPropsNoSpreadMulti { From 7a76a0a4a8c98c596cb593188f20b3d17728dfd6 Mon Sep 17 00:00:00 2001 From: keita hino Date: Wed, 14 Aug 2024 10:03:53 +0900 Subject: [PATCH 8/8] refactor: Simplify using as_spread --- .../src/rules/react/jsx_props_no_spread_multi.rs | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/crates/oxc_linter/src/rules/react/jsx_props_no_spread_multi.rs b/crates/oxc_linter/src/rules/react/jsx_props_no_spread_multi.rs index 7e9b9b63a5f91..37b8b60a66e40 100644 --- a/crates/oxc_linter/src/rules/react/jsx_props_no_spread_multi.rs +++ b/crates/oxc_linter/src/rules/react/jsx_props_no_spread_multi.rs @@ -41,14 +41,8 @@ declare_oxc_lint!( impl Rule for JsxPropsNoSpreadMulti { fn run<'a>(&self, node: &AstNode<'a>, ctx: &LintContext<'a>) { if let AstKind::JSXOpeningElement(jsx_opening_el) = node.kind() { - let spread_attrs = jsx_opening_el.attributes.iter().filter_map(|attr| { - if let JSXAttributeItem::SpreadAttribute(spread_attr) = attr { - if spread_attr.argument.is_identifier_reference() { - return Some(spread_attr); - } - } - None - }); + let spread_attrs = + jsx_opening_el.attributes.iter().filter_map(JSXAttributeItem::as_spread); let mut identifier_names: FxHashMap<&Atom, Span> = FxHashMap::default(); let mut duplicate_spreads: FxHashMap<&Atom, Vec> = FxHashMap::default();