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.
Modifiers came to mind since we had a bit of discussion way back when,
about whether parse or check was the best place to do validation. Trying
to capture the tradeoffs to consider here.
Note, I'm writing this up mainly because I was asked why I don't reject
`fn destroy;` in parse twice (but still allowing things like `fn
destroy();` or `fn destroy[]();`), so I've been thinking a reference of
the higher-level parsing philosophy would be helpful.
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.
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.
Currently file tests for LSP must provide carbon source code as an
escaped string within notify params, i.e.
```
[[@LSP-NOTIFY:textDocument/didOpen:
"textDocument": {
"uri": "file:/class.carbon",
"languageId": "carbon",
"text": "class A {\n fn F();\n fn G() {}\n}\n"
}
]]
```
This works fine for simple, single line files but gets annoying when
working with more complicated files which are necessary when testing
more complicated features e.g. goto-definition
```
--- class.carbon
class A {
fn F();
fn G() {}
}
--- STDIN
[[@LSP-NOTIFY:textDocument/didOpen:
"textDocument": {
"uri": "file:/class.carbon",
"languageId": "carbon",
"text": "AUTOFILL"
}
]]
```
This PR extends file_test_base to be able to parse the notify/call
params and inject files from the test_file's splits into the JSON input.
I purposely avoid using the clangd types and manually parse the
llvm::json::Value here to avoid introducing a depdendency on clangd to
the generic file_test_base, but happy to change if we think that is
fine. Also happy to accept other suggestions on alternative methods to
achieve same result.
---------
Co-authored-by: Jon Ross-Perkins <jperkins@google.com>
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 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 doesn't change functionality, but I was seeing better diagnostics
in VS Code.
This also changes the NodeId constructors for related types (also
NodeCategory and NodeIdForKind) to use UnsafeMake for construction. That
originated from avoiding ambiguity coming from `requires`, but the
constructor mode is also one we should typically avoid (e.g., preferring
`Parse::Tree::As`).
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>
Trying to break apart the function on reasonable boundaries because it's
big. Changes behavior of class functions with virtual modifiers to
remove the modifier when diagnosing, which is more consistent with other
modifier diagnostics.
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).
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>
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>
Resolve instruction id depending on the context from which a function is
called.
Calling a function in a function context that does not have a definition
emitted before reaching lowering will cause a crash. This needs a change
in check/eval layer.
Now that BitAnd for types exists, we should get an error about type
conversion, not about BitAnd not existing. Add `type.and` to the Core
package in the test in order to improve the error message.
This test will likely be made to pass by #5060.
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>
While current examples of this could also be addressed by emitting
declarations on use (by which stage the associated types would have to
be complete by construction) - it's expected that future examples
(vtables, function pointers) will need to work in this case anyway, so
might as well implement this feature.
Fix destructor syntax ambiguity by switching to `fn destroy` mirroring
standard function syntax. This is a purely syntactic change, maintaining
destructor semantics.
This comes from leads question #4999
---------
Co-authored-by: Chandler Carruth <chandlerc@gmail.com>
We only had the single `clang` symlink, but in case its useful to use
the toolchain with some other build system that expects `clang++`, or
even `clang-cl` or `clang-cpp`, fill in the rest of the symlinks.
The different `clang` flavors don't really need anything to support in
the subcommand as there is already an excellent way to get the exact
behavior of these names using Clang's `--driver-mode` flag, so these
just use that. That makes this change really *only* about busybox
behavior.
We don't really have a dedicated test path for things that are only
exposed via the symlinks, so I've added a simple Python integration test
we can use for that. I can backfill some testing of other symlinks if
useful (the `ld.lld` one might be worthwhile), although there is minimal
interesting logic to cover there.
- 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.
Alina Sbirlea
josh11bandJosh L
Dana Jansens
Jon Ross-Perkins
DavidLoftusandJon Ross-Perkins
Richard Smith
Boaz Brickner
Geoff RomerandRichard Smith
David Blaikie
