Fix definition_id_to_declaration_id for SelfReceiver methods#726
Conversation
st0012
force-pushed
the
fix-self-receiver-definition-mapping
branch
3 times, most recently
from
447e822 to
3d8b03c
Compare
|
|
||
| /// Looks up the declaration for a `SelfReceiver` method/alias through the singleton class. | ||
| fn find_self_receiver_declaration(&self, def_id: DefinitionId, member_str_id: StringId) -> Option<&DeclarationId> { | ||
| let owner_decl_id = self.definition_id_to_declaration_id(def_id)?; |
There was a problem hiding this comment.
How about panicking early instead of returning None for these ? cases? They indicate data corruption.
The last member(...) call is the only valid case to return None I think.
There was a problem hiding this comment.
Depending on resolution order, it's possible that the owner and its singleton class aren't resolved yet. In those cases, returning None is ok. If you check other branches of the caller definition_to_declaration_id function, you can see methods like name_id_to_declaration_id can return None when NameRef is unresolved as well.
There was a problem hiding this comment.
Got it — owner_decl_id can be None when the owner isn't resolved yet, and singleton_class() can be None when it hasn't been created yet. I'm good to return None for these cases.
But I think declarations.get() and as_namespace() in between should never fail once we have a valid ID. It might be worth using unwrap() there to make the invariant explicit, but it's a minor point — up to you.
st0012
force-pushed
the
fix-self-receiver-definition-mapping
branch
from
3d8b03c to
ac5a646
Compare
st0012
force-pushed
the
fix-self-receiver-definition-mapping
branch
from
ac5a646 to
1766982
Compare
vinistock
left a comment
vinistock
left a comment
There was a problem hiding this comment.
Do we have tests for the connection of definition -> declaration for instance variables inside a method with receiver? E.g.:
def Foo.bar
@baz = 1 # do we find the right declaration for @baz?
end
definition_to_declaration_id maps a definition to its owning
declaration. For Method definitions, it used the enclosing namespace
+ member lookup, which works for instance methods but returns the
wrong declaration for SelfReceiver methods (def self.foo).
When both def self.run and def run exist, member("run") on the
enclosing class finds the instance method declaration instead of
the singleton method declaration. This caused pending_detachments
during invalidation to detach from the wrong place, leaving the
SelfReceiver definition attached to the singleton declaration.
On re-add, add_definition panicked with "duplicate definition".
Fix: check for SelfReceiver and look up through the singleton class,
mirroring how resolution places these methods.
st0012
force-pushed
the
fix-self-receiver-definition-mapping
branch
from
1766982 to
90dff59
Compare
|
@vinistock Yeah we're handling that correctly. Just added a test |
3 participants
Summary
definition_to_declaration_idforMethoddefinitions used the enclosing namespace +member(str_id)lookup, which returns the wrong declaration when bothdef self.runanddef runexist — it finds the instance method instead of the singleton methodpending_detachmentsduring invalidation to detach from the wrong declaration, leaving the SelfReceiver definition attached to the singleton. On re-add,add_definitionpanicked with "duplicate definition"SelfReceiverand look up through the singleton class, mirroring how resolution places these methods. Same fix applied toMethodAliaswithSelfReceiver(RBSalias self.x self.y)Tests
resolution_for_self_method_with_same_name_instance_method— verifiesdefinition_id_to_declaration_idmaps correctlyresolution_for_self_method_alias_with_same_name_instance_method— same for RBS method aliasesno_duplicate_definition_on_identical_file_delete_readd— incremental regression test