In #7436 we stopped substituting `.Self` when collecting witnesses out
of a facet type. While this was correct, it did not capture all the
cases that need to avoid substituting `.Self`. And it poisoned the
`IdentifiedFacetType` cache by not replacing `.Self` but storing the
result in the cache. This led to incoherent behaviour, where the result
of an impl lookup would change depending on which ones had been done
previously.
Now we use a flag to track for each `.Self` if we're currently
type-checking inside the scope where it was introduced in a facet type.
While inside that scope, identify should not replace the `.Self`. Any
use of it should remain as-is since we don't yet know what value will
replace it. We call this state "frozen" since it should not be modified
by identify. This requires a substitution step when we leave the scope
that introduced the `.Self`, to remove the flag. The flag is set in the
`EntityName` of the `SymbolicBinding`, and is part of the canonical
value, since `.Self` can become part of types, which are constants, and
the flag needs to follow it for correct behaviour.
We also have to ensure the flag is the same when doing comparison with
constants from inside a facet type and constants from outside. For
instance in `(Z where .Z1 = ()) where .Z2 = .Z1`, when we arrive at the
second `.Z1` its `.Self` will be frozen, while the `.Z1 = ()` contains a
non-frozen `.Self`. So we add the frozen flag to the first when storing
it in `where_stack` in order to compare the constant values of the two
`.Z1`.
The `WhereExpr` requirement inst kinds now have an `InstConstantKind` of
`AlwaysUnique` instead of `Never`. This allows us to add them to the
usual InstBlocks, and in an `eval fn` body they have a constant value,
so eval does not fail when trying to call that function. We have to be
careful to not consider `AlwaysUnique` as being actually concrete
though, since their constant value erases `.Self`-dependence. This
allows us to stop special casing them when thawing the requirements
block in a `WhereExpr`, and we can just thaw each `InstId` in the block
in a straightforward manner.
We add the new flag to the instruction's fingerprint and name in
formatted semir.
This adds SemIR structs and implements building `observe` lists, as well
as naming, formatting, and importing `observe` declarations.
---------
Co-authored-by: Dana Jansens <danakj@orodu.net>
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>
We exposed `Core.IntLiteral()`, `Core.FloatLiteral()`,
`Core.CharLiteral()`, and `Core.Bool()` as functions as a workaround,
because we had no way to provide the type names without parentheses that
the design requests. But now we can do so, by using an alias. Switch all
of these over from being functions to simply being names of the
corresponding types.
Assisted-by: Gemini via Antigravity
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.
This is a step toward removing the index from `InitForm`, so that equal
form values always have equal representations.
---------
Co-authored-by: Jon Ross-Perkins <jperkins@google.com>
This allows us to capture the location at which a type literal was used,
even in the cases where we don't otherwise need to create a new
instruction to represent the type such as for `char` or `str`.
The logic used to build the underlying type is now marked as desugaring.
For cases such as `iN`, this causes the call to `Core.Int` to no longer
be added as a dedicated IR instruction, and instead its constant value
is used directly as the value of the `type_literal`. This results in
this being on balance a reduction in the size of the IR.
This also fixes a crash in C++ interop when using a `char` literal as a
template argument. The crash was caused by the template argument not
having an associated location when mapping to a C++ location. See
changes to check/testdata/interop/cpp/template/type_param.carbon for an
example that used to crash before this change.
Update alias handling to allow an alias to point at any type literal,
reinstating support for aliases for type literals such as `bool` and
`i32` that had previously worked but stopped working when we
transitioned those types to being defined in the prelude. See changes to
toolchain/check/testdata/alias/builtins.carbon.
All the test changes other than the two mentioned above are mechanical
autoupdate changes switching to the new instruction.
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>
Currently each interface has a `Self` facet internally that becomes a
binding to every entity inside the interface: associated constants,
functions, and require decls. Each of these has to be independently
generic as a result. This makes is challenging in extended name lookup
to move into an extended scope of an interface, as we have a specific
for the interface, but the names within require a different specific
that includes a `Self` facet value.
We generalize this relationship by adding a second generic to Interface,
called `generic_with_self`. When we want to work with entities inside
the interface, we move from the interface-without-specific to the
interface-with-self specific by adding a Self to the specific. This is
done independently of any particular entity inside the Interface, as
those entities are now all members of the interface-with-self generic.
Associated constants no longer need a generic of their own, as they do
not have separate generic bindings. Functions retain a generic, but if
the function has no generic arguments, it will have no bindings of its
own now.
Require decls retain a generic so that their specific can be
instantiated separately from the interface. Requiring the interface to
be complete does not require the types in a require decl to be complete
unless it is modified by `extend`. So we allow them to be completed
later by keeping them in a separate generic.
Named constraints look like interfaces and gain the additional inner
generic-with-self, with the same relationship to require decls.
This removes the need for name lookup to perform Substitution of a Self
facet into the extended scope instruction. Instead, the
`SpecificConstant` instruction inserted by a `require` decl is part of
the interface-with-self generic. When looking through a FacetType for
extended scopes, for each interface, we push the scope with the specific
for the interface-with-self. Then the constant value of the
`SpecificConstant` is correctly modified by the provided self
automatically through applying that specific.
The key changes are:
- Function output parameters are now prefixed with `out`, and more
consistently formatted as named parameters.
- Function and inst output arguments are now written as part of the inst
form, rather than as one of the inst arguments.
As a drive-by fix, this also changes `Temporary::storage_id` from
`DestInstId` to `InstId`, because it doesn't represent an output
parameter of the `Temporary` inst itself.
See the review of
[#6532](https://github.com/carbon-language/carbon-lang/pull/6532) and
[this Discord
discussion](https://discord.com/channels/655572317891461132/999638000126394370/1458268977020141589)
for additional background.
Also clarify and enforce that `ConversionTarget::init_id` is used only
as storage for in-place initialization, and correspondingly rename it to
`storage_id`.
Not all functions have a return slot, and once we have composite forms,
functions will be able to have any number of return slots. Obtaining a
unique return slot for a function only makes sense in `returned var`
handling.
The main changes here are:
- Introducing `InitForm` and `RefForm` to represent initializing and
reference forms (the two return forms currently supported by the
parser).
- Introducing the `FormType` singleton inst to represent their type
(i.e. `Core.Form`).
- Emitting an inst representing a function's declared return form as
part of handling the function signature.
The return form inst is currently ignored. Subsequent PRs will expose it
in `SemIR::Function` and use it to determine the form of call
expressions.
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 is a prerequisite for support for interop with C++ template names.
No behavior change here, except that it sadly changes the fingerprinting
for a lot of tests.
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.
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.
If a `BindSymbolicName` is converted to `type` and then to its exact
`FacetType`, we get a `FacetValue` wrapping the `BindSymbolicName` but
providing no different information: it has the same witnesses and
`FacetType` as the original `BindSymbolicName`. Yet it is a different
constant value, creating multiple canonical forms with the same meaning.
Now we make that `FacetValue` with the same `FacetType` as the
`BindSymbolicName` it wraps evaluate back to the `BindSymbolicName`,
making it the unique canonical form.
This makes the "shortcut" in convert for avoiding impl lookup when
converting from `FacetAccessType` to `FacetType` in this exact scenario
work the same as doing the full impl lookup.
---------
Co-authored-by: Richard Smith <richard@metafoo.co.uk>
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.
If it's just `TypeType` then the `BindSymbolicName` appears directly in
type positions, but if it is replaced with another facet value, then we
would need to insert a `FacetAccessType` around it. By giving it a
`FacetType` type, like other `BindSymbolicName`s we make it consistent
and avoid having to introduce extra instructions.
Generalize the f64 support to support other sizes. Also provide interop
support for `float`, `_Float16`, and `__float128`.
Also lay some groundwork for non-standard floating-point types, though
we don't have any syntax to name them yet.
We add a virtual node (`CompileTimeBindingPatternStart`) as the first
child of `CompileTimeBindingPattern` which holds the identifier
underneath it, so that it is checked just before the type expression of
the `CompileTimeBindingPattern`. When we reach this virtual node during
check, we add `.Self` as a name in the current scope, and when we reach
`CompileTimeBindingPattern` we remove it from scope, which ensures it's
present during only the checking of the type expression for the compile
time pattern.
At the moment the `.Self` has a different type (it's a `TypeType`) than
other `.Self` in the facet type (which are a single `FacetType`), but
the intention is to immediately substitute it out of the facet type
entirely, replacing it with a reference to the compile time binding (a
`BindSymbolicName`) itself. A TODO has been added for this.
---------
Co-authored-by: Jon Ross-Perkins <jperkins@google.com>
Add missing builtins for float compound assignment, for building a
FloatType, and for converting a float literal to FloatType. Switch
`Core.Float` to being a class and add impls for the various
floating-point operators.
---------
Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com>
Co-authored-by: Carbon Infra Bot <carbon-external-infra@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>
Currently if the first operand contains an error, we will return error,
even though the second operands contains a runtime, and it has a
stronger priority (the phase always goes up if possible).
Import is only allowed on instructions with compile-time values, so we
crash if we ever try to import a runtime value. Importable instructions
must diagnose unexpected runtime values and produce errors in the semir
from which they would be imported so that runtime values are never
imported by another semir.
If we had an instruction where you had an error value from the first
operand, and runtime from the second, and we imported it:
- Before https://github.com/carbon-language/carbon-lang/pull/5728 we
would crash in import, but only because we treated errors as runtime
- After https://github.com/carbon-language/carbon-lang/pull/5728 we
would import ErrorInst because we propagate errors. This is desirable
for cases with compile-time values and errors present only.
- After this PR, we would crash again, cuz you're importing a runtime
thing.
This change means that instructions containing an
`InstConstantKind::Never` instruction like`ValueParam` will consistently
evaluate to a runtime value, even if there are errors present. This is
visible in the `BindName` instructions changing in the semir, where they
became constant `ErrorInst` values previously but no longer do.
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.
Previously we walked the global variables defined by the current file
and emitted an LLVM global variable definition for each of them. Now
instead, when emitting a constant reference to a global variable, we
emit an LLVM global variable declaration, and we then subsequently walk
the global variables defined by the current file and convert each of
them from a declaration to a definition.
In order to make import of names of global variables work, add support
for import of `var`, as well as support for importing `tuple_access` and
`tuple_pattern` in the case where the `var` has a tuple pattern in its
declaration. Also treat `bind_name`s that are reference bindings to
`var`s as having the same constant reference value as their `var` so
that we can properly import and lower them.
---------
Co-authored-by: Jon Ross-Perkins <jperkins@google.com>
Co-authored-by: Carbon Infra Bot <carbon-external-infra@google.com>
This was originally needed to support constant evaluation of name
expressions, but that's now done in a different way.
This is actually a step toward treating all patterns as constants. The
upcoming change will do so in a slightly different way, and so it will
simplify the review to start from a baseline where patterns are never
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`.
Factor out logic to evaluate `EntityNameId` instead of duplicating it
between `BindSymbolicName` and `SymbolicBindingPattern`. Remove support
in `SymbolicBindingPattern` for evaluating a pattern to the constant
value of the corresponding binding, which doesn't really make any sense
given that patterns don't generally evaluate to the value that they
matched.
This results in the handling for `SymbolicBindingPattern` being simply
the default handling for an always-constant instruction, so remove the
special case for it entirely and change its constant kind to `Always`.
It's not entirely clear that it makes sense for `SymbolicBindingPattern`
to be treated as a constant when other patterns aren't, but we seem to
be relying on this in various places, so leave it as a constant for now.
Changing it to never be constant will be a smaller change now -- it just
requires changing the `constant_kind`.
The IR changes in the tests are fairly widespread, but mechanical, and
there are two kinds of things changing:
- `symbolic_binding_pattern`s in specifics now evaluate to
`symbolic_binding_pattern`s, not to the argument values. This means in a
few cases we end up with additional `symbolic_binding_pattern`
constants.
- We evaluate the type operand of `symbolic_binding_pattern` now, so an
error in the type will now properly be propagated into an error in the
pattern's constant value.
- 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.
Dana Jansens
Özgür T. ÖnsoyandDana Jansens
Geoff Romer
