From 5432238cec29b11001dbd5a655e1e4510bffc899 Mon Sep 17 00:00:00 2001 From: Lucas Weng <30640930+lucasweng@users.noreply.github.com> Date: Mon, 7 Nov 2022 16:15:19 +0800 Subject: [PATCH] fix(rome_js_analyzer): `noInvalidConstructorSuper` false positive for class expressions (#3561) --- .../nursery/no_invalid_constructor_super.rs | 21 +++- .../nursery/noInvalidConstructorSuper.js | 29 ++++++ .../nursery/noInvalidConstructorSuper.js.snap | 95 +++++++++++++++++-- .../lint/rules/noInvalidConstructorSuper.md | 16 ++++ 4 files changed, 152 insertions(+), 9 deletions(-) diff --git a/crates/rome_js_analyze/src/analyzers/nursery/no_invalid_constructor_super.rs b/crates/rome_js_analyze/src/analyzers/nursery/no_invalid_constructor_super.rs index 26d51146e0c..68d7189cc98 100644 --- a/crates/rome_js_analyze/src/analyzers/nursery/no_invalid_constructor_super.rs +++ b/crates/rome_js_analyze/src/analyzers/nursery/no_invalid_constructor_super.rs @@ -2,8 +2,7 @@ use rome_analyze::context::RuleContext; use rome_analyze::{declare_rule, Ast, Rule, RuleDiagnostic}; use rome_console::{markup, MarkupBuf}; use rome_js_syntax::{ - JsAnyExpression, JsAssignmentOperator, JsClassDeclaration, JsConstructorClassMember, - JsLogicalOperator, + JsAnyClass, JsAnyExpression, JsAssignmentOperator, JsConstructorClassMember, JsLogicalOperator, }; use rome_rowan::{AstNode, AstNodeList, TextRange}; @@ -29,6 +28,22 @@ declare_rule! { /// } /// ``` /// + /// ### Valid + /// + /// ```js + /// export default class A extends B { + /// constructor() { + /// super(); + /// } + /// } + /// ``` + /// + /// ```js + /// export class A { + /// constructor() {} + /// } + /// ``` + /// pub(crate) NoInvalidConstructorSuper { version: "10.0.0", name: "noInvalidConstructorSuper", @@ -103,7 +118,7 @@ impl Rule for NoInvalidConstructorSuper { let extends_clause = node .syntax() .ancestors() - .find_map(|node| JsClassDeclaration::cast(node)?.extends_clause()); + .find_map(|node| JsAnyClass::cast(node)?.extends_clause()); match (super_expression, extends_clause) { (Some(super_expression), Some(extends_clause)) => { diff --git a/crates/rome_js_analyze/tests/specs/nursery/noInvalidConstructorSuper.js b/crates/rome_js_analyze/tests/specs/nursery/noInvalidConstructorSuper.js index b6e161b84e7..3ddde488c4f 100644 --- a/crates/rome_js_analyze/tests/specs/nursery/noInvalidConstructorSuper.js +++ b/crates/rome_js_analyze/tests/specs/nursery/noInvalidConstructorSuper.js @@ -24,6 +24,20 @@ class A extends (B -= C) { constructor() { super(); } } class A extends (B **= C) { constructor() { super(); } } class A extends (B |= C) { constructor() { super(); } } class A extends undefined { constructor() { super(); } } +module.exports = class A extends B { + constructor() { + } +} +export class A extends B { + constructor() { + missing_super(); + } +} +export default class A extends null { + constructor() { + super(); + } +} // valid class A extends B { constructor() { @@ -37,3 +51,18 @@ class A extends null { constructor() { return a; } } class A extends Object { constructor() { super() } } class A extends (5 && B) { constructor() { super(); } } class A extends (B || C) { constructor() { super(); } } +module.exports = class A extends B { + constructor() { + super(); + } +} +export class A extends B { + constructor() { + super(); + } +} +export default class A extends B { + constructor() { + super(); + } +} diff --git a/crates/rome_js_analyze/tests/specs/nursery/noInvalidConstructorSuper.js.snap b/crates/rome_js_analyze/tests/specs/nursery/noInvalidConstructorSuper.js.snap index 7dab30c5290..834a2404a86 100644 --- a/crates/rome_js_analyze/tests/specs/nursery/noInvalidConstructorSuper.js.snap +++ b/crates/rome_js_analyze/tests/specs/nursery/noInvalidConstructorSuper.js.snap @@ -30,6 +30,20 @@ class A extends (B -= C) { constructor() { super(); } } class A extends (B **= C) { constructor() { super(); } } class A extends (B |= C) { constructor() { super(); } } class A extends undefined { constructor() { super(); } } +module.exports = class A extends B { + constructor() { + } +} +export class A extends B { + constructor() { + missing_super(); + } +} +export default class A extends null { + constructor() { + super(); + } +} // valid class A extends B { constructor() { @@ -43,6 +57,21 @@ class A extends null { constructor() { return a; } } class A extends Object { constructor() { super() } } class A extends (5 && B) { constructor() { super(); } } class A extends (B || C) { constructor() { super(); } } +module.exports = class A extends B { + constructor() { + super(); + } +} +export class A extends B { + constructor() { + super(); + } +} +export default class A extends B { + constructor() { + super(); + } +} ``` @@ -317,7 +346,7 @@ noInvalidConstructorSuper.js:25:44 lint/nursery/noInvalidConstructorSuper ━━ > 25 │ class A extends (B |= C) { constructor() { super(); } } │ ^^^^^ 26 │ class A extends undefined { constructor() { super(); } } - 27 │ // valid + 27 │ module.exports = class A extends B { i This is where the non-constructor is used. @@ -326,7 +355,7 @@ noInvalidConstructorSuper.js:25:44 lint/nursery/noInvalidConstructorSuper ━━ > 25 │ class A extends (B |= C) { constructor() { super(); } } │ ^^^^^^^^ 26 │ class A extends undefined { constructor() { super(); } } - 27 │ // valid + 27 │ module.exports = class A extends B { ``` @@ -340,8 +369,8 @@ noInvalidConstructorSuper.js:26:45 lint/nursery/noInvalidConstructorSuper ━━ 25 │ class A extends (B |= C) { constructor() { super(); } } > 26 │ class A extends undefined { constructor() { super(); } } │ ^^^^^ - 27 │ // valid - 28 │ class A extends B { + 27 │ module.exports = class A extends B { + 28 │ constructor() { i This is where the non-constructor is used. @@ -349,8 +378,62 @@ noInvalidConstructorSuper.js:26:45 lint/nursery/noInvalidConstructorSuper ━━ 25 │ class A extends (B |= C) { constructor() { super(); } } > 26 │ class A extends undefined { constructor() { super(); } } │ ^^^^^^^^^ - 27 │ // valid - 28 │ class A extends B { + 27 │ module.exports = class A extends B { + 28 │ constructor() { + + +``` + +``` +noInvalidConstructorSuper.js:27:26 lint/nursery/noInvalidConstructorSuper ━━━━━━━━━━━━━━━━━━━━━━━━━━ + + ! This class extends another class and a super() call is expected. + + 25 │ class A extends (B |= C) { constructor() { super(); } } + 26 │ class A extends undefined { constructor() { super(); } } + > 27 │ module.exports = class A extends B { + │ ^^^^^^^^^ + 28 │ constructor() { + 29 │ } + + +``` + +``` +noInvalidConstructorSuper.js:31:16 lint/nursery/noInvalidConstructorSuper ━━━━━━━━━━━━━━━━━━━━━━━━━━ + + ! This class extends another class and a super() call is expected. + + 29 │ } + 30 │ } + > 31 │ export class A extends B { + │ ^^^^^^^^^ + 32 │ constructor() { + 33 │ missing_super(); + + +``` + +``` +noInvalidConstructorSuper.js:38:9 lint/nursery/noInvalidConstructorSuper ━━━━━━━━━━━━━━━━━━━━━━━━━━━ + + ! This class calls super(), but the class extends from a non-constructor. + + 36 │ export default class A extends null { + 37 │ constructor() { + > 38 │ super(); + │ ^^^^^ + 39 │ } + 40 │ } + + i This is where the non-constructor is used. + + 34 │ } + 35 │ } + > 36 │ export default class A extends null { + │ ^^^^ + 37 │ constructor() { + 38 │ super(); ``` diff --git a/website/docs/src/lint/rules/noInvalidConstructorSuper.md b/website/docs/src/lint/rules/noInvalidConstructorSuper.md index d65e16b1d1f..f772ceff810 100644 --- a/website/docs/src/lint/rules/noInvalidConstructorSuper.md +++ b/website/docs/src/lint/rules/noInvalidConstructorSuper.md @@ -49,3 +49,19 @@ class A { {% endraw %} +### Valid + +```jsx +export default class A extends B { + constructor() { + super(); + } +} +``` + +```jsx +export class A { + constructor() {} +} +``` +