Skip to content

Fix corner cases of re-opening aliases #621

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

Merged
vinistock merged 1 commit into from
Mar 4, 2026
Merged

Fix corner cases of re-opening aliases #621

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
186 changes: 170 additions & 16 deletions rust/rubydex/src/resolution.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -308,29 +308,41 @@ impl<'a> Resolver<'a> {

singleton_id
}
None => self.resolve_lexical_owner(*method_definition.lexical_nesting_id()),
None => {
let Some(resolved) = self.resolve_lexical_owner(*method_definition.lexical_nesting_id())
else {
continue;
};
resolved
}
};

self.create_declaration(str_id, id, owner_id, |name| {
Declaration::Method(Box::new(MethodDeclaration::new(name, owner_id)))
});
}
Definition::AttrAccessor(attr) => {
let owner_id = self.resolve_lexical_owner(*attr.lexical_nesting_id());
let Some(owner_id) = self.resolve_lexical_owner(*attr.lexical_nesting_id()) else {
continue;
};

self.create_declaration(*attr.str_id(), id, owner_id, |name| {
Declaration::Method(Box::new(MethodDeclaration::new(name, owner_id)))
});
}
Definition::AttrReader(attr) => {
let owner_id = self.resolve_lexical_owner(*attr.lexical_nesting_id());
let Some(owner_id) = self.resolve_lexical_owner(*attr.lexical_nesting_id()) else {
continue;
};

self.create_declaration(*attr.str_id(), id, owner_id, |name| {
Declaration::Method(Box::new(MethodDeclaration::new(name, owner_id)))
});
}
Definition::AttrWriter(attr) => {
let owner_id = self.resolve_lexical_owner(*attr.lexical_nesting_id());
let Some(owner_id) = self.resolve_lexical_owner(*attr.lexical_nesting_id()) else {
continue;
};

self.create_declaration(*attr.str_id(), id, owner_id, |name| {
Declaration::Method(Box::new(MethodDeclaration::new(name, owner_id)))
Expand Down Expand Up @@ -403,7 +415,9 @@ impl<'a> Resolver<'a> {
}

// If the method has no explicit receiver, we resolve the owner based on the lexical nesting
let method_owner_id = self.resolve_lexical_owner(*method.lexical_nesting_id());
let Some(method_owner_id) = self.resolve_lexical_owner(*method.lexical_nesting_id()) else {
continue;
};

// If the method is in a singleton class, the instance variable belongs to the class object
// Like `class << Foo; def bar; @bar = 1; end; end`, where `@bar` is owned by `Foo::<Foo>`
Expand Down Expand Up @@ -436,9 +450,10 @@ impl<'a> Resolver<'a> {
.definition_id_to_declaration_id(nesting_id)
.copied()
.unwrap_or(*OBJECT_ID);
let owner_id = self
.get_or_create_singleton_class(nesting_decl_id)
.expect("class/module nesting should always be a namespace");

let Some(owner_id) = self.get_or_create_singleton_class(nesting_decl_id) else {
continue;
};
{
debug_assert!(
matches!(
Expand Down Expand Up @@ -494,7 +509,9 @@ impl<'a> Resolver<'a> {
}
}
Definition::MethodAlias(alias) => {
let owner_id = self.resolve_lexical_owner(*alias.lexical_nesting_id());
let Some(owner_id) = self.resolve_lexical_owner(*alias.lexical_nesting_id()) else {
continue;
};

self.create_declaration(*alias.new_name_str_id(), id, owner_id, |name| {
Declaration::Method(Box::new(MethodDeclaration::new(name, owner_id)))
Expand Down Expand Up @@ -537,7 +554,8 @@ impl<'a> Resolver<'a> {
self.graph.add_member(&owner_id, declaration_id, str_id);
}

/// Resolves owner for class variables, bypassing singleton classes.
/// Resolves owner for class variables, bypassing singleton classes. Returns `None` if the owner can't be
/// determined (e.g., unresolved constant alias).
fn resolve_class_variable_owner(&self, lexical_nesting_id: Option<DefinitionId>) -> Option<DeclarationId> {
let mut current_nesting = lexical_nesting_id;
while let Some(nesting_id) = current_nesting {
Expand All @@ -549,13 +567,24 @@ impl<'a> Resolver<'a> {
break;
}
}
current_nesting.and_then(|id| self.graph.definition_id_to_declaration_id(id).copied())
let declaration_id = current_nesting.and_then(|id| self.graph.definition_id_to_declaration_id(id).copied())?;

// If the declaration is a constant alias, follow the alias chain to find the
// target namespace. Returns None if the alias target is unresolved.
if matches!(
self.graph.declarations().get(&declaration_id),
Some(Declaration::ConstantAlias(_))
) {
self.resolve_to_namespace(declaration_id)
} else {
Some(declaration_id)
}
}

/// Resolves owner from lexical nesting.
fn resolve_lexical_owner(&self, lexical_nesting_id: Option<DefinitionId>) -> DeclarationId {
fn resolve_lexical_owner(&self, lexical_nesting_id: Option<DefinitionId>) -> Option<DeclarationId> {
let Some(id) = lexical_nesting_id else {
return *OBJECT_ID;
return Some(*OBJECT_ID);
};

// If no declaration exists yet for this definition, walk up the lexical chain.
Expand All @@ -566,16 +595,20 @@ impl<'a> Resolver<'a> {
return self.resolve_lexical_owner(*definition.lexical_nesting_id());
};

let declarations = self.graph.declarations();
let decl = self.graph.declarations().get(declaration_id).unwrap();

// If the associated declaration is a namespace that can own things, we found the right owner. Otherwise, we might
// have found something nested inside something else (like a method), in which case we have to recurse until we find
// the appropriate owner
if matches!(
declarations.get(declaration_id).unwrap(),
decl,
Declaration::Namespace(Namespace::Class(_) | Namespace::Module(_) | Namespace::SingletonClass(_))
) {
*declaration_id
Some(*declaration_id)
} else if matches!(decl, Declaration::ConstantAlias(_)) {
// Follow the alias chain to find the target namespace. If the alias is unresolved,
// the definition cannot be properly owned and should be skipped by the caller.
self.resolve_to_namespace(*declaration_id)
} else {
let definition = self.graph.definitions().get(&id).unwrap();
self.resolve_lexical_owner(*definition.lexical_nesting_id())
Expand Down Expand Up @@ -5031,4 +5064,125 @@ mod tests {
context.resolve();
assert_declaration_exists!(context, "CONST");
}

#[test]
fn including_unresolved_alias() {
let mut context = GraphTest::new();
context.index_uri("file:///a.rb", {
r"
module Foo; end
Foo::Bar = Bar

module Baz
include Foo::Bar
end
"
});

context.resolve();
assert_ancestors_eq!(context, "Baz", ["Baz"]);
}

#[test]
fn prepending_unresolved_alias() {
let mut context = GraphTest::new();
context.index_uri("file:///a.rb", {
r"
module Foo; end
Foo::Bar = Bar

module Baz
prepend Foo::Bar
end
"
});

context.resolve();
assert_ancestors_eq!(context, "Baz", ["Baz"]);
}

#[test]
fn inheriting_unresolved_alias() {
let mut context = GraphTest::new();
context.index_uri("file:///a.rb", {
r"
module Foo; end
Foo::Bar = Bar

class Baz < Foo::Bar
end
"
});

context.resolve();
assert_ancestors_eq!(context, "Baz", ["Baz", "Object"]);
}

#[test]
fn re_opening_unresolved_alias() {
let mut context = GraphTest::new();
context.index_uri("file:///a.rb", {
r"
module Foo; end
Foo::Bar = Bar

module Foo::Bar
CONST = 123
@class_ivar = 123
@@class_var = 789

attr_reader :some_attr

def self.class_method; end

def initialize
@instance_ivar = 456
end
end
"
});

context.resolve();
assert_declaration_does_not_exist!(context, "Foo::Bar::CONST");
assert_declaration_does_not_exist!(context, "Foo::Bar::<Bar>#@class_ivar");
assert_declaration_does_not_exist!(context, "Foo::Bar#@instance_ivar");
assert_declaration_does_not_exist!(context, "Foo::Bar#@@class_var");
assert_declaration_does_not_exist!(context, "Foo::Bar#some_attr()");
assert_declaration_does_not_exist!(context, "Foo::Bar::<Bar>#class_method()");
assert_declaration_does_not_exist!(context, "Foo::Bar#initialize()");
}

#[test]
fn re_opening_namespace_alias() {
let mut context = GraphTest::new();
context.index_uri("file:///a.rb", {
r"
module Foo; end
ALIAS = Foo

module ALIAS
CONST = 123
@class_ivar = 123
@@class_var = 789

attr_reader :some_attr

def self.class_method; end

def initialize
@instance_ivar = 456
end
end
"
});

context.resolve();
assert_declaration_exists!(context, "Foo::CONST");
assert_declaration_exists!(context, "Foo::<Foo>#@class_ivar");
assert_declaration_exists!(context, "Foo#@instance_ivar");
assert_declaration_exists!(context, "Foo#@@class_var");
assert_declaration_exists!(context, "Foo#some_attr()");
assert_declaration_exists!(context, "Foo::<Foo>#class_method()");
assert_declaration_exists!(context, "Foo#initialize()");
Copy link
Contributor

@Morriar Morriar Mar 4, 2026

Choose a reason for hiding this comment

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

The test covers every definition type (constants, ivars, class vars, attrs, methods) except alias new_name old_name (Definition::MethodAlias), whose callsite was also updated on line 512. Any chance we can add one more assertion here?

Suggested change
assert_declaration_exists!(context, "Foo#initialize()");
assert_declaration_exists!(context, "Foo#initialize()");
assert_declaration_exists!(context, "Foo#bar()"); // alias bar initialize
}

With a matching alias bar initialize added to the module ALIAS body.

Copy link
Member Author

@vinistock vinistock Mar 5, 2026

Choose a reason for hiding this comment

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

#642

}
}
Loading