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>
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.
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>
* 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>
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.
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>
Instead of treating `Core.Int` as the toolchain's builtin `IntType`,
model it as a class that adapts the builtin type. This aligns us better
with the intended language model, gives an associated library for
`impl`s involving `Core.Int` to live within, and opens the door adding
member functions to `Core.Int` if we decide that is desirable.
Remarkably it also seems to make the formatted SemIR a little smaller,
because a call to a generic class generates less IR than a call to a
function.
When a generic requires a symbolic type to be complete, add a new
`require_complete_type` instruction to the generic eval block. During
monomorphization of such an instruction, require that type to be
complete.
The offsets were originally added to deal with churn from builtins in
the raw semir. In textual semir, we mostly see instruction IDs for
imports, and builtins have also settled down more.
On imports, where possible, use the `EntityNameId` for an import instead
of printing an instruction. Next, show the source location if we have a
node. Only show the instruction if there's no location.
This also exposes `Parse::Tree` and `TokenizedBuffer`, so that we can
pass a `SemIR::File` without the component parts. In particular this
allows us to get the `TokenizedBuffer` for import IRs without
substantial structural modifications. We may want to make these optional
for serialized `SemIR` later, but the nodes/tokens contain source
location, which we'd need for debug information -- so it's not clear how
much we can really make them optional without substantial information
loss.
Reduce arguments to just `File` in a few spots, as a result of the
accompanying `TokenizedBuffer` and `Parse::Tree`. Also updates style to
pass around `const File*` where the reference is maintained, instead of
`const File&`.
I was considering keeping a direct reference to the tree and tokens on
`Context`, but initially my thought was it wouldn't make much
difference. I can re-add those if desired, just as direct caching of the
`File` fields.
This fixes a crash in lowering, at least (though only initializes the
vptr to
null for now) - certainly open to naming feedback on the instruction, or
the
exact semantics (we could have a global vptr instruction that's
referenced from
the existing instructions for reading globals, for instance).
I guess we'll want one type parameter for the vptr_init instruction,
which is
the type that this is a vptr for? (can do that here or in a follow-on
patch)
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.
Represent the type as an `InstId` rather than as a `TypeId` to preserve
how it was written and better support tracking its value in a generic.
Add accessors to `Class` to get the base and adapted type to reduce code
duplication, and add `TypeStore::GetObjectRepr` to make it easier to map
from a type to its possibly-adapted object representation type. In
passing, also move `GetIntTypeInfo` and `GetUnqualifiedType` into
`TypeStore`.
This fixes specifics of generic adapters to properly look at the
specific adapted type, and also fixes importing of adapters.
---------
Co-authored-by: Jon Ross-Perkins <jperkins@google.com>
We considered a couple of other options for this:
* https://github.com/carbon-language/carbon-lang/pull/4515 Keep the
`ElementIndex` numbering vptr-ignorant, and do +1 offsets as needed -
seems subtle/easy to miss
* https://github.com/carbon-language/carbon-lang/pull/4517 Always have a
zeroth element in the object representation, make it zero-size in the
case of no-vptr - @zygoloid was concerned this would add overhead
especially to stateless objects used in type-trait-like things.
But currently moving forward with this direction - of initializing field
indexes with an invalid value until the end of the class definition,
then assigning field indexes during construction of the class's object
representation struct type. This direction might reinforce/help avoid
premature access to the object representation before the class is
complete, and give a single place where class layout is done (at class
completion) if we want to add more options there, such as class layout
optimizations, etc.
This patch still has problems with object initialization (that #4515
does not have/does address) but does address normal `obj.member` access
correctly.
---------
Co-authored-by: Richard Smith <richard@metafoo.co.uk>
- 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>
* The `extended_scopes` in a `NameScope` were represented by a
`NameScopeId`. Replace that with an `InstId` of an instruction returning
the type that is extending this name scope.
* `Context::LookupQualifiedName` now can take multiple scopes to look
in.
* `GetAsLookupScope` was moved out of `member_access.cpp` and is now
`Context::AppendLookupScopesForConstant`
This PR also fixes some existing issues that were revealed as part of
writing and testing this PR:
* Additional validation and handling of invalid ids.
* `extend impl` in a class is not properly imported yet, but at least
now it doesn't crash.
The change to use an `InstId` also allowed some diagnostics and
formatting to be improved.
---------
Co-authored-by: Josh L <josh11b@users.noreply.github.com>
The test update shows a class derived from an imported base class with a
vptr, and without this change the derived class got its own vptr, with
this change the derived class can see the base is dynamic, so the
derived doesn't need to add a vptr and can rely on the base class's vptr
instead.
The cehck for vptr could be done differently (is this class dynamic and
its base class non-dynamic), and if we change the layout (to put the
vptr after any base) then this code will break (could add an assertion
that the vptr isn't present apart from at the start of the field list if
that'd seem worthwhile).
There's still later issues with lowering (if this patch causes crashes
in lowering, do they result in fuzz failures/need to be avoided before
this change is submitted?)
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>
Also surround it in square brackets rather than parentheses. This
matches the format used by Clang and GCC, and means diagnostics will
still match the `file:line:col: error: ` pattern used by some IDE tools.
Before:
```console
fail_builtins.carbon:11:11: error(AliasRequiresNameRef): alias initializer must be a name reference
```
After:
```console
fail_builtins.carbon:11:11: error: alias initializer must be a name reference [AliasRequiresNameRef]
```
Also tighten up test regex to only match on `STDERR` lines that list a
file name.
This is to help identify which diagnostics we're actually using.
Note that driver/testdata still has tests which don't pass this flag,
and so continue to test the kind-less (default) behavior.
Per discussion on #toolchain, add "s" as a special-case for the common
plural format.
Note this removes periods from a few diagnostics; the periods shouldn't
be there per message style. Also, while I'm ignoring llvm::StringLiteral
uses, those should be addressed as #4416 -- this'll probably conflict
and make me clean up one or the other.
A small step to virtual functions - adding vtable pointers to the
layout, but not initializing or otherwise using them at this stage.
A few open design questions I'd love feedback on:
* Is this the right/good enough SemIR representation for now? This patch
adds a `is_dynamic` attribute to `SemIR::Class` and populates/flags it
based on the flag of the base class, or if any virtual function is
declared in the class (or, at least that's my intent). Some other
options include:
* Each `Class` could store a `ClassId` (or `TypeId`?) of the (possibly
indirect, possibly self) base class that is the first one that is
dynamic/has a vtable pointer
* Could make the property narrower, like `has vtable pointer` and have
it `true` only on the type that introduces the vtable - then derived
classes would have to walk their base classes to check if they're the
one that needs to define the vtable pointer or not
* Should the vtable be the first element in the type? If there's a
non-dynamic base type, we could have a layout that's `{<non-dynamic base
type>, vtable ptr, <derived members>}`? Derived types would still be
able to uniquely identify where their vtable pointer is just fine... -
and the vtable pointer is, in a sense, a member of that intermediate
type, so it does seem a bit strange to force it to the front - but I
guess it's probably more efficient in some ways?
Open to any other suggestions/advice/thoughts on the direction, etc.
---------
Co-authored-by: Richard Smith <richard@metafoo.co.uk>
Introduces the `BindingPattern` and `SymbolicBindingPattern` insts, and
a separate stack of pattern blocks that they are emitted into. The
intent is to generate the corresponding pattern-matching insts (like
`BindName`) from them in a separate pass, but that is deferred to future
PRs.
See
[here](https://docs.google.com/document/d/1U_vQH17V893J9aF1LJXUnFYBNSs2MjKl4bJPaWCB2zo/edit?usp=sharing&resourcekey=0-w0xGYZ0An31Kpz-wvzSXwQ)
for the design this is based on, but note that during review we have
chosen to deviate from that design by putting the patterns in separate
blocks, and omitting the "forward references" from a `BindingPattern` to
its corresponding `BindName`. This in turn necessitates having separate
inst kinds for symbolic and non-symbolic binding patterns.
---------
Co-authored-by: Jon Ross-Perkins <jperkins@google.com>
Add support for initializing types like `GenericClass(i32)` from a
struct literal. A new kind of instruction, `complete_type_witness`, is
added to the class definition to track the object representation type so
that it's visible to the generics machinery. Accesses to the object
representation of a class have all been updated to pass in the class's
`SpecificId` so that the types of the fields of the specific class are
used instead of the types of the fields of the generic class in places
that look at the object representation -- primarily class
initialization.
I guess this technically would also allow code to pass check that hasn't
before, and that isn't covered by tests (since it's masked by other
failures in the tests that already test this functionality) - should I
add another test/add some code to a valid test case?
Also, this'll miscompile in lowering, since there's no support there yet
- should I do anything about that to make lowering fail in some way? Or
is it acceptable that some things just silently mis-lower? (I could add
a currently-miscompiling test case too, to demonstrate this? (not sure
if the autogenerated tests leave space for comments that would explain
that the currently-tested behavior is incorrect?))
Is the addition to EntityWithParamsBase suitable? of course not all
functions can be virtual, so it's a wasted bit at the moment for all
those cases (though it's free, since it's bitpacked - but as we want to
add more bits in there it might not be a scalable solution)?
---------
Co-authored-by: Jon Ross-Perkins <jperkins@google.com>
Boaz Brickner
Richard Smith
Calvin
Geoff RomerandRichard Smith
Dana Jansens
Brymer MenesesandJon Ross-Perkins