Avoid poisoning non identifier names #4884
Conversation
There are different use cases where we call `Context::LookupQualifiedName()` on `NameId::SelfType`, which implicitly triggers poisoning this name. I don't think poisoning `Self` is ever useful, and it's likely that's true for other (if not all) special names (`Self` seems to be the most common use case though). Use cases where `Self` is being poisoned: * Checking allowed access: https://github.com/carbon-language/carbon-lang/blob/e257051612e4217295e206fd3274fc75e22d206a/toolchain/check/member_access.cpp#L62, for example, in https://github.com/carbon-language/carbon-lang/blob/e257051612e4217295e206fd3274fc75e22d206a/toolchain/check/testdata/alias/no_prelude/fail_aliased_name_in_diag.carbon#L21 * Using the type `Self` as a parameter type: https://github.com/carbon-language/carbon-lang/blob/e257051612e4217295e206fd3274fc75e22d206a/core/prelude/operators/arithmetic.carbon#L78 Part of carbon-language#4622.
toolchain/check/context.cpp
Outdated
| if (auto entry_id = is_being_declared || name_id == SemIR::NameId::SelfType | ||
| ? scope.Lookup(name_id) | ||
| : scope.LookupOrPoison(name_id)) { |
There was a problem hiding this comment.
Can you point to a test where this fires? At trunk, I added here:
CARBON_CHECK(!is_being_declared || (name_id == SemIR::NameId::Base ||
name_id.AsIdentifierId().has_value()),
"{0}", name_id);
And that seems to pass. But also, that could just be put into LookupOrPoison without changing this code if you wanted to just assert that the code was never used that way. What am I missing that you're special-casing SelfType here?
There was a problem hiding this comment.
How does this assert help checking this?
This is what I added, which fails:
CARBON_CHECK(is_being_declared || name_id != SemIR::NameId::SelfType, "{0}",
name_id);
I've encountered that when I tried to add poisoned names to the formatter and noticed that in a lot of cases, Self is being poisoned.
I don't think poisoning Self has an effect except costing more resources.
I wanted to clean this before trying to format poisoning names (which might be debateable regardless) so there we won't see a lot of file diffs because of that unnecessary poisoning.
There was a problem hiding this comment.
Sorry, maybe I'm being unclear here.
You're adding "|| name_id == SemIR::NameId::SelfType". I'm saying, I tried adding a CHECK that that condition was never true, and it seems to never be true. What am I missing? Why does it need to be handled?
There was a problem hiding this comment.
I've changed the logic of LookupOrPoison() to not poison non identifiers and improved the unit tests to properly demonstrate that.
There are a lot of tests that trigger that, but you don't see the impact because poisoning |
toolchain/sem_ir/name_scope.h
Outdated
| // | ||
| // This cannot be used to lookup `SelfType`; use `Lookup` instead. | ||
| // poisons the name so it can't be declared later. Names that are not | ||
| // identifiers would not be poisoned. |
There was a problem hiding this comment.
| // identifiers would not be poisoned. | |
| // identifiers will not be poisoned. |
There are different use cases where we call
Context::LookupQualifiedName()on non identifiers, likeNameId::SelfType, which implicitly triggers poisoning these names. I don't think poisoning non identifiers likeSelfis ever useful.Use cases where
Selfis being poisoned:carbon-lang/toolchain/check/member_access.cpp
Line 62 in e257051
carbon-lang/toolchain/check/testdata/alias/no_prelude/fail_aliased_name_in_diag.carbon
Line 21 in e257051
Selfas a parameter type:carbon-lang/core/prelude/operators/arithmetic.carbon
Line 78 in e257051
Part of #4622.