Skip to content

Commit

Permalink
feat(linter): start fixer for no-unused-vars
Browse files Browse the repository at this point in the history
  • Loading branch information
DonIsaac committed Aug 8, 2024
1 parent 41f861f commit ff2a55a
Show file tree
Hide file tree
Showing 9 changed files with 327 additions and 26 deletions.
1 change: 1 addition & 0 deletions crates/oxc_linter/src/fixer/fix.rs
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ bitflags! {
const None = 0;
const SafeFix = Self::Fix.bits();
const DangerousFix = Self::Dangerous.bits() | Self::Fix.bits();
const DangerousSuggestion = Self::Dangerous.bits() | Self::Suggestion.bits();
/// Fixes and Suggestions that are safe or dangerous.
const All = Self::Dangerous.bits() | Self::Fix.bits() | Self::Suggestion.bits();
}
Expand Down
125 changes: 125 additions & 0 deletions crates/oxc_linter/src/rules/eslint/no_unused_vars/fixers.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,125 @@
use oxc_ast::{ast::VariableDeclarator, AstKind};
use oxc_semantic::{AstNode, AstNodeId};
use oxc_span::{CompactStr, GetSpan};
use regex::Regex;

use super::{NoUnusedVars, Symbol};
use crate::fixer::{Fix, RuleFix, RuleFixer};

impl NoUnusedVars {
pub(super) fn rename_or_remove_var_declaration<'a>(
&self,
fixer: RuleFixer<'_, 'a>,
symbol: &Symbol<'_, 'a>,
decl: &VariableDeclarator<'a>,
decl_id: AstNodeId,
) -> RuleFix<'a> {
let Some(AstKind::VariableDeclaration(declaration)) =
symbol.nodes().parent_node(decl_id).map(AstNode::kind)
else {
panic!("VariableDeclarator nodes should always be direct children of VariableDeclaration nodes");
};

// `true` even if references aren't considered a usage.
let has_references = symbol.has_references();

// we can delete variable declarations that aren't referenced anywhere
if !has_references {
let has_neighbors = declaration.declarations.len() > 1;

// `let x = 1;` the whole declaration can be removed
if !has_neighbors {
return fixer.delete(declaration).dangerously();
}

let own_position =
declaration.declarations.iter().position(|d| symbol == &d.id).expect(
"VariableDeclarator not found within its own parent VariableDeclaration",
);
let mut delete_range = decl.span();
let mut has_left = false;
let mut has_right = false;

// `let x = 1, y = 2, z = 3;` -> `let x = 1, y = 2, z = 3;`
// ^^^^^ ^^^^^^^
if let Some(right_neighbor) = declaration.declarations.get(own_position + 1) {
delete_range.end = right_neighbor.span.start;
has_right = true;
}

// `let x = 1, y = 2, z = 3;` -> `let x = 1, y = 2, z = 3;`
// ^^^^^ ^^^^^^^
if own_position > 0 {
if let Some(left_neighbor) = declaration.declarations.get(own_position - 1) {
delete_range.start = left_neighbor.span.end;
has_left = true;
}
}

// both left and right neighbors are present, so we need to
// re-insert a comma
// `let x = 1, y = 2, z = 3;`
// ^^^^^^^^^
if has_left && has_right {
return fixer.replace(delete_range, ", ").dangerously();
}

return fixer.delete(&delete_range).dangerously();
}

// otherwise, try to rename the variable to match the unused variable
// pattern
if let Some(new_name) = self.get_unused_var_name(symbol) {
return symbol.rename(&new_name).dangerously();
}

fixer.noop()
}

fn get_unused_var_name(&self, symbol: &Symbol<'_, '_>) -> Option<CompactStr> {
let pat = self.vars_ignore_pattern.as_ref().map(Regex::as_str)?;

let ignored_name: String = match pat {
// TODO: suppport more patterns

Check warning on line 83 in crates/oxc_linter/src/rules/eslint/no_unused_vars/fixers.rs

View workflow job for this annotation

GitHub Actions / Spell Check

"suppport" should be "support".
"^_" => format!("_{}", symbol.name()),
_ => return None,
};

// adjust name to avoid conflicts
let scopes = symbol.scopes();
let scope_id = symbol.scope_id();
let mut i = 0;
let mut new_name = ignored_name.clone();
while scopes.has_binding(scope_id, &new_name) {
new_name = format!("{ignored_name}{i}");
i += 1;
}

Some(new_name.into())
}
}

