feat: impl no-dupe-class-members rule

rome · ematipico · Feb 3, 2023 · Dec 30, 2022 · Dec 30, 2022 · Jan 4, 2023
commit 70c18650ea7b69430cb915c249663be43653e84e
diff --git a/crates/rome_js_analyze/src/analyzers/nursery/no_duplicate_class_members.rs b/crates/rome_js_analyze/src/analyzers/nursery/no_duplicate_class_members.rs
@@ -1,8 +1,16 @@
-use crate::semantic_services::Semantic;
-use rome_analyze::{context::RuleContext, declare_rule, Rule, RuleDiagnostic};
-use rome_console::markup;
-use rome_js_semantic::{Reference, ReferencesExtensions};
-use rome_js_syntax::JsIdentifierBinding;
+use std::collections::HashMap;
+
+use rome_analyze::{context::RuleContext, declare_rule, Ast, Rule, RuleDiagnostic};
+use rome_js_syntax::{
+    AnyJsClassMember, AnyJsClassMemberName, AnyJsMethodModifier, AnyJsPropertyModifier,
+    AnyTsMethodSignatureModifier, AnyTsPropertySignatureModifier, JsClassDeclaration,
+    JsClassExpression, JsClassMemberList, JsGetterClassMember, JsMethodClassMember,
+    JsMethodModifierList, JsPropertyClassMember, JsPropertyModifierList, JsSetterClassMember,
+    TextRange, TsGetterSignatureClassMember, TsMethodSignatureClassMember,
+    TsMethodSignatureModifierList, TsPropertySignatureClassMember, TsPropertySignatureModifierList,
+    TsSetterSignatureClassMember,
+};
+use rome_rowan::{declare_node_union, AstNode};
 
 declare_rule! {
     /// Disallow duplicate class members.
@@ -87,31 +95,311 @@ declare_rule! {
     }
 }
 
