When impl lookup results in an `ImplWitness`, either directly or later
through `LookupImplWitness` evaluating to an `ImplWitness` inside some
specific, it represents a dependency on the eval blocks of the
`ImplDecl` that the `ImplWitness` comes from. The `ImplWitness` contains
a `SpecificId` of the `ImplDecl`, and the constants in the witness table
must be viewed through that `SpecificId`. The constants may depend on
the declaration or the definition block of the `ImplDecl`'s specific, so
any use of the witness table from `ImplWitness` requires the specific
from the `ImplWitness` to be fully resolved: both the decl and
definition blocks.
The specific declaration block is always resolved immediately when
forming a specific id, so there's nothing to do there. And things are
simple when the `ImplWitness` returned from impl lookup is concrete. We
would simply resolve the specific definition directly in the evaluation
of `LookupImplWitness`.
But when the `ImplWitness` is symbolic, meaning there's a dependency on
generic parameters in its referenced specific, then we have a dependency
on resolving the specific definition in the `ImplWitness` for every
specific that gets applied to the `ImplWitness`, eventually resulting in
a concrete specific.
Previously we attempted to do this by representing a dependency edge
from the eval block containing the `ImplWitness` instruction to the
definition block of the specific inside the `ImplWitness`. We
represented this dependency edge with the `RequireSpecificDefinition`
instruction.
The challenge with the `RequireSpecificDefinition` instruction was
ensuring it always landed in the _same eval block_ as the one holding
the symbolic `ImplWitness`. Ultimately, we've found a way to do this
without the `RequireSpecificDefinition`. The witness table is accessed
through `ImplWitnessAccess`, so we resolve the specific definition
before each access to the witness table. If the definition is already
resolved, this just costs a lookup of the Specific from the value store.
We document the other things we tried, which don't work or were too
complex, below.
In PR #6236, we added the `RequireSpecificDefinition` instruction in the
eval of `LookupImplWitness`, but this is too late. If impl lookup
resolved to a symbolic `LookupImplWitness`, the `LookupImplWitness` is
added some eval block. Later, we form specifics and re-evaluate that
eval block, and the `LookupImplWitness` can evaluate to an
`ImplWitness`. Adding the `RequireSpecificDefinition` instruction at
this point inserts the instruction into whatever generic's eval block
happens to be on the top of the generic stack, not the eval block of the
generic we are building a specific for. And indeed we can't add
instructions to a generic's eval block later like this, as it would be
missing from specifics that were already resolved for the generic. This
incorrect insertion shows up as a crash in eval at the CHECK introduced
but commented out in [eval.cpp in
#7981](https://github.com/carbon-language/carbon-lang/blob/56d9bbc40add1e9263f2f9b2555c3a27292d1725/toolchain/check/eval.cpp#L3507-L3512).
This is covered by the
`require_specific_impl_definition_through_specific_instantiation.carbon`
test.
We tried moving the insertion of `RequireSpecificDefinition` to be
earlier - at the point where the `LookupImplWitness` is constructed, in
the `LookupImplWitness()` function, instead of inside evaluation. This
seemed like a straightforwardly correct answer, as the eval block that
contains the `LookupImplWitness` will then also contain the
`RequireSpecificDefinition`. However, this can cause us to lose the
`RequireSpecificDefinition` entirely. Because it's a separate
instruction, not an operand of `LookupImplWitness`, it does not get
treated as a dependency of the witness. When we do an impl lookup inside
an `alias`, we build a generic for the alias and then discard it. This
discards the eval block containing the `LookupImplWitness`. Then, in an
enclosing generic, we refer to the constructed `AliasDecl` instruction,
which pulls all its dependent operands into the enclosing eval block.
This pulls in the `LookupImplWitness` into the enclosing eval block, but
it does not preserve the `RequireSpecificDefinition`. This is covered by
the
`require_specific_impl_definition_from_lookup_impl_witness_in_class_definition.carbon`
test.
We tried having `ImplWitness` evaluation resolve the specific definition
of the specific `ImplDecl` it refers to. This seems like the simplest
approach by far as we should always be working with the correct eval
block and nothing can get lost. However it does not work because of the
recursive nature of `ImplWitness`. The `ImplWitness` instruction appears
in the declaration of an `ImplDecl`, and the `ImplWitness` holds a
`SpecificId` for that same `ImplDecl`. When building a specific
`ImplDecl`, we evaluate the `ImplWitness` in the `ImplDecl`'s
declaration block which then immediately resolves the specific
definition before the declaration is finished. This leads to missing
instructions in the specific definition. In short, this puts the
dependency edge to the `ImplDecl`'s specific inside the `ImplDecl`
itself, which is problematic.
We tried wrapping `ImplWitness` in another instruction,
`ImplWitnessUse`. Inside the `ImplDecl` declaration there is still
`ImplWitness`, but then in the definition there is `ImplWitnessUse` and
that is what is returned from the evaluation of `LookupImplWitness`. The
`ImplWitnessUse` is an `ImplWitness` plus it ensures the specific
referred to by the witness has its definition resolved. However this
breaks constant comparisons, as we get different witnesses inside an
impl versus outside.
The first iteration of this PR's solution had us insert the
`RequireSpecificDefinition` into the eval block when we sniffed a
`LookupImplWitness` instruction, however that missed cases where impl
lookup immediately resolved to a symbolic `ImplWitness`, which is why we
now sniff for the `ImplWitness` in the eval block. This is covered by
the
`require_specific_impl_definition_from_symbolic_impl_in_class_definition.carbon`
test.
We also originally made a mistake of not inserting
`RequireSpecificDefinition` when inside the impl definition that the
`ImplWitness` refers to. But we can form specifics ImplWitness values
through function signatures, and these must add the dependency edge on
that specific ImplDecl definition. This is covered by the
`require_specific_impl_definition_from_inside_impl_definition.carbon`
test.
Finally, we did have a working solution. It was to sniff instructions
added to the eval block of a generic, and when an `ImplWitness` is added
to the eval block (it is implied that it is symbolic), also add a
`RequireSpecificDefinition` to that eval block immediately. We also
resolved the definition of a specific referred to by `ImplWitness` when
`LookupImplWitness` evaluates to `ImplWitness` to cover the concrete
witness case. This complicated the construction of the eval block
immensely, however. It caused a situation where adding one inst to the
eval block actually adds two, which we needed to avoid when importing.
And it required inst-kind specific behaviour in the very general
construction of an eval block, completely outside of eval itself.
There were a number of iterations on the design that weren't reflected
in the initial port to the terminal rendering library. This PR tries to
complete the rendering side of things to match the design we landed on:
code-focused, with messages attached by lines to the code, and condensed
as much as possible to avoid the vertical space.
It also improves the styling to make things more visually
distinguishable such as adding a `->` to the main error line, and making
the underlines and connectors for it have a heavy weight (when the font
supports that).
Another major change is having multiple messages on a single line be
drawn in source lexical order. This requires the connectors to cross
without joining, but makes it easier to read the messages.
This PR also has some refactorings to enable these improvements, and a
_number_ of improvements to comment prose and terminology to try and
avoid jargon and make things read more naturally. Sorry for the fact
that Claude left a bunch of this mannerly language in comments despite
my efforts, but I'm continuing to try and drive the prose towards
something direct and simple to read. Some of the terminology
improvements also needed to be made in the design doc and those have
been made here.
Note that I don't think this rendering is perfect or should be viewed as
final. I'm happy to iterate on it further, and have a few adjustments in
mind already, but would like to focus this on getting the
_implementation_ of the current rendering design in place, and then we
can make targeted changes or tweaks as needed. For example, I'm not yet
fully happy with the rendering when messages are too long and we move
them all the way to the left, but am leaving that for a follow-up.
Assisted-by: Claude Code
Diagnostic rendering was part of the data model: `Loc::FormatLocation`
and
`Loc::FormatSnippet` wrote plain text to a stream, so no capability,
color, or
terminal width could reach them.
A diagnostic is now one message with labels, contexts, and location
information
attached, and `Diagnostics::Renderer` draws it into a
`Terminal::Buffer`. The
design is in `toolchain/docs/diagnostics_rendering.md` (#7668),
authoring in
`toolchain/docs/diagnostics.md`.
Before:
```
example.carbon:6:3: error: 1 argument passed to function expecting 0 arguments
Run0(1);
^~~~~~~
example.carbon:3:1: note: calling function declared here
fn Run0() {}
^~~~~~~~~~~
```
After:
```
error: 1 argument passed to function expecting 0 arguments
╭─┤ example.carbon:6:3
┆
3 │ fn Run0() {}
· ─────┬─────
· ╰──┤ calling function declared here
┆
-> 6 │ Run0(1);
· ───────
╰────────────
```
`--no-diagnostic-snippets` gives a compact form, `example.carbon:6:3-9`.
File
tests use it; the rendering tests under `toolchain/diagnostics/testdata`
ask for
snippets.
C++ interop diagnostics are data now rather than text Clang drew:
`SemIR::ClangSourceLocId` names a `clang::SourceRange`, and a Clang
location is
marked as the point it names.
`StreamConsumer` moves to a `:renderer` target that `:emitter` does not
depend
on, keeping `common/terminal` out of `consumer.h`. Capabilities come
from the
file behind the error stream, so tests get plain ASCII.
`renderer_fuzzer` draws diagnostics built from fuzzer bytes with every
capability combination, since a shape the renderer doesn't expect must
never be
why the compiler dies while reporting a problem.
TODOs mark the follow-ups: line weights and crossing lines, wrapping
that keeps
a quoted type whole, and ordering diagnostics that share a byte offset.
Also: `IncorrectExtension`'s hint moves into its message, since its note
had no
location of its own, and `strip_noise` in `check_diagnostics.py` blanks
multi-line `#define`s instead of deleting them.
Assisted-by: Claude Code
When identifying a facet type, defer evaluation of LookupImplWitnesses
by redirecting them to evaluate into IdentifiedWitness. Then the caller
of identify will need to substitute the IdentifiedWitnesses, either with
witnesses of their own, or with LookupImplWitness to cause a lookup
operation.
This allows:
- An `impl as` on a named constraint can subst the witnesses in
designators from the named constraint with a witness to the impl itself.
This prevents them from looking for and finding some other overlapping
impl. (This PR)
- Impl lookup to collect rewrite/equality requirements from all named
constraints and then use the impls it has already found to evaluate any
designators in those requirements instead of performing impl lookup
again (creating a cycle). (Done in
https://github.com/carbon-language/carbon-lang/pull/7944)
- Similarly, an `impl as` can collect impls and equality requirements
from all named constraints and verify them correctly by replacing
witnesses that should refer to the impl with the correct witness instead
of performing a lookup and finding an overlapping impl. (Done in
https://github.com/carbon-language/carbon-lang/pull/7937)
- ImplWitnessAccess to be able to find rewrites from a named constraint,
not just from the initial DeclaredFacetType. (TBD)
To do this we need to introduce the concept of "identified" specifics.
In order to identify a facet type, we must construct a specific for each
require decl in a named constraint. These specifics will have
IdentifiedWitnesses inside them, but their args would otherwise match a
specific for the require decl constructed in another context. To avoid
polluting the canonical specific namespace, we introduce a second
namespace of specifics that are "identified" since they are created by
identifying.
As we have introduced a new global state that modifies eval behaviour,
we must disable that state when resolving a specific (as per
https://github.com/carbon-language/carbon-lang/pull/7923). But inside an
"identified" specific, we preserve the behaviour so that the witnesses
modified by identify (through SubstPeriodSelf or through evaluating a
require specific) have their evaluation deferred.
C++ interop used `GetFormatted` to turn a Carbon name into a C++ name.
For an identifier that is a Carbon keyword, such as `base`,
`GetFormatted` adds an `r#` prefix, so the C++ name came out as `r#base`
instead of `base`. This showed up in three places:
- A failed call to the C++ function `base` was reported as "no matching
function for call to 'r#base'".
- An exported Carbon function was mangled as `_ZN6CarbonL6r#baseEv`
instead of `_ZN6CarbonL4baseEv`.
- Its thunk was named `r#base__carbon_thunk` instead of
`base__carbon_thunk`.
All three now use `GetClangIdentifierInfo`, which `export.cpp` already
uses for other C++ names. The name is always an identifier in these
places: an overload set is only imported by looking up an identifier in
C++, and only functions declared in Carbon are exported, which are
always named by an identifier. So they check for an identifier, as the
export of a class name already does, rather than falling back to
`GetFormatted`. This removes a TODO about naming a `NameId::CppOperator`
overload set, which can't happen: C++ operators are resolved in
`operators.cpp` without an overload set.
Assisted-by: Claude Code
The type `type` is now a `FacetType` inst with no constraints. This
brings the model implemented in the toolchain into better alignment with
the language design. The `SemIR::TypeType` struct remains as a scope for
holding the `TypeInstId`, `ConstantId`, and `TypeId` constants, but is
not an `InstKind` anymore.
The `TypeType` inst looks a lot like singletons, but there are many
`FacetType` insts so it doesn't quite fit that model. So we put it
alongside singletons with a fixed inst id but refer to it as a more
general "builtin" inst that is not a singleton.
`Namespace::PackageInstId` is similar, and we group it with `TypeType`
conceptually as another builtin instruction with a fixed id.
No conversion is needed anymore to use a `type` as a facet, since types
also have a `FacetType` type. This simplifies and removes a number of
helpers and branches throughout the code.
The `TypeType` inst is now part of the constant store, so we end up
printing it in the constants block in every test. But it's also named
`type` rather than `%type` to preserve the majority of existing
formatting behaviour, though this does look different from other
constants.
Assisted-by: Opus 5 was used to generate a first draft and validate the
refactoring. Though nearly everything non-trivial the tool wrote has
been modified or rewritten.
Fix a bunch of cases where we use the same external name to mean
multiple different things in the same test. We've historically gotten
away with this, but under `--share-cpp-ast`, it becomes an error, at
least if the entity is either defined in, or used from, C++ code.
Assisted-by: Gemini via Antigravity (original change) and Claude Code
(suggested edits in review)
---------
Co-authored-by: Chandler Carruth <chandlerc@gmail.com>
Implementing interface modifiers causes an infinite loop when generating
fingerprints because the witness value generates a fingerprint that's
dependent on something dependent on the witness value. We've debugged
this to the witness table's `elements_id` field.
This hack is a workaround for creating a new block type whose value is
not codependent with its identity.
Co-authored-by: Richard Smith <richard@metafoo.co.uk>
Co-authored-by: Richard Smith <richard@metafoo.co.uk>
Instead of building one Clang `ASTContext` per compilation, the
`--share-cpp-ast` flag causes us to build a single `ASTContext` and
share it across all contexts. One new abstraction is added: `CppDomain`
represents the Carbon-side view of a Clang AST that might be shared
across multiple `SemIR::File`s. This object owns the Clang instance and
the AST.
For now, we have no isolation between the C++ state exposed to different
Carbon compilations, and we have no multiplexing of generated LLVM IR
from C++ into different Carbon compilations, so the mode is not usable
yet. The plan is to keep it behind a flag until it's ready.
Assisted-by: Gemini via Antigravity
This adds SemIR structs and implements building `observe` lists, as well
as naming, formatting, and importing `observe` declarations.
---------
Co-authored-by: Dana Jansens <danakj@orodu.net>
Fix import logic to make all imported packages be children of the
`NameScopeId::Package` scope. Previously, indirectly-imported packages
would end up as children of their importing package's scope, which
resulted in them not being treated as packages at all, and in particular
not being fingerprinted as packages.
Fixing that caused a failure in the fingerprinting logic as we started
to encounter packages with no correspoding import scopes. Instead of
looking for import scopes, use a simpler mechanism to map packages to
their package names, and clean up.
Unfortunately the latter change churns all the fingerprints again :(
Hopefully this is the last time for a while.
When we import from another library in the same package, its entities
end up with our library as their parent scope, resulting in cross-file
fingerprint mismatches. Instead, only include the library ID when
fingerprinting either a package-private entity or an `ImportIRId` that
refers to a particular `SemIR::File`.
Fixes link failures when referencing a symbol involving a fingerprint
from a different package.
Previously we included the `Namespace`'s `import_id` as part of its
fingerprint, which caused local and imported namespaces to get different
fingerprints. We now store the `import_id` on the `NameScope` instead of
on the `Namespace` inst to avoid this problem.
Also, when we reach a package-level `NameScopeId`, consistently
fingerprint it as a (package name, library name) pair. Previously the
fingerprinting depended on whether it was imported or not, as an
imported `NameScopeId` had a parent scope (the current package). We need
to include the library name here so that private entities with the same
name in different libraries have different fingerprints.
Add import/ and export/ under function/. Move most top-level tests to a
new basics/ with subdirectories for `import` directives and `inline
Cpp`. Add subdirectory for primitive type handling. Move all `reverse/`
tests to somewhere else, typically under an `export/` directory.
I split two test files up: constexpr.carbon got split into var/ and
function/ pieces, and reverse/simple.carbon was inlined into
namespace/export.carbon. The rest are just simple renames.
Dana Jansens
Chandler Carruth
Geoff Romer
Richard SmithandChandler Carruth
Christopher Di BellaandRichard Smith
Özgür T. ÖnsoyandDana Jansens