fix(rome_js_analyze): noRedeclaration declaration merging (#4288)

crates/rome_js_analyze/src/semantic_analyzers/nursery/no_redeclaration.rs

@@ -2,10 +2,11 @@ use crate::semantic_services::SemanticServices;
 use rome_analyze::declare_rule;
 use rome_analyze::{context::RuleContext, Rule, RuleDiagnostic};
 use rome_console::markup;
-use rome_js_semantic::{Binding, Scope};
-use rome_js_syntax::{TextRange, TsMethodSignatureClassMember};
+use rome_js_semantic::Scope;
+use rome_js_syntax::binding_ext::AnyJsBindingDeclaration;
+use rome_js_syntax::TextRange;
 use rome_rowan::AstNode;
-use std::{collections::HashMap, vec::IntoIter};
+use std::collections::HashMap;
 
 declare_rule! {
     /// Eliminate variables that have multiple declarations in the same scope.
@@ -20,6 +21,16 @@ declare_rule! {
     /// ```
     ///
     /// ```js,expect_diagnostic
+    /// let a = 3;
+    /// let a = 10;
+    /// ```
+    ///
+    /// ```js,expect_diagnostic
+    /// function f() {}
+    /// function f() {}
+    /// ```
+    ///
+    /// ```js,expect_diagnostic
     /// class C {
     ///     static {
     ///         var c = 3;
@@ -28,6 +39,11 @@ declare_rule! {
     /// }
     /// ```
     ///
+    /// ```ts,expect_diagnostic
+    /// type Person = { name: string; }
+    /// class Person { name: string; }
+    /// ```
+    ///
     /// ### Valid
     ///
     /// ```js
@@ -42,43 +58,49 @@ declare_rule! {
     ///     bar(a: A, b: B) {}
     /// }
     /// ```
-    ///
     pub(crate) NoRedeclaration {
         version: "12.0.0",
         name: "noRedeclaration",
         recommended: true,
     }
 }
 
-type Duplicates = HashMap<String, Vec<Binding>>;
-
-type Redeclaration = (String, TextRange, Binding);
+#[derive(Debug)]
+pub(crate) struct Redeclaration {
+    name: String,
+    declaration: TextRange,
+    redeclaration: TextRange,
+}
 
 impl Rule for NoRedeclaration {
     type Query = SemanticServices;
     type State = Redeclaration;
-    type Signals = IntoIter<Redeclaration>;
+    type Signals = Vec<Redeclaration>;
     type Options = ();
 
     fn run(ctx: &RuleContext<Self>) -> Self::Signals {
         let mut redeclarations = Vec::default();
         for scope in ctx.query().scopes() {
             check_redeclarations_in_single_scope(&scope, &mut redeclarations);
         }
-        redeclarations.into_iter()
+        redeclarations
     }
 
     fn diagnostic(_ctx: &RuleContext<Self>, state: &Self::State) -> Option<RuleDiagnostic> {
-        let (name, text_range, binding) = state;
+        let Redeclaration {
+            name,
+            declaration,
+            redeclaration,
+        } = state;
         let diag = RuleDiagnostic::new(
             rule_category!(),
-            binding.syntax().text_trimmed_range(),
+            redeclaration,
             markup! {
                "Shouldn't redeclare '"{ name }"'. Consider to delete it or rename it"
             },
         )
         .detail(
-            text_range,
+            declaration,
             markup! {
                "'"{ name }"' is defined here."
             },
@@ -88,28 +110,31 @@ impl Rule for NoRedeclaration {
 }
 
 fn check_redeclarations_in_single_scope(scope: &Scope, redeclarations: &mut Vec<Redeclaration>) {
-    let mut duplicates = Duplicates::default();
-    let bindings = scope.bindings();
-    for binding in bindings {
-        let name = binding.tree().text();
-        duplicates.entry(name).or_default().push(binding)
-    }
-
-    // only keep the actual re-declarations
-    duplicates.retain(|_, list| list.len() > 1);
-
-    for (name, list) in duplicates {
-        let first_binding_range = list[0].syntax().text_trimmed_range();
-        list.into_iter()
-            .skip(1) // skip the first binding
-            .for_each(|binding| {
-                if !binding
-                    .syntax()
-                    .ancestors()
-                    .any(|node| TsMethodSignatureClassMember::can_cast(node.kind()))
+    let mut declarations = HashMap::<String, (TextRange, AnyJsBindingDeclaration)>::default();
+    for binding in scope.bindings() {
+        let id_binding = binding.tree();
+        // We consider only binding of a declaration
+        // This allows to skip function parameters, methods, ...
+        if let Some(decl) = id_binding.declaration() {
+            let name = id_binding.text();
+            if let Some((first_text_range, first_decl)) = declarations.get(&name) {
+                // Do not report:
+                // - mergeable declarations.
+                //   e.g. a `function` and a `namespace`
+                // - when both are parameter-like.
+                //   A parameter can override a previous parameter.
+                if !(first_decl.is_mergeable(&decl)
+                    || first_decl.is_parameter_like() && decl.is_parameter_like())
                 {
-                    redeclarations.push((name.clone(), first_binding_range, binding))
+                    redeclarations.push(Redeclaration {
+                        name,
+                        declaration: *first_text_range,
+                        redeclaration: id_binding.syntax().text_trimmed_range(),
+                    })
                 }
-            })
+            } else {
+                declarations.insert(name, (id_binding.syntax().text_trimmed_range(), decl));
+            }
+        }
     }
 }

crates/rome_js_analyze/tests/specs/nursery/noRedeclaration/invalid-declaration-merging.ts

@@ -0,0 +1,16 @@
+// Type and value merging
+export type Order = -1 | 0 | 1;
+
+interface Order {
+	f(): void;
+}
+
+class Order {
+	prop: number;
+}
+
+enum Order {
+	Lower = -1,
+	Equal = 0,
+	Upper = 1,
+}

crates/rome_js_analyze/tests/specs/nursery/noRedeclaration/invalid-declaration-merging.ts.snap

@@ -0,0 +1,97 @@
+---
+source: crates/rome_js_analyze/tests/spec_tests.rs
+assertion_line: 91
+expression: invalid-declaration-merging.ts
+---
+# Input
+```js
+// Type and value merging
+export type Order = -1 | 0 | 1;
+
+interface Order {
+	f(): void;
+}
+
+class Order {
+	prop: number;
+}
+
+enum Order {
+	Lower = -1,
+	Equal = 0,
+	Upper = 1,
+}
+
+```
+
+# Diagnostics
+```
+invalid-declaration-merging.ts:4:11 lint/nursery/noRedeclaration ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
+
+  ! Shouldn't redeclare 'Order'. Consider to delete it or rename it
+  
+    2 │ export type Order = -1 | 0 | 1;
+    3 │ 
+  > 4 │ interface Order {
+      │           ^^^^^
+    5 │ 	f(): void;
+    6 │ }
+  
+  i 'Order' is defined here.
+  
+    1 │ // Type and value merging
+  > 2 │ export type Order = -1 | 0 | 1;
+      │             ^^^^^
+    3 │ 
+    4 │ interface Order {
+  
+
+```
+
+```
+invalid-declaration-merging.ts:8:7 lint/nursery/noRedeclaration ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
+
+  ! Shouldn't redeclare 'Order'. Consider to delete it or rename it
+  
+     6 │ }
+     7 │ 
+   > 8 │ class Order {
+       │       ^^^^^
+     9 │ 	prop: number;
+    10 │ }
+  
+  i 'Order' is defined here.
+  
+    1 │ // Type and value merging
+  > 2 │ export type Order = -1 | 0 | 1;
+      │             ^^^^^
+    3 │ 
+    4 │ interface Order {
+  
+
+```
+
+```
+invalid-declaration-merging.ts:12:6 lint/nursery/noRedeclaration ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
+
+  ! Shouldn't redeclare 'Order'. Consider to delete it or rename it
+  
+    10 │ }
+    11 │ 
+  > 12 │ enum Order {
+       │      ^^^^^
+    13 │ 	Lower = -1,
+    14 │ 	Equal = 0,
+  
+  i 'Order' is defined here.
+  
+    1 │ // Type and value merging
+  > 2 │ export type Order = -1 | 0 | 1;
+      │             ^^^^^
+    3 │ 
+    4 │ interface Order {
+  
+
+```
+
+

crates/rome_js_analyze/tests/specs/nursery/noRedeclaration/invalid.jsonc

@@ -24,5 +24,6 @@
 	"function f() { var a; var a; }",
 	"function f() { var a; if (test) { var a; } }",
 	"for (var a, a;;);",
-	"for (;;){ var a, a,;}"
+	"for (;;){ var a, a,;}",
+	"function f(x) { var x = 5; }"
 ]

crates/rome_js_analyze/tests/specs/nursery/noRedeclaration/invalid.jsonc.snap

@@ -1,5 +1,6 @@
 ---
 source: crates/rome_js_analyze/tests/spec_tests.rs
+assertion_line: 91
 expression: invalid.jsonc
 ---
 # Input
@@ -527,4 +528,9 @@ invalid.jsonc:1:18 lint/nursery/noRedeclaration ━━━━━━━━━━
 
 ```
 
+# Input
+```js
+function f(x) { var x = 5; }
+```
+
 

crates/rome_js_analyze/tests/specs/nursery/noRedeclaration/valid-declaration-merging.ts

@@ -0,0 +1,88 @@
+// Type and value merging
+export type Order = -1 | 0 | 1;
+export const Order = {
+	LOWER: -1,
+	EQUAL: 0,
+	UPPER: 1,
+} as const;
+
+export type Direction = "Up" | "Down";
+export namespace Direction {
+	export const Up = "Up";
+	export type Up = "Up";
+	export const Down = "Down";
+	export type Down = "Down";
+}
+
+export type Person = {
+	readonly name: string;
+};
+export function Person(name: string): Person {
+	return { name };
+}
+
+interface Organization {
+	readonly name: string;
+}
+export function Organization(name: string): Organization {
+	return { name };
+}
+
+// Interface merging
+export interface Splitted {
+	f(): void;
+}
+export interface Splitted {
+	g(): void;
+}
+
+// interface, class, and namespace merging
+export interface MoralPerson {
+	phantom(): void;
+}
+export class MoralPerson {
+	name: string;
+	constructor(name: string) {
+		this.name = name;
+	}
+}
+export namespace MoralPerson {
+	export function from(name: string) {
+		return new MoralPerson(name);
+	}
+}
+
+// function and namespace merging
+export function mod(): void {}
+export namespace MoralPerson {
+	export function f(): void {}
+}
+
+// enum and namespace merging
+export enum Orientation {
+	North,
+	East,
+	South,
+	West,
+}
+export namespace Orientation {
+	export function f(): void {}
+}
+
+// namespace merging
+export namespace X {
+	export function f(): void {}
+}
+export namespace X {
+	export function g(): void {}
+}
+
+// enum merging
+export enum Orientation2 {
+	North = 0,
+	South = 1,
+}
+export enum Orientation2 {
+	East = 2,
+	West = 3,
+}