From eff42d1b26ce02ad906ed7ca64ed0bd62a43721e Mon Sep 17 00:00:00 2001 From: no-yan <63000297+no-yan@users.noreply.github.com> Date: Mon, 28 Oct 2024 03:49:08 +0900 Subject: [PATCH 01/16] feat(linter): add jsx support for only-used-in-recursion --- .../src/rules/oxc/only_used_in_recursion.rs | 448 +++++++++++++++++- .../src/snapshots/only_used_in_recursion.snap | 55 +++ 2 files changed, 477 insertions(+), 26 deletions(-) diff --git a/crates/oxc_linter/src/rules/oxc/only_used_in_recursion.rs b/crates/oxc_linter/src/rules/oxc/only_used_in_recursion.rs index 92e35c9199bfe..92ea4b43edd87 100644 --- a/crates/oxc_linter/src/rules/oxc/only_used_in_recursion.rs +++ b/crates/oxc_linter/src/rules/oxc/only_used_in_recursion.rs @@ -1,10 +1,14 @@ use oxc_ast::{ - ast::{BindingIdentifier, BindingPatternKind, CallExpression, Expression, FormalParameters}, + ast::{ + BindingIdentifier, BindingPatternKind, BindingProperty, CallExpression, Expression, + FormalParameters, IdentifierReference, JSXAttributeItem, JSXElementName, + JSXMemberExpression, JSXMemberExpressionObject, PropertyKey, + }, 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 +96,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; - }; - - 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, - ); + 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; + }; + + 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 +187,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 +202,104 @@ fn create_diagnostic( ); } +fn create_diagnostic_jsx( + ctx: &LintContext, + function_id: &BindingIdentifier, + property: &BindingProperty, +) { + let is_exported = ctx + .semantic() + .symbols() + .get_flags(function_id.symbol_id.get().expect("`symbol_id` should be set")) + .is_export(); + + let property_name = &property.key.static_name().unwrap(); + if is_exported { + return ctx.diagnostic(only_used_in_recursion_diagnostic(property.span(), property_name)); + } + + let property_ident = + property.value.get_binding_identifier().expect("`bind_identifier` should be set"); + let mut references = ctx + .semantic() + .symbol_references(property_ident.symbol_id.get().expect("symbol should be set")); + + let has_spread_attribute = references.any(|x| used_with_spread_attribute(x.node_id(), ctx)); + + if has_spread_attribute { + return ctx.diagnostic(only_used_in_recursion_diagnostic( + property.span(), + get_property_name(property).unwrap(), + )); + } + + 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))); + let Some(property_id) = get_binding_identifier(property) else { + // never happen + return fix; + }; + + // search for references to the function and remove the argument + for reference in ctx + .semantic() + .symbol_references(property_id.symbol_id.get().expect("symbol should be set")) + { + let mut ancestors = ctx.nodes().ancestors(reference.node_id()); + + let Some((node_id, attr)) = + ancestors.find_map(|node| match ctx.nodes().get_node(node).kind() { + AstKind::JSXAttributeItem(attr) => Some((node, attr)), + _ => None, + }) + else { + continue; + }; + + // if JSXElement has spread attribute, we cannot operate fix safely. + // because + let Some(AstKind::JSXOpeningElement(opening_elem)) = + ctx.nodes().parent_kind(node_id) + else { + continue; + }; + + fix.push(Fix::delete(attr.span())); + } + + fix + }, + ); +} + +fn used_with_spread_attribute(node_id: NodeId, ctx: &LintContext) -> bool { + let opening_element = ctx + .nodes() + .ancestors(node_id) + .find_map(|node| match ctx.nodes().kind(node) { + AstKind::JSXOpeningElement(opening_element) => Some(opening_element), + _ => None, + }) + .expect("should be safe"); + + opening_element.attributes.iter().any(|attr| matches!(attr, JSXAttributeItem::SpreadAttribute(_))) +} + fn is_argument_only_used_in_recursion<'a>( function_id: &'a BindingIdentifier, arg: &'a BindingIdentifier, @@ -222,6 +344,83 @@ 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 mut references = ctx + .semantic() + .symbol_references(ident.symbol_id.get().expect("`symbol_id` should be set")) + .peekable(); + + if references.peek().is_none() { + return false; + } + + // let has_spread_attribute = references.any(|reference| { + // }) + + let function_symbol_id = function_ident.symbol_id.get().expect("`symbol_id` should be set"); + + for reference in references { + // 1. Reference is inside JSXExpressionContainer + // 2. JSXElement is calling recursive function itself + // 3. reference is in JSXAttribute and attribute name has same name + 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 symply ignore the references inside JSXExpressionContainer are not single-node expression. + // e.g. + // + // To support that 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) = ctx.nodes().get_node(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().ancestors(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, @@ -251,16 +450,48 @@ fn is_function_maybe_reassigned<'a>( }) } -// skipping whitespace, commas, finds the next character (exclusive) +fn get_jsx_element_symbol_id<'a>( + node: &'a JSXElementName<'a>, + ctx: &'a LintContext<'_>, +) -> Option { + let node = match node { + JSXElementName::IdentifierReference(ident) => Some(ident.as_ref()), + JSXElementName::MemberExpression(expr) => expr.get_identifier(), + JSXElementName::Identifier(_) + | JSXElementName::NamespacedName(_) + | JSXElementName::ThisExpression(_) => None, + }?; + + node.reference_id().and_then(|id| ctx.symbols().get_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) -> u32 { - for (i, c) in s.char_indices().skip(start as usize) { - if !c.is_whitespace() && c != ',' { - return i as u32; - } +fn skip_to_next_char(s: &str, start: u32, direction: &Direction) -> Option { + // span is a half-open interval: [start, end) + // so we should return in that way. + let start = start.try_into().expect("converting u32 to usize should be safe"); + 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(), } - - s.len() as u32 } #[test] @@ -274,12 +505,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); @@ -327,13 +569,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); @@ -367,6 +609,80 @@ fn test() { }; validator() "#, + // no params, no recursion + " + function Test() { + return ; + } + ", + " + function Listitem({ depth }) { + console.log(depth); + return ; + } + ", + // allowed case + // multi-node expressions are not supported for now + " + function Listitem({ depth }) { + return + } + ", + // conditional recursion + " + function Listitem({ depth }) { + if (depth < 10) { + return ; + } + return null; + } + ", + // conditional recursion (shorthand) + " + function Listitem({ depth }) { + return depth > 5 ? : null; + } + ", + // reference inside jsx expression but not attribute + " + function List({item}) { + return ( + + {item} + + ) + } + ", + // create_diagnostic_jsx - JSX element without property match + r" + function Listitem({ depth }) { + return ; + } + ", + // JSX attribute not referencing function name + r" + function TestComponent({ body }) { + return ; + } + ", + // property not used in recursion + r" + function test({prop1, prop2}) { + return (<>) + } + ", + // property swapped in recursion + r" + function Test({prop1, prop2}) { + return () + } + ", + // arguments swapped in recursion (arrow) + r" + const Test = ({prop1, prop2}) => { + return () + } + ", ]; let fail = vec![ @@ -421,6 +737,33 @@ fn test() { ", "//¿ function writeChunks(a,callac){writeChunks(m,callac)}writeChunks(i,{})", + " + function ListItem({ depth }) { + return + } + ", + " + function ListItem({ depth: listDepth }) { + return ; + } + ", + " + function ListItem({depth = 0}) { + return + } + ", + " + function ListItem({depth, ...otherProps}) { + return + } + ", + r" + function Test({a, b}) { + return ( + + ) + } + ", ]; let fix = vec![ @@ -487,6 +830,59 @@ function writeChunks(a,callac){writeChunks(m,callac)}writeChunks(i,{})", export default test; ", ), + ( + r"function Test({a, b}) { + a++; + return ( + + ) + } + export default test; + ", + r"function Test({a}) { + a++; + return ( + + ) + } + export default test; + ", + ), + ( + r"function Test({a, b}) { + console.log(b) + return () + } + ", + r"function Test({b}) { + console.log(b) + return () + } + ", + ), + ( + r"function Test({a, b}) { + b++; + return () + } + ", + r"function Test({b}) { + b++; + return () + } + ", + ), + // Expecting no fix: function is exported + ( + r"function ListItem({depth, ...otherProps}) { + return + } + ", + r"function ListItem({depth, ...otherProps}) { + return + } + ", + ), ]; Tester::new(OnlyUsedInRecursion::NAME, pass, fail).expect_fix(fix).test_and_snapshot(); diff --git a/crates/oxc_linter/src/snapshots/only_used_in_recursion.snap b/crates/oxc_linter/src/snapshots/only_used_in_recursion.snap index 2882fc1d3adbd..43aa0e1f5681b 100644 --- a/crates/oxc_linter/src/snapshots/only_used_in_recursion.snap +++ b/crates/oxc_linter/src/snapshots/only_used_in_recursion.snap @@ -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 + ╰──── + 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 ; + ╰──── + 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 + ╰──── + 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 + ╰──── + 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. From b9a97383617245bd0409151f912f8c9bd8282673 Mon Sep 17 00:00:00 2001 From: no-yan <63000297+no-yan@users.noreply.github.com> Date: Mon, 4 Nov 2024 23:03:46 +0900 Subject: [PATCH 02/16] chore(linter): simplify error handling for unreachable case --- .../oxc_linter/src/rules/oxc/only_used_in_recursion.rs | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/crates/oxc_linter/src/rules/oxc/only_used_in_recursion.rs b/crates/oxc_linter/src/rules/oxc/only_used_in_recursion.rs index 92ea4b43edd87..91d95467f084a 100644 --- a/crates/oxc_linter/src/rules/oxc/only_used_in_recursion.rs +++ b/crates/oxc_linter/src/rules/oxc/only_used_in_recursion.rs @@ -250,10 +250,11 @@ fn create_diagnostic_jsx( .unwrap_or(property.span.end); fix.push(Fix::delete(Span::new(span_start, span_end))); - let Some(property_id) = get_binding_identifier(property) else { - // never happen - return fix; - }; + + let property_id = property + .value + .get_binding_identifier() + .expect("`binding identifier` should be set"); // search for references to the function and remove the argument for reference in ctx From f31f790be021b560d194cc19d4b4c06070476aa4 Mon Sep 17 00:00:00 2001 From: no-yan <63000297+no-yan@users.noreply.github.com> Date: Mon, 4 Nov 2024 23:04:06 +0900 Subject: [PATCH 03/16] style(linter): cargo fmt --- crates/oxc_linter/src/rules/oxc/only_used_in_recursion.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/crates/oxc_linter/src/rules/oxc/only_used_in_recursion.rs b/crates/oxc_linter/src/rules/oxc/only_used_in_recursion.rs index 91d95467f084a..ce00fe2aa8a59 100644 --- a/crates/oxc_linter/src/rules/oxc/only_used_in_recursion.rs +++ b/crates/oxc_linter/src/rules/oxc/only_used_in_recursion.rs @@ -298,7 +298,10 @@ fn used_with_spread_attribute(node_id: NodeId, ctx: &LintContext) -> bool { }) .expect("should be safe"); - opening_element.attributes.iter().any(|attr| matches!(attr, JSXAttributeItem::SpreadAttribute(_))) + opening_element + .attributes + .iter() + .any(|attr| matches!(attr, JSXAttributeItem::SpreadAttribute(_))) } fn is_argument_only_used_in_recursion<'a>( From fa0ae40a6ceaf0828653c61708711c5963bdd5e6 Mon Sep 17 00:00:00 2001 From: no-yan <63000297+no-yan@users.noreply.github.com> Date: Mon, 4 Nov 2024 23:11:17 +0900 Subject: [PATCH 04/16] chore(linter): refine comments in `only_used_in_recursion` rule --- .../src/rules/oxc/only_used_in_recursion.rs | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/crates/oxc_linter/src/rules/oxc/only_used_in_recursion.rs b/crates/oxc_linter/src/rules/oxc/only_used_in_recursion.rs index ce00fe2aa8a59..8766490e5f888 100644 --- a/crates/oxc_linter/src/rules/oxc/only_used_in_recursion.rs +++ b/crates/oxc_linter/src/rules/oxc/only_used_in_recursion.rs @@ -363,23 +363,21 @@ fn is_property_only_used_in_recursion_jsx( return false; } - // let has_spread_attribute = references.any(|reference| { - // }) - let function_symbol_id = function_ident.symbol_id.get().expect("`symbol_id` should be set"); for reference in references { - // 1. Reference is inside JSXExpressionContainer - // 2. JSXElement is calling recursive function itself - // 3. reference is in JSXAttribute and attribute name has same name + // 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 symply ignore the references inside JSXExpressionContainer are not single-node expression. + // In this case, we simply ignore the references inside JSXExpressionContainer that are not single-node expression. // e.g. // - // To support that case, we need to check whether expression contains side-effect like ++val + // To support this case, we need to check whether expression contains side-effect like ++val return false; }; From 8a709f528dd286ebf2838a77041301ab8073cf0d Mon Sep 17 00:00:00 2001 From: no-yan <63000297+no-yan@users.noreply.github.com> Date: Mon, 4 Nov 2024 23:28:34 +0900 Subject: [PATCH 05/16] chore(linter): clean up unreachable case --- .../src/rules/oxc/only_used_in_recursion.rs | 12 ++---------- 1 file changed, 2 insertions(+), 10 deletions(-) diff --git a/crates/oxc_linter/src/rules/oxc/only_used_in_recursion.rs b/crates/oxc_linter/src/rules/oxc/only_used_in_recursion.rs index 8766490e5f888..f238655dd2c13 100644 --- a/crates/oxc_linter/src/rules/oxc/only_used_in_recursion.rs +++ b/crates/oxc_linter/src/rules/oxc/only_used_in_recursion.rs @@ -263,23 +263,15 @@ fn create_diagnostic_jsx( { let mut ancestors = ctx.nodes().ancestors(reference.node_id()); - let Some((node_id, attr)) = + let Some(attr) = ancestors.find_map(|node| match ctx.nodes().get_node(node).kind() { - AstKind::JSXAttributeItem(attr) => Some((node, attr)), + AstKind::JSXAttributeItem(attr) => Some(attr), _ => None, }) else { continue; }; - // if JSXElement has spread attribute, we cannot operate fix safely. - // because - let Some(AstKind::JSXOpeningElement(opening_elem)) = - ctx.nodes().parent_kind(node_id) - else { - continue; - }; - fix.push(Fix::delete(attr.span())); } From cac788d3d2e89b596487de978e2f5503f37d6934 Mon Sep 17 00:00:00 2001 From: no-yan <63000297+no-yan@users.noreply.github.com> Date: Mon, 4 Nov 2024 23:29:05 +0900 Subject: [PATCH 06/16] chore(linter): add comment for spread attribute case --- crates/oxc_linter/src/rules/oxc/only_used_in_recursion.rs | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/crates/oxc_linter/src/rules/oxc/only_used_in_recursion.rs b/crates/oxc_linter/src/rules/oxc/only_used_in_recursion.rs index f238655dd2c13..7480a551d4d5e 100644 --- a/crates/oxc_linter/src/rules/oxc/only_used_in_recursion.rs +++ b/crates/oxc_linter/src/rules/oxc/only_used_in_recursion.rs @@ -227,10 +227,9 @@ fn create_diagnostic_jsx( let has_spread_attribute = references.any(|x| used_with_spread_attribute(x.node_id(), ctx)); if has_spread_attribute { - return ctx.diagnostic(only_used_in_recursion_diagnostic( - property.span(), - get_property_name(property).unwrap(), - )); + // 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 { From 800956916141ce684478af4334fe04a269c659c3 Mon Sep 17 00:00:00 2001 From: no-yan <63000297+no-yan@users.noreply.github.com> Date: Tue, 5 Nov 2024 00:07:23 +0900 Subject: [PATCH 07/16] chore(linter): fix comment --- crates/oxc_linter/src/rules/oxc/only_used_in_recursion.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/oxc_linter/src/rules/oxc/only_used_in_recursion.rs b/crates/oxc_linter/src/rules/oxc/only_used_in_recursion.rs index 7480a551d4d5e..54345242c16a2 100644 --- a/crates/oxc_linter/src/rules/oxc/only_used_in_recursion.rs +++ b/crates/oxc_linter/src/rules/oxc/only_used_in_recursion.rs @@ -255,7 +255,7 @@ fn create_diagnostic_jsx( .get_binding_identifier() .expect("`binding identifier` should be set"); - // search for references to the function and remove the argument + // search for references to the function and remove the property for reference in ctx .semantic() .symbol_references(property_id.symbol_id.get().expect("symbol should be set")) From b22a5dc4cbf9eb2f357a1b5fd978a9d830cdf370 Mon Sep 17 00:00:00 2001 From: no-yan <63000297+no-yan@users.noreply.github.com> Date: Tue, 5 Nov 2024 00:51:23 +0900 Subject: [PATCH 08/16] test: add passing (due to implementation limitations) --- .../oxc_linter/src/rules/oxc/only_used_in_recursion.rs | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/crates/oxc_linter/src/rules/oxc/only_used_in_recursion.rs b/crates/oxc_linter/src/rules/oxc/only_used_in_recursion.rs index 54345242c16a2..75a274dd65bd4 100644 --- a/crates/oxc_linter/src/rules/oxc/only_used_in_recursion.rs +++ b/crates/oxc_linter/src/rules/oxc/only_used_in_recursion.rs @@ -608,6 +608,15 @@ fn test() { return ; } ", + // 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 ; + } + ", " function Listitem({ depth }) { console.log(depth); From 23a0533777a730ad725c84adccb62ed824ac0834 Mon Sep 17 00:00:00 2001 From: no-yan <63000297+no-yan@users.noreply.github.com> Date: Tue, 5 Nov 2024 09:21:56 +0900 Subject: [PATCH 09/16] chore(linter): clean up unused imports --- crates/oxc_linter/src/rules/oxc/only_used_in_recursion.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/crates/oxc_linter/src/rules/oxc/only_used_in_recursion.rs b/crates/oxc_linter/src/rules/oxc/only_used_in_recursion.rs index 75a274dd65bd4..dd525d9867495 100644 --- a/crates/oxc_linter/src/rules/oxc/only_used_in_recursion.rs +++ b/crates/oxc_linter/src/rules/oxc/only_used_in_recursion.rs @@ -1,8 +1,7 @@ use oxc_ast::{ ast::{ BindingIdentifier, BindingPatternKind, BindingProperty, CallExpression, Expression, - FormalParameters, IdentifierReference, JSXAttributeItem, JSXElementName, - JSXMemberExpression, JSXMemberExpressionObject, PropertyKey, + FormalParameters, JSXAttributeItem, JSXElementName, }, AstKind, }; From bb2dabf0cd8962dad1e10a824235cbed0bfb5b2a Mon Sep 17 00:00:00 2001 From: no-yan <63000297+no-yan@users.noreply.github.com> Date: Wed, 6 Nov 2024 01:22:42 +0900 Subject: [PATCH 10/16] refactor: update get_jsx_element to use direct reference_id retrieval --- crates/oxc_linter/src/rules/oxc/only_used_in_recursion.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/oxc_linter/src/rules/oxc/only_used_in_recursion.rs b/crates/oxc_linter/src/rules/oxc/only_used_in_recursion.rs index 60b39d94d0347..5f9beaffef151 100644 --- a/crates/oxc_linter/src/rules/oxc/only_used_in_recursion.rs +++ b/crates/oxc_linter/src/rules/oxc/only_used_in_recursion.rs @@ -453,7 +453,7 @@ fn get_jsx_element_symbol_id<'a>( | JSXElementName::ThisExpression(_) => None, }?; - node.reference_id().and_then(|id| ctx.symbols().get_reference(id).symbol_id()) + ctx.symbols().get_reference(node.reference_id()).symbol_id() } enum Direction { From 7cad64a170113cb33522939ccd0a41a040e8a515 Mon Sep 17 00:00:00 2001 From: no-yan <63000297+no-yan@users.noreply.github.com> Date: Fri, 8 Nov 2024 00:33:14 +0900 Subject: [PATCH 11/16] fix(linter): avoid `expect` by using .any() and iter_parents Co-authored-by: Cam McHenry --- .../src/rules/oxc/only_used_in_recursion.rs | 24 ++++++++++--------- 1 file changed, 13 insertions(+), 11 deletions(-) diff --git a/crates/oxc_linter/src/rules/oxc/only_used_in_recursion.rs b/crates/oxc_linter/src/rules/oxc/only_used_in_recursion.rs index 5f9beaffef151..387d292f5cbe3 100644 --- a/crates/oxc_linter/src/rules/oxc/only_used_in_recursion.rs +++ b/crates/oxc_linter/src/rules/oxc/only_used_in_recursion.rs @@ -279,19 +279,21 @@ fn create_diagnostic_jsx( } fn used_with_spread_attribute(node_id: NodeId, ctx: &LintContext) -> bool { - let opening_element = ctx + ctx .nodes() - .ancestors(node_id) - .find_map(|node| match ctx.nodes().kind(node) { - AstKind::JSXOpeningElement(opening_element) => Some(opening_element), - _ => None, + .iter_parents(node_id) + .any(|node| match node.kind() { + AstKind::JSXOpeningElement(opening_element) => { + opening_element + .attributes + .iter() + .any(|attr| matches!(attr, JSXAttributeItem::SpreadAttribute(_))) + }, + _ => false, }) - .expect("should be safe"); - opening_element - .attributes - .iter() - .any(|attr| matches!(attr, JSXAttributeItem::SpreadAttribute(_))) + + } fn is_argument_only_used_in_recursion<'a>( @@ -467,7 +469,7 @@ enum Direction { fn skip_to_next_char(s: &str, start: u32, direction: &Direction) -> Option { // span is a half-open interval: [start, end) // so we should return in that way. - let start = start.try_into().expect("converting u32 to usize should be safe"); + let start = start as usize; match direction { Direction::Forward => s .char_indices() From 18cfa6645af6041825b3de4cf9cb5da8571ea1e3 Mon Sep 17 00:00:00 2001 From: "autofix-ci[bot]" <114827586+autofix-ci[bot]@users.noreply.github.com> Date: Thu, 7 Nov 2024 15:34:02 +0000 Subject: [PATCH 12/16] [autofix.ci] apply automated fixes --- .../src/rules/oxc/only_used_in_recursion.rs | 22 ++++++------------- 1 file changed, 7 insertions(+), 15 deletions(-) diff --git a/crates/oxc_linter/src/rules/oxc/only_used_in_recursion.rs b/crates/oxc_linter/src/rules/oxc/only_used_in_recursion.rs index 387d292f5cbe3..bb35174a1d897 100644 --- a/crates/oxc_linter/src/rules/oxc/only_used_in_recursion.rs +++ b/crates/oxc_linter/src/rules/oxc/only_used_in_recursion.rs @@ -279,21 +279,13 @@ fn create_diagnostic_jsx( } fn used_with_spread_attribute(node_id: NodeId, ctx: &LintContext) -> bool { - ctx - .nodes() - .iter_parents(node_id) - .any(|node| match node.kind() { - AstKind::JSXOpeningElement(opening_element) => { - opening_element - .attributes - .iter() - .any(|attr| matches!(attr, JSXAttributeItem::SpreadAttribute(_))) - }, - _ => false, - }) - - - + ctx.nodes().iter_parents(node_id).any(|node| match node.kind() { + AstKind::JSXOpeningElement(opening_element) => opening_element + .attributes + .iter() + .any(|attr| matches!(attr, JSXAttributeItem::SpreadAttribute(_))), + _ => false, + }) } fn is_argument_only_used_in_recursion<'a>( From e1f23ba60eae840fa37e289ee44ec9d1a20f7184 Mon Sep 17 00:00:00 2001 From: no-yan <63000297+no-yan@users.noreply.github.com> Date: Fri, 8 Nov 2024 00:36:00 +0900 Subject: [PATCH 13/16] refactor(linter): use `iter_parents` to handle nodes directly Co-authored-by: Cam McHenry --- crates/oxc_linter/src/rules/oxc/only_used_in_recursion.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/oxc_linter/src/rules/oxc/only_used_in_recursion.rs b/crates/oxc_linter/src/rules/oxc/only_used_in_recursion.rs index bb35174a1d897..500fe1a7b8f1c 100644 --- a/crates/oxc_linter/src/rules/oxc/only_used_in_recursion.rs +++ b/crates/oxc_linter/src/rules/oxc/only_used_in_recursion.rs @@ -365,8 +365,8 @@ fn is_property_only_used_in_recursion_jsx( return false; }; - let Some(attr) = ctx.nodes().ancestors(may_jsx_expr_container.id()).find_map(|node| { - if let AstKind::JSXAttributeItem(attr) = ctx.nodes().get_node(node).kind() { + let Some(attr) = ctx.nodes().iter_parents(may_jsx_expr_container.id()).find_map(|node| { + if let AstKind::JSXAttributeItem(attr) = node.kind() { Some(attr) } else { None From c9e1bb5bde3be2fd1aca63a621c26d69b2cd30d0 Mon Sep 17 00:00:00 2001 From: no-yan <63000297+no-yan@users.noreply.github.com> Date: Fri, 8 Nov 2024 00:51:50 +0900 Subject: [PATCH 14/16] fix(linter): avoid `expect` by returning early --- .../src/rules/oxc/only_used_in_recursion.rs | 35 ++++++------------- 1 file changed, 10 insertions(+), 25 deletions(-) diff --git a/crates/oxc_linter/src/rules/oxc/only_used_in_recursion.rs b/crates/oxc_linter/src/rules/oxc/only_used_in_recursion.rs index 500fe1a7b8f1c..dc9345389898a 100644 --- a/crates/oxc_linter/src/rules/oxc/only_used_in_recursion.rs +++ b/crates/oxc_linter/src/rules/oxc/only_used_in_recursion.rs @@ -206,22 +206,17 @@ fn create_diagnostic_jsx( function_id: &BindingIdentifier, property: &BindingProperty, ) { - let is_exported = ctx - .semantic() - .symbols() - .get_flags(function_id.symbol_id.get().expect("`symbol_id` should be set")) - .is_export(); + 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 property_name = &property.key.static_name().unwrap(); + 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 property_ident = - property.value.get_binding_identifier().expect("`bind_identifier` should be set"); - let mut references = ctx - .semantic() - .symbol_references(property_ident.symbol_id.get().expect("symbol should be set")); + 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)); @@ -249,16 +244,8 @@ fn create_diagnostic_jsx( fix.push(Fix::delete(Span::new(span_start, span_end))); - let property_id = property - .value - .get_binding_identifier() - .expect("`binding identifier` should be set"); - // search for references to the function and remove the property - for reference in ctx - .semantic() - .symbol_references(property_id.symbol_id.get().expect("symbol should be set")) - { + for reference in ctx.semantic().symbol_references(property_symbol_id) { let mut ancestors = ctx.nodes().ancestors(reference.node_id()); let Some(attr) = @@ -338,16 +325,14 @@ fn is_property_only_used_in_recursion_jsx( function_ident: &BindingIdentifier, ctx: &LintContext, ) -> bool { - let mut references = ctx - .semantic() - .symbol_references(ident.symbol_id.get().expect("`symbol_id` should be set")) - .peekable(); + 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 function_symbol_id = function_ident.symbol_id.get().expect("`symbol_id` should be set"); + let Some(function_symbol_id) = function_ident.symbol_id.get() else { return false }; for reference in references { // Conditions: From c42d5ed29ddb489d5e74cebc2820499abd8e41e8 Mon Sep 17 00:00:00 2001 From: no-yan <63000297+no-yan@users.noreply.github.com> Date: Sat, 9 Nov 2024 16:43:48 +0900 Subject: [PATCH 15/16] chore: sync with main - rename ancestors and iter_parents --- .../src/rules/oxc/only_used_in_recursion.rs | 24 ++++++++++--------- 1 file changed, 13 insertions(+), 11 deletions(-) diff --git a/crates/oxc_linter/src/rules/oxc/only_used_in_recursion.rs b/crates/oxc_linter/src/rules/oxc/only_used_in_recursion.rs index dc9345389898a..198f5defd6475 100644 --- a/crates/oxc_linter/src/rules/oxc/only_used_in_recursion.rs +++ b/crates/oxc_linter/src/rules/oxc/only_used_in_recursion.rs @@ -246,10 +246,10 @@ fn create_diagnostic_jsx( // search for references to the function and remove the property for reference in ctx.semantic().symbol_references(property_symbol_id) { - let mut ancestors = ctx.nodes().ancestors(reference.node_id()); + let mut ancestor_ids = ctx.nodes().ancestor_ids(reference.node_id()); let Some(attr) = - ancestors.find_map(|node| match ctx.nodes().get_node(node).kind() { + ancestor_ids.find_map(|node| match ctx.nodes().get_node(node).kind() { AstKind::JSXAttributeItem(attr) => Some(attr), _ => None, }) @@ -266,7 +266,7 @@ fn create_diagnostic_jsx( } fn used_with_spread_attribute(node_id: NodeId, ctx: &LintContext) -> bool { - ctx.nodes().iter_parents(node_id).any(|node| match node.kind() { + ctx.nodes().ancestors(node_id).any(|node| match node.kind() { AstKind::JSXOpeningElement(opening_element) => opening_element .attributes .iter() @@ -350,7 +350,7 @@ fn is_property_only_used_in_recursion_jsx( return false; }; - let Some(attr) = ctx.nodes().iter_parents(may_jsx_expr_container.id()).find_map(|node| { + let Some(attr) = ctx.nodes().ancestors(may_jsx_expr_container.id()).find_map(|node| { if let AstKind::JSXAttributeItem(attr) = node.kind() { Some(attr) } else { @@ -370,13 +370,15 @@ fn is_property_only_used_in_recursion_jsx( return false; } - let Some(opening_element) = ctx.nodes().ancestors(reference.node_id()).find_map(|node| { - if let AstKind::JSXOpeningElement(elem) = ctx.nodes().get_node(node).kind() { - Some(elem) - } else { - None - } - }) else { + 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; }; From be722431437790e1a0dba367f5458abf8b5f096b Mon Sep 17 00:00:00 2001 From: no-yan <63000297+no-yan@users.noreply.github.com> Date: Sat, 9 Nov 2024 17:48:39 +0900 Subject: [PATCH 16/16] refactor: use ancestor_kinds to iterate ancestor's node kind --- crates/oxc_linter/src/rules/oxc/only_used_in_recursion.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/oxc_linter/src/rules/oxc/only_used_in_recursion.rs b/crates/oxc_linter/src/rules/oxc/only_used_in_recursion.rs index 198f5defd6475..cbee4ffbb467e 100644 --- a/crates/oxc_linter/src/rules/oxc/only_used_in_recursion.rs +++ b/crates/oxc_linter/src/rules/oxc/only_used_in_recursion.rs @@ -266,7 +266,7 @@ fn create_diagnostic_jsx( } fn used_with_spread_attribute(node_id: NodeId, ctx: &LintContext) -> bool { - ctx.nodes().ancestors(node_id).any(|node| match node.kind() { + ctx.nodes().ancestor_kinds(node_id).any(|kind| match kind { AstKind::JSXOpeningElement(opening_element) => opening_element .attributes .iter()