Doing so results in TODOs in the resulting semir, since we don't handle
combining the facet types together properly or doing lookup into them.
There's a test added demonstrating this, which will be made to work in
followups.
---------
Co-authored-by: Jon Ross-Perkins <jperkins@google.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.
An imported generic has bindings which are of type ImportRefLoaded, and
if they come from another package, they have no entity name attached to
them.
Since a binding name is always a constant-time value, we can get the
constant value instruction for the imported instruction to get a
canonical non-imported instruction. And that one will have a local
`NameId`.
---------
Co-authored-by: josh11b <15258583+josh11b@users.noreply.github.com>
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>
Emit diagnostics for a function declared in a non-owning library, that
is not redeclared (or defined) in the owning library.
---------
Co-authored-by: jonmeow <jperkins@google.com>
In line with the proposal in #4682, this changes the array syntax to be
array(T, N). `array` is a builtin keyword which must be followed by
parens containing two expressions and a separating comma.
The array type expression is still fully builtin, it does not forward to
a Core.Array library type yet. It merely adds the `ArrayType`
instruction, as was done with the previous syntax.
Followup work will change the instruction to reference to Core.Array,
once the library type exists and can be used directly.
---------
Co-authored-by: zygoloid <richard@metafoo.co.uk>
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".
Currently it returns false which just ends typechecking. Instead handle
the error state later and avoid firing overlapping diagnostics in
'extend impl as'.
---------
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.
When a function has a generic FacetType parameter, it can depend on
other FacetTypes bound as earlier parameters. To deduce the FacetValue,
we need to know the impl to attach to it, which requires knowing the
full type signature of the generic FacetType parameter. To do this,
after deducing other arguments to determine the value of non-generic
FacetType parameters, we substitute them sequentially into later
symbolic parameters to get their full facet types, then converts the
arguments to those full facet types to get the FacetValue. For example:
```
fn F(T: type, U: Interface(T));
```
Here the `T` binding is deduced to the be the caller's argument type.
But the Interface(T) can not be properly deduced to a FacetValue in the
first pass, and it will just be the caller's argument type directly.
After the first deduce pass, we will substitute the deduced T binding
into Interface(T), at which point the argument type can and will be
converted to a matching FacetValue as long as an impl can be found.
Closes#4868
This is based on PRs #4881 and #4863
---------
Co-authored-by: Richard Smith <richard@metafoo.co.uk>
Functions with positional parameters omit any implicit or explicit
parameter lists. This causes them to not have a pattern block, which
crashes if there is a return type that needs to add to the pattern
block.
Add a test covering this and handle it by having the ReturnTypeId
handler peek at the node stack and conditionally add the missing pattern
block. To do so it looks to see if the previous node is a
`IdentifierNameNotBeforeParams` which implies it was not expecting a
pattern (since there are no params) and thus the pattern block was not
added to the stack.
Note that lambdas also allow functions to omit an identifier, which will
need a pattern block on the stack for implicit parameters, explicit
parameters or a return type, without seeing any IdentifierName-like
parse nodes. To handle this, we will need to look for additional nodes
in the future and add the missing pattern block to the stack - possibly
for the FunctionInitializer, but the parse support needs to be created
for lambdas first.
AsCompatible changes a source instruction's type to a compatible type,
so it also needs its constant value to take on the compatible type.
Otherwise the type of the instruction and its constant value will
differ, which makes moving to the constant value into a lossy
transformation.
Part of #4868
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
This removes some churn when adding new diagnostic cases to test files
(where previous to this change the newly added newline would cause the
previous diagnostic CHECKs to be updated including changes to the line
number because the CHECK for the blank line meant an extra line between
CHECK and source line).
A few alternatives discussed here:
https://discord.com/channels/655572317891461132/655578254970716160/1329573358475673723
---------
Co-authored-by: Jon Ross-Perkins <jperkins@google.com>
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.
Rename `CheckIsAllowedRedecl` to `DiagnoseIfInvalidRedecl` to try to
better document behavior, and clean up comments.
This extends the no-merge-if-defined behavior to functions. It was
already the case for class/interface, and just added for impl, so if
anything functions were now inconsistent. I was kind of tempted to make
a helper for it, but I didn't think of a great structure/name to get
there: `DiagnoseRedef` isn't always called when it's a redefinition, for
example due to `extern` diagnostics, it's hard to combine.
Cleans up `is_defined` calls to rely more on `has_definition_started`,
removing some code paths that are unused since definitions aren't
merged.
Insert the poison at the same time we do the name lookup to avoid doing
two hash table lookups into each scope. This adds a bit of complication
because import logic now needs to cope with importing a name that is
already poisoned, but the complexity seems worthwhile to reduce the
number of name lookups performed.
This incidentally fixes a bug where we wouldn't poison any name scopes
if we found the name in an enclosing lexical scope, leading to one extra
diagnostic in existing tests.
Part of #4622
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.
This change deliberately breaks away from the line/column ordering, and
instead focuses on a last byte offset corresponding to the final token
processed as part of producing the message. Where that's equal, this
maintains stable ordering in order to reflect the order that diagnostics
were produced.
The intent of this approach is that lex, parse, and check diagnostics
are interleaved based on where they are produced, but that
subexpressions still have diagnostics emitted prior to containing
expressions. In particular, the prior line/column sort essentially
sorted on the _start_ of where a diagnostic was associated, and this is
closer to sorting based on the _end_. As a consequence, something like
`F(1 2)` will have the error for `1 2` emitted _before_ a diagnostic for
`F(1 2)` not matching parameters, instead of _after_.
In check, we track the last handled node. This provides a
last_byte_offset _separate_ from where a diagnostic is associated. The
intent is that this creates an ordering of diagnostics which may be
associated with earlier code, to cause the diagnostics to be emitted
later. An example consequence of this is the change in ordering of
modifier diagnostics: we are diagnosing those from the same place, but
they have the same last_byte_offset, so we print them out in the order
produced.
I've added similar tracking to parse, but cannot identify any test which
is affected by it (note the separate commit, I thought about this late).
I'm not sure whether we have good out-of-order errors we could produce
for this.
A significant number of tests have reordered diagnostics as a
consequence of this change, so this change does not add further testing.
These don't fully work in check and beyond yet, because they're not
added into lexical lookup, but already mostly do the right thing.
Per #3407, disallow namespace declarations anywhere other than at file
scope for now.
We don't treat statements starting with a packaging introducer keyword
(`package`, `library`, `import`) as declarations because they're
sufficiently unlikely to occur that the error recovery doesn't seem
important, and this avoids needing to disambiguate `package.` at the
start of an expression.
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>
* Change `InterfaceWitness` -> `ImplWitness`
* Include a `SpecificId` in the `ImplWitness`. This allows the
`InstBlock` it contains to have its own identity, allowing it to be
changed as the impl is processed. Evaluation only updates the specific.
* Create the `ImplWitness` at the start of the impl definition. In the
future, this will be populated with the values of non-function
associated constants. For now, it starts full of invalid instruction
ids.
* Implements the model suggested in #4672 .
Note that the non-SemIR testdata changes are to these file:
* `toolchain/check/testdata/impl/lookup/fail_todo_undefined_impl.carbon`
* `toolchain/check/testdata/struct/import.carbon`
* `toolchain/check/testdata/tuple/import.carbon`
The last two are due to an import of generics bug exposed by this PR,
which will be fixed in a follow-on.
---------
Co-authored-by: Josh L <josh11b@users.noreply.github.com>
Co-authored-by: Richard Smith <richard@metafoo.co.uk>
Fixes integer builtins to produce the correct values (and not
CHECK-fail) when used on integer literals. Also adds impls to the
prelude to use the new builtins to perform operations on integer
literals.
Perhaps most importantly, this allows directly initializing `i32` values
with negative numbers, as the negation operation on integer literals now
works.
For testing I've added tests for use of literals with one operator in
each class (addition, multiplication, ordering, bitwise, etc) for which
there are distinct rules or overflow behavior, rather than exhaustively
testing all the combinations. This is aimed at finding a good tradeoff
between maintainability of the tests and thorough test coverage.
Also fixes lowering of heterogeneous shifts and comparisons. These are
currently disabled when one of the operands is an integer literal, but
we may want to allow that when the integer literal operand has a known
constant value.
Add `EXTRA-ARGS:` support to file_test, to add arguments without
overriding the default arguments. Use `EXTRA-ARGS: --no-dump-sem-ir` to
turn off SemIR dumping and thus SemIR testing in the int builtin tests,
which validate correct behavior through diagnostics instead.
This doesn't get us any closer to supporting more targeted SemIR dumping
/ testing, but this seems to be a generally useful feature anyway. Most
existing
tests using `ARGS` have been switched over to using `EXTRA-ARGS`.
Requested in review of #4716.
Use a deque to maintain the set of instructions to be walked over. so
that the loop can append more instructions (with their related scope)
during iteration without requiring recursion.
---------
Co-authored-by: jonmeow <jperkins@google.com>
Instead of providing operations only for `i32`, provide them for all
`iN` and `uN` types.
For now, this excludes the `*Assign`, `Inc` and `Dec` interfaces,
because the implementations for those are defined as Carbon functions
rather than builtins, and we can't yet lower definitions for specific
functions, so converting those to be generic breaks the build for our
examples.
Boaz Brickner
Dana JansensandJon Ross-Perkins
Richard Smith
Alina Sbirleaandjonmeow
Geoff RomerandJon Ross-Perkins
Calvin
David Blaikie