Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor(linter): improve recursive argument handling and diagnostics creation #6513

Merged
merged 3 commits into from
Oct 14, 2024
Merged

refactor(linter): improve recursive argument handling and diagnostics creation #6513

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6e6bd04
refactor(linter): clean up and refactor recursion check logic
no-yan Oct 7, 2024
a133ec6
refactor(linter): simplify assignment detection with early return
no-yan Oct 12, 2024
3bf0015
refactor(linter): extract create_diagnostic function for clarity
no-yan Oct 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
246 changes: 133 additions & 113 deletions crates/oxc_linter/src/rules/oxc/only_used_in_recursion.rs
View file
Open in desktop
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()));
}
Boshen marked this conversation as resolved.
Show resolved Hide resolved

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() {
camc314 marked this conversation as resolved.
Show resolved Hide resolved
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