- A test that should fail that looks to see we poison impls when we do a
concrete lookup, so you can't define an impl specialization after we
looked for it. This currently passes but should fail.
- A test with a final specialization with a type constant written before
a generic function using it. The generic function should be able to know
the concrete type of the constant. This currently fails, and was
discussed in open discussion here:
https://docs.google.com/document/d/1Iut5f2TQBrtBNIduF4vJYOKfw7MbS8xH_J01_Q4e6Rk/edit?resourcekey=0-mc_vh5UzrzXfU4kO-3tOjA&tab=t.0#heading=h.swr8311y952x
- A test with a specialization written after a generic function, which
will be used symbolically so the type constant will not be known. This
fails and should continue to, though the error diagnostic may change in
time.
- A test with a specialization written after a generic function, and
which returns a value typed as the type constant from that
specialization. The generic is called with types that should cause it to
use that specialization in the specific, so the caller gets back the
type expected. This currently fails but should pass.
---------
Co-authored-by: josh11b <15258583+josh11b@users.noreply.github.com>
When the parameter is a deduced symbolic FacetValue, refering to a
BindSymbolicName, and the argument is a concrete FacetValue that would
match the FacetType requirements on the BindSymbolicName's type, we
currently do not deduce that the argument matches the parameter.
The argument is not _converted_ to the parameter type because they are
both FacetValues of the same FacetType type. However they are also not
equal constant values so the argument is not saved as a deduced match
for the parameter.
In order to accept the FacetValue, we need to consider them as
`deduce_through`, which attempts to deduce each of the fields in the
argument FacetValue against the fields in the parameter FacetValue.
This deduces that the argument's concrete type matches the symbolic
BindSymbolicName and its witnesses are the same.
Since the parameter is a FacetValue, its argument is not the type that
needs to be recorded as the deduced type for the binding. The
BindSymbolicName inside the parameter is the place that we need to find
the deduced type for the binding. So simply walking into the FacetValue
gets us to that position, where we eventually record the deduced
argument type as being the concrete type from the original argument
FacetValue.
Similarly, when determining what interfaces are satisfied by a
FacetValue for deduce, we want to use the full type available in the
FacetValue rather than just those from its FacetType. Determining
availability of interfaces here is equivalent to converting, and we want
converting a FacetValue to always work on the full available type info.
Only API access (member lookup) is restricted by a FacetValue to the
interfaces provided by its FacetType type.
---------
Co-authored-by: Richard Smith <richard@metafoo.co.uk>
This example motivated a decision to add a feature where we do impl
lookup to see if there is a matching `final` impl even in some cases
where it is already established that the type implements the interface.
I don't know when we plan on implementing this, but I wanted to capture
the example as test so we would have a TODO to address it.
Co-authored-by: Josh L <josh11b@users.noreply.github.com>
When performing a call through an impl witness, the callee that we
type-check against is the function in the interface, so we form a
specific for that callee. However, once the impl witness access
resolves, the eventual callee is a different function -- the function in
the impl -- so this would cause us to form a `SpecificFunction` where
the callee is one function but the specific refers to a different
function.
Address this by adding another instruction, `SpecificImplFunction`, that
takes a function in an impl and a specific for the corresponding
function in the interface, and computes and returns a `SpecificFunction`
referring to the corresponding specific function in the impl, or returns
a direct reference to the function in the `impl` if it's not a generic
function.
This flows out of #5084 and trying to reduce UnsafeMake use. It turns
out imports and namespaces were using unexpected node kinds (previously
ImportIntroducer instead of ImportDecl, for example). This fixes and
adds validation.
I was uncertain about whether to just remove the is_convertible check,
since I don't see it as motivating creation of a conversion between
NodeIdOneOf types. So I've just left a TODO for now.
When converting to a facet there are three different failure modes:
1. You provided a non-type value. Only types can convert to facets. So
we tell you that we found a non-type value.
2. You provided a facet type (which has type TypeType) which does not
have witnesses for the the target facet's type. So we tell you that the
type `T` implements `X` but needs to implement `Y`.
2. You provided a (non-facet-type) concrete type (of type TypeType)
which does not implement the target facet's type (which is a FacetType).
So we tell you that we need the type to implement the FacetType but it
does not.
3. You provided a FacetAccessType (which is of type TypeType also, but
we special case this), whose underlying FacetType is not compatible with
the target facet's type. So we tell you that we need the type to
implement `X` but found a FacetAccessType `T` which implements `Y`.
Closes#5027
Currently this test fails with trying to access a comptime function with
runtime values:
```
fn F() {
let a: J = {} as J;
let b: J = {} as J;
// CHECK:STDERR: fail_bit_and_values_no_impl.carbon:[[@LINE+7]]:3: error: non-constant call to compile-time-only function [NonConstantCallToCompTimeOnlyFunction]
// CHECK:STDERR: a & b;
// CHECK:STDERR: ^~~~~
// CHECK:STDERR: core/prelude/operators/bitwise.carbon:96:3: note: compile-time-only function declared here [CompTimeOnlyFunctionHere]
// CHECK:STDERR: fn Op[self: Self](other: Self) -> Self = "type.and";
// CHECK:STDERR: ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
// CHECK:STDERR:
a & b;
}
```
The issue is that the BitAnd impl for facet types is matching on any
value of any type:
```
impl forall [T:! type] T as BitAnd
```
What we really want is for it to match on facet types (which are type
values), which is written as:
```
impl type as BitAnd
```
After this change, the error makes more sense in the above test:
```
fn F() {
let a: J = {} as J;
let b: J = {} as J;
// CHECK:STDERR: fail_bit_and_values_no_impl.carbon:[[@LINE+4]]:3: error: cannot access member of interface `Core.BitAnd` in type `J` that does not implement that interface [MissingImplInMemberAccess]
// CHECK:STDERR: a & b;
// CHECK:STDERR: ^~~~~
// CHECK:STDERR:
a & b;
}
```
This required allowing incomplete facet types where previously
completeness was required. Once we support named constraints, we will
need a way to consistently go from an interface to a facet type witness
index without requiring the interface to be complete in these cases.
---------
Co-authored-by: Josh L <josh11b@users.noreply.github.com>
A query facet type may contain multiple required interfaces, in which
case impl lookup should return an ImplWitness for an impl that is used
for each interface in the query. We bundle these together into an
instruction block and return that from impl lookup. The witnesses are in
the same order as the interfaces in the
`CompleteFacetType::required_interfaces`. This allows walking the
`required_interfaces` to find an interface to give an index that can
also be used to grab a witness from this set, or from FacetValue.
FacetValue now has an InstBlockId for the set of witnesses of the
FacetType, instead of a single ImplWitness instruction id.
FacetAccessWitness includes the index of the witness (determined from
the position in `required_interfaces`) of the witness it's accessing
from the FacetType.
The
toolchain/check/testdata/facet/no_prelude/fail_todo_call_combined_impl_witness.carbon
test demonstrates the fix in the resulting SemIR. We can see the calls
to methods on a multi-interface FacetType result in a FacetAccessWitness
with an index of the correct interface, and this results in a witness
that leads to the correct impl's function.
There is a TODO in member access, where it does not have a
`CompleteFacetType` yet, so it uses the index in
`FacetTypeInfo::impls_constraints` instead, but this can be incorrect in
the presence of named constraints, which when completed can add more
interfaces to the `CompleteFacetType` and which are sorted into an
arbitrary order with the rest there.
---------
Co-authored-by: josh11b <15258583+josh11b@users.noreply.github.com>
During SemIR, when identifying a specific in a specific context, we'd
have to either look through a specific or through a bound method.
Canonicalize which one to look through first, by having the BoundMethod
created around a SpecificFunction instead.
This changes a lot of check tests.
TODO: As the SemIr does not currently allow removal (access to insts()
is intentionally const), the bound instruction created prior to finding
the specific is not removed from the instructions.
Options: (1) leave as is, (2) add a way to remove the previous bound,
(3) rethink how/when the BoundMethod inst is created.
Implements the rule:
> For compound member access `a.(b)` where `b` names a _non-instance_
member of an interface `I`:
> * `a` is implicitly converted to `I`
> * let `T` be the result of symbolically evaluating the converted
expression
> * `impl` lookup is performed for `T as I`.
>
> Instance binding is never performed.
See
https://docs.carbon-lang.dev/docs/design/expressions/member_access.html#impl-lookup-for-compound-member-access.
Before this PR, non-instance members were treated as instance members.
---------
Co-authored-by: Josh L <josh11b@users.noreply.github.com>
Impl lookup for an interface `I` for a facet with facet type requiring
an interface `I` will now succeed, getting the witness from the facet.
---------
Co-authored-by: Josh L <josh11b@users.noreply.github.com>
Co-authored-by: Dana Jansens <danakj@orodu.net>
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>
- Explicitly document that `*Param` and `*ParamPattern` insts represent
`Call` parameters.
- Stop wrapping compile-time parameter patterns in `ValueParamPattern`
insts (because they aren't `Call` parameters).
- Document how `MatchContext::results_` relates to the `Call`
parameters, and be more consistent about when it's written to.
- Remove `RuntimeParamIndex::Unknown`: we no longer need to distinguish
"this `Param`'s runtime index is unknown" from "this `Param` isn't a
runtime param", because we no longer use `Param`s at all in the latter
case.
- Rename `RuntimeParamIndex` to `CallParamIndex`.
As a side effect of removing the `ValueParamPattern` insts, this fixes a
minor diagnostic bug where `NoteInitializingParam` didn't identify the
specific parameter that led to a deduction failure, because it expects
generic parameters to only be represented by `SymbolicBindingPattern`s,
but before this change they could be wrapped in `ValueParamPattern`s.
Here are some test changes so the diffs that come from my upcoming
functionality changes are easier to see. Upcoming functionality
includes:
* Compound member access with non-instance associated constants will
change to comply with the design
https://docs.carbon-lang.dev/docs/design/expressions/member_access.html#impl-lookup-for-compound-member-access
in #5059 .
* Impl lookup will add support for finding impl witnesses in facets
#5060
I'm also making the toolchain/check/testdata/impl/compound.carbon test
into a no_prelude version, and adding a version that tests with
importing.
---------
Co-authored-by: Josh L <josh11b@users.noreply.github.com>
TODO to resolve whether it should conditionally say "object of"
depending on the category
---------
Co-authored-by: Josh L <josh11b@users.noreply.github.com>
Replace the large and growing `TryEvalInstInContext` function with one
function per kind. While we still have special-case handling for a small
number of instruction kinds, most instructions are now handled either
fully automatically or use a common codepath that evaluates the
instruction operands and then performs an eval-context-independent
evaluation of the instruction.
To support this, `InstConstantKind` is expanded to describe more
fine-grained details about how each kind of instruction interacts with
constant evaluation. Also, the operand kinds of instructions become
slightly more fine-grained: we now distinguish between operands that
describe the destination of an initializing expression (`DestInstId`)
from other `InstId` operands, because `DestInstId` operands need
different treatment during constant evaluation. In particular, an
initializing expression can have a constant value even if its
destination is non-constant or has not yet been set, because evaluation
of an initializing expression doesn't include the store to the
destination.
Some minor test changes:
- We now more consistently propagate errors into the results of constant
evaluation, so more instructions that depend on errors have a constant
value of `<error>`.
- Diagnostic location for invalid array types now point at the whole
array type rather than the array index expression, because
`EvalConstantinst` doesn't have access to the original expression.
- Diagnostic for failed `RequireCompleteType` doesn't print the original
type any more because `EvalConstantInst` doesn't have access to the
original expression.
As a follow-up, some of this -- in particular, the `EvalConstantInst`
overloads -- will be moved to a separate file, in an effort to split the
overall constant evaluation machinery apart from the logic to evaluate
each individual kind of instruction.
If the `impl as` clause is on a generic interface, the parameters to the
generic may be constrained by _other_ interfaces. This then requires
another impl lookup, but it should be looking for a different impl since
it's for a different interface.
To avoid considering the same impl again, we discard it from
consideration if the interface itself does not match the interface being
queried.
Note that the query FacetType can have more than one interface in it
eventually, and a `context.TODO()` call is left to notify when we run
into this.
Make facet types complete like other types. This means that in the body
of an interface, the type of `Self` is incomplete. This involved fixing
an issue where eval of a specific_id that was already canonical was not
resolving the specific declaration, which could occur as part of
substituting into a facet type.
---------
Co-authored-by: Josh L <josh11b@users.noreply.github.com>
Co-authored-by: Richard Smith <richard@metafoo.co.uk>
Co-authored-by: Dana Jansens <danakj@orodu.net>
In line with the proposal in #4682, this changes the array syntax to be
array(T, N). `array` is a builtin keyword which must be followed by
parens containing two expressions and a separating comma.
The array type expression is still fully builtin, it does not forward to
a Core.Array library type yet. It merely adds the `ArrayType`
instruction, as was done with the previous syntax.
Followup work will change the instruction to reference to Core.Array,
once the library type exists and can be used directly.
---------
Co-authored-by: zygoloid <richard@metafoo.co.uk>
These tests expose cycles during deduction, when the generic parameters
in an impl statement require deduction and the impl clause that
satisfies them comes after the one containing the generic parameters.
This causes the same impl to be looked at repeatedly, and produces a
cycle diagnostic.
---------
Co-authored-by: Geoff Romer <gromer@google.com>
* Add `RequireCompleteFacetType` and `ResolveFacetTypeImplWitness` to
`check::Context`. Goal was to move code from `impl.cpp` (mostly) without
functional changes.
* Complete type information is cached with the facet type, and is stored
in a `complete_facet_types()` table.
* Main functional change is to diagnose attempts to use a rewrite
constraint on an associated function. Some existing diagnostics have
been updated.
* Remove `check::Context::RequireDefinedType`:
* For class types, use `RequireCompleteType`
* For facet types, use `RequireCompleteFacetType`
* Introduce a `SemIR::SpecificInterface` to hold an interface and
specific id pair.
* Keep the specific interface ids in the impl object.
* Avoid some extra copies in `Dump` functions.
* Future work missing from this PR:
* Resolving for member access or actions that require impl lookup.
* Resolving rewrites constraints that refer to non-concrete values.
* Any support for adding implied constraints that result from a `where`
clause (though TODOs have been added).
---------
Co-authored-by: Josh L <josh11b@users.noreply.github.com>
Co-authored-by: Richard Smith <richard@metafoo.co.uk>
Co-authored-by: Dana Jansens <danakj@orodu.net>
Treat template bindings as introducing template phase, and propagate it
in the same way we propagate the checked generic phase.
Rename "symbolic" to "checked symbolic" to make room for "template
symbolic". Also rename "phase" to "dependence".
Cycles are defined as reaching two independent lookups in a chain that
have all the same types involved. The acyclic rule states that this is
not possible and results in an error:
https://docs.carbon-lang.dev/docs/design/generics/details.html#acyclic-rule
To do this we need to track the types involved in impl lookup. The
interface constant includes the whole facet type being looked up, which
includes any specific types for generics or where constraints. Thus we
just need to compare the constant ids to look for this condition.
---------
Co-authored-by: josh11b <15258583+josh11b@users.noreply.github.com>
Currently it returns false which just ends typechecking. Instead handle
the error state later and avoid firing overlapping diagnostics in
'extend impl as'.
---------
Co-authored-by: josh11b <15258583+josh11b@users.noreply.github.com>
If 'extend impl' is invalid, mark the impl as invalid by putting an
ErrorInst in the witness_id field.
The construction of the witness_id can otherwise return an ErrorInst but
impl lookup was not checking for that. Now have impl lookup check for an
error there before attempting to deduce generic parameters, which avoids
infinite recursion through deduction in cases like a cyclical impl of
itself.
Adds a testcase for infinite impl-of-itself lookup found by fuzzer.
BuildValueRepr is used to determine the value representation of a type,
but the instruction determining the type may be an indirection through
to another instruction, such as a TupleAccess with `T.0` or a
StructAccess with `T.f`. In these cases, step through the indirection
and try again on the resulting type.
This eliminates a crash as these Access instructions do not resolve to a
type themselves and would otherwise end up in this FATAL line:
```
CARBON_FATAL("Type refers to non-type inst {0}", inst);
```
While here, remove the reference to TupleIndex in typed_insts.h as it
has been subsumed by TupleAccess in 7f930d0f58.
ClassElementAccess is not yet handled, but a fail_todo test is added. It
fails because `ConvertToValueOfType()` in `ExprAsType()` returns a
non-constant value for a ClassElementAccess instruction, where it must
not for a StructAccess.
AsCompatible changes a source instruction's type to a compatible type,
so it also needs its constant value to take on the compatible type.
Otherwise the type of the instruction and its constant value will
differ, which makes moving to the constant value into a lossy
transformation.
Part of #4868
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>
Based on [the lastest thinking on
#4672](https://github.com/carbon-language/carbon-lang/issues/4672#issuecomment-2606209281)
, require a full syntactic match for impl redeclaration, instead of
excluding the `where` restriction. This means no updates to the impl
witness on redeclaration, and no diagnostics that those updates are
consistent.
Not included in this PR, but will need to be done in the future:
* Support for assigning values to associated constants in the body of
the impl definition. This will require moving the checking that
non-function associated constants are set from the definition start to
definition end.
* Identify semantic redeclarations that are not syntactic matches to
give a failed redeclaration diagnostic. This should be done once we are
already identifying impl declarations with the same type structure in
order to require they be identified in an impl_priority/match_first
block.
* Merging of the functions in `check/impl.cpp` that are now always
called together.
Also add some test coverage of `where` parsing I developed in PR I've
now abandoned because of this new simplification of the impl
redeclaration semantics.
---------
Co-authored-by: Josh L <josh11b@users.noreply.github.com>
Co-authored-by: Jon Ross-Perkins <jperkins@google.com>
Add a full entity representation for associated constants, and build a
`Generic` object for them. This `Generic` is parameterized by the
enclosing `Self` type, allowing the use of `Self` within the type of the
associated constant to be supported.
When performing impl lookup for an associated constant, produce the type
with the provided self type substituted for its `Self` along with any
generic parameters of the interface.
Split the handling of associated constant declarations into two parts,
corresponding to the code before the `=`, and the code between the `=`
and `;` (if any). The former goes into the generic declaration region;
the latter into the generic definition region. This prepares us to
handle the default value for an associated constant, but for now we're
just storing the information and not actually using it.
Remove the entity type field from `assoc_entity_type`, because it's
almost unused and is an attractive nuisance -- it must necessarily be a
type in the generic scope of the associated constant rather than in the
scope of the instruction (because there is no `Self` anywhere else),
which means that it's hard to substitute into or derive meaning from.
See `toolchain/check/testdata/impl/assoc_const_self.carbon` for tests of
the new functionality; these used to cause the toolchain to crash.
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.
Changes the name of SemIR `import_ref`s to use the format
`<package>.<entity>`.
<table>
<tr><th>Before</th><th>After</th></tr>
<tr>
<td><code>%import_ref.05a: type</code></td>
<td><code>%Main.D: type</code></td>
</tr>
<tr>
<td><code>%import_ref.8f2: <witness></code></td>
<td><code>%Main.import_ref.8f2: <witness></code></td>
</tr>
</table>
* [Discord discussion in
#toolchain](https://discord.com/channels/655572317891461132/655578254970716160/1330253540999827577)
* Closes#4769
Dana Jansensandjosh11b
Jon Ross-Perkins
Alina Sbirlea
Geoff Romer
Boaz Brickner
Calvin