When deducing arguments for generic parameters of an `impl`, the
deduction calls `Convert` on the input arguments. Often, the input
argument is a facet, and needs to be converted to a type via
FacetAccessType in order to produce a different facet. These
instructions end up being added to the semir, but only their constant
values are needed for the resulting specific returned from Deduce.
In the best case, these extra instructions are just noise in the semir,
or they just cause instruction names to get differentiated with larger
suffixes.
In the worst case, these extra instructions contain references to
instructions from a generic context, and leak them out of that generic
context and into another. In particular, when importing a
LookupImplWitness instruction, the re-evaluation of it can do deduce
(when the lookup is against a generic `impl`). The instructions created
in Deduce are not part of the import, and end up referring to imported
instructions from the local context, which leads to confusion in the
toolchain, and can crash.
The `import_self_specific.carbon` test demonstrates this. It causes the
`I.F` function to be imported from the `I` interface when building the
witness table for the `impl`. Doing so imports the specific of `C` which
includes a LookupImplWitness for `Self.Accoc` in `I`. The `Self` is a
BindSymbolicName with generic binding index 0, in `I`. When Convert
creates instructions in the generic `impl forall D`, however, they end
up referencing and including this BindSymbolicName into its eval block.
But the generic binding 0 in the `impl` is a very different thing (a
value of type `E`). This confusion leads to crashes.
When deducing an argument against a type that is `<facet value> as type`
we don't care about the `as type` part of that expression. We want to
find an argument that can convert to the `FacetType` of the facet value
for the generic binding that is the `<facet value>`.
This was done after-the-fact in the Deduce switch, but we move this
canonicalization step to be more explicit and done up front at the start
of the Deduce loop. This:
- Avoids a trip through the Deduce loop for a `FacetAccessType`
parameter, just to deduce through it in the switch, which avoids convert
and creation of extraneous constant values.
- Uses the `GetCanonicalFacetOrTypeValue()` function so that when we add
`SymbolicBindingType` handling to that function it will apply to Deduce
as well correctly, instead of needing to handle both in the switch.
`DeduceImplArguments()` works with the argument as a constant value, but
starts with the non-canonical `impl.self_id`. As these are both derived
from `impl.self_id` it just means an extra trip through the work loop to
try again with the canonical `impl.self_id` as the param. Save that work
and give canonical instructions for both param and arg in
`DeduceImplArguments()`.
If convert fails after applying a substitution in deduce, fail deduction
rather than succeeding deduction with an ErrorInst in the deduced
argument.
For example, in
`toolchain/check/testdata/facet/fail_convert_class_type_to_generic_facet_value.carbon`
the `WrongGenericParam` is deduced for the first argument, then
substituted into the second parameter, but the argument can not convert
to the parameter after substitution. In this case, deduction fails
instead of producig a call with an error in the second argument. The
resulting semir drops the call with an error argument:
```
-// CHECK:STDOUT: %CallGenericMethod.specific_fn: <specific function> = specific_function %CallGenericMethod.ref, @CallGenericMethod(constants.%WrongGenericParam, <error>) [concrete = <error>]
-// CHECK:STDOUT: %CallGenericMethod.call: init %empty_tuple.type = call %CallGenericMethod.specific_fn() [concrete = <error>]
```
When deduce determines the type of its parameters, it converts each
argument to the correct type, generating an error if the argument can't
convert. It is at this point that enclosing specifics are applied to
arguments as well, as the enclosing specifics are stored in
`substitutions_`. However we were only doing this step for concrete
argument values. If the argument is a symbolic value, like a facet value
that has an enclosing specific, we failed to apply that enclosing
specific. Then the unconverted argument (without the enclosing specific
applied) would end up failing to convert to the parameter type even
though deduce found that it should.
Also fix substitution into constants to provide a source location. This
matters if the result of substitution ends up being part of a generic
eval block.
---------
Co-authored-by: Dana Jansens <danakj@orodu.net>
Co-authored-by: Jon Ross-Perkins <jperkins@google.com>
Undo changes that were meant to prevent use of a reference into
`ValueStore` after being invalidated. After #5576, the `ValueStore`
makes such references stable, so there's no need to worry about
invalidation.
This is fixing a deduce crash, with regression tests added in
binding_pattern.carbon. In `needs_substitution`, it adds the
`BindSymbolicName` with the compile time bind index corresponding to the
wrong generic scope, which causes a bad result. It appears
`needs_substitution` logic is no longer needed (per zygoloid,
`CheckDeductionIsComplete` handles related issues) so can be removed.
This changes the order of IR in use_assoc_const.carbon but the result
appears equivalent to me.
This was a fuzzer-found crash.
We frequently want to operate on singletons. Per discussion, drop
`Singleton` to make the code shorter.
This started off as wanting to write `inst_id.is_error()`, but the
dependency relationship between ids.h and singleton_insts.h would
require some kind of delayed evaluation to allow the implementation to
remain in headers (which I suspect is helpful to have for inlining). I
could have added something like `IsErrorInst`, forward declared in ids.h
and defined in singleton_insts.h (which would always be included by
typed_insts.h), but the template approach felt like a decent balance
between (a) removing the boilerplate `::SingletonInstId`, (b)
understandability, (c) still visually mirroring if we immediately return
a singleton, and (d) flexibility for more than just `ErrorInst`. But TBH
I'd probably still have written `is_error()` if it didn't require
addressing the cross-header cycle.
Then I tried `SemIR::InstId::Is<SemIR::ErrorInst>`, which generally
worked with types but generated the complaint that it didn't shorten
*all* singleton uses. So pulling back on `::Is`, and instead just
dropping `Singleton`.
TypeInstId is an InstId whose constant value has a type of TypeType.
This includes:
- Type value instructions, the `ClassType` or `IntLiteralType`
instructions.
- Constraint value instructions, which are the `FacetType` and
`TypeType` instructions, each of which also have type TypeType.
TypeInstId encodes in the type system that it is safe to convert the
instruction's value to a TypeId, and CHECKs at construction that this
invariant is maintained.
---------
Co-authored-by: Richard Smith <richard@metafoo.co.uk>
In preparation for shifting from `TypeId`s potentially representing
attached types to always representing unattached types, using
[terminology suggested on
Discord](https://discord.com/channels/655572317891461132/963846118964350976/1359286326779973712).
This change causes us to track slightly more type spelling information
through SemIR.
One change that has significant impact on the SemIR output is that we
now build a `struct_type` instruction in each class representing the
types of the fields, including the spelling used for those types. This
is now no longer always identical to the corresponding canonical
`struct_type` for the object representation, so it's built separately
and owned by the class.
Also remove `TypeBlock` support entirely, as its only use was
representing `TupleType`s, which now use an `InstBlock`.
This builds on #5212 which is adding ArgAndKind. This further modifies
CARBON_KIND_SWITCH support so that we can use it with ArgAndKind in
addition to Inst. That creates a quirk where it's easier if ArgAndKind
provides `kind` as an accessor instead of a data member, so I'm just
switching it to a class.
#5171 ran into an issue where the wrong kind was associated with an arg
(`auto arg1 = RefineOperand(context, loc_id, arg0_kind,
action.arg1());`). This PR is trying to reduce risk of similar errors by
replaced `ArgKinds()` with instead an `ArgAndKind` structure and
corresponding accessors.
A couple things I considered and discarded were:
- Adding `CARBON_KIND_SWITCH` support (in this PR -- see #5216).
- The particular way that `ForCase` works would need to change, and I
was hesitant to do that here.
- But this is why I did add `As` to `ArgAndKind`, because it had me
thinking in that direction.
- Trying to make wrapper functions like `MutateArgs(callback_fn);`. This
kind of approach gets a little messy due to some of the conditional
passes, and in particular the reverse-iteration done for `PopOperand` in
subst.cpp
- Making something like `args_and_kinds() -> std::array<ArgAndKind, 2>`.
There's one spot where iteration is already set up as a loop, but for
others it felt a little convoluted with less gain than
`MutateArgs`-style things.
I'm not sure if there's a better way to set up the table generators, I
might keep tinkering with those for ideas.
What this really does is avoids shadowing names, so that we can
comfortable have things like `Check::DiagnosticEmitter` or
`Check::DiagnosticLoc` without shadowing being a concern.
Note, down this path I'm also thinking about:
- Renaming misc DiagnosticConsumer/DiagnosticEmitter classes, possibly
just to DiagnosticConsumer/DiagnosticEmitter (so
`Check::DiagnosticEmitter` instead of `SemIRLocDiagnosticEmitter`).
- Dropping `Diagnostic` from `Emitter::DiagnosticBuilder`.
- But not for `Check::DiagnosticBuilder`, because `Check::Builder` would
be ambiguous.
- Renaming diagnostics/diagnostic_* to drop "diagnostic".
[Discussion about SemIRLoc ->
DiagnosticLoc](https://discord.com/channels/655572317891461132/655578254970716160/1353771570463768698)
reminded me of this (in particular the older [Check::DiagnosticBuilder
discussion](https://discord.com/channels/655572317891461132/655578254970716160/1344363562608627763)),
but I'd only do that rename if there's matching consensus about a path
forward where we keep SemIRLoc, and in a way that it's only ever used
for diagnostics (the divergence from which is at the root of current
LocId discussion).
I'm trying to keep that separate from a namespace addition for clarity.
Deduction can do conversion, and conversion can import impls from the
Core package. If you have the right number of impls in your ImplStore at
that moment, it will reallocate and any pointer into context.impls()
will be invalidated.
In particular, in impl lookup, we currentl loop over context.impls() and
do deduction on each impl. So this can break the for loop. Additionally,
we pass around a reference to the currently-being-looked-at Impl, which
becomes invalidated.
This is very challenging to test in any reliable way as you need a
specific number of impls in your ImplStore. I hit it when making changes
to a test in the middle of a bunch of file splits. Putting the same test
in its own file did not trigger the issue. It was caught by ASAN, which
showed:
- The memory was allocated by SmallVector in handle_impl when making the
Impl.
- The memory was freed by SmallVector reallocating in import_ref.cpp
- The memory was accessed when reading through the `impl` reference in
FindWitnessInImpls(). I was able to reproduce by printing the
`impl.interface.interface_id` after the call to GetWitnessIdForImpl()
which does the deduction.
I didn't save the ASAN stack and now I can't find the exact permutation
of the test file that caused it to occur in order to reproduce. :(
To avoid the UAF we stop passing around the Impl reference, and pass
around either the ImplId, or values from the Impl. To avoid copying the
entirety of the impl ids in context.impls() into a separate container in
order to iterate safely, we move the early outs from
GetWitnessIdForImpl() up to the caller where it can use them to reduce
the number impl ids that we iterate over. Type structures will be able
to further reduce the size of this set.
When deduction has to substitute binding parameters into further generic
parameters, we do conversion of the argument to the substituted type.
Then we replace the argument instruction id with that Converted
instruction. This causes a redundant specific to be created for the
Converted instruction which is not needed. What we want is the specific
for its constant value.
So when we replace the argument instruction id, replace it with the
instruction from the constant value of the converted argument.
This was raised in [discord
#toolchain](https://discord.com/channels/655572317891461132/655578254970716160/1349067541070217306).
If the query facet type has more than one interface, we must find an
impl that provides that interface for the query type for each interface.
This just looks like a for loop over the interfaces and ensuring we
found one impl witness for every one.
However the impl matching must change since it can't look at the
constant value of the entire query facet type for comparison with the
impl, as that query facet type may be for multiple interfaces and we are
looking to match an impl of a single interface.
To do this we break the query facet type up into each interface and make
sure the interface ids match. Then ensure that the impl was able to
deduce any generic parameters using the specific of the single query
interface.
There are some TODOs left here:
1. If the facet type for the query or the impl constraint has
"other_requirements" then we can't verify that they match since they are
lost. We fall back to comparing the constant id of the query to the
impl's constraint (after deducing generics in the impl). This correctly
eliminates mismatches but eagerly eliminates impls that could match the
query interface as well when there's more than one interface in the
query.
2. We don't return a witness for every interface in the query facet
type. Since we can't demonstrate any use of the witness there yet, for
cases that can have more than one interface in the query facet type,
this doesn't break anything that was previously working. The return
value is currently treated as a bool for cases with multiple interfaces
in the facet type (as a test for "can this be converted") but the
converted-to facet value's witnesses are unused.
---------
Co-authored-by: josh11b <15258583+josh11b@users.noreply.github.com>
When finding the binding entity name, always go through the binding
instruction's constant value to get a canonical instruction which will
always have an entity name attached to it.
Currently the only instructions in this position without an entity name
are ImportRefLoaded. But other indirect instructions may exist in the
future, which evaluate to an AnyBindName but are not themselves one. So
this makes the code more robust to change in the future.
An imported generic has bindings which are of type ImportRefLoaded, and
if they come from another package, they have no entity name attached to
them.
Since a binding name is always a constant-time value, we can get the
constant value instruction for the imported instruction to get a
canonical non-imported instruction. And that one will have a local
`NameId`.
---------
Co-authored-by: josh11b <15258583+josh11b@users.noreply.github.com>
There aren't remaining uses on `Context` other than `DiagnosticEmitter`
itself. I'm adding `SemIRLoc` because I feel odd about having both
`Carbon::Check::DiagnosticBuilder` and
`Carbon::DiagnosticEmitter<T>::DiagnosticBuilder`, but it seems
relatively little additional typing outside the handful of
`DiagnosticEmitter` uses on `Context` itself:
```
Context::DiagnosticEmitter
DiagnosticEmitter<SemIRLoc>
Context::DiagnosticBuilder
SemIRLocDiagnosticBuilder
Context::BuildDiagnosticFn
BuildSemIRLocDiagnosticFn
```
Also clean up #include's while I'm finishing here.
This creates a new check/type.h for most logic, and also moves some
functions to TypeStore in sem_ir/type.h. My approach for TypeStore is to
focus on moving the read-only functions there.
Change parse tree from `template (T:! type)` to `(template T):! type`,
so that we have information about whether a binding is a template
binding available when forming the representation of the binding
pattern. This incidentally fixes a bug that we would accept `template
addr A:! B` instead of the intended `addr template A:! B`.
Track whether a symbolic binding is a template binding on the
`EntityName` object. I'm borrowing a bit from the `CompileTimeBindIndex`
for this in order to avoid making `EntityName`s larger. Longer-term, we
should think about using a different representation for symbolic
bindings, to avoid including these fields in all `EntityName`s, but
that's out of scope for this change.
So far, template bindings are treated as having the same phase as
checked bindings, but that will change in a future PR.
Also makes the style guide explicitly comment on void, but this was the
intent IIRC because it matches Carbon's `-> ()` (and "always" versus
"except for void", which we definitely went back and forth on).
Includes adjusting function pointers, which I definitely forget this
syntax works sometimes.
Excludes utils/tree_sitter/src/scanner.c because it claims to be C, but
really we should probably fix that to be cpp.
The fn receiving a facet type needs to deduce a type from a
FacetAccessType, which is the SemIR type representing the parameter type
that is a generic parameter. For example:
```
fn F[T: Interface](val: T);
```
Here T is a generic parameter that is a facet value, but the `val`
parameter's type is the facet value converted from a FacetType to a
TypeType, with `as type`. The result of that conversion is a
FacetAccessType. So the deduction code sees a FacetAccessType for the
type of `val`.
We make deduction undo the `as type` conversion to move back to the `T`
parameter declaration, which has type FacetType in order to deduce the
required facet value (which is itself a type constrained by the
FacetType). And when we have a facet value (of type FacetType) to be
deduced, we will also convert the argument if it is of an appropriate
type to a FacetValue that matches the FacetType using the changes from
PR #4863.
Tests with `impl forall` can cause impl deduction to recurse forever and
crash, so those tests are omitted in this PR and they will come in
follow-up work that address the infinite recursion.
Rebased on top of PR #4885
When a function has a generic FacetType parameter, it can depend on
other FacetTypes bound as earlier parameters. To deduce the FacetValue,
we need to know the impl to attach to it, which requires knowing the
full type signature of the generic FacetType parameter. To do this,
after deducing other arguments to determine the value of non-generic
FacetType parameters, we substitute them sequentially into later
symbolic parameters to get their full facet types, then converts the
arguments to those full facet types to get the FacetValue. For example:
```
fn F(T: type, U: Interface(T));
```
Here the `T` binding is deduced to the be the caller's argument type.
But the Interface(T) can not be properly deduced to a FacetValue in the
first pass, and it will just be the caller's argument type directly.
After the first deduce pass, we will substitute the deduced T binding
into Interface(T), at which point the argument type can and will be
converted to a matching FacetValue as long as an impl can be found.
Closes#4868
This is based on PRs #4881 and #4863
---------
Co-authored-by: Richard Smith <richard@metafoo.co.uk>
For an expression such as `(Type as Interface).AssocFn()`, track the
`Self` type `Type` in the result of the member access so that it's
available when checking the function call.
This introduces a new kind of type, `ImplFunctionType`, that represents
the type of a function that is expected within an impl, modeled as the
type of the function within the interface plus a value to use as `Self`.
Calls to values of this type behave like calls to the underlying
function except that the `Self` parameter is pre-bound to the self type
from the facet.
In order to support this, fix an issue where the imported list of
generic bindings lost their association with their enclosing generic.
This adds a little complexity to `import_ref`, including a new recursive
cycle that I intend to address in a follow-up PR.
---------
Co-authored-by: Jon Ross-Perkins <jperkins@google.com>
This is a followup from #4834, I searched for "invalid" uses in our
codebase. This is mostly changing comments, and a couple debug
functions, but shouldn't affect testable behavior.
Note a couple things I'll highlight as not changing (but could) are:
- `ReturnTypeInfo::is_valid`
- `"invalid"` uses in the formatter
- `AddInvalid` for `!has_value` in `inst_fingerprinter` (because the
cases it's called sound invalid-ish)
High level, replacing `Id::Invalid` with `Id::None` and `Id::is_valid`
with `Id::has_value` for clarity, as discussed
[here](https://discord.com/channels/655572317891461132/655578254970716160/1331664574545395794).
The `IntId` refactoring is needed together with `AnyIdBase` because it's
also used with `ValueStore`.
Note, trying to be careful not to rewrite `EnumBase::InvalidIndex`, or
`is_valid` in general (e.g., `IdKind::is_valid`).
I've tried to sequence commits here:
1. Automatic replacements:
- `((?:Id|Index)(?: |::|\(|Base(?:\(|::)))Invalid((?:Index)?\W)` ->
`$1None$2`
- `<invalid>` -> `<none>`
- `InvalidNodeId` -> `NoneNodeId`
- `/\*invalid\*/` -> `/*none*/`
- `id((?:_|\(\))(?:\.|->))is_valid` -> `id$1has_value`
2. Manual edits:
- In `int.h` and `int_test.cpp`
- `IntT` has `is_value`, which I'm renaming to `is_embedded_value`.
- Manual edits to comments in this file.
- `AnyIdBase` and `IdBase`
- Declaration of `is_valid` -> `has_value`, `InvalidIndex` ->
`NoneIndex`.
- In `ids.h` and `ids.cpp`
- `is_valid` -> `has_value`
- `// An explicitly invalid ID.` -> `// An ID with no value.`; similar
for index
- Various math on `InvalidIndex` -> `NoneIndex`
- Various mentions of "valid" in comments
- In `value_store.h`, for `IdT::Invalid`, plus one comment
- In `impl.h` and `tokenized_buffer.h`, we had different initialization
of `::None` values (versus `ids.h` syntax) that I fixed manually.
- Spot checks to compile
- Particularly where `is_valid` replacements didn't catch spots due to
different naming.
3. Autoupdate tests
4. verbose.carbon (NOAUTOUPDATE)
5. Comment spot checks
Note there are probably other mentions of "Invalid" that should be swept
up, but I'd like to argue for merging and separating out remaining
cleanup since this is so sweeping (and likely to hit merge conflicts
from churn). We'll probably have lingering mentions of "invalid" for a
bit regardless, just because there are uses of "invalid" in non-Id APIs.
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.
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.
Previously it would allow interface mismatches. We only need to support
the case where there is a single interface, though, which makes checking
much more straightforward.
Co-authored-by: Josh L <josh11b@users.noreply.github.com>
This is for more clearly distinct names, and to make it a clearer
transition from `BuiltinInst` for name conflicts. `FloatType` is also an
instruction, and we have `Carbon::Error` (common/error.h). This avoids
affecting tests, although the name is embedded in the builtin test.
In `LegacyFloatType`, `Legacy` because I was having trouble coming up
with a more appropriate name. I'm not clear this is a `FloatLiteralType`
at present, it needs some work to mirror `IntLiteralType`.
In `ErrorInst`, the suffix `Inst` was discussed as good and similar to
`BuiltinInst` (although I'm trying to get rid of that).
* The `extended_scopes` in a `NameScope` were represented by a
`NameScopeId`. Replace that with an `InstId` of an instruction returning
the type that is extending this name scope.
* `Context::LookupQualifiedName` now can take multiple scopes to look
in.
* `GetAsLookupScope` was moved out of `member_access.cpp` and is now
`Context::AppendLookupScopesForConstant`
This PR also fixes some existing issues that were revealed as part of
writing and testing this PR:
* Additional validation and handling of invalid ids.
* `extend impl` in a class is not properly imported yet, but at least
now it doesn't crash.
The change to use an `InstId` also allowed some diagnostics and
formatting to be improved.
---------
Co-authored-by: Josh L <josh11b@users.noreply.github.com>
This does a few things:
* Replaces the single `TypeId` in the `FacetTypeInfo` struct with a
vector of `InterfaceId`, `SpecificId` pairs (sorted in id order)
representing the set of interface requirements of the facet type. This
will later be used to support facet types with multiple interface
requirements (as in `I & J` or `I where .Self impls J`).
* Replace `InterfaceType` instructions (used as the type of an
`InterfaceDecl` instruction) with `FacetType` instructions (introduced
in #4460) with a (newly introduced) `FacetTypeFromInterface()` function.
* Replace code that consumed `InterfaceType` values with code that
consumed `FaceType` values. I've generally left the assumption in the
code that it is dealing with a single interface, using the (newly
introduced) `FacetTypeInfo::TryAsSingleInterface`, and producing an
error otherwise. There isn't yet support for the `&` operator or `where
.Self impls`, so this is generally a good assumption for now, except you
can get a facet type with no associated interfaces from a `type
where`... expression. In some cases, the facet type value is pulled from
the evaluation of an `InterfaceDecl` instruction, where the single
interface assumption will hold permanently.
* Some related cleans up: nicer stringification and formatting of facet
types, suppression of some errors when there already was an error.
There is still a lot left to do, including:
* Type `type` should be a facet type with a reserved id, replacing the
built-in instruction.
* Code using `TryAsSingleInterface` should generally be upgraded to
handle more than (or less than) one interface. Name lookup should be
particularly exciting.
* Operator `&` should be defined on facet types, unioning their
interface and other requirements.
* Requirements from a `where` clause don't do anything yet.
* Impls and impl lookup need to resolve facet types, and do things like
determine if all the associated constants are given values.
---------
Co-authored-by: Josh L <josh11b@users.noreply.github.com>
Co-authored-by: Geoff Romer <gromer@google.com>
Distinguish between deduction against a symbolic binding pattern and
deduction against a symbolic binding name. In the former case, the value
is being explicitly specified and must be constant. In the latter case
we encountered a use of the binding name as a subexpression, and should
deduce against it if it's not explicitly specified.
Also propagate the pattern IR along with the pattern-match IR, and use
it where appropriate.
Strictly speaking, some parts of the pattern-match IR are allocated
eagerly, while traversing the pattern's parse tree, but they still
aren't actually emitted until we traverse the associated pattern insts.
---------
Co-authored-by: Jon Ross-Perkins <jperkins@google.com>
Geoff Romer
Dana Jansens
Boaz Brickner