Skip to content

Commit 8a1b573

Browse files
authored
Merge pull request #529 from Shopify/at-todo-decl
Create TODO declarations for unresolved parent scopes
2 parents 1cb25cb + f7e4c2f commit 8a1b573

6 files changed

Lines changed: 236 additions & 16 deletions

File tree

ext/rubydex/declaration.c

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ VALUE cNamespace;
1111
VALUE cClass;
1212
VALUE cModule;
1313
VALUE cSingletonClass;
14+
VALUE cTodo;
1415
VALUE cConstant;
1516
VALUE cConstantAlias;
1617
VALUE cMethod;
@@ -27,6 +28,8 @@ VALUE rdxi_declaration_class_for_kind(CDeclarationKind kind) {
2728
return cModule;
2829
case CDeclarationKind_SingletonClass:
2930
return cSingletonClass;
31+
case CDeclarationKind_Todo:
32+
return cTodo;
3033
case CDeclarationKind_Constant:
3134
return cConstant;
3235
case CDeclarationKind_ConstantAlias:
@@ -334,6 +337,7 @@ void rdxi_initialize_declaration(VALUE mRubydex) {
334337
cClass = rb_define_class_under(mRubydex, "Class", cNamespace);
335338
cModule = rb_define_class_under(mRubydex, "Module", cNamespace);
336339
cSingletonClass = rb_define_class_under(mRubydex, "SingletonClass", cNamespace);
340+
cTodo = rb_define_class_under(mRubydex, "Todo", cNamespace);
337341
cConstant = rb_define_class_under(mRubydex, "Constant", cDeclaration);
338342
cConstantAlias = rb_define_class_under(mRubydex, "ConstantAlias", cDeclaration);
339343
cMethod = rb_define_class_under(mRubydex, "Method", cDeclaration);

ext/rubydex/declaration.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ extern VALUE cNamespace;
99
extern VALUE cClass;
1010
extern VALUE cModule;
1111
extern VALUE cSingletonClass;
12+
extern VALUE cTodo;
1213
extern VALUE cConstant;
1314
extern VALUE cConstantAlias;
1415
extern VALUE cMethod;

rust/rubydex-sys/src/declaration_api.rs

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@ pub enum CDeclarationKind {
2323
GlobalVariable = 6,
2424
InstanceVariable = 7,
2525
ClassVariable = 8,
26+
Todo = 9,
2627
}
2728

2829
#[repr(C)]
@@ -52,6 +53,7 @@ impl CDeclaration {
5253
Declaration::Namespace(Namespace::Class(_)) => CDeclarationKind::Class,
5354
Declaration::Namespace(Namespace::Module(_)) => CDeclarationKind::Module,
5455
Declaration::Namespace(Namespace::SingletonClass(_)) => CDeclarationKind::SingletonClass,
56+
Declaration::Namespace(Namespace::Todo(_)) => CDeclarationKind::Todo,
5557
Declaration::Constant(_) => CDeclarationKind::Constant,
5658
Declaration::ConstantAlias(_) => CDeclarationKind::ConstantAlias,
5759
Declaration::Method(_) => CDeclarationKind::Method,

rust/rubydex/src/model/declaration.rs

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -60,6 +60,7 @@ macro_rules! all_declarations {
6060
Declaration::Namespace(Namespace::Class($var)) => $expr,
6161
Declaration::Namespace(Namespace::Module($var)) => $expr,
6262
Declaration::Namespace(Namespace::SingletonClass($var)) => $expr,
63+
Declaration::Namespace(Namespace::Todo($var)) => $expr,
6364
Declaration::Constant($var) => $expr,
6465
Declaration::ConstantAlias($var) => $expr,
6566
Declaration::Method($var) => $expr,
@@ -76,6 +77,7 @@ macro_rules! all_namespaces {
7677
Namespace::Class($var) => $expr,
7778
Namespace::Module($var) => $expr,
7879
Namespace::SingletonClass($var) => $expr,
80+
Namespace::Todo($var) => $expr,
7981
}
8082
};
8183
}
@@ -378,6 +380,7 @@ pub enum Namespace {
378380
Class(Box<ClassDeclaration>),
379381
SingletonClass(Box<SingletonClassDeclaration>),
380382
Module(Box<ModuleDeclaration>),
383+
Todo(Box<TodoDeclaration>),
381384
}
382385
assert_mem_size!(Namespace, 16);
383386

@@ -388,6 +391,7 @@ impl Namespace {
388391
Namespace::Class(_) => "Class",
389392
Namespace::SingletonClass(_) => "SingletonClass",
390393
Namespace::Module(_) => "Module",
394+
Namespace::Todo(_) => "<TODO>",
391395
}
392396
}
393397

@@ -504,7 +508,8 @@ namespace_declaration!(Module, ModuleDeclaration);
504508
assert_mem_size!(ModuleDeclaration, 224);
505509
namespace_declaration!(SingletonClass, SingletonClassDeclaration);
506510
assert_mem_size!(SingletonClassDeclaration, 224);
507-
511+
namespace_declaration!(Todo, TodoDeclaration);
512+
assert_mem_size!(TodoDeclaration, 224);
508513
simple_declaration!(ConstantDeclaration);
509514
assert_mem_size!(ConstantDeclaration, 112);
510515
simple_declaration!(MethodDeclaration);

rust/rubydex/src/model/graph.rs

Lines changed: 18 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -84,14 +84,20 @@ impl Graph {
8484
{
8585
let declaration_id = DeclarationId::from(&fully_qualified_name);
8686

87-
let should_promote = self.declarations.get(&declaration_id).is_some_and(|existing| {
88-
matches!(existing, Declaration::Constant(_))
89-
&& matches!(
90-
self.definitions.get(&definition_id),
91-
Some(Definition::Class(_) | Definition::Module(_) | Definition::SingletonClass(_))
92-
)
93-
&& self.all_definitions_promotable(existing)
94-
});
87+
let is_namespace_definition = matches!(
88+
self.definitions.get(&definition_id),
89+
Some(Definition::Class(_) | Definition::Module(_) | Definition::SingletonClass(_))
90+
);
91+
92+
let should_promote = is_namespace_definition
93+
&& self
94+
.declarations
95+
.get(&declaration_id)
96+
.is_some_and(|existing| match existing {
97+
Declaration::Constant(_) => self.all_definitions_promotable(existing),
98+
Declaration::Namespace(Namespace::Todo(_)) => true,
99+
_ => false,
100+
});
95101

96102
match self.declarations.entry(declaration_id) {
97103
Entry::Occupied(mut occupied_entry) => {
@@ -633,6 +639,10 @@ impl Graph {
633639
Declaration::Namespace(Namespace::SingletonClass(it)) => {
634640
it.add_member(member_str_id, member_declaration_id);
635641
}
642+
Declaration::Namespace(Namespace::Todo(it)) => it.add_member(member_str_id, member_declaration_id),
643+
Declaration::Constant(_) => {
644+
// TODO: temporary hack to avoid crashing on `Struct.new`, `Class.new` and `Module.new`
645+
}
636646
_ => panic!("Tried to add member to a declaration that isn't a namespace"),
637647
}
638648
}

rust/rubydex/src/resolution.rs

Lines changed: 205 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@ use crate::model::{
77
declaration::{
88
Ancestor, Ancestors, ClassDeclaration, ClassVariableDeclaration, ConstantAliasDeclaration, ConstantDeclaration,
99
Declaration, GlobalVariableDeclaration, InstanceVariableDeclaration, MethodDeclaration, ModuleDeclaration,
10-
Namespace, SingletonClassDeclaration,
10+
Namespace, SingletonClassDeclaration, TodoDeclaration,
1111
},
1212
definitions::{Definition, Mixin, Receiver},
1313
graph::{CLASS_ID, Graph, MODULE_ID, OBJECT_ID},
@@ -1057,12 +1057,60 @@ impl<'a> Resolver<'a> {
10571057
fn name_owner_id(&mut self, name_id: NameId) -> Outcome {
10581058
let name_ref = self.graph.names().get(&name_id).unwrap();
10591059

1060-
if let Some(parent_scope) = name_ref.parent_scope().as_ref() {
1060+
if let Some(&parent_scope) = name_ref.parent_scope().as_ref() {
10611061
// If we have `A::B`, the owner of `B` is whatever `A` resolves to.
10621062
// If `A` is an alias, resolve through to get the actual namespace.
1063-
match self.resolve_constant_internal(*parent_scope) {
1063+
// On `Retry`, we don't create a Todo: the parent may still resolve through inheritance once ancestors are
1064+
// linearized. We only create Todos for `Unresolved` outcomes where the parent is genuinely unknown.
1065+
match self.resolve_constant_internal(parent_scope) {
10641066
Outcome::Resolved(id, linearization) => self.resolve_to_primary_namespace(id, linearization),
1065-
other => other,
1067+
// Retry or Unresolved(Some(_)) means we might find it later through ancestor linearization
1068+
Outcome::Retry(id) => Outcome::Retry(id),
1069+
Outcome::Unresolved(Some(id)) => Outcome::Unresolved(Some(id)),
1070+
// Only create a Todo when genuinely unresolvable (no pending linearizations)
1071+
Outcome::Unresolved(None) => {
1072+
let parent_name = self.graph.names().get(&parent_scope).unwrap();
1073+
let parent_str_id = *parent_name.str();
1074+
let parent_has_explicit_prefix = parent_name.parent_scope().as_ref().is_some();
1075+
// NLL: borrow of parent_name ends here
1076+
1077+
// For bare names (no explicit `::` prefix), always use OBJECT_ID as the owner.
1078+
// Using nesting here would create "Nesting::Bar" instead of "Bar" for a bare `Bar`
1079+
// reference, which is incorrect: if `Bar` can't be found anywhere, the placeholder
1080+
// should live at the top level so it can be promoted when `module Bar` appears later.
1081+
let parent_owner_id = if parent_has_explicit_prefix {
1082+
match self.name_owner_id(parent_scope) {
1083+
Outcome::Resolved(id, _) => id,
1084+
_ => *OBJECT_ID,
1085+
}
1086+
} else {
1087+
*OBJECT_ID
1088+
};
1089+
1090+
let fully_qualified_name = if parent_owner_id == *OBJECT_ID {
1091+
self.graph.strings().get(&parent_str_id).unwrap().to_string()
1092+
} else {
1093+
format!(
1094+
"{}::{}",
1095+
self.graph.declarations().get(&parent_owner_id).unwrap().name(),
1096+
self.graph.strings().get(&parent_str_id).unwrap().as_str()
1097+
)
1098+
};
1099+
1100+
let declaration_id = DeclarationId::from(&fully_qualified_name);
1101+
1102+
if let std::collections::hash_map::Entry::Vacant(e) =
1103+
self.graph.declarations_mut().entry(declaration_id)
1104+
{
1105+
e.insert(Declaration::Namespace(Namespace::Todo(Box::new(TodoDeclaration::new(
1106+
fully_qualified_name,
1107+
parent_owner_id,
1108+
)))));
1109+
self.graph.add_member(&parent_owner_id, declaration_id, parent_str_id);
1110+
}
1111+
1112+
Outcome::Resolved(declaration_id, None)
1113+
}
10661114
}
10671115
} else if let Some(nesting_id) = name_ref.nesting()
10681116
&& !name_ref.parent_scope().is_top_level()
@@ -1916,9 +1964,12 @@ mod tests {
19161964

19171965
assert_no_diagnostics!(&context);
19181966

1919-
assert_declaration_does_not_exist!(context, "Foo");
1920-
assert_declaration_does_not_exist!(context, "Foo::Bar");
1921-
assert_declaration_does_not_exist!(context, "Foo::Bar::Baz");
1967+
assert_declaration_kind_eq!(context, "Foo", "<TODO>");
1968+
1969+
assert_members_eq!(context, "Object", vec!["Foo"]);
1970+
assert_members_eq!(context, "Foo", vec!["Bar"]);
1971+
assert_members_eq!(context, "Foo::Bar", vec!["Baz"]);
1972+
assert_no_members!(context, "Foo::Bar::Baz");
19221973
}
19231974

19241975
#[test]
@@ -5261,4 +5312,151 @@ mod tests {
52615312
assert_declaration_does_not_exist!(context, "Foo::<Foo>");
52625313
assert_declaration_does_not_exist!(context, "Foo::<Foo>#bar()");
52635314
}
5315+
5316+
#[test]
5317+
fn resolve_missing_declaration_to_todo() {
5318+
let mut context = GraphTest::new();
5319+
context.index_uri("file:///foo.rb", {
5320+
r"
5321+
class Foo::Bar
5322+
include Foo::Baz
5323+
5324+
def bar; end
5325+
end
5326+
5327+
module Foo::Baz
5328+
def baz; end
5329+
end
5330+
"
5331+
});
5332+
context.resolve();
5333+
5334+
assert_no_diagnostics!(&context);
5335+
5336+
assert_declaration_kind_eq!(context, "Foo", "<TODO>");
5337+
5338+
assert_members_eq!(context, "Object", vec!["Foo"]);
5339+
assert_members_eq!(context, "Foo", vec!["Bar", "Baz"]);
5340+
assert_members_eq!(context, "Foo::Bar", vec!["bar()"]);
5341+
assert_members_eq!(context, "Foo::Baz", vec!["baz()"]);
5342+
}
5343+
5344+
#[test]
5345+
fn todo_declaration_promoted_to_real_namespace() {
5346+
let mut context = GraphTest::new();
5347+
context.index_uri("file:///foo.rb", {
5348+
r"
5349+
class Foo::Bar
5350+
def bar; end
5351+
end
5352+
5353+
class Foo
5354+
def foo; end
5355+
end
5356+
"
5357+
});
5358+
context.resolve();
5359+
5360+
assert_no_diagnostics!(&context);
5361+
5362+
// Foo was initially created as a Todo (from class Foo::Bar), then promoted to Class
5363+
assert_declaration_kind_eq!(context, "Foo", "Class");
5364+
5365+
assert_members_eq!(context, "Object", vec!["Foo"]);
5366+
assert_members_eq!(context, "Foo", vec!["Bar", "foo()"]);
5367+
assert_members_eq!(context, "Foo::Bar", vec!["bar()"]);
5368+
}
5369+
5370+
#[test]
5371+
fn todo_declaration_promoted_to_real_namespace_incrementally() {
5372+
let mut context = GraphTest::new();
5373+
context.index_uri("file:///bar.rb", {
5374+
r"
5375+
class Foo::Bar
5376+
def bar; end
5377+
end
5378+
"
5379+
});
5380+
context.resolve();
5381+
5382+
assert_no_diagnostics!(&context);
5383+
assert_declaration_kind_eq!(context, "Foo", "<TODO>");
5384+
5385+
context.index_uri("file:///foo.rb", {
5386+
r"
5387+
class Foo
5388+
def foo; end
5389+
end
5390+
"
5391+
});
5392+
context.resolve();
5393+
5394+
assert_no_diagnostics!(&context);
5395+
5396+
// Foo was promoted from Todo to Class after the second resolution
5397+
assert_declaration_kind_eq!(context, "Foo", "Class");
5398+
5399+
assert_members_eq!(context, "Object", vec!["Foo"]);
5400+
assert_members_eq!(context, "Foo", vec!["Bar", "foo()"]);
5401+
assert_members_eq!(context, "Foo::Bar", vec!["bar()"]);
5402+
}
5403+
5404+
#[test]
5405+
fn qualified_name_inside_nesting_resolves_to_top_level() {
5406+
let mut context = GraphTest::new();
5407+
context.index_uri("file:///foo.rb", {
5408+
r"
5409+
module Foo
5410+
class Bar::Baz
5411+
def qux; end
5412+
end
5413+
end
5414+
5415+
module Bar
5416+
end
5417+
"
5418+
});
5419+
context.resolve();
5420+
5421+
assert_no_diagnostics!(&context);
5422+
assert_declaration_kind_eq!(context, "Bar", "Module");
5423+
assert_members_eq!(context, "Bar", vec!["Baz"]);
5424+
assert_declaration_exists!(context, "Bar::Baz");
5425+
assert_members_eq!(context, "Bar::Baz", vec!["qux()"]);
5426+
assert_declaration_does_not_exist!(context, "Foo::Bar");
5427+
}
5428+
5429+
#[test]
5430+
fn qualified_name_inside_nesting_resolves_when_discovered_incrementally() {
5431+
let mut context = GraphTest::new();
5432+
context.index_uri("file:///baz.rb", {
5433+
r"
5434+
module Foo
5435+
class Bar::Baz
5436+
def qux; end
5437+
end
5438+
end
5439+
"
5440+
});
5441+
context.resolve();
5442+
5443+
// Bar is unknown — a Todo is created at the top level, not "Foo::Bar"
5444+
assert_declaration_kind_eq!(context, "Bar", "<TODO>");
5445+
assert_declaration_does_not_exist!(context, "Foo::Bar");
5446+
5447+
context.index_uri("file:///bar.rb", {
5448+
r"
5449+
module Bar
5450+
end
5451+
"
5452+
});
5453+
context.resolve();
5454+
5455+
// After discovering top-level Bar, the Todo should be promoted and Baz re-homed.
5456+
assert_no_diagnostics!(&context);
5457+
assert_declaration_kind_eq!(context, "Bar", "Module");
5458+
assert_members_eq!(context, "Bar", vec!["Baz"]);
5459+
assert_members_eq!(context, "Bar::Baz", vec!["qux()"]);
5460+
assert_declaration_does_not_exist!(context, "Foo::Bar");
5461+
}
52645462
}

0 commit comments

Comments
 (0)