Skip to content

Propagate singleton class creation to descendants #937

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
base: main
Choose a base branch
from
Open

Propagate singleton class creation to descendants #937

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
12 changes: 12 additions & 0 deletions rust/rubydex/src/model/declaration.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,18 @@ pub enum Ancestors {
assert_mem_size!(Ancestors, 32);

impl Ancestors {
#[must_use]
pub fn contains(&self, declaration_id: DeclarationId) -> bool {
match self {
Ancestors::Complete(ancestors) | Ancestors::Partial(ancestors) | Ancestors::Cyclic(ancestors) => {
ancestors.iter().any(|ancestor| match ancestor {
Ancestor::Complete(id) => *id == declaration_id,
Ancestor::Partial(_) => false,
})
}
}
}

pub fn iter(&self) -> std::slice::Iter<'_, Ancestor> {
match self {
Ancestors::Complete(ancestors) | Ancestors::Partial(ancestors) | Ancestors::Cyclic(ancestors) => {
Expand Down
109 changes: 95 additions & 14 deletions rust/rubydex/src/resolution.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1077,22 +1077,24 @@ impl<'a> Resolver<'a> {
Declaration::Namespace(Namespace::Class(_)) => {
let definition_ids = declaration.definitions().to_vec();

Some(match self.linearize_parent_class(&definition_ids, context) {
Ancestors::Complete(ids) => ids,
Ancestors::Cyclic(ids) => {
context.cyclic = true;
ids
}
Ancestors::Partial(ids) => {
context.partial = true;
ids
}
})
Some(
match self.linearize_parent_class(declaration_id, &definition_ids, context) {
Ancestors::Complete(ids) => ids,
Ancestors::Cyclic(ids) => {
context.cyclic = true;
ids
}
Ancestors::Partial(ids) => {
context.partial = true;
ids
}
},
)
}
Declaration::Namespace(Namespace::SingletonClass(_)) => {
let owner_id = *declaration.owner_id();

let (singleton_parent_id, partial_singleton) = self.singleton_parent_id(owner_id);
let (singleton_parent_id, partial_singleton) = self.singleton_parent_id(owner_id, 1);
if partial_singleton {
context.partial = true;
}
Expand Down Expand Up @@ -2019,7 +2021,7 @@ impl<'a> Resolver<'a> {
/// - Module: parent is the `Module` class
/// - Class: parent is the singleton class of the original parent class
/// - Singleton class: recurse as many times as necessary to wrap the original attached object's parent class
fn singleton_parent_id(&mut self, attached_id: DeclarationId) -> (DeclarationId, bool) {
fn singleton_parent_id(&mut self, attached_id: DeclarationId, depth: u16) -> (DeclarationId, bool) {
// Base case: if we reached `BasicObject`, then the parent is `Class`
if attached_id == *BASIC_OBJECT_ID {
return (*CLASS_ID, false);
Expand All @@ -2034,7 +2036,7 @@ impl<'a> Resolver<'a> {
// object
let owner_id = *decl.owner_id();

let (inner_parent, partial) = self.singleton_parent_id(owner_id);
let (inner_parent, partial) = self.singleton_parent_id(owner_id, depth + 1);
(
self.get_or_create_singleton_class(inner_parent, SingletonAncestors::Deferred)
.expect("singleton parent should always be a namespace"),
Expand All @@ -2045,6 +2047,51 @@ impl<'a> Resolver<'a> {
// For classes (the regular case), we need to return the singleton class of its parent
let definition_ids = decl.definitions().to_vec();

let class_ancestors = self
.graph
.declarations()
.get(&*CLASS_ID)
.unwrap()
.as_namespace()
.unwrap()
.ancestors();

// When creating a new singleton class, we need to ensure that all descendants also get a singleton
// class, otherwise we end up with broken chains. Because we also enqueue ancestor linearization for
// them, we land here again (making this process recursive). The stop condition is finding any ancestors
// of `Class`, which is the default parent for singletons.
//
// All singleton classes inherit from `Class` and its ancestors, so without this, the algorithm goes
// into infinite recursion:
//
// 1. Create a singleton class for a descendant
// 2. Enqueue ancestor linearization for the descendant's singleton class
// 3. Eventually, we reach this exact spot with `Class`. Since the singleton we just created is a
// descendant of `Class`, we create a new singleton one level deeper, taking us back to 1
if !class_ancestors.contains(attached_id) {
let descendants = decl
.as_namespace()
.unwrap()
.descendants()
.iter()
.copied()
.collect::<Vec<_>>();

for descendant in descendants {

@Morriar Morriar Jul 30, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since descendants is already transitive, aren't we repeating a lot of work here? Creating the singleton for A visits all of its descendants, and then each singleton we enqueue does another overlapping scan when it is linearized. For a deep hierarchy, this looks quadratic.

Could we track the singleton depth already propagated for each declaration during this resolution and skip these repeated scans?

if descendant == attached_id {
Comment thread
vinistock marked this conversation as resolved.
continue;

@Morriar Morriar Jul 30, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add a test like this:

# First resolution
class Child; end

# Later resolution
def Object.foo; end

I'm not sure we're properly updating the Child::<Child> parent.

}

let mut needs_singleton_id = descendant;

for _ in 0..depth {
Comment thread
vinistock marked this conversation as resolved.
needs_singleton_id = self
.get_or_create_singleton_class(needs_singleton_id, SingletonAncestors::Enqueue)
.expect("descendants are always namespaces");
}
}
}

let (picked_parent, unresolved_parent) = self.get_parent_class(&definition_ids);
(
self.get_or_create_singleton_class(picked_parent, SingletonAncestors::Deferred)
Expand Down Expand Up @@ -2096,12 +2143,15 @@ impl<'a> Resolver<'a> {

fn linearize_parent_class(
&mut self,
declaration_id: DeclarationId,
definition_ids: &[DefinitionId],
context: &mut LinearizationContext,
) -> Ancestors {
let (picked_parent, unresolved_parent) = self.get_parent_class(definition_ids);
let mut result = self.linearize_ancestors(picked_parent, context);

self.ensure_matching_singleton_class_depth(declaration_id, picked_parent);

@Morriar Morriar Jul 30, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you try this:

# resolution 1

# parents.rb
class OldParent
  def self.old_method; end
end

class NewParent
  def self.new_method; end
end

# child_a.rb
class Child < OldParent; end

# child_b.rb
class Child < OldParent; end

then

# resolution 2

# child_a.rb
class Child < NewParent; end

# child_b.rb
class Child < NewParent; end

Is the parent of Child::<Child> updated?


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

Expand All @@ -2118,6 +2168,37 @@ impl<'a> Resolver<'a> {
}
}

fn ensure_matching_singleton_class_depth(&mut self, declaration_id: DeclarationId, parent_id: DeclarationId) {
// Incremental resolution scenario: if a new class is created inheriting from a parent that already has a
// singleton class, we need to create its own singleton to avoid broken descendants
let mut parent_singleton = self
.graph
.declarations()
.get(&parent_id)
.unwrap()
.as_namespace()
.unwrap()
.singleton_class()
.copied();
let mut attached = declaration_id;

while let Some(parent_singleton_id) = parent_singleton {
attached = self
.get_or_create_singleton_class(attached, SingletonAncestors::Enqueue)
.expect("the declaration being linearized is always a namespace");

parent_singleton = self
.graph
.declarations()
.get(&parent_singleton_id)
.unwrap()
.as_namespace()
.unwrap()
.singleton_class()
.copied();
}
}

fn mixins_of(&self, definition_id: DefinitionId) -> Option<Vec<Mixin>> {
let definition = self.graph.definitions().get(&definition_id).unwrap();

Expand Down
Loading
Loading