Skip to content

Commit

Permalink
refactor(linter/no-unused-vars): improve implementation to remove usi…
Browse files Browse the repository at this point in the history
…ng SymbolFlags::Export
  • Loading branch information
Dunqing committed Nov 22, 2024
1 parent 130a6df commit e0e6329
Show file tree
Hide file tree
Showing 6 changed files with 31 additions and 114 deletions.
97 changes: 21 additions & 76 deletions crates/oxc_linter/src/rules/eslint/no_unused_vars/allowed.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,84 +2,35 @@
//! consider variables ignored by name pattern, but by where they are declared.
use oxc_ast::{ast::*, AstKind};
use oxc_semantic::{NodeId, Semantic};
use oxc_span::GetSpan;

use super::{options::ArgsOption, NoUnusedVars, Symbol};
use crate::rules::eslint::no_unused_vars::binding_pattern::{BindingContext, HasAnyUsedBinding};

impl<'s, 'a> Symbol<'s, 'a> {
/// Returns `true` if this function is use.
/// Check if the declaration of this [`Symbol`] is use.
///
/// Checks for these cases
/// 1. passed as a callback to another [`CallExpression`] or [`NewExpression`]
/// 2. invoked as an IIFE
/// 3. Returned from another function
/// 4. Used as an attribute in a JSX element
/// If it's an expression, then it's always passed in as an argument
/// or assigned to a variable, so it's always used.
///
/// ```js
/// // True:
/// const a = class Name{}
/// export default (function Name() {})
/// console.log(function Name() {})
///
/// // False
/// function foo() {}
/// {
/// class Foo {}
/// }
/// ```
#[inline]
pub fn is_function_or_class_declaration_used(&self) -> bool {
#[cfg(debug_assertions)]
{
let kind = self.declaration().kind();
assert!(kind.is_function_like() || matches!(kind, AstKind::Class(_)));
}

for parent in self.iter_relevant_parents() {
match parent.kind() {
AstKind::MemberExpression(_) | AstKind::ParenthesizedExpression(_)
// e.g. `const x = [function foo() {}]`
// Only considered used if the array containing the symbol is used.
| AstKind::ArrayExpressionElement(_)
| AstKind::ArrayExpression(_)
// a ? b : function foo() {}
// Only considered used if the function is the test or the selected branch,
// but we can't determine that here.
| AstKind::ConditionalExpression(_)
=> {
continue;
}
// Returned from another function. Definitely won't be the same
// function because we're walking up from its declaration
AstKind::ReturnStatement(_)
// <Component onClick={function onClick(e) { }} />
| AstKind::JSXExpressionContainer(_)
// Function declaration is passed as an argument to another function.
| AstKind::CallExpression(_) | AstKind::Argument(_)
// e.g. `const x = { foo: function foo() {} }`
// Allowed off-the-bat since objects being the only child of an
// ExpressionStatement is rare, since you would need to wrap the
// object in parentheses to avoid creating a block statement.
| AstKind::ObjectProperty(_)
// e.g. var foo = function bar() { }
// we don't want to check for violations on `bar`, just `foo`
| AstKind::VariableDeclarator(_)
// new (class CustomRenderer{})
// new (function() {})
| AstKind::NewExpression(_)
=> {
return true;
}
// !function() {}; is an IIFE
AstKind::UnaryExpression(expr) => return expr.operator.is_not(),
// function is used as a value for an assignment
// e.g. Array.prototype.sort ||= function sort(a, b) { }
AstKind::AssignmentExpression(assignment) if assignment.right.span().contains_inclusive(self.span()) => {
return self != &assignment.left;
}
AstKind::ExpressionStatement(_) => {
// implicit return in arrow function expression
let Some(AstKind::FunctionBody(body)) = self.nodes().parent_kind(parent.id()) else {
return false;
};
return body.span.contains_inclusive(self.span()) && body.statements.len() == 1 && !self.get_snippet(body.span).starts_with('{')
}
_ => {
parent.kind().debug_name();
return false;
}
}
pub(crate) fn is_function_or_class_declaration_used(&self) -> bool {
match self.declaration().kind() {
AstKind::Class(class) => class.is_expression(),
AstKind::Function(func) => func.is_expression(),
_ => false,
}

false
}

fn is_declared_in_for_of_loop(&self) -> bool {
Expand Down Expand Up @@ -124,12 +75,6 @@ fn is_ambient_namespace(namespace: &TSModuleDeclaration) -> bool {
}

impl NoUnusedVars {
#[allow(clippy::unused_self)]
pub(super) fn is_allowed_class_or_function(&self, symbol: &Symbol<'_, '_>) -> bool {
symbol.is_function_or_class_declaration_used()
// || symbol.is_function_or_class_assigned_to_same_name_variable()
}

#[allow(clippy::unused_self)]
pub(super) fn is_allowed_ts_namespace<'a>(
&self,
Expand Down
9 changes: 1 addition & 8 deletions crates/oxc_linter/src/rules/eslint/no_unused_vars/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -232,8 +232,7 @@ impl NoUnusedVars {
}

// Order matters. We want to call cheap/high "yield" functions first.
let is_exported = symbol.is_exported();
let is_used = is_exported || symbol.has_usages(self);
let is_used = symbol.is_exported() || symbol.has_usages(self);

match (is_used, is_ignored) {
(true, true) => {
Expand Down Expand Up @@ -306,12 +305,6 @@ impl NoUnusedVars {
}
ctx.diagnostic(diagnostic::declared(symbol, &self.vars_ignore_pattern));
}
AstKind::Class(_) | AstKind::Function(_) => {
if self.is_allowed_class_or_function(symbol) {
return;
}
ctx.diagnostic(diagnostic::declared(symbol, &IgnorePattern::<&str>::None));
}
AstKind::TSModuleDeclaration(namespace) => {
if self.is_allowed_ts_namespace(symbol, namespace) {
return;
Expand Down
14 changes: 4 additions & 10 deletions crates/oxc_linter/src/rules/eslint/no_unused_vars/symbol.rs
Original file line number Diff line number Diff line change
Expand Up @@ -105,11 +105,6 @@ impl<'s, 'a> Symbol<'s, 'a> {
self.nodes().ancestors(self.declaration_id())
}

#[inline]
pub fn iter_relevant_parents(&self) -> impl Iterator<Item = &AstNode<'a>> + Clone + '_ {
self.iter_relevant_parents_of(self.declaration_id())
}

pub fn iter_relevant_parents_of(
&self,
node_id: NodeId,
Expand Down Expand Up @@ -176,10 +171,9 @@ impl<'s, 'a> Symbol<'s, 'a> {
/// NOTE: does not support CJS right now.
pub fn is_exported(&self) -> bool {
let is_in_exportable_scope = self.is_root() || self.is_in_ts_namespace();
(is_in_exportable_scope
&& (self.flags.contains(SymbolFlags::Export)
|| self.semantic.module_record().exported_bindings.contains_key(self.name())))
|| self.in_export_node()
is_in_exportable_scope
&& (self.semantic.module_record().exported_bindings.contains_key(self.name())
|| self.in_export_node())
}

#[inline]
Expand All @@ -194,7 +188,7 @@ impl<'s, 'a> Symbol<'s, 'a> {
AstKind::ModuleDeclaration(module) => {
return module.is_export();
}
AstKind::ExportDefaultDeclaration(_) => {
AstKind::ExportNamedDeclaration(_) | AstKind::ExportDefaultDeclaration(_) => {
return true;
}
AstKind::VariableDeclaration(_)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -819,11 +819,7 @@ fn test_used_declarations() {
"export const Foo = class Bar {}",
"export const Foo = @SomeDecorator() class Foo {}",
];
let fail = vec![
// array is not used, so the function is not used
";[function foo() {}]",
";[class Foo {}]",
];
let fail = vec![];

Tester::new(NoUnusedVars::NAME, pass, fail)
.intentionally_allow_no_fix_tests()
Expand Down
4 changes: 4 additions & 0 deletions crates/oxc_linter/src/rules/eslint/no_unused_vars/usage.rs
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,10 @@ impl<'s, 'a> Symbol<'s, 'a> {

/// Check if this [`Symbol`] has any [`Reference`]s that are considered a usage.
pub fn has_usages(&self, options: &NoUnusedVars) -> bool {
if self.is_function_or_class_declaration_used() {
return true;
}

// Use symbol flags to skip the usage checks we are certain don't need
// to be run.
let do_reassignment_checks = self.is_possibly_reassignable();
Expand Down
15 changes: 0 additions & 15 deletions crates/oxc_linter/src/snapshots/[email protected]
Original file line number Diff line number Diff line change
@@ -1,19 +1,4 @@
---
source: crates/oxc_linter/src/tester.rs
snapshot_kind: text
---
eslint(no-unused-vars): Function 'foo' is declared but never used.
╭─[no_unused_vars.tsx:1:12]
1 │ ;[function foo() {}]
· ─┬─
· ╰── 'foo' is declared here
╰────
help: Consider removing this declaration.

eslint(no-unused-vars): Class 'Foo' is declared but never used.
╭─[no_unused_vars.tsx:1:9]
1 │ ;[class Foo {}]
· ─┬─
· ╰── 'Foo' is declared here
╰────
help: Consider removing this declaration.

0 comments on commit e0e6329

Please sign in to comment.