mirror of
https://github.com/carbon-language/carbon-lang.git
synced 2026-11-15 11:38:33 +00:00
dependabot/github_actions/github-actions-major-8a185ec9c8
5
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Dana Jansens
|
9fb428d713 |
Ensure specific in ImplWitness has resolved definition before accessing witness table (#8040)
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. |
||
 Chandler Carruth
|
2c9739799b |
Render diagnostics through the terminal library (#7683)
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 |
||
|
Dana Jansens
|
24766e213e |
Defer evaluation of witnesses substituted while identifying a facet type (#7930)
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. |
||
 Geoff Romer
|
62f1cc6a35 |
Rename GenericClassType to ParameterizedClassType (#7953)
The old name was potentially confusing, because non-parameterized generic classes do not use this inst kind. |
||
 Richard Smith
|
947c1eb527 |
Add a Core.Array class type, and make array refer to it. (#7892)
As described in proposal #4682, make `array(T, N)` be syntactic sugar for the type `Core.Array` defined in the standard library. This provides a home for impls that are specific to `Core.Array`. Assisted-by: Gemini and Claude via Antigravity |