Skip to content

Commit

Permalink
refactor(linter): improve recursive argument handling and diagnostics…
Browse files Browse the repository at this point in the history 
… creation (#6513)

### Overview
This PR refactors `only-used-in-recursion` codebase to make the
implementation of #5530 easier.

The diff isn't displaying cleanly, so it might be better to review it
commit by commit when looking at the changes.

### Key changes
1. Extracted diagnostic logic into `craete_diagnostic` function:
3bf0015
2. Removed redundant check in `is_function_maybe_reassigned`:
a133ec6
3. Simplified `is_argument_only_used_in_recursion` by removing nesting:
6e6bd04
  • Loading branch information
@no-yan
no-yan authored Oct 14, 2024
1 parent 111e2d7 commit ecce5c5
Showing 1 changed file with 133 additions and 113 deletions.
246 changes: 133 additions & 113 deletions crates/oxc_linter/src/rules/oxc/only_used_in_recursion.rs
View file
Original file line number Diff line number Diff line change
@@ -1,9 +1,10 @@
use oxc_ast::{
ast::{BindingIdentifier, BindingPatternKind, Expression},
ast::{BindingIdentifier, BindingPatternKind, CallExpression, Expression, FormalParameters},
AstKind,
};
use oxc_diagnostics::OxcDiagnostic;
use oxc_macros::declare_oxc_lint;
use oxc_semantic::SymbolId;
use oxc_span::{GetSpan, Span};

use crate::{
Expand Down Expand Up @@ -96,139 +97,158 @@ impl Rule for OnlyUsedInRecursion {
};

if is_argument_only_used_in_recursion(function_id, arg, arg_index, ctx) {
if arg_index == function_parameters.items.len() - 1
&& !ctx
.semantic()
.symbols()
.get_flags(function_id.symbol_id.get().expect("`symbol_id` should be set"))
.is_export()
{
ctx.diagnostic_with_dangerous_fix(
only_used_in_recursion_diagnostic(arg.span, arg.name.as_str()),
|fixer| {
let mut fix = fixer.new_fix_with_capacity(
ctx.semantic()
.symbol_references(
arg.symbol_id.get().expect("`symbol_id` should be set"),
)
.count()
+ 1,
);
fix.push(Fix::delete(arg.span()));

for reference in ctx.semantic().symbol_references(
arg.symbol_id.get().expect("`symbol_id` should be set"),
) {
let node = ctx.nodes().get_node(reference.node_id());

fix.push(Fix::delete(node.span()));
}

// search for references to the function and remove the argument
for reference in ctx.semantic().symbol_references(
function_id.symbol_id.get().expect("`symbol_id` should be set"),
) {
let node = ctx.nodes().get_node(reference.node_id());

if let Some(AstKind::CallExpression(call_expr)) =
ctx.nodes().parent_kind(node.id())
{
// check if the number of arguments is the same
if call_expr.arguments.len() != function_parameters.items.len()
|| function_span.contains_inclusive(call_expr.span)
{
continue;
}

// remove the argument
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),
)));
}
}

fix
},
);
} else {
ctx.diagnostic(only_used_in_recursion_diagnostic(arg.span, arg.name.as_str()));
}
create_diagnostic(
ctx,
function_id,
function_parameters,
arg,
arg_index,
function_span,
);
}
}
}
}

fn create_diagnostic(
ctx: &LintContext,
function_id: &BindingIdentifier,
function_parameters: &FormalParameters,
arg: &BindingIdentifier,
arg_index: usize,
function_span: Span,
) {
let is_last_arg = arg_index == function_parameters.items.len() - 1;
let is_exported = ctx
.semantic()
.symbols()
.get_flags(function_id.symbol_id.get().expect("`symbol_id` should be set"))
.is_export();

let is_diagnostic_only = !is_last_arg || is_exported;

if is_diagnostic_only {
return ctx.diagnostic(only_used_in_recursion_diagnostic(arg.span, arg.name.as_str()));
}

ctx.diagnostic_with_dangerous_fix(
only_used_in_recursion_diagnostic(arg.span, arg.name.as_str()),
|fixer| {
let mut fix = fixer.new_fix_with_capacity(
ctx.semantic()
.symbol_references(arg.symbol_id.get().expect("`symbol_id` should be set"))
.count()
+ 1,
);
fix.push(Fix::delete(arg.span()));

for reference in ctx
.semantic()
.symbol_references(arg.symbol_id.get().expect("`symbol_id` should be set"))
{
let node = ctx.nodes().get_node(reference.node_id());
fix.push(Fix::delete(node.span()));
}

// search for references to the function and remove the argument
for reference in ctx
.semantic()
.symbol_references(function_id.symbol_id.get().expect("`symbol_id` should be set"))
{
let node = ctx.nodes().get_node(reference.node_id());

if let Some(AstKind::CallExpression(call_expr)) = ctx.nodes().parent_kind(node.id())
{
if call_expr.arguments.len() != function_parameters.items.len()
|| function_span.contains_inclusive(call_expr.span)
{
continue;
}

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),
)));
}
}

