Skip to content
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.

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

Basic name poisoning support #4654

Merged
merged 49 commits into from
Dec 17, 2024
Merged

Basic name poisoning support #4654

merged 49 commits into from
Dec 17, 2024

Conversation

bricknerb
Copy link
Contributor

@bricknerb bricknerb commented Dec 9, 2024

#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.

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`.
@github-actions github-actions bot requested a review from jonmeow December 9, 2024 13:44
Copy link
Contributor

@jonmeow jonmeow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! This made me think a little more about what's needed to get a good diagnostic -- some comments inline, sorry I didn't think about that before.

toolchain/sem_ir/name_scope.h Outdated Show resolved Hide resolved
toolchain/sem_ir/name_scope.cpp Outdated Show resolved Hide resolved
toolchain/check/decl_name_stack.cpp Outdated Show resolved Hide resolved
toolchain/check/handle_class.cpp Outdated Show resolved Hide resolved
toolchain/check/context.cpp Outdated Show resolved Hide resolved
toolchain/check/context.cpp Outdated Show resolved Hide resolved
@bricknerb bricknerb requested a review from jonmeow December 10, 2024 10:29
toolchain/sem_ir/name_scope.h Outdated Show resolved Hide resolved
toolchain/sem_ir/name_scope.h Outdated Show resolved Hide resolved
toolchain/sem_ir/name_scope.h Outdated Show resolved Hide resolved
toolchain/sem_ir/name_scope.h Outdated Show resolved Hide resolved
@bricknerb bricknerb requested a review from jonmeow December 10, 2024 21:26
toolchain/check/context.cpp Outdated Show resolved Hide resolved
toolchain/check/context.cpp Outdated Show resolved Hide resolved
…ote to the place where the name is declared after it was poisoned.
@jonmeow jonmeow enabled auto-merge December 12, 2024 22:23
@bricknerb bricknerb requested a review from jonmeow December 13, 2024 05:46
@jonmeow
Copy link
Contributor

jonmeow commented Dec 13, 2024

Can you take a look at the test failures?

zygoloid and others added 17 commits December 16, 2024 10:25
…age#4657)

For example, format the `ImplicitAs` interface as `Core.ImplicitAs`
rather than simply `ImplicitAs`.

When importing an entity in a namespace, also import a declaration of
the enclosing namespace if necessary so that we can determine its name.
This is essentially the result of looking at `.begin()` uses. We also
frequently do `std::shuffle`, but unfortunately STLExtras doesn't
provide a wrapper for that.
Found by fuzzer.

---------

Co-authored-by: Jon Ross-Perkins <jperkins@google.com>
…phization fails. (carbon-language#4662)

Also fix a bug in `Context::GetClassType` that previously tried to
complete the class type before returning it. That's not correct --
`GetCompleteTypeImpl` is only appropriate for cases where the type can
trivially be completed and completing it can't fail -- and led to
infinite recursion with this change because we would call `GetClassType`
when producing a diagnostic if completing that class type failed.
…oCompleteType`. (carbon-language#4670)

Almost all callers actually could never fail and nearly all of those
already `CHECK`-failed on failure. Add a new overload for that case, and
add a location parameter for the one remaining call.
- Provide `Check::Dump(context, arg)` and similar.
- gdb and lldb should do contextual lookup, and `call Dump(*this,
Lex::TokenIndex::Invalid)` has been tested with gdb.
- Since this is only for debug, keeps the functions fully separated from
code.
- Uses alwayslink to ensure objects are correctly linked, even though
there are no calls.
- `-Wno-missing-prototypes` is needed when we don't have forward
declarations.
- Code is not linked in opt builds, using `#ifndef NDEBUG`.
- This probably could be doing something in BUILD files with a
`select()`, but the `#ifndef` seemed easier.

This is based on carbon-language#4620, but uses free functions instead of member
functions.

Co-authored-by: Dana Jansens <danakj@orodu.net>

---------

Co-authored-by: danakj <danakj@orodu.net>
…-language#4677)

This is so that diagnostics which lack a location get split file
clustering.
Given code like the following:
```
auto kind = ConversionTarget::Kind{0};
CARBON_CHECK(!loc_id.is_valid(), "hello {0} world", kind);
```

Currently we would print 'hello <the next line>', as the check string
would be treated as terminating at the '{0}', so it does not print the
rest of the string or a newline. This is because ConversionTarget::Kind
is an enum with underlying type `int8_t` which is a char, and
llvm::formatv does not look if the type is an enum and treat is
specially. So it prints it as a char rather than a number, which in this
case is a nul terminator.

With this change, the '{0}' value will be converted to a larger integer
before being passed through to llvm::formatv so that char-sized enums
will print as a number, and the result is that we will print 'hello 0
world\n' as the developer intended.
If the user's fork is not named 'origin' then the script will fail and
needs to know the user's remote name.

Fixes carbon-language#1899
The call got misplaced during refactoring.
…4686)

Such indexing operations are created by array initialization. Since we
switched integer literals to be of type IntLiteral we've been attempting
to index arrays with the (empty) representation of an IntLiteral rather
than with an actual integer value.
auto-merge was automatically disabled December 16, 2024 09:25

Head branch was pushed to by a user without write access

@bricknerb
Copy link
Contributor Author

It's probably only failing after merging with trunk.

Thanks!
This makes sense, just wasn't aware this is what it does.
I think I've fixed it, but I can probably not rerun it myself.

@jonmeow jonmeow enabled auto-merge December 16, 2024 19:04
@jonmeow
Copy link
Contributor

jonmeow commented Dec 17, 2024

I see this failed on another merge conflict. I've merged to try to get it in now. The contributor access should help avoid this repeating, though; I encourage resolving conflicts & merging after approval, now that you can. :)

@jonmeow jonmeow added this pull request to the merge queue Dec 17, 2024
@jonmeow
Copy link
Contributor

jonmeow commented Dec 17, 2024

And maybe I just missed that I hadn't approved...

Merged via the queue into carbon-language:trunk with commit 9c8773d Dec 17, 2024
8 checks passed
@bricknerb
Copy link
Contributor Author

Thanks for the merge Jon!
Indeed I was waiting for an approval. :)

@bricknerb bricknerb deleted the poison4 branch December 19, 2024 14:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants