Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(linter): eslint-plugin-react jsx-props-no-spread-multi #4866

Merged
merged 9 commits into from
Aug 14, 2024
2 changes: 2 additions & 0 deletions crates/oxc_linter/src/rules.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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,
Expand Down
105 changes: 105 additions & 0 deletions crates/oxc_linter/src/rules/react/jsx_props_no_spread_multi.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,105 @@
use rustc_hash::FxHashMap;

use oxc_ast::{ast::JSXAttributeItem, AstKind};
use oxc_diagnostics::OxcDiagnostic;
use oxc_macros::declare_oxc_lint;
use oxc_span::{Atom, Span};

use crate::{context::LintContext, rule::Rule, AstNode};

fn jsx_props_no_spread_multi_diagnostic(spans: Vec<Span>, prop_name: &str) -> OxcDiagnostic {
OxcDiagnostic::warn("Disallow JSX prop spreading the same identifier multiple times.")
camc314 marked this conversation as resolved.
Show resolved Hide resolved
.with_help(format!("Prop '{prop_name}' is spread multiple times."))
.with_labels(spans)
}

#[derive(Debug, Default, Clone)]
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.
///
/// ### Example
DonIsaac marked this conversation as resolved.
Show resolved Hide resolved
/// ```jsx
/// // Bad
/// <App {...props} myAttr="1" {...props} />
///
/// // Good
/// <App myAttr="1" {...props} />
/// <App {...props} myAttr="1" />
/// ```
JsxPropsNoSpreadMulti,
correctness,
camc314 marked this conversation as resolved.
Show resolved Hide resolved
pending // TODO: add auto-fix to remove the first spread. Removing the second one would change program behavior.
);

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(JSXAttributeItem::as_spread);

let mut identifier_names: FxHashMap<&Atom, Span> = FxHashMap::default();
let mut duplicate_spreads: FxHashMap<&Atom, Vec<Span>> = FxHashMap::default();

for spread_attr in spread_attrs {
DonIsaac marked this conversation as resolved.
Show resolved Hide resolved
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 (identifier_name, spans) in duplicate_spreads {
ctx.diagnostic(jsx_props_no_spread_multi_diagnostic(spans, identifier_name));
}
}
}
}

#[test]
fn test() {
use crate::tester::Tester;

let pass = vec![
"
const a = {};
<App {...a} />
",
"
const a = {};
const b = {};
<App {...a} {...b} />
",
];

let fail = vec![
"
const props = {};
<App {...props} {...props} />
",
r#"
const props = {};
<div {...props} a="a" {...props} />
"#,
"
const props = {};
<div {...props} {...props} {...props} />
camc314 marked this conversation as resolved.
Show resolved Hide resolved
",
];

Tester::new(JsxPropsNoSpreadMulti::NAME, pass, fail).test_and_snapshot();
}
29 changes: 29 additions & 0 deletions crates/oxc_linter/src/snapshots/jsx_props_no_spread_multi.snap
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
---
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:16]
2 │ const props = {};
3 │ <App {...props} {...props} />
· ────────── ──────────
4 │
╰────
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]
2 │ const props = {};
3 │ <div {...props} a="a" {...props} />
· ────────── ──────────
4 │
╰────
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]
2 │ const props = {};
3 │ <div {...props} {...props} {...props} />
· ────────── ────────── ──────────
4 │
╰────
help: Prop 'props' is spread multiple times.