mirror of
https://github.com/danbulant/oxc
synced 2026-05-19 12:19:15 +00:00
feat(linter): add jsx support for only-used-in-recursion (#7120)
close #5530 This PR adds support for handling recursive JSX components, allowing detect functions like the one below: ```javascript function ListItem({ depth }) { return <ListItem depth={depth} /> } ``` Some logic is duplicated with the non-JSX case. But the refactor will be complicated and affects over-all, so we can make it in another PR. cc: @DonIsaac @camc314 --------- Co-authored-by: Cam McHenry <camchenry@users.noreply.github.com> Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
This commit is contained in:
no-yan
GitHub
parent
4d577cf69f
commit
1fcd70932d
2 changed files with 459 additions and 26 deletions
|
|
@ -1,10 +1,13 @@
|
|||
use oxc_ast::{
|
||||
ast::{BindingIdentifier, BindingPatternKind, CallExpression, Expression, FormalParameters},
|
||||
ast::{
|
||||
BindingIdentifier, BindingPatternKind, BindingProperty, CallExpression, Expression,
|
||||
FormalParameters, JSXAttributeItem, JSXElementName,
|
||||
},
|
||||
AstKind,
|
||||
};
|
||||
use oxc_diagnostics::OxcDiagnostic;
|
||||
use oxc_macros::declare_oxc_lint;
|
||||
use oxc_semantic::SymbolId;
|
||||
use oxc_semantic::{NodeId, SymbolId};
|
||||
use oxc_span::{GetSpan, Span};
|
||||
|
||||
use crate::{
|
||||
|
|
@ -92,19 +95,34 @@ impl Rule for OnlyUsedInRecursion {
|
|||
}
|
||||
|
||||
for (arg_index, formal_parameter) in function_parameters.items.iter().enumerate() {
|
||||
let BindingPatternKind::BindingIdentifier(arg) = &formal_parameter.pattern.kind else {
|
||||
continue;
|
||||
};
|
||||
match &formal_parameter.pattern.kind {
|
||||
BindingPatternKind::BindingIdentifier(arg) => {
|
||||
if is_argument_only_used_in_recursion(function_id, arg, arg_index, ctx) {
|
||||
create_diagnostic(
|
||||
ctx,
|
||||
function_id,
|
||||
function_parameters,
|
||||
arg,
|
||||
arg_index,
|
||||
function_span,
|
||||
);
|
||||
}
|
||||
}
|
||||
BindingPatternKind::ObjectPattern(pattern) => {
|
||||
for property in &pattern.properties {
|
||||
let Some(ident) = property.value.get_binding_identifier() else {
|
||||
continue;
|
||||
};
|
||||
|
||||
if is_argument_only_used_in_recursion(function_id, arg, arg_index, ctx) {
|
||||
create_diagnostic(
|
||||
ctx,
|
||||
function_id,
|
||||
function_parameters,
|
||||
arg,
|
||||
arg_index,
|
||||
function_span,
|
||||
);
|
||||
let Some(name) = property.key.name() else {
|
||||
continue;
|
||||
};
|
||||
if is_property_only_used_in_recursion_jsx(ident, &name, function_id, ctx) {
|
||||
create_diagnostic_jsx(ctx, function_id, property);
|
||||
}
|
||||
}
|
||||
}
|
||||
_ => continue,
|
||||
}
|
||||
}
|
||||
}
|
||||
|
|
@ -168,7 +186,12 @@ fn create_diagnostic(
|
|||
let arg_to_delete = call_expr.arguments[arg_index].span();
|
||||
fix.push(Fix::delete(Span::new(
|
||||
arg_to_delete.start,
|
||||
skip_to_next_char(ctx.source_text(), arg_to_delete.end),
|
||||
skip_to_next_char(
|
||||
ctx.source_text(),
|
||||
arg_to_delete.end,
|
||||
&Direction::Forward,
|
||||
)
|
||||
.unwrap_or(arg_to_delete.end),
|
||||
)));
|
||||
}
|
||||
}
|
||||
|
|
@ -178,6 +201,80 @@ fn create_diagnostic(
|
|||
);
|
||||
}
|
||||
|
||||
fn create_diagnostic_jsx(
|
||||
ctx: &LintContext,
|
||||
function_id: &BindingIdentifier,
|
||||
property: &BindingProperty,
|
||||
) {
|
||||
let Some(function_symbol_id) = function_id.symbol_id.get() else { return };
|
||||
let is_exported = ctx.semantic().symbols().get_flags(function_symbol_id).is_export();
|
||||
|
||||
let Some(property_name) = &property.key.static_name() else { return };
|
||||
if is_exported {
|
||||
return ctx.diagnostic(only_used_in_recursion_diagnostic(property.span(), property_name));
|
||||
}
|
||||
|
||||
let Some(property_ident) = property.value.get_binding_identifier() else { return };
|
||||
let Some(property_symbol_id) = property_ident.symbol_id.get() else { return };
|
||||
let mut references = ctx.semantic().symbol_references(property_symbol_id);
|
||||
|
||||
let has_spread_attribute = references.any(|x| used_with_spread_attribute(x.node_id(), ctx));
|
||||
|
||||
if has_spread_attribute {
|
||||
// If the JSXElement has a spread attribute, we cannot apply a fix safely,
|
||||
// as the same property name could be exist within the spread attribute.
|
||||
return ctx.diagnostic(only_used_in_recursion_diagnostic(property.span(), property_name));
|
||||
}
|
||||
|
||||
let Some(property_name) = property.key.static_name() else {
|
||||
return;
|
||||
};
|
||||
|
||||
ctx.diagnostic_with_dangerous_fix(
|
||||
only_used_in_recursion_diagnostic(property.span, &property_name),
|
||||
|fixer| {
|
||||
let mut fix = fixer.new_fix_with_capacity(references.count() + 1);
|
||||
|
||||
let source = ctx.source_text();
|
||||
let span_start = skip_to_next_char(source, property.span.start, &Direction::Backward)
|
||||
.unwrap_or(property.span.start);
|
||||
let span_end =
|
||||
skip_to_next_char(ctx.source_text(), property.span.end, &Direction::Forward)
|
||||
.unwrap_or(property.span.end);
|
||||
|
||||
fix.push(Fix::delete(Span::new(span_start, span_end)));
|
||||
|
||||
// search for references to the function and remove the property
|
||||
for reference in ctx.semantic().symbol_references(property_symbol_id) {
|
||||
let mut ancestor_ids = ctx.nodes().ancestor_ids(reference.node_id());
|
||||
|
||||
let Some(attr) =
|
||||
ancestor_ids.find_map(|node| match ctx.nodes().get_node(node).kind() {
|
||||
AstKind::JSXAttributeItem(attr) => Some(attr),
|
||||
_ => None,
|
||||
})
|
||||
else {
|
||||
continue;
|
||||
};
|
||||
|
||||
fix.push(Fix::delete(attr.span()));
|
||||
}
|
||||
|
||||
fix
|
||||
},
|
||||
);
|
||||
}
|
||||
|
||||
fn used_with_spread_attribute(node_id: NodeId, ctx: &LintContext) -> bool {
|
||||
ctx.nodes().ancestor_kinds(node_id).any(|kind| match kind {
|
||||
AstKind::JSXOpeningElement(opening_element) => opening_element
|
||||
.attributes
|
||||
.iter()
|
||||
.any(|attr| matches!(attr, JSXAttributeItem::SpreadAttribute(_))),
|
||||
_ => false,
|
||||
})
|
||||
}
|
||||
|
||||
fn is_argument_only_used_in_recursion<'a>(
|
||||
function_id: &'a BindingIdentifier,
|
||||
arg: &'a BindingIdentifier,
|
||||
|
|
@ -222,6 +319,81 @@ fn is_argument_only_used_in_recursion<'a>(
|
|||
true
|
||||
}
|
||||
|
||||
fn is_property_only_used_in_recursion_jsx(
|
||||
ident: &BindingIdentifier,
|
||||
property_name: &str,
|
||||
function_ident: &BindingIdentifier,
|
||||
ctx: &LintContext,
|
||||
) -> bool {
|
||||
let Some(ident_symbol_id) = ident.symbol_id.get() else { return false };
|
||||
let mut references = ctx.semantic().symbol_references(ident_symbol_id).peekable();
|
||||
|
||||
if references.peek().is_none() {
|
||||
return false;
|
||||
}
|
||||
|
||||
let Some(function_symbol_id) = function_ident.symbol_id.get() else { return false };
|
||||
|
||||
for reference in references {
|
||||
// Conditions:
|
||||
// 1. The reference is inside a JSXExpressionContainer.
|
||||
// 2. The JSXElement calls the recursive function itself.
|
||||
// 3. The reference is in a JSXAttribute, and the attribute name has the same name as the function.
|
||||
let Some(may_jsx_expr_container) = ctx.nodes().parent_node(reference.node_id()) else {
|
||||
return false;
|
||||
};
|
||||
let AstKind::JSXExpressionContainer(_) = may_jsx_expr_container.kind() else {
|
||||
// In this case, we simply ignore the references inside JSXExpressionContainer that are not single-node expression.
|
||||
// e.g. <Increment count={count+1} />
|
||||
//
|
||||
// To support this case, we need to check whether expression contains side-effect like ++val
|
||||
return false;
|
||||
};
|
||||
|
||||
let Some(attr) = ctx.nodes().ancestors(may_jsx_expr_container.id()).find_map(|node| {
|
||||
if let AstKind::JSXAttributeItem(attr) = node.kind() {
|
||||
Some(attr)
|
||||
} else {
|
||||
None
|
||||
}
|
||||
}) else {
|
||||
return false;
|
||||
};
|
||||
|
||||
let JSXAttributeItem::Attribute(jsx_attr_name) = attr else {
|
||||
return false;
|
||||
};
|
||||
let Some(attr_name) = jsx_attr_name.name.as_identifier() else {
|
||||
return false;
|
||||
};
|
||||
if attr_name.name != property_name {
|
||||
return false;
|
||||
}
|
||||
|
||||
let Some(opening_element) =
|
||||
ctx.nodes().ancestor_ids(reference.node_id()).find_map(|node| {
|
||||
if let AstKind::JSXOpeningElement(elem) = ctx.nodes().get_node(node).kind() {
|
||||
Some(elem)
|
||||
} else {
|
||||
None
|
||||
}
|
||||
})
|
||||
else {
|
||||
return false;
|
||||
};
|
||||
|
||||
let Some(jsx_ident_symbol_id) = get_jsx_element_symbol_id(&opening_element.name, ctx)
|
||||
else {
|
||||
return false;
|
||||
};
|
||||
if jsx_ident_symbol_id != function_symbol_id {
|
||||
return false;
|
||||
}
|
||||
}
|
||||
|
||||
true
|
||||
}
|
||||
|
||||
fn is_recursive_call(
|
||||
call_expr: &CallExpression,
|
||||
function_symbol_id: SymbolId,
|
||||
|
|
@ -250,16 +422,48 @@ fn is_function_maybe_reassigned<'a>(
|
|||
})
|
||||
}
|
||||
|
||||
// skipping whitespace, commas, finds the next character (exclusive)
|
||||
#[allow(clippy::cast_possible_truncation)]
|
||||
fn skip_to_next_char(s: &str, start: u32) -> u32 {
|
||||
for (i, c) in s.char_indices().skip(start as usize) {
|
||||
if !c.is_whitespace() && c != ',' {
|
||||
return i as u32;
|
||||
}
|
||||
}
|
||||
fn get_jsx_element_symbol_id<'a>(
|
||||
node: &'a JSXElementName<'a>,
|
||||
ctx: &'a LintContext<'_>,
|
||||
) -> Option<SymbolId> {
|
||||
let node = match node {
|
||||
JSXElementName::IdentifierReference(ident) => Some(ident.as_ref()),
|
||||
JSXElementName::MemberExpression(expr) => expr.get_identifier(),
|
||||
JSXElementName::Identifier(_)
|
||||
| JSXElementName::NamespacedName(_)
|
||||
| JSXElementName::ThisExpression(_) => None,
|
||||
}?;
|
||||
|
||||
s.len() as u32
|
||||
ctx.symbols().get_reference(node.reference_id()).symbol_id()
|
||||
}
|
||||
|
||||
enum Direction {
|
||||
Forward,
|
||||
Backward,
|
||||
}
|
||||
|
||||
// Skips whitespace and commas in a given direction and
|
||||
// returns the next character if found.
|
||||
#[allow(clippy::cast_possible_truncation)]
|
||||
fn skip_to_next_char(s: &str, start: u32, direction: &Direction) -> Option<u32> {
|
||||
// span is a half-open interval: [start, end)
|
||||
// so we should return in that way.
|
||||
let start = start as usize;
|
||||
match direction {
|
||||
Direction::Forward => s
|
||||
.char_indices()
|
||||
.skip(start)
|
||||
.find(|&(_, c)| !c.is_whitespace() && c != ',')
|
||||
.map(|(i, _)| i as u32),
|
||||
|
||||
Direction::Backward => s
|
||||
.char_indices()
|
||||
.rev()
|
||||
.skip(s.len() - start)
|
||||
.take_while(|&(_, c)| c.is_whitespace() || c == ',')
|
||||
.map(|(i, _)| i as u32)
|
||||
.last(),
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
|
|
@ -273,12 +477,23 @@ fn test() {
|
|||
// some code
|
||||
}
|
||||
",
|
||||
"
|
||||
function test(arg0) {
|
||||
test(arg0+1)
|
||||
}
|
||||
",
|
||||
// unused arg, no recursion
|
||||
"
|
||||
function test(arg0) {
|
||||
// arg0 not used
|
||||
}
|
||||
",
|
||||
// no recursion, assignment pattern
|
||||
r"
|
||||
function test({ arg0 = 10 }) {
|
||||
return arg0;
|
||||
}
|
||||
",
|
||||
"
|
||||
function test(arg0) {
|
||||
anotherTest(arg0);
|
||||
|
|
@ -326,13 +541,13 @@ fn test() {
|
|||
test(arg1, arg0)
|
||||
}
|
||||
",
|
||||
// Arguments Swapped in Recursion
|
||||
// arguments swapped in recursion
|
||||
r"
|
||||
function test(arg0, arg1) {
|
||||
test(arg1, arg0);
|
||||
}
|
||||
",
|
||||
// Arguments Swapped in Recursion (arrow)
|
||||
// arguments swapped in recursion (arrow)
|
||||
r"
|
||||
const test = (arg0, arg1) => {
|
||||
test(arg1, arg0);
|
||||
|
|
@ -366,6 +581,89 @@ fn test() {
|
|||
};
|
||||
validator()
|
||||
"#,
|
||||
// no params, no recursion
|
||||
"
|
||||
function Test() {
|
||||
return <Test1 />;
|
||||
}
|
||||
",
|
||||
// allowed case: The parameter 'depth' is used outside of the recursive call.
|
||||
// it is logged and reassigned, so the linter does not flag it as only used in recursion.
|
||||
"
|
||||
function Listitem({ depth }) {
|
||||
console.log(depth);
|
||||
depth = depth + 1;
|
||||
return <Listitem depth={depth}/>;
|
||||
}
|
||||
",
|
||||
"
|
||||
function Listitem({ depth }) {
|
||||
console.log(depth);
|
||||
return <Listitem depth={depth}/>;
|
||||
}
|
||||
",
|
||||
// allowed case
|
||||
// multi-node expressions are not supported for now
|
||||
"
|
||||
function Listitem({ depth }) {
|
||||
return <Listitem depth={depth + 1} />
|
||||
}
|
||||
",
|
||||
// conditional recursion
|
||||
"
|
||||
function Listitem({ depth }) {
|
||||
if (depth < 10) {
|
||||
return <Listitem depth={depth + 1} />;
|
||||
}
|
||||
return null;
|
||||
}
|
||||
",
|
||||
// conditional recursion (shorthand)
|
||||
"
|
||||
function Listitem({ depth }) {
|
||||
return depth > 5 ? <Listitem depth={depth + 1} /> : null;
|
||||
}
|
||||
",
|
||||
// reference inside jsx expression but not attribute
|
||||
"
|
||||
function List({item}) {
|
||||
return (
|
||||
<List>
|
||||
{item}
|
||||
</List>
|
||||
)
|
||||
}
|
||||
",
|
||||
// create_diagnostic_jsx - JSX element without property match
|
||||
r"
|
||||
function Listitem({ depth }) {
|
||||
return <Listitem level={depth} />;
|
||||
}
|
||||
",
|
||||
// JSX attribute not referencing function name
|
||||
r"
|
||||
function TestComponent({ body }) {
|
||||
return <AnotherComponent body={body} />;
|
||||
}
|
||||
",
|
||||
// property not used in recursion
|
||||
r"
|
||||
function test({prop1, prop2}) {
|
||||
return (<></>)
|
||||
}
|
||||
",
|
||||
// property swapped in recursion
|
||||
r"
|
||||
function Test({prop1, prop2}) {
|
||||
return (<Test prop1={prop2} prop2={prop1} />)
|
||||
}
|
||||
",
|
||||
// arguments swapped in recursion (arrow)
|
||||
r"
|
||||
const Test = ({prop1, prop2}) => {
|
||||
return (<Test prop1={prop2} prop2={prop1} />)
|
||||
}
|
||||
",
|
||||
];
|
||||
|
||||
let fail = vec![
|
||||
|
|
@ -420,6 +718,33 @@ fn test() {
|
|||
",
|
||||
"//¿
|
||||
function writeChunks(a,callac){writeChunks(m,callac)}writeChunks(i,{})",
|
||||
"
|
||||
function ListItem({ depth }) {
|
||||
return <ListItem depth={depth} />
|
||||
}
|
||||
",
|
||||
"
|
||||
function ListItem({ depth: listDepth }) {
|
||||
return <ListItem depth={listDepth} />;
|
||||
}
|
||||
",
|
||||
"
|
||||
function ListItem({depth = 0}) {
|
||||
return <ListItem depth={depth} />
|
||||
}
|
||||
",
|
||||
"
|
||||
function ListItem({depth, ...otherProps}) {
|
||||
return <ListItem depth={depth} />
|
||||
}
|
||||
",
|
||||
r"
|
||||
function Test({a, b}) {
|
||||
return (
|
||||
<Test a={a} b={b}/>
|
||||
)
|
||||
}
|
||||
",
|
||||
];
|
||||
|
||||
let fix = vec![
|
||||
|
|
@ -486,6 +811,59 @@ function writeChunks(a,callac){writeChunks(m,callac)}writeChunks(i,{})",
|
|||
export default test;
|
||||
",
|
||||
),
|
||||
(
|
||||
r"function Test({a, b}) {
|
||||
a++;
|
||||
return (
|
||||
<Test a={a} b={b}/>
|
||||
)
|
||||
}
|
||||
export default test;
|
||||
",
|
||||
r"function Test({a}) {
|
||||
a++;
|
||||
return (
|
||||
<Test a={a} />
|
||||
)
|
||||
}
|
||||
export default test;
|
||||
",
|
||||
),
|
||||
(
|
||||
r"function Test({a, b}) {
|
||||
console.log(b)
|
||||
return (<Test a={a} b={b} />)
|
||||
}
|
||||
",
|
||||
r"function Test({b}) {
|
||||
console.log(b)
|
||||
return (<Test b={b} />)
|
||||
}
|
||||
",
|
||||
),
|
||||
(
|
||||
r"function Test({a, b}) {
|
||||
b++;
|
||||
return (<Test a={a} b={b}/>)
|
||||
}
|
||||
",
|
||||
r"function Test({b}) {
|
||||
b++;
|
||||
return (<Test b={b}/>)
|
||||
}
|
||||
",
|
||||
),
|
||||
// Expecting no fix: function is exported
|
||||
(
|
||||
r"function ListItem({depth, ...otherProps}) {
|
||||
return <ListItem depth={depth} {...otherProps}/>
|
||||
}
|
||||
",
|
||||
r"function ListItem({depth, ...otherProps}) {
|
||||
return <ListItem depth={depth} {...otherProps}/>
|
||||
}
|
||||
",
|
||||
),
|
||||
];
|
||||
|
||||
Tester::new(OnlyUsedInRecursion::NAME, pass, fail).expect_fix(fix).test_and_snapshot();
|
||||
|
|
|
|||
|
|
@ -1,5 +1,6 @@
|
|||
---
|
||||
source: crates/oxc_linter/src/tester.rs
|
||||
snapshot_kind: text
|
||||
---
|
||||
⚠ oxc(only-used-in-recursion): Parameter `arg0` is only used in recursive calls
|
||||
╭─[only_used_in_recursion.tsx:2:27]
|
||||
|
|
@ -80,3 +81,57 @@ source: crates/oxc_linter/src/tester.rs
|
|||
· ──────
|
||||
╰────
|
||||
help: Remove the argument and its usage. Alternatively, use the argument in the function body.
|
||||
|
||||
⚠ oxc(only-used-in-recursion): Parameter `depth` is only used in recursive calls
|
||||
╭─[only_used_in_recursion.tsx:2:33]
|
||||
1 │
|
||||
2 │ function ListItem({ depth }) {
|
||||
· ─────
|
||||
3 │ return <ListItem depth={depth} />
|
||||
╰────
|
||||
help: Remove the argument and its usage. Alternatively, use the argument in the function body.
|
||||
|
||||
⚠ oxc(only-used-in-recursion): Parameter `depth` is only used in recursive calls
|
||||
╭─[only_used_in_recursion.tsx:2:33]
|
||||
1 │
|
||||
2 │ function ListItem({ depth: listDepth }) {
|
||||
· ────────────────
|
||||
3 │ return <ListItem depth={listDepth} />;
|
||||
╰────
|
||||
help: Remove the argument and its usage. Alternatively, use the argument in the function body.
|
||||
|
||||
⚠ oxc(only-used-in-recursion): Parameter `depth` is only used in recursive calls
|
||||
╭─[only_used_in_recursion.tsx:2:32]
|
||||
1 │
|
||||
2 │ function ListItem({depth = 0}) {
|
||||
· ─────────
|
||||
3 │ return <ListItem depth={depth} />
|
||||
╰────
|
||||
help: Remove the argument and its usage. Alternatively, use the argument in the function body.
|
||||
|
||||
⚠ oxc(only-used-in-recursion): Parameter `depth` is only used in recursive calls
|
||||
╭─[only_used_in_recursion.tsx:2:32]
|
||||
1 │
|
||||
2 │ function ListItem({depth, ...otherProps}) {
|
||||
· ─────
|
||||
3 │ return <ListItem depth={depth} />
|
||||
╰────
|
||||
help: Remove the argument and its usage. Alternatively, use the argument in the function body.
|
||||
|
||||
⚠ oxc(only-used-in-recursion): Parameter `a` is only used in recursive calls
|
||||
╭─[only_used_in_recursion.tsx:2:28]
|
||||
1 │
|
||||
2 │ function Test({a, b}) {
|
||||
· ─
|
||||
3 │ return (
|
||||
╰────
|
||||
help: Remove the argument and its usage. Alternatively, use the argument in the function body.
|
||||
|
||||
⚠ oxc(only-used-in-recursion): Parameter `b` is only used in recursive calls
|
||||
╭─[only_used_in_recursion.tsx:2:31]
|
||||
1 │
|
||||
2 │ function Test({a, b}) {
|
||||
· ─
|
||||
3 │ return (
|
||||
╰────
|
||||
help: Remove the argument and its usage. Alternatively, use the argument in the function body.
|
||||
|
|
|
|||
Loading…
Reference in a new issue