Add `Dependent` value and initializing representations for types whose
representations are unknown because they are dependent. When generating
SemIR in such cases, use a worst-case initializing representation that
both provides a destination address and also propagates a potential
result value.
Use this to fix incorrect lowering and lowering crashes for specific
functions involving generic types that don't use a copy value
representation.
In lowering, be careful to distinguish between whether the initializing
representation for the generic return type uses a return slot (which
affects whether the SemIR declaration and call have one) and whether the
initializing representation for the specific return type uses a return
slot (which affects whether the LLVM IR declaration and call have one).
This is in support of a goal of changing the blanket `destroy` impl to
use (roughly):
```
private fn CanAggregateDestroy() -> type = "type.can_aggregate_destroy";
// Handles aggregate type destruction.
impl forall [AggregateDestroyT:! CanAggregateDestroy()] AggregateDestroyT as Destroy {
fn Op[addr self: Self*]() = "type.aggregate_destroy";
}
```
That isn't done here because there's still other issues that migrating
raises. What this *does* do is add the builtin functions, and in
particular, support to `FacetTypeInfo` to make `CanAggregateDestroy`
work.
The "special requirement" approach in `FacetTypeInfo` allows us to
support restricting a blanket impl under the current approach of impls.
Maybe we'll find a cleaner approach that can work in the future, but
this fits into the current model by propagating similar to other
requirements. I'm using an enum mask because we have a number of similar
things to add (e.g. copy, move) but I'm not sure we need a full vector.
A few alternatives considered were:
- Supporting syntax more like `where .Self impls
TypeCanAggregateDestroy(.Self, SupportedInterface,
UnsupportedInterface)`. I think it'd be a little cleaner, but requires
better compile-time evaluation in order to assess the type of the call.
Right now it's expected to be a `FacetType` too early to make this work,
and I was concerned about pouring too much more time down this route.
- Providing an actual interface, in particular doing name lookup back
into `Core.` for an interface. This would've added name lookup overhead,
and the question of whether an `impl` exists.
- Generating an interface. This avoids the name lookup, but would still
raise the question of whether an `impl` should also be generated. Work
I've previously done generating interfaces for class destruction also
feels complex to both write and understand (an unfortunate issue).
- Still modeling as an `ImplsConstraint`, for example by defining a
special `InterfaceId::CanAggregateDestroy = -2` similar to what we do on
other ids. I was hesitant because of how this expands the number of
modes of `InterfaceId`, and things for consuming code to watch out for,
for what feels like a relatively niche set of use-cases that are only
interface-like.
---------
Co-authored-by: Dana Jansens <danakj@orodu.net>
When returning a value from a function whose return type has a by-copy
initializing representation, perform initialization like we do when the
return type has an in-place initializing representation. This makes our
SemIR representation more uniform, as the return expression will now
always be an initializing expression rather than a value expression, but
more importantly it means that attempts to return a non-copyable type by
value now fail, even if the type has a by-copy initializing
representation.
This catches a bunch of places where we were returning a value of an
unconstrained template parameter `T:! type`, which we were incorrectly
allowing because we didn't notice it was not copyable. Unfortunately
this then requires quite a few test updates.
Like #6034, this exposes a lowering issue where lowering crashes when
attempting to lower a specific copy operation for certain types; a
couple more tests are temporarily disabled here. An upcoming PR
dependent on this one will fix the issue and re-enable those tests.
Instead of hardcoding which types are copyable, add a `Core.Copy`
interface to perform copying. Move almost all the current copy support
to that interface. Some remaining pieces are still using builtin logic
after this PR:
* For tuples and structs, builtin logic is used to perform elementwise
copies. This also supports copying *adapters of* tuples and structs,
which seems like it may not be desirable, especially for non-extending
adapters. A `Copy` impl is provided for tuples of at most 2 elements, so
that `Core.Copy` constraints are satisfied, but we can't implement this
generally until we have variadics support, and don't yet have a
mechanism to generalize this to structs.
* For `enum` types imported from C++, builtin logic is used to perform a
copy. This is temporary until we have a mechanism to identify these
types from an impl in the prelude.
One lowering test in `toolchain/lower/testdata/class/generic.carbon` is
disabled for now, as it causes a crash in the lowering code due to an
ABI mismatch between the call signature in the lowered declaration of a
specific function and the call that is generated in the specific callee.
Fixing this is a little involved, and will be done in a separate PR.
---------
Co-authored-by: Geoff Romer <gromer@google.com>
This avoids impl lookups involving, say, `Core.Int` pulling in all ~65
impls in "prelude/types/int", which resulted in a lot of unnecessary
importing work, followed by a lot of unnecessary inst namer and inst
formatter work.
Before:
```
Ran 1335 tests in 6186 ms wall time, 146818 ms across threads
Slowest tests:
- toolchain/check/testdata/interop/cpp/function/arithmetic_types_bridged.carbon: 5611 ms, 5532 ms in Run
- toolchain/check/testdata/interop/cpp/function/operators.carbon: 2034 ms, 1981 ms in Run
- toolchain/check/testdata/primitives/import_symbolic.carbon: 1796 ms, 1786 ms in Run
- toolchain/lower/testdata/operators/arithmetic.carbon: 1729 ms, 1728 ms in Run
- toolchain/lower/testdata/function/generic/call_recursive_sccs_deep.carbon: 1700 ms, 1697 ms in Run
[==========] 1335 tests from 1 test suite ran. (682 ms total)
```
After:
```
Ran 1335 tests in 2419 ms wall time, 109587 ms across threads
Slowest tests:
- toolchain/check/testdata/interop/cpp/function/arithmetic_types_bridged.carbon: 1748 ms, 1665 ms in Run
- toolchain/check/testdata/interop/cpp/function/operators.carbon: 1106 ms, 1057 ms in Run
- toolchain/lower/testdata/function/generic/call_recursive_diamond.carbon: 1044 ms, 1041 ms in Run
- toolchain/lower/testdata/function/generic/call_recursive_sccs_deep.carbon: 1015 ms, 1012 ms in Run
- toolchain/lower/testdata/operators/arithmetic.carbon: 998 ms, 997 ms in Run
[==========] 1335 tests from 1 test suite ran. (652 ms total)
```
That's still slower than it should be, but a large improvement
nonetheless.
Fixes#6029
If it's just `TypeType` then the `BindSymbolicName` appears directly in
type positions, but if it is replaced with another facet value, then we
would need to insert a `FacetAccessType` around it. By giving it a
`FacetType` type, like other `BindSymbolicName`s we make it consistent
and avoid having to introduce extra instructions.
This uses each vector's size as a barrier between lists, to eliminate
the possibility of incidental collisions between entries of different
lists. This is the same as is done inside `AddBlock`.
Attach the cleanup to the `Temporary` instruction instead of to the
`TemporaryStorage` instruction. We create `TemporaryStorage`
instructions speculatively when creating an initializing expression, and
may overwrite those instructions with other instructions if it turns out
that a temporary is not required. Instead, wait until we finalize the
temporary and create a `Temporary` instruction to register the cleanup.
There are a few instructions that import in multiple phases, which
receive the `const_id` and use it to construct multiple constants until
building the final constant value. These include
`AssociatedConstantDecl`, `FunctionDecl`, and `InterfaceDecl`.
Other instructions just construct a constant value in a single attempt,
once all their dependencies are imported. For these instruction types,
avoid importing the non-canonical instruction. Always get the canonical
constant instruction and import that.
Since the constant value of an instruction can have a very different
structure than its non-canonical value, this ensures import has a
consistent structure to work with, by only working with canonical values
as much as possible.
The `VtableDecl` and `VtablePtr` were set up to pass along `const_id`
but do not actually require multiple phases, so they have been changed
to stop passing along the unused (and always empty) `const_id`.
---------
Co-authored-by: Jon Ross-Perkins <jperkins@google.com>
Decouples associated constants from being special cased in let handlers.
Enforces associated constant grammar restrictions in parsing instead of
checking.
Closes#5411
Following the direction of #5913, add support for parsing an `unsafe as`
operator. For now, we allow one additional conversion using `unsafe as`
beyond the conversions supported by `as`: we permit pointer conversions
that remove qualifiers, such as `const T*` -> `T*`.
This is necessary if the source type is an adapter, as we would not
otherwise be able to determine what type it adapts and hence could be
converted to.
We add a virtual node (`CompileTimeBindingPatternStart`) as the first
child of `CompileTimeBindingPattern` which holds the identifier
underneath it, so that it is checked just before the type expression of
the `CompileTimeBindingPattern`. When we reach this virtual node during
check, we add `.Self` as a name in the current scope, and when we reach
`CompileTimeBindingPattern` we remove it from scope, which ensures it's
present during only the checking of the type expression for the compile
time pattern.
At the moment the `.Self` has a different type (it's a `TypeType`) than
other `.Self` in the facet type (which are a single `FacetType`), but
the intention is to immediately substitute it out of the facet type
entirely, replacing it with a reference to the compile time binding (a
`BindSymbolicName`) itself. A TODO has been added for this.
---------
Co-authored-by: Jon Ross-Perkins <jperkins@google.com>
The self access is important; for the test `generic_class.carbon` being
added to `toolchain/check/testdata/class/destroy_calls.carbon`, it was
using `%T.as.Destroy` instead of `%D.as.Destroy`, indicating the default
blank impl was being used instead of the type-specific version. That
test is trying to focus on the issue, but the delta is visible in a
couple other files in this PR, for example
`toolchain/check/testdata/class/generic/init.carbon`.
I'm separately working on getting rid of the default impl, which is how
I noticed this.
Add missing builtins for float compound assignment, for building a
FloatType, and for converting a float literal to FloatType. Switch
`Core.Float` to being a class and add impls for the various
floating-point operators.
---------
Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com>
Co-authored-by: Carbon Infra Bot <carbon-external-infra@google.com>
This makes all `.Self` references in a facet type canonically the same
(which will remain true iff they refer to the same `Self` type in the
future), removing the need to do more complex comparisons between them
using the EntityName, interface, and index. This allows the comparison
of types containing `.Self` references to be done correctly regardless
of where the `.Self` appears, as such type expressions will all be
canonically equal if they otherwise equal now, regardless of whether
they are written in the context where `.Self` could have seen different
`Self` facet types.
In order to retain access to constraints on a base `.Self` facet type,
in the case of applying `where` to an existing facet type, we:
- Give the base facet type as a `RequirementBaseFacetType` constraint so
that eval of `WhereExpr` can find and copy all the constraints off of
it.
- Introduce eager/early rewrite constraint resolution, which allows a
constraint to eagerly resolve access to earlier rewrite constraints
(`where .A = () and .B = .A` is eagerly transformed into `where .A = ()
and .B = ()`) before the full constraint resolution step. This allows
use of rewrite constraints in larger type expressions, such as `where .A
= () and .B = C(.A)` and `C` will know that the argument is `()`.
I was thinking about this for destruction, which I may not be able to
use it for, but still think this may be a good change to keep features
consistent.
toolchain/check/testdata/builtins/char/basics.carbon and
toolchain/lower/testdata/builtins/char.carbon are probably the most
interesting tests here. The parse tests is required because this adds a
new node kind, and we need coverage of it; but the attached info is
minor. There's a fair amount of test churn here because I'm adding the
Core.Char and Core.CharLiteral types as new singletons.
My intent here is that `CharId` is always a unicode code point, even
when the type is a `Char` and thus must be a single UTF-8 code unit
(single byte). This mainly means the stored value of a `CharValue` can
be printed internally without knowing the type.
---------
Co-authored-by: josh11b <15258583+josh11b@users.noreply.github.com>
Co-authored-by: Richard Smith <richard@metafoo.co.uk>
Although this focused on `Destroy` support, some choices here around
`implicit_type_impls` are because copy/move will likely follow a similar
approach. I'm trying not to predict too much about how we'll structure
those, but I'm putting `Destroy` impl logic in a file that could perhaps
be shared with those. They'd likely be interested in similar things,
e.g. traversing members of types (particularly class, struct literal,
tuple literal).
At present this sets the destroy function as `no_op` which is consistent
with current logic, but has a TODO to correctly define.
Constant importing for functions changes slightly due to some issues I
was having with `GetFunctionType`. zygoloid suggested this approach to
avoid `EvalInst` logic.
Adds a flag for controlling whether to generating these impls. While
this does generation for `class`, as noted above this'll also need to be
done for tuples and struct literals, which would leave the `none.carbon`
min_prelude unable to use any types. Note if destruction *would* occur,
it'll still look up `Core.Destroy` for that and fail, but that's already
true of any test using `none.carbon`. I'm trying to use the flag to see
if we can keep `none.carbon` working mostly-consistently.
I'd tried separating out the flag to #5852, but that got a lot of
pushback over whether the behavior was appropriate. I'm hoping that the
interactions here make it clearer why the particular approach -- the
goal is not to enable advanced testing, or create some new end-user
behavior that we really support, it's just to keep no-prelude tests
functional. The main question raised there was why not just keep
generating `impl T as Core.Destroy` if `fn destroy` is present -- but I
think here it should be apparent that would require additional
complexity, as the generation of `impl T as Core.Destroy` is not
currently conditioned based on the implementation of `fn destroy`. I'd
rather add complexity to this flag only if it's enabling interesting
test functionality.
---------
Co-authored-by: Geoff Romer <gromer@google.com>
In order to verify rewrite constraints at the end of
`LookupImplWitness()` we need to replace references to associated
constants in the query facet type with values that come from the query's
self. To do this, we find any `ImplWitnessAccess` that is a reference to
`.Self` and replace its witness with the witness found through the impl
lookup process, if the interfaces match. This allows the
`ImplWitnessAccess` to resolve to a concrete value if that witness was
concrete. Then we just need to compare that for each rewrite constraint
the lhs and rhs are the same constant value. If they differ, the self
provided a different value for one side (either through its own facet
value constraints or through an associated impl), or the self did not
provide a value at all.
For now, only .Self references in the top-level facet type are
rewritten. Nested facet types are not, even if they contain a .Self
reference up to the top level facet value. This will be addressed by
adding numbering to the EntityName of of .Self in a BindSymbolicName.
See the third model in
https://docs.google.com/document/d/1Yt-i5AmF76LSvD4TrWRIAE_92kii6j5yFiW-S7ahzlg/edit?tab=t.0
for the plan. For now, there is a TODO addressing this.
Iterate over arrays by producing their elements in the obvious way. We
use `i32` as the cursor type because that's the type that check converts
array indexes to. This may need revisiting if we support arrays with
more than 2Bi elements.
Also includes a fix for an import crash bug that's triggered by this
change, borrowed from #5873.
Per discussion at
https://github.com/carbon-language/carbon-lang/pull/5875#issuecomment-3137288037,
a different approach to the same solution.
A key difference is that whereas #5875 would build a `TypeStructure`
containing `ConcreteType{error}`, this instead just returns nothing.
This means impls with errors can't be compared in the same way, though
I'm not sure how much impact that'll really have (I've added a test here
to show a case where it seemed interesting to see what effect it'd have,
and it seems to have none).
---------
Co-authored-by: Dana Jansens <danakj@orodu.net>
Dropping this in with basic.carbon as an aspirational way to encourage
more tests there.
This currently crashes because `CollectCandidateImplsForQuery` tries
building a type structure which cannot contain `ErrorInst`.
Make Subst perform "recursion" on the RHS instructions as they are
replaced, effectively doing a depth-first traversal through the rewrite
constraints doing replacements. This allows us to fully compute
individual associated constants in the minimal amount of work, and cache
the results so they can be reused cheaply in cases where the rewrite
constraints generate an exponential number of references to associated
constants.
Fixes https://github.com/carbon-language/carbon-lang/issues/5672
This is in anticipation of having `class` depending on the `Destroy`
interface, in order to automatically generate implementations of it.
I'm doing some sorting of imports in the prelude too, which I hope will
be uncontroversial; clang-format would do similar in C++...
Change impls from `<interface>.impl` to `<self>.as.<interface>.impl`,
and *member* functions to `<parent scope>.<fn>` (non-member functions
exclude their parent scope). Stop special-casing builtin functions,
given the new naming scheme.
The purpose of this is to make it clearer when a member function is
being accessed and, if so, which member function. In particular, we
often access interface `Op` functions. The builtin function
special-casing was intended to help with that, but we still have lots of
`Op` functions. This particular approach should make the interactions
clearer.
This changes up queueing of block IDs a little because, in particular,
we need to process bodies of entities only after constants finish
processing. But, it should also result in less memory usage during
processing because it means we have less on the insts stack at any given
time, since we track a block rather than all instructions contained by
the block.
This switches from the `CollectNamesInBlock` approach for entities, to
instead traversing entities as they're encountered. For example, when
traversing constants, when a type is found, the entity will have its
block queued for processing.
This leads to a change in the traversal order, which affects
disambiguation done by numeric sequencing (since that's just showing the
traversal order).
This will allow for simpler "name based on name" logic. This is
something I plan to use for:
- impls: `<type>.as.<interface>.impl`
- functions: `<entity>.<member function>`
- Note an impl may be used as the entity for a bound function.
By naming the entities as they're encountered, I'll be able to rely on
the generated names rather than recalculating them.
To assist this, I'm also differentiating between the ambiguous and
disambiguated name. Otherwise, we could end up with things like
`<function>.<disambiguator>.<call>.<other disambiguator>`, where the
repeated disambiguator may not be necessary in order to get full
disambiguation. It's also a smaller delta from the current output.
Note, changing `Name` to a class felt appropriate given its shape. I was
also noticing that parts of its API were unused, and the class helps
detect unused private members.
---------
Co-authored-by: Geoff Romer <gromer@google.com>
Use `full.carbon` min-prelude for any tests using the full prelude. A
few tests were using it unnecessarily and are changed to a more minimal
one in the process.
Any test which does not include some min-prelude will now fail with an
error.
Direct use of an error means we can delay looking at the actual type,
and also use the `self_id` of the stored `ImplInfo` directly.
Note, I'm assuming we should be okay with this not always being a
`NameRef`. In error cases though, it seems like we shouldn't mind it
being an error instead of a name reference to an error.
In `BuildUnaryOperator`, `GetOperatorOpFunction` is treated as
desugaring, but `PerformCompoundMemberAccess` and `PerformCall` are not.
This treats all of destruction as desugaring.
This leads to some instructions being elided, because of `GetOrAddInst`
behaviors:
> // If the instruction has a desugared location and a constant value,
returns
> // the constant value's instruction ID. Otherwise, same as AddInst.
This changes instructions that previously had a non-desugared location
to instead have a desugared location, so if they also have a constant
value then the constant value can be used directly.
This just catches uses equivalent to `return;` and `return <expr>;`.
Note it's just extending the current implicit return logic, not really
adding much unique here.
I'm still delaying break and continue because those require partial
destruction, which is more work and I want to be careful to get it
right.
Currently if the first operand contains an error, we will return error,
even though the second operands contains a runtime, and it has a
stronger priority (the phase always goes up if possible).
Import is only allowed on instructions with compile-time values, so we
crash if we ever try to import a runtime value. Importable instructions
must diagnose unexpected runtime values and produce errors in the semir
from which they would be imported so that runtime values are never
imported by another semir.
If we had an instruction where you had an error value from the first
operand, and runtime from the second, and we imported it:
- Before https://github.com/carbon-language/carbon-lang/pull/5728 we
would crash in import, but only because we treated errors as runtime
- After https://github.com/carbon-language/carbon-lang/pull/5728 we
would import ErrorInst because we propagate errors. This is desirable
for cases with compile-time values and errors present only.
- After this PR, we would crash again, cuz you're importing a runtime
thing.
This change means that instructions containing an
`InstConstantKind::Never` instruction like`ValueParam` will consistently
evaluate to a runtime value, even if there are errors present. This is
visible in the `BindName` instructions changing in the semir, where they
became constant `ErrorInst` values previously but no longer do.
This changes `Destroy` to use an interface for its implementation.
Note that this change includes a lot of test updates. Even when
`Destroy` is a no-op, it still causes code generation as part of
determining that.
Originally I was trying to use ranges to cut down the scope of this, and
to a degree I think they have. But a flipside here is that cases where
no destructors should be generated -- particularly globals -- would be
needed to completely remove destructor calls. Even for ranges, the range
can often include the destructor placement. So I've shifted
frame-of-thought a little: accept a bunch of destructor churn, because
destructors are needed and will be prevalent. The verbosity is a feature
of the design to make desugaring apparent in IR, not a bug.
This drops file_test runtime from about 3s to 2.5s on my machine, which
is now ~30% faster than before #5653 slowed things down by adding a lot
of stuff to the production prelude.
---------
Co-authored-by: Jon Ross-Perkins <jperkins@google.com>
The none.carbon min-prelude is not just an empty prelude, it also
prevents any prelude from being imported at all. So no import machinery
runs before the test, only the `package` statement from the prelude
would run.
Use the none.carbon min-prelude in a few tests that were specifying
`--no-prelude-import` to give it a trial run.
---------
Co-authored-by: Jon Ross-Perkins <jperkins@google.com>
This drops the wall clock time for running file_test from 10s to 8s on
my machine. There's many more tests to convert, as each one takes the
test from ~1s to ~100ms. Compiling the full prelude is a bit slow now
since #5653, and before that file_test was taking about 3.5s.
We introduce a few more flavours of min_prelude to support more tests.
Remove use of i32/bool when a builtin type or test-define class type can
work. Make `Sub` user-defines in a test that is testing builtin
functions and not trying to test the prelude, in the same way that it
defines its own Negate. Reduce use of the + operator when it isn't
contributing to the test's coverage, since the + operator needs the full
prelude. Remove use of Core.Print when it's not required for the test.
Move `deduce_nested_facet_value.carbon` to its own file since it uses
TypeAnd, and the rest of deduce.carbon does not, but uses i32. This
means they can each use a different min-prelude.
---------
Co-authored-by: Jon Ross-Perkins <jperkins@google.com>
When the binding pattern appears within a `var` pattern, convert to a
reference. Otherwise, convert to a value.
This gets the advent of code examples to produce the right answers again
:)
---------
Co-authored-by: Geoff Romer <gromer@google.com>
import_use_generic.carbon has the comment "// We're just checking that
this doesn't crash. It's not expected to compile." Because it involves
import behavior by name, I'm not touching it. Other than that, while
maybe it's better to test with less, the `ImplicitAs` errors at best
feel difficult to understand, and at worst could be masking an issue.
Replace the binary `Operation` interfaces with the `OperationWith(T:!
type)` interfaces described in the design, and add a `Result` associated
type for both unary and binary operations. Update the `impl`s in the
prelude for integer types to use the new form, including supporting
implicit conversion of either operand.
I've tried to split this PR up into commits focused on distinct changes
for review convenience. It may be simplest to review it one commit at a
time.
Given a facet type: `(Z where .X = .Y) where .X =.Y`
The rewrite constraints in the inner facet type are each an
`ImplWitnessAccess` into a witness for the self of type `Z` (which is
the facet type before the `where`). The rewrite constraints in the outer
facet type are each an `ImplWitnessAccess` for the self of type `Z where
.X = .Y`, which is a different self facet type.
This means when deduping in canonicalization, the first `.X` and the
second `.X` are different instructions, and different constant values,
so they both remain in the rewrite constraints, incorrectly. Then if the
outer `.X` is allowed to evaluate to a value from its facet type, it
finds `.Y` resulting in `.Y = .Y` which is also incorrect.
Because of the failure to dedupe the first facet type, that is also
diagnosed as two different assignments to the same `.X`. To resolve
that, we introduce `CompareFacetTypeConstraintValues()` compare values
in facet type constraints, and treat accesses to the same associated
constant in the same facet value as `equivalent` even when through
different witnesses. This allows us to dedupe the two `.X = .Y` rules
into one in the combined facet type.
Given a different facet type: `(Z where .X = ()) where .X = {}`. Here we
want to diagnose that `.X` has been assigned two different values. To do
so, we need to see that the two `.X` values are the same, and we use
`CompareFacetTypeConstraintValues()` to do this comparison. Then we see
two rewrite rules for the same LHS, and we can diagnose that.
We enable evaluating `ImplWitnessAccess` on `.Self` to pull a value from
rewrite constraints in a facet type so that we can see that we are not
incorrect evaluating the LHS of rewrite constraints and producing
cycles. By doing so, also enable generic code to see and use concrete
values in associated constants in facet types.
The `BitAnd` operation combines two `FacetTypeInfo` structures by
concatenating their lists, but did not apply the current specific to the
instructions in the `FacetTypeInfo` as it forgot to go through
`GetContantFacetTypeInfo`.
`WhereExpr` handling duplicates a lot of the logic in
`GetConstantFacetTypeInfo` by calling `GetConstantValue` on things,
instead of calling `GetConstantFacetTypeInfo` on the `FacetTypeInfo` it
constructs. This meant it also needed to call `GetConstantFacetTypeInfo`
on the base facet type, and on any `impls`-requirement facet types
before merging their values together into a single `FacetTypeInfo`.
Instead, make `WhereExpr` more like `BitAnd`, and have it concatenate
things together as-is to construct a `FacetTypeInfo`. Then call
`GetConstantFacetTypeInfo` to canonicalize it and return a constant
value referring to it.
In `GetConstantFacetTypeInfo` we fix a crasher by propagating errors
inserted into the `FacetTypeInfo` out to the `Phase` so that the
resulting instruction depending on the `FacetTypeInfo` is not resolved
to a constant value with errors inside it. A test is added for this,
which was crashing on import of the `FacetType` with an error within
from the imported `impl` decl.
This refactoring gives us three benefits:
* There's now only a single place that does
`ResolveRewriteConstraintsAndCanonicalize`, which is inside
`GetConstantFacetTypeInfo`. This makes the inputs/behaviour of
`ResolveRewriteConstraintsAndCanonicalize` more consistent.
* There's now only a single place that updates the instructions in
`FacetTypeInfo` constraints with new constant values, so that changes
that rely on observing and interacting with that code only need to be
written in a single place. This will avoid duplicating logic in
https://github.com/carbon-language/carbon-lang/pull/5644.
* This will make it easier to move `WhereExpr` handling to a
`EvalConstantInst` function, as it no longer directly depends on
`GetConstantValue()` from `eval.cpp`.
This preserves the constant values of the arguments to the thunk, which
is important if the thunk requires conversion of an `IntLiteral` to some
other type. This should become unnecessary once we have form support,
but avoiding the indirection through a thunk function seems valuable
even once that support is in place.
To support this, track whether a function is a thunk on the Function
object, and if so, what the callee of the thunk is. This information is
also included in formatted SemIR when dumping the thunk.
Richard Smith
Jon Ross-PerkinsandDana Jansens
Elliott Kalt
