Rearrange name poisoning logic to do a little less work. #4766
Conversation
Insert the poison at the same time we do the name lookup to avoid doing two hash table lookups into each scope. This adds a bit of complication because import logic now needs to cope with importing a name that is already poisoned, but the complexity seems worthwhile to reduce the number of name lookups performed. This incidentally fixes a bug where we wouldn't poison any name scopes if we found the name in an enclosing lexical scope, leading to one extra diagnostic in existing tests.
There was a problem hiding this comment.
Thanks for doing this!
It's definitely more optimized.
I do not fully understand the behavior change though.
What I thought we were trying to do before is when lookup was successful, poison the name in all the scopes between where the name was found and where we started looking.
From the test change, it seems that now we poison a name even when the lookup is not successful.
Is this what we want?
See #4774
| EXPECT_THAT(name_scope.entries(), | ||
| ElementsAre(NameScopeEntryEquals( | ||
| NameScope::Entry({.name_id = poison1, | ||
| .inst_id = InstId::PoisonedName, | ||
| .access_kind = AccessKind::Public})))); | ||
|
|
||
| NameId poison2(++id); | ||
| name_scope.AddPoison(poison2); | ||
| EXPECT_EQ(name_scope.LookupOrPoison(poison2), std::nullopt); |
There was a problem hiding this comment.
I think we should add coverage for cases where LookupOrPoison() doesn't return std::nullopt.
| CARBON_CHECK(result.is_inserted(), "Failed to add required name: {0}", | ||
| name_entry.name_id); | ||
| if (!result.is_inserted()) { | ||
| // A required name can overwrite poison. |
There was a problem hiding this comment.
Is this tested in the unit tests?
|
Can you add some context for why #4622 is mentioned in the PR description? |
| // If `for_decl_name` is false, then this is a regular name lookup, and the | ||
| // name will be poisoned if not found so that later lookups will fail; a | ||
| // poisoned name will be treated as if it is not declared. Otherwise, this is | ||
| // a lookup for a name being declared, so the name will not be poisoned, but |
There was a problem hiding this comment.
"for_decl_name" making me stumble a little, partly with "is" versus "for". Noting the way you describe it here and how it's called, had you considered something like "is_in_decl" to match "LookupNameInDecl", or "is_declaring", something like that?
| // Adds a new name known to not exist. The new entry may not be poisoned. This | ||
| // is allowed even if the name has already been poisoned. |
There was a problem hiding this comment.
If the name was already poisoned, is the new entry still poisoned? That's particularly important to capture since we're looking at storing instructions for poisoned names, so "has instruction and is poisoned" is becoming a valid state. Maybe:
| // Adds a new name known to not exist. The new entry may not be poisoned. This | |
| // is allowed even if the name has already been poisoned. | |
| // Adds a new name known to not exist. The new entry won't be poisoned, and | |
| // can overwrite poisoned names. |
| // Searches for the given name and returns an EntryId if found or nullopt if | ||
| // not. If the name is not found, it will be poisoned so it can't be declared | ||
| // later. |
There was a problem hiding this comment.
Would it be worth phrasing this closer to how LookupOrAdd's comment is phrased? e.g.:
| // Searches for the given name and returns an EntryId if found or nullopt if | |
| // not. If the name is not found, it will be poisoned so it can't be declared | |
| // later. | |
| // If the given name already exists, return the EntryId; the entry might be | |
| // poisoned. Otherwise, poisons the name so that it can't be declared later | |
| // and returns nullopt. |
There was a problem hiding this comment.
(note, either way, I think it'd be good to have "if not found" behavior grouped instead of "or nullopt if not. If the name is not found,")
The main behavior change is that we now poison the name in the case where lookup is successful and finds a lexical result. Previously we only poisoned the name when lookup was successful and found a non-lexical result. Ultimately, the idea is that any time we look for a name in a declarative scope, other than when declaring it, it's an error if we don't find it and we later add it. So poisoning the name as part of the lookup seems like the right model. In the case of a failed lookup followed by a declaration, I do agree that the best thing would be to produce only a single error. Diagnosing both the failed lookup and the declaration of a poisoned name doesn't seem terrible to me -- it provides all the information that we have, which I think is probably a little more useful than diagnosing only the use of the undeclared identifier. Ideally I think I'd want us to produce a single "use of name before its declaration" error for the use that also points at where the name is later declared. That said, we can't really do that unless we produce diagnostics out of order (or modify the diagnostic after we initially emit it), and I'm nervous about building dependencies on diagnostic reordering given #3054, so we might need to think a bit about how to fit that into our diagnostic infrastructure. |
I've added it as this change seems to be part of name poisoning feature. |
Thanks for clarifying! Regarding the behavior change, I'm not sure I understand what is a lexical result vs. a non-lexical result here. |
Insert the poison at the same time we do the name lookup to avoid doing
two hash table lookups into each scope. This adds a bit of complication
because import logic now needs to cope with importing a name that is
already poisoned, but the complexity seems worthwhile to reduce the
number of name lookups performed.
This incidentally fixes a bug where we wouldn't poison any name scopes
if we found the name in an enclosing lexical scope, leading to one extra
diagnostic in existing tests.
Part of #4622