From ff2a55abb36ffccb0b8e803629008bfaa316c5d6 Mon Sep 17 00:00:00 2001 From: Don Isaac Date: Tue, 6 Aug 2024 22:24:07 -0400 Subject: [PATCH] feat(linter): start fixer for no-unused-vars --- crates/oxc_linter/src/fixer/fix.rs | 1 + .../src/rules/eslint/no_unused_vars/fixers.rs | 125 ++++++++++++++++++ .../src/rules/eslint/no_unused_vars/mod.rs | 15 ++- .../src/rules/eslint/no_unused_vars/symbol.rs | 50 ++++++- .../rules/eslint/no_unused_vars/tests/oxc.rs | 52 +++++++- .../no_unused_vars@oxc-arguments.snap | 8 ++ .../snapshots/no_unused_vars@oxc-classes.snap | 8 +- .../no_unused_vars@typescript-eslint.snap | 4 +- crates/oxc_linter/src/tester.rs | 90 +++++++++++-- 9 files changed, 327 insertions(+), 26 deletions(-) create mode 100644 crates/oxc_linter/src/rules/eslint/no_unused_vars/fixers.rs diff --git a/crates/oxc_linter/src/fixer/fix.rs b/crates/oxc_linter/src/fixer/fix.rs index abd4cccddcb5d..af0696ed8afb9 100644 --- a/crates/oxc_linter/src/fixer/fix.rs +++ b/crates/oxc_linter/src/fixer/fix.rs @@ -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(); } diff --git a/crates/oxc_linter/src/rules/eslint/no_unused_vars/fixers.rs b/crates/oxc_linter/src/rules/eslint/no_unused_vars/fixers.rs new file mode 100644 index 0000000000000..a0bbec0f449d3 --- /dev/null +++ b/crates/oxc_linter/src/rules/eslint/no_unused_vars/fixers.rs @@ -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 { + let pat = self.vars_ignore_pattern.as_ref().map(Regex::as_str)?; + + let ignored_name: String = match pat { + // TODO: suppport more patterns + "^_" => 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> = 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())); + } +} diff --git a/crates/oxc_linter/src/rules/eslint/no_unused_vars/mod.rs b/crates/oxc_linter/src/rules/eslint/no_unused_vars/mod.rs index e4377114d28c4..206fd8872c1c0 100644 --- a/crates/oxc_linter/src/rules/eslint/no_unused_vars/mod.rs +++ b/crates/oxc_linter/src/rules/eslint/no_unused_vars/mod.rs @@ -1,6 +1,7 @@ mod allowed; mod binding_pattern; mod diagnostic; +mod fixers; mod ignored; mod options; mod symbol; @@ -135,7 +136,8 @@ declare_oxc_lint!( /// var global_var = 42; /// ``` NoUnusedVars, - nursery + nursery, + dangerous_suggestion ); impl Deref for NoUnusedVars { @@ -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) { @@ -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) { diff --git a/crates/oxc_linter/src/rules/eslint/no_unused_vars/symbol.rs b/crates/oxc_linter/src/rules/eslint/no_unused_vars/symbol.rs index b61b93fe04ccb..67df42249e860 100644 --- a/crates/oxc_linter/src/rules/eslint/no_unused_vars/symbol.rs +++ b/crates/oxc_linter/src/rules/eslint/no_unused_vars/symbol.rs @@ -1,4 +1,4 @@ -use std::fmt; +use std::{cell::OnceCell, fmt}; use oxc_ast::{ ast::{AssignmentTarget, BindingIdentifier, BindingPattern, IdentifierReference}, @@ -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, } impl PartialEq for Symbol<'_, '_> { @@ -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] @@ -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] @@ -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 + '_ { self.symbols().get_resolved_references(self.id) @@ -91,8 +103,13 @@ impl<'s, 'a> Symbol<'s, 'a> { self.semantic.symbols() } + #[inline] pub fn iter_parents(&self) -> impl Iterator> + '_ { - 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> + '_ { + self.nodes().iter_parents(self.declaration_id()) } pub fn iter_relevant_parents( @@ -123,6 +140,29 @@ impl<'s, 'a> Symbol<'s, 'a> { const fn is_relevant_kind(kind: AstKind<'a>) -> bool { !matches!(kind, AstKind::ParenthesizedExpression(_)) } + + /// + 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(¶m.pattern), + _ => break, + } + } + self.symbols().get_span(self.id) + } + + /// + 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> { diff --git a/crates/oxc_linter/src/rules/eslint/no_unused_vars/tests/oxc.rs b/crates/oxc_linter/src/rules/eslint/no_unused_vars/tests/oxc.rs index 13f831ad63555..93044f12134b7 100644 --- a/crates/oxc_linter/src/rules/eslint/no_unused_vars/tests/oxc.rs +++ b/crates/oxc_linter/src/rules/eslint/no_unused_vars/tests/oxc.rs @@ -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] @@ -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(); } @@ -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" }]))), ]; diff --git a/crates/oxc_linter/src/snapshots/no_unused_vars@oxc-arguments.snap b/crates/oxc_linter/src/snapshots/no_unused_vars@oxc-arguments.snap index e9ecc13c0a0bf..87d44d095ec13 100644 --- a/crates/oxc_linter/src/snapshots/no_unused_vars@oxc-arguments.snap +++ b/crates/oxc_linter/src/snapshots/no_unused_vars@oxc-arguments.snap @@ -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] + 1 │ function 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] 1 │ function foo({ a }, b) { return b } foo() diff --git a/crates/oxc_linter/src/snapshots/no_unused_vars@oxc-classes.snap b/crates/oxc_linter/src/snapshots/no_unused_vars@oxc-classes.snap index 7ac81ac172fb6..8816cb29f5f9a 100644 --- a/crates/oxc_linter/src/snapshots/no_unused_vars@oxc-classes.snap +++ b/crates/oxc_linter/src/snapshots/no_unused_vars@oxc-classes.snap @@ -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] 1 │ export class Foo { constructor(a: number) {} } - · ────┬──── - · ╰── 'a' is declared here + · ┬ + · ╰── 'a' is declared here ╰──── help: Consider removing this parameter. @@ -21,8 +21,8 @@ source: crates/oxc_linter/src/tester.rs ╭─[no_unused_vars.tsx:3:24] 2 │ export abstract class Foo { 3 │ public bar(a: number): string {} - · ────┬──── - · ╰── 'a' is declared here + · ┬ + · ╰── 'a' is declared here 4 │ } ╰──── help: Consider removing this parameter. diff --git a/crates/oxc_linter/src/snapshots/no_unused_vars@typescript-eslint.snap b/crates/oxc_linter/src/snapshots/no_unused_vars@typescript-eslint.snap index e1f771ecea33a..d9b7ed687228b 100644 --- a/crates/oxc_linter/src/snapshots/no_unused_vars@typescript-eslint.snap +++ b/crates/oxc_linter/src/snapshots/no_unused_vars@typescript-eslint.snap @@ -336,7 +336,7 @@ source: crates/oxc_linter/src/tester.rs ⚠ eslint(no-unused-vars): Variable 'foo' is declared but never used. ╭─[no_unused_vars.ts:1:7] 1 │ const foo: number = 1; - · ─────┬───── - · ╰── 'foo' is declared here + · ─┬─ + · ╰── 'foo' is declared here ╰──── help: Consider removing this declaration. diff --git a/crates/oxc_linter/src/tester.rs b/crates/oxc_linter/src/tester.rs index 46c602950a9f7..4fc01a1bdd099 100644 --- a/crates/oxc_linter/src/tester.rs +++ b/crates/oxc_linter/src/tester.rs @@ -65,24 +65,96 @@ impl From<(&str, Option, Option, Option)> for TestCase { } } +#[derive(Debug, Default, Clone, Copy, PartialEq, Eq)] +pub enum ExpectFixKind { + /// We expect no fix to be applied + #[default] + None, + /// We expect some fix to be applied, but don't care what kind it is + Any, + /// We expect a fix of a certain [`FixKind`] to be applied + Specific(FixKind), +} + +impl ExpectFixKind { + #[inline] + pub fn is_none(self) -> bool { + matches!(self, Self::None) + } + + #[inline] + pub fn is_some(self) -> bool { + !self.is_none() + } +} + +impl From for ExpectFixKind { + fn from(kind: FixKind) -> Self { + Self::Specific(kind) + } +} +impl From for FixKind { + fn from(expected_kind: ExpectFixKind) -> Self { + match expected_kind { + ExpectFixKind::None => FixKind::None, + ExpectFixKind::Any => FixKind::All, + ExpectFixKind::Specific(kind) => kind, + } + } +} + +impl From> for ExpectFixKind { + fn from(maybe_kind: Option) -> Self { + match maybe_kind { + Some(kind) => Self::Specific(kind), + None => Self::Any, // intentionally not None + } + } +} + #[derive(Debug, Clone)] pub struct ExpectFix { /// Source code being tested source: String, /// Expected source code after fix has been applied expected: String, + kind: ExpectFixKind, rule_config: Option, } impl> From<(S, S, Option)> for ExpectFix { fn from(value: (S, S, Option)) -> Self { - Self { source: value.0.into(), expected: value.1.into(), rule_config: value.2 } + Self { + source: value.0.into(), + expected: value.1.into(), + kind: ExpectFixKind::None, + rule_config: value.2, + } } } impl> From<(S, S)> for ExpectFix { fn from(value: (S, S)) -> Self { - Self { source: value.0.into(), expected: value.1.into(), rule_config: None } + Self { + source: value.0.into(), + expected: value.1.into(), + kind: ExpectFixKind::None, + rule_config: None, + } + } +} +impl From<(S, S, Option, F)> for ExpectFix +where + S: Into, + F: Into, +{ + fn from((source, expected, config, kind): (S, S, Option, F)) -> Self { + Self { + source: source.into(), + expected: expected.into(), + kind: kind.into(), + rule_config: config, + } } } @@ -237,7 +309,7 @@ impl Tester { fn test_pass(&mut self) { for TestCase { source, rule_config, eslint_config, path } in self.expect_pass.clone() { - let result = self.run(&source, rule_config, &eslint_config, path, false); + let result = self.run(&source, rule_config, &eslint_config, path, ExpectFixKind::None); let passed = result == TestResult::Passed; assert!(passed, "expect test to pass: {source} {}", self.snapshot); } @@ -245,7 +317,7 @@ impl Tester { fn test_fail(&mut self) { for TestCase { source, rule_config, eslint_config, path } in self.expect_fail.clone() { - let result = self.run(&source, rule_config, &eslint_config, path, false); + let result = self.run(&source, rule_config, &eslint_config, path, ExpectFixKind::None); let failed = result == TestResult::Failed; assert!(failed, "expect test to fail: {source}"); } @@ -253,8 +325,8 @@ impl Tester { fn test_fix(&mut self) { for fix in self.expect_fix.clone() { - let ExpectFix { source, expected, rule_config: config } = fix; - let result = self.run(&source, config, &None, None, true); + let ExpectFix { source, expected, kind, rule_config: config } = fix; + let result = self.run(&source, config, &None, None, kind); match result { TestResult::Fixed(fixed_str) => assert_eq!( expected, fixed_str, @@ -272,12 +344,12 @@ impl Tester { rule_config: Option, eslint_config: &Option, path: Option, - is_fix: bool, + fix: ExpectFixKind, ) -> TestResult { let allocator = Allocator::default(); let rule = self.find_rule().read_json(rule_config.unwrap_or_default()); let options = LintOptions::default() - .with_fix(is_fix.then_some(FixKind::All).unwrap_or_default()) + .with_fix(fix.into()) .with_import_plugin(self.import_plugin) .with_jest_plugin(self.jest_plugin) .with_vitest_plugin(self.vitest_plugin) @@ -312,7 +384,7 @@ impl Tester { return TestResult::Passed; } - if is_fix { + if fix.is_some() { let fix_result = Fixer::new(source_text, result).fix(); return TestResult::Fixed(fix_result.fixed_code.to_string()); }