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-useless-constructor #3594

Merged
merged 16 commits into from
Jun 13, 2024
Merged
Show file tree
Hide file tree
Changes from 15 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
2 changes: 1 addition & 1 deletion .typos.toml
Original file line number Diff line number Diff line change
Expand Up @@ -29,5 +29,5 @@ seeked = "seeked"
labeledby = "labeledby"

[default.extend-identifiers]
IIFEs = "IIFEs"
IIFEs = "IIFEs"
allowIIFEs = "allowIIFEs"
3 changes: 3 additions & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -186,6 +186,9 @@ ignored = ["napi", "oxc_traverse"]
# and we don't rely on it for debugging that much.
debug = false

[profile.dev.package]
oxc_linter = { debug = true }
Boshen marked this conversation as resolved.
Show resolved Hide resolved

Boshen marked this conversation as resolved.
Show resolved Hide resolved
[profile.release.package.oxc_wasm]
opt-level = 'z'

Expand Down
6 changes: 6 additions & 0 deletions crates/oxc_allocator/src/arena.rs
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,12 @@ impl<'alloc, T: ?Sized> ops::DerefMut for Box<'alloc, T> {
}
}

impl<'alloc, T: ?Sized> AsRef<T> for Box<'alloc, T> {
fn as_ref(&self) -> &T {
self
}
}

