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 deducing arguments for generic parameters of an `impl`, the
deduction calls `Convert` on the input arguments. Often, the input
argument is a facet, and needs to be converted to a type via
FacetAccessType in order to produce a different facet. These
instructions end up being added to the semir, but only their constant
values are needed for the resulting specific returned from Deduce.
In the best case, these extra instructions are just noise in the semir,
or they just cause instruction names to get differentiated with larger
suffixes.
In the worst case, these extra instructions contain references to
instructions from a generic context, and leak them out of that generic
context and into another. In particular, when importing a
LookupImplWitness instruction, the re-evaluation of it can do deduce
(when the lookup is against a generic `impl`). The instructions created
in Deduce are not part of the import, and end up referring to imported
instructions from the local context, which leads to confusion in the
toolchain, and can crash.
The `import_self_specific.carbon` test demonstrates this. It causes the
`I.F` function to be imported from the `I` interface when building the
witness table for the `impl`. Doing so imports the specific of `C` which
includes a LookupImplWitness for `Self.Accoc` in `I`. The `Self` is a
BindSymbolicName with generic binding index 0, in `I`. When Convert
creates instructions in the generic `impl forall D`, however, they end
up referencing and including this BindSymbolicName into its eval block.
But the generic binding 0 in the `impl` is a very different thing (a
value of type `E`). This confusion leads to crashes.
The SymbolicBindingType refers to the type value that will be
substituted in for the BindSymbolicName, but holds onto the EntityNameId
from the BindSymbolicName instead of (or in addition to, for now) the
instruction.
The EntityNameId will be used to look in the ScopeStack to find the
witnesses either from the BindSymbolicName instruction, or other
instructions that specify `impls` constraints against the EntityName.
This will allow us to have the `T` in `I(T)` resolve to a `.Self`
reference in the type so that we get type equality with the binding's
type: `T:! I(.Self)`.
This also does a little restructuring in the same direction, following
#6124.
Leads want `Destroy` to work similarly now for all types. As a
consequence, there doesn't seem to be as much benefit to splitting off
aggregate destruction. In this PR, the `type.destroy` function can now
be expected to destroy anything that's destructible; that means it'll be
usable for the `final fn` once that support is available.
Similarly, this gets rid of the impls other than the single blanket
impl, now using `type.can_destroy`. Since they all need to use the same
function, there's no benefit to splitting approaches. Also, now it can
just be a `final impl` since there should be no need for people to
create specializations -- if this blanket impl applies, it means the
`final fn` is the same.
This also slips in `partial` support since there's no reason to have it
diverge anymore. Also `abstract`, which I'm not sure is broadly testable
since most cases it'd come up, the `abstract` keyword is explicitly
detected/rejected.
Note though that this doesn't make any really big changes. It's just
realigning on the leads decision. I'm going this way to try to reduce
name-related churn for other changes.
Locations are similarly fragile, because adding a comment changes them.
This has made me pause when making prelude changes in #6144, so dropping
them for those cases.
Instruction ids aren't actually that interesting outside debugging, and
can be churny when doing other structural changes. I've seen this in
particular when doing singleton changes, which bump every instruction
id.
Note there are still other ways fragility from locations can crop up.
This shouldn't be considered a complete fix, but hopefully a small
improvement.
Right now, the class destroy impl is incorrectly generated (first
discussed [in
Discord](https://discord.com/channels/655572317891461132/941071822756143115/1418614787449032826)).
If we want it to be correct, deferred definition logic would need to be
added, and the declaration would need to be moved inside the `class`
scope (along with whatever generic logic that needs).
This instead switches to a blanket impl, to avoid creating latent bugs
with generating the `impl` and function body in the wrong scope. This
approach uses the same blanket impl as aggregate destruction that was
added by #6098.
The intent here is to allow progress on other parts of `Destroy`. For
example, under this model the implementation of the function body could
be done as part of lowering the specific.
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>
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 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`.
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.
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.
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>
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>
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 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 2.5s to 2s on my machine, which
is now ~50% faster than before
https://github.com/carbon-language/carbon-lang/pull/5653 slowed things
down by adding a lot of stuff to the production prelude.
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>
Where `--no-dump-sem-ir` is used, change to `--dump-sem-ir-ranges=only`.
Otherwise, add `--dump-sem-ir-ranges=if-present` with a TODO to change
to `only`.
Note, SemIR is affected just because the extra comments change line
numbers in files where splits aren't in use.
- Track the `VarPattern` instruction on the `VarStorage` instruction so
that it's available for name mangling.
- Mangle global variables based on the first binding name within their
pattern.
- Give global variables external rather than internal linkage, except if
they have no bindings whatsoever in their pattern.
- To support lowering references to bindings nested within a global var,
such as for `var (x: i32, b: i32)`, add some basic initial support for
reference constant expressions. Treat a global `var` as a reference
constant, and treat an aggregate access into a reference constant as a
reference constant.
In preparation for shifting from `TypeId`s potentially representing
attached types to always representing unattached types, using
[terminology suggested on
Discord](https://discord.com/channels/655572317891461132/963846118964350976/1359286326779973712).
This change causes us to track slightly more type spelling information
through SemIR.
One change that has significant impact on the SemIR output is that we
now build a `struct_type` instruction in each class representing the
types of the fields, including the spelling used for those types. This
is now no longer always identical to the corresponding canonical
`struct_type` for the object representation, so it's built separately
and owned by the class.
Also remove `TypeBlock` support entirely, as its only use was
representing `TupleType`s, which now use an `InstBlock`.
Each of these types takes another type as an operand. Instead of storing
that other type as a `TypeId`, store it as an `InstId` so that we can
track how it was written, not only its canonical form.
The canonical constant values of these types continue to store the
canonical constant values of their operands, as normal.
---------
Co-authored-by: Dana Jansens <danakj@orodu.net>
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.
Add a Vtable typed inst with a type_id (of the type this vtable applies
to) and list of virtual function decls (or import refs to function
object constants).
This doesn't add lowering/emission of the vtable, or usage when
initializing objects of the type.
Some questions in case they're interesting to discuss:
* is it right/worth having the type_id in the vtable? (probably makes it
easier to emit - using the type to get the class name to figure out the
mangled name for the vtable) perhaps it should be a ClassId?
* I'm thinking the logic in CheckCompleteClassType could be the place we
handle diagnostics for mismatched keywords (virtual/abstract for a
function that's already virtual/abstract, maybe checking for non-virtual
functions with the same name in a base class, or derived class functions
without `impl`, etc) - but we could move some of that to the moment we
walk the function decl, and record our findings in the function decl
(record the base function it overrides, or the index of the vtable to
slot to use when building the vtable at the end of the class)
* the Vtable typed inst has `constant_kind = InstConstantKind::Always`
and `is_lowered = false`, I think I added that in to workaround/address
some failures in lowering. And seems correct for this intermediate step
- I'll add lowering in a follow-up patch. But the constant_kind - what
should this be? We can just say all vtables are of VtableType (in which
case the `Always` constant kind sounds right to me) or we could have
them introduce a type with each virtual function as a named member,
even?
---------
Co-authored-by: Richard Smith <richard@metafoo.co.uk>
Co-authored-by: Jon Ross-Perkins <jperkins@google.com>
When printing a Class, the complete_type_witness was printed last but
this gave a somewhat misleading representation as it appeared to be part
of the !members label. Move it above the label so that the label more
clearly refers to everything below it.
Include the index rather than the name in the fingerprint of a symbolic
binding. While both the index and the name contribute to the canonical
identity, using either one of them in the fingerprint is sufficient to
ensure that distinct entities get different fingerprints. Changing the
name of a symbolic binding should ideally not result in fingerprint
changes, so exclude the name from the fingerprint when we have an index.
Use the canonical type and constraint when fingerprinting an impl, so
that uses of names in `name_ref` instructions aren't considered, only
the entity the name resolves to, and different ways of spelling the same
type have the same fingerprint. This similarly allows compatible changes
to be made to impls without changing the fingerprint.
Exclude the declaration block when determining the fingerprint of a
declaration. The declaration block contains the declarations of
parameters of the declaration, which do affect whether two declarations
are identical, but not whether they denote the same entity, because it
would be invalid to have different declaration blocks for declarations
with the same name in the same scope. Therefore changes to the
declaration block are compatible, and it's useful for such changes to
not affect the fingerprint.
This is not easy to test in isolation with our current testing
machinery. However, a follow-on PR will change the name of a parameter
in the prelude, and with this in place, will not cause any changes to
occur elsewhere in the toolchain tests.
Use it in the instruction namer to make instruction names more stable
across unrelated changes to the toolchain or the prelude.
---------
Co-authored-by: Dana Jansens <danakj@orodu.net>
This is a precondition for enabling the new pattern-matching subsystem
to support binding patterns that have `if` expressions in the type
position.
---------
Co-authored-by: Richard Smith <richard@metafoo.co.uk>
Goal is to reduce churn in names in test updates (by churning a lot of
them in this PR).
---------
Co-authored-by: Josh L <josh11b@users.noreply.github.com>
This seems slightly redundant for a locally-defined class, where there
will be a complete_type_witness instruction earlier in the class, but is
important for imported classes, where we're currently doing the wrong
thing in a way that's invisible in formatted SemIR.
- Do not include entities imported from files that we are not dumping.
- Do not include constants and import_refs that are not referenced by
something that we are including in the formatted output.
---------
Co-authored-by: Jon Ross-Perkins <jperkins@google.com>
Co-authored-by: Carbon Infra Bot <carbon-external-infra@google.com>
This is in particular to avoid churn from changes such as #4370. I think
the import list can be helpful (particularly to understand what the
library is aware of), but it's a different trade-off for the prelude
package due to the implicit imports.
This PR makes it so that types can implement the `IndexWith` interface
so that they can provide their custom indexing behavior.
---------
Co-authored-by: Jon Ross-Perkins <jperkins@google.com>
Geoff Romer
Dana Jansens
Jon Ross-Perkins
Richard Smith
Boaz Brickner
David Blaikie
Brymer MenesesandJon Ross-Perkins