Skip to content

Commit f04b1d2

Browse files
DonIsaacDonIsaac
committed
feat(linter/react-perf): handle new objects and arrays in prop assignment patterns
1 parent fb28ae4 commit f04b1d2

File tree

10 files changed

+538
-159
lines changed

10 files changed

+538
-159
lines changed

‎crates/oxc_ast/src/ast_impl/js.rs‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -304,6 +304,11 @@ impl<'a> IdentifierReference<'a> {
304304
reference_flag: ReferenceFlag::Read,
305305
}
306306
}
307+
308+
#[inline]
309+
pub fn reference_id(&self) -> Option<ReferenceId> {
310+
self.reference_id.get()
311+
}
307312
}
308313

309314
impl<'a> Hash for BindingIdentifier<'a> {

‎crates/oxc_linter/src/rules/react_perf/jsx_no_jsx_as_prop.rs‎

Lines changed: 19 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -1,18 +1,8 @@
1-
use oxc_ast::{
2-
ast::{Expression, JSXAttributeValue, JSXElement},
3-
AstKind,
4-
};
5-
use oxc_diagnostics::OxcDiagnostic;
1+
use crate::utils::ReactPerfRule;
2+
use oxc_ast::{ast::Expression, AstKind};
63
use oxc_macros::declare_oxc_lint;
7-
use oxc_span::Span;
8-
9-
use crate::{context::LintContext, rule::Rule, utils::get_prop_value, AstNode};
10-
11-
fn jsx_no_jsx_as_prop_diagnostic(span0: Span) -> OxcDiagnostic {
12-
OxcDiagnostic::warn("JSX attribute values should not contain other JSX.")
13-
.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).")
14-
.with_label(span0)
15-
}
4+
use oxc_semantic::SymbolId;
5+
use oxc_span::{GetSpan, Span};
166

177
#[derive(Debug, Default, Clone)]
188
pub struct JsxNoJsxAsProp;
@@ -36,34 +26,23 @@ declare_oxc_lint!(
3626
perf
3727
);
3828

