diff --git a/rust/rubydex/src/indexing/local_graph.rs b/rust/rubydex/src/indexing/local_graph.rs index dfa62eb8..df711b07 100644 --- a/rust/rubydex/src/indexing/local_graph.rs +++ b/rust/rubydex/src/indexing/local_graph.rs @@ -45,6 +45,11 @@ impl LocalGraph { } } + #[must_use] + pub fn empty(uri: &str) -> Self { + Self::new(UriId::from(uri), Document::new(uri.to_string(), "")) + } + #[must_use] pub fn uri_id(&self) -> UriId { self.uri_id diff --git a/rust/rubydex/src/model.rs b/rust/rubydex/src/model.rs index 35090a35..700a44da 100644 --- a/rust/rubydex/src/model.rs +++ b/rust/rubydex/src/model.rs @@ -9,5 +9,6 @@ pub mod identity_maps; pub mod ids; pub mod name; pub mod references; +pub mod shape; pub mod string_ref; pub mod visibility; diff --git a/rust/rubydex/src/model/graph.rs b/rust/rubydex/src/model/graph.rs index d771129f..b62f2372 100644 --- a/rust/rubydex/src/model/graph.rs +++ b/rust/rubydex/src/model/graph.rs @@ -3,14 +3,15 @@ use std::sync::LazyLock; use crate::diagnostic::Diagnostic; use crate::indexing::local_graph::LocalGraph; -use crate::model::declaration::{Ancestor, Declaration, Namespace}; +use crate::model::declaration::{Declaration, Namespace}; use crate::model::definitions::Definition; use crate::model::document::Document; use crate::model::encoding::Encoding; -use crate::model::identity_maps::{IdentityHashMap, IdentityHashSet}; +use crate::model::identity_maps::IdentityHashMap; use crate::model::ids::{DeclarationId, DefinitionId, NameId, ReferenceId, StringId, UriId}; use crate::model::name::{Name, NameRef, ParentScope, ResolvedName}; use crate::model::references::{ConstantReference, MethodRef}; +use crate::model::shape::{extract_shape_keys, shapes_match}; use crate::model::string_ref::StringRef; use crate::stats; @@ -18,6 +19,27 @@ pub static OBJECT_ID: LazyLock = LazyLock::new(|| DeclarationId:: pub static MODULE_ID: LazyLock = LazyLock::new(|| DeclarationId::from("Module")); pub static CLASS_ID: LazyLock = LazyLock::new(|| DeclarationId::from("Class")); +/// Result of an incremental update operation. +/// +/// Contains the definition and reference IDs that need to be resolved after an update. +/// When `update` or `delete_uri` returns `Some(IndexResult)`, incremental resolution can be used. +/// When they return `None`, a full re-resolution is required. +#[derive(Debug)] +pub struct IndexResult { + pub definition_ids: Vec, + pub reference_ids: Vec, +} + +impl IndexResult { + #[must_use] + pub fn empty() -> Self { + Self { + definition_ids: Vec::new(), + reference_ids: Vec::new(), + } + } +} + // The `Graph` is the global representation of the entire Ruby codebase. It contains all declarations and their // relationships #[derive(Default, Debug)] @@ -79,6 +101,10 @@ impl Graph { pub fn clear_declarations(&mut self) { self.declarations.clear(); + // Reset all resolved names back to unresolved so they can be re-resolved + for name_ref in self.names.values_mut() { + name_ref.reset_to_unresolved(); + } } // Returns an immutable reference to the definitions map @@ -495,10 +521,9 @@ impl Graph { } //// Handles the deletion of a document identified by `uri` - pub fn delete_uri(&mut self, uri: &str) { - let uri_id = UriId::from(uri); - self.remove_definitions_for_uri(uri_id); - self.documents.remove(&uri_id); + pub fn delete_uri(&mut self, uri: &str) -> Option { + let empty = LocalGraph::empty(uri); + self.process_update(&empty) } /// Merges everything in `other` into this Graph. This method is meant to merge all graph representations from @@ -535,13 +560,46 @@ impl Graph { /// Updates the global representation with the information contained in `other`, handling deletions, insertions and /// updates to existing entries - pub fn update(&mut self, other: LocalGraph) { + pub fn update(&mut self, other: LocalGraph) -> Option { // For each URI that was indexed through `other`, check what was discovered and update our current global // representation + let result = self.process_update(&other); + self.extend(other); + result + } + + fn process_update(&mut self, other: &LocalGraph) -> Option { let uri_id = other.uri_id(); + + let old_shape_keys = if let Some(document) = self.documents.get(&uri_id) { + let old_defs = document + .definitions() + .iter() + .filter_map(|def_id| self.definitions.get(def_id).map(|def| (*def_id, def))); + + extract_shape_keys(old_defs, |ref_id| { + self.constant_references.get(ref_id).map(|r| *r.name_id()) + }) + } else { + Vec::new() + }; + + let new_shape_keys = extract_shape_keys(other.definitions().iter().map(|(id, def)| (*id, def)), |ref_id| { + other.constant_references().get(ref_id).map(|r| *r.name_id()) + }); + + let shape_unchanged = shapes_match(&old_shape_keys, &new_shape_keys); + self.remove_definitions_for_uri(uri_id); - self.extend(other); + if shape_unchanged { + Some(IndexResult { + definition_ids: other.definitions().keys().copied().collect(), + reference_ids: other.constant_references().keys().copied().collect(), + }) + } else { + None + } } // Removes all nodes and relationships associated to the given URI. This is used to clean up stale data when a @@ -573,7 +631,6 @@ impl Graph { let mut members_to_delete: Vec<(DeclarationId, StringId)> = Vec::new(); let mut definitions_to_delete: Vec = Vec::new(); let mut declarations_to_delete: Vec = Vec::new(); - let mut declarations_to_invalidate_ancestor_chains: Vec = Vec::new(); for def_id in document.definitions() { definitions_to_delete.push(*def_id); @@ -583,9 +640,6 @@ impl Graph { && declaration.remove_definition(def_id) { declaration.clear_diagnostics(); - if declaration.as_namespace().is_some() { - declarations_to_invalidate_ancestor_chains.push(declaration_id); - } if declaration.has_no_definitions() { let unqualified_str_id = StringId::from(&declaration.unqualified_name()); @@ -605,8 +659,6 @@ impl Graph { } } - self.invalidate_ancestor_chains(declarations_to_invalidate_ancestor_chains); - for declaration_id in declarations_to_delete { self.declarations.remove(&declaration_id); } @@ -636,49 +688,6 @@ impl Graph { } } - fn invalidate_ancestor_chains(&mut self, initial_ids: Vec) { - let mut queue = initial_ids; - let mut visited = IdentityHashSet::::default(); - - while let Some(declaration_id) = queue.pop() { - if !visited.insert(declaration_id) { - continue; - } - - let namespace = self - .declarations_mut() - .get_mut(&declaration_id) - .unwrap() - .as_namespace_mut() - .expect("expected namespace declaration"); - - for ancestor in &namespace.ancestors() { - if let Ancestor::Complete(ancestor_id) = ancestor { - self.declarations_mut() - .get_mut(ancestor_id) - .unwrap() - .as_namespace_mut() - .unwrap() - .remove_descendant(&declaration_id); - } - } - - let namespace = self - .declarations_mut() - .get_mut(&declaration_id) - .unwrap() - .as_namespace_mut() - .unwrap(); - - namespace.for_each_descendant(|descendant_id| { - queue.push(*descendant_id); - }); - - namespace.clear_ancestors(); - namespace.clear_descendants(); - } - } - /// Sets the encoding that should be used for transforming byte offsets into LSP code unit line/column positions pub fn set_encoding(&mut self, encoding: Encoding) { self.position_encoding = encoding; @@ -768,9 +777,8 @@ impl Graph { mod tests { use super::*; use crate::model::comment::Comment; - use crate::model::declaration::Ancestors; use crate::test_utils::GraphTest; - use crate::{assert_descendants, assert_members_eq, assert_no_diagnostics, assert_no_members}; + use crate::{assert_members_eq, assert_no_diagnostics, assert_no_members}; #[test] fn deleting_a_uri() { @@ -778,7 +786,6 @@ mod tests { context.index_uri("file:///foo.rb", "module Foo; end"); context.delete_uri("file:///foo.rb"); - context.resolve(); assert!(context.graph().documents.is_empty()); assert!(context.graph().definitions.is_empty()); @@ -884,76 +891,6 @@ mod tests { } } - #[test] - fn invalidating_ancestor_chains_when_document_changes() { - let mut context = GraphTest::new(); - - context.index_uri("file:///a.rb", "class Foo; include Bar; def method_name; end; end"); - context.index_uri("file:///b.rb", "class Foo; end"); - context.index_uri("file:///c.rb", "module Bar; end"); - context.index_uri("file:///d.rb", "class Baz < Foo; end"); - context.resolve(); - - let foo_declaration = context.graph().declarations().get(&DeclarationId::from("Foo")).unwrap(); - assert!(matches!( - foo_declaration.as_namespace().unwrap().ancestors(), - Ancestors::Complete(_) - )); - - let baz_declaration = context.graph().declarations().get(&DeclarationId::from("Baz")).unwrap(); - assert!(matches!( - baz_declaration.as_namespace().unwrap().ancestors(), - Ancestors::Complete(_) - )); - - { - let Declaration::Namespace(Namespace::Module(_bar)) = - context.graph().declarations().get(&DeclarationId::from("Bar")).unwrap() - else { - panic!("Expected Bar to be a module"); - }; - assert_descendants!(context, "Bar", ["Foo"]); - } - assert_descendants!(context, "Foo", ["Baz"]); - - context.index_uri("file:///a.rb", ""); - - { - let Declaration::Namespace(Namespace::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::Namespace(Namespace::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::Namespace(Namespace::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"))); - } - - context.resolve(); - - let baz_declaration = context.graph().declarations().get(&DeclarationId::from("Baz")).unwrap(); - assert!(matches!( - baz_declaration.as_namespace().unwrap().ancestors(), - Ancestors::Complete(_) - )); - - assert_descendants!(context, "Foo", ["Baz"]); - } - #[test] fn name_count_increments_for_duplicates() { let mut context = GraphTest::new(); @@ -1288,6 +1225,7 @@ mod tests { context.resolve(); // Removing the method should not remove the constant context.index_uri("file:///foo2.rb", ""); + context.resolve(); let foo = context .graph() @@ -1322,6 +1260,7 @@ mod tests { context.resolve(); // Removing the method should not remove the constant context.index_uri("file:///foo.rb", ""); + context.resolve(); let foo = context .graph() @@ -1431,4 +1370,190 @@ mod tests { assert!(context.graph().get("Foo::").is_none()); assert!(context.graph().get("Foo::::<>").is_none()); } + + #[test] + fn adding_method_does_not_change_shape() { + let mut context = GraphTest::new(); + + context.index_uri("file:///foo.rb", "class Foo; end"); + context.resolve(); + + let result = context.index_uri("file:///foo.rb", "class Foo; def bar; end; end"); + assert!(result.is_some(), "Adding a method should not change shape"); + + let index_result = result.unwrap(); + assert!(!index_result.definition_ids.is_empty()); + context.resolve_incremental(&index_result); + + let foo = context + .graph() + .declarations() + .get(&DeclarationId::from("Foo")) + .unwrap() + .as_namespace() + .unwrap(); + assert!(foo.member(&StringId::from("bar()")).is_some()); + } + + #[test] + fn removing_method_does_not_change_shape() { + let mut context = GraphTest::new(); + + context.index_uri("file:///foo.rb", "class Foo; def bar; end; end"); + context.resolve(); + + let foo = context + .graph() + .declarations() + .get(&DeclarationId::from("Foo")) + .unwrap() + .as_namespace() + .unwrap(); + assert!(foo.member(&StringId::from("bar()")).is_some()); + + let result = context.index_uri("file:///foo.rb", "class Foo; end"); + assert!(result.is_some(), "Removing a method should not change shape"); + context.resolve_incremental(&result.unwrap()); + + let foo = context + .graph() + .declarations() + .get(&DeclarationId::from("Foo")) + .unwrap() + .as_namespace() + .unwrap(); + assert!(foo.member(&StringId::from("bar()")).is_none()); + } + + #[test] + fn adding_constant_does_not_change_shape() { + let mut context = GraphTest::new(); + + context.index_uri("file:///foo.rb", "class Foo; end"); + context.resolve(); + + let result = context.index_uri("file:///foo.rb", "class Foo; BAR = 1; end"); + assert!(result.is_some(), "Adding a constant should not change shape"); + context.resolve_incremental(&result.unwrap()); + + let foo = context + .graph() + .declarations() + .get(&DeclarationId::from("Foo")) + .unwrap() + .as_namespace() + .unwrap(); + assert!(foo.member(&StringId::from("BAR")).is_some()); + } + + #[test] + fn adding_class_changes_shape() { + let mut context = GraphTest::new(); + + context.index_uri("file:///foo.rb", "class Foo; end"); + context.resolve(); + + let result = context.index_uri("file:///foo.rb", "class Foo; end; class Bar; end"); + assert!(result.is_none(), "Adding a class should change shape"); + } + + #[test] + fn changing_superclass_changes_shape() { + let mut context = GraphTest::new(); + + context.index_uri("file:///foo.rb", "class Parent; end; class Foo < Parent; end"); + context.resolve(); + + let result = context.index_uri( + "file:///foo.rb", + "class Parent; end; class Other; end; class Foo < Other; end", + ); + assert!(result.is_none(), "Changing superclass should change shape"); + } + + #[test] + fn adding_include_changes_shape() { + let mut context = GraphTest::new(); + + context.index_uri("file:///foo.rb", "module Bar; end; class Foo; end"); + context.resolve(); + + let result = context.index_uri("file:///foo.rb", "module Bar; end; class Foo; include Bar; end"); + assert!(result.is_none(), "Adding include should change shape"); + } + + #[test] + fn adding_prepend_changes_shape() { + let mut context = GraphTest::new(); + + context.index_uri("file:///foo.rb", "module Bar; end; class Foo; end"); + context.resolve(); + + let result = context.index_uri("file:///foo.rb", "module Bar; end; class Foo; prepend Bar; end"); + assert!(result.is_none(), "Adding prepend should change shape"); + } + + #[test] + fn adding_extend_changes_shape() { + let mut context = GraphTest::new(); + + context.index_uri("file:///foo.rb", "module Bar; end; class Foo; end"); + context.resolve(); + + let result = context.index_uri("file:///foo.rb", "module Bar; end; class Foo; extend Bar; end"); + assert!(result.is_none(), "Adding extend should change shape"); + } + + #[test] + fn adding_constant_alias_changes_shape() { + let mut context = GraphTest::new(); + + context.index_uri("file:///foo.rb", "class Foo; end"); + context.resolve(); + + let result = context.index_uri("file:///foo.rb", "class Foo; end; Bar = Foo"); + assert!(result.is_none(), "Adding constant alias should change shape"); + } + + #[test] + fn deleting_file_with_class_changes_shape() { + let mut context = GraphTest::new(); + + context.index_uri("file:///foo.rb", "class Foo; end"); + context.resolve(); + + let result = context.delete_uri("file:///foo.rb"); + assert!(result.is_none(), "Deleting a file with a class should change shape"); + } + + #[test] + fn deleting_file_with_only_methods_does_not_change_shape() { + let mut context = GraphTest::new(); + + context.index_uri("file:///methods.rb", "def top_level_method; end"); + context.resolve(); + + let result = context.delete_uri("file:///methods.rb"); + assert!( + result.is_some(), + "Deleting a file with only top-level methods should not change shape" + ); + } + + #[test] + fn deleting_file_that_reopens_class_changes_shape() { + let mut context = GraphTest::new(); + + context.index_uri("file:///foo.rb", "class Foo; end"); + context.index_uri("file:///methods.rb", "class Foo; def bar; end; end"); + context.resolve(); + + // Even though methods.rb "reopens" Foo, it still contains a class definition + // which affects the shape + let result = context.delete_uri("file:///methods.rb"); + assert!( + result.is_none(), + "Deleting a file with a class definition should change shape" + ); + } } diff --git a/rust/rubydex/src/model/name.rs b/rust/rubydex/src/model/name.rs index 8f5d264b..b33f3925 100644 --- a/rust/rubydex/src/model/name.rs +++ b/rust/rubydex/src/model/name.rs @@ -97,6 +97,15 @@ impl Name { } } + fn placeholder() -> Self { + Self { + str: StringId::new(0), + parent_scope: ParentScope::None, + nesting: None, + ref_count: 0, + } + } + #[must_use] pub fn str(&self) -> &StringId { &self.str @@ -140,6 +149,21 @@ impl ResolvedName { &self.name } + /// Consumes self and returns the underlying Name. + #[must_use] + pub fn into_name(self) -> Name { + self.name + } + + /// Creates a placeholder `ResolvedName` for use with `mem::replace`. + /// The placeholder has an empty name and zero declaration ID. + fn placeholder() -> Self { + Self { + name: Name::placeholder(), + declaration_id: DeclarationId::new(0), + } + } + #[must_use] pub fn declaration_id(&self) -> &DeclarationId { &self.declaration_id @@ -181,6 +205,14 @@ impl NameRef { } } + /// Resets a resolved name back to unresolved state. + pub fn reset_to_unresolved(&mut self) { + if let NameRef::Resolved(resolved) = self { + let resolved_name = std::mem::replace(resolved, Box::new(ResolvedName::placeholder())); + *self = NameRef::Unresolved(Box::new(resolved_name.into_name())); + } + } + #[must_use] pub fn nesting(&self) -> &Option { match self { diff --git a/rust/rubydex/src/model/shape.rs b/rust/rubydex/src/model/shape.rs new file mode 100644 index 00000000..54fb4592 --- /dev/null +++ b/rust/rubydex/src/model/shape.rs @@ -0,0 +1,177 @@ +use crate::model::{ + definitions::{Definition, Mixin}, + ids::{DefinitionId, NameId, ReferenceId}, +}; + +#[derive(Debug, Clone, PartialEq, Eq, Hash)] +pub enum MixinKey { + Include(NameId), + Prepend(NameId), + Extend(NameId), +} + +#[derive(Debug, Clone, PartialEq, Eq, Hash)] +pub enum ShapeKey { + Class { + name_id: NameId, + superclass_name_id: Option, + mixins: Vec, + }, + Module { + name_id: NameId, + mixins: Vec, + }, + SingletonClass { + name_id: NameId, + mixins: Vec, + }, + ConstantAlias { + name_id: NameId, + target_name_id: NameId, + }, +} + +impl ShapeKey { + #[must_use] + pub fn name_id(&self) -> NameId { + match self { + ShapeKey::Class { name_id, .. } + | ShapeKey::Module { name_id, .. } + | ShapeKey::SingletonClass { name_id, .. } + | ShapeKey::ConstantAlias { name_id, .. } => *name_id, + } + } +} + +pub fn extract_shape_keys<'a, F>( + definitions: impl IntoIterator, + lookup_reference: F, +) -> Vec +where + F: Fn(&ReferenceId) -> Option, +{ + let mut keys: Vec = definitions + .into_iter() + .filter_map(|(_, def)| shape_key_for_definition(def, &lookup_reference)) + .collect(); + + keys.sort_by_key(|k| *k.name_id()); + keys +} + +fn shape_key_for_definition(definition: &Definition, lookup_reference: &F) -> Option +where + F: Fn(&ReferenceId) -> Option, +{ + match definition { + Definition::Class(class) => { + let superclass_name_id = class.superclass_ref().and_then(lookup_reference); + let mixins = extract_mixin_keys(class.mixins(), lookup_reference); + + Some(ShapeKey::Class { + name_id: *class.name_id(), + superclass_name_id, + mixins, + }) + } + Definition::Module(module) => { + let mixins = extract_mixin_keys(module.mixins(), lookup_reference); + + Some(ShapeKey::Module { + name_id: *module.name_id(), + mixins, + }) + } + Definition::SingletonClass(singleton) => { + let mixins = extract_mixin_keys(singleton.mixins(), lookup_reference); + + Some(ShapeKey::SingletonClass { + name_id: *singleton.name_id(), + mixins, + }) + } + Definition::ConstantAlias(alias) => Some(ShapeKey::ConstantAlias { + name_id: *alias.name_id(), + target_name_id: *alias.target_name_id(), + }), + Definition::Constant(_) + | Definition::Method(_) + | Definition::AttrAccessor(_) + | Definition::AttrReader(_) + | Definition::AttrWriter(_) + | Definition::GlobalVariable(_) + | Definition::InstanceVariable(_) + | Definition::ClassVariable(_) + | Definition::MethodAlias(_) + | Definition::GlobalVariableAlias(_) => None, + } +} + +fn extract_mixin_keys(mixins: &[Mixin], lookup_reference: &F) -> Vec +where + F: Fn(&ReferenceId) -> Option, +{ + mixins + .iter() + .filter_map(|mixin| { + let ref_id = mixin.constant_reference_id(); + lookup_reference(ref_id).map(|name_id| match mixin { + Mixin::Include(_) => MixinKey::Include(name_id), + Mixin::Prepend(_) => MixinKey::Prepend(name_id), + Mixin::Extend(_) => MixinKey::Extend(name_id), + }) + }) + .collect() +} + +#[must_use] +pub fn shapes_match(old_keys: &[ShapeKey], new_keys: &[ShapeKey]) -> bool { + old_keys == new_keys +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn test_shape_key_ordering() { + let key1 = ShapeKey::Class { + name_id: NameId::from("A"), + superclass_name_id: None, + mixins: vec![], + }; + let key2 = ShapeKey::Class { + name_id: NameId::from("B"), + superclass_name_id: None, + mixins: vec![], + }; + + assert_eq!(key1.name_id(), NameId::from("A")); + assert_eq!(key2.name_id(), NameId::from("B")); + } + + #[test] + fn test_mixin_order_matters() { + let mixins1 = vec![ + MixinKey::Include(NameId::from("A")), + MixinKey::Include(NameId::from("B")), + ]; + let mixins2 = vec![ + MixinKey::Include(NameId::from("B")), + MixinKey::Include(NameId::from("A")), + ]; + + assert_ne!(mixins1, mixins2); + } + + #[test] + fn test_mixin_type_matters() { + let include = MixinKey::Include(NameId::from("A")); + let prepend = MixinKey::Prepend(NameId::from("A")); + let extend = MixinKey::Extend(NameId::from("A")); + + assert_ne!(include, prepend); + assert_ne!(include, extend); + assert_ne!(prepend, extend); + } +} diff --git a/rust/rubydex/src/resolution.rs b/rust/rubydex/src/resolution.rs index a0345274..5ed0cc0c 100644 --- a/rust/rubydex/src/resolution.rs +++ b/rust/rubydex/src/resolution.rs @@ -10,7 +10,7 @@ use crate::model::{ Namespace, SingletonClassDeclaration, }, definitions::{Definition, Mixin}, - graph::{CLASS_ID, Graph, MODULE_ID, OBJECT_ID}, + graph::{CLASS_ID, Graph, IndexResult, MODULE_ID, OBJECT_ID}, identity_maps::{IdentityHashMap, IdentityHashSet}, ids::{DeclarationId, DefinitionId, NameId, ReferenceId, StringId}, name::{Name, NameRef, ParentScope}, @@ -121,8 +121,28 @@ impl<'a> Resolver<'a> { ); } - let other_ids = self.prepare_units(); + let definition_ids: Vec<_> = self.graph.definitions().keys().copied().collect(); + let reference_ids: Vec<_> = self.graph.constant_references().keys().copied().collect(); + let other_ids = self.prepare_units(&definition_ids, &reference_ids); + self.resolve_loop(); + self.handle_remaining_definitions(other_ids); + } + + /// Runs incremental resolution for the given definitions and references. + /// + /// This is used when the shape of a document hasn't changed. + /// The declarations already exist, so we just need to link the new definitions and references. + /// + /// # Panics + /// + /// Can panic if there's inconsistent data in the graph + pub fn resolve_incremental(&mut self, result: &IndexResult) { + let other_ids = self.prepare_units(&result.definition_ids, &result.reference_ids); + self.resolve_loop(); + self.handle_remaining_definitions(other_ids); + } + fn resolve_loop(&mut self) { loop { // Flag to ensure the end of the resolution loop. We go through all items in the queue based on its current // length. If we made any progress in this pass of the queue, we can continue because we're unlocking more work @@ -153,8 +173,6 @@ impl<'a> Resolver<'a> { break; } } - - self.handle_remaining_definitions(other_ids); } /// Resolves a single constant against the graph. This method is not meant to be used by the resolution phase, but by @@ -1328,13 +1346,16 @@ impl<'a> Resolver<'a> { parent_depth + nesting_depth + 1 } - fn prepare_units(&mut self) -> Vec { - let estimated_length = self.graph.definitions().len() / 2; + fn prepare_units(&mut self, definition_ids: &[DefinitionId], reference_ids: &[ReferenceId]) -> Vec { + let estimated_length = definition_ids.len() / 2; let mut definitions = Vec::with_capacity(estimated_length); let mut others = Vec::with_capacity(estimated_length); let names = self.graph.names(); - for (id, definition) in self.graph.definitions() { + for id in definition_ids { + let Some(definition) = self.graph.definitions().get(id) else { + continue; + }; let uri = self.graph.documents().get(definition.uri_id()).unwrap().uri(); match definition { @@ -1380,10 +1401,14 @@ impl<'a> Resolver<'a> { (Self::name_depth(name_a, names), uri_a, offset_a).cmp(&(Self::name_depth(name_b, names), uri_b, offset_b)) }); - let mut references = self - .graph - .constant_references() + let mut references: Vec<_> = reference_ids .iter() + .filter_map(|id| { + self.graph + .constant_references() + .get(id) + .map(|constant_ref| (id, constant_ref)) + }) .map(|(id, constant_ref)| { let uri = self.graph.documents().get(&constant_ref.uri_id()).unwrap().uri(); @@ -4201,4 +4226,72 @@ mod tests { assert_ancestors_eq!(context, "C", ["C", "O::A", "B", "Object"]); assert_constant_reference_to!(context, "O::A::X", "file:///1.rb:7:3-7:4"); } + + #[test] + fn resolving_class_defined_through_constant_alias_scope() { + // This is a case that Sorbet doesn't support, but Rubydex should. + // When B = A, defining class B::C should create A::C, not a separate B::C. + let mut context = GraphTest::new(); + context.index_uri("file:///foo.rb", { + r" + module A + end + + B = A + + class B::C + end + + A::C + " + }); + context.resolve(); + + assert_no_diagnostics!(&context, &[Rule::ParseWarning]); + + // B should be an alias to A + assert_constant_alias_target_eq!(context, "B", "A"); + + // C should be a member of A, not B (because B is an alias to A) + assert_members_eq!(context, "A", ["C"]); + + // The reference to C in A::C should resolve to A::C + // (A is at 9:1-9:2, C is at 9:4-9:5) + assert_constant_reference_to!(context, "A::C", "file:///foo.rb:9:4-9:5"); + } + + #[test] + fn resolving_class_defined_through_ancestor_scope() { + // This is a case that Sorbet doesn't support, but Rubydex should. + // When Foo includes Bar and Bar contains module A, defining class A::B + // inside Foo should create Bar::A::B. + let mut context = GraphTest::new(); + context.index_uri("file:///foo.rb", { + r" + module Bar + module A + end + end + + module Foo + include Bar + + class A::B + end + end + + Bar::A::B + " + }); + context.resolve(); + + assert_no_diagnostics!(&context, &[Rule::ParseWarning]); + + // B should be a member of Bar::A + assert_members_eq!(context, "Bar::A", ["B"]); + + // The reference to B in Bar::A::B should resolve to Bar::A::B + // (Bar is at 13:1-13:4, A is at 13:6-13:7, B is at 13:9-13:10) + assert_constant_reference_to!(context, "Bar::A::B", "file:///foo.rb:13:9-13:10"); + } } diff --git a/rust/rubydex/src/test_utils/graph_test.rs b/rust/rubydex/src/test_utils/graph_test.rs index 8a8fca99..d795294e 100644 --- a/rust/rubydex/src/test_utils/graph_test.rs +++ b/rust/rubydex/src/test_utils/graph_test.rs @@ -3,7 +3,7 @@ use super::normalize_indentation; use crate::diagnostic::Rule; use crate::indexing::local_graph::LocalGraph; use crate::indexing::ruby_indexer::RubyIndexer; -use crate::model::graph::Graph; +use crate::model::graph::{Graph, IndexResult}; use crate::resolution::Resolver; #[derive(Default)] @@ -29,14 +29,14 @@ impl GraphTest { indexer.local_graph() } - pub fn index_uri(&mut self, uri: &str, source: &str) { + pub fn index_uri(&mut self, uri: &str, source: &str) -> Option { let source = normalize_indentation(source); let local_index = Self::index_source(uri, &source); - self.graph.update(local_index); + self.graph.update(local_index) } - pub fn delete_uri(&mut self, uri: &str) { - self.graph.delete_uri(uri); + pub fn delete_uri(&mut self, uri: &str) -> Option { + self.graph.delete_uri(uri) } pub fn resolve(&mut self) { @@ -44,6 +44,11 @@ impl GraphTest { resolver.resolve_all(); } + pub fn resolve_incremental(&mut self, result: &IndexResult) { + let mut resolver = Resolver::new(&mut self.graph); + resolver.resolve_incremental(result); + } + /// # Panics /// /// Panics if a diagnostic points to an invalid document