Create Todo declarations for multi-level compact namespaces#656
Open
Create Todo declarations for multi-level compact namespaces#656
Conversation
127a76d to
e0f5147
Compare
Previously, `class A::B::C` where A is unknown would only create a Todo for A (the first unresolvable parent), leaving B and C stuck in the retry loop indefinitely. This caused a panic when the class contained `def self.foo` with instance variables, because the SelfReceiver lookup expected C's definition to have a declaration. The root cause was in `name_owner_id`: `resolve_constant_internal(B)` returned `Retry(None)` (because A's name was unresolved), which was passed through unchanged. The `Unresolved(None)` branch that creates Todos was never reached for intermediate names in the constant path. Fix: merge `Retry(None)` with `Unresolved(None)` in `name_owner_id`. The recursive `name_owner_id(parent_scope)` call naturally walks the entire chain, creating Todos at each level. If real definitions appear later, the existing promotion mechanism upgrades Todos in place.
e0f5147 to
a1d6f41
Compare
Member
Author
|
@Morriar There are different ways to resolve the panic mentioned in the description. Looking at #529, I don't think 1-level-only todo declaration creation is intentional, so I opened this PR. If that's the intend, I think we should document it somewhere (architecture.md?), and I have another way to address the panic. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
class A::B::CwhereAis unknown panics when the class body containsdef self.foowith instance variables:The panic occurs in
handle_remaining_definitionsbecauseC's definition never gets a declaration — the resolution loop retries indefinitely and then gives up.Fixes #663
Root cause
The Todo mechanism from #529 only handles one level of unknown parent in compact notation. For
class Foo::BarwhereFoois unknown,resolve_constant_internal(Foo)returnsUnresolved(None), which triggers Todo creation forFooinname_owner_id.For multi-level paths like
class A::B::C, the flow differs:name_owner_id(C)callsresolve_constant_internal(B)BhasParentScope::Attached(A)— checks ifA's name is resolvedAis unresolved → returnsRetry(None)(line 1189)name_owner_idpassesRetry(None)through unchanged — never reaches the Todo creation branchA, so the loop stallsThe
Unresolved(None)branch (which creates Todos) is only reached whenresolve_constant_internalgoes throughrun_resolutionfor a bare name. Intermediate names withParentScope::Attachedshort-circuit at the unresolved parent check.Fix
In
name_owner_id, mergeRetry(None)withUnresolved(None)to eagerly create Todos for the entire parent chain:The recursive
name_owner_id(parent_scope)call at line 1089 naturally handles arbitrary depth — each level either resolves to an existing declaration or creates a Todo. If real definitions appear later, the existing promotion mechanism upgrades Todos in place.Retry(Some(id))(pending ancestor linearization) is still passed through unchanged — this only affectsRetry(None), which in theParentScope::Attachedcode path exclusively means "the parent's parent name is unresolved."