impl<'s, 'a> Symbol<'s, 'a> {
fn rename(&self, new_name: &CompactStr) -> RuleFix<'a> {
let mut fixes: Vec<Fix<'a>> = vec![];
let decl_span = self.symbols().get_span(self.id());
fixes.push(Fix::new(new_name.clone(), decl_span));

for reference in self.references() {
match self.nodes().get_node(reference.node_id()).kind() {
AstKind::IdentifierReference(id) => {
fixes.push(Fix::new(new_name.clone(), id.span()));
}
AstKind::TSTypeReference(ty) => {
fixes.push(Fix::new(new_name.clone(), ty.type_name.span()));
}
// we found a reference to an unknown node and we don't know how
// to replace it, so we abort the whole process
_ => return Fix::empty().into(),
}
}

return RuleFix::from(fixes)
.with_message(format!("Rename '{}' to '{new_name}'", self.name()));
}
}
15 changes: 10 additions & 5 deletions crates/oxc_linter/src/rules/eslint/no_unused_vars/mod.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
mod allowed;
mod binding_pattern;
mod diagnostic;
mod fixers;
mod ignored;
mod options;
mod symbol;
Expand Down Expand Up @@ -135,7 +136,8 @@ declare_oxc_lint!(
/// var global_var = 42;
/// ```
NoUnusedVars,
nursery
nursery,
dangerous_suggestion
);

