Skip to content

Index includes, prepends, and extends with RBS indexer #617

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
alexcrocha merged 1 commit into from
Mar 4, 2026
Merged

Index includes, prepends, and extends with RBS indexer #617

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
113 changes: 109 additions & 4 deletions rust/rubydex/src/indexing/rbs_indexer.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,17 +1,19 @@
//! Visit the RBS AST and create type definitions.

use ruby_rbs::node::{
self, ClassNode, CommentNode, ConstantNode, GlobalNode, ModuleNode, Node, NodeList, TypeNameNode, Visit,
self, ClassNode, CommentNode, ConstantNode, ExtendNode, GlobalNode, IncludeNode, ModuleNode, Node, NodeList,
PrependNode, TypeNameNode, Visit,
};

use crate::diagnostic::Rule;
use crate::indexing::local_graph::LocalGraph;
use crate::model::comment::Comment;
use crate::model::definitions::{
ClassDefinition, ConstantDefinition, Definition, DefinitionFlags, GlobalVariableDefinition, ModuleDefinition,
ClassDefinition, ConstantDefinition, Definition, DefinitionFlags, ExtendDefinition, GlobalVariableDefinition,
IncludeDefinition, Mixin, ModuleDefinition, PrependDefinition,
};
use crate::model::document::Document;
use crate::model::ids::{DefinitionId, NameId, UriId};
use crate::model::ids::{DefinitionId, NameId, ReferenceId, UriId};
use crate::model::name::{Name, ParentScope};
use crate::model::references::ConstantReference;
use crate::offset::Offset;
Expand Down Expand Up @@ -103,6 +105,35 @@ impl<'a> RBSIndexer<'a> {
})
}

fn add_mixin_to_current_lexical_scope(&mut self, owner_id: DefinitionId, mixin: Mixin) {
let owner = self
.local_graph
.get_definition_mut(owner_id)
.expect("owner definition should exist");

match owner {
Definition::Class(class) => class.add_mixin(mixin),
Definition::Module(module) => module.add_mixin(mixin),
_ => unreachable!("RBS nesting stack only contains modules/classes"),
}
}

fn index_mixin(&mut self, type_name: &TypeNameNode, mixin_fn: fn(ReferenceId) -> Mixin) {
let Some(lexical_nesting_id) = self.parent_lexical_scope_id() else {
return;
};

let nesting_name_id = self.nesting_name_id(Some(lexical_nesting_id));
let name_id = self.index_type_name(type_name, nesting_name_id);
let offset = Offset::from_rbs_location(&type_name.location());

let constant_ref_id =
self.local_graph
.add_constant_reference(ConstantReference::new(name_id, self.uri_id, offset));

self.add_mixin_to_current_lexical_scope(lexical_nesting_id, mixin_fn(constant_ref_id));
}

fn add_member_to_current_lexical_scope(&mut self, owner_id: DefinitionId, member_id: DefinitionId) {
let owner = self
.local_graph
Expand Down Expand Up @@ -275,6 +306,24 @@ impl Visit for RBSIndexer<'_> {

self.register_definition(definition, lexical_nesting_id);
}

fn visit_include_node(&mut self, include_node: &IncludeNode) {
self.index_mixin(&include_node.name(), |ref_id| {
Mixin::Include(IncludeDefinition::new(ref_id))
});
}

fn visit_prepend_node(&mut self, prepend_node: &PrependNode) {
self.index_mixin(&prepend_node.name(), |ref_id| {
Mixin::Prepend(PrependDefinition::new(ref_id))
});
}

fn visit_extend_node(&mut self, extend_node: &ExtendNode) {
self.index_mixin(&extend_node.name(), |ref_id| {
Mixin::Extend(ExtendDefinition::new(ref_id))
});
}
}

#[cfg(test)]
Expand All @@ -285,7 +334,7 @@ mod tests {
use crate::model::definitions::DefinitionFlags;
use crate::test_utils::LocalGraphTest;
use crate::{
assert_def_comments_eq, assert_def_name_eq, assert_def_name_offset_eq, assert_def_str_eq,
assert_def_comments_eq, assert_def_mixins_eq, assert_def_name_eq, assert_def_name_offset_eq, assert_def_str_eq,
assert_def_superclass_ref_eq, assert_definition_at, assert_local_diagnostics_eq, assert_no_local_diagnostics,
};

Expand Down Expand Up @@ -506,6 +555,62 @@ mod tests {
});
Copy link
Contributor

@Morriar Morriar Mar 2, 2026

Choose a reason for hiding this comment

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

The Ruby indexer has index_includes_at_top_level to confirm top-level mixins are silently ignored. Should we add an equivalent here for the RBS indexer to document that behavior explicitly?

Copy link
Member

@vinistock vinistock Mar 2, 2026

Choose a reason for hiding this comment

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

Wait, is that the case? If so, we have a bug. Mixin operations at the top level impact Object:

module Foo; end

include Foo

puts Object.ancestors.inspect
# => [Object, Foo, Kernel, BasicObject]

Regardless, we should add the test.

Copy link
Contributor Author

@alexcrocha alexcrocha Mar 3, 2026

Choose a reason for hiding this comment

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

Including at the top level returns a parse error.

In RBS docs, mixins are not part of the declarations and I believe we're hitting the default case here.

Doesn't look like this is an issue for the RBS indexer

}

#[test]
fn index_mixins() {
let context = index_source({
"
class Foo
include Bar
prepend Baz
extend Qux
end
"
});

assert_no_local_diagnostics!(&context);

assert_definition_at!(&context, "1:1-5:4", Class, |def| {
assert_def_mixins_eq!(&context, def, Include, ["Bar"]);
assert_def_mixins_eq!(&context, def, Prepend, ["Baz"]);
assert_def_mixins_eq!(&context, def, Extend, ["Qux"]);
});
}

