From 268cdea2b9f2b4f6df2fd09b825ad151d1e86e66 Mon Sep 17 00:00:00 2001 From: Thomas Marshall Date: Thu, 5 Feb 2026 14:47:35 +0000 Subject: [PATCH] Implement basic shape-based incremental updates This commit adds a very basic incremental update mechanism, similar to what Sorbet does when it chooses between a fast and slow path. If the semantic shape of the document hasn't changed (i.e. no namespaces added or removed) then we don't need to create any new namespace declarations or perform any resolution. Otherwise, we'll do a full re-resolve. This doesn't do any form of invalidation, so there is no need to do any form of dependency tracking. It's beneficial primarily for simple LSP use-cases, where we want to avoid running expensive resolution when not necessary. For example, if the user is just typing code in the body of a method or adding a new method, there isn't any reason to re-run resolution. We are very likely missing some cases, but this is a prototype to demonstrate how the fast/slow path approach could work quite straightforwardly in Rubydex. It doesn't achieve what we want to eventually achieve (more granular invalidation and re-resolution) but it makes common case editing more bearable. --- rust/rubydex/src/indexing/local_graph.rs | 5 + rust/rubydex/src/model.rs | 1 + rust/rubydex/src/model/graph.rs | 385 ++++++++++++++-------- rust/rubydex/src/model/name.rs | 32 ++ rust/rubydex/src/model/shape.rs | 177 ++++++++++ rust/rubydex/src/resolution.rs | 113 ++++++- rust/rubydex/src/test_utils/graph_test.rs | 15 +- 7 files changed, 583 insertions(+), 145 deletions(-) create mode 100644 rust/rubydex/src/model/shape.rs diff --git a/rust/rubydex/src/indexing/local_graph.rs b/rust/rubydex/src/indexing/local_graph.rs index dfa62eb84..df711b07f 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 35090a354..700a44dab 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 d771129f4..b62f23720 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 8f5d264bf..b33f39251 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 000000000..54fb45923 --- /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 a03452745..5ed0cc0c0 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 8a8fca99d..d795294e5 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