Add resolution diagnostic for declaration kind mismatch

rust/rubydex/src/diagnostic.rs

@@ -19,11 +19,6 @@ impl Diagnostic {
         }
     }
 
-    #[must_use]
-    pub fn make(rule: Rule, uri_id: UriId, offset: Offset, message: String) -> Self {
-        Self::new(rule, uri_id, offset, message)
-    }
-
     #[must_use]
     pub fn rule(&self) -> &Rule {
         &self.rule
@@ -108,4 +103,5 @@ rules! {
     TopLevelMixinSelf;
 
     // Resolution
+    KindRedefinition;
 }

rust/rubydex/src/indexing/local_graph.rs

@@ -131,7 +131,7 @@ impl LocalGraph {
     }
 
     pub fn add_diagnostic(&mut self, rule: Rule, offset: Offset, message: String) {
-        let diagnostic = Diagnostic::make(rule, self.uri_id, offset, message);
+        let diagnostic = Diagnostic::new(rule, self.uri_id, offset, message);
         self.diagnostics.push(diagnostic);
     }
 

rust/rubydex/src/model/declaration.rs

@@ -53,6 +53,34 @@ impl<'a> IntoIterator for &'a Ancestors {
     }
 }
 
+#[derive(PartialEq, Eq)]
+pub enum DeclarationKind {
+    Class,
+    SingletonClass,
+    Module,
+    Constant,
+    Method,
+    GlobalVariable,
+    InstanceVariable,
+    ClassVariable,
+}
+
+impl DeclarationKind {
+    #[must_use]
+    pub fn as_str(&self) -> &'static str {
+        match self {
+            DeclarationKind::Class => "class",
+            DeclarationKind::SingletonClass => "singleton class",
+            DeclarationKind::Module => "module",
+            DeclarationKind::Constant => "constant",
+            DeclarationKind::Method => "method",
+            DeclarationKind::GlobalVariable => "global variable",
+            DeclarationKind::InstanceVariable => "instance variable",
+            DeclarationKind::ClassVariable => "class variable",
+        }
+    }
+}
+
 macro_rules! all_declarations {
     ($value:expr, $var:ident => $expr:expr) => {
         match $value {
@@ -229,6 +257,36 @@ pub enum Declaration {
 }
 
 impl Declaration {
+    #[must_use]
+    pub fn build(fully_qualified_name: String, kind: &DeclarationKind, owner_id: DeclarationId) -> Self {
+        match kind {
+            DeclarationKind::Class => {
+                Declaration::Class(Box::new(ClassDeclaration::new(fully_qualified_name, owner_id)))
+            }
+            DeclarationKind::SingletonClass => {
+                Declaration::SingletonClass(Box::new(SingletonClassDeclaration::new(fully_qualified_name, owner_id)))
+            }
+            DeclarationKind::Module => {
+                Declaration::Module(Box::new(ModuleDeclaration::new(fully_qualified_name, owner_id)))
+            }
+            DeclarationKind::Constant => {
+                Declaration::Constant(Box::new(ConstantDeclaration::new(fully_qualified_name, owner_id)))
+            }
+            DeclarationKind::Method => {
+                Declaration::Method(Box::new(MethodDeclaration::new(fully_qualified_name, owner_id)))
+            }
+            DeclarationKind::GlobalVariable => {
+                Declaration::GlobalVariable(Box::new(GlobalVariableDeclaration::new(fully_qualified_name, owner_id)))
+            }
+            DeclarationKind::InstanceVariable => Declaration::InstanceVariable(Box::new(
+                InstanceVariableDeclaration::new(fully_qualified_name, owner_id),
+            )),
+            DeclarationKind::ClassVariable => {
+                Declaration::ClassVariable(Box::new(ClassVariableDeclaration::new(fully_qualified_name, owner_id)))
+            }
+        }
+    }
+
     // Extend this declaration with more definitions by moving `other.definition_ids` inside
     pub fn extend(&mut self, other: Declaration) {
         all_declarations!(self, it => {
@@ -244,16 +302,16 @@ impl Declaration {
     }
 
     #[must_use]
-    pub fn kind(&self) -> &'static str {
+    pub fn kind(&self) -> DeclarationKind {
         match self {
-            Declaration::Class(_) => "Class",
-            Declaration::SingletonClass(_) => "SingletonClass",
-            Declaration::Module(_) => "Module",
-            Declaration::Constant(_) => "Constant",
-            Declaration::Method(_) => "Method",
-            Declaration::GlobalVariable(_) => "GlobalVariable",
-            Declaration::InstanceVariable(_) => "InstanceVariable",
-            Declaration::ClassVariable(_) => "ClassVariable",
+            Declaration::Class(_) => DeclarationKind::Class,
+            Declaration::SingletonClass(_) => DeclarationKind::SingletonClass,
+            Declaration::Module(_) => DeclarationKind::Module,
+            Declaration::Constant(_) => DeclarationKind::Constant,
+            Declaration::Method(_) => DeclarationKind::Method,
+            Declaration::GlobalVariable(_) => DeclarationKind::GlobalVariable,
+            Declaration::InstanceVariable(_) => DeclarationKind::InstanceVariable,
+            Declaration::ClassVariable(_) => DeclarationKind::ClassVariable,
         }
     }
 

rust/rubydex/src/model/graph.rs

@@ -1,9 +1,9 @@
 use std::collections::hash_map::Entry;
 use std::sync::LazyLock;
 
-use crate::diagnostic::Diagnostic;
+use crate::diagnostic::{Diagnostic, Rule};
 use crate::indexing::local_graph::LocalGraph;
-use crate::model::declaration::{Ancestor, Declaration};
+use crate::model::declaration::{Ancestor, Declaration, DeclarationKind};
 use crate::model::definitions::Definition;
 use crate::model::document::Document;
 use crate::model::identity_maps::{IdentityHashMap, IdentityHashSet};
@@ -73,14 +73,71 @@ impl Graph {
     /// # Panics
     ///
     /// Panics if acquiring a write lock fails
-    pub fn add_declaration<F>(&mut self, declaration_id: DeclarationId, definition_id: DefinitionId, constructor: F)
-    where
-        F: FnOnce() -> Declaration,
-    {
+    pub fn add_declaration(
+        &mut self,
+        fully_qualified_name: String,
+        declaration_kind: &DeclarationKind,
+        owner_id: DeclarationId,
+        definition_id: DefinitionId,
+    ) -> DeclarationId {
+        let declaration_id = DeclarationId::from(&fully_qualified_name);
+
+        let entry = self.declarations.entry(declaration_id);
+        let diagnostics = &mut self.diagnostics;
+
+        let declaration = match entry {
+            Entry::Occupied(entry) => {
+                // We already have a declaration for this name
+
+                let old_declaration = entry.get();
+                let old_kind = old_declaration.kind();
+
+                if *declaration_kind != old_kind {
+                    // We're trying to redefine the declaration with a different kind, let's produce a diagnostic
+
+                    let old_definition_id = old_declaration.definitions().first().unwrap();
+                    let old_definition = self.definitions.get(old_definition_id).unwrap();
+                    let new_definition = self.definitions.get(&definition_id).unwrap();
+
+                    let mut definitions = [
+                        (old_definition, old_kind.as_str()),
+                        (new_definition, declaration_kind.as_str()),
+                    ];
+
+                    // We want to show the diagnostic for the most recent definition, so we sort by uri and offset
+                    definitions.sort_by_key(|(d, _)| {
+                        let uri_id = d.uri_id();
+                        let uri = self.documents.get(uri_id).unwrap().uri();
+                        let offset = d.offset();
+                        (uri, offset)
+                    });
+
+                    let (_first_definition, first_kind) = definitions.first().unwrap();
+                    let (last_definition, last_kind) = definitions.last().unwrap();
+
+                    diagnostics.push(Diagnostic::new(
+                        Rule::KindRedefinition,
+                        *last_definition.uri_id(),
+                        last_definition.offset().clone(),
+                        format!(
+                            "Redefining `{fully_qualified_name}` as `{last_kind}`, previously defined as `{first_kind}`"
+                        ),
+                    ));
+                }
+
+                entry.into_mut()
+            }
+            Entry::Vacant(entry) => {
+                let declaration = Declaration::build(fully_qualified_name, declaration_kind, owner_id);
+                entry.insert(declaration)
+            }
+        };
+
         self.definitions_to_declarations.insert(definition_id, declaration_id);
 
-        let declaration = self.declarations.entry(declaration_id).or_insert_with(constructor);
         declaration.add_definition(definition_id);
+
+        declaration_id
     }
 
     /// # Panics
@@ -164,11 +221,17 @@ impl Graph {
         &self.method_references
     }
 
+    // Diagnostics
+
     #[must_use]
     pub fn diagnostics(&self) -> &Vec<Diagnostic> {
         &self.diagnostics
     }
 
+    pub fn add_diagnostic(&mut self, diagnostic: Diagnostic) {
+        self.diagnostics.push(diagnostic);
+    }
+
     /// # Panics
     ///
     /// Panics if acquiring a read lock fails
@@ -507,7 +570,7 @@ impl Graph {
                 }
             }
 
-            *declarations_types.entry(declaration.kind()).or_insert(0) += 1;
+            *declarations_types.entry(declaration.kind().as_str()).or_insert(0) += 1;
 
             // Count definitions by type
             let definition_count = declaration.definitions().len();
@@ -704,12 +767,14 @@ mod tests {
         assert!(matches!(baz_ancestors, Ancestors::Complete(_)));
 
         {
-            let Declaration::Module(bar) = context.graph().declarations().get(&DeclarationId::from("Bar")).unwrap() else {
+            let Declaration::Module(bar) = context.graph().declarations().get(&DeclarationId::from("Bar")).unwrap()
+            else {
                 panic!("Expected Bar to be a module");
             };
             assert!(bar.descendants().contains(&DeclarationId::from("Foo")));
 
-            let Declaration::Class(foo) = context.graph().declarations().get(&DeclarationId::from("Foo")).unwrap() else {
+            let Declaration::Class(foo) = context.graph().declarations().get(&DeclarationId::from("Foo")).unwrap()
+            else {
                 panic!("Expected Foo to be a class");
             };
             assert!(foo.descendants().contains(&DeclarationId::from("Baz")));
@@ -718,19 +783,22 @@ mod tests {
         context.index_uri("file:///a.rb", "");
 
         {
-            let Declaration::Class(foo) = context.graph().declarations().get(&DeclarationId::from("Foo")).unwrap() else {
+            let Declaration::Class(foo) = context.graph().declarations().get(&DeclarationId::from("Foo")).unwrap()
+            else {
                 panic!("Expected Foo to be a class");
             };
             assert!(matches!(foo.clone_ancestors(), Ancestors::Partial(a) if a.is_empty()));
             assert!(foo.descendants().is_empty());
 
-            let Declaration::Class(baz) = context.graph().declarations().get(&DeclarationId::from("Baz")).unwrap() else {
+            let Declaration::Class(baz) = context.graph().declarations().get(&DeclarationId::from("Baz")).unwrap()
+            else {
                 panic!("Expected Baz to be a class");
             };
             assert!(matches!(baz.clone_ancestors(), Ancestors::Partial(a) if a.is_empty()));
             assert!(baz.descendants().is_empty());
 
-            let Declaration::Module(bar) = context.graph().declarations().get(&DeclarationId::from("Bar")).unwrap() else {
+            let Declaration::Module(bar) = context.graph().declarations().get(&DeclarationId::from("Bar")).unwrap()
+            else {
                 panic!("Expected Bar to be a module");
             };
             assert!(!bar.descendants().contains(&DeclarationId::from("Foo")));
@@ -742,7 +810,8 @@ mod tests {
         assert!(matches!(baz_ancestors, Ancestors::Complete(_)));
 
         {
-            let Declaration::Class(foo) = context.graph().declarations().get(&DeclarationId::from("Foo")).unwrap() else {
+            let Declaration::Class(foo) = context.graph().declarations().get(&DeclarationId::from("Foo")).unwrap()
+            else {
                 panic!("Expected Foo to be a class");
             };
             assert!(foo.descendants().contains(&DeclarationId::from("Baz")));