From 8f5655dfe6cfa7bcae5dacde8567bda3feb3ae65 Mon Sep 17 00:00:00 2001 From: Don Isaac Date: Thu, 13 Jun 2024 01:12:18 -0400 Subject: [PATCH] feat(linter): add eslint/no-useless-constructor (#3594) Co-authored-by: Boshen --- .typos.toml | 2 +- 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 | 274 ++++++++++++++++++ .../src/snapshots/no_useless_constructor.snap | 66 +++++ 6 files changed, 385 insertions(+), 1 deletion(-) 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/.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" 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 b8e41bd1b5d96..b4bb890ec8103 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; @@ -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, 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..e19be06cd02b8 --- /dev/null +++ b/crates/oxc_linter/src/rules/eslint/no_useless_constructor.rs @@ -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(¶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.