refactor(lint/noUselessConstructor): reduce false positives (#158)

biomejs · Sep 5, 2023 · 7b43bb3 · 7b43bb3
1 parent a97bb03
commit 7b43bb3
Show file tree

Hide file tree

Showing 18 changed files with 222 additions and 66 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -62,6 +62,9 @@ Read our [guidelines for writing a good changelog entry](https://github.com/biom
   + (a++) ** /**/ (2)
   ```
 
+- [noUselessConstructor](https://biomejs.dev/linter/rules/no-useless-constructor/) now ignores decorated classes and decorated parameters.
+  The rule now gives suggestions instead of safe fixes when parameters are annotated with types.
+
 #### Bug fixes
 
 - Fix [#80](https://github.com/biomejs/biome/issues/95), making [noDuplicateJsxProps](https://biomejs.dev/linter/rules/no-duplicate-jsx-props/) case-insensitive.

diff --git a/crates/rome_cli/tests/commands/check.rs b/crates/rome_cli/tests/commands/check.rs
@@ -2220,7 +2220,7 @@ fn check_stdin_apply_successfully() {
 
     console
         .in_buffer
-        .push("function f() {return{}} class Foo { constructor() {} }".to_string());
+        .push("import {a as a} from 'mod'; function f() {return{a}} class Foo {}".to_string());
 
     let result = run_cli(
         DynRef::Borrowed(&mut fs),
@@ -2239,7 +2239,10 @@ fn check_stdin_apply_successfully() {
         {message.content}
     });
 
-    assert_eq!(content, "function f() {\n\treturn {};\n}\nclass Foo {}\n");
+    assert_eq!(
+        content,
+        "import { a } from \"mod\";\nfunction f() {\n\treturn { a };\n}\nclass Foo {}\n"
+    );
 
     assert_cli_snapshot(SnapshotPayload::new(
         module_path!(),
@@ -2256,7 +2259,7 @@ fn check_stdin_apply_unsafe_successfully() {
     let mut console = BufferConsole::default();
 
     console.in_buffer.push(
-        "import 'zod'; import 'lodash'; function f() {return{}} class Foo { constructor() {} }"
+        "import 'zod'; import 'lodash'; function f() {var x = 1; return{x}} class Foo {}"
             .to_string(),
     );
 
@@ -2288,7 +2291,7 @@ fn check_stdin_apply_unsafe_successfully() {
 
     assert_eq!(
         content,
-        "import \"lodash\";\nimport \"zod\";\nfunction f() {\n\treturn {};\n}\nclass Foo {}\n"
+        "import \"lodash\";\nimport \"zod\";\nfunction f() {\n\tconst x = 1;\n\treturn { x };\n}\nclass Foo {}\n"
     );
 
     assert_cli_snapshot(SnapshotPayload::new(
@@ -2305,10 +2308,9 @@ fn check_stdin_apply_unsafe_only_organize_imports() {
     let mut fs = MemoryFileSystem::default();
     let mut console = BufferConsole::default();
 
-    console.in_buffer.push(
-        "import 'zod'; import 'lodash'; function f() {return{}} class Foo { constructor() {} }"
-            .to_string(),
-    );
+    console
+        .in_buffer
+        .push("import 'zod'; import 'lodash'; function f() {return{}} class Foo {}".to_string());
 
     let result = run_cli(
         DynRef::Borrowed(&mut fs),
@@ -2340,7 +2342,7 @@ fn check_stdin_apply_unsafe_only_organize_imports() {
 
     assert_eq!(
         content,
-        "import 'lodash'; import 'zod'; function f() {return{}} class Foo { constructor() {} }"
+        "import 'lodash'; import 'zod'; function f() {return{}} class Foo {}"
     );
 
     assert_cli_snapshot(SnapshotPayload::new(

diff --git a/crates/rome_cli/tests/commands/lint.rs b/crates/rome_cli/tests/commands/lint.rs
@@ -1875,7 +1875,7 @@ fn check_stdin_apply_successfully() {
 
     console
         .in_buffer
-        .push("function f() {return{}} class Foo { constructor() {} }".to_string());
+        .push("import {a as a} from 'mod'; function f() {return{a}} class Foo {}".to_string());
 
     let result = run_cli(
         DynRef::Borrowed(&mut fs),
@@ -1894,7 +1894,10 @@ fn check_stdin_apply_successfully() {
         {message.content}
     });
 
-    assert_eq!(content, "function f() {return{}} class Foo { }");
+    assert_eq!(
+        content,
+        "import {a} from 'mod'; function f() {return{a}} class Foo {}"
+    );
 
     assert_cli_snapshot(SnapshotPayload::new(
         module_path!(),
@@ -1912,7 +1915,7 @@ fn check_stdin_apply_unsafe_successfully() {
 
     console
         .in_buffer
-        .push("function f() {return{}} class Foo { constructor() {} }".to_string());
+        .push("function f() {var x=1; return{x}} class Foo {}".to_string());
 
     let result = run_cli(
         DynRef::Borrowed(&mut fs),
@@ -1939,7 +1942,7 @@ fn check_stdin_apply_unsafe_successfully() {
         {message.content}
     });
 
-    assert_eq!(content, "function f() {return{}} class Foo { }");
+    assert_eq!(content, "function f() {const x=1; return{x}} class Foo {}");
 
     assert_cli_snapshot(SnapshotPayload::new(
         module_path!(),

diff --git a/crates/rome_cli/tests/snapshots/main_commands_check/check_stdin_apply_successfully.snap b/crates/rome_cli/tests/snapshots/main_commands_check/check_stdin_apply_successfully.snap
@@ -5,14 +5,15 @@ expression: content
 # Input messages
 
 ```block
-function f() {return{}} class Foo { constructor() {} }
+import {a as a} from 'mod'; function f() {return{a}} class Foo {}
 ```
 
 # Emitted Messages
 
 ```block
+import { a } from "mod";
 function f() {
-	return {};
+	return { a };
 }
 class Foo {}
 

diff --git a/...i/tests/snapshots/main_commands_check/check_stdin_apply_unsafe_only_organize_imports.snap b/...i/tests/snapshots/main_commands_check/check_stdin_apply_unsafe_only_organize_imports.snap
@@ -5,13 +5,13 @@ expression: content
 # Input messages
 
 ```block
-import 'zod'; import 'lodash'; function f() {return{}} class Foo { constructor() {} }
+import 'zod'; import 'lodash'; function f() {return{}} class Foo {}
 ```
 
 # Emitted Messages
 
 ```block
-import 'lodash'; import 'zod'; function f() {return{}} class Foo { constructor() {} }
+import 'lodash'; import 'zod'; function f() {return{}} class Foo {}
 ```
 
 
diff --git a/...s/rome_cli/tests/snapshots/main_commands_check/check_stdin_apply_unsafe_successfully.snap b/...s/rome_cli/tests/snapshots/main_commands_check/check_stdin_apply_unsafe_successfully.snap
@@ -5,7 +5,7 @@ expression: content
 # Input messages
 
 ```block
-import 'zod'; import 'lodash'; function f() {return{}} class Foo { constructor() {} }
+import 'zod'; import 'lodash'; function f() {var x = 1; return{x}} class Foo {}
 ```
 
 # Emitted Messages
@@ -14,7 +14,8 @@ import 'zod'; import 'lodash'; function f() {return{}} class Foo { constructor()
 import "lodash";
 import "zod";
 function f() {
-	return {};
+	const x = 1;
+	return { x };
 }
 class Foo {}
 

diff --git a/crates/rome_cli/tests/snapshots/main_commands_lint/check_stdin_apply_successfully.snap b/crates/rome_cli/tests/snapshots/main_commands_lint/check_stdin_apply_successfully.snap
@@ -5,13 +5,13 @@ expression: content
 # Input messages
 
 ```block
-function f() {return{}} class Foo { constructor() {} }
+import {a as a} from 'mod'; function f() {return{a}} class Foo {}
 ```
 
 # Emitted Messages
 
 ```block
-function f() {return{}} class Foo { }
+import {a} from 'mod'; function f() {return{a}} class Foo {}
 ```
 
 
diff --git a/...es/rome_cli/tests/snapshots/main_commands_lint/check_stdin_apply_unsafe_successfully.snap b/...es/rome_cli/tests/snapshots/main_commands_lint/check_stdin_apply_unsafe_successfully.snap
@@ -5,13 +5,13 @@ expression: content
 # Input messages
 
 ```block
-function f() {return{}} class Foo { constructor() {} }
+function f() {var x=1; return{x}} class Foo {}
 ```
 
 # Emitted Messages
 
 ```block
-function f() {return{}} class Foo { }
+function f() {const x=1; return{x}} class Foo {}
 ```
 
 
diff --git a/crates/rome_js_analyze/src/analyzers/complexity/no_useless_constructor.rs b/crates/rome_js_analyze/src/analyzers/complexity/no_useless_constructor.rs
@@ -2,8 +2,8 @@ use rome_analyze::{context::RuleContext, declare_rule, ActionCategory, Ast, Rule
 use rome_console::markup;
 use rome_diagnostics::Applicability;
 use rome_js_syntax::{
-    AnyJsCallArgument, AnyJsClass, AnyJsConstructorParameter, JsCallExpression,
-    JsConstructorClassMember, TsPropertyParameter,
+    AnyJsCallArgument, AnyJsClass, AnyJsConstructorParameter, AnyJsFormalParameter,
+    JsCallExpression, JsConstructorClassMember,
 };
 use rome_rowan::{AstNode, AstNodeList, AstSeparatedList, BatchMutationExt};
 
@@ -27,7 +27,7 @@ declare_rule! {
     /// }
     /// ```
     ///
-    /// ```js,expect_diagnostic
+    /// ```ts,expect_diagnostic
     /// class B extends A {
     ///     constructor (a) {
     ///         super(a);
@@ -68,6 +68,13 @@ declare_rule! {
     ///     constructor (private prop: number) {}
     /// }
     /// ```
+    ///
+    /// ```ts
+    /// @Decorator
+    /// class C {
+    ///     constructor (prop: number) {}
+    /// }
+    /// ```
     pub(crate) NoUselessConstructor {
         version: "1.0.0",
         name: "noUselessConstructor",
@@ -90,30 +97,35 @@ impl Rule for NoUselessConstructor {
         if is_not_public {
             return None;
         }
-        let has_param_property_or_default_param = constructor
-            .parameters()
-            .ok()?
-            .parameters()
-            .iter()
-            .filter_map(|x| x.ok())
-            .any(|x| {
-                TsPropertyParameter::can_cast(x.syntax().kind())
-                    || x.as_any_js_formal_parameter()
-                        .and_then(|x| x.as_js_formal_parameter())
-                        .and_then(|x| x.initializer())
-                        .is_some()
-            });
-        if has_param_property_or_default_param {
-            return None;
+        for parameter in constructor.parameters().ok()?.parameters() {
+            let decorators = match parameter.ok()? {
+                AnyJsConstructorParameter::AnyJsFormalParameter(
+                    AnyJsFormalParameter::JsBogusParameter(_),
+                )
+                | AnyJsConstructorParameter::TsPropertyParameter(_) => {
+                    // Ignore constructors with Bogus parameters or parameter properties
+                    return None;
+                }
+                AnyJsConstructorParameter::AnyJsFormalParameter(
+                    AnyJsFormalParameter::JsFormalParameter(parameter),
+                ) => parameter.decorators(),
+                AnyJsConstructorParameter::JsRestParameter(parameter) => parameter.decorators(),
+            };
+            if !decorators.is_empty() {
+                // Ignore constructors with decorated parameters
+                return None;
+            }
+        }
+        let class = constructor.syntax().ancestors().find_map(AnyJsClass::cast);
+        if let Some(class) = &class {
+            if !class.decorators().is_empty() {
+                // Ignore decorated classes
+                return None;
+            }
         }
-        let has_parent_class = constructor
-            .syntax()
-            .ancestors()
-            .find_map(AnyJsClass::cast)
-            .filter(|x| x.extends_clause().is_some())
-            .is_some();
         let mut body_statements = constructor.body().ok()?.statements().iter();
         let Some(first) = body_statements.next() else {
+            let has_parent_class = class.and_then(|x| x.extends_clause()).is_some();
             if has_parent_class {
                 // A `super` call is missing.
                 // Do not report as useless constructor.
@@ -160,11 +172,19 @@ impl Rule for NoUselessConstructor {
         let mut mutation = ctx.root().begin();
         mutation.remove_node(constructor.clone());
         // Safely remove the constructor whether there is no comments.
-        let applicability = if constructor.syntax().has_comments_descendants() {
-            Applicability::MaybeIncorrect
-        } else {
-            Applicability::Always
-        };
+        let has_typed_parameters = constructor
+            .parameters()
+            .ok()?
+            .parameters()
+            .iter()
+            .find_map(|x| x.ok()?.type_annotation())
+            .is_some();
+        let applicability =
+            if has_typed_parameters || constructor.syntax().has_comments_descendants() {
+                Applicability::MaybeIncorrect
+            } else {
+                Applicability::Always
+            };
         Some(JsRuleAction {
             category: ActionCategory::QuickFix,
             applicability,

diff --git a/crates/rome_js_analyze/tests/specs/complexity/noUselessConstructor/invalid.ts b/crates/rome_js_analyze/tests/specs/complexity/noUselessConstructor/invalid.ts
@@ -0,0 +1,5 @@
+class B extends A {
+    constructor(foo: number) {
+        super(foo);
+    }
+}
diff --git a/crates/rome_js_analyze/tests/specs/complexity/noUselessConstructor/invalid.ts.snap b/crates/rome_js_analyze/tests/specs/complexity/noUselessConstructor/invalid.ts.snap
@@ -0,0 +1,42 @@
+---
+source: crates/rome_js_analyze/tests/spec_tests.rs
+expression: invalid.ts
+---
+# Input
+```js
+class B extends A {
+    constructor(foo: number) {
+        super(foo);
+    }
+}
+
+```
+
+# Diagnostics
+```
+invalid.ts:2:5 lint/complexity/noUselessConstructor  FIXABLE  ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
+
+  ! This constructor is unnecessary.
+  
+    1 │ class B extends A {
+  > 2 │     constructor(foo: number) {
+      │     ^^^^^^^^^^^^^^^^^^^^^^^^^^
+  > 3 │         super(foo);
+  > 4 │     }
+      │     ^
+    5 │ }
+    6 │ 
+  
+  i Suggested fix: Remove the unnecessary constructor.
+  
+    1 1 │   class B extends A {
+    2   │ - ····constructor(foo:·number)·{
+    3   │ - ········super(foo);
+    4   │ - ····}
+    5 2 │   }
+    6 3 │   
+  
+
+```
+
+
diff --git a/crates/rome_js_analyze/tests/specs/complexity/noUselessConstructor/valid.jsonc.snap b/crates/rome_js_analyze/tests/specs/complexity/noUselessConstructor/valid.jsonc.snap
@@ -12,6 +12,22 @@ class A { }
 class A { constructor(a, b = 0){ } }
 ```
 
+# Diagnostics
+```
+valid.jsonc:1:11 lint/complexity/noUselessConstructor  FIXABLE  ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
+
+  ! This constructor is unnecessary.
+  
+  > 1 │ class A { constructor(a, b = 0){ } }
+      │           ^^^^^^^^^^^^^^^^^^^^^^^^
+  
+  i Safe fix: Remove the unnecessary constructor.
+  
+    1 │ class·A·{·constructor(a,·b·=·0){·}·}
+      │           ------------------------- 
+
+```
+
 # Input
 ```js
 class A { constructor(){ doSomething(); } }

diff --git a/crates/rome_js_analyze/tests/specs/complexity/noUselessConstructor/validDecorator.js b/crates/rome_js_analyze/tests/specs/complexity/noUselessConstructor/validDecorator.js
@@ -0,0 +1,8 @@
+class A {
+    constructor(@inject("foo") foo) {}
+}
+
+@autoInjectable()
+class B {
+    constructor(foo) {}
+}