-
Notifications
You must be signed in to change notification settings - Fork 0
Add incremental invalidation/resolution #589
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -60,7 +60,7 @@ impl RubydexServer { | |
| } | ||
|
|
||
| let mut resolver = rubydex::resolution::Resolver::new(&mut graph); | ||
| resolver.resolve_all(); | ||
| resolver.resolve(); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we want to rename this, can we move it to a separate PR? This is a complex change, so I'm trying to make sure we're focused on the core aspects of the implementation and that we lower the barrier for better reviews. |
||
|
|
||
| eprintln!( | ||
| "Rubydex indexed {} files, {} declarations", | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,6 +3,7 @@ use std::collections::hash_map::Entry; | |
| use crate::diagnostic::{Diagnostic, Rule}; | ||
| 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::{DefinitionId, NameId, ReferenceId, StringId, UriId}; | ||
| use crate::model::name::{Name, NameRef}; | ||
|
|
@@ -18,6 +19,7 @@ type LocalGraphParts = ( | |
| IdentityHashMap<NameId, NameRef>, | ||
| IdentityHashMap<ReferenceId, ConstantReference>, | ||
| IdentityHashMap<ReferenceId, MethodRef>, | ||
| IdentityHashMap<NameId, Vec<NameDependent>>, | ||
| ); | ||
|
|
||
| #[derive(Debug)] | ||
|
|
@@ -29,6 +31,7 @@ pub struct LocalGraph { | |
| names: IdentityHashMap<NameId, NameRef>, | ||
| constant_references: IdentityHashMap<ReferenceId, ConstantReference>, | ||
| method_references: IdentityHashMap<ReferenceId, MethodRef>, | ||
| name_dependents: IdentityHashMap<NameId, Vec<NameDependent>>, | ||
| } | ||
|
|
||
| impl LocalGraph { | ||
|
|
@@ -42,6 +45,7 @@ impl LocalGraph { | |
| names: IdentityHashMap::default(), | ||
| constant_references: IdentityHashMap::default(), | ||
| method_references: IdentityHashMap::default(), | ||
| name_dependents: IdentityHashMap::default(), | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Populating and managing the |
||
| } | ||
| } | ||
|
|
||
|
|
@@ -70,6 +74,10 @@ impl LocalGraph { | |
| pub fn add_definition(&mut self, definition: Definition) -> DefinitionId { | ||
| let definition_id = definition.id(); | ||
|
|
||
| if let Some(name_id) = definition.name_id() { | ||
| self.insert_name_dependent(*name_id, NameDependent::Definition(definition_id)); | ||
| } | ||
|
|
||
| if self.definitions.insert(definition_id, definition).is_some() { | ||
| debug_assert!(false, "DefinitionId collision in local graph"); | ||
| } | ||
|
|
@@ -133,6 +141,7 @@ impl LocalGraph { | |
|
|
||
| pub fn add_constant_reference(&mut self, reference: ConstantReference) -> ReferenceId { | ||
| let reference_id = reference.id(); | ||
| self.insert_name_dependent(*reference.name_id(), NameDependent::Reference(reference_id)); | ||
|
|
||
| if self.constant_references.insert(reference_id, reference).is_some() { | ||
| debug_assert!(false, "ReferenceId collision in local graph"); | ||
|
|
@@ -172,6 +181,26 @@ impl LocalGraph { | |
| self.document.add_diagnostic(diagnostic); | ||
| } | ||
|
|
||
| // Name dependents | ||
|
|
||
| /// Registers a dependent under its own `name_id` and under the `nesting`/`parent_scope` | ||
| /// of that name, so invalidation can trace from any of those scopes. | ||
| fn insert_name_dependent(&mut self, name_id: NameId, dep: NameDependent) { | ||
| self.name_dependents.entry(name_id).or_default().push(dep); | ||
|
|
||
| if let Some(name_ref) = self.names.get(&name_id) { | ||
| for owner_id in [name_ref.nesting().as_ref().copied(), name_ref.parent_scope().as_ref().copied()] | ||
| .into_iter() | ||
| .flatten() | ||
| { | ||
| let deps = self.name_dependents.entry(owner_id).or_default(); | ||
| if !deps.contains(&dep) { | ||
| deps.push(dep); | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Into parts | ||
|
|
||
| #[must_use] | ||
|
|
@@ -184,6 +213,7 @@ impl LocalGraph { | |
| self.names, | ||
| self.constant_references, | ||
| self.method_references, | ||
| self.name_dependents, | ||
| ) | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO, this makes for a weirder API than returning the units that API consumers can just ignore.
Basically this:
vs
@Morriar what do you think?
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No the idea is to pass it as constructor arg:
Rubydex::Graph.new(without_resolution: true). I just haven't found a good way to set the state and past it to the Rust side.