When a generic function declaration was encountered for the second or
more time, we would FinishGenericRedecl() for the function decl, but
this just popped the generic region stack and moved on.
The issue with that is when the stack entry is gone, we lose the
symbolic constants from that declaration, and are unable to rewrite them
to point to the actual generic. This left us with a function declaration
with abstract symbolic values that were not useful, and in a function
call we use the declaration attached to the definition, which would be a
declaration with broken symbolic values. Then the function would be
uncallable since deduce would be unable to determine argument types
without the generic bindings.
This resolves the issue for functions, as well as ensuring the correct
generic id from a previous declaration is used for other generic entity
types that have redeclarations.
When a function declaration is qualified, such as defining a class
method outside the class body, we need only the function declaration to
contribute to its generic region stack. The code was collecting constant
values from all qualifier segments together incorrectly.
So when we PushNameQualifierScope(), we also drop the current generic
region stack and rewrite its constant values by calling
FinishGenericRedecl(), and open a new stack entry for the next part of
the qualified declaration.
If a generic declaration somehow has more dependent instruction than a
previous declaration, it would add new instructions to its eval block
with indices beyond the elements in the actual declaration eval block,
since we only store the block from the first declaration found. To avoid
this we plumb through that we are in a redeclaration, and terminate with
an ICE instead of adding new instructions to crash on later.
Fixes#5136.
It's helpful for stability to not have the path in the repo reflected in
test files, something I'm separately running into. So to reduce this,
align INCLUDE-FILE with other split behavior:
- Use the filename (with a "include_files/" subdir to disambiguate),
rather than the full path.
- Note if we eventually want to support splits in these, the same
approach could be extended.
- Only provide as an arg if the user requests files as args.
Factoring AddFile back because it's hard to share; I'm also advocating
to remove the prelude manifest, which would mean the remaining call
could be removed.
If a self type is FacetAccessType, we look through it at the facet
value, both for the query and the impl declaration. This ensures that
while FacetAccessTypes match each other for constant value equality
still, they an also match with facet value queries, such as:
```
impl forall [T:! Y] T as Z {}
^ FacetAccessType for BindSymbolicName of type FacetType(provides Y).
fn F(y:! Y) {
y as Z;
^ Facet value
}
```
Here the facet value and FacetAccessType don't have the same constant
value. Deduction will give the `T!: y` binding the facet value of `y`,
but the `T` in `T as Z` is a FacetAccessType to that binding, which is a
different constant value. Looking through the FacetAccessType gives us
the desired constant value for comparison with the query.
Additionally if the query self value is a FacetValue instruction, look
through that at the underlying type value. Impl lookup is used to
convert from one type or facet value to a new facet value of the desired
facet type. We want facet values to always be able to convert to
everything possible, rather than to have that restricted to just their
current FacetType:
https://github.com/carbon-language/carbon-lang/issues/5137. So this
allows queries such as `(C as Y) as Z` for a class `C` and interfaces
`Y` and `Z`.
Resolves TODO by creating new function `GetAssociatedValue` with the
logic from `PerformCompoundMemberAccess` restricted to the
non-instance-binding case.
---------
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.
According to approved syntax at
https://github.com/carbon-language/carbon-lang/blob/trunk/proposals/p3848.md#syntax-defined,
`fn F[]` without explicit parameters should be valid. This makes it
work, then adds some validation to prevent `class C[]` in check.
Note that for `fn`, positional parameters are a TODO -- but this allows
me to test validation in `fn destroy[]` which is rejected, not just a
TODO.
New planned direction is to not require completeness. Updated comments
to reflect that some care will be needed once named constraints are
supported. Long term plan is [discussed in
#5089](https://github.com/carbon-language/carbon-lang/pull/5089#discussion_r1985908453):
> We have some options. In our last conversation, it sounded like it
would be beneficial for named constraints to have their own witnesses,
with entries in declaration order. This would allow accesses to the
named constraint while it was being defined. So there would be something
of a hierarchy in a facet type witness, with a named constraint taking a
single slot in a facet type witness, independent of its definition.
Co-authored-by: Josh L <josh11b@users.noreply.github.com>
When converting from a facet value (an instruction whose type is
FacetType), we require making a FacetAccessType
to have a type instruction when building the resulting FacetValue.
Otherwise, the conversion is the same for values of type TypeType, and
we relax the convert function to support either.
Corrects the test expectations for converting `Goat as Animal`, a facet
value of type FacetType, into `Eats`, a facet type of type TypeType.
This would be a promotion in the typish hierarchy which is incorrect. We
had an extra case in Convert that was handling this, and it's now
removed. `Animal`, a facet type, does still correctly convert into
`Eats`, a facet type, if `impl Animal as Eats` exists.
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
This changes the SemIR of invalid redeclarations, because previously
they lacked a name. We've avoided this in diagnostics so it doesn't
otherwise come up, but I plan to use it for more easily validating
redeclarations.
This allows the numbering of the parameters to match when checking for a
valid redeclaration. It also prepares us to produce the proper numbering
when generating a thunk.
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>
This makes inline and out-of-line functions produce the same type.
I was looking at this and wasn't sure if it was deliberate. It seems
desirable to reuse the type when possible, since it's probably cheaper
too.
The INCLUDE-FILE option is only used in the toolchain tests for now. If
specified in a file test, the given file path is added to the test's
arguments. For toolchain tests this makes the file's package available
to the test. The `--custom-core` command line flag is added to the
driver, which avoids adding the production `Core` package to the command
line. Together, these allow a test to provide their own minimal `Core`
package.
For example, this would replace `Core` with the package and prelude in
`facet_types.carbon`.
```
// INCLUDE-FILE: toolchain/testing/min_prelude/facet_types.carbon
// EXTRA-ARGS: --custom-core
```
To support this:
* //testing knows how to parse INCLUDE-FILE out of the header of a test
file.
* //testing adds the file to the virtual file system, and includes it in
the test's arguments.
* //toolchain/driver grows the --custom-core command line flag to avoid
loading the production `Core` package.
Tests that were creating their own minimal prelude to define BitAnd on
types are now pointed to
toolchain/testing/min_prelude/facet_types.carbon as the prelude. They no
longer need to `import Core` in each test as a result.
Such tests are no longer `no_prelude`, but instead have their own
prelude. So they are moved to a `min_prelude` subdirectory.
Closes#5076
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>
Exclude SemIR since the point of the test is that it diagnoses something
invalid, not that any particular SemIR is produced.
Co-authored-by: Josh L <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>
- 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.
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>
Parameter lists need substantially different treatment than tuple
patterns in other contexts, so this change splits them into separate
parse node kinds.
---------
Co-authored-by: Jon Ross-Perkins <jperkins@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>
In order to have the name available for diagnostics, we now always set
`NameId` in `NameContext` and put `poisoning_loc_id` as part of the
union with `resolved_inst_id` instead (since we never need both).
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".
A value of type FacetAccessType can convert to a facet value of a target
FacetType if the value's underlying facet value's FacetType is
compatible.
A value of type FacetType can convert to another value of type FacetType
if the value's FacetType is compatible with the target FacetType.
During impl lookup, the comparison of the lookup type and the impl's
type needs to consider more than strict equality. If the impl's self
type is a FacetAccessType, we instead need to verify that the FacetType
of the lookup type and of the impl's self type are compatible (which is
like a problem of another impl lookup). For now we check that they are
equal by unwrapping the FacetAccessType to the constant facet value
within, and compare that with the lookup type.
---------
Co-authored-by: josh11b <15258583+josh11b@users.noreply.github.com>
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.
If a type satisfies the requirements of a FacetType, then that type as a
value can be converted to a FacetValue, which binds the type value to
the FacetType.
For instance, if the class A implements an interface B, then
```
fn F(b:! B) {}
```
can be called with the type `A`
```
F(A);
```
This does not handle yet receiving non-type values matching a FacetType,
as that requires deducing the required FacetValue for the caller's
argument. Follow-up work will do this step.
---------
Co-authored-by: josh11b <15258583+josh11b@users.noreply.github.com>
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>
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 Jansens
Jon Ross-Perkins
josh11bandJosh L
Richard Smith
Alina Sbirlea
Geoff RomerandRichard Smith
Boaz Brickner
Calvin