#[test]
fn index_multiple_includes() {
let context = index_source({
"
module Foo
include Bar
include Baz
end
"
});

assert_no_local_diagnostics!(&context);

assert_definition_at!(&context, "1:1-4:4", Module, |def| {
assert_def_mixins_eq!(&context, def, Include, ["Bar", "Baz"]);
});
}

#[test]
fn index_include_qualified_name() {
let context = index_source({
"
class Foo
include Bar::Baz
end
"
});
Copy link
Contributor

@Morriar Morriar Mar 2, 2026

Choose a reason for hiding this comment

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

The test name says "qualified name" but assert_def_mixins_eq\! only checks the leaf segment ("Baz"), not the full Bar::Baz path. I wonder if we should assert the full path here to confirm the qualified name resolves correctly — maybe using assert_name_path_eq\! on the mixin's reference?

Copy link
Member

@vinistock vinistock Mar 2, 2026

Choose a reason for hiding this comment

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

Yeah, we should assert on the entire string Bar::Baz and not just Baz. Although, that's an existing limitation. If this involves updating too many tests, let's do it in a different PR.

Copy link
Contributor Author

@alexcrocha alexcrocha Mar 3, 2026

Choose a reason for hiding this comment

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

I'll take the suggestion and fix the macro to assert the full qualified path in a follow-up PR 👍


assert_no_local_diagnostics!(&context);

assert_definition_at!(&context, "1:1-3:4", Class, |def| {
assert_def_mixins_eq!(&context, def, Include, ["Baz"]);
});
}

#[test]
fn index_class_and_module_nesting() {
let context = index_source({
Expand Down
44 changes: 2 additions & 42 deletions rust/rubydex/src/indexing/ruby_indexer.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2035,57 +2035,17 @@ impl Visit<'_> for RubyIndexer<'_> {
#[cfg(test)]
mod tests {
use crate::{
assert_def_comments_eq, assert_def_name_eq, assert_def_name_offset_eq, assert_def_str_eq,
assert_def_comments_eq, assert_def_mixins_eq, assert_def_name_eq, assert_def_name_offset_eq, assert_def_str_eq,
assert_def_superclass_ref_eq, assert_definition_at, assert_local_diagnostics_eq, assert_name_path_eq,
assert_no_local_diagnostics, assert_string_eq,
model::{
definitions::{Definition, Mixin, Parameter, Receiver},
definitions::{Definition, Parameter, Receiver},
ids::{StringId, UriId},
visibility::Visibility,
},
test_utils::LocalGraphTest,
};

/// Asserts that a definition's mixins matches the expected mixins.
///
/// Usage:
/// - `assert_def_mixins_eq!(ctx, def, Include, ["Foo", "Bar"])`
macro_rules! assert_def_mixins_eq {
($context:expr, $def:expr, $mixin_type:ident, $expected_names:expr) => {{
let actual_names = $def
.mixins()
.iter()
.filter_map(|mixin| {
if let Mixin::$mixin_type(def) = mixin {
let name = $context
.graph()
.names()
.get(
$context
.graph()
.constant_references()
.get(def.constant_reference_id())
.unwrap()
.name_id(),
)
.unwrap();
Some($context.graph().strings().get(name.str()).unwrap().as_str())
} else {
None
}
})
.collect::<Vec<_>>();

assert_eq!(
$expected_names,
actual_names.as_slice(),
"mixins mismatch: expected `{:?}`, got `{:?}`",
$expected_names,
actual_names
);
}};
}

// Method assertions

/// Asserts that a method has the expected receiver.
Expand Down
24 changes: 24 additions & 0 deletions rust/rubydex/src/resolution.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4526,6 +4526,30 @@ mod tests {
assert_members_eq!(context, "Object", ["$foo"]);
}

#[test]
fn rbs_mixin_resolution() {
let mut context = GraphTest::new();
context.index_rbs_uri("file:///test.rbs", {
r"
module Bar
end

module Baz
end

class Foo
include Bar
include Baz
end
"
});
context.resolve();

assert_no_diagnostics!(&context);

assert_ancestors_eq!(context, "Foo", ["Foo", "Baz", "Bar", "Object"]);
}

#[test]
fn resolving_meta_programming_class_reopened() {
// It's often not possible to provide first-class support to meta-programming constructs, but we have to prevent
Expand Down
46 changes: 46 additions & 0 deletions rust/rubydex/src/test_utils/local_graph_test.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -321,6 +321,52 @@ macro_rules! assert_def_comments_eq {
}};
}

// Mixin assertions

/// Asserts that a definition's mixins match the expected names for a given mixin type.
///
/// Usage:
/// - `assert_def_mixins_eq!(ctx, def, Include, ["Foo", "Bar"])`
#[cfg(test)]
#[macro_export]
macro_rules! assert_def_mixins_eq {
($context:expr, $def:expr, $mixin_type:ident, $expected_names:expr) => {{
use $crate::model::definitions::Mixin;

let actual_names = $def
.mixins()
.iter()
.filter_map(|mixin| {
if let Mixin::$mixin_type(def) = mixin {
let name = $context
.graph()
.names()
.get(
$context
.graph()
.constant_references()
.get(def.constant_reference_id())
.unwrap()
.name_id(),
)
.unwrap();
Some($context.graph().strings().get(name.str()).unwrap().as_str())
} else {
None
}
})
.collect::<Vec<_>>();

assert_eq!(
$expected_names,
actual_names.as_slice(),
"mixins mismatch: expected `{:?}`, got `{:?}`",
$expected_names,
actual_names
);
}};
}

// Diagnostic assertions

#[cfg(test)]
Expand Down
Loading