diff --git a/rust/rubydex/src/indexing/local_graph.rs b/rust/rubydex/src/indexing/local_graph.rs index df29e6cc..11284c22 100644 --- a/rust/rubydex/src/indexing/local_graph.rs +++ b/rust/rubydex/src/indexing/local_graph.rs @@ -5,9 +5,11 @@ use crate::model::definitions::Definition; use crate::model::document::Document; use crate::model::graph::NameDependent; use crate::model::identity_maps::IdentityHashMap; -use crate::model::ids::{ConstantReferenceId, DefinitionId, MethodReferenceId, NameId, StringId, UriId}; +use crate::model::ids::{ + ConstantReferenceId, DefinitionId, InstanceVariableReferenceId, MethodReferenceId, NameId, StringId, UriId, +}; use crate::model::name::{Name, NameRef}; -use crate::model::references::{ConstantReference, MethodRef}; +use crate::model::references::{ConstantReference, InstanceVariableRef, InstanceVariableReference, MethodRef}; use crate::model::string_ref::StringRef; use crate::offset::Offset; @@ -19,6 +21,7 @@ type LocalGraphParts = ( IdentityHashMap, IdentityHashMap, IdentityHashMap, + IdentityHashMap, IdentityHashMap>, ); @@ -31,6 +34,7 @@ pub struct LocalGraph { names: IdentityHashMap, constant_references: IdentityHashMap, method_references: IdentityHashMap, + instance_variable_references: IdentityHashMap, name_dependents: IdentityHashMap>, } @@ -45,6 +49,7 @@ impl LocalGraph { names: IdentityHashMap::default(), constant_references: IdentityHashMap::default(), method_references: IdentityHashMap::default(), + instance_variable_references: IdentityHashMap::default(), name_dependents: IdentityHashMap::default(), } } @@ -187,6 +192,27 @@ impl LocalGraph { reference_id } + // Instance variable references + + #[must_use] + pub fn instance_variable_references( + &self, + ) -> &IdentityHashMap { + &self.instance_variable_references + } + + pub fn add_instance_variable_reference(&mut self, reference: InstanceVariableRef) -> InstanceVariableReferenceId { + let reference_id = reference.id(); + let entry = InstanceVariableReference::Unresolved(Box::new(reference)); + + if self.instance_variable_references.insert(reference_id, entry).is_some() { + debug_assert!(false, "InstanceVariableReferenceId collision in local graph"); + } + + self.document.add_instance_variable_reference(reference_id); + reference_id + } + // Diagnostics #[must_use] @@ -218,6 +244,7 @@ impl LocalGraph { self.names, self.constant_references, self.method_references, + self.instance_variable_references, self.name_dependents, ) } diff --git a/rust/rubydex/src/indexing/ruby_indexer.rs b/rust/rubydex/src/indexing/ruby_indexer.rs index 193bb370..19e9fe80 100644 --- a/rust/rubydex/src/indexing/ruby_indexer.rs +++ b/rust/rubydex/src/indexing/ruby_indexer.rs @@ -13,7 +13,7 @@ use crate::model::definitions::{ use crate::model::document::Document; use crate::model::ids::{DefinitionId, NameId, StringId, UriId}; use crate::model::name::{Name, ParentScope}; -use crate::model::references::{ConstantReference, MethodRef}; +use crate::model::references::{ConstantReference, InstanceVariableRef, MethodRef}; use crate::model::visibility::Visibility; use crate::offset::Offset; @@ -557,6 +557,15 @@ impl<'a> RubyIndexer<'a> { definition_id } + fn add_instance_variable_reference(&mut self, location: &ruby_prism::Location) { + let name = Self::location_to_string(location); + let str_id = self.local_graph.intern_string(name); + let offset = Offset::from_prism_location(location); + let parent_nesting_id = self.parent_nesting_id(); + let reference = InstanceVariableRef::new(str_id, self.uri_id, offset, parent_nesting_id); + self.local_graph.add_instance_variable_reference(reference); + } + /// Adds a class variable definition. /// /// Class variables use lexical scoping - they belong to the lexically enclosing class/module, @@ -2256,7 +2265,7 @@ impl Visit<'_> for RubyIndexer<'_> { } fn visit_instance_variable_operator_write_node(&mut self, node: &ruby_prism::InstanceVariableOperatorWriteNode) { - self.add_instance_variable_definition(&node.name_loc()); + self.add_instance_variable_reference(&node.name_loc()); self.visit(&node.value()); } @@ -2270,6 +2279,10 @@ impl Visit<'_> for RubyIndexer<'_> { self.visit(&node.value()); } + fn visit_instance_variable_read_node(&mut self, node: &ruby_prism::InstanceVariableReadNode) { + self.add_instance_variable_reference(&node.location()); + } + fn visit_class_variable_and_write_node(&mut self, node: &ruby_prism::ClassVariableAndWriteNode) { self.add_class_variable_definition(&node.name_loc()); self.visit(&node.value()); diff --git a/rust/rubydex/src/indexing/ruby_indexer_tests.rs b/rust/rubydex/src/indexing/ruby_indexer_tests.rs index e6ca7651..7b60bc53 100644 --- a/rust/rubydex/src/indexing/ruby_indexer_tests.rs +++ b/rust/rubydex/src/indexing/ruby_indexer_tests.rs @@ -85,6 +85,34 @@ macro_rules! assert_method_references_eq { }}; } +macro_rules! assert_instance_variable_references_eq { + ($context:expr, $expected_names:expr) => {{ + let mut actual_references = $context + .graph() + .instance_variable_references() + .values() + .map(|r| { + ( + r.offset().start(), + $context.graph().strings().get(r.str_id()).unwrap().as_str(), + ) + }) + .collect::>(); + + actual_references.sort(); + + let actual_names = actual_references.iter().map(|(_, name)| *name).collect::>(); + + assert_eq!( + $expected_names, + actual_names.as_slice(), + "instance variable references mismatch: expected `{:?}`, got `{:?}`", + $expected_names, + actual_names + ); + }}; +} + fn index_source(source: &str) -> LocalGraphTest { LocalGraphTest::new("file:///foo.rb", source) } @@ -499,6 +527,38 @@ mod variable_tests { }); } + #[test] + fn instance_variable_reads_are_indexed_as_references() { + let context = index_source({ + " + class Foo + def bar + @baz + end + end + " + }); + + assert_no_local_diagnostics!(&context); + assert_instance_variable_references_eq!(context, ["@baz"]); + } + + #[test] + fn instance_variable_operator_writes_are_references() { + let context = index_source({ + " + class Foo + def bar + @baz += 1 + end + end + " + }); + + assert_no_local_diagnostics!(&context); + assert_instance_variable_references_eq!(context, ["@baz"]); + } + #[test] fn index_instance_variable_definition() { let context = index_source({ @@ -549,40 +609,26 @@ mod variable_tests { }); }); - assert_definition_at!(&context, "8:1-8:5", InstanceVariable, |def| { - assert_def_str_eq!(&context, def, "@bar"); - assert!(def.lexical_nesting_id().is_none()); - }); - assert_definition_at!(&context, "9:1-9:5", InstanceVariable, |def| { assert_def_str_eq!(&context, def, "@baz"); assert!(def.lexical_nesting_id().is_none()); }); - assert_definition_at!(&context, "10:1-10:5", InstanceVariable, |def| { - assert_def_str_eq!(&context, def, "@qux"); - assert!(def.lexical_nesting_id().is_none()); - }); - assert_definition_at!(&context, "12:1-16:4", Class, |bar_class_def| { - assert_definition_at!(&context, "13:3-13:7", InstanceVariable, |def| { - assert_def_str_eq!(&context, def, "@foo"); - assert_eq!(bar_class_def.id(), def.lexical_nesting_id().unwrap()); - assert_eq!(bar_class_def.members()[0], def.id()); - }); - assert_definition_at!(&context, "14:3-14:7", InstanceVariable, |def| { assert_def_str_eq!(&context, def, "@bar"); assert_eq!(bar_class_def.id(), def.lexical_nesting_id().unwrap()); - assert_eq!(bar_class_def.members()[1], def.id()); + assert_eq!(bar_class_def.members()[0], def.id()); }); assert_definition_at!(&context, "15:3-15:7", InstanceVariable, |def| { assert_def_str_eq!(&context, def, "@baz"); assert_eq!(bar_class_def.id(), def.lexical_nesting_id().unwrap()); - assert_eq!(bar_class_def.members()[2], def.id()); + assert_eq!(bar_class_def.members()[1], def.id()); }); }); + + assert_instance_variable_references_eq!(context, ["@bar", "@foo"]); } #[test] diff --git a/rust/rubydex/src/model/declaration.rs b/rust/rubydex/src/model/declaration.rs index 91c775a7..fbf16230 100644 --- a/rust/rubydex/src/model/declaration.rs +++ b/rust/rubydex/src/model/declaration.rs @@ -496,6 +496,43 @@ impl Declaration { _ => unreachable!("Cannot remove constant reference from {} declaration", self.kind()), } } + + /// Returns the instance variable reference IDs for `InstanceVariable` declarations. + /// Returns `None` for other declaration types. + #[must_use] + pub fn instance_variable_references(&self) -> Option<&IdentityHashSet> { + match self { + Declaration::InstanceVariable(it) => Some(it.references()), + _ => None, + } + } + + /// Adds an instance variable reference to this declaration. + /// + /// # Panics + /// + /// Panics if called on a declaration that doesn't track instance variable references. + pub fn add_instance_variable_reference(&mut self, reference_id: InstanceVariableReferenceId) { + match self { + Declaration::InstanceVariable(it) => it.add_reference(reference_id), + _ => unreachable!("Cannot add instance variable reference to {} declaration", self.kind()), + } + } + + /// Removes an instance variable reference from this declaration. + /// + /// # Panics + /// + /// Panics if called on a declaration that doesn't track instance variable references. + pub fn remove_instance_variable_reference(&mut self, reference_id: &InstanceVariableReferenceId) { + match self { + Declaration::InstanceVariable(it) => it.remove_reference(reference_id), + _ => unreachable!( + "Cannot remove instance variable reference from {} declaration", + self.kind() + ), + } + } } #[derive(Debug)] diff --git a/rust/rubydex/src/model/document.rs b/rust/rubydex/src/model/document.rs index 339a5ae5..bd35c90e 100644 --- a/rust/rubydex/src/model/document.rs +++ b/rust/rubydex/src/model/document.rs @@ -4,7 +4,7 @@ use line_index::LineIndex; use url::Url; use crate::diagnostic::Diagnostic; -use crate::model::ids::{ConstantReferenceId, DefinitionId, MethodReferenceId}; +use crate::model::ids::{ConstantReferenceId, DefinitionId, InstanceVariableReferenceId, MethodReferenceId}; // Represents a document currently loaded into memory. Identified by its unique URI, it holds the edges to all // definitions and references discovered in it @@ -15,6 +15,7 @@ pub struct Document { definition_ids: Vec, method_reference_ids: Vec, constant_reference_ids: Vec, + instance_variable_reference_ids: Vec, diagnostics: Vec, } @@ -27,6 +28,7 @@ impl Document { definition_ids: Vec::new(), method_reference_ids: Vec::new(), constant_reference_ids: Vec::new(), + instance_variable_reference_ids: Vec::new(), diagnostics: Vec::new(), } } @@ -73,6 +75,15 @@ impl Document { self.constant_reference_ids.push(reference_id); } + #[must_use] + pub fn instance_variable_references(&self) -> &[InstanceVariableReferenceId] { + &self.instance_variable_reference_ids + } + + pub fn add_instance_variable_reference(&mut self, reference_id: InstanceVariableReferenceId) { + self.instance_variable_reference_ids.push(reference_id); + } + #[must_use] pub fn diagnostics(&self) -> &[Diagnostic] { &self.diagnostics diff --git a/rust/rubydex/src/model/graph.rs b/rust/rubydex/src/model/graph.rs index e3760c3a..1ecf97bb 100644 --- a/rust/rubydex/src/model/graph.rs +++ b/rust/rubydex/src/model/graph.rs @@ -10,9 +10,12 @@ use crate::model::definitions::{Definition, MethodVisibilityDefinition, Receiver use crate::model::document::Document; use crate::model::encoding::Encoding; use crate::model::identity_maps::{IdentityHashMap, IdentityHashSet}; -use crate::model::ids::{ConstantReferenceId, DeclarationId, DefinitionId, MethodReferenceId, NameId, StringId, UriId}; +use crate::model::ids::{ + ConstantReferenceId, DeclarationId, DefinitionId, InstanceVariableReferenceId, MethodReferenceId, NameId, StringId, + UriId, +}; use crate::model::name::{Name, NameRef, ParentScope, ResolvedName}; -use crate::model::references::{ConstantReference, MethodRef}; +use crate::model::references::{ConstantReference, InstanceVariableReference, MethodRef, ResolvedInstanceVariableRef}; use crate::model::string_ref::StringRef; use crate::model::visibility::Visibility; use crate::stats; @@ -69,6 +72,8 @@ pub struct Graph { constant_references: IdentityHashMap, // Map of method references that still need to be resolved method_references: IdentityHashMap, + // Map of instance variable references + instance_variable_references: IdentityHashMap, /// The position encoding used for LSP line/column locations. Not related to the actual encoding of the file position_encoding: Encoding, @@ -96,6 +101,7 @@ impl Graph { names: IdentityHashMap::default(), constant_references: IdentityHashMap::default(), method_references: IdentityHashMap::default(), + instance_variable_references: IdentityHashMap::default(), position_encoding: Encoding::default(), name_dependents: IdentityHashMap::default(), pending_work: Vec::default(), @@ -481,6 +487,14 @@ impl Graph { &self.method_references } + // Returns an immutable reference to the instance variable references map + #[must_use] + pub fn instance_variable_references( + &self, + ) -> &IdentityHashMap { + &self.instance_variable_references + } + #[must_use] pub fn all_diagnostics(&self) -> Vec<&Diagnostic> { let document_diagnostics = self.documents.values().flat_map(Document::diagnostics); @@ -904,6 +918,44 @@ impl Graph { .add_constant_reference(reference_id); } + /// Records a resolved instance variable reference by linking it to its target declaration. + /// + /// # Panics + /// + /// Will panic if invoked for a non existing reference id, or for a non existing declaration. + pub fn record_resolved_ivar_reference( + &mut self, + reference_id: InstanceVariableReferenceId, + declaration_id: DeclarationId, + ) { + match self.instance_variable_references.entry(reference_id) { + Entry::Occupied(entry) => match entry.remove() { + InstanceVariableReference::Unresolved(unresolved) => { + let resolved = InstanceVariableReference::Resolved(Box::new(ResolvedInstanceVariableRef::new( + *unresolved, + declaration_id, + ))); + + self.instance_variable_references.insert(reference_id, resolved); + } + already_resolved @ InstanceVariableReference::Resolved(_) => { + debug_assert!( + false, + "Tried to resolve an instance variable reference that was already resolved" + ); + self.instance_variable_references.insert(reference_id, already_resolved); + return; + } + }, + Entry::Vacant(_) => panic!("Tried to record an ivar reference for an id that doesn't exist"), + } + + self.declarations + .get_mut(&declaration_id) + .expect("Tried to record an ivar reference for a declaration that doesn't exist") + .add_instance_variable_reference(reference_id); + } + /// Handles the deletion of a document identified by `uri`. /// Returns the `UriId` of the removed document, or `None` if it didn't exist. /// @@ -920,8 +972,17 @@ impl Graph { /// Merges everything in `other` into this Graph. This method is meant to merge all graph representations from /// different threads, but not meant to handle updates to the existing global representation pub fn extend(&mut self, local_graph: LocalGraph) { - let (uri_id, document, definitions, strings, names, constant_references, method_references, name_dependents) = - local_graph.into_parts(); + let ( + uri_id, + document, + definitions, + strings, + names, + constant_references, + method_references, + instance_variable_references, + name_dependents, + ) = local_graph.into_parts(); if self.documents.insert(uri_id, document).is_some() { debug_assert!(false, "UriId collision in global graph"); @@ -973,6 +1034,16 @@ impl Graph { } } + for (ivar_ref_id, ivar_ref) in instance_variable_references { + if self + .instance_variable_references + .insert(ivar_ref_id, ivar_ref) + .is_some() + { + debug_assert!(false, "InstanceVariable ReferenceId collision in global graph"); + } + } + for (name_id, deps) in name_dependents { let global_deps = self.name_dependents.entry(name_id).or_default(); for dep in deps { @@ -1071,6 +1142,18 @@ impl Graph { } } + for ref_id in document.instance_variable_references() { + if let Some(ivar_ref) = self.instance_variable_references.remove(ref_id) { + if let InstanceVariableReference::Resolved(resolved) = &ivar_ref + && let Some(declaration) = self.declarations.get_mut(resolved.declaration_id()) + { + declaration.remove_instance_variable_reference(ref_id); + } + + self.untrack_string(*ivar_ref.str_id()); + } + } + for ref_id in document.constant_references() { if let Some(constant_ref) = self.constant_references.remove(ref_id) { // Detach from target declaration. References unresolved during invalidation @@ -1221,13 +1304,25 @@ impl Graph { } } - // Clean up owner membership and queue remaining definitions for re-resolution + // Clean up owner membership and queue remaining definitions for re-resolution. if let Some(decl) = self.declarations.get(&decl_id) { let def_ids: Vec = decl.definitions().to_vec(); let unqualified_str_id = StringId::from(&decl.unqualified_name()); let owner_id = *decl.owner_id(); let is_singleton_class = matches!(decl, Declaration::Namespace(Namespace::SingletonClass(_))); + for ref_id in decl.instance_variable_references().into_iter().flatten().copied() { + if let Entry::Occupied(entry) = self.instance_variable_references.entry(ref_id) + && matches!(entry.get(), InstanceVariableReference::Resolved(_)) + && let InstanceVariableReference::Resolved(resolved) = entry.remove() + { + self.instance_variable_references.insert( + ref_id, + InstanceVariableReference::Unresolved(Box::new(resolved.into_inner())), + ); + } + } + for def_id in def_ids { self.push_work(Unit::Definition(def_id)); } @@ -1523,8 +1618,8 @@ mod tests { use crate::model::declaration::Ancestors; use crate::test_utils::GraphTest; use crate::{ - assert_declaration_does_not_exist, assert_dependents, assert_descendants, assert_members_eq, - assert_no_diagnostics, assert_no_members, + assert_declaration_does_not_exist, assert_declaration_references_count_eq, assert_dependents, + assert_descendants, assert_members_eq, assert_no_diagnostics, assert_no_members, }; #[test] @@ -1682,6 +1777,49 @@ mod tests { } } + #[test] + fn ivar_reference_resolves_across_incremental_updates() { + let mut context = GraphTest::new(); + context.index_uri( + "file:///decl.rb", + r" + class Foo + @var = 1 + end + ", + ); + context.index_uri( + "file:///read.rb", + r" + class Bar + def Foo.baz + @var + end + end + ", + ); + + context.resolve(); + assert_declaration_references_count_eq!(context, "Foo::#@var", 1); + + context.delete_uri("file:///read.rb"); + context.resolve(); + assert_declaration_references_count_eq!(context, "Foo::#@var", 0); + + context.index_uri( + "file:///read.rb", + r" + class Bar + def Foo.baz + @var + end + end + ", + ); + context.resolve(); + assert_declaration_references_count_eq!(context, "Foo::#@var", 1); + } + #[test] fn invalidating_ancestor_chains_when_document_changes() { let mut context = GraphTest::new(); @@ -2426,7 +2564,8 @@ mod incremental_resolution_tests { use crate::{ assert_alias_targets_contain, assert_ancestors_eq, assert_constant_reference_to, assert_constant_reference_unresolved, assert_declaration_does_not_exist, assert_declaration_exists, - assert_declaration_references_count_eq, assert_members_eq, assert_no_constant_alias_target, + assert_declaration_references_count_eq, assert_ivar_reference_unresolved, assert_members_eq, + assert_no_constant_alias_target, }; const NO_ANCESTORS: [&str; 0] = []; @@ -4002,6 +4141,160 @@ mod incremental_resolution_tests { assert_declaration_exists!(incremental, "Foo::::<>#@bar"); } + #[test] + fn ivar_resolution_idempotent_under_repeated_resolves() { + let mut context = GraphTest::new(); + context.index_uri("file:///decl.rb", "class Foo; @var = 1; end"); + context.index_uri( + "file:///read.rb", + r" + class Bar + def Foo.baz + @var + end + end + ", + ); + + context.resolve(); + assert_declaration_references_count_eq!(context, "Foo::#@var", 1); + + context.resolve(); + context.resolve(); + assert_declaration_references_count_eq!(context, "Foo::#@var", 1); + } + + #[test] + fn ivar_back_ref_preserved_when_decl_file_is_reindexed() { + let mut context = GraphTest::new(); + context.index_uri("file:///decl.rb", "class Foo; @var = 1; end"); + context.index_uri( + "file:///read.rb", + r" + class Bar + def Foo.baz + @var + end + end + ", + ); + + context.resolve(); + assert_declaration_references_count_eq!(context, "Foo::#@var", 1); + + context.index_uri("file:///decl.rb", "class Foo; @var = 1; end"); + assert_ivar_reference_unresolved!(context, "@var"); + + context.resolve(); + assert_declaration_exists!(context, "Foo::#@var"); + assert_declaration_references_count_eq!(context, "Foo::#@var", 1); + } + + #[test] + fn ivar_reference_re_resolves_when_decl_file_deleted_and_re_added() { + let mut context = GraphTest::new(); + context.index_uri("file:///decl.rb", "class Foo; @var = 1; end"); + context.index_uri( + "file:///read.rb", + r" + class Bar + def Foo.baz + @var + end + end + ", + ); + + context.resolve(); + assert_declaration_exists!(context, "Foo::#@var"); + assert_declaration_references_count_eq!(context, "Foo::#@var", 1); + + context.delete_uri("file:///decl.rb"); + context.resolve(); + assert_declaration_does_not_exist!(context, "Foo::#@var"); + assert_ivar_reference_unresolved!(context, "@var"); + + context.index_uri("file:///decl.rb", "class Foo; @var = 1; end"); + context.resolve(); + assert_declaration_exists!(context, "Foo::#@var"); + assert_declaration_references_count_eq!(context, "Foo::#@var", 1); + } + + #[test] + fn ivar_reference_re_resolves_type_of_self_changes() { + let mut context = GraphTest::new(); + context.index_uri("file:///foo.rb", "class Foo; @var = 1; end"); + context.index_uri("file:///bar.rb", "class Bar; @var = 1; end"); + context.index_uri( + "file:///read.rb", + r" + class Qux + def Foo.baz + @var + end + end + ", + ); + + context.resolve(); + assert_declaration_references_count_eq!(context, "Foo::#@var", 1); + + context.index_uri( + "file:///read.rb", + r" + class Qux + def Bar.baz + @var + end + end + ", + ); + context.resolve(); + assert_declaration_references_count_eq!(context, "Foo::#@var", 0); + assert_declaration_references_count_eq!(context, "Bar::#@var", 1); + } + + #[test] + fn ivar_reference_re_resolves_if_self_ancestors_change() { + let mut context = GraphTest::new(); + context.index_uri( + "file:///foo.rb", + r" + class Foo + def initialize + @var = 1 + end + end + ", + ); + context.index_uri( + "file:///bar.rb", + r" + class Bar + def read + @var + end + end + ", + ); + + context.resolve(); + assert_declaration_references_count_eq!(context, "Foo#@var", 0); + + context.index_uri( + "file:///bar.rb", + r" + class Bar < Foo + def read + @var + end + end + ", + ); + context.resolve(); + assert_declaration_references_count_eq!(context, "Foo#@var", 1); + } + #[test] fn no_duplicate_definition_on_identical_file_delete_readd() { let source = "class Foo; def self.run; end; def run; end; end"; diff --git a/rust/rubydex/src/model/references.rs b/rust/rubydex/src/model/references.rs index bb1b5053..b4771895 100644 --- a/rust/rubydex/src/model/references.rs +++ b/rust/rubydex/src/model/references.rs @@ -1,6 +1,9 @@ use crate::{ assert_mem_size, - model::ids::{ConstantReferenceId, MethodReferenceId, NameId, StringId, UriId}, + model::ids::{ + ConstantReferenceId, DeclarationId, DefinitionId, InstanceVariableReferenceId, MethodReferenceId, NameId, + StringId, UriId, + }, offset::Offset, }; @@ -109,3 +112,153 @@ impl MethodRef { )) } } + +/// A reference to an instance variable +#[derive(Debug)] +pub struct InstanceVariableRef { + /// The interned name of the instance variable (e.g., "@foo") + str_id: StringId, + /// The document where we found the reference + uri_id: UriId, + /// The offsets inside of the document where we found the reference + offset: Offset, + /// The lexical nesting at the point of the read, used to determine the owner during resolution + lexical_nesting_id: Option, +} +assert_mem_size!(InstanceVariableRef, 32); + +impl InstanceVariableRef { + #[must_use] + pub fn new(str_id: StringId, uri_id: UriId, offset: Offset, lexical_nesting_id: Option) -> Self { + Self { + str_id, + uri_id, + offset, + lexical_nesting_id, + } + } + + #[must_use] + pub fn str_id(&self) -> &StringId { + &self.str_id + } + + #[must_use] + pub fn uri_id(&self) -> UriId { + self.uri_id + } + + #[must_use] + pub fn offset(&self) -> &Offset { + &self.offset + } + + #[must_use] + pub fn lexical_nesting_id(&self) -> Option { + self.lexical_nesting_id + } + + #[must_use] + pub fn id(&self) -> InstanceVariableReferenceId { + InstanceVariableReferenceId::from(&format!( + "{}:{}:{}-{}", + self.str_id, + self.uri_id, + self.offset.start(), + self.offset.end() + )) + } +} + +/// An instance variable reference that has been resolved to its target declaration. +#[derive(Debug)] +pub struct ResolvedInstanceVariableRef { + inner: InstanceVariableRef, + declaration_id: DeclarationId, +} +assert_mem_size!(ResolvedInstanceVariableRef, 40); + +impl ResolvedInstanceVariableRef { + #[must_use] + pub fn new(inner: InstanceVariableRef, declaration_id: DeclarationId) -> Self { + Self { inner, declaration_id } + } + + #[must_use] + pub fn inner(&self) -> &InstanceVariableRef { + &self.inner + } + + #[must_use] + pub fn declaration_id(&self) -> &DeclarationId { + &self.declaration_id + } + + /// Consumes the resolved reference and returns the underlying unresolved data. + /// Used when invalidation needs to flip a `Resolved` back to `Unresolved`. + #[must_use] + pub fn into_inner(self) -> InstanceVariableRef { + self.inner + } +} + +/// A usage of an instance variable. Mirrors `NameRef`: the resolved variant carries the +/// `DeclarationId` directly so that going from a reference to its target declaration is O(1). +#[derive(Debug)] +pub enum InstanceVariableReference { + /// Not yet linked to a declaration. + Unresolved(Box), + /// Linked to a declaration in the graph. + Resolved(Box), +} +assert_mem_size!(InstanceVariableReference, 16); + +impl InstanceVariableReference { + #[must_use] + pub fn str_id(&self) -> &StringId { + match self { + InstanceVariableReference::Unresolved(it) => it.str_id(), + InstanceVariableReference::Resolved(it) => it.inner.str_id(), + } + } + + #[must_use] + pub fn uri_id(&self) -> UriId { + match self { + InstanceVariableReference::Unresolved(it) => it.uri_id(), + InstanceVariableReference::Resolved(it) => it.inner.uri_id(), + } + } + + #[must_use] + pub fn offset(&self) -> &Offset { + match self { + InstanceVariableReference::Unresolved(it) => it.offset(), + InstanceVariableReference::Resolved(it) => it.inner.offset(), + } + } + + #[must_use] + pub fn lexical_nesting_id(&self) -> Option { + match self { + InstanceVariableReference::Unresolved(it) => it.lexical_nesting_id(), + InstanceVariableReference::Resolved(it) => it.inner.lexical_nesting_id(), + } + } + + #[must_use] + pub fn id(&self) -> InstanceVariableReferenceId { + match self { + InstanceVariableReference::Unresolved(it) => it.id(), + InstanceVariableReference::Resolved(it) => it.inner.id(), + } + } + + #[must_use] + pub fn declaration_id(&self) -> Option<&DeclarationId> { + match self { + InstanceVariableReference::Unresolved(_) => None, + InstanceVariableReference::Resolved(it) => Some(it.declaration_id()), + } + } +} diff --git a/rust/rubydex/src/resolution.rs b/rust/rubydex/src/resolution.rs index 47015b06..56cd57d9 100644 --- a/rust/rubydex/src/resolution.rs +++ b/rust/rubydex/src/resolution.rs @@ -14,8 +14,9 @@ use crate::model::{ definitions::{Definition, Mixin, Receiver}, graph::{Graph, Unit}, identity_maps::{IdentityHashBuilder, IdentityHashMap, IdentityHashSet}, - ids::{ConstantReferenceId, DeclarationId, DefinitionId, NameId, StringId}, + ids::{ConstantReferenceId, DeclarationId, DefinitionId, InstanceVariableReferenceId, NameId, StringId}, name::{Name, NameRef, ParentScope}, + references::InstanceVariableReference, }; enum Outcome { @@ -134,6 +135,7 @@ impl<'a> Resolver<'a> { self.graph.extend_work(std::mem::take(&mut self.unit_queue)); self.handle_remaining_definitions(other_ids); + self.resolve_instance_variable_references(); } /// Resolves a single constant against the graph. This method is not meant to be used by the resolution phase, but by @@ -358,146 +360,15 @@ impl<'a> Resolver<'a> { } Definition::InstanceVariable(var) => { let str_id = *var.str_id(); + let lexical_nesting_id = *var.lexical_nesting_id(); - // Top-level instance variables belong to the `
` object, not `Object`. - // We can't represent `
` yet, so skip creating declarations for these. - // TODO: Make sure we introduce `
` representation later and update this - let Some(nesting_id) = *var.lexical_nesting_id() else { + let Some(owner_id) = self.resolve_ivar_owner(lexical_nesting_id, id) else { continue; }; - let Some(nesting_def) = self.graph.definitions().get(&nesting_id) else { - continue; - }; - - match nesting_def { - // When the instance variable is inside a method body, we determine the owner based on the method's receiver - Definition::Method(method) => { - if let Some(receiver) = method.receiver() { - let receiver_decl_id = match receiver { - Receiver::SelfReceiver(def_id) => *self - .graph - .definition_id_to_declaration_id(*def_id) - .expect("SelfReceiver definition should have a declaration"), - Receiver::ConstantReceiver(name_id) => { - let Some(receiver_decl_id) = self.resolve_constant_receiver(*name_id, id) - else { - continue; - }; - receiver_decl_id - } - }; - - // Instance variable in singleton method - owned by the receiver's singleton class - let Some(owner_id) = self.get_or_create_singleton_class(receiver_decl_id, true) else { - continue; - }; - { - debug_assert!( - matches!( - self.graph.declarations().get(&owner_id), - Some(Declaration::Namespace(Namespace::SingletonClass(_))) - ), - "Instance variable in singleton method should be owned by a SingletonClass" - ); - } - self.create_declaration(str_id, id, owner_id, |name| { - Declaration::InstanceVariable(Box::new(InstanceVariableDeclaration::new( - name, owner_id, - ))) - }); - continue; - } - - // If the method has no explicit receiver, we resolve the owner based on the lexical nesting - let Some(method_owner_id) = self.resolve_lexical_owner(*method.lexical_nesting_id(), id) - else { - continue; - }; - - // If the method is in a singleton class, the instance variable belongs to the class object - // Like `class << Foo; def bar; @bar = 1; end; end`, where `@bar` is owned by `Foo::` - if let Some(decl) = self.graph.declarations().get(&method_owner_id) - && matches!(decl, Declaration::Namespace(Namespace::SingletonClass(_))) - { - // Method in singleton class - owner is the singleton class itself - self.create_declaration(str_id, id, method_owner_id, |name| { - Declaration::InstanceVariable(Box::new(InstanceVariableDeclaration::new( - name, - method_owner_id, - ))) - }); - } else { - // Regular instance method - // Create an instance variable declaration for the method's owner - self.create_declaration(str_id, id, method_owner_id, |name| { - Declaration::InstanceVariable(Box::new(InstanceVariableDeclaration::new( - name, - method_owner_id, - ))) - }); - } - } - // If the instance variable is directly in a class/module body, it belongs to the class object - // and is owned by the singleton class of that class/module - Definition::Class(_) | Definition::Module(_) => { - let nesting_decl_id = self - .graph - .definition_id_to_declaration_id(nesting_id) - .copied() - .unwrap_or(*OBJECT_ID); - - let Some(owner_id) = self.get_or_create_singleton_class(nesting_decl_id, true) else { - continue; - }; - { - debug_assert!( - matches!( - self.graph.declarations().get(&owner_id), - Some(Declaration::Namespace(Namespace::SingletonClass(_))) - ), - "Instance variable in class/module body should be owned by a SingletonClass" - ); - } - self.create_declaration(str_id, id, owner_id, |name| { - Declaration::InstanceVariable(Box::new(InstanceVariableDeclaration::new( - name, 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::::<>` - Definition::SingletonClass(_) => { - // The singleton's declaration may be missing (e.g. its receiver was - // just deleted). Re-queue and let the next resolve place `@bar` on - // the right owner instead of falling back to Object. - let Some(&singleton_class_decl_id) = self.graph.definition_id_to_declaration_id(nesting_id) - else { - self.graph.push_work(Unit::Definition(id)); - continue; - }; - let owner_id = self - .get_or_create_singleton_class(singleton_class_decl_id, true) - .expect("singleton class nesting should always be a namespace"); - { - debug_assert!( - matches!( - self.graph.declarations().get(&owner_id), - Some(Declaration::Namespace(Namespace::SingletonClass(_))) - ), - "Instance variable in singleton class body should be owned by a SingletonClass" - ); - } - self.create_declaration(str_id, id, owner_id, |name| { - Declaration::InstanceVariable(Box::new(InstanceVariableDeclaration::new( - name, owner_id, - ))) - }); - } - _ => { - panic!("Unexpected lexical nesting for instance variable: {nesting_def:?}"); - } - } + self.create_declaration(str_id, id, owner_id, |name| { + Declaration::InstanceVariable(Box::new(InstanceVariableDeclaration::new(name, owner_id))) + }); } Definition::ClassVariable(var) => { // TODO: add diagnostic on the else branch. Defining class variables at the top level crashes @@ -791,11 +662,23 @@ impl<'a> Resolver<'a> { lexical_nesting_id: Option, definition_id: DefinitionId, ) -> Option { + let resolved = self.lookup_lexical_owner(lexical_nesting_id); + if resolved.is_none() { + self.graph.push_work(Unit::Definition(definition_id)); + } + resolved + } + + /// Read-only walk of the lexical nesting chain to find the owning namespace. + /// + /// Returns `None` when the chain hits a `SingletonClass` whose declaration is missing + /// (callers that can re-queue should do so via [`Self::resolve_lexical_owner`]). + fn lookup_lexical_owner(&self, lexical_nesting_id: Option) -> Option { let mut current_nesting = lexical_nesting_id; - let resolved = loop { + loop { let Some(id) = current_nesting else { - break Some(*OBJECT_ID); + return Some(*OBJECT_ID); }; // If no declaration exists yet for this definition, walk up the lexical chain. @@ -806,7 +689,7 @@ impl<'a> Resolver<'a> { let Some(declaration_id) = self.graph.definition_id_to_declaration_id(id) else { let definition = self.graph.definitions().get(&id).unwrap(); if matches!(definition, Definition::SingletonClass(_)) { - break None; + return None; } current_nesting = *definition.lexical_nesting_id(); continue; @@ -821,27 +704,177 @@ impl<'a> Resolver<'a> { decl, Declaration::Namespace(Namespace::Class(_) | Namespace::Module(_) | Namespace::SingletonClass(_)) ) { - break Some(*declaration_id); + return Some(*declaration_id); } if matches!(decl, Declaration::ConstantAlias(_)) { // Follow the alias chain to find the target namespace. If the alias is unresolved, // the definition cannot be properly owned yet and should be retried later. - break self.resolve_to_namespace(*declaration_id); + return self.resolve_to_namespace(*declaration_id); } let definition = self.graph.definitions().get(&id).unwrap(); current_nesting = *definition.lexical_nesting_id(); - }; + } + } - if resolved.is_none() { - self.graph.push_work(Unit::Definition(definition_id)); + /// Resolves all instance variable references by linking them to their target declarations. + /// Runs as a post-pass after `handle_remaining_definitions` since ivar declarations must exist first. + fn resolve_instance_variable_references(&mut self) { + let ivar_ref_ids: Vec = self + .graph + .instance_variable_references() + .iter() + .filter_map(|(id, ivar_ref)| matches!(ivar_ref, InstanceVariableReference::Unresolved(_)).then_some(*id)) + .collect(); + + for ref_id in ivar_ref_ids { + let ivar_ref = self.graph.instance_variable_references().get(&ref_id).unwrap(); + let str_id = *ivar_ref.str_id(); + let lexical_nesting_id = ivar_ref.lexical_nesting_id(); + + let Some(owner_id) = self.find_ivar_owner(lexical_nesting_id) else { + continue; + }; + + let Some(member_decl_id) = self.find_ivar_declaration(owner_id, str_id) else { + continue; + }; + + self.graph.record_resolved_ivar_reference(ref_id, member_decl_id); } + } - resolved + /// Searches for an instance variable declaration by walking the owner's ancestor chain. + fn find_ivar_declaration(&self, owner_id: DeclarationId, str_id: StringId) -> Option { + let namespace = self.graph.declarations().get(&owner_id)?.as_namespace()?; + + for ancestor in namespace.ancestors() { + let Ancestor::Complete(ancestor_id) = ancestor else { + continue; + }; + + let Some(Declaration::Namespace(ancestor_ns)) = self.graph.declarations().get(ancestor_id) else { + continue; + }; + + if let Some(&decl_id) = ancestor_ns.member(&str_id) { + return Some(decl_id); + } + } + + None + } + + /// Resolve an instance variable owner, creating missing singleton classes and re-queueing the definition when its + /// receiver or surrounding namespace is temporarily unresolved. + fn resolve_ivar_owner( + &mut self, + lexical_nesting_id: Option, + definition_id: DefinitionId, + ) -> Option { + let nesting_id = lexical_nesting_id?; + let nesting_def = self.graph.definitions().get(&nesting_id)?; + + match nesting_def { + Definition::Method(method) => { + if let Some(receiver) = method.receiver() { + let receiver_decl_id = match receiver { + Receiver::SelfReceiver(def_id) => *self + .graph + .definition_id_to_declaration_id(*def_id) + .expect("SelfReceiver definition should have a declaration"), + Receiver::ConstantReceiver(name_id) => { + self.resolve_constant_receiver(*name_id, definition_id)? + } + }; + + self.get_or_create_singleton_class(receiver_decl_id, true) + } else { + // Owner is the method's lexical owner directly. If the method lives inside a singleton class body, + // that owner is the singleton class itself (so `@bar` belongs to `Foo::`); otherwise it's the + // surrounding regular namespace. + let method_lexical = *method.lexical_nesting_id(); + self.resolve_lexical_owner(method_lexical, definition_id) + } + } + Definition::Class(_) | Definition::Module(_) => { + let nesting_decl_id = self + .graph + .definition_id_to_declaration_id(nesting_id) + .copied() + .unwrap_or(*OBJECT_ID); + + self.get_or_create_singleton_class(nesting_decl_id, true) + } + Definition::SingletonClass(_) => { + // The singleton's declaration may be missing (e.g. its receiver was just deleted). Re-queue and let the + // next resolve place `@bar` on the right owner instead of falling back to Object. + let Some(&singleton_class_decl_id) = self.graph.definition_id_to_declaration_id(nesting_id) else { + self.graph.push_work(Unit::Definition(definition_id)); + return None; + }; + + Some( + self.get_or_create_singleton_class(singleton_class_decl_id, true) + .expect("singleton class nesting should always be a namespace"), + ) + } + other => panic!("Unexpected lexical nesting for instance variable: {other:?}"), + } + } + + /// Read-only sibling of [`Self::resolve_ivar_owner`] used during the post-pass that resolves instance variable + /// references. Cannot create singleton classes or re-queue work — returns `None` whenever the corresponding + /// definition path would have done either. + fn find_ivar_owner(&self, lexical_nesting_id: Option) -> Option { + let nesting_id = lexical_nesting_id?; + let nesting_def = self.graph.definitions().get(&nesting_id)?; + + match nesting_def { + Definition::Method(method) => { + if let Some(receiver) = method.receiver() { + let receiver_decl_id = match receiver { + Receiver::SelfReceiver(def_id) => *self.graph.definition_id_to_declaration_id(*def_id)?, + Receiver::ConstantReceiver(name_id) => { + let NameRef::Resolved(resolved) = self.graph.names().get(name_id)? else { + return None; + }; + *resolved.declaration_id() + } + }; + self.lookup_singleton_class(receiver_decl_id) + } else { + self.lookup_lexical_owner(*method.lexical_nesting_id()) + } + } + Definition::Class(_) | Definition::Module(_) => { + let nesting_decl_id = self + .graph + .definition_id_to_declaration_id(nesting_id) + .copied() + .unwrap_or(*OBJECT_ID); + self.lookup_singleton_class(nesting_decl_id) + } + Definition::SingletonClass(_) => { + let &singleton_class_decl_id = self.graph.definition_id_to_declaration_id(nesting_id)?; + self.lookup_singleton_class(singleton_class_decl_id) + } + _ => None, + } + } + + /// Read-only lookup of an attached declaration's existing singleton class. + fn lookup_singleton_class(&self, attached_id: DeclarationId) -> Option { + self.graph + .declarations() + .get(&attached_id)? + .as_namespace()? + .singleton_class() + .copied() } - /// Gets or creates a singleton class declaration for a given class/module declaration. For class `Foo`, this + /// Gets or creates a singleton class declaration for a given class/module declaration. For class `Foo`, this /// returns the declaration for `Foo::`. /// /// If the declaration is a `Constant` with all-promotable definitions, it is automatically promoted to a `Class` diff --git a/rust/rubydex/src/resolution_tests.rs b/rust/rubydex/src/resolution_tests.rs index 0ab164d1..7b9f2a9d 100644 --- a/rust/rubydex/src/resolution_tests.rs +++ b/rust/rubydex/src/resolution_tests.rs @@ -3,8 +3,9 @@ use crate::{ assert_alias_targets_contain, assert_ancestors_eq, assert_constant_alias_target_eq, assert_constant_reference_to, assert_constant_reference_unresolved, assert_declaration_definitions_count_eq, assert_declaration_does_not_exist, assert_declaration_exists, assert_declaration_kind_eq, assert_declaration_references_count_eq, assert_descendants, - assert_diagnostics_eq, assert_instance_variables_eq, assert_members_eq, assert_no_constant_alias_target, - assert_no_diagnostics, assert_no_members, assert_owner_eq, assert_singleton_class_eq, + assert_diagnostics_eq, assert_instance_variables_eq, assert_ivar_reference_unresolved, assert_members_eq, + assert_no_constant_alias_target, assert_no_diagnostics, assert_no_members, assert_owner_eq, + assert_singleton_class_eq, diagnostic::Rule, model::{declaration::Ancestors, ids::DeclarationId, name::NameRef}, test_utils::GraphTest, @@ -3692,6 +3693,229 @@ mod fqn_and_naming_tests { } } +#[test] +fn ivar_read_resolves_to_declaration() { + let mut context = GraphTest::new(); + context.index_uri( + "file:///foo.rb", + r" + class Foo + def initialize + @bar = 1 + end + + def baz + @bar + end + end + ", + ); + context.resolve(); + + assert_no_diagnostics!(&context); + assert_declaration_references_count_eq!(context, "Foo#@bar", 1); +} + +#[test] +fn ivar_class_instance_variable_read_resolves() { + let mut context = GraphTest::new(); + context.index_uri( + "file:///foo.rb", + r" + class Foo + @bar = 1 + + def self.baz + @bar + end + end + ", + ); + context.resolve(); + + assert_no_diagnostics!(&context); + assert_declaration_references_count_eq!(context, "Foo::#@bar", 1); +} + +#[test] +fn ivar_incremental_delete_removes_references() { + let mut context = GraphTest::new(); + context.index_uri( + "file:///a.rb", + r" + class Foo + def initialize + @bar = 1 + end + end + ", + ); + context.index_uri( + "file:///b.rb", + r" + class Foo + def baz + @bar + end + end + ", + ); + + context.resolve(); + assert_declaration_references_count_eq!(context, "Foo#@bar", 1); + + // Delete the file containing the read + context.delete_uri("file:///b.rb"); + context.resolve(); + + assert_declaration_references_count_eq!(context, "Foo#@bar", 0); +} + +#[test] +fn ivar_read_resolves_through_included_module() { + let mut context = GraphTest::new(); + context.index_uri( + "file:///foo.rb", + r" + module Describable + def initialize + @name = '' + end + end + + class Foo + include Describable + + def display + @name + end + end + ", + ); + context.resolve(); + + assert_no_diagnostics!(&context); + assert_declaration_references_count_eq!(context, "Describable#@name", 1); +} + +#[test] +fn ivar_read_resolves_through_superclass() { + let mut context = GraphTest::new(); + context.index_uri( + "file:///foo.rb", + r" + class Parent + def initialize + @bar = 1 + end + end + + class Child < Parent + def baz + @bar + end + end + ", + ); + context.resolve(); + + assert_no_diagnostics!(&context); + assert_declaration_references_count_eq!(context, "Parent#@bar", 1); +} + +#[test] +fn ivar_reference_carries_resolved_declaration_id() { + let mut context = GraphTest::new(); + context.index_uri( + "file:///foo.rb", + r" + class Foo + def initialize + @bar = 1 + end + + def baz + @bar + end + end + ", + ); + context.resolve(); + + assert_declaration_references_count_eq!(context, "Foo#@bar", 1); +} + +#[test] +fn ivar_reference_inside_method_with_receiver() { + let mut context = GraphTest::new(); + context.index_uri( + "file:///foo.rb", + r" + class Foo + @var = 1 + end + + class Bar + def Foo.baz + @var + end + end + ", + ); + context.resolve(); + + assert_no_diagnostics!(&context); + assert_declaration_references_count_eq!(context, "Foo::#@var", 1); +} + +#[test] +fn ivar_reference_in_singleton_class_body() { + let mut context = GraphTest::new(); + context.index_uri( + "file:///foo.rb", + r" + class Foo + class << self + @bar = 1 + @bar + + def self.bar + @bar + end + end + end + ", + ); + context.resolve(); + + assert_no_diagnostics!(&context, &[Rule::ParseWarning]); + assert_declaration_references_count_eq!(context, "Foo::::<>#@bar", 2); +} + +#[test] +fn ivar_reference_unresolved_when_not_defined_in_ancestor() { + let mut context = GraphTest::new(); + context.index_uri( + "file:///foo.rb", + r" + class Foo + end + + class Bar < Foo + def bar + @bar + end + end + ", + ); + context.resolve(); + + assert_no_diagnostics!(&context); + assert_declaration_does_not_exist!(context, "Foo#@bar"); + assert_declaration_does_not_exist!(context, "Bar#@bar"); + assert_ivar_reference_unresolved!(context, "@bar"); +} + mod todo_tests { use super::*; diff --git a/rust/rubydex/src/test_utils/graph_test.rs b/rust/rubydex/src/test_utils/graph_test.rs index 6a1a673c..d08ceb50 100644 --- a/rust/rubydex/src/test_utils/graph_test.rs +++ b/rust/rubydex/src/test_utils/graph_test.rs @@ -439,6 +439,28 @@ macro_rules! assert_constant_reference_unresolved { }; } +#[cfg(test)] +#[macro_export] +macro_rules! assert_ivar_reference_unresolved { + ($context:expr, $ivar_name:expr) => { + let ivar_ref = $context + .graph() + .instance_variable_references() + .values() + .find(|r| $context.graph().strings().get(r.str_id()).unwrap().as_str() == $ivar_name) + .unwrap_or_else(|| panic!("No instance variable reference with name `{}`", $ivar_name)); + + assert!( + matches!( + ivar_ref, + $crate::model::references::InstanceVariableReference::Unresolved(_) + ), + "Expected instance variable reference `{}` to be unresolved, but it was resolved", + $ivar_name + ); + }; +} + #[cfg(test)] #[macro_export] macro_rules! assert_ancestors_eq {