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.

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

feat(linter): add eslint/no-unused-vars (⭐ attempt 3.2) #4445

Merged
merged 1 commit into from
Jul 31, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
3 changes: 3 additions & 0 deletions .typos.toml
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,9 @@ extend-exclude = [
"**/*.snap",
"**/*/CHANGELOG.md",
"crates/oxc_linter/fixtures",
"crates/oxc_linter/src/rules/eslint/no_unused_vars/ignored.rs",
"crates/oxc_linter/src/rules/eslint/no_unused_vars/options.rs",
"crates/oxc_linter/src/rules/eslint/no_unused_vars/tests/eslint.rs",
"crates/oxc_linter/src/rules/jsx_a11y/aria_props.rs",
"crates/oxc_linter/src/rules/jsx_a11y/img_redundant_alt.rs",
"crates/oxc_linter/src/rules/react/no_unknown_property.rs",
Expand Down
6 changes: 3 additions & 3 deletions apps/oxlint/src/lint/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -427,7 +427,7 @@ mod test {
];
let result = test(args);
assert_eq!(result.number_of_files, 1);
assert_eq!(result.number_of_warnings, 2);
assert_eq!(result.number_of_warnings, 3);
assert_eq!(result.number_of_errors, 0);
}

Expand All @@ -441,7 +441,7 @@ mod test {
];
let result = test(args);
assert_eq!(result.number_of_files, 1);
assert_eq!(result.number_of_warnings, 1);
assert_eq!(result.number_of_warnings, 2);
assert_eq!(result.number_of_errors, 0);
}

Expand Down Expand Up @@ -477,7 +477,7 @@ mod test {
let args = &["fixtures/svelte/debugger.svelte"];
let result = test(args);
assert_eq!(result.number_of_files, 1);
assert_eq!(result.number_of_warnings, 1);
assert_eq!(result.number_of_warnings, 2);
assert_eq!(result.number_of_errors, 0);
}

Expand Down
5 changes: 5 additions & 0 deletions crates/oxc_ast/src/ast_impl/js.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1203,6 +1203,11 @@ impl<'a> Hash for Class<'a> {
}