impl<'alloc, T: ?Sized + Debug> Debug for Box<'alloc, T> {
fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result {
self.deref().fmt(f)
Expand Down
36 changes: 36 additions & 0 deletions crates/oxc_ast/src/ast/js.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2453,6 +2453,14 @@ impl<'a> FormalParameters<'a> {
pub fn parameters_count(&self) -> usize {
self.items.len() + self.rest.as_ref().map_or(0, |_| 1)
}

/// Iterates over all bound parameters, including rest parameters.
pub fn iter_bindings(&self) -> impl Iterator<Item = &BindingPattern<'a>> + '_ {
self.items
.iter()
.map(|param| &param.pattern)
.chain(self.rest.iter().map(|rest| &rest.argument))
}
}

#[visited_node]
Expand Down Expand Up @@ -2651,14 +2659,42 @@ impl<'a> Class<'a> {
}
}

/// `true` if this [`Class`] is an expression.
///
/// For example,
/// ```ts
/// var Foo = class { /* ... */ }
/// ```
pub fn is_expression(&self) -> bool {
self.r#type == ClassType::ClassExpression
}

/// `true` if this [`Class`] is a declaration statement.
///
/// For example,
/// ```ts
/// class Foo {
/// // ...
/// }
/// ```
///
/// Not to be confused with [`Class::is_declare`].
pub fn is_declaration(&self) -> bool {
self.r#type == ClassType::ClassDeclaration
}

/// `true` if this [`Class`] is being within a typescript declaration file
/// or `declare` statement.
///
/// For example,
/// ```ts
/// declare global {
/// declare class Foo {
/// // ...
/// }
/// }
///
/// Not to be confused with [`Class::is_declaration`].
pub fn is_declare(&self) -> bool {
self.modifiers.contains(ModifierKind::Declare)
}
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 @@ -102,6 +102,7 @@ mod eslint {
pub mod no_unused_private_class_members;
pub mod no_useless_catch;
pub mod no_useless_concat;
pub mod no_useless_constructor;
pub mod no_useless_escape;
pub mod no_useless_rename;
pub mod no_var;
Expand Down Expand Up @@ -491,6 +492,7 @@ oxc_macros::declare_all_lint_rules! {
eslint::no_useless_escape,
eslint::no_useless_rename,
eslint::no_useless_concat,
eslint::no_useless_constructor,
eslint::no_var,
eslint::no_void,
eslint::no_with,
Expand Down
274 changes: 274 additions & 0 deletions crates/oxc_linter/src/rules/eslint/no_useless_constructor.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,274 @@
use oxc_ast::{
ast::{
Argument, BindingPattern, BindingPatternKind, BindingRestElement, CallExpression,
Expression, FormalParameters, FunctionBody, MethodDefinition, Statement,
},
AstKind,
};
use oxc_diagnostics::OxcDiagnostic;
use oxc_macros::declare_oxc_lint;
use oxc_span::Span;

use crate::{context::LintContext, rule::Rule, AstNode};

fn no_empty_constructor(constructor_span: Span) -> OxcDiagnostic {
OxcDiagnostic::warn("eslint(no-useless-constructor): Empty constructors are unnecessary")
.with_labels([constructor_span.into()])
.with_help("Remove the constructor or add code to it.")
}
fn no_redundant_super_call(constructor_span: Span) -> OxcDiagnostic {
OxcDiagnostic::warn("eslint(no-useless-constructor): Redundant super call in constructor")
.with_labels([constructor_span.into()])
.with_help("Constructors of subclasses invoke super() automatically if they are empty. Remove this constructor or add code to it.")
}

#[derive(Debug, Default, Clone)]
pub struct NoUselessConstructor;

declare_oxc_lint!(
/// ### What it does
///
/// Disallow unnecessary constructors
///
/// This rule flags class constructors that can be safely removed without
/// changing how the class works.
///
/// ES2015 provides a default class constructor if one is not specified. As
/// such, it is unnecessary to provide an empty constructor or one that
/// simply delegates into its parent class, as in the following examples:
///
///
/// ### Example
///
/// Examples of **incorrect** code for this rule:
/// ```javascript
/// class A {
/// constructor () {
/// }
/// }
///
/// class B extends A {
/// constructor (...args) {
/// super(...args);
/// }
/// }
/// ```
///
/// Examples of **correct** code for this rule:
/// ```javascript
/// class A { }
///
/// class B {
/// constructor () {
/// doSomething();
/// }
/// }
///
/// class C extends A {
/// constructor() {
/// super('foo');
/// }
/// }
///
/// class D extends A {
/// constructor() {
/// super();
/// doSomething();
/// }
/// }
///```
NoUselessConstructor,
suspicious,
);

impl Rule for NoUselessConstructor {
fn run<'a>(&self, node: &AstNode<'a>, ctx: &LintContext<'a>) {
let AstKind::MethodDefinition(constructor) = node.kind() else {
return;
};
if !constructor.kind.is_constructor() {
return;
}
let Some(body) = &constructor.value.body else {
return;
};

let class = ctx
.nodes()
.iter_parents(node.id())
.skip(1)
.find(|parent| matches!(parent.kind(), AstKind::Class(_)));
debug_assert!(class.is_some(), "Found a constructor outside of a class definition");
let Some(class_node) = class else {
return;
};
let AstKind::Class(class) = class_node.kind() else { unreachable!() };
if class.is_declare() {
return;
}

if class.super_class.is_none() {
lint_empty_constructor(ctx, constructor, body);
} else {
lint_redundant_super_call(ctx, constructor, body);
}
}
}

fn lint_empty_constructor<'a>(
ctx: &LintContext<'a>,
constructor: &MethodDefinition<'a>,
body: &FunctionBody<'a>,
) {
if !body.statements.is_empty() {
return;
}
ctx.diagnostic_with_fix(no_empty_constructor(constructor.span), |fixer| {
fixer.delete_range(constructor.span)
});
}

fn lint_redundant_super_call<'a>(
ctx: &LintContext<'a>,
constructor: &MethodDefinition<'a>,
body: &FunctionBody<'a>,
) {
let Some(super_call) = is_single_super_call(body) else {
return;
};

let params = &*constructor.value.params;
let super_args = &super_call.arguments;

if is_only_simple_params(params)
&& (is_spread_arguments(super_args) || is_passing_through(params, super_args))
{
ctx.diagnostic_with_fix(no_redundant_super_call(constructor.span), |fixer| {
fixer.delete_range(constructor.span)
});
}
}
/// Check if a function body only contains a single super call. Ignores directives.
///
/// Returns the call expression if the body contains a single super call, otherwise [`None`].
fn is_single_super_call<'f, 'a: 'f>(body: &'f FunctionBody<'a>) -> Option<&'f CallExpression<'a>> {
if body.statements.len() != 1 {
return None;
}
let Statement::ExpressionStatement(expr) = &body.statements[0] else { return None };
let Expression::CallExpression(call) = &expr.expression else { return None };

