From f87f26c24b20082c6a7a55a4cf248041d129ab98 Mon Sep 17 00:00:00 2001 From: Don Isaac Date: Sun, 9 Jun 2024 13:49:43 -0400 Subject: [PATCH 01/11] feat(linter): add eslint/no-useless-constructor --- Cargo.toml | 3 + crates/oxc_allocator/src/arena.rs | 6 + crates/oxc_ast/src/ast/js.rs | 36 +++ crates/oxc_linter/src/rules.rs | 2 + .../rules/eslint/no_useless_constructor.rs | 232 ++++++++++++++++++ .../src/snapshots/no_useless_constructor.snap | 66 +++++ 6 files changed, 345 insertions(+) create mode 100644 crates/oxc_linter/src/rules/eslint/no_useless_constructor.rs create mode 100644 crates/oxc_linter/src/snapshots/no_useless_constructor.snap diff --git a/Cargo.toml b/Cargo.toml index 2a42c9ecb6b08..e78856efa9054 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -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 } + [profile.release.package.oxc_wasm] opt-level = 'z' diff --git a/crates/oxc_allocator/src/arena.rs b/crates/oxc_allocator/src/arena.rs index 69f8a5da6f45f..3250d2b0bb27b 100644 --- a/crates/oxc_allocator/src/arena.rs +++ b/crates/oxc_allocator/src/arena.rs @@ -69,6 +69,12 @@ impl<'alloc, T: ?Sized> ops::DerefMut for Box<'alloc, T> { } } +impl<'alloc, T: ?Sized> AsRef 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) diff --git a/crates/oxc_ast/src/ast/js.rs b/crates/oxc_ast/src/ast/js.rs index 7705ba5bf87d2..79288b08fbba5 100644 --- a/crates/oxc_ast/src/ast/js.rs +++ b/crates/oxc_ast/src/ast/js.rs @@ -2438,6 +2438,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> + '_ { + self.items + .iter() + .map(|param| ¶m.pattern) + .chain(self.rest.iter().map(|rest| &rest.argument)) + } } #[visited_node] @@ -2633,14 +2641,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) } diff --git a/crates/oxc_linter/src/rules.rs b/crates/oxc_linter/src/rules.rs index db30b32abc919..91243b47b1656 100644 --- a/crates/oxc_linter/src/rules.rs +++ b/crates/oxc_linter/src/rules.rs @@ -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; @@ -484,6 +485,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, diff --git a/crates/oxc_linter/src/rules/eslint/no_useless_constructor.rs b/crates/oxc_linter/src/rules/eslint/no_useless_constructor.rs new file mode 100644 index 0000000000000..d9c1d0431db6f --- /dev/null +++ b/crates/oxc_linter/src/rules/eslint/no_useless_constructor.rs @@ -0,0 +1,232 @@ +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, fixer::Fix, 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 + /// + /// + /// ### Why is this bad? + /// + /// + /// ### Example + /// ```javascript + /// ``` + 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), || { + Fix::delete(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), || { + Fix::delete(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(¶m.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 (¶m.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(); +} diff --git a/crates/oxc_linter/src/snapshots/no_useless_constructor.snap b/crates/oxc_linter/src/snapshots/no_useless_constructor.snap new file mode 100644 index 0000000000000..63be1ed0ec3f9 --- /dev/null +++ b/crates/oxc_linter/src/snapshots/no_useless_constructor.snap @@ -0,0 +1,66 @@ +--- +source: crates/oxc_linter/src/tester.rs +expression: no_useless_constructor +--- + ⚠ eslint(no-useless-constructor): Empty constructors are unnecessary + ╭─[no_useless_constructor.tsx:1:11] + 1 │ class A { constructor(){} } + · ─────────────── + ╰──── + help: Remove the constructor or add code to it. + + ⚠ eslint(no-useless-constructor): Empty constructors are unnecessary + ╭─[no_useless_constructor.tsx:1:11] + 1 │ class A { 'constructor'(){} } + · ───────────────── + ╰──── + help: Remove the constructor or add code to it. + + ⚠ eslint(no-useless-constructor): Redundant super call in constructor + ╭─[no_useless_constructor.tsx:1:21] + 1 │ class A extends B { constructor() { super(); } } + · ────────────────────────── + ╰──── + help: Constructors of subclasses invoke super() automatically if they are empty. Remove this constructor or add code to it. + + ⚠ eslint(no-useless-constructor): Redundant super call in constructor + ╭─[no_useless_constructor.tsx:1:21] + 1 │ class A extends B { constructor(foo){ super(foo); } } + · ─────────────────────────────── + ╰──── + help: Constructors of subclasses invoke super() automatically if they are empty. Remove this constructor or add code to it. + + ⚠ eslint(no-useless-constructor): Redundant super call in constructor + ╭─[no_useless_constructor.tsx:1:21] + 1 │ class A extends B { constructor(foo, bar){ super(foo, bar); } } + · ───────────────────────────────────────── + ╰──── + help: Constructors of subclasses invoke super() automatically if they are empty. Remove this constructor or add code to it. + + ⚠ eslint(no-useless-constructor): Redundant super call in constructor + ╭─[no_useless_constructor.tsx:1:21] + 1 │ class A extends B { constructor(...args){ super(...args); } } + · ─────────────────────────────────────── + ╰──── + help: Constructors of subclasses invoke super() automatically if they are empty. Remove this constructor or add code to it. + + ⚠ eslint(no-useless-constructor): Redundant super call in constructor + ╭─[no_useless_constructor.tsx:1:23] + 1 │ class A extends B.C { constructor() { super(...arguments); } } + · ────────────────────────────────────── + ╰──── + help: Constructors of subclasses invoke super() automatically if they are empty. Remove this constructor or add code to it. + + ⚠ eslint(no-useless-constructor): Redundant super call in constructor + ╭─[no_useless_constructor.tsx:1:21] + 1 │ class A extends B { constructor(a, b, ...c) { super(...arguments); } } + · ──────────────────────────────────────────────── + ╰──── + help: Constructors of subclasses invoke super() automatically if they are empty. Remove this constructor or add code to it. + + ⚠ eslint(no-useless-constructor): Redundant super call in constructor + ╭─[no_useless_constructor.tsx:1:21] + 1 │ class A extends B { constructor(a, b, ...c) { super(a, b, ...c); } } + · ────────────────────────────────────────────── + ╰──── + help: Constructors of subclasses invoke super() automatically if they are empty. Remove this constructor or add code to it. From 7ffa3e9209e3c8c0ff553d69637168ef9e1423b3 Mon Sep 17 00:00:00 2001 From: Don Isaac Date: Sun, 9 Jun 2024 14:30:24 -0400 Subject: [PATCH 02/11] docs(linter): add rule documentation --- .../rules/eslint/no_useless_constructor.rs | 44 ++++++++++++++++++- 1 file changed, 43 insertions(+), 1 deletion(-) diff --git a/crates/oxc_linter/src/rules/eslint/no_useless_constructor.rs b/crates/oxc_linter/src/rules/eslint/no_useless_constructor.rs index d9c1d0431db6f..bc49a096b8a27 100644 --- a/crates/oxc_linter/src/rules/eslint/no_useless_constructor.rs +++ b/crates/oxc_linter/src/rules/eslint/no_useless_constructor.rs @@ -28,13 +28,55 @@ pub struct NoUselessConstructor; declare_oxc_lint!( /// ### What it does /// + /// Disallow unnecessary constructors /// - /// ### Why is this bad? + /// 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, ); From 6f17bcc28364a8ec712c778bc1e9c5873465d9d1 Mon Sep 17 00:00:00 2001 From: Don Isaac Date: Mon, 10 Jun 2024 14:05:54 -0400 Subject: [PATCH 03/11] fix: add fixer --- .../oxc_linter/src/rules/eslint/no_useless_constructor.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/crates/oxc_linter/src/rules/eslint/no_useless_constructor.rs b/crates/oxc_linter/src/rules/eslint/no_useless_constructor.rs index bc49a096b8a27..5b1ff47081936 100644 --- a/crates/oxc_linter/src/rules/eslint/no_useless_constructor.rs +++ b/crates/oxc_linter/src/rules/eslint/no_useless_constructor.rs @@ -123,8 +123,8 @@ fn lint_empty_constructor<'a>( if !body.statements.is_empty() { return; } - ctx.diagnostic_with_fix(no_empty_constructor(constructor.span), || { - Fix::delete(constructor.span) + ctx.diagnostic_with_fix(no_empty_constructor(constructor.span), |fixer| { + fixer.delete_range(constructor.span) }); } @@ -143,8 +143,8 @@ fn lint_redundant_super_call<'a>( 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), || { - Fix::delete(constructor.span) + ctx.diagnostic_with_fix(no_redundant_super_call(constructor.span), |fixer| { + fixer.delete_range(constructor.span) }); } } From aa11d60284df3797dfe313b9dbb1d84d57b41898 Mon Sep 17 00:00:00 2001 From: Don Isaac Date: Sun, 9 Jun 2024 13:49:43 -0400 Subject: [PATCH 04/11] feat(linter): add eslint/no-useless-constructor --- Cargo.toml | 3 + crates/oxc_allocator/src/arena.rs | 6 + crates/oxc_ast/src/ast/js.rs | 36 +++ crates/oxc_linter/src/rules.rs | 2 + .../rules/eslint/no_useless_constructor.rs | 232 ++++++++++++++++++ .../src/snapshots/no_useless_constructor.snap | 66 +++++ 6 files changed, 345 insertions(+) create mode 100644 crates/oxc_linter/src/rules/eslint/no_useless_constructor.rs create mode 100644 crates/oxc_linter/src/snapshots/no_useless_constructor.snap diff --git a/Cargo.toml b/Cargo.toml index 0ba74e0c9af0d..910190d4b8030 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -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 } + [profile.release.package.oxc_wasm] opt-level = 'z' diff --git a/crates/oxc_allocator/src/arena.rs b/crates/oxc_allocator/src/arena.rs index 69f8a5da6f45f..3250d2b0bb27b 100644 --- a/crates/oxc_allocator/src/arena.rs +++ b/crates/oxc_allocator/src/arena.rs @@ -69,6 +69,12 @@ impl<'alloc, T: ?Sized> ops::DerefMut for Box<'alloc, T> { } } +impl<'alloc, T: ?Sized> AsRef 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) diff --git a/crates/oxc_ast/src/ast/js.rs b/crates/oxc_ast/src/ast/js.rs index 39610e0267201..fde7463af5d64 100644 --- a/crates/oxc_ast/src/ast/js.rs +++ b/crates/oxc_ast/src/ast/js.rs @@ -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> + '_ { + self.items + .iter() + .map(|param| ¶m.pattern) + .chain(self.rest.iter().map(|rest| &rest.argument)) + } } #[visited_node] @@ -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) } diff --git a/crates/oxc_linter/src/rules.rs b/crates/oxc_linter/src/rules.rs index 644db2d1d5ceb..a28ba91c65aef 100644 --- a/crates/oxc_linter/src/rules.rs +++ b/crates/oxc_linter/src/rules.rs @@ -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; @@ -489,6 +490,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, diff --git a/crates/oxc_linter/src/rules/eslint/no_useless_constructor.rs b/crates/oxc_linter/src/rules/eslint/no_useless_constructor.rs new file mode 100644 index 0000000000000..d9c1d0431db6f --- /dev/null +++ b/crates/oxc_linter/src/rules/eslint/no_useless_constructor.rs @@ -0,0 +1,232 @@ +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, fixer::Fix, 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 + /// + /// + /// ### Why is this bad? + /// + /// + /// ### Example + /// ```javascript + /// ``` + 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), || { + Fix::delete(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), || { + Fix::delete(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(¶m.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 (¶m.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(); +} diff --git a/crates/oxc_linter/src/snapshots/no_useless_constructor.snap b/crates/oxc_linter/src/snapshots/no_useless_constructor.snap new file mode 100644 index 0000000000000..63be1ed0ec3f9 --- /dev/null +++ b/crates/oxc_linter/src/snapshots/no_useless_constructor.snap @@ -0,0 +1,66 @@ +--- +source: crates/oxc_linter/src/tester.rs +expression: no_useless_constructor +--- + ⚠ eslint(no-useless-constructor): Empty constructors are unnecessary + ╭─[no_useless_constructor.tsx:1:11] + 1 │ class A { constructor(){} } + · ─────────────── + ╰──── + help: Remove the constructor or add code to it. + + ⚠ eslint(no-useless-constructor): Empty constructors are unnecessary + ╭─[no_useless_constructor.tsx:1:11] + 1 │ class A { 'constructor'(){} } + · ───────────────── + ╰──── + help: Remove the constructor or add code to it. + + ⚠ eslint(no-useless-constructor): Redundant super call in constructor + ╭─[no_useless_constructor.tsx:1:21] + 1 │ class A extends B { constructor() { super(); } } + · ────────────────────────── + ╰──── + help: Constructors of subclasses invoke super() automatically if they are empty. Remove this constructor or add code to it. + + ⚠ eslint(no-useless-constructor): Redundant super call in constructor + ╭─[no_useless_constructor.tsx:1:21] + 1 │ class A extends B { constructor(foo){ super(foo); } } + · ─────────────────────────────── + ╰──── + help: Constructors of subclasses invoke super() automatically if they are empty. Remove this constructor or add code to it. + + ⚠ eslint(no-useless-constructor): Redundant super call in constructor + ╭─[no_useless_constructor.tsx:1:21] + 1 │ class A extends B { constructor(foo, bar){ super(foo, bar); } } + · ───────────────────────────────────────── + ╰──── + help: Constructors of subclasses invoke super() automatically if they are empty. Remove this constructor or add code to it. + + ⚠ eslint(no-useless-constructor): Redundant super call in constructor + ╭─[no_useless_constructor.tsx:1:21] + 1 │ class A extends B { constructor(...args){ super(...args); } } + · ─────────────────────────────────────── + ╰──── + help: Constructors of subclasses invoke super() automatically if they are empty. Remove this constructor or add code to it. + + ⚠ eslint(no-useless-constructor): Redundant super call in constructor + ╭─[no_useless_constructor.tsx:1:23] + 1 │ class A extends B.C { constructor() { super(...arguments); } } + · ────────────────────────────────────── + ╰──── + help: Constructors of subclasses invoke super() automatically if they are empty. Remove this constructor or add code to it. + + ⚠ eslint(no-useless-constructor): Redundant super call in constructor + ╭─[no_useless_constructor.tsx:1:21] + 1 │ class A extends B { constructor(a, b, ...c) { super(...arguments); } } + · ──────────────────────────────────────────────── + ╰──── + help: Constructors of subclasses invoke super() automatically if they are empty. Remove this constructor or add code to it. + + ⚠ eslint(no-useless-constructor): Redundant super call in constructor + ╭─[no_useless_constructor.tsx:1:21] + 1 │ class A extends B { constructor(a, b, ...c) { super(a, b, ...c); } } + · ────────────────────────────────────────────── + ╰──── + help: Constructors of subclasses invoke super() automatically if they are empty. Remove this constructor or add code to it. From d60fba143d5031998199e2dbad8f801ea0da0299 Mon Sep 17 00:00:00 2001 From: Don Isaac Date: Sun, 9 Jun 2024 14:30:24 -0400 Subject: [PATCH 05/11] docs(linter): add rule documentation --- .../rules/eslint/no_useless_constructor.rs | 44 ++++++++++++++++++- 1 file changed, 43 insertions(+), 1 deletion(-) diff --git a/crates/oxc_linter/src/rules/eslint/no_useless_constructor.rs b/crates/oxc_linter/src/rules/eslint/no_useless_constructor.rs index d9c1d0431db6f..bc49a096b8a27 100644 --- a/crates/oxc_linter/src/rules/eslint/no_useless_constructor.rs +++ b/crates/oxc_linter/src/rules/eslint/no_useless_constructor.rs @@ -28,13 +28,55 @@ pub struct NoUselessConstructor; declare_oxc_lint!( /// ### What it does /// + /// Disallow unnecessary constructors /// - /// ### Why is this bad? + /// 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, ); From ce647ffb72a21fb9e6636f861a824f0d64d454b8 Mon Sep 17 00:00:00 2001 From: Don Isaac Date: Mon, 10 Jun 2024 14:05:54 -0400 Subject: [PATCH 06/11] fix: add fixer --- .../oxc_linter/src/rules/eslint/no_useless_constructor.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/crates/oxc_linter/src/rules/eslint/no_useless_constructor.rs b/crates/oxc_linter/src/rules/eslint/no_useless_constructor.rs index bc49a096b8a27..5b1ff47081936 100644 --- a/crates/oxc_linter/src/rules/eslint/no_useless_constructor.rs +++ b/crates/oxc_linter/src/rules/eslint/no_useless_constructor.rs @@ -123,8 +123,8 @@ fn lint_empty_constructor<'a>( if !body.statements.is_empty() { return; } - ctx.diagnostic_with_fix(no_empty_constructor(constructor.span), || { - Fix::delete(constructor.span) + ctx.diagnostic_with_fix(no_empty_constructor(constructor.span), |fixer| { + fixer.delete_range(constructor.span) }); } @@ -143,8 +143,8 @@ fn lint_redundant_super_call<'a>( 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), || { - Fix::delete(constructor.span) + ctx.diagnostic_with_fix(no_redundant_super_call(constructor.span), |fixer| { + fixer.delete_range(constructor.span) }); } } From 2c0234e5d347416ceaa160230f57f8da2ebcc7b1 Mon Sep 17 00:00:00 2001 From: Boshen Date: Tue, 11 Jun 2024 16:41:07 +0800 Subject: [PATCH 07/11] Update Cargo.toml --- Cargo.toml | 2 -- 1 file changed, 2 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 910190d4b8030..de1a59fbd9acb 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -186,8 +186,6 @@ ignored = ["napi", "oxc_traverse"] # and we don't rely on it for debugging that much. debug = false -[profile.dev.package] -oxc_linter = { debug = true } [profile.release.package.oxc_wasm] opt-level = 'z' From ae964abb09a59bd97fe8e4401a4928edf5b2d7fa Mon Sep 17 00:00:00 2001 From: Boshen Date: Wed, 12 Jun 2024 10:56:49 +0800 Subject: [PATCH 08/11] u --- Cargo.toml | 1 - 1 file changed, 1 deletion(-) diff --git a/Cargo.toml b/Cargo.toml index de1a59fbd9acb..0ba74e0c9af0d 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -186,7 +186,6 @@ ignored = ["napi", "oxc_traverse"] # and we don't rely on it for debugging that much. debug = false - [profile.release.package.oxc_wasm] opt-level = 'z' From a750098c42ca668267deb84efae5ba58a770f69b Mon Sep 17 00:00:00 2001 From: Don Isaac Date: Wed, 12 Jun 2024 20:34:15 -0400 Subject: [PATCH 09/11] clippy fix --- crates/oxc_linter/src/rules/eslint/no_useless_constructor.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/oxc_linter/src/rules/eslint/no_useless_constructor.rs b/crates/oxc_linter/src/rules/eslint/no_useless_constructor.rs index 5b1ff47081936..e19be06cd02b8 100644 --- a/crates/oxc_linter/src/rules/eslint/no_useless_constructor.rs +++ b/crates/oxc_linter/src/rules/eslint/no_useless_constructor.rs @@ -9,7 +9,7 @@ use oxc_diagnostics::OxcDiagnostic; use oxc_macros::declare_oxc_lint; use oxc_span::Span; -use crate::{context::LintContext, fixer::Fix, rule::Rule, AstNode}; +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") From 4ce0b6682c9434c308cf93d53064147a24221abf Mon Sep 17 00:00:00 2001 From: Don Isaac Date: Wed, 12 Jun 2024 20:35:38 -0400 Subject: [PATCH 10/11] taplo fmt --- .typos.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.typos.toml b/.typos.toml index dc2a675e9baa5..233c0fc41cbad 100644 --- a/.typos.toml +++ b/.typos.toml @@ -29,5 +29,5 @@ seeked = "seeked" labeledby = "labeledby" [default.extend-identifiers] -IIFEs = "IIFEs" +IIFEs = "IIFEs" allowIIFEs = "allowIIFEs" From 7f4848ea93e192ec4722bf84338302522d347538 Mon Sep 17 00:00:00 2001 From: Boshen Date: Thu, 13 Jun 2024 13:03:48 +0800 Subject: [PATCH 11/11] Update Cargo.toml --- Cargo.toml | 3 --- 1 file changed, 3 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 7d82ad281ff02..00422621c1ff4 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -186,9 +186,6 @@ ignored = ["napi", "oxc_traverse"] # and we don't rely on it for debugging that much. debug = false -[profile.dev.package] -oxc_linter = { debug = true } - [profile.release.package.oxc_wasm] opt-level = 'z'