impl Deref for NoUnusedVars {
Expand Down Expand Up @@ -207,9 +209,7 @@ impl NoUnusedVars {
| AstKind::ImportExpression(_)
| AstKind::ImportDefaultSpecifier(_)
| AstKind::ImportNamespaceSpecifier(_) => {
if !is_ignored {
ctx.diagnostic(diagnostic::imported(symbol));
}
ctx.diagnostic(diagnostic::imported(symbol));
}
AstKind::VariableDeclarator(decl) => {
if self.is_allowed_variable_declaration(symbol, decl) {
Expand All @@ -223,7 +223,12 @@ impl NoUnusedVars {
} else {
diagnostic::declared(symbol)
};
ctx.diagnostic(report);

ctx.diagnostic_with_suggestion(report, |fixer| {
// NOTE: suggestions produced by this fixer are all flagged
// as dangerous
self.rename_or_remove_var_declaration(fixer, symbol, decl, declaration.id())
});
}
AstKind::FormalParameter(param) => {
if self.is_allowed_argument(ctx.semantic().as_ref(), symbol, param) {
Expand Down
50 changes: 45 additions & 5 deletions crates/oxc_linter/src/rules/eslint/no_unused_vars/symbol.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use std::fmt;
use std::{cell::OnceCell, fmt};

use oxc_ast::{
ast::{AssignmentTarget, BindingIdentifier, BindingPattern, IdentifierReference},
Expand All @@ -8,13 +8,14 @@ use oxc_semantic::{
AstNode, AstNodeId, AstNodes, Reference, ScopeId, ScopeTree, Semantic, SymbolFlags, SymbolId,
SymbolTable,
};
use oxc_span::Span;
use oxc_span::{GetSpan, Span};

#[derive(Clone)]
pub(super) struct Symbol<'s, 'a> {
semantic: &'s Semantic<'a>,
id: SymbolId,
flags: SymbolFlags,
span: OnceCell<Span>,
}

impl PartialEq for Symbol<'_, '_> {
Expand All @@ -27,7 +28,7 @@ impl PartialEq for Symbol<'_, '_> {
impl<'s, 'a> Symbol<'s, 'a> {
pub fn new(semantic: &'s Semantic<'a>, symbol_id: SymbolId) -> Self {
let flags = semantic.symbols().get_flag(symbol_id);
Self { semantic, id: symbol_id, flags }
Self { semantic, id: symbol_id, flags, span: OnceCell::new() }
}

#[inline]
Expand All @@ -43,7 +44,11 @@ impl<'s, 'a> Symbol<'s, 'a> {
/// [`Span`] for the node declaring the [`Symbol`].
#[inline]
pub fn span(&self) -> Span {
self.symbols().get_span(self.id)
// TODO: un-comment and replace when BindingIdentifier spans are fixed
// https://github.com/oxc-project/oxc/issues/4739

// self.symbols().get_span(self.id)
*self.span.get_or_init(|| self.derive_span())
}

#[inline]
Expand All @@ -61,6 +66,13 @@ impl<'s, 'a> Symbol<'s, 'a> {
self.nodes().get_node(self.declaration_id())
}

/// Returns `true` if this symbol has any references of any kind. Does not
/// check if a references is "used" under the criteria of this rule.
#[inline]
pub fn has_references(&self) -> bool {
!self.symbols().get_resolved_reference_ids(self.id).is_empty()
}

#[inline]
pub fn references(&self) -> impl DoubleEndedIterator<Item = &Reference> + '_ {
self.symbols().get_resolved_references(self.id)
Expand Down Expand Up @@ -91,8 +103,13 @@ impl<'s, 'a> Symbol<'s, 'a> {
self.semantic.symbols()
}

#[inline]
pub fn iter_parents(&self) -> impl Iterator<Item = &AstNode<'a>> + '_ {
self.nodes().iter_parents(self.declaration_id()).skip(1)
self.iter_self_and_parents().skip(1)
}

pub fn iter_self_and_parents(&self) -> impl Iterator<Item = &AstNode<'a>> + '_ {
self.nodes().iter_parents(self.declaration_id())
}

pub fn iter_relevant_parents(
Expand Down Expand Up @@ -123,6 +140,29 @@ impl<'s, 'a> Symbol<'s, 'a> {
const fn is_relevant_kind(kind: AstKind<'a>) -> bool {
!matches!(kind, AstKind::ParenthesizedExpression(_))
}

/// <https://github.com/oxc-project/oxc/issues/4739>
fn derive_span(&self) -> Span {
for kind in self.iter_self_and_parents().map(AstNode::kind) {
match kind {
AstKind::BindingIdentifier(_) => continue,
AstKind::BindingRestElement(rest) => return rest.span,
AstKind::VariableDeclarator(decl) => return self.clean_binding_id(&decl.id),
AstKind::FormalParameter(param) => return self.clean_binding_id(&param.pattern),
_ => break,
}
}
self.symbols().get_span(self.id)
}

/// <https://github.com/oxc-project/oxc/issues/4739>
fn clean_binding_id(&self, binding: &BindingPattern) -> Span {
if binding.kind.is_destructuring_pattern() {
return self.symbols().get_span(self.id);
}
let own = binding.kind.span();
binding.type_annotation.as_ref().map_or(own, |ann| Span::new(own.start, ann.span.start))
}
}

impl<'s, 'a> Symbol<'s, 'a> {
Expand Down
52 changes: 51 additions & 1 deletion crates/oxc_linter/src/rules/eslint/no_unused_vars/tests/oxc.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
//! Test cases created by oxc maintainers

use super::NoUnusedVars;
use crate::{tester::Tester, RuleMeta as _};
use crate::{tester::Tester, FixKind, RuleMeta as _};
use serde_json::json;

#[test]
Expand All @@ -23,7 +23,56 @@ fn test_vars_simple() {
("let _a = 1", Some(json!([{ "argsIgnorePattern": "^_" }]))),
];

let fix = vec![
("let a = 1;", "", None, FixKind::DangerousSuggestion),
// FIXME: b should be deleted as well.
("let a = 1, b = 2;", "let b = 2", None, FixKind::DangerousSuggestion),
(
"let a = 1; let b: typeof a = 2; console.log(b)",
"let a = 1; let b: typeof a = 2; console.log(b)",
None,
FixKind::DangerousSuggestion,
),
(
"let a = 1; let b = 2; console.log(a);",
"let a = 1; console.log(a);",
None,
FixKind::DangerousSuggestion,
),
(
"let a = 1; let b = 2; console.log(b);",
" let b = 2; console.log(b);",
None,
FixKind::DangerousSuggestion,
),
(
"let a = 1, b = 2; console.log(b);",
"let b = 2; console.log(b);",
None,
FixKind::DangerousSuggestion,
),
(
"let a = 1, b = 2; console.log(a);",
"let a = 1; console.log(a);",
None,
FixKind::DangerousSuggestion,
),
(
"let a = 1, b = 2, c = 3; console.log(a + c);",
"let a = 1, c = 3; console.log(a + c);",
None,
FixKind::DangerousSuggestion,
),
(
"let a = 1, b = 2, c = 3; console.log(b + c);",
"let b = 2, c = 3; console.log(b + c);",
None,
FixKind::DangerousSuggestion,
),
];

Tester::new(NoUnusedVars::NAME, pass, fail)
.expect_fix(fix)
.with_snapshot_suffix("oxc-vars-simple")
.test_and_snapshot();
}
Expand Down Expand Up @@ -440,6 +489,7 @@ fn test_arguments() {
];
let fail = vec![
("function foo(a) {} foo()", None),
("function foo(a: number) {} foo()", None),
("function foo({ a }, b) { return b } foo()", Some(json!([{ "args": "after-used" }]))),
];

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,14 @@ source: crates/oxc_linter/src/tester.rs
╰────
help: Consider removing this parameter.

eslint(no-unused-vars): Parameter 'a' is declared but never used.
╭─[no_unused_vars.tsx:1:14]
1function foo(a: number) {} foo()
· ┬
· ╰── 'a' is declared here
╰────
help: Consider removing this parameter.

eslint(no-unused-vars): Parameter 'a' is declared but never used.
╭─[no_unused_vars.tsx:1:16]
1function foo({ a }, b) { return b } foo()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,8 @@ source: crates/oxc_linter/src/tester.rs
eslint(no-unused-vars): Parameter 'a' is declared but never used.
╭─[no_unused_vars.tsx:1:32]
1export class Foo { constructor(a: number) {} }
· ────┬────
· ╰── 'a' is declared here
·
· ╰── 'a' is declared here
╰────
help: Consider removing this parameter.

Expand All @@ -21,8 +21,8 @@ source: crates/oxc_linter/src/tester.rs
╭─[no_unused_vars.tsx:3:24]
2export abstract class Foo {
3public bar(a: number): string {}
· ────┬────
· ╰── 'a' is declared here
·
· ╰── 'a' is declared here
4 │ }
╰────
help: Consider removing this parameter.
Loading

0 comments on commit ff2a55a

Please sign in to comment.