fix
},
);
}

fn is_argument_only_used_in_recursion<'a>(
function_id: &'a BindingIdentifier,
arg: &'a BindingIdentifier,
arg_index: usize,
ctx: &'a LintContext<'_>,
) -> bool {
let mut is_used_only_in_recursion = true;
let mut has_references = false;

for reference in
ctx.semantic().symbol_references(arg.symbol_id.get().expect("`symbol_id` should be set"))
{
has_references = true;
if let Some(AstKind::Argument(argument)) = ctx.nodes().parent_kind(reference.node_id()) {
if let Some(AstKind::CallExpression(call_expr)) =
ctx.nodes().parent_kind(ctx.nodes().parent_node(reference.node_id()).unwrap().id())
{
if !call_expr.arguments.iter().enumerate().any(|(index, arg)| {
index == arg_index
&& arg.span() == argument.span()
&& if let Expression::Identifier(identifier) = &call_expr.callee {
identifier
.reference_id()
.and_then(|id| ctx.symbols().get_reference(id).symbol_id())
.is_some_and(|v| {
let function_symbol_id = function_id.symbol_id.get();
debug_assert!(function_symbol_id.is_some());
function_symbol_id
.is_some_and(|function_symbol_id| function_symbol_id == v)
})
} else {
false
}
}) {
is_used_only_in_recursion = false;
break;
}
} else {
is_used_only_in_recursion = false;
break;
}
} else {
is_used_only_in_recursion = false;
break;
let mut references = ctx
.semantic()
.symbol_references(arg.symbol_id.get().expect("`symbol_id` should be set"))
.peekable();

// Avoid returning true for an empty iterator
if references.peek().is_none() {
return false;
}

let function_symbol_id = function_id.symbol_id.get().unwrap();

for reference in references {
let Some(AstKind::Argument(argument)) = ctx.nodes().parent_kind(reference.node_id()) else {
return false;
};
let Some(AstKind::CallExpression(call_expr)) =
ctx.nodes().parent_kind(ctx.nodes().parent_node(reference.node_id()).unwrap().id())
else {
return false;
};

let Some(call_arg) = call_expr.arguments.get(arg_index) else {
return false;
};

if argument.span() != call_arg.span() {
return false;
}

if !is_recursive_call(call_expr, function_symbol_id, ctx) {
return false;
}
}

has_references && is_used_only_in_recursion
true
}

fn is_function_maybe_reassigned<'a>(
function_id: &'a BindingIdentifier,
ctx: &'a LintContext<'_>,
fn is_recursive_call(
call_expr: &CallExpression,
function_symbol_id: SymbolId,
ctx: &LintContext,
) -> bool {
let mut is_maybe_reassigned = false;

for reference in ctx
.semantic()
.symbol_references(function_id.symbol_id.get().expect("`symbol_id` should be set"))
{
if let Some(AstKind::SimpleAssignmentTarget(_)) =
ctx.nodes().parent_kind(reference.node_id())
if let Expression::Identifier(identifier) = &call_expr.callee {
if let Some(symbol_id) =
identifier.reference_id().and_then(|id| ctx.symbols().get_reference(id).symbol_id())
{
is_maybe_reassigned = true;
return symbol_id == function_symbol_id;
}
}
false
}

is_maybe_reassigned
fn is_function_maybe_reassigned<'a>(
function_id: &'a BindingIdentifier,
ctx: &'a LintContext<'_>,
) -> bool {
ctx.semantic()
.symbol_references(function_id.symbol_id.get().expect("`symbol_id` should be set"))
.any(|reference| {
matches!(
ctx.nodes().parent_kind(reference.node_id()),
Some(AstKind::SimpleAssignmentTarget(_))
)
})
}

// skipping whitespace, commas, finds the next character (exclusive)
Expand Down

0 comments on commit ecce5c5

Please sign in to comment.