From d2734f3bc46164c2228e1edf9f9c7d1a31d701d0 Mon Sep 17 00:00:00 2001 From: DonIsaac <22823424+DonIsaac@users.noreply.github.com> Date: Sat, 10 Aug 2024 15:27:11 +0000 Subject: [PATCH] feat(linter): start fixer for no-unused-vars (#4718) Start to add dangerous suggestions for `no-unused-vars`. This PR focuses on fixing unused variable declarations. - declarations with no references of any kind will be removed with a `FixKind` of dangerous suggestion. - declarations with some usages will be renamed when the rule is configured with certain `"varsIgnorePattern"`s with a `FixKind` of dangerous fix. --- crates/oxc_linter/src/fixer/fix.rs | 1 + crates/oxc_linter/src/fixer/mod.rs | 5 + .../rules/eslint/no_unused_vars/diagnostic.rs | 2 +- .../src/rules/eslint/no_unused_vars/fixers.rs | 318 ++++++++++++++++++ .../src/rules/eslint/no_unused_vars/mod.rs | 15 +- .../src/rules/eslint/no_unused_vars/symbol.rs | 62 +++- .../rules/eslint/no_unused_vars/tests/oxc.rs | 132 +++++++- .../no_unused_vars@oxc-arguments.snap | 8 + .../snapshots/no_unused_vars@oxc-classes.snap | 8 +- .../no_unused_vars@oxc-vars-destructure.snap | 50 +++ .../no_unused_vars@oxc-vars-simple.snap | 16 + .../no_unused_vars@typescript-eslint.snap | 4 +- crates/oxc_linter/src/tester.rs | 90 ++++- 13 files changed, 677 insertions(+), 34 deletions(-) create mode 100644 crates/oxc_linter/src/rules/eslint/no_unused_vars/fixers.rs create mode 100644 crates/oxc_linter/src/snapshots/no_unused_vars@oxc-vars-destructure.snap 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/fixer/mod.rs b/crates/oxc_linter/src/fixer/mod.rs index ed2bd0cbd085f..0947d08feb6a9 100644 --- a/crates/oxc_linter/src/fixer/mod.rs +++ b/crates/oxc_linter/src/fixer/mod.rs @@ -64,6 +64,11 @@ impl<'c, 'a: 'c> RuleFixer<'c, 'a> { RuleFix::new(self.kind, None, CompositeFix::Multiple(Vec::with_capacity(capacity))) } + #[inline] + pub fn source_text(&self) -> &'a str { + self.ctx.source_text() + } + /// Get a snippet of source text covered by the given [`Span`]. For details, /// see [`Span::source_text`]. #[inline] diff --git a/crates/oxc_linter/src/rules/eslint/no_unused_vars/diagnostic.rs b/crates/oxc_linter/src/rules/eslint/no_unused_vars/diagnostic.rs index 95990c749a0f1..0f5fff6b8e4c2 100644 --- a/crates/oxc_linter/src/rules/eslint/no_unused_vars/diagnostic.rs +++ b/crates/oxc_linter/src/rules/eslint/no_unused_vars/diagnostic.rs @@ -1,6 +1,6 @@ use oxc_diagnostics::OxcDiagnostic; use oxc_semantic::SymbolFlags; -use oxc_span::Span; +use oxc_span::{GetSpan, Span}; use super::Symbol; 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..f7a4113ad6def --- /dev/null +++ b/crates/oxc_linter/src/rules/eslint/no_unused_vars/fixers.rs @@ -0,0 +1,318 @@ +use oxc_ast::{ + ast::{BindingPatternKind, VariableDeclaration, VariableDeclarator}, + AstKind, +}; +use oxc_semantic::{AstNode, AstNodeId}; +use oxc_span::{CompactStr, GetSpan, Span}; +use regex::Regex; + +use super::{NoUnusedVars, Symbol}; +use crate::fixer::{Fix, RuleFix, RuleFixer}; + +impl NoUnusedVars { + #[allow(clippy::cast_possible_truncation)] + 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 { + // for `let x = 1;` or `const { x } = obj; the whole declaration can + // be removed, but for `const { x, y } = obj;` or `let x = 1, y = 2` + // we need to keep the other declarations + let has_neighbors = declaration.declarations.len() > 1; + debug_assert!(!declaration.declarations.is_empty()); + let binding_info = symbol.get_binding_info(&decl.id.kind); + + match binding_info { + BindingInfo::SingleDestructure | BindingInfo::NotDestructure => { + if has_neighbors { + return self + .delete_declarator(fixer, symbol, declaration, decl) + .dangerously(); + } + return fixer.delete(declaration).dangerously(); + } + BindingInfo::MultiDestructure(mut span, is_object, is_last) => { + let source_after = &fixer.source_text()[(span.end as usize)..]; + // remove trailing commas + span = span.expand_right(count_whitespace_or_commas(source_after.chars())); + + // remove leading commas when removing the last element in + // an array + // `const [a, b] = [1, 2];` -> `const [a, b] = [1, 2];` + // ^ ^^^ + if !is_object && is_last { + debug_assert!(span.start > 0); + let source_before = &fixer.source_text()[..(span.start as usize)]; + let chars = source_before.chars().rev(); + let start_offset = count_whitespace_or_commas(chars); + // do not walk past the beginning of the file + debug_assert!(start_offset < span.start); + span = span.expand_left(start_offset); + } + + return if is_object || is_last { + fixer.delete_range(span).dangerously() + } else { + // infix array elements need a comma to preserve + // unpacking order of symbols around them + // e.g. `const [a, b, c] = [1, 2, 3];` -> `const [a, , c] = [1, 2, 3];` + fixer.replace(span, ",").dangerously() + }; + } + BindingInfo::NotFound => { + return fixer.noop(); + } + } + } + + // 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() + } + + /// Delete a single declarator from a [`VariableDeclaration`] list with more + /// than one declarator. + #[allow(clippy::unused_self)] + fn delete_declarator<'a>( + &self, + fixer: RuleFixer<'_, 'a>, + symbol: &Symbol<'_, 'a>, + declaration: &VariableDeclaration<'a>, + decl: &VariableDeclarator<'a>, + ) -> RuleFix<'a> { + 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, ", "); + } + + return fixer.delete(&delete_range); + } + + 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: support 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.span(); + 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())); + } + + /// - `true` if `pattern` is a destructuring pattern and only contains one symbol + /// - `false` if `pattern` is a destructuring pattern and contains more than one symbol + /// - `not applicable` if `pattern` is not a destructuring pattern + fn get_binding_info(&self, pattern: &BindingPatternKind<'a>) -> BindingInfo { + match pattern { + BindingPatternKind::ArrayPattern(arr) => match arr.elements.len() { + 0 => { + debug_assert!(arr.rest.is_some()); + + BindingInfo::multi_or_single(arr.rest.as_ref().map(|r| (r.span, true)), true) + } + 1 => { + let own_span = + arr.elements.iter().filter_map(|el| el.as_ref()).find(|el| self == *el); + if let Some(rest) = arr.rest.as_ref() { + if rest.span.contains_inclusive(self.span()) { + // spreads are after all elements, otherwise it + // would be a spread element + return BindingInfo::MultiDestructure(rest.span, false, true); + } + + BindingInfo::multi_or_missing(own_span.map(|el| (el.span(), false)), false) + } else { + BindingInfo::single_or_missing(own_span.is_some()) + } + } + _ => { + let last_position = arr.elements.len() - 1; + BindingInfo::multi_or_missing( + arr.elements + .iter() + .enumerate() + .filter_map(|(idx, el)| el.as_ref().map(|el| (idx, el))) + .find_map(|(idx, el)| { + if self == el { + Some((el.span(), idx == last_position)) + } else { + None + } + }), + false, + ) + } + }, + BindingPatternKind::ObjectPattern(obj) => match obj.properties.len() { + 0 => { + debug_assert!(obj.rest.is_some()); + BindingInfo::multi_or_single(obj.rest.as_ref().map(|r| (r.span, true)), true) + } + 1 => { + let last_property = obj.properties.len() - 1; + let own_span = obj.properties.iter().enumerate().find_map(|(idx, el)| { + if self == &el.value { + Some((el.span, idx == last_property)) + } else { + None + } + }); + if let Some(rest) = obj.rest.as_ref() { + if rest.span.contains_inclusive(self.span()) { + // assume rest spreading in objects are at the end + return BindingInfo::MultiDestructure(rest.span, true, true); + } + BindingInfo::multi_or_missing(own_span, true) + } else { + BindingInfo::single_or_missing(own_span.is_some()) + } + } + _ => { + let last_property = obj.properties.len() - 1; + let own_span = obj.properties.iter().enumerate().find_map(|(idx, el)| { + (self == &el.value).then_some((el.span, idx == last_property)) + }); + BindingInfo::multi_or_missing(own_span, true) + } + }, + BindingPatternKind::AssignmentPattern(assignment) => { + self.get_binding_info(&assignment.left.kind) + } + // not in a destructure + BindingPatternKind::BindingIdentifier(_) => BindingInfo::NotDestructure, + } + } +} + +#[derive(Debug, Clone, Copy)] +enum BindingInfo { + NotDestructure, + SingleDestructure, + /// Notes: + /// 1. Symbol declaration span will cover the entire pattern, so we need + /// to extract the symbol's _exact_ span + /// 2. Unused symbols created in array destructures need to have a comma if + /// they are not in the last position. The second boolean arg indicates + /// if the pattern is an object destructure (true) or an array + /// destructure (false). When the symbol is in the last position, it + /// doesn't need a comma, which is what the third boolean arg indicates. + /// It is not used for objects. + MultiDestructure(Span, /* object? */ bool, /* last? */ bool), + /// Usually indicates a problem in the AST, though it's possible for it to + /// be a problem in the fixer + NotFound, +} + +impl BindingInfo { + #[inline] + const fn single_or_missing(found: bool) -> Self { + if found { + BindingInfo::SingleDestructure + } else { + BindingInfo::NotFound + } + } + + fn multi_or_missing(found: Option<(Span, bool)>, is_object: bool) -> Self { + match found { + Some((span, is_last)) => BindingInfo::MultiDestructure(span.span(), is_object, is_last), + None => BindingInfo::NotFound, + } + } + + fn multi_or_single(found: Option<(Span, bool)>, is_object: bool) -> Self { + match found { + Some((span, is_last)) => BindingInfo::MultiDestructure(span.span(), is_object, is_last), + None => BindingInfo::SingleDestructure, + } + } +} + +// source text will never be large enough for this usize to be truncated when +// getting cast to a u32 +#[allow(clippy::cast_possible_truncation)] +fn count_whitespace_or_commas>(iter: I) -> u32 { + iter.take_while(|c| c.is_whitespace() || *c == ',').count() as u32 +} 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..a77bf6060f639 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] @@ -40,12 +41,6 @@ impl<'s, 'a> Symbol<'s, 'a> { self.symbols().get_name(self.id) } - /// [`Span`] for the node declaring the [`Symbol`]. - #[inline] - pub fn span(&self) -> Span { - self.symbols().get_span(self.id) - } - #[inline] pub const fn flags(&self) -> SymbolFlags { self.flags @@ -61,6 +56,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 +93,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 +130,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> { @@ -176,6 +206,18 @@ impl<'s, 'a> Symbol<'s, 'a> { } } +impl GetSpan for Symbol<'_, '_> { + /// [`Span`] for the node declaring the [`Symbol`]. + #[inline] + fn span(&self) -> Span { + // 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()) + } +} + impl<'a> PartialEq> for Symbol<'_, 'a> { fn eq(&self, other: &IdentifierReference<'a>) -> bool { // cheap: no resolved reference means its a global reference 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..daed9201cbfe3 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,9 +1,31 @@ //! 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] +fn test_debug() { + let pass: Vec<&'static str> = vec![]; + let fail = vec![]; + let fix = vec![ + // ( + // "const { foo: fooBar, baz } = obj; f(baz);", + // "const { baz } = obj; f(baz);", + // None, + // FixKind::DangerousSuggestion, + // ), + ("const [a, b] = arr; f(a)", "const [a] = arr; f(a)", None, FixKind::DangerousSuggestion), + // ( + // "let x = 1; x = 2;", + // "let x = 1; x = 2;", + // Some(json!( [{ "varsIgnorePattern": "^tooCompli[cated]" }] )), + // FixKind::DangerousFix, + // ), + ]; + Tester::new(NoUnusedVars::NAME, pass, fail).expect_fix(fix).test(); +} + #[test] fn test_vars_simple() { let pass = vec![ @@ -12,18 +34,76 @@ fn test_vars_simple() { ("let a = 1; let b = a + 1; console.log(b)", None), ("let a = 1; if (true) { console.log(a) }", None), ("let _a = 1", Some(json!([{ "varsIgnorePattern": "^_" }]))), + ("const { foo: _foo, baz } = obj; f(baz);", Some(json!([{ "varsIgnorePattern": "^_" }]))), ]; let fail = vec![ ("let a = 1", None), + ("let a: number = 1", None), ("let a = 1; a = 2", None), ( "let _a = 1; console.log(_a)", Some(json!([{ "varsIgnorePattern": "^_", "reportUsedIgnorePattern": true }])), ), + ("const { foo: fooBar, baz } = obj; f(baz);", None), ("let _a = 1", Some(json!([{ "argsIgnorePattern": "^_" }]))), ]; + let fix = vec![ + // unused vars should be removed + ("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 = 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, + ), + // vars with references get renamed + ("let x = 1; x = 2;", "let _x = 1; _x = 2;", None, FixKind::DangerousFix), + ( + "let x = 1; x = 2;", + "let x = 1; x = 2;", + Some(json!( [{ "varsIgnorePattern": "^tooCompli[cated]" }] )), + FixKind::DangerousFix, + ), + // type annotations do not get clobbered + ("let x: number = 1; x = 2;", "let _x: number = 1; _x = 2;", None, FixKind::DangerousFix), + ("const { a } = obj;", "", None, FixKind::DangerousSuggestion), + ]; + Tester::new(NoUnusedVars::NAME, pass, fail) + .expect_fix(fix) .with_snapshot_suffix("oxc-vars-simple") .test_and_snapshot(); } @@ -185,7 +265,7 @@ fn test_vars_reassignment() { } #[test] -fn test_vars_destructure_ignored() { +fn test_vars_destructure() { let pass = vec![ // ("const { a, ...rest } = obj; console.log(rest)", Some(json![{ "ignoreRestSiblings": true }])) ]; @@ -198,8 +278,53 @@ fn test_vars_destructure_ignored() { ), ]; + let fix = vec![ + ("const { a } = obj;", "", None, FixKind::DangerousSuggestion), + ("const [a] = arr;", "", None, FixKind::DangerousSuggestion), + ( + "const { a, b } = obj; f(b)", + "const { b } = obj; f(b)", + None, + FixKind::DangerousSuggestion, + ), + ("const [a, b] = arr; f(b)", "const [,b] = arr; f(b)", None, FixKind::DangerousSuggestion), + ("const [a, b] = arr; f(a)", "const [a] = arr; f(a)", None, FixKind::DangerousSuggestion), + ( + "const [a, b, c] = arr; f(a, c)", + "const [a, ,c] = arr; f(a, c)", + None, + FixKind::DangerousSuggestion, + ), + ( + "const [a, b, c, d, e] = arr; f(a, e)", + "const [a, ,,,e] = arr; f(a, e)", + None, + FixKind::DangerousSuggestion, + ), + ( + "const [a, b, c, d, e, f] = arr; fn(a, e)", + "const [a, ,,,e] = arr; fn(a, e)", + None, + FixKind::DangerousSuggestion, + ), + ( + "const { foo: fooBar, baz } = obj; f(baz);", + "const { baz } = obj; f(baz);", + None, + FixKind::DangerousSuggestion, + ), + // renaming + // ( + // "let a = 1; a = 2;", + // "let _a = 1; _a = 2;", + // Some(json!([{ "varsIgnorePattern": "^_" }])), + // FixKind::DangerousSuggestion, + // ), + ]; + Tester::new(NoUnusedVars::NAME, pass, fail) - .with_snapshot_suffix("oxc-vars-destructure-ignored") + .expect_fix(fix) + .with_snapshot_suffix("oxc-vars-destructure") .test_and_snapshot(); } @@ -440,6 +565,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@oxc-vars-destructure.snap b/crates/oxc_linter/src/snapshots/no_unused_vars@oxc-vars-destructure.snap new file mode 100644 index 0000000000000..81cb1df9e6c8f --- /dev/null +++ b/crates/oxc_linter/src/snapshots/no_unused_vars@oxc-vars-destructure.snap @@ -0,0 +1,50 @@ +--- +source: crates/oxc_linter/src/tester.rs +--- + ⚠ eslint(no-unused-vars): Variable 'a' is declared but never used. + ╭─[no_unused_vars.tsx:1:9] + 1 │ const { a, ...rest } = obj + · ┬ + · ╰── 'a' is declared here + ╰──── + help: Consider removing this declaration. + + ⚠ eslint(no-unused-vars): Variable 'rest' is declared but never used. + ╭─[no_unused_vars.tsx:1:15] + 1 │ const { a, ...rest } = obj + · ──┬─ + · ╰── 'rest' is declared here + ╰──── + help: Consider removing this declaration. + + ⚠ eslint(no-unused-vars): Variable 'a' is declared but never used. + ╭─[no_unused_vars.tsx:1:8] + 1 │ const [a, ...rest] = arr + · ┬ + · ╰── 'a' is declared here + ╰──── + help: Consider removing this declaration. + + ⚠ eslint(no-unused-vars): Variable 'rest' is declared but never used. + ╭─[no_unused_vars.tsx:1:14] + 1 │ const [a, ...rest] = arr + · ──┬─ + · ╰── 'rest' is declared here + ╰──── + help: Consider removing this declaration. + + ⚠ eslint(no-unused-vars): Variable 'b' is declared but never used. + ╭─[no_unused_vars.tsx:1:14] + 1 │ const { a: { b }, ...rest } = obj; console.log(a) + · ┬ + · ╰── 'b' is declared here + ╰──── + help: Consider removing this declaration. + + ⚠ eslint(no-unused-vars): Variable 'rest' is declared but never used. + ╭─[no_unused_vars.tsx:1:22] + 1 │ const { a: { b }, ...rest } = obj; console.log(a) + · ──┬─ + · ╰── 'rest' is declared here + ╰──── + help: Consider removing this declaration. diff --git a/crates/oxc_linter/src/snapshots/no_unused_vars@oxc-vars-simple.snap b/crates/oxc_linter/src/snapshots/no_unused_vars@oxc-vars-simple.snap index b841860f1470c..cabd463aba758 100644 --- a/crates/oxc_linter/src/snapshots/no_unused_vars@oxc-vars-simple.snap +++ b/crates/oxc_linter/src/snapshots/no_unused_vars@oxc-vars-simple.snap @@ -9,6 +9,14 @@ source: crates/oxc_linter/src/tester.rs ╰──── help: Consider removing this declaration. + ⚠ eslint(no-unused-vars): Variable 'a' is declared but never used. + ╭─[no_unused_vars.tsx:1:5] + 1 │ let a: number = 1 + · ┬ + · ╰── 'a' is declared here + ╰──── + help: Consider removing this declaration. + ⚠ eslint(no-unused-vars): Variable 'a' is assigned a value but never used. ╭─[no_unused_vars.tsx:1:5] 1 │ let a = 1; a = 2 @@ -26,6 +34,14 @@ source: crates/oxc_linter/src/tester.rs ╰──── help: Consider renaming this variable. + ⚠ eslint(no-unused-vars): Variable 'fooBar' is declared but never used. + ╭─[no_unused_vars.tsx:1:14] + 1 │ const { foo: fooBar, baz } = obj; f(baz); + · ───┬── + · ╰── 'fooBar' is declared here + ╰──── + help: Consider removing this declaration. + ⚠ eslint(no-unused-vars): Variable '_a' is declared but never used. ╭─[no_unused_vars.tsx:1:5] 1 │ let _a = 1 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..c599a84205642 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::Any, + 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::Any, + 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()); }