From 68efcd40001f5ab048a25ed03ca14199bd8450f3 Mon Sep 17 00:00:00 2001 From: DonIsaac <22823424+DonIsaac@users.noreply.github.com> Date: Tue, 23 Jul 2024 01:52:59 +0000 Subject: [PATCH] feat(linter/react-perf): handle new objects and arrays in prop assignment patterns (#4396) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit # What This PR Does Massively improves all `react-perf` rules - feat: handle new objects/etc assigned to variables ```tsx const Foo = () => { const x = { foo: 'bar' } // <- now reports this new object return } ``` - feat: handle new objects/etc in binding patterns ```tsx const Foo = ({ x = [] }) => { // ^^^^^^ now reports this new array return } ``` -feat: nice and descriptive labels for new objects/etc assigned to intermediate variables ``` ⚠ eslint-plugin-react-perf(jsx-no-new-object-as-prop): JSX attribute values should not contain objects created in the same scope. ╭─[jsx_no_new_object_as_prop.tsx:1:27] 1 │ const Foo = () => { const x = {}; return } · ┬ ─┬ ┬ · │ │ ╰── And used here · │ ╰── And assigned a new value here · ╰── The prop was declared here ╰──── help: simplify props or memoize props in the parent component (https://react.dev/reference/react/memo#my-component-rerenders-when-a-prop-is-an-object-or-array). ``` - feat: consider `Object.assign()` and `Object.create()` as a new object - feat: consider `arr.[map, filter, concat]` as a new array - refactor: move shared implementation code to `ReactPerfRule` in `oxc_linter::utils::react_perf` --- crates/oxc_ast/src/ast_impl/js.rs | 5 + .../rules/react_perf/jsx_no_jsx_as_prop.rs | 57 ++---- .../react_perf/jsx_no_new_array_as_prop.rs | 85 ++++---- .../react_perf/jsx_no_new_function_as_prop.rs | 111 ++++++---- .../react_perf/jsx_no_new_object_as_prop.rs | 105 ++++++---- .../src/snapshots/jsx_no_jsx_as_prop.snap | 10 + .../snapshots/jsx_no_new_array_as_prop.snap | 41 ++++ .../jsx_no_new_function_as_prop.snap | 58 ++++++ .../snapshots/jsx_no_new_object_as_prop.snap | 34 ++++ crates/oxc_linter/src/utils/react_perf.rs | 191 +++++++++++++++++- 10 files changed, 538 insertions(+), 159 deletions(-) diff --git a/crates/oxc_ast/src/ast_impl/js.rs b/crates/oxc_ast/src/ast_impl/js.rs index 5017ec8c13e84..25224e65ca9ce 100644 --- a/crates/oxc_ast/src/ast_impl/js.rs +++ b/crates/oxc_ast/src/ast_impl/js.rs @@ -304,6 +304,11 @@ impl<'a> IdentifierReference<'a> { reference_flag: ReferenceFlag::Read, } } + + #[inline] + pub fn reference_id(&self) -> Option { + self.reference_id.get() + } } impl<'a> Hash for BindingIdentifier<'a> { diff --git a/crates/oxc_linter/src/rules/react_perf/jsx_no_jsx_as_prop.rs b/crates/oxc_linter/src/rules/react_perf/jsx_no_jsx_as_prop.rs index 6eb43a7a4099b..968a4efc243b9 100644 --- a/crates/oxc_linter/src/rules/react_perf/jsx_no_jsx_as_prop.rs +++ b/crates/oxc_linter/src/rules/react_perf/jsx_no_jsx_as_prop.rs @@ -1,18 +1,8 @@ -use oxc_ast::{ - ast::{Expression, JSXAttributeValue, JSXElement}, - AstKind, -}; -use oxc_diagnostics::OxcDiagnostic; +use crate::utils::ReactPerfRule; +use oxc_ast::{ast::Expression, AstKind}; use oxc_macros::declare_oxc_lint; -use oxc_span::Span; - -use crate::{context::LintContext, rule::Rule, utils::get_prop_value, AstNode}; - -fn jsx_no_jsx_as_prop_diagnostic(span0: Span) -> OxcDiagnostic { - OxcDiagnostic::warn("JSX attribute values should not contain other JSX.") - .with_help(r"simplify props or memoize props in the parent component (https://react.dev/reference/react/memo#my-component-rerenders-when-a-prop-is-an-object-or-array).") - .with_label(span0) -} +use oxc_semantic::SymbolId; +use oxc_span::{GetSpan, Span}; #[derive(Debug, Default, Clone)] pub struct JsxNoJsxAsProp; @@ -36,34 +26,23 @@ declare_oxc_lint!( perf ); -impl Rule for JsxNoJsxAsProp { - fn run<'a>(&self, node: &AstNode<'a>, ctx: &LintContext<'a>) { - if node.scope_id() == ctx.scopes().root_scope_id() { - return; - } - if let AstKind::JSXElement(jsx_elem) = node.kind() { - check_jsx_element(jsx_elem, ctx); - } - } +impl ReactPerfRule for JsxNoJsxAsProp { + const MESSAGE: &'static str = "JSX attribute values should not contain other JSX."; - fn should_run(&self, ctx: &LintContext) -> bool { - ctx.source_type().is_jsx() + fn check_for_violation_on_expr(&self, expr: &Expression<'_>) -> Option { + check_expression(expr) } -} -fn check_jsx_element<'a>(jsx_elem: &JSXElement<'a>, ctx: &LintContext<'a>) { - for item in &jsx_elem.opening_element.attributes { - match get_prop_value(item) { - None => return, - Some(JSXAttributeValue::ExpressionContainer(container)) => { - if let Some(expr) = container.expression.as_expression() { - if let Some(span) = check_expression(expr) { - ctx.diagnostic(jsx_no_jsx_as_prop_diagnostic(span)); - } - } - } - _ => {} + fn check_for_violation_on_ast_kind( + &self, + kind: &AstKind<'_>, + _symbol_id: SymbolId, + ) -> Option<(/* decl */ Span, /* init */ Option)> { + let AstKind::VariableDeclarator(decl) = kind else { + return None; }; + let init_span = decl.init.as_ref().and_then(check_expression)?; + Some((decl.id.span(), Some(init_span))) } } @@ -91,6 +70,7 @@ fn test() { r"} />", r"} />", r")} />", + r"const Icon = ; const Foo = () => ()", ]; let fail = vec![ @@ -98,6 +78,7 @@ fn test() { r"const Foo = () => (} />)", r"const Foo = () => (} />)", r"const Foo = () => ()} />)", + r"const Foo = () => { const Icon = ; return () }", ]; Tester::new(JsxNoJsxAsProp::NAME, pass, fail).with_react_perf_plugin(true).test_and_snapshot(); diff --git a/crates/oxc_linter/src/rules/react_perf/jsx_no_new_array_as_prop.rs b/crates/oxc_linter/src/rules/react_perf/jsx_no_new_array_as_prop.rs index bccfc9e9a8a6d..2b1c57a31e479 100644 --- a/crates/oxc_linter/src/rules/react_perf/jsx_no_new_array_as_prop.rs +++ b/crates/oxc_linter/src/rules/react_perf/jsx_no_new_array_as_prop.rs @@ -1,24 +1,13 @@ -use oxc_ast::{ - ast::{Expression, JSXAttributeValue, JSXElement}, - AstKind, -}; -use oxc_diagnostics::OxcDiagnostic; +use oxc_ast::{ast::Expression, AstKind}; use oxc_macros::declare_oxc_lint; -use oxc_span::Span; +use oxc_semantic::SymbolId; +use oxc_span::{GetSpan, Span}; use crate::{ - context::LintContext, - rule::Rule, - utils::{get_prop_value, is_constructor_matching_name}, - AstNode, + ast_util::is_method_call, + utils::{find_initialized_binding, is_constructor_matching_name, ReactPerfRule}, }; -fn jsx_no_new_array_as_prop_diagnostic(span0: Span) -> OxcDiagnostic { - OxcDiagnostic::warn("JSX attribute values should not contain Arrays created in the same scope.") - .with_help(r"simplify props or memoize props in the parent component (https://react.dev/reference/react/memo#my-component-rerenders-when-a-prop-is-an-object-or-array).") - .with_label(span0) -} - #[derive(Debug, Default, Clone)] pub struct JsxNoNewArrayAsProp; @@ -44,34 +33,33 @@ declare_oxc_lint!( perf ); -impl Rule for JsxNoNewArrayAsProp { - fn run<'a>(&self, node: &AstNode<'a>, ctx: &LintContext<'a>) { - if node.scope_id() == ctx.scopes().root_scope_id() { - return; - } - if let AstKind::JSXElement(jsx_elem) = node.kind() { - check_jsx_element(jsx_elem, ctx); - } - } +impl ReactPerfRule for JsxNoNewArrayAsProp { + const MESSAGE: &'static str = + "JSX attribute values should not contain Arrays created in the same scope."; - fn should_run(&self, ctx: &LintContext) -> bool { - ctx.source_type().is_jsx() + fn check_for_violation_on_expr(&self, expr: &Expression<'_>) -> Option { + check_expression(expr) } -} -fn check_jsx_element<'a>(jsx_elem: &JSXElement<'a>, ctx: &LintContext<'a>) { - for item in &jsx_elem.opening_element.attributes { - match get_prop_value(item) { - None => return, - Some(JSXAttributeValue::ExpressionContainer(container)) => { - if let Some(expr) = container.expression.as_expression() { - if let Some(span) = check_expression(expr) { - ctx.diagnostic(jsx_no_new_array_as_prop_diagnostic(span)); - } + fn check_for_violation_on_ast_kind( + &self, + kind: &AstKind<'_>, + symbol_id: SymbolId, + ) -> Option<(/* decl */ Span, /* init */ Option)> { + match kind { + AstKind::VariableDeclarator(decl) => { + if let Some(init_span) = decl.init.as_ref().and_then(check_expression) { + return Some((decl.id.span(), Some(init_span))); } + None + } + AstKind::FormalParameter(param) => { + let (id, init) = find_initialized_binding(¶m.pattern, symbol_id)?; + let init_span = check_expression(init)?; + Some((id.span(), Some(init_span))) } - _ => {} - }; + _ => None, + } } } @@ -79,7 +67,15 @@ fn check_expression(expr: &Expression) -> Option { match expr.without_parenthesized() { Expression::ArrayExpression(expr) => Some(expr.span), Expression::CallExpression(expr) => { - if is_constructor_matching_name(&expr.callee, "Array") { + if is_constructor_matching_name(&expr.callee, "Array") + || is_method_call( + expr.as_ref(), + None, + Some(&["concat", "map", "filter"]), + Some(1), + Some(1), + ) + { Some(expr.span) } else { None @@ -108,22 +104,29 @@ fn test() { let pass = vec![ r"", - r"const Foo = () => ", r"", r"", r"", r"", r"", r"", + r"const Foo = () => ", + r"const x = []; const Foo = () => ", + r"const DEFAULT_X = []; const Foo = ({ x = DEFAULT_X }) => ", ]; let fail = vec![ r"const Foo = () => ()", r"const Foo = () => ()", r"const Foo = () => ()", + r"const Foo = () => ()", + r"const Foo = () => ( x > 0)} />)", + r"const Foo = () => ( x * x)} />)", r"const Foo = () => ()", r"const Foo = () => ()", r"const Foo = () => ()", + r"const Foo = () => { let x = []; return }", + r"const Foo = ({ x = [] }) => ", ]; Tester::new(JsxNoNewArrayAsProp::NAME, pass, fail) diff --git a/crates/oxc_linter/src/rules/react_perf/jsx_no_new_function_as_prop.rs b/crates/oxc_linter/src/rules/react_perf/jsx_no_new_function_as_prop.rs index ce86756e1fa63..2ccd8c755618b 100644 --- a/crates/oxc_linter/src/rules/react_perf/jsx_no_new_function_as_prop.rs +++ b/crates/oxc_linter/src/rules/react_perf/jsx_no_new_function_as_prop.rs @@ -1,23 +1,12 @@ use oxc_ast::{ - ast::{Expression, JSXAttributeValue, JSXElement, MemberExpression}, + ast::{Expression, MemberExpression}, AstKind, }; -use oxc_diagnostics::OxcDiagnostic; use oxc_macros::declare_oxc_lint; -use oxc_span::Span; +use oxc_semantic::SymbolId; +use oxc_span::{GetSpan, Span}; -use crate::{ - context::LintContext, - rule::Rule, - utils::{get_prop_value, is_constructor_matching_name}, - AstNode, -}; - -fn jsx_no_new_function_as_prop_diagnostic(span0: Span) -> OxcDiagnostic { - OxcDiagnostic::warn("JSX attribute values should not contain functions created in the same scope.") - .with_help(r"simplify props or memoize props in the parent component (https://react.dev/reference/react/memo#my-component-rerenders-when-a-prop-is-an-object-or-array).") - .with_label(span0) -} +use crate::utils::{is_constructor_matching_name, ReactPerfRule}; #[derive(Debug, Default, Clone)] pub struct JsxNoNewFunctionAsProp; @@ -39,34 +28,31 @@ declare_oxc_lint!( perf ); -impl Rule for JsxNoNewFunctionAsProp { - fn run<'a>(&self, node: &AstNode<'a>, ctx: &LintContext<'a>) { - if node.scope_id() == ctx.scopes().root_scope_id() { - return; - } - if let AstKind::JSXElement(jsx_elem) = node.kind() { - check_jsx_element(jsx_elem, ctx); - } - } +impl ReactPerfRule for JsxNoNewFunctionAsProp { + const MESSAGE: &'static str = + "JSX attribute values should not contain functions created in the same scope."; - fn should_run(&self, ctx: &LintContext) -> bool { - ctx.source_type().is_jsx() + fn check_for_violation_on_expr(&self, expr: &Expression<'_>) -> Option { + check_expression(expr) } -} -fn check_jsx_element<'a>(jsx_elem: &JSXElement<'a>, ctx: &LintContext<'a>) { - for item in &jsx_elem.opening_element.attributes { - match get_prop_value(item) { - None => return, - Some(JSXAttributeValue::ExpressionContainer(container)) => { - if let Some(expr) = container.expression.as_expression() { - if let Some(span) = check_expression(expr) { - ctx.diagnostic(jsx_no_new_function_as_prop_diagnostic(span)); - } - } + fn check_for_violation_on_ast_kind( + &self, + kind: &AstKind<'_>, + _symbol_id: SymbolId, + ) -> Option<(/* decl */ Span, /* init */ Option)> { + match kind { + AstKind::VariableDeclarator(decl) + if decl.init.as_ref().and_then(check_expression).is_some() => + { + // don't report init span, b/c thats usually an arrow + // function expression which gets quite large. It also + // doesn't add any value. + Some((decl.id.span(), None)) } - _ => {} - }; + AstKind::Function(f) => Some((f.id.as_ref().map_or(f.span, GetSpan::span), None)), + _ => None, + } } } @@ -131,6 +117,24 @@ fn test() { r"", r"", r"", + r" + import { FC, useCallback } from 'react'; + export const Foo: FC = props => { + const onClick = useCallback( + e => { props.onClick?.(e) }, + [props.onClick] + ); + return