Skip to content

Commit

Permalink
fix(linter): handle useful but empty constructors in no-useless-const…
Browse files Browse the repository at this point in the history
…ructor (#3951)

Closes #3665
  • Loading branch information
DonIsaac committed Jun 28, 2024
1 parent 6498a08 commit 94329e4
Show file tree
Hide file tree
Showing 2 changed files with 47 additions and 2 deletions.
42 changes: 40 additions & 2 deletions crates/oxc_linter/src/rules/eslint/no_useless_constructor.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
use oxc_ast::{
ast::{
Argument, BindingPattern, BindingPatternKind, BindingRestElement, CallExpression,
Expression, FormalParameters, FunctionBody, MethodDefinition, Statement,
Expression, FormalParameters, FunctionBody, MethodDefinition, Statement, TSAccessibility,
},
AstKind,
};
Expand Down Expand Up @@ -77,7 +77,7 @@ declare_oxc_lint!(
/// }
///```
NoUselessConstructor,
nursery,
suspicious,
);

impl Rule for NoUselessConstructor {
Expand All @@ -91,6 +91,13 @@ impl Rule for NoUselessConstructor {
let Some(body) = &constructor.value.body else {
return;
};
// allow `private private constructor() {}`. However, `public private
// constructor() {}` is the same as `constructor() {}` and so is not allowed.
if constructor.accessibility.is_some_and(|access| {
matches!(access, TSAccessibility::Private | TSAccessibility::Protected)
}) {
return;
}

let class = ctx
.nodes()
Expand All @@ -114,6 +121,7 @@ impl Rule for NoUselessConstructor {
}
}

// Check for an empty constructor in a class without a superclass.
fn lint_empty_constructor<'a>(
ctx: &LintContext<'a>,
constructor: &MethodDefinition<'a>,
Expand All @@ -122,6 +130,19 @@ fn lint_empty_constructor<'a>(
if !body.statements.is_empty() {
return;
}

// allow constructors with access modifiers since they actually declare
// class members
if constructor
.value
.params
.items
.iter()
.any(|param| param.accessibility.is_some() || param.readonly)
{
return;
}

ctx.diagnostic_with_fix(no_empty_constructor(constructor.span), |fixer| {
fixer.delete_range(constructor.span)
});
Expand All @@ -140,13 +161,19 @@ fn lint_redundant_super_call<'a>(
let super_args = &super_call.arguments;

if is_only_simple_params(params)
&& !is_overriding(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)
});
}
}

fn is_overriding(params: &FormalParameters) -> bool {
params.items.iter().any(|param| param.r#override)
}

/// 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`].
Expand Down Expand Up @@ -232,7 +259,16 @@ fn test() {
"class A extends B { constructor(test) { super(); } }",
"class A extends B { constructor() { foo; } }",
"class A extends B { constructor(foo, bar) { super(bar); } }",
// ts
"declare class A { constructor(options: any); }", // { "parser": require("../../fixtures/parsers/typescript-parsers/declare-class") }
"class A { private constructor() {} }",
"class A { protected constructor() {} }",
"class A { constructor(private x: number) {} }",
"class A { constructor(public x: number) {} }",
"class A { constructor(protected x: number) {} }",
"class A { constructor(readonly x: number) {} }",
"class A { constructor(private readonly x: number) {} }",
"class A extends B { constructor(override x: number) { super(x); } }",
];

let fail = vec![
Expand All @@ -245,6 +281,8 @@ fn test() {
"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); } }",
// ts
"class A { public constructor(){} }",
];

let fix = vec![
Expand Down
7 changes: 7 additions & 0 deletions crates/oxc_linter/src/snapshots/no_useless_constructor.snap
Original file line number Diff line number Diff line change
Expand Up @@ -63,3 +63,10 @@ source: crates/oxc_linter/src/tester.rs
· ──────────────────────────────────────────────
╰────
help: Constructors of subclasses invoke super() automatically if they are empty. Remove this constructor or add code to it.

eslint(no-useless-constructor): Empty constructors are unnecessary
╭─[no_useless_constructor.tsx:1:11]
1class A { public constructor(){} }
· ──────────────────────
╰────
help: Remove the constructor or add code to it.

0 comments on commit 94329e4

Please sign in to comment.