Skip to content

Remove unnecessary context allocation #952

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.

Sign up for GitHub

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

Open
vinistock wants to merge 1 commit into from
+100 −96
Open

Remove unnecessary context allocation #952

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
159 changes: 63 additions & 96 deletions rust/rubydex/src/resolution.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,4 @@
use std::{
collections::{HashSet, VecDeque, hash_map::Entry},
hash::BuildHasher,
};
use std::collections::{HashSet, VecDeque, hash_map::Entry};

use crate::diagnostic::{Diagnostic, Rule};
use crate::model::{
Expand Down Expand Up @@ -51,20 +48,39 @@ enum SingletonAncestors {
Deferred,
}

#[derive(Default)]
struct ChainState {
cyclic: bool,
partial: bool,
}

impl ChainState {
/// Fold a nested linearization's result into this frame's state, returning its ancestor list
fn fold(&mut self, ancestors: Ancestors) -> Vec<Ancestor> {
match ancestors {
Ancestors::Complete(ids) => ids,
Ancestors::Cyclic(ids) => {
self.cyclic = true;
ids
}
Ancestors::Partial(ids) => {
self.partial = true;
ids
}
}
}
}

struct LinearizationContext {
descendants: IdentityHashSet<DeclarationId>,
seen_ids: IdentityHashSet<DeclarationId>,
cyclic: bool,
partial: bool,
}

impl LinearizationContext {
fn new() -> Self {
Self {
descendants: IdentityHashSet::default(),
seen_ids: IdentityHashSet::default(),
cyclic: false,
partial: false,
}
}

Expand All @@ -83,6 +99,8 @@ pub struct Resolver<'a> {
unit_queue: VecDeque<Unit>,
/// Whether we made any progress in the last pass of the resolution loop
made_progress: bool,
/// The linearization context carried across recursion
context: LinearizationContext,
}

impl<'a> Resolver<'a> {
Expand All @@ -91,6 +109,7 @@ impl<'a> Resolver<'a> {
graph,
unit_queue: VecDeque::new(),
made_progress: false,
context: LinearizationContext::new(),
}
}

Expand Down Expand Up @@ -257,7 +276,7 @@ impl<'a> Resolver<'a> {

/// Handles a unit of work for linearizing ancestors of a declaration
fn handle_ancestor_unit(&mut self, id: DeclarationId) {
match self.ancestors_of(id) {
match self.linearize_ancestors(id) {
Ancestors::Complete(_) | Ancestors::Cyclic(_) => {
// We succeeded in some capacity this time
self.made_progress = true;
Expand Down Expand Up @@ -928,7 +947,7 @@ impl<'a> Resolver<'a> {
fn schedule_singleton_ancestors(&mut self, id: DeclarationId, mode: SingletonAncestors) {
match mode {
SingletonAncestors::Eager => {
let _ = self.ancestors_of(id);
let _ = self.linearize_ancestors(id);
}
SingletonAncestors::Enqueue => {
self.unit_queue.push_back(Unit::Ancestors(id));
Expand All @@ -945,35 +964,24 @@ impl<'a> Resolver<'a> {
///
/// Can panic if there's inconsistent data in the graph
#[must_use]
fn ancestors_of(&mut self, declaration_id: DeclarationId) -> Ancestors {
let mut context = LinearizationContext::new();
self.linearize_ancestors(declaration_id, &mut context)
}

/// Linearizes the ancestors of a declaration, returning the list of ancestor declaration IDs
///
/// # Panics
///
/// Can panic if there's inconsistent data in the graph
#[must_use]
fn linearize_ancestors(&mut self, declaration_id: DeclarationId, context: &mut LinearizationContext) -> Ancestors {
fn linearize_ancestors(&mut self, declaration_id: DeclarationId) -> Ancestors {
{
let declaration = self.graph.declarations_mut().get_mut(&declaration_id).unwrap();

// Add this declaration to the descendants so that we capture transitive descendant relationships
context.descendants.insert(declaration_id);
self.context.descendants.insert(declaration_id);

// Return the cached ancestors if we already computed them. If they are partial ancestors, ignore the cache to try
// again
if declaration.as_namespace().unwrap().has_complete_ancestors() {
let cached = declaration.as_namespace().unwrap().clone_ancestors();
self.propagate_descendants(&mut context.descendants, &cached);
self.propagate_descendants(&cached);

context.finalize(declaration_id);
self.context.finalize(declaration_id);
return cached;
}

if !context.seen_ids.insert(declaration_id) {
if !self.context.seen_ids.insert(declaration_id) {
// If we find a cycle when linearizing ancestors, it's an error that the programmer must fix. However, we try to
// still approximate features by assuming that it must inherit from `Object` at some point (which is what most
// classes/modules inherit from). This is not 100% correct, but it allows us to provide a bit better IDE support
Expand All @@ -988,19 +996,20 @@ impl<'a> Resolver<'a> {
.unwrap()
.set_ancestors(estimated_ancestors.clone());

context.finalize(declaration_id);
self.context.finalize(declaration_id);
return estimated_ancestors;
}

// Automatically track descendants as we recurse. This has to happen before checking the cache since we may have
// already linearized the parent's ancestors, but it's the first time we're discovering the descendant
let namespace = declaration.as_namespace_mut().unwrap();
for descendant in &context.descendants {
for descendant in &self.context.descendants {
namespace.add_descendant(*descendant);
}
}

let parent_ancestors = self.linearize_parent_ancestors(declaration_id, context);
let mut state = ChainState::default();
let parent_ancestors = self.linearize_parent_ancestors(declaration_id, &mut state);
let declaration = self.graph.declarations().get(&declaration_id).unwrap();
let mut mixins = Vec::new();

Expand Down Expand Up @@ -1038,7 +1047,7 @@ impl<'a> Resolver<'a> {
}

let (linearized_prepends, linearized_includes) =
self.linearize_mixins(context, mixins, parent_ancestors.as_ref());
self.linearize_mixins(mixins, parent_ancestors.as_ref(), &mut state);

// Build the final list
let mut ancestors = Vec::new();
Expand All @@ -1049,9 +1058,9 @@ impl<'a> Resolver<'a> {
ancestors.extend(parents);
}

let result = if context.cyclic {
let result = if state.cyclic {
Ancestors::Cyclic(ancestors)
} else if context.partial {
} else if state.partial {
Ancestors::Partial(ancestors)
} else {
Ancestors::Complete(ancestors)
Expand All @@ -1065,50 +1074,32 @@ impl<'a> Resolver<'a> {
.unwrap()
.set_ancestors(result.clone());

context.finalize(declaration_id);
self.context.finalize(declaration_id);
result
}

fn linearize_parent_ancestors(
&mut self,
declaration_id: DeclarationId,
context: &mut LinearizationContext,
state: &mut ChainState,
) -> Option<Vec<Ancestor>> {
let declaration = self.graph.declarations().get(&declaration_id).unwrap();

match declaration {
Declaration::Namespace(Namespace::Class(_)) => {
Some(match self.linearize_superclass(declaration_id, context)? {
Ancestors::Complete(ids) => ids,
Ancestors::Cyclic(ids) => {
context.cyclic = true;
ids
}
Ancestors::Partial(ids) => {
context.partial = true;
ids
}
})
let superclass = self.linearize_superclass(declaration_id, state)?;
Some(state.fold(superclass))
}
Declaration::Namespace(Namespace::SingletonClass(_)) => {
let owner_id = *declaration.owner_id();

let (singleton_parent_id, partial_singleton) = self.singleton_parent_id(owner_id);
if partial_singleton {
context.partial = true;
state.partial = true;
}

Some(match self.linearize_ancestors(singleton_parent_id, context) {
Ancestors::Complete(ids) => ids,
Ancestors::Cyclic(ids) => {
context.cyclic = true;
ids
}
Ancestors::Partial(ids) => {
context.partial = true;
ids
}
})
let parent = self.linearize_ancestors(singleton_parent_id);
Some(state.fold(parent))
}
_ => None,
}
Expand All @@ -1118,9 +1109,9 @@ impl<'a> Resolver<'a> {
/// modules are deduplicated against them
fn linearize_mixins(
&mut self,
context: &mut LinearizationContext,
mixins: Vec<Mixin>,
parent_ancestors: Option<&Vec<Ancestor>>,
state: &mut ChainState,
) -> (VecDeque<Ancestor>, VecDeque<Ancestor>) {
let mut linearized_prepends = VecDeque::new();
let mut linearized_includes = VecDeque::new();
Expand All @@ -1143,17 +1134,8 @@ impl<'a> Resolver<'a> {
continue;
};

let ids = match self.linearize_ancestors(module_id, context) {
Ancestors::Complete(ids) => ids,
Ancestors::Cyclic(ids) => {
context.cyclic = true;
ids
}
Ancestors::Partial(ids) => {
context.partial = true;
ids
}
};
let module_ancestors = self.linearize_ancestors(module_id);
let ids = state.fold(module_ancestors);

// Only reorder if there are new modules to add. If all modules being
// prepended are already in the chain (e.g., `prepend A` when A is already
Expand All @@ -1170,7 +1152,7 @@ impl<'a> Resolver<'a> {
NameRef::Unresolved(_) => {
// We haven't been able to resolve this name yet, so we push it as a partial linearization to finish
// later
context.partial = true;
state.partial = true;
linearized_prepends.push_front(Ancestor::Partial(*constant_reference.name_id()));
}
}
Expand All @@ -1182,17 +1164,8 @@ impl<'a> Resolver<'a> {
continue;
};

let mut ids = match self.linearize_ancestors(module_id, context) {
Ancestors::Complete(ids) => ids,
Ancestors::Cyclic(ids) => {
context.cyclic = true;
ids
}
Ancestors::Partial(ids) => {
context.partial = true;
ids
}
};
let module_ancestors = self.linearize_ancestors(module_id);
let mut ids = state.fold(module_ancestors);

// Prepended module are deduped based only on other prepended modules
ids.retain(|id| {
Expand All @@ -1210,7 +1183,7 @@ impl<'a> Resolver<'a> {
NameRef::Unresolved(_) => {
// We haven't been able to resolve this name yet, so we push it as a partial linearization to finish
// later
context.partial = true;
state.partial = true;
linearized_includes.push_front(Ancestor::Partial(*constant_reference.name_id()));
}
}
Expand All @@ -1222,11 +1195,9 @@ impl<'a> Resolver<'a> {
}

/// Propagate descendants to all cached ancestors
fn propagate_descendants<S: BuildHasher>(
&mut self,
descendants: &mut HashSet<DeclarationId, S>,
cached: &Ancestors,
) {
fn propagate_descendants(&mut self, cached: &Ancestors) {
let descendants = &self.context.descendants;

if !descendants.is_empty() {
for ancestor in cached {
if let Ancestor::Complete(ancestor_id) = ancestor {
Expand All @@ -1238,7 +1209,7 @@ impl<'a> Resolver<'a> {
.as_namespace_mut()
.unwrap();

for descendant in descendants.iter() {
for descendant in descendants {
namespace.add_descendant(*descendant);
}
}
Expand Down Expand Up @@ -1749,7 +1720,7 @@ impl<'a> Resolver<'a> {

/// Search for a member in a declaration's ancestor chain.
fn search_ancestors(&mut self, declaration_id: DeclarationId, str_id: StringId) -> Outcome {
match self.ancestors_of(declaration_id) {
match self.linearize_ancestors(declaration_id) {
Ancestors::Complete(ids) | Ancestors::Cyclic(ids) => ids
.iter()
.find_map(|ancestor_id| {
Expand Down Expand Up @@ -2064,16 +2035,12 @@ impl<'a> Resolver<'a> {
))
}

fn linearize_superclass(
&mut self,
declaration_id: DeclarationId,
context: &mut LinearizationContext,
) -> Option<Ancestors> {
fn linearize_superclass(&mut self, declaration_id: DeclarationId, state: &mut ChainState) -> Option<Ancestors> {
let (superclass_id, unresolved_superclass) = self.get_superclass(declaration_id)?;
let mut result = self.linearize_ancestors(superclass_id, context);
let mut result = self.linearize_ancestors(superclass_id);

if let Some(name_id) = unresolved_superclass {
context.partial = true;
state.partial = true;

// Insert the unresolved superclass as a Partial ancestor at the front of the chain, so it
// appears before the default Object ancestors
Expand Down
37 changes: 37 additions & 0 deletions rust/rubydex/src/resolution_tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1494,6 +1494,43 @@ mod include_tests {
assert_descendants!(context, "Bar", ["Baz"]);
assert_descendants!(context, "Foo", ["Bar", "Baz"]);
}

#[test]
fn unresolved_include_followed_by_resolved_include_stays_partial() {
let mut context = graph_test();
context.index_uri("file:///foo.rb", {
r"
module M
end

class C
include Missing
include M
end
"
});
context.resolve();

assert_ancestors_eq!(
context,
"C",
["C", "M", Partial("Missing"), "Object", "Kernel", "BasicObject"]
);
assert!(
matches!(
context
.graph()
.declarations()
.get(&DeclarationId::from("C"))
.unwrap()
.as_namespace()
.unwrap()
.ancestors(),
Ancestors::Partial(_)
),
"C should have a partial chain: the `Missing` include never resolves"
);
}
}

mod prepend_tests {
Expand Down
Loading