39-
impl Rule for JsxNoJsxAsProp {
40-
fn run<'a>(&self, node: &AstNode<'a>, ctx: &LintContext<'a>) {
41-
if node.scope_id() == ctx.scopes().root_scope_id() {
42-
return;
43-
}
44-
if let AstKind::JSXElement(jsx_elem) = node.kind() {
45-
check_jsx_element(jsx_elem, ctx);
46-
}
47-
}
29+
impl ReactPerfRule for JsxNoJsxAsProp {
30+
const MESSAGE: &'static str = "JSX attribute values should not contain other JSX.";
4831

49-
fn should_run(&self, ctx: &LintContext) -> bool {
50-
ctx.source_type().is_jsx()
32+
fn check_for_violation_on_expr(&self, expr: &Expression<'_>) -> Option<Span> {
33+
check_expression(expr)
5134
}
52-
}
5335

54-
fn check_jsx_element<'a>(jsx_elem: &JSXElement<'a>, ctx: &LintContext<'a>) {
55-
for item in &jsx_elem.opening_element.attributes {
56-
match get_prop_value(item) {
57-
None => return,
58-
Some(JSXAttributeValue::ExpressionContainer(container)) => {
59-
if let Some(expr) = container.expression.as_expression() {
60-
if let Some(span) = check_expression(expr) {
61-
ctx.diagnostic(jsx_no_jsx_as_prop_diagnostic(span));
62-
}
63-
}
64-
}
65-
_ => {}
36+
fn check_for_violation_on_ast_kind(
37+
&self,
38+
kind: &AstKind<'_>,
39+
_symbol_id: SymbolId,
40+
) -> Option<(/* decl */ Span, /* init */ Option<Span>)> {
41+
let AstKind::VariableDeclarator(decl) = kind else {
42+
return None;
6643
};
44+
let init_span = decl.init.as_ref().and_then(check_expression)?;
45+
Some((decl.id.span(), Some(init_span)))
6746
}
6847
}
6948

@@ -91,13 +70,15 @@ fn test() {
9170
r"<Item jsx={this.props.jsx || <SubItem />} />",
9271
r"<Item jsx={this.props.jsx ? this.props.jsx : <SubItem />} />",
9372
r"<Item jsx={this.props.jsx || (this.props.component ? this.props.component : <SubItem />)} />",
73+
r"const Icon = <svg />; const Foo = () => (<IconButton icon={Icon} />)",
9474
];
9575

9676
let fail = vec![
9777
r"const Foo = () => (<Item jsx={<SubItem />} />)",
9878
r"const Foo = () => (<Item jsx={this.props.jsx || <SubItem />} />)",
9979
r"const Foo = () => (<Item jsx={this.props.jsx ? this.props.jsx : <SubItem />} />)",
10080
r"const Foo = () => (<Item jsx={this.props.jsx || (this.props.component ? this.props.component : <SubItem />)} />)",
81+
r"const Foo = () => { const Icon = <svg />; return (<IconButton icon={Icon} />) }",
10182
];
10283

10384
Tester::new(JsxNoJsxAsProp::NAME, pass, fail).with_react_perf_plugin(true).test_and_snapshot();

‎crates/oxc_linter/src/rules/react_perf/jsx_no_new_array_as_prop.rs‎

Lines changed: 44 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -1,24 +1,13 @@
1-
use oxc_ast::{
2-
ast::{Expression, JSXAttributeValue, JSXElement},
3-
AstKind,
4-
};
5-
use oxc_diagnostics::OxcDiagnostic;
1+
use oxc_ast::{ast::Expression, AstKind};
62
use oxc_macros::declare_oxc_lint;
7-
use oxc_span::Span;
3+
use oxc_semantic::SymbolId;
4+
use oxc_span::{GetSpan, Span};
85

96
use crate::{
10-
context::LintContext,
11-
rule::Rule,
12-
utils::{get_prop_value, is_constructor_matching_name},
13-
AstNode,
7+
ast_util::is_method_call,
8+
utils::{find_initialized_binding, is_constructor_matching_name, ReactPerfRule},
149
};
1510

16-
fn jsx_no_new_array_as_prop_diagnostic(span0: Span) -> OxcDiagnostic {
17-
OxcDiagnostic::warn("JSX attribute values should not contain Arrays created in the same scope.")
18-
.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).")
19-
.with_label(span0)
20-
}
21-
2211
#[derive(Debug, Default, Clone)]
2312
pub struct JsxNoNewArrayAsProp;
2413

@@ -44,42 +33,49 @@ declare_oxc_lint!(
4433
perf
4534
);
4635

47-
impl Rule for JsxNoNewArrayAsProp {
48-
fn run<'a>(&self, node: &AstNode<'a>, ctx: &LintContext<'a>) {
49-
if node.scope_id() == ctx.scopes().root_scope_id() {
50-
return;
51-
}
52-
if let AstKind::JSXElement(jsx_elem) = node.kind() {
53-
check_jsx_element(jsx_elem, ctx);
54-
}
55-
}
36+
impl ReactPerfRule for JsxNoNewArrayAsProp {
37+
const MESSAGE: &'static str =
38+
"JSX attribute values should not contain Arrays created in the same scope.";
5639

57-
fn should_run(&self, ctx: &LintContext) -> bool {
58-
ctx.source_type().is_jsx()
40+
fn check_for_violation_on_expr(&self, expr: &Expression<'_>) -> Option<Span> {
41+
check_expression(expr)
5942
}
60-
}
6143

62-
fn check_jsx_element<'a>(jsx_elem: &JSXElement<'a>, ctx: &LintContext<'a>) {
63-
for item in &jsx_elem.opening_element.attributes {
64-
match get_prop_value(item) {
65-
None => return,
66-
Some(JSXAttributeValue::ExpressionContainer(container)) => {
67-
if let Some(expr) = container.expression.as_expression() {
68-
if let Some(span) = check_expression(expr) {
69-
ctx.diagnostic(jsx_no_new_array_as_prop_diagnostic(span));
70-
}
44+
fn check_for_violation_on_ast_kind(
45+
&self,
46+
kind: &AstKind<'_>,
47+
symbol_id: SymbolId,
48+
) -> Option<(/* decl */ Span, /* init */ Option<Span>)> {
49+
match kind {
50+
AstKind::VariableDeclarator(decl) => {
51+
if let Some(init_span) = decl.init.as_ref().and_then(check_expression) {
52+
return Some((decl.id.span(), Some(init_span)));
7153
}
54+
None
55+
}
56+
AstKind::FormalParameter(param) => {
57+
let (id, init) = find_initialized_binding(&param.pattern, symbol_id)?;
58+
let init_span = check_expression(init)?;
59+
Some((id.span(), Some(init_span)))
7260
}
73-
_ => {}
74-
};
61+
_ => None,
62+
}
7563
}
7664
}
7765

7866
fn check_expression(expr: &Expression) -> Option<Span> {
7967
match expr.without_parenthesized() {
8068
Expression::ArrayExpression(expr) => Some(expr.span),
8169
Expression::CallExpression(expr) => {
82-
if is_constructor_matching_name(&expr.callee, "Array") {
70+
if is_constructor_matching_name(&expr.callee, "Array")
71+
|| is_method_call(
72+
expr.as_ref(),
73+
None,
74+
Some(&["concat", "map", "filter"]),
75+
Some(1),
76+
Some(1),
77+
)
78+
{
8379
Some(expr.span)
8480
} else {
8581
None
@@ -108,22 +104,29 @@ fn test() {
108104

109105
let pass = vec![
110106
r"<Item list={this.props.list} />",
111-
r"const Foo = () => <Item list={this.props.list} />",
112107
r"<Item list={[]} />",
113108
r"<Item list={new Array()} />",
114109
r"<Item list={Array()} />",
115110
r"<Item list={this.props.list || []} />",
116111
r"<Item list={this.props.list ? this.props.list : []} />",
117112
r"<Item list={this.props.list || (this.props.arr ? this.props.arr : [])} />",
113+
r"const Foo = () => <Item list={this.props.list} />",
114+
r"const x = []; const Foo = () => <Item list={x} />",
115+
r"const DEFAULT_X = []; const Foo = ({ x = DEFAULT_X }) => <Item list={x} />",
118116
];
119117

120118
let fail = vec![
121119
r"const Foo = () => (<Item list={[]} />)",
122120
r"const Foo = () => (<Item list={new Array()} />)",
123121
r"const Foo = () => (<Item list={Array()} />)",
122+
r"const Foo = () => (<Item list={arr1.concat(arr2)} />)",
123+
r"const Foo = () => (<Item list={arr1.filter(x => x > 0)} />)",
124+
r"const Foo = () => (<Item list={arr1.map(x => x * x)} />)",
124125
r"const Foo = () => (<Item list={this.props.list || []} />)",
125126
r"const Foo = () => (<Item list={this.props.list ? this.props.list : []} />)",
126127
r"const Foo = () => (<Item list={this.props.list || (this.props.arr ? this.props.arr : [])} />)",
128+
r"const Foo = () => { let x = []; return <Item list={x} /> }",
129+
r"const Foo = ({ x = [] }) => <Item list={x} />",
127130
];
128131

129132
Tester::new(JsxNoNewArrayAsProp::NAME, pass, fail)

‎crates/oxc_linter/src/rules/react_perf/jsx_no_new_function_as_prop.rs‎

Lines changed: 72 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -1,23 +1,12 @@
11
use oxc_ast::{
2-
ast::{Expression, JSXAttributeValue, JSXElement, MemberExpression},
2+
ast::{Expression, MemberExpression},
33
AstKind,
44
};
5-
use oxc_diagnostics::OxcDiagnostic;
65
use oxc_macros::declare_oxc_lint;
7-
use oxc_span::Span;
6+
use oxc_semantic::SymbolId;
7+
use oxc_span::{GetSpan, Span};
88

9-
use crate::{
10-
context::LintContext,
11-
rule::Rule,
12-
utils::{get_prop_value, is_constructor_matching_name},
13-
AstNode,
14-
};
15-
16-
fn jsx_no_new_function_as_prop_diagnostic(span0: Span) -> OxcDiagnostic {
17-
OxcDiagnostic::warn("JSX attribute values should not contain functions created in the same scope.")
18-
.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).")
19-
.with_label(span0)
20-
}
9+
use crate::utils::{is_constructor_matching_name, ReactPerfRule};
2110

2211
#[derive(Debug, Default, Clone)]
2312
pub struct JsxNoNewFunctionAsProp;
@@ -39,34 +28,31 @@ declare_oxc_lint!(
3928
perf
4029
);
4130

42-
impl Rule for JsxNoNewFunctionAsProp {
43-
fn run<'a>(&self, node: &AstNode<'a>, ctx: &LintContext<'a>) {
44-
if node.scope_id() == ctx.scopes().root_scope_id() {
45-
return;
46-
}
47-
if let AstKind::JSXElement(jsx_elem) = node.kind() {
48-
check_jsx_element(jsx_elem, ctx);
49-
}
50-
}
31+
impl ReactPerfRule for JsxNoNewFunctionAsProp {
32+
const MESSAGE: &'static str =
33+
"JSX attribute values should not contain functions created in the same scope.";
5134

52-
fn should_run(&self, ctx: &LintContext) -> bool {
53-
ctx.source_type().is_jsx()
35+
fn check_for_violation_on_expr(&self, expr: &Expression<'_>) -> Option<Span> {
36+
check_expression(expr)
5437
}
55-
}
5638

57-
fn check_jsx_element<'a>(jsx_elem: &JSXElement<'a>, ctx: &LintContext<'a>) {
58-
for item in &jsx_elem.opening_element.attributes {
59-
match get_prop_value(item) {
60-
None => return,
61-
Some(JSXAttributeValue::ExpressionContainer(container)) => {
62-
if let Some(expr) = container.expression.as_expression() {
63-
if let Some(span) = check_expression(expr) {
64-
ctx.diagnostic(jsx_no_new_function_as_prop_diagnostic(span));
65-
}
66-
}
39+
fn check_for_violation_on_ast_kind(
40+
&self,
41+
kind: &AstKind<'_>,
42+
_symbol_id: SymbolId,
43+
) -> Option<(/* decl */ Span, /* init */ Option<Span>)> {
44+
match kind {
45+
AstKind::VariableDeclarator(decl)
46+
if decl.init.as_ref().and_then(check_expression).is_some() =>
47+
{
48+
// don't report init span, b/c thats usually an arrow
49+
// function expression which gets quite large. It also
50+
// doesn't add any value.
51+
Some((decl.id.span(), None))
6752
}
68-
_ => {}
69-
};
53+
AstKind::Function(f) => Some((f.id.as_ref().map_or(f.span, GetSpan::span), None)),
54+
_ => None,
55+
}
7056
}
7157
}
7258

@@ -131,6 +117,24 @@ fn test() {
131117
r"<Item callback={this.props.callback ? this.props.callback : function() {}} />",
132118
r"<Item prop={this.props.callback || this.props.callback ? this.props.callback : function(){}} />",
133119
r"<Item prop={this.props.callback || (this.props.cb ? this.props.cb : function(){})} />",
120+
r"
121+
import { FC, useCallback } from 'react';
122+
export const Foo: FC = props => {
123+
const onClick = useCallback(
124+
e => { props.onClick?.(e) },
125+
[props.onClick]
126+
);
127+
return <button onClick={onClick} />
128+
}",
129+
r"
130+
import React from 'react'
131+
function onClick(e: React.MouseEvent) {
132+
window.location.navigate(e.target.href)
133+
}
134+
export default function Foo() {
135+
return <a onClick={onClick} />
136+
}
137+
",
134138
];
135139

136140
let fail = vec![
@@ -143,6 +147,35 @@ fn test() {
143147
r"const Foo = () => (<Item callback={this.props.callback ? this.props.callback : function() {}} />)",
144148
r"const Foo = () => (<Item prop={this.props.callback || this.props.callback ? this.props.callback : function(){}} />)",
145149
r"const Foo = () => (<Item prop={this.props.callback || (this.props.cb ? this.props.cb : function(){})} />)",
150+
r"
151+
const Foo = ({ onClick }) => {
152+
const _onClick = onClick.bind(this)
153+
return <button onClick={_onClick} />
154+
}",
155+
r"
156+
const Foo = () => {
157+
function onClick(e) {
158+
window.location.navigate(e.target.href)
159+
}
160+
return <a onClick={onClick} />
161+
}
162+
",
163+
r"
164+
const Foo = () => {
165+
const onClick = (e) => {
166+
window.location.navigate(e.target.href)
167+
}
168+
return <a onClick={onClick} />
169+
}
170+
",
171+
r"
172+
const Foo = () => {
173+
const onClick = function (e) {
174+
window.location.navigate(e.target.href)
175+
}
176+
return <a onClick={onClick} />
177+
}
178+
",
146179
];
147180

148181
Tester::new(JsxNoNewFunctionAsProp::NAME, pass, fail)

0 commit comments

Comments
 (0)