matches!(call.callee, Expression::Super(_)).then(|| call.as_ref())
}

/// Returns `false` if any parameter is an array/object unpacking binding or an
/// assignment pattern.
fn is_only_simple_params(params: &FormalParameters) -> bool {
params.iter_bindings().all(|param| param.kind.is_binding_identifier())
}

fn is_spread_arguments(super_args: &[Argument<'_>]) -> bool {
super_args.len() == 1 && super_args[0].is_spread()
}

fn is_passing_through<'a>(
constructor_params: &FormalParameters<'a>,
super_args: &[Argument<'a>],
) -> bool {
if constructor_params.parameters_count() != super_args.len() {
return false;
}
if let Some(rest) = &constructor_params.rest {
let all_but_last = super_args.iter().take(super_args.len() - 1);
let Some(last_arg) = super_args.iter().next_back() else { return false };
constructor_params
.items
.iter()
.zip(all_but_last)
.all(|(param, arg)| is_matching_identifier_pair(&param.pattern, arg))
&& is_matching_rest_spread_pair(rest, last_arg)
} else {
constructor_params
.iter_bindings()
.zip(super_args)
.all(|(param, arg)| is_matching_identifier_pair(param, arg))
}
}

fn is_matching_identifier_pair<'a>(param: &BindingPattern<'a>, arg: &Argument<'a>) -> bool {
match (&param.kind, arg) {
(BindingPatternKind::BindingIdentifier(param), Argument::Identifier(arg)) => {
param.name == arg.name
}
_ => false,
}
}
fn is_matching_rest_spread_pair<'a>(rest: &BindingRestElement<'a>, arg: &Argument<'a>) -> bool {
match (&rest.argument.kind, arg) {
(BindingPatternKind::BindingIdentifier(param), Argument::SpreadElement(spread)) => {
matches!(&spread.argument, Expression::Identifier(ident) if param.name == ident.name)
}
_ => false,
}
}

#[test]
fn test() {
use crate::tester::Tester;

let pass = vec![
"class A { }",
"class A { constructor(){ doSomething(); } }",
"class A extends B { constructor(){} }",
"class A extends B { constructor(){ super('foo'); } }",
"class A extends B { constructor(foo, bar){ super(foo, bar, 1); } }",
"class A extends B { constructor(){ super(); doSomething(); } }",
"class A extends B { constructor(...args){ super(...args); doSomething(); } }",
"class A { dummyMethod(){ doSomething(); } }",
"class A extends B.C { constructor() { super(foo); } }",
"class A extends B.C { constructor([a, b, c]) { super(...arguments); } }",
"class A extends B.C { constructor(a = f()) { super(...arguments); } }",
"class A extends B { constructor(a, b, c) { super(a, b); } }",
"class A extends B { constructor(foo, bar){ super(foo); } }",
"class A extends B { constructor(test) { super(); } }",
"class A extends B { constructor() { foo; } }",
"class A extends B { constructor(foo, bar) { super(bar); } }",
"declare class A { constructor(options: any); }", // { "parser": require("../../fixtures/parsers/typescript-parsers/declare-class") }
];

let fail = vec![
"class A { constructor(){} }",
"class A { 'constructor'(){} }",
"class A extends B { constructor() { super(); } }",
"class A extends B { constructor(foo){ super(foo); } }",
"class A extends B { constructor(foo, bar){ super(foo, bar); } }",
"class A extends B { constructor(...args){ super(...args); } }",
"class A extends B.C { constructor() { super(...arguments); } }",
"class A extends B { constructor(a, b, ...c) { super(...arguments); } }",
"class A extends B { constructor(a, b, ...c) { super(a, b, ...c); } }",
];

let fix = vec![
("class A { constructor(){} }", "class A { }"),
(
r"
class A extends B {
constructor() {
super();
}
foo() {
bar();
}
}",
r"
class A extends B {

foo() {
bar();
}
}",
),
];

Tester::new(NoUselessConstructor::NAME, pass, fail).expect_fix(fix).test_and_snapshot();
}
Loading