diff --git a/rust/rubydex/src/diagnostic.rs b/rust/rubydex/src/diagnostic.rs index 6d105e2f..400c121e 100644 --- a/rust/rubydex/src/diagnostic.rs +++ b/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; } diff --git a/rust/rubydex/src/indexing/local_graph.rs b/rust/rubydex/src/indexing/local_graph.rs index f17fb9da..5053ed2d 100644 --- a/rust/rubydex/src/indexing/local_graph.rs +++ b/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); } diff --git a/rust/rubydex/src/model/declaration.rs b/rust/rubydex/src/model/declaration.rs index 76bd721e..7584aa3c 100644 --- a/rust/rubydex/src/model/declaration.rs +++ b/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, } } diff --git a/rust/rubydex/src/model/graph.rs b/rust/rubydex/src/model/graph.rs index 82b031e3..fc8b32ed 100644 --- a/rust/rubydex/src/model/graph.rs +++ b/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(&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 { &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"))); diff --git a/rust/rubydex/src/resolution.rs b/rust/rubydex/src/resolution.rs index b729bde9..afdd0137 100644 --- a/rust/rubydex/src/resolution.rs +++ b/rust/rubydex/src/resolution.rs @@ -4,11 +4,7 @@ use std::{ }; use crate::model::{ - declaration::{ - Ancestor, Ancestors, ClassDeclaration, ClassVariableDeclaration, ConstantDeclaration, Declaration, - GlobalVariableDeclaration, InstanceVariableDeclaration, MethodDeclaration, ModuleDeclaration, - SingletonClassDeclaration, - }, + declaration::{Ancestor, Ancestors, ClassDeclaration, Declaration, DeclarationKind, SingletonClassDeclaration}, definitions::{Definition, Mixin}, graph::{CLASS_ID, Graph, MODULE_ID, OBJECT_ID}, identity_maps::{IdentityHashMap, IdentityHashSet}, @@ -140,24 +136,16 @@ fn handle_definition_unit( ) { let outcome = match graph.definitions().get(&id).unwrap() { Definition::Class(class) => { - handle_constant_declaration(graph, *class.name_id(), id, false, |name, owner_id| { - Declaration::Class(Box::new(ClassDeclaration::new(name, owner_id))) - }) + handle_constant_declaration(graph, *class.name_id(), &DeclarationKind::Class, id, false) } Definition::Module(module) => { - handle_constant_declaration(graph, *module.name_id(), id, false, |name, owner_id| { - Declaration::Module(Box::new(ModuleDeclaration::new(name, owner_id))) - }) + handle_constant_declaration(graph, *module.name_id(), &DeclarationKind::Module, id, false) } Definition::Constant(constant) => { - handle_constant_declaration(graph, *constant.name_id(), id, false, |name, owner_id| { - Declaration::Constant(Box::new(ConstantDeclaration::new(name, owner_id))) - }) + handle_constant_declaration(graph, *constant.name_id(), &DeclarationKind::Constant, id, false) } Definition::SingletonClass(singleton) => { - handle_constant_declaration(graph, *singleton.name_id(), id, true, |name, owner_id| { - Declaration::SingletonClass(Box::new(SingletonClassDeclaration::new(name, owner_id))) - }) + handle_constant_declaration(graph, *singleton.name_id(), &DeclarationKind::SingletonClass, id, true) } _ => panic!("Expected constant definitions"), }; @@ -265,36 +253,26 @@ fn handle_remaining_definitions( resolve_lexical_owner(graph, *method_definition.lexical_nesting_id()) }; - create_declaration(graph, str_id, id, owner_id, |name| { - Declaration::Method(Box::new(MethodDeclaration::new(name, owner_id))) - }); + create_declaration(graph, str_id, &DeclarationKind::Method, id, owner_id); } Definition::AttrAccessor(attr) => { let owner_id = resolve_lexical_owner(graph, *attr.lexical_nesting_id()); - create_declaration(graph, *attr.str_id(), id, owner_id, |name| { - Declaration::Method(Box::new(MethodDeclaration::new(name, owner_id))) - }); + create_declaration(graph, *attr.str_id(), &DeclarationKind::Method, id, owner_id); } Definition::AttrReader(attr) => { let owner_id = resolve_lexical_owner(graph, *attr.lexical_nesting_id()); - create_declaration(graph, *attr.str_id(), id, owner_id, |name| { - Declaration::Method(Box::new(MethodDeclaration::new(name, owner_id))) - }); + create_declaration(graph, *attr.str_id(), &DeclarationKind::Method, id, owner_id); } Definition::AttrWriter(attr) => { let owner_id = resolve_lexical_owner(graph, *attr.lexical_nesting_id()); - create_declaration(graph, *attr.str_id(), id, owner_id, |name| { - Declaration::Method(Box::new(MethodDeclaration::new(name, owner_id))) - }); + create_declaration(graph, *attr.str_id(), &DeclarationKind::Method, id, owner_id); } Definition::GlobalVariable(var) => { let owner_id = *OBJECT_ID; - create_declaration(graph, *var.str_id(), id, owner_id, |name| { - Declaration::GlobalVariable(Box::new(GlobalVariableDeclaration::new(name, owner_id))) - }); + create_declaration(graph, *var.str_id(), &DeclarationKind::GlobalVariable, id, owner_id); } Definition::InstanceVariable(var) => { let str_id = *var.str_id(); @@ -332,11 +310,9 @@ fn handle_remaining_definitions( "Instance variable in singleton method should be owned by a SingletonClass" ); } - create_declaration(graph, str_id, id, owner_id, |name| { - Declaration::InstanceVariable(Box::new(InstanceVariableDeclaration::new( - name, owner_id, - ))) - }); + + create_declaration(graph, str_id, &DeclarationKind::InstanceVariable, id, owner_id); + continue; } @@ -349,21 +325,11 @@ fn handle_remaining_definitions( && matches!(decl, Declaration::SingletonClass(_)) { // Method in singleton class - owner is the singleton class itself - create_declaration(graph, str_id, id, method_owner_id, |name| { - Declaration::InstanceVariable(Box::new(InstanceVariableDeclaration::new( - name, - method_owner_id, - ))) - }); + create_declaration(graph, str_id, &DeclarationKind::InstanceVariable, id, method_owner_id); } else { // Regular instance method // Create an instance variable declaration for the method's owner - create_declaration(graph, str_id, id, method_owner_id, |name| { - Declaration::InstanceVariable(Box::new(InstanceVariableDeclaration::new( - name, - method_owner_id, - ))) - }); + create_declaration(graph, str_id, &DeclarationKind::InstanceVariable, id, method_owner_id); } } // If the instance variable is directly in a class/module body, it belongs to the class object @@ -384,9 +350,7 @@ fn handle_remaining_definitions( "Instance variable in class/module body should be owned by a SingletonClass" ); } - create_declaration(graph, str_id, id, owner_id, |name| { - Declaration::InstanceVariable(Box::new(InstanceVariableDeclaration::new(name, owner_id))) - }); + create_declaration(graph, str_id, &DeclarationKind::InstanceVariable, id, owner_id); } // If in a singleton class body directly, the owner is the singleton class's singleton class // Like `class << Foo; @bar = 1; end`, where `@bar` is owned by `Foo::::<>` @@ -406,9 +370,7 @@ fn handle_remaining_definitions( "Instance variable in singleton class body should be owned by a SingletonClass" ); } - create_declaration(graph, str_id, id, owner_id, |name| { - Declaration::InstanceVariable(Box::new(InstanceVariableDeclaration::new(name, owner_id))) - }); + create_declaration(graph, str_id, &DeclarationKind::InstanceVariable, id, owner_id); } _ => { panic!("Unexpected lexical nesting for instance variable: {nesting_def:?}"); @@ -418,9 +380,7 @@ fn handle_remaining_definitions( Definition::ClassVariable(var) => { // TODO: add diagnostic on the else branch. Defining class variables at the top level crashes if let Some(owner_id) = resolve_class_variable_owner(graph, *var.lexical_nesting_id()) { - create_declaration(graph, *var.str_id(), id, owner_id, |name| { - Declaration::ClassVariable(Box::new(ClassVariableDeclaration::new(name, owner_id))) - }); + create_declaration(graph, *var.str_id(), &DeclarationKind::ClassVariable, id, owner_id); } } Definition::Class(_) | Definition::SingletonClass(_) | Definition::Module(_) | Definition::Constant(_) => { @@ -429,38 +389,35 @@ fn handle_remaining_definitions( Definition::MethodAlias(alias) => { let owner_id = resolve_lexical_owner(graph, *alias.lexical_nesting_id()); - create_declaration(graph, *alias.new_name_str_id(), id, owner_id, |name| { - Declaration::Method(Box::new(MethodDeclaration::new(name, owner_id))) - }); + create_declaration(graph, *alias.new_name_str_id(), &DeclarationKind::Method, id, owner_id); } Definition::GlobalVariableAlias(alias) => { - create_declaration(graph, *alias.new_name_str_id(), id, *OBJECT_ID, |name| { - Declaration::GlobalVariable(Box::new(GlobalVariableDeclaration::new(name, *OBJECT_ID))) - }); + create_declaration( + graph, + *alias.new_name_str_id(), + &DeclarationKind::GlobalVariable, + id, + *OBJECT_ID, + ); } } } } -fn create_declaration( +fn create_declaration( graph: &mut Graph, str_id: StringId, + declaration_kind: &DeclarationKind, definition_id: DefinitionId, owner_id: DeclarationId, - declaration_builder: F, -) where - F: FnOnce(String) -> Declaration, -{ +) { let fully_qualified_name = { let owner = graph.declarations().get(&owner_id).unwrap(); let name_str = graph.strings().get(&str_id).unwrap(); format!("{}::{name_str}", owner.name()) }; - let declaration_id = DeclarationId::from(&fully_qualified_name); - graph.add_declaration(declaration_id, definition_id, || { - declaration_builder(fully_qualified_name) - }); + let declaration_id = graph.add_declaration(fully_qualified_name, declaration_kind, owner_id, definition_id); graph.add_member(&owner_id, declaration_id, str_id); } @@ -829,16 +786,13 @@ fn propagate_descendants( } // Handles the resolution of the namespace name, the creation of the declaration and membership -fn handle_constant_declaration( +fn handle_constant_declaration( graph: &mut Graph, name_id: NameId, + declaration_kind: &DeclarationKind, definition_id: DefinitionId, singleton: bool, - declaration_builder: F, -) -> Outcome -where - F: FnOnce(String, DeclarationId) -> Declaration, -{ +) -> Outcome { let name_ref = graph.names().get(&name_id).unwrap(); let str_id = *name_ref.str(); @@ -858,7 +812,7 @@ where } } - let declaration_id = DeclarationId::from(&fully_qualified_name); + let declaration_id = graph.add_declaration(fully_qualified_name, declaration_kind, owner_id, definition_id); if singleton { let owner = graph.declarations_mut().get_mut(&owner_id).unwrap(); @@ -867,9 +821,6 @@ where graph.add_member(&owner_id, declaration_id, str_id); } - graph.add_declaration(declaration_id, definition_id, || { - declaration_builder(fully_qualified_name, owner_id) - }); graph.record_resolved_name(name_id, declaration_id); Outcome::Resolved(declaration_id, id_needing_linearization) } @@ -1608,6 +1559,47 @@ mod tests { assert_constant_reference_to!(context, "Foo::Bar", "file:///bar.rb:4:5-4:8"); } + #[test] + fn resolution_diagnostics_for_kind_redefinition() { + let mut context = GraphTest::new(); + context.index_uri("file:///foo.rb", { + r" + module Foo; end + class Foo; end + + class Bar; end + module Bar; end + + class Baz; end + Baz = 123 + + module Qux; end + module Qux; end + + def foo; end + attr_reader :foo + + class Qaz + class Array; end + def Array; end + end + " + }); + + context.resolve(); + + assert_diagnostics_eq!( + &context, + vec![ + "kind-redefinition: Redefining `Foo` as `class`, previously defined as `module` (2:1-2:15)", + "kind-redefinition: Redefining `Bar` as `module`, previously defined as `class` (5:1-5:16)", + "kind-redefinition: Redefining `Baz` as `constant`, previously defined as `class` (8:1-8:4)", + // FIXME: We should allow this case: + "kind-redefinition: Redefining `Qaz::Array` as `method`, previously defined as `class` (18:3-18:17)", + ] + ); + } + #[test] fn resolving_reference_for_non_existing_declaration() { let mut context = GraphTest::new();