fix(rome_js_analyzer): noInvalidConstructorSuper false positive for…

… class expressions (#3561)
rome · Nov 7, 2022 · 5432238 · 5432238
1 parent 1911a0b
commit 5432238
Show file tree

Hide file tree

Showing 4 changed files with 152 additions and 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
@@ -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
@@ -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
@@ -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,17 +369,71 @@ 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.
   
     24 │ class A extends (B **= C) { constructor() { super(); } }
     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
@@ -49,3 +49,19 @@ class A {
 
 </code></pre>{% endraw %}
 
+### Valid
+
+```jsx
+export default class A extends B {
+    constructor() {
+        super();
+    }
+}
+```
+
+```jsx
+export class A {
+    constructor() {}
+}
+```
+