+fn get_member_name_string(member_name_node: AnyJsClassMemberName) -> Option<String> {
+    match member_name_node {
+        AnyJsClassMemberName::JsLiteralMemberName(node) => node.name().ok(),
+        AnyJsClassMemberName::JsPrivateClassMemberName(node) => Some(node.text()),
+        _ => None,
+    }
+}
+
+#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)]
+enum AccessType {
+    Public,
+    Private,
+}
+
+fn get_access_type(member_name_node: AnyJsClassMemberName) -> Option<AccessType> {
+    match member_name_node {
+        AnyJsClassMemberName::JsPrivateClassMemberName(_) => Some(AccessType::Private),
+        _ => Some(AccessType::Public),
+    }
+}
+
+#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)]
+enum StaticType {
+    Static,
+    NonStatic,
+}
+
+#[allow(clippy::enum_variant_names)]
+enum AnyModifierList {
+    JsPropertyModifierList(JsPropertyModifierList),
+    JsMethodModifierList(JsMethodModifierList),
+    TsPropertySignatureModifierList(TsPropertySignatureModifierList),
+    TsMethodSignatureModifierList(TsMethodSignatureModifierList),
+}
+
+fn get_static_type(modifier_list: AnyModifierList) -> StaticType {
+    match modifier_list {
+        AnyModifierList::JsPropertyModifierList(node) => {
+            if node
+                .into_iter()
+                .any(|m| matches!(m, AnyJsPropertyModifier::JsStaticModifier(_)))
+            {
+                StaticType::Static
+            } else {
+                StaticType::NonStatic
+            }
+        }
+        AnyModifierList::JsMethodModifierList(node) => {
+            if node
+                .into_iter()
+                .any(|m| matches!(m, AnyJsMethodModifier::JsStaticModifier(_)))
+            {
+                StaticType::Static
+            } else {
+                StaticType::NonStatic
+            }
+        }
+        AnyModifierList::TsPropertySignatureModifierList(node) => {
+            if node
+                .into_iter()
+                .any(|m| matches!(m, AnyTsPropertySignatureModifier::JsStaticModifier(_)))
+            {
+                StaticType::Static
+            } else {
+                StaticType::NonStatic
+            }
+        }
+        AnyModifierList::TsMethodSignatureModifierList(node) => {
+            if node
+                .into_iter()
+                .any(|m| matches!(m, AnyTsMethodSignatureModifier::JsStaticModifier(_)))
+            {
+                StaticType::Static
+            } else {
+                StaticType::NonStatic
+            }
+        }
+    }
+}
+
+declare_node_union! {
+    pub(crate) ClassMemberDefinition = JsGetterClassMember | JsMethodClassMember | JsPropertyClassMember | JsSetterClassMember | TsGetterSignatureClassMember | TsMethodSignatureClassMember | TsPropertySignatureClassMember | TsSetterSignatureClassMember
+}
+
+#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)]
+pub(crate) enum MemberType {
+    Normal,
+    Getter,
+    Setter,
+}
+
+impl ClassMemberDefinition {
+    fn member_name(&self) -> Option<String> {
+        match self {
+            ClassMemberDefinition::JsGetterClassMember(node) => {
+                get_member_name_string(node.name().ok()?)
+            }
+            ClassMemberDefinition::JsMethodClassMember(node) => {
+                get_member_name_string(node.name().ok()?)
+            }
+            ClassMemberDefinition::JsPropertyClassMember(node) => {
+                get_member_name_string(node.name().ok()?)
+            }
+            ClassMemberDefinition::JsSetterClassMember(node) => {
+                get_member_name_string(node.name().ok()?)
+            }
+            ClassMemberDefinition::TsGetterSignatureClassMember(node) => {
+                get_member_name_string(node.name().ok()?)
+            }
+            ClassMemberDefinition::TsMethodSignatureClassMember(node) => {
+                get_member_name_string(node.name().ok()?)
+            }
+            ClassMemberDefinition::TsPropertySignatureClassMember(node) => {
+                get_member_name_string(node.name().ok()?)
+            }
+            ClassMemberDefinition::TsSetterSignatureClassMember(node) => {
+                get_member_name_string(node.name().ok()?)
+            }
+        }
+    }
+
+    fn member_type(&self) -> MemberType {
+        match self {
+            ClassMemberDefinition::JsGetterClassMember(_)
+            | ClassMemberDefinition::TsGetterSignatureClassMember(_) => MemberType::Getter,
+            ClassMemberDefinition::JsSetterClassMember(_)
+            | ClassMemberDefinition::TsSetterSignatureClassMember(_) => MemberType::Setter,
+            _ => MemberType::Normal,
+        }
+    }
+
+    fn access_type(&self) -> Option<AccessType> {
+        match self {
+            ClassMemberDefinition::JsGetterClassMember(node) => get_access_type(node.name().ok()?),
+            ClassMemberDefinition::JsMethodClassMember(node) => get_access_type(node.name().ok()?),
+            ClassMemberDefinition::JsPropertyClassMember(node) => {
+                get_access_type(node.name().ok()?)
+            }
+            ClassMemberDefinition::JsSetterClassMember(node) => get_access_type(node.name().ok()?),
+            ClassMemberDefinition::TsGetterSignatureClassMember(node) => {
+                get_access_type(node.name().ok()?)
+            }
+            ClassMemberDefinition::TsMethodSignatureClassMember(node) => {
+                get_access_type(node.name().ok()?)
+            }
+            ClassMemberDefinition::TsPropertySignatureClassMember(node) => {
+                get_access_type(node.name().ok()?)
+            }
+            ClassMemberDefinition::TsSetterSignatureClassMember(node) => {
+                get_access_type(node.name().ok()?)
+            }
+        }
+    }
+
+    fn static_type(&self) -> StaticType {
+        match self {
+            ClassMemberDefinition::JsGetterClassMember(node) => {
+                get_static_type(AnyModifierList::JsMethodModifierList(node.modifiers()))
+            }
+            ClassMemberDefinition::JsMethodClassMember(node) => {
+                get_static_type(AnyModifierList::JsMethodModifierList(node.modifiers()))
+            }
+            ClassMemberDefinition::JsPropertyClassMember(node) => {
+                get_static_type(AnyModifierList::JsPropertyModifierList(node.modifiers()))
+            }
+            ClassMemberDefinition::JsSetterClassMember(node) => {
+                get_static_type(AnyModifierList::JsMethodModifierList(node.modifiers()))
+            }
+            ClassMemberDefinition::TsGetterSignatureClassMember(node) => get_static_type(
+                AnyModifierList::TsMethodSignatureModifierList(node.modifiers()),
+            ),
+            ClassMemberDefinition::TsMethodSignatureClassMember(node) => get_static_type(
+                AnyModifierList::TsMethodSignatureModifierList(node.modifiers()),
+            ),
+            ClassMemberDefinition::TsPropertySignatureClassMember(node) => get_static_type(
+                AnyModifierList::TsPropertySignatureModifierList(node.modifiers()),
+            ),
+            ClassMemberDefinition::TsSetterSignatureClassMember(node) => get_static_type(
+                AnyModifierList::TsMethodSignatureModifierList(node.modifiers()),
+            ),
+        }
+    }
+
+    fn range(&self) -> TextRange {
+        match self {
+            ClassMemberDefinition::JsGetterClassMember(node) => node.range(),
+            ClassMemberDefinition::JsMethodClassMember(node) => node.range(),
+            ClassMemberDefinition::JsPropertyClassMember(node) => node.range(),
+            ClassMemberDefinition::JsSetterClassMember(node) => node.range(),
+            ClassMemberDefinition::TsGetterSignatureClassMember(node) => node.range(),
+            ClassMemberDefinition::TsMethodSignatureClassMember(node) => node.range(),
+            ClassMemberDefinition::TsPropertySignatureClassMember(node) => node.range(),
+            ClassMemberDefinition::TsSetterSignatureClassMember(node) => node.range(),
+        }
+    }
+}
+
+impl TryFrom<AnyJsClassMember> for ClassMemberDefinition {
+    type Error = AnyJsClassMember;
+
+    fn try_from(member: AnyJsClassMember) -> Result<Self, Self::Error> {
+        match member {
+            AnyJsClassMember::JsGetterClassMember(node) => {
+                Ok(ClassMemberDefinition::JsGetterClassMember(node))
+            }
+            AnyJsClassMember::JsMethodClassMember(node) => {
+                Ok(ClassMemberDefinition::JsMethodClassMember(node))
+            }
+            AnyJsClassMember::JsPropertyClassMember(node) => {
+                Ok(ClassMemberDefinition::JsPropertyClassMember(node))
+            }
+            AnyJsClassMember::JsSetterClassMember(node) => {
+                Ok(ClassMemberDefinition::JsSetterClassMember(node))
+            }
+            AnyJsClassMember::TsGetterSignatureClassMember(node) => {
+                Ok(ClassMemberDefinition::TsGetterSignatureClassMember(node))
+            }
+            AnyJsClassMember::TsMethodSignatureClassMember(node) => {
+                Ok(ClassMemberDefinition::TsMethodSignatureClassMember(node))
+            }
+            AnyJsClassMember::TsPropertySignatureClassMember(node) => {
+                Ok(ClassMemberDefinition::TsPropertySignatureClassMember(node))
+            }
+            AnyJsClassMember::TsSetterSignatureClassMember(node) => {
+                Ok(ClassMemberDefinition::TsSetterSignatureClassMember(node))
+            }
+            _ => Err(member),
+        }
+    }
+}
+
+declare_node_union! {
+    pub(crate) ClassDefinition = JsClassExpression | JsClassDeclaration
+}
+
+impl ClassDefinition {
+    fn members(&self) -> JsClassMemberList {
+        match self {
+            ClassDefinition::JsClassExpression(node) => node.members(),
+            ClassDefinition::JsClassDeclaration(node) => node.members(),
+        }
+    }
+}
+
 impl Rule for NoDuplicateClassMembers {
-    type Query = Semantic<JsIdentifierBinding>;
-    type State = Reference;
+    type Query = Ast<ClassDefinition>;
+    type State = ClassMemberDefinition;
     type Signals = Vec<Self::State>;
     type Options = ();
 
-    fn run(ctx: &RuleContext<Self>) -> Vec<Self::State> {
-        let binding = ctx.query();
-        let model = ctx.model();
+    fn run(ctx: &RuleContext<Self>) -> Self::Signals {
+        let mut defined_members: HashMap<
+            (String, StaticType, AccessType),
+            HashMap<MemberType, ClassMemberDefinition>,
+        > = HashMap::new();
+        let mut signals: Self::Signals = Vec::new();
+
+        let node = ctx.query();
+        for member in node.members() {
+            if let Ok(member_def) = ClassMemberDefinition::try_from(member) {
+                if let (Some(member_name), Some(access_type)) =
+                    (member_def.member_name(), member_def.access_type())
+                {
+                    let static_type = member_def.static_type();
+                    let member_type = member_def.member_type();
+                    defined_members
+                        .entry((member_name, static_type, access_type))
 #[derive(Clone, PartialEq, Eq, Hash)] 
 #[derive(Clone, PartialEq, Eq, Hash)] 
+                        .and_modify(|element| {
+                            if element.get(&member_type).is_some() {
+                                signals.push(member_def.clone());
+                            } else {
+                                if member_type != MemberType::Normal
+                                    && element.get(&MemberType::Normal).is_some()
+                                {
+                                    signals.push(member_def.clone());
+                                }
+
+                                if member_type == MemberType::Normal
+                                    && (element.get(&MemberType::Getter).is_some()
+                                        || element.get(&MemberType::Setter).is_some())
+                                {
+                                    signals.push(member_def.clone());
+                                }
 
-        binding.all_references(model).collect()
+                                element.insert(member_type, member_def.clone());
+                            }
+                        })
+                        .or_insert_with(|| HashMap::from([(member_type, member_def)]));
+                }
+            }
+        }
+
+        signals
     }
 
-    fn diagnostic(_: &RuleContext<Self>, reference: &Self::State) -> Option<RuleDiagnostic> {
-        Some(
-            RuleDiagnostic::new(
-                rule_category!(),
-                reference.syntax().text_trimmed_range(),
-                markup! {
-                    "Variable is read here"
-                },
-            )
-            .note(markup! {
-                "This note will give you more information"
-            }),
-        )
+    fn diagnostic(_: &RuleContext<Self>, state: &Self::State) -> Option<RuleDiagnostic> {
+        let diagnostic = RuleDiagnostic::new(
+            rule_category!(),
+            state.range(),
+            format!(
+                "Duplicate class member name {:?}",
+                state.member_name().unwrap()
+            ),
+        );
+
+        Some(diagnostic)
     }
 }
diff --git a/crates/rome_js_analyze/tests/specs/nursery/noDuplicateClassMembers/invalid.js b/crates/rome_js_analyze/tests/specs/nursery/noDuplicateClassMembers/invalid.js
@@ -1,5 +1,11 @@
-
-
- var a = 1;
-a = 2;
- a = 3;
+class A { foo() {} foo() {} }
+!class A { foo() {} foo() {} };
+class A { foo() {} foo() {} foo() {} }
+class A { static foo() {} static foo() {} }
+class A { foo() {} get foo() {} }
+class A { set foo(value) {} foo() {} }
+class A { foo; foo; }
+class A { #foo; #foo; }
+class A { 'foo'() {} 'foo'() {} }
+class A { foo() {} 'foo'() {} }
+class A { static constructor() {} static 'constructor'() {} }