diff --git a/crates/oxc_linter/src/rules.rs b/crates/oxc_linter/src/rules.rs index 3813f3772..b0d1e2c16 100644 --- a/crates/oxc_linter/src/rules.rs +++ b/crates/oxc_linter/src/rules.rs @@ -253,6 +253,7 @@ mod react { pub mod jsx_no_undef; pub mod jsx_no_useless_fragment; pub mod jsx_props_no_spread_multi; + pub mod no_array_index_key; pub mod no_children_prop; pub mod no_danger; pub mod no_danger_with_children; @@ -804,6 +805,7 @@ oxc_macros::declare_all_lint_rules! { react::jsx_no_undef, react::jsx_no_useless_fragment, react::jsx_props_no_spread_multi, + react::no_array_index_key, react::no_children_prop, react::no_danger_with_children, react::no_danger, diff --git a/crates/oxc_linter/src/rules/react/no_array_index_key.rs b/crates/oxc_linter/src/rules/react/no_array_index_key.rs new file mode 100644 index 000000000..cbf11b1fd --- /dev/null +++ b/crates/oxc_linter/src/rules/react/no_array_index_key.rs @@ -0,0 +1,305 @@ +use oxc_ast::{ + ast::{ + Argument, CallExpression, Expression, JSXAttributeItem, JSXAttributeName, + JSXAttributeValue, JSXElement, JSXExpression, ObjectPropertyKind, PropertyKey, + }, + AstKind, +}; +use oxc_diagnostics::OxcDiagnostic; +use oxc_macros::declare_oxc_lint; +use oxc_span::Span; + +use crate::{ast_util::is_method_call, context::LintContext, rule::Rule, AstNode}; + +fn no_array_index_key_diagnostic(span: Span) -> OxcDiagnostic { + OxcDiagnostic::warn("Usage of Array index in keys is not allowed") + .with_help("Use a unique data-dependent key to avoid unnecessary rerenders") + .with_label(span) +} + +#[derive(Debug, Default, Clone)] +pub struct NoArrayIndexKey; + +declare_oxc_lint!( + /// ### What it does + /// Warn if an element uses an Array index in its key. + /// + /// ### Why is this bad? + /// It's a bad idea to use the array index since it doesn't uniquely identify your elements. + /// In cases where the array is sorted or an element is added to the beginning of the array, + /// the index will be changed even though the element representing that index may be the same. + /// This results in unnecessary renders. + /// + /// ### Examples + /// + /// Examples of **incorrect** code for this rule: + /// ```jsx + /// things.map((thing, index) => ( + /// + /// )); + /// ``` + /// + /// Examples of **correct** code for this rule: + /// ```jsx + /// things.map((thing, index) => ( + /// + /// )); + /// ``` + NoArrayIndexKey, + perf, +); + +fn check_jsx_element<'a>( + jsx: &'a JSXElement, + node: &'a AstNode, + ctx: &'a LintContext, + prop_name: &'static str, +) { + let Some(index_param_name) = find_index_param_name(node, ctx) else { + return; + }; + + for attr in &jsx.opening_element.attributes { + let JSXAttributeItem::Attribute(attr) = attr else { + return; + }; + + let JSXAttributeName::Identifier(ident) = &attr.name else { + return; + }; + + if ident.name.as_str() != prop_name { + return; + } + + let Some(JSXAttributeValue::ExpressionContainer(container)) = &attr.value else { + return; + }; + + let JSXExpression::Identifier(expr) = &container.expression else { + return; + }; + + if expr.name.as_str() == index_param_name { + ctx.diagnostic(no_array_index_key_diagnostic(attr.span)); + } + } +} + +fn check_react_clone_element<'a>( + call_expr: &'a CallExpression, + node: &'a AstNode, + ctx: &'a LintContext, +) { + let Some(index_param_name) = find_index_param_name(node, ctx) else { + return; + }; + + if is_method_call(call_expr, Some(&["React"]), Some(&["cloneElement"]), Some(2), Some(3)) { + let Some(Argument::ObjectExpression(obj_expr)) = call_expr.arguments.get(1) else { + return; + }; + + for prop_kind in &obj_expr.properties { + let ObjectPropertyKind::ObjectProperty(prop) = prop_kind else { + continue; + }; + + let PropertyKey::StaticIdentifier(key_ident) = &prop.key else { + continue; + }; + + let Expression::Identifier(value_ident) = &prop.value else { + continue; + }; + + if key_ident.name.as_str() == "key" && value_ident.name.as_str() == index_param_name { + ctx.diagnostic(no_array_index_key_diagnostic(obj_expr.span)); + } + } + } +} + +fn find_index_param_name<'a>(node: &'a AstNode, ctx: &'a LintContext) -> Option<&'a str> { + for ancestor in ctx.nodes().iter_parents(node.id()).skip(1) { + if let AstKind::CallExpression(call_expr) = ancestor.kind() { + let Expression::StaticMemberExpression(expr) = &call_expr.callee else { + return None; + }; + + if SECOND_INDEX_METHODS.contains(expr.property.name.as_str()) { + return find_index_param_name_by_position(call_expr, 1); + } + + if THIRD_INDEX_METHODS.contains(expr.property.name.as_str()) { + return find_index_param_name_by_position(call_expr, 2); + } + } + } + + None +} + +fn find_index_param_name_by_position<'a>( + call_expr: &'a CallExpression, + position: usize, +) -> Option<&'a str> { + match &call_expr.arguments[0] { + Argument::ArrowFunctionExpression(arrow_fn_expr) => { + return Some( + arrow_fn_expr.params.items.get(position)?.pattern.get_identifier()?.as_str(), + ); + } + + Argument::FunctionExpression(regular_fn_expr) => { + return Some( + regular_fn_expr.params.items.get(position)?.pattern.get_identifier()?.as_str(), + ); + } + + _ => (), + } + + None +} + +const SECOND_INDEX_METHODS: phf::Set<&'static str> = phf::phf_set! { + // things.map((thing, index) => ()); + "map", + // things.forEach((thing, index) => {otherThings.push();}); + "forEach", + // things.filter((thing, index) => {otherThings.push();}); + "filter", + // things.some((thing, index) => {otherThings.push();}); + "some", + // things.every((thing, index) => {otherThings.push();}); + "every", + // things.find((thing, index) => {otherThings.push();}); + "find", + // things.findIndex((thing, index) => {otherThings.push();}); + "findIndex", + // things.flatMap((thing, index) => ()); + "flatMap", +}; + +const THIRD_INDEX_METHODS: phf::Set<&'static str> = phf::phf_set! { + // things.reduce((collection, thing, index) => (collection.concat()), []); + "reduce", + // things.reduceRight((collection, thing, index) => (collection.concat()), []); + "reduceRight", +}; + +impl Rule for NoArrayIndexKey { + fn run<'a>(&self, node: &AstNode<'a>, ctx: &LintContext<'a>) { + match node.kind() { + AstKind::JSXElement(jsx) => { + check_jsx_element(jsx, node, ctx, "key"); + } + AstKind::CallExpression(call_expr) => { + check_react_clone_element(call_expr, node, ctx); + } + _ => (), + } + } +} + +#[test] +fn test() { + use crate::tester::Tester; + + let pass = vec![ + r"things.map((thing) => ( + + )); + ", + r"things.map((thing, index) => ( + React.cloneElement(thing, { key: thing.id }) + )); + ", + r"things.forEach((thing, index) => { + otherThings.push(); + }); + ", + r"things.filter((thing, index) => { + otherThings.push(); + }); + ", + r"things.some((thing, index) => { + otherThings.push(); + }); + ", + r"things.every((thing, index) => { + otherThings.push(); + }); + ", + r"things.find((thing, index) => { + otherThings.push(); + }); + ", + r"things.findIndex((thing, index) => { + otherThings.push(); + }); + ", + r"things.flatMap((thing, index) => ( + + )); + ", + r"things.reduce((collection, thing, index) => ( + collection.concat() + ), []); + ", + r"things.reduceRight((collection, thing, index) => ( + collection.concat() + ), []); + ", + ]; + + let fail = vec![ + r"things.map((thing, index) => ( + + )); + ", + r"things.map((thing, index) => ( + React.cloneElement(thing, { key: index }) + )); + ", + r"things.forEach((thing, index) => { + otherThings.push(); + }); + ", + r"things.filter((thing, index) => { + otherThings.push(); + }); + ", + r"things.some((thing, index) => { + otherThings.push(); + }); + ", + r"things.every((thing, index) => { + otherThings.push(); + }); + ", + r"things.find((thing, index) => { + otherThings.push(); + }); + ", + r"things.findIndex((thing, index) => { + otherThings.push(); + }); + ", + r"things.flatMap((thing, index) => ( + + )); + ", + r"things.reduce((collection, thing, index) => ( + collection.concat() + ), []); + ", + r"things.reduceRight((collection, thing, index) => ( + collection.concat() + ), []); + ", + ]; + + Tester::new(NoArrayIndexKey::NAME, pass, fail).test_and_snapshot(); +} diff --git a/crates/oxc_linter/src/snapshots/no_array_index_key.snap b/crates/oxc_linter/src/snapshots/no_array_index_key.snap new file mode 100644 index 000000000..a9cc27387 --- /dev/null +++ b/crates/oxc_linter/src/snapshots/no_array_index_key.snap @@ -0,0 +1,101 @@ +--- +source: crates/oxc_linter/src/tester.rs +--- + ⚠ eslint-plugin-react(no-array-index-key): Usage of Array index in keys is not allowed + ╭─[no_array_index_key.tsx:2:20] + 1 │ things.map((thing, index) => ( + 2 │ + · ─────────── + 3 │ )); + ╰──── + help: Use a unique data-dependent key to avoid unnecessary rerenders + + ⚠ eslint-plugin-react(no-array-index-key): Usage of Array index in keys is not allowed + ╭─[no_array_index_key.tsx:2:39] + 1 │ things.map((thing, index) => ( + 2 │ React.cloneElement(thing, { key: index }) + · ────────────── + 3 │ )); + ╰──── + help: Use a unique data-dependent key to avoid unnecessary rerenders + + ⚠ eslint-plugin-react(no-array-index-key): Usage of Array index in keys is not allowed + ╭─[no_array_index_key.tsx:2:37] + 1 │ things.forEach((thing, index) => { + 2 │ otherThings.push(); + · ─────────── + 3 │ }); + ╰──── + help: Use a unique data-dependent key to avoid unnecessary rerenders + + ⚠ eslint-plugin-react(no-array-index-key): Usage of Array index in keys is not allowed + ╭─[no_array_index_key.tsx:2:37] + 1 │ things.filter((thing, index) => { + 2 │ otherThings.push(); + · ─────────── + 3 │ }); + ╰──── + help: Use a unique data-dependent key to avoid unnecessary rerenders + + ⚠ eslint-plugin-react(no-array-index-key): Usage of Array index in keys is not allowed + ╭─[no_array_index_key.tsx:2:37] + 1 │ things.some((thing, index) => { + 2 │ otherThings.push(); + · ─────────── + 3 │ }); + ╰──── + help: Use a unique data-dependent key to avoid unnecessary rerenders + + ⚠ eslint-plugin-react(no-array-index-key): Usage of Array index in keys is not allowed + ╭─[no_array_index_key.tsx:2:37] + 1 │ things.every((thing, index) => { + 2 │ otherThings.push(); + · ─────────── + 3 │ }); + ╰──── + help: Use a unique data-dependent key to avoid unnecessary rerenders + + ⚠ eslint-plugin-react(no-array-index-key): Usage of Array index in keys is not allowed + ╭─[no_array_index_key.tsx:2:37] + 1 │ things.find((thing, index) => { + 2 │ otherThings.push(); + · ─────────── + 3 │ }); + ╰──── + help: Use a unique data-dependent key to avoid unnecessary rerenders + + ⚠ eslint-plugin-react(no-array-index-key): Usage of Array index in keys is not allowed + ╭─[no_array_index_key.tsx:2:37] + 1 │ things.findIndex((thing, index) => { + 2 │ otherThings.push(); + · ─────────── + 3 │ }); + ╰──── + help: Use a unique data-dependent key to avoid unnecessary rerenders + + ⚠ eslint-plugin-react(no-array-index-key): Usage of Array index in keys is not allowed + ╭─[no_array_index_key.tsx:2:20] + 1 │ things.flatMap((thing, index) => ( + 2 │ + · ─────────── + 3 │ )); + ╰──── + help: Use a unique data-dependent key to avoid unnecessary rerenders + + ⚠ eslint-plugin-react(no-array-index-key): Usage of Array index in keys is not allowed + ╭─[no_array_index_key.tsx:2:38] + 1 │ things.reduce((collection, thing, index) => ( + 2 │ collection.concat() + · ─────────── + 3 │ ), []); + ╰──── + help: Use a unique data-dependent key to avoid unnecessary rerenders + + ⚠ eslint-plugin-react(no-array-index-key): Usage of Array index in keys is not allowed + ╭─[no_array_index_key.tsx:2:38] + 1 │ things.reduceRight((collection, thing, index) => ( + 2 │ collection.concat() + · ─────────── + 3 │ ), []); + ╰──── + help: Use a unique data-dependent key to avoid unnecessary rerenders