The bulk of this change is changing most pattern insts to be `Always`
rather than `AlwaysUnique` constants, so that they can be wrapped in
`SpecificConstant`s to perform substitution. That then lets thunking
rely much more on `SpecificConstant` wrappers instead of deep-copying
the inst tree with modified types.
This approach to thunking should scale better, particularly as things
like form generics make function signatures more complex, because we can
leverage the existing support for constant evaluation and substitution.
Unfortunately, applying this approach to binding patterns will require
more work; see the TODO near the top of `thunk.cpp` for details.
---------
Co-authored-by: Richard Smith <richard@metafoo.co.uk>
Fix import logic to make all imported packages be children of the
`NameScopeId::Package` scope. Previously, indirectly-imported packages
would end up as children of their importing package's scope, which
resulted in them not being treated as packages at all, and in particular
not being fingerprinted as packages.
Fixing that caused a failure in the fingerprinting logic as we started
to encounter packages with no correspoding import scopes. Instead of
looking for import scopes, use a simpler mechanism to map packages to
their package names, and clean up.
Unfortunately the latter change churns all the fingerprints again :(
Hopefully this is the last time for a while.
When we import from another library in the same package, its entities
end up with our library as their parent scope, resulting in cross-file
fingerprint mismatches. Instead, only include the library ID when
fingerprinting either a package-private entity or an `ImportIRId` that
refers to a particular `SemIR::File`.
Fixes link failures when referencing a symbol involving a fingerprint
from a different package.
Previously we included the `Namespace`'s `import_id` as part of its
fingerprint, which caused local and imported namespaces to get different
fingerprints. We now store the `import_id` on the `NameScope` instead of
on the `Namespace` inst to avoid this problem.
Also, when we reach a package-level `NameScopeId`, consistently
fingerprint it as a (package name, library name) pair. Previously the
fingerprinting depended on whether it was imported or not, as an
imported `NameScopeId` had a parent scope (the current package). We need
to include the library name here so that private entities with the same
name in different libraries have different fingerprints.
Class vars are still restricted to simple `name: type` bindings, not
full patterns. This is now handled in the check phase instead of during
parsing.
This is in preparation for supporting `static var`.
This shifts logic a little so that empty top-level scopes are printed
less often. This affects imports mainly for now, but should be expected
to affect the soon-to-be-added generated scope more significantly.
Assisted-by: Google Antigravity with Gemini 3 Flash
Implementation of unused pattern bindings #2022, continued.
Whereas previous PR #6460 took care of parsing, and PR #6479 prepared
the stage by using _ in some test cases, this PR has the the actual
implementation, using a simple dataflow analysis.
---------
Co-authored-by: Burak Emir <bqe@google.com>
Co-authored-by: jonmeow <jperkins@google.com>
When doing name lookup into an extended scope of an interface or named
constraint, the containing scope has an inner `Self` facet which can
appear in the specific of the extended scope. For instance a constraint
`N` which requires an interface `Z(Self)`:
```js
constraint N {
extend require impls Z(Self);
}
```
When doing member lookup into a facet constrained by `N`, we need to
find the specific interface `Z(...)` where the `Self` is replaced by the
self-type the member lookup is happening on in order for impl lookup to
find a witness later.
Inside that specific interface we repeat the name lookup to find an
associated entity. Then to produce a witness we perform impl lookup
against the specific interface that name lookup returned with the
self-type of the member access. So if we do member access into `A:! N`
for a member `F`, like `A.F`, we would be doing impl lookup with a query
self of `A` and looking for the interface `Z(...)` returned from name
lookup.
When impl lookup has a facet as the query self, which we do here as `A`,
it takes its type (a facet type) and identifies it to find all the
required interfaces, and it substitutes the query self into those
specific interfaces for `Self`. If the `Z(...)` we acquired from name
lookup is `Z(Self)` it will fail the lookup for `A as Z(Self)`, since in
the facet type of `A` it finds a witness for `Z(A)` instead.
Thus, we replace the inner `Self` in extended scopes, such as `N`, with
the self-type of the member access, which produces the extended scope
`Z(A)` for this example. This allows the impl lookup for `A as Z(A)` to
find a witness from the facet type of `A`.
In order to do this, we include an instruction for the inner self when
registering the extended scope. Then, when we find the extended scope in
name lookup, we can use its CompileTimeBindIndex to replace any instance
of that `Self` facet with a new facet. If the self-type of member access
is a type, we construct a FacetValue with an empty facet type that
refers to the type.
Pursuant to recent decisions on #6124, switch `Destroy` to use a
`CustomWitness` for its implementation. Right now this is manufacturing
no-op implementation functions on each lookup, which obviously isn't
ideal but is intended as a first pass. I'm mostly trying to find the
right balance between updating the approach to reflect new decisions,
while still breaking apart work in a way.
The `CoreInterface` logic is intended to build on `CoreIdentifier`
support. We have a number of additional interfaces that require
specialized logic, and that'll extend pretty far with C++ interop, so it
seemed easiest to have a generic function for it. That's what's
replacing the logic inside C++ interop that was doing string comparisons
(which could have already been moved to `CoreIdentifier`, I just missed
it in my first pass).
This adds `CustomWitness` support because the `Destroy` witnesses can be
imported cross-file. `CustomWitness` was previously only used for C++
types, which don't yet support import, which is why that wasn't
previously an issue. The addition of `query_specific_interface_id` is
similarly needed in order to get correct sorting of witness blocks when
imported.
This PR also removes builtin constraint logic (note this is in a
separate commit to help review; it's not a separate PR because it's
difficult to split apart without tests breaking). This had been made
generic with the expectation that destroy, copy, move, and conversions
would all need related support. Under the new decision, we are not going
to do blanket impls and will instead just manufacture a `CustomWitness`
for everything.
A lot of SemIR fingerprints change, but that's probably because the
addition of `Destroy` on core classes is yielding structural changes.
Instead of naming the root namespace `package` (because it's accessed by
the `package` keyword), change it to use the current package name. Note,
buried in the checksum changes,
`toolchain/check/testdata/package_expr/fail_not_found.carbon`:
```
- // CHECK:STDERR: fail_not_found.carbon:[[@LINE+4]]:16: error: member name `x` not found in `package` [MemberNameNotFoundInInstScope]
+ // CHECK:STDERR: fail_not_found.carbon:[[@LINE+4]]:16: error: member name `x` not found in `Main` [MemberNameNotFoundInInstScope]
```
for:
```
// CHECK:STDERR: var y: i32 = package.x;
// CHECK:STDERR: ^~~~~~~~~
```
I'll leave it to you if you prefer this; the alternative I see is to
just rename `IsCorePackage` to `IsImportedCorePackage`, and/or change it
to a helper that takes a `Context` and does the right thing with
`parse_tree` (which, I need for `Destroy`-related reasons and was my
default approach).
This resolves a TODO in `expr_info.cpp` by using the inst kind rather
than the bound value to track the binding's category.
Since we're churning all the `bind_name` insts in testdata anyway, I'm
also taking this opportunity to align the inst naming with the design's
terminology, by calling these insts "bindings" (this aspect of the PR is
dependent on #6231 resolving an ambiguity in that terminology). For
consistency we'll need to rename several other insts as well (see the
TODO on `RefBinding`); I'm deferring that to a separate PR to minimize
the review load, but I think those name changes are in-scope for this
review.
When we do member access `x.F` we attempt to look into the type of `x`
for `F`. If `x` is a facet, we look at its FacetType to find `F`. We
want `facet` and `facet as type` expressions to be generally treated as
equivalent, so we need to get at the "canonical facet value" of `x` in
order to look into its type. Add a new operation that allows us to
preserve the non-canonical `base_id` in the member access for
diagnostics if no conversion to a canonical facet value was needed.
The main direction of this change is the edits to `destroy.carbon`
(matching in both prelude and min_prelude).
Previously there was a no-op blanket impl for `Destroy`, which hid all
missing implementations of `Destroy`. This does a few things:
- Sets up builtin aggregate destruction for struct and tuple types as
before, but also adds C++ class types and array types to the same
handling. (all as a TODO for actual implementation)
- Also maybe-unformed destruction, for now at least. (there's a chance I
may try a different approach on this, but the impl lookup wasn't working
as I'd hope in order to write it in code)
- Adds handlers for simple things that are easy to do in code: `type`,
`bool`, pointers. (because these are no-op destruction)
- Redirect `const T` destruction to `T` destruction.
This leaves as future issues:
- `partial T` destruction. (this can't be done similar to `const`
because it only works for non-`final` class types; I think `class`
definitions should just generate what's needed)
- Destruction of other prelude-provided types. (will probably come up as
we implement class destruction, that the adapted builtin type doesn't
implement `Destroy` -- but may end up special-casing that in a way that
moots it)
This moves the `&` operator from `facet_types.carbon` to
`convert.carbon` because more things need to handle type and now that
we're getting separate copy and destroy interfaces. It should be
low-cost (an interface and builtin) so hopefully this is the right
balance for complexity and re-use.
A few tests are also edited in order to focus them more on what they
intend to test, and avoid a `Destroy` dependency.
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>
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`.
Now that included files can specify `EXTRA-ARGS`, this uses that to
handle min_prelude files. Also moves `As`/`ImplicitAs` out to a shared
file, partly because we duplicate it a few times over, partly just to
show that it works.
Also removes the `min_prelude/` subdirectories because many of these
files were touched by autoupdate regardless. I noticed one conflict for
`impl_thunk.carbon`, so renaming that one to
`impl_thunk_min_prelude.carbon`.
`GetArgReplacements` I simply noticed was unused, so removing it.
Geoff Romer
Nicholas Bishop
Jon Ross-Perkins
Dana Jansens