impl<'a> ClassElement<'a> {
/// Returns `true` if this is a [`ClassElement::StaticBlock`].
pub fn is_static_block(&self) -> bool {
matches!(self, Self::StaticBlock(_))
}

/// Returns `true` if this [`ClassElement`] is a static block or has a
/// static modifier.
pub fn r#static(&self) -> bool {
Expand Down
2 changes: 2 additions & 0 deletions crates/oxc_linter/src/rules.rs
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,7 @@ mod eslint {
pub mod no_unsafe_optional_chaining;
pub mod no_unused_labels;
pub mod no_unused_private_class_members;
pub mod no_unused_vars;
pub mod no_useless_catch;
pub mod no_useless_concat;
pub mod no_useless_constructor;
Expand Down Expand Up @@ -526,6 +527,7 @@ oxc_macros::declare_all_lint_rules! {
eslint::no_unsafe_negation,
eslint::no_unsafe_optional_chaining,
eslint::no_unused_labels,
eslint::no_unused_vars,
eslint::no_unused_private_class_members,
eslint::no_useless_catch,
eslint::no_useless_escape,
Expand Down
290 changes: 290 additions & 0 deletions crates/oxc_linter/src/rules/eslint/no_unused_vars/allowed.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,290 @@
//! This module checks if an unused variable is allowed. Note that this does not
//! consider variables ignored by name pattern, but by where they are declared.
#[allow(clippy::wildcard_imports)]
use oxc_ast::{ast::*, AstKind};
use oxc_semantic::{AstNode, AstNodeId, Semantic};
use oxc_span::GetSpan;

use crate::rules::eslint::no_unused_vars::binding_pattern::{BindingContext, HasAnyUsedBinding};

use super::{options::ArgsOption, NoUnusedVars, Symbol};

impl<'s, 'a> Symbol<'s, 'a> {
/// Returns `true` if this function 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
#[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_parents() {
match parent.kind() {
AstKind::MemberExpression(_) | AstKind::ParenthesizedExpression(_) => {
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() {} }`
| AstKind::ObjectProperty(_)
// e.g. var foo = function bar() { }
// we don't want to check for violations on `bar`, just `foo`
| AstKind::VariableDeclarator(_)
=> {
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;
}
_ => {
return false;
}
}
}

false
}

fn is_declared_in_for_of_loop(&self) -> bool {
for parent in self.iter_parents() {
match parent.kind() {
AstKind::ParenthesizedExpression(_)
| AstKind::VariableDeclaration(_)
| AstKind::BindingIdentifier(_)
| AstKind::SimpleAssignmentTarget(_)
| AstKind::AssignmentTarget(_) => continue,
AstKind::ForInStatement(ForInStatement { body, .. })
| AstKind::ForOfStatement(ForOfStatement { body, .. }) => match body {
Statement::ReturnStatement(_) => return true,
Statement::BlockStatement(b) => {
return b
.body
.first()
.is_some_and(|s| matches!(s, Statement::ReturnStatement(_)))
}
_ => return false,
},
_ => return false,
}
}

false
}

pub fn is_in_declared_module(&self) -> bool {
let scopes = self.scopes();
let nodes = self.nodes();
scopes.ancestors(self.scope_id())
.map(|scope_id| scopes.get_node_id(scope_id))
.map(|node_id| nodes.get_node(node_id))
.any(|node| matches!(node.kind(), AstKind::TSModuleDeclaration(namespace) if is_ambient_namespace(namespace)))
}
}

#[inline]
fn is_ambient_namespace(namespace: &TSModuleDeclaration) -> bool {
namespace.declare || namespace.kind.is_global()
}

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,
symbol: &Symbol<'_, 'a>,
namespace: &TSModuleDeclaration<'a>,
) -> bool {
if is_ambient_namespace(namespace) {
return true;
}
symbol.is_in_declared_module()
}

/// Returns `true` if this unused variable declaration should be allowed
/// (i.e. not reported)
pub(super) fn is_allowed_variable_declaration<'a>(
&self,
symbol: &Symbol<'_, 'a>,
decl: &VariableDeclarator<'a>,
) -> bool {
if decl.kind.is_var() && self.vars.is_local() && symbol.is_root() {
return true;
}

// allow unused iterators, since they're required for valid syntax
if symbol.is_declared_in_for_of_loop() {
return true;
}

false
}

#[allow(clippy::unused_self)]
pub(super) fn is_allowed_type_parameter(
&self,
symbol: &Symbol<'_, '_>,
declaration_id: AstNodeId,
) -> bool {
matches!(symbol.nodes().parent_kind(declaration_id), Some(AstKind::TSMappedType(_)))
}

/// Returns `true` if this unused parameter should be allowed (i.e. not
/// reported)
pub(super) fn is_allowed_argument<'a>(
&self,
semantic: &Semantic<'a>,
symbol: &Symbol<'_, 'a>,
param: &FormalParameter<'a>,
) -> bool {
// early short-circuit when no argument checking should be performed
if self.args.is_none() {
return true;
}

// find FormalParameters. Should be the next parent of param, but this
// is safer.
let Some((params, params_id)) = symbol.iter_parents().find_map(|p| {
if let AstKind::FormalParameters(params) = p.kind() {
Some((params, p.id()))
} else {
None
}
}) else {
debug_assert!(false, "FormalParameter should always have a parent FormalParameters");
return false;
};

if Self::is_allowed_param_because_of_method(semantic, param, params_id) {
return true;
}

// Parameters are always checked. Must be done after above checks,
// because in those cases a parameter is required. However, even if
// `args` is `all`, it may be ignored using `ignoreRestSiblings` or `destructuredArrayIgnorePattern`.
if self.args.is_all() {
return false;
}

debug_assert_eq!(self.args, ArgsOption::AfterUsed);

// from eslint rule documentation:
// after-used - unused positional arguments that occur before the last
// used argument will not be checked, but all named arguments and all
// positional arguments after the last used argument will be checked.

// unused non-positional arguments are never allowed
if param.pattern.kind.is_destructuring_pattern() {
return false;
}

// find the index of the parameter in the parameters list. We want to
// check all parameters after this one for usages.
let position =
params.items.iter().enumerate().find(|(_, p)| p.span == param.span).map(|(i, _)| i);
debug_assert!(
position.is_some(),
"could not find FormalParameter in a FormalParameters node that is its parent."
);
let Some(position) = position else {
return false;
};

// This is the last parameter, so need to check for usages on following parameters
if position == params.items.len() - 1 {
return false;
}

let ctx = BindingContext { options: self, semantic };
params
.items
.iter()
.skip(position + 1)
// has_modifier() to handle:
// constructor(unused: number, public property: string) {}
// no need to check if param is in a constructor, because if it's
// not that's a parse error.
.any(|p| p.has_modifier() || p.pattern.has_any_used_binding(ctx))
}

/// `params_id` is the [`AstNodeId`] to a [`AstKind::FormalParameters`] node.
///
/// The following allowed conditions are handled:
/// 1. setter parameters - removing them causes a syntax error.
/// 2. TS constructor property definitions - they declare class members.
fn is_allowed_param_because_of_method<'a>(
semantic: &Semantic<'a>,
param: &FormalParameter<'a>,
params_id: AstNodeId,
) -> bool {
let mut parents_iter = semantic.nodes().iter_parents(params_id).skip(1).map(AstNode::kind);

// in function declarations, the parent immediately before the
// FormalParameters is a TSDeclareBlock
let Some(parent) = parents_iter.next() else {
return false;
};
if matches!(parent, AstKind::Function(f) if f.r#type == FunctionType::TSDeclareFunction) {
return true;
}

// for non-overloads, the next parent will be the function
let Some(maybe_method_or_fn) = parents_iter.next() else {
return false;
};

match maybe_method_or_fn {
// arguments inside setters are allowed. Without them, the program
// has invalid syntax
AstKind::MethodDefinition(MethodDefinition {
kind: MethodDefinitionKind::Set, ..
})
| AstKind::ObjectProperty(ObjectProperty { kind: PropertyKind::Set, .. }) => true,

// Allow unused parameters in function overloads
AstKind::Function(f)
if f.body.is_none() || f.r#type == FunctionType::TSDeclareFunction =>
{
true
}
// Allow unused parameters in method overloads and overrides
AstKind::MethodDefinition(method)
if method.value.r#type == FunctionType::TSEmptyBodyFunctionExpression
|| method.r#override =>
{
true
}
// constructor property definitions are allowed because they declare
// class members
// e.g. `class Foo { constructor(public a) {} }`
AstKind::MethodDefinition(method) if method.kind.is_constructor() => {
param.has_modifier()
}
// parameters in abstract methods will never be directly used b/c
// abstract methods have no bodies. However, since this establishes
// an API contract and gets used by subclasses, it is allowed.
AstKind::MethodDefinition(method) if method.r#type.is_abstract() => true,
_ => false,
}
}
}
Loading