Skip to content

Name Poisoning #4622

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
bricknerb opened this issue Dec 4, 2024 · 2 comments
Open

Name Poisoning #4622

bricknerb opened this issue Dec 4, 2024 · 2 comments
Labels
long term issue Issues expected to take over 90 days to resolve. Does not apply to PRs.

Comments

@bricknerb
Copy link
Contributor

bricknerb commented Dec 4, 2024 •
edited
Loading

This was referenced Dec 4, 2024
Merged
Merged
github-merge-queue bot pushed a commit that referenced this issue Dec 5, 2024
@bricknerb
In indirect_import_member test, make the alias avoid name poisoning (#…
1409666 
…4635)

This is a preparation change for introducing name poisoning
(#4622), which
would have broken this test.
bricknerb added a commit to bricknerb/carbon-lang that referenced this issue Dec 6, 2024
@bricknerb
Basic name poisoning support
69b7dcc 
carbon-language#4622
When using an unqualified name disallow using that name in all scopes that would make it ambiguous in retrospect.
Doesn't include support for poisoning when importing (see new test for that with TODO).
Implemented by introduce InstId::PoisonedName and entries with it to NameScope.
@bricknerb bricknerb mentioned this issue Dec 6, 2024
Closed
github-merge-queue bot pushed a commit that referenced this issue Dec 6, 2024
@bricknerb @jonmeow
[NFC] Convert NameScope from struct to class (#4623)
daba2c7 
This is a preparation change for adding name poisoning support
(#4622), which is
expected to require more elaborate logic around NameScope since a name
can be not defined yet, defined, or poisoned.

The API separates looking up a name from getting the full entry since we
have cases where the entries are invalidated between the time we're
looking for the name and when we access (and sometimes modify) the
entry.

This change has the following benefits:
* `names` and `name_map` are internal to `NameScope` and are guaranteed
to match.
* `extended_scopes` and `import_ir_scopes` can not be manipulated (only
new scopes can be added).
* `inst_id`, `name_id` and `parent_scope_id` are constants.
* `has_error` can only be mutated from false to true.

---------

Co-authored-by: jonmeow <[email protected]>
bricknerb added a commit to bricknerb/carbon-lang that referenced this issue Dec 9, 2024
@bricknerb
Basic name poisoning support
24c0d19 
carbon-language#4622
When using an unqualified name disallow using that name in all scopes that would make it ambiguous in retrospect.
Doesn't include support for poisoning when importing (see new test for that with TODO).
Implemented by introduce `InstId::PoisonedName` and entries with it to `NameScope`.
@bricknerb bricknerb mentioned this issue Dec 9, 2024
Merged
@bricknerb bricknerb self-assigned this Dec 17, 2024
github-merge-queue bot pushed a commit that referenced this issue Dec 17, 2024
@bricknerb @danakj
@zygoloid @geoffromer @jonmeow @josh11b @chandlerc @CarbonInfraBot
Basic name poisoning support (#4654)
9c8773d 
#4622
When using an unqualified name, disallow declaring that name in all
scopes that would make it ambiguous in retrospect.
Doesn't include support for poisoning in `impl library` (see new test
for that with TODO).
Implemented by introduce `InstId::PoisonedName` and entries with it to
`NameScope`.

---------

Co-authored-by: Dana Jansens <[email protected]>
Co-authored-by: Richard Smith <[email protected]>
Co-authored-by: Geoff Romer <[email protected]>
Co-authored-by: Jon Ross-Perkins <[email protected]>
Co-authored-by: josh11b <[email protected]>
Co-authored-by: josh11b <[email protected]>
Co-authored-by: Chandler Carruth <[email protected]>
Co-authored-by: Carbon Infra Bot <[email protected]>
bricknerb added a commit to bricknerb/carbon-lang that referenced this issue Jan 3, 2025
@bricknerb
Do not try to recover from using impl outside class error
8c572f4 
This prevents crashing when the wrongly used impl uses a poisoned name.
carbon-language#4622
@bricknerb bricknerb mentioned this issue Jan 3, 2025
Merged
github-merge-queue bot pushed a commit that referenced this issue Jan 3, 2025
@bricknerb
Do not try to recover from using impl outside class error (#4755)
72e594d 
This prevents crashing when the wrongly used `impl` uses a poisoned
name.
#4622
bricknerb added a commit to bricknerb/carbon-lang that referenced this issue Jan 7, 2025
@bricknerb
[NFC] Change name poisoning implementation to use an explicit `is_poi…
a138aed 
…soned` bit instead of `InstId::PoisonedName` value. This would allow to more easily change the API to support accessing the poisoning delcaration so we can have better name poisoning diagnosis.

carbon-language#4622
@bricknerb bricknerb mentioned this issue Jan 7, 2025
Merged
github-merge-queue bot pushed a commit that referenced this issue Jan 8, 2025
@bricknerb
Change name poisoning implementation to allow better diagnostics (#4764)
74395ce 
Change the implementation to use an explicit `is_poisoned` bit instead
of `InstId::PoisonedName` value.
Zero behavior change.
This would allow to more easily change the API to support accessing the
poisoning declaration so we can have better name poisoning diagnosis.
#4622
@bricknerb bricknerb mentioned this issue Jan 8, 2025
Merged
bricknerb added a commit to bricknerb/carbon-lang that referenced this issue Jan 8, 2025
@bricknerb
Add a test that shows names are not poisoned when lookup fails
c893671 
carbon-language#4622
@bricknerb bricknerb mentioned this issue Jan 8, 2025
Merged
bricknerb added a commit to bricknerb/carbon-lang that referenced this issue Jan 8, 2025
@bricknerb
Add a test that shows names are not poisoned when lookup fails
0ee5ed5 
carbon-language#4622
github-merge-queue bot pushed a commit that referenced this issue Jan 8, 2025
@bricknerb
Add a test that shows names are not poisoned when lookup fails (#4774)
7d92aa7 
#4622
github-merge-queue bot pushed a commit that referenced this issue Jan 14, 2025
@zygoloid
Rearrange name poisoning logic to do a little less work. (#4766)
6bc36b0 
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
bricknerb added a commit to bricknerb/carbon-lang that referenced this issue Jan 21, 2025
@bricknerb
When adding a namespace, explicit unpoison an optimistically poisoned…
20013db 
… name

Part of carbon-language#4622
@bricknerb bricknerb mentioned this issue Jan 21, 2025
Merged
bricknerb added a commit to bricknerb/carbon-lang that referenced this issue Jan 21, 2025
@bricknerb
Remove comment about inst_id not being poisoned
e2b14c9 
`InstId` cannot be poisoned since carbon-language#4764.
Part of carbon-language#4622.
@bricknerb bricknerb mentioned this issue Jan 21, 2025
Merged
github-merge-queue bot pushed a commit that referenced this issue Jan 21, 2025
@bricknerb
When adding a namespace, explicitly unpoison an optimistically poison…
c304e73 
…ed name (#4826)

Part of #4622
github-merge-queue bot pushed a commit that referenced this issue Jan 21, 2025
@bricknerb
Remove comment about inst_id not being poisoned (#4827)
6636baf 
`InstId` cannot be poisoned since #4764.
Part of #4622.
dwblaikie pushed a commit to dwblaikie/carbon-lang that referenced this issue Jan 21, 2025
@bricknerb @dwblaikie
When adding a namespace, explicitly unpoison an optimistically poison…
72e7f65 
…ed name (carbon-language#4826)

Part of carbon-language#4622
dwblaikie pushed a commit to dwblaikie/carbon-lang that referenced this issue Jan 21, 2025
@bricknerb @dwblaikie
Remove comment about inst_id not being poisoned (carbon-language#4827)
874d703 
`InstId` cannot be poisoned since carbon-language#4764.
Part of carbon-language#4622.
github-merge-queue bot pushed a commit that referenced this issue Feb 14, 2025
@bricknerb
Fix bad merge of comments introduced in #4884 (#4955)
809bcf1 
Part of #4622.
github-merge-queue bot pushed a commit that referenced this issue Feb 14, 2025
@bricknerb
Add poisoned names to the format (#4961)
986a2a0 
Part of #4622.
github-merge-queue bot pushed a commit that referenced this issue Feb 14, 2025
@bricknerb
Add a test for the case that extend poisons a class member (#4960)
d65f0d9 
Part of #4622.
github-merge-queue bot pushed a commit that referenced this issue Feb 14, 2025
@bricknerb
Add the used name to the NameUseBeforeDecl diagnostic (#4901)
4a93b66 
Part of #4622.
@bricknerb bricknerb mentioned this issue Feb 20, 2025
Merged
github-merge-queue bot pushed a commit that referenced this issue Feb 24, 2025
@bricknerb
Refactor name poisoning tests to be more organized, complete and cons…
5b67bb8 
…istent (#4987)

This is also following
#4900 (comment),
which points that name poisoning tests are not in the correct place.
Part of #4622.
bricknerb added a commit to bricknerb/carbon-lang that referenced this issue Feb 25, 2025
@bricknerb
Use Generics in no_poison test instead of pointers.
45bcea1 
Followup of carbon-language#4987.
Part of carbon-language#4622.
bricknerb added a commit to bricknerb/carbon-lang that referenced this issue Feb 25, 2025
@bricknerb
Use Generics in no_poison test instead of pointers.
b4c5453 
Followup of carbon-language#4987.
Part of carbon-language#4622.
@bricknerb bricknerb mentioned this issue Feb 25, 2025
Merged
github-merge-queue bot pushed a commit that referenced this issue Feb 26, 2025
@bricknerb
Use Generics in no_poison test instead of pointers. (#5011)
3573763 
Followup of [#4987
comment](https://github.com/carbon-language/carbon-lang/pull/4987/files/b015f99d0e86f5dfe3b1709bec8a426a584f7804#r1964119857).
Part of #4622.
bricknerb added a commit to bricknerb/carbon-lang that referenced this issue Feb 27, 2025
@bricknerb
Split impl/no_prelude/name_poisoning.carbon to `interface/no_prelud…
c95d43e 
…e/name_poisoning.carbon` and move `interface` tests there

Part of carbon-language#4622.
@bricknerb bricknerb mentioned this issue Feb 27, 2025
Merged
bricknerb added a commit to bricknerb/carbon-lang that referenced this issue Feb 27, 2025
@bricknerb
Avoid copying NameScope and only allow moving it
21b1443 
This class is not intended to be copied.
Part of carbon-language#4622.
@bricknerb bricknerb mentioned this issue Feb 27, 2025
Merged
github-merge-queue bot pushed a commit that referenced this issue Feb 27, 2025
@bricknerb
Avoid copying NameScope and only allow moving it (#5032)
80e1a6e 
This class is not intended to be copied.
Part of #4622.
github-merge-queue bot pushed a commit that referenced this issue Feb 28, 2025
@bricknerb
Split impl/no_prelude/name_poisoning.carbon to `interface/no_prelud…
43b9969 
…e/name_poisoning.carbon` and move `interface` tests there (#5031)

See
#4950 (comment).
Part of #4622.
github-merge-queue bot pushed a commit that referenced this issue Feb 28, 2025
@bricknerb
Add a test for the case that impl function is poisoned (#4950)
fc5dcfe 
This adds missing coverage.
Part of #4622.
bricknerb added a commit to bricknerb/carbon-lang that referenced this issue Mar 3, 2025
@bricknerb
Add --no-dump-sem-ir to all name_poisoning tests
f701e86 
Follow up of previous PR discussions:
* https://github.com/carbon-language/carbon-lang/pull/4950/files/89c2e66dc3190159e2f8d94c31bff31bdd0d81a1..a1650a7d73c8f4013b4b77e4d7d60933f1f6d676#r1972257889
* https://github.com/carbon-language/carbon-lang/pull/4987/files#r1964105413

Part of carbon-language#4622.
@bricknerb bricknerb mentioned this issue Mar 3, 2025
Merged
github-merge-queue bot pushed a commit that referenced this issue Mar 3, 2025
@bricknerb
Add --no-dump-sem-ir to all name_poisoning tests (#5053)
28de6c9 
Follow up of previous PR discussions
([#4950](https://github.com/carbon-language/carbon-lang/pull/4950/files/89c2e66dc3190159e2f8d94c31bff31bdd0d81a1..a1650a7d73c8f4013b4b77e4d7d60933f1f6d676#r1972257889),
[#4987](https://github.com/carbon-language/carbon-lang/pull/4987/files#r1964105413)).
Part of #4622.
@bricknerb bricknerb mentioned this issue Mar 5, 2025
Merged
github-merge-queue bot pushed a commit that referenced this issue Mar 6, 2025
@bricknerb
Add a test that should catch checking poisoned names when extending a…
d25140e 
… class (#5071)

See
https://discord.com/channels/655572317891461132/655578254970716160/1346727881912619081

Part of #4622.
@github-actions GitHub Actions
Copy link

We triage inactive PRs and issues in order to make it easier to find active work. If this issue should remain active or becomes active again, please comment or remove the inactive label. The long term issue label can also be added for issues which are expected to take time.

This issue is labeled inactive because the last activity was over 90 days ago.

@github-actions github-actions bot added the inactive Issues and PRs which have been inactive for at least 90 days. label Mar 18, 2025
@bricknerb bricknerb added long term issue Issues expected to take over 90 days to resolve. Does not apply to PRs. and removed inactive Issues and PRs which have been inactive for at least 90 days. labels Mar 18, 2025
bricknerb added a commit to bricknerb/carbon-lang that referenced this issue Apr 29, 2025
@bricknerb
When poisoning a name, keep the triggering instruction instead of the…
0327a6d 
… triggering location

This will allow us to provide better diagnostics.
`Destroy` is a special case (in `LookupMemberNameInScope()`) because we only add it if lookup is fails finding the name.

Part of carbon-language#4622.
@bricknerb bricknerb mentioned this issue Apr 29, 2025
Draft
@bricknerb bricknerb removed their assignment May 2, 2025
@bricknerb
Copy link
Contributor Author

Unassigning myself since diagnostic quality is not a priority right now, in case someone would like to look into the improvements necessary here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
long term issue Issues expected to take over 90 days to resolve. Does not apply to PRs.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
1 participant
@bricknerb