This avoids import cycles, and reduces the number of temporary vectors
we build (and potentially throw away on retry). Import the self specific
when importing a generic, now that there's no risk that will introduce
cycles.
Note that we could take the same approach to import classes, interfaces,
and so on, instead of the current third phase of resolution for those
instructions, but in this PR I'm just addressing the import cycle I'm
currently seeing in a work-in-progress PR.
TryEvalInst was assuming this to be the case when forming canonical
constants, but it previously wasn't.
This fixes an issue where we can end up with two identical-looking
constants for an empty struct value: one with an `Empty` block and
another with the canonical empty block.
It's actually possible to get into all four combinations of having
parameter lists versus being generic:
- An entity nested within a generic, such as a member class, can be
generic even if it has no parameters.
- As a corner case, an entity with an *empty* parameter list has
parameter lists, but isn't a generic because it doesn't have any generic
parameters.
A symbolic constant has an instruction to compute the constant value, as
well as potentially also having a generic ID and an index within that
generic to indicate where corresponding values can be found in a
specific. Import those pieces of information when importing such a
constant.
We try to import the generic before we start the main work of importing
the constant, and retry the import process if importing the generic adds
work to the worklist. This means that the first time we import anything
within a generic, we can now perform three passes calling
`TryResolveInst` instead of two, but the first pass is very lightweight
and only looks up and adds a single instruction, so the added overhead
of the extra pass should be minimal.
To avoid introducing cycles when importing a generic function, make the
import of a function declaration build the new `Function`,
`FunctionDecl`, and `FunctionType` in the first pass, like classes and
interfaces do.
Import generics and specifics when they are referenced by imported
entities.
When importing a generic, we import the symbolic constants required by
its eval block, and then rebuild the eval block itself given the list of
constants it needs to compute. This is likely a bit less efficient than
directly importing the contents of the eval block, but avoids needing to
either extend the importer code to be able to import the instructions
that can appear in the eval block or extend the evaluator to cope with
instructions from a different `SemIR::File`.
Importing a symbolic constant is unaffected, and does not yet preserve
the associated generic and index within that generic, so uses of a
generic from an imported IR still don't pick up values from the
specific, but the improved functionality can be seen in the changes to
the SemIR in the testcases.
Instead of updating the `value_id` on the canonical constant
`BindSymbolicName` to refer to some particular instance of that
constant, create a new instruction, and attach the proper location to
it.
This implements a few closely related features:
- Starts merging namespaces discovered inside imports.
- Stores results of cross-package name lookup as an entry inside the
scope.
- Note this is particularly visible with `i32`.
- Moves more of the imported instructions to the import scope.
Note this is primarily for executing the namespace TODO in check.cpp,
which is removed here.
`testdata/namespace/merging_with_indirections.carbon` tests key
behavior.
---------
Co-authored-by: Richard Smith <richard@metafoo.co.uk>
When evaluating within the context of a specific, we can encounter uses
of bindings that are nested within that specific, for example parts of
the declaration of a nested generic. Those bindings should evaluate to
the canonical form of themselves, as they would when evaluating outside
the context of the specific.
Fixes#4157.
Instead of reusing instructions from the generic entity in the eval
block, rebuild constants in the same way we rebuild types. The previous
attempt to not rebuild these constants assumed that every constant used
in a generic would be built in that generic, and not referenced directly
or referenced from some enclosing scope, which isn't true in practice
and is a fragile assumption in any case.
We could add back some reuse of instructions from the generic -- if we
happen to see the right instruction to build a constant, we could
opportunistically reuse it -- but given the complexity added by doing
so, I'm not pursuing that here.
Now that the eval block for a generic consists of instructions uniquely
owned by that generic, rather than often being shared with another
entity, include the generic in the formatted SemIR output. I'm using the
same scope name for the generic object itself as for the parameterized
class / function / interface, because there are very frequently
references between them and this keeps the IR simpler and more readable,
and avoids needing to invent a second name for the scope.
Note this isn't implementing checking through imports. The parse node
there is harder to access through the context, so would require
examining the entity in order to get the import declaration, to get at
the ImportIR. We also don't have a parse tree attached in that case, and
would need to add one to SemIR::File. But I believe we do want to add
that, so it's explicitly a TODO.
Note GetTokenText re-lexes literal values, so there's a bit of potential
overhead there. Not sure if we want a more efficient manner for
comparing in cases like this.
Also adds import_ir_scope to namespace formatting. I'd done this as an
aid for #4153, and am splitting it out.
---------
Co-authored-by: Geoff Romer <gromer@google.com>
Changes crash messages to start printing verbose forms of instructions,
rather than just the ID. Fixes some indentation issues with stacks. Also
switches unexpected inst formatting, because now there are lots, and
it'd be helpful to know where they are.
This uses a pimpl pattern for Formatter due to the number of member
functions on Formatter. Maybe we should refactor that, but this didn't
feel like a good place to do so.
Note, I have two concerns about this change... to note them here, to
make sure others are considering them when evaluating the
implementation:
1. Some instructions are very verbose to print, as evidenced by the
fn_decl printing (which includes function params) or scope printing
(which includes scope members).
- I'm not sure whether there's a way to simply reduce this, as it seems
essential to the requested printing of instructions.
- Long-term, we may at least want to limit the number of lines printed
here. However, I've already spent a fair amount of time here and I think
it's in a good state to evaluate.
2. Increased complexity in the crash handler may result in crash
messages failing to generate.
- For example, a crash in Formatter (and its deps, such as InstNamer or
location handling) prevents a stack from being printed. I'm pretty sure
I've written crashes in Formatter before.
Here's an example crash snippet (generated by adding a crash inside
`return` handling) before:
```
2. NodeStack:
0. FunctionDefinitionStart -> function2
1. ReturnStatementStart -> no value
2. IntLiteral -> inst+26
inst_block_stack_:
0. block<invalid> {inst+0, inst+1, inst+2, inst+23}
1. block9 {inst+26}
param_and_arg_refs_stack:
args_type_info_stack_:
```
And after:
```
2. Check::Context
NodeStack:
0. FunctionDefinitionStart: function2
1. ReturnStatementStart: no value
2. IntLiteral:
unexpected.inst+26.loc12_10: i32 = int_literal 0 [template = constants.%.2]
inst_block_stack_:
0. block<invalid> {
package: <namespace> = namespace [template] {
.Core = unexpected.inst+2
.F = unexpected.inst+23.loc11_22
}
unexpected.inst+1 = import Core
unexpected.inst+2: <namespace> = namespace unexpected.inst+1, [template] {}
unexpected.inst+23.loc11_22: %F.type = fn_decl @F [template = constants.%F] {
unexpected.inst+9.loc11_9: init type = call constants.%Bool() [template = bool]
unexpected.inst+10.loc11_9: type = value_of_initializer unexpected.inst+9.loc11_9 [template = bool]
unexpected.inst+11.loc11_9: type = converted unexpected.inst+9.loc11_9, unexpected.inst+10.loc11_9 [template = bool]
unexpected.inst+12.loc11_6: bool = param b
@F.%b: bool = bind_name b, unexpected.inst+12.loc11_6
unexpected.inst+19.loc11_18: init type = call constants.%Int32() [template = i32]
unexpected.inst+20.loc11_18: type = value_of_initializer unexpected.inst+19.loc11_18 [template = i32]
unexpected.inst+21.loc11_18: type = converted unexpected.inst+19.loc11_18, unexpected.inst+20.loc11_18 [template = i32]
@F.%return: ref i32 = var <return slot>
}
}
1. block9 {
unexpected.inst+26.loc12_10: i32 = int_literal 0 [template = constants.%.2]
}
param_and_arg_refs_stack:
args_type_info_stack_:
```
When forming a specific (previously called a generic instance), evaluate
the eval block of the generic to determine the values of any constants
used in that specific. The majority of the work here is updating
eval.cpp so that it can use the results of prior evaluations in the same
block when computing later values.
Include the computed results in the formatted SemIR output.
---------
Co-authored-by: Jon Ross-Perkins <jperkins@google.com>
We can't use the instruction from the generic directly, because it
doesn't have the right constant value. Instead add an instruction that
models the transition from the constant value in the generic to the
constant value in the generic instance.
Also start associating the self generic instance with unqualified
lookups that find results in an enclosing generic, so that we track the
information necessary to create the new instruction.
---------
Co-authored-by: Jon Ross-Perkins <jperkins@google.com>
Require types into which qualified lookup is performed to be completely
defined. Eventually this will trigger substitution into the definition
for generic types.
For each generic, build a list of instructions describing the
computations we need to do when resolving an instance of the generic:
this is a list of the instance-specific constants and types that the
generic uses. Another way of viewing this list is as a block of Carbon
SemIR code that is evaluated in order to form an instance of the generic
-- this is referenced in the code as the "eval block" for the generic.
For each instruction in the generic whose type or value is a symbolic
constant, replace that type or constant value with a symbolic reference
that says "to find the actual type or value, look at index N in the list
of values for the generic instance".
For an instruction with a symbolic constant value, we can just add that
instruction to our list. For an instruction with a symbolic constant
type, however, we may not have a corresponding instruction computing the
type within the generic and may need to build a new instruction, but
will reuse one where possible. In the case where we build a new
instruction, we use the existing substitution code to build the type
within the eval block.
For now, this transformation is only done in the declaration region of
the generic, not in the definition region. Also, we map back from the
symbolic references to the underlying constant value in a few places
where we will eventually need to do a lookup into a generic instance, in
order to avoid regressing the tests.
This executes on a TODO in AddImportRef to add instructions to their own
block instead of the File block. This has an important consequence of
removing a pattern from InstBlockStack that added to blocks not
currently at the top, cleaning up an issue for ArrayStack. The delta
here is then mostly in different formatting of the import refs, a
consequence of the separation.
By adding an `ImportDecl` instruction, this creates something that can
be referenced through `ImportIRInst`.
packages/no_prelude/implicit_imports_entities.carbon is getting a test
of this (import_conflict and import_conflict_reverse).
Also re-packs ImportIR from 24 bytes to 16 on 64-bit, since I'm touching
everywhere that makes one anyways.
This refactors how the implicit import is handled in order to retain
more name scope information. As a consequence, private access control
works better between api files and implementation files. Note though
that this will also be essential for name poisoning between the API and
implementation, as discussed in #3763.
In implementing this, I ran into a couple issues with namespaces that I
think point to flaws in their handling. I've fixed some and added a TODO
for the biggest issue (in check.cpp line 281-288), which relates to the
handling of namespaces of direct imports which are first evaluated
indirectly.
In a `class C(T:! type)`, the type `Self` should be `C(T)`, not merely
`C`. Similarly, in an `interface I(T:! type)`, the type of self should
be `I(T)`, not merely `I`.
In `ClassType`s and `InterfaceType`s, track a `GenericInstanceId` for
the instance rather than just the argument list.
---------
Co-authored-by: Jon Ross-Perkins <jperkins@google.com>
Build a `Generic` object for generic functions. This object tracks the
generic parameters that are in scope for the generic entity. Eventually
it will track other information about the generic too.
Add basic SemIR formatting support for generic functions.
Name scopes store the names in their scope in a `DenseMap`. Several
places reasonably avoid depending on the iteration order by sorting the
names -- they're in the formatting code path where that's a solid
approach.
Unfortunately, when we're importing one scope into another, we also need
to walk the entire scope and do something for each name. =[ This doesn't
seem like a great place to sort things to stabilize them.
I've switched to a fairly simplistic solution of having a vector of name
entries that can be iterated stably, and a separate map for lookups. I
didn't use the set-of-indices trick here because it's not clear that's
the right trade-off for a scope: likely a lot of small scopes here with
relatively hot name lookups. And the key here isn't a large or
dynamically sized thing that we're canonicalizing, it's a `NameId`. That
made me lean towards duplicating the name in the hashtable for lookup
and the vector for iteration.
I thought about a fancy approach of sorting the hashtable keys by their
values (the indices), but that would still require a bit of copying and
more code.
I also thought a bit about other optimizations, but decided to leave a
comment for now -- it's not obvious to me exactly how hot this is and
whether it's better served by faster lookups, being more memory dense,
etc. And that might involve more of an SOA layout change or some other
approach. Rather than do that here, and especially before switching
hashtables, I stuck with a simple approach to address the ordering.
---------
Co-authored-by: Richard Smith <richard@metafoo.co.uk>
Model such calls as call operations, and compute the resulting type in
constant evaluation rather than in type-checking. This means we no
longer form non-constant `ClassType` or `InterfaceType` values, and that
we produce an `<error>` type in the case of bad argument lists rather
than a broken / meaningless `ClassType` / `InterfaceType` that doesn't
actually represent a type. This in turn suppresses some follow-on
diagnostics.
In some cases this is just a refactoring; in others, we no longer import
both an instruction in no block plus a constant instruction, resulting
in creating fewer instructions. The non-constant version of the
instruction always ended up unreferenced in the affected cases.
Following up on a comment in #4042.
Merges handle_let.cpp and handle_variable.cpp into a single file in
order to take better advantage of commonalities.
Both still have a bunch of kludges, and there's still divergent handling
in handle_binding_pattern that will yield different results. However,
this fixes some things about `let` like starting to make use of
global_init (imperfectly, due to how VarStorage works, I grant), and in
particular adding `let` names to a name scope, not just lexical lookup.
This is mostly mechanically duplicating work done for generic classes to
also support generic interfaces.
Also fix both generic interfaces and generic classes to support
importing class and interface types with arguments from another file.
---------
Co-authored-by: Jon Ross-Perkins <jperkins@google.com>
Co-authored-by: Carbon Infra Bot <carbon-external-infra@google.com>
Don't use the pretty-printed type name, because that's intended for
diagnostics, not for a theoretically machine-readable format like SemIR.
Types are always constants, so omit the leading `constant.` on the type
instruction name.
Don't format the *type of* the function `F` as `"F"`. Instead use
`"<type of F>"`.
Also improve instruction naming for function and generic class name
values: name the value after the function or generic class, and add a
`.type` suffix to the type.
Adds access to the name lookup table in name scopes. This is so that we
can quickly check access during name lookup without resolving the entity
itself. Does this for names in general, but does not implement handling
for entity-scoped names, only namespace-scoped names (where they're
essentially just not exported).
Excludes `private` names from exports. Although names should be
accessible to `impl` files, that's not implemented here because we'll
probably want to do it by directly copying name lookup tables.
Check the parameters specified in a name qualifier against the
parameters of the entity that the qualifier refers to.
For interfaces, this required adding minimal support for parameterized
interface names.
---------
Co-authored-by: Jon Ross-Perkins <jperkins@google.com>
This has is a nice-to-have for me. Frequently I want to run a specific
test, and end up digging through output to be able to copy-paste the run
line. This uses TIP lines to inject the command into the file when using
AUTOUPDATE.
Note, one of the reasons I want this is because "bazel test
//toolchain/testing:file_test --test_output=all" has been regularly
exceeding bazel's output limit for me (workaround is either opening the
output file or specifying an obscure output limit flag), making it a
little harder for me to get the commands. However, frequently I'm adding
a file and want to iterate on it, so that's really the use case I have
in mind here.
Previously we did some of this in decl_name_stack and some of it in the
callers of decl_name_stack. Factor out a single place to pop a name and
its optional following parameters.
Part of making this behavior consistent is that we now track whether an
implicit parameter list was present or absent rather than mapping an
absent list to `InstBlockId::Empty`. This improves our redeclaration
checking and the precision of some diagnostics.
Parse the name of a declaration as a sequence of `NameQualifier`s --
which have a name, possibly parameters, and a trailing period --
followed by a name and possibly parameters. This prepares us for parsing
declarations of members of generic classes and similar cases, but
actually supporting such member redeclarations is left to a future
change.
We previously required functions to have parameters, but no longer do,
following the direction of #3848. Cases like namespaces that can't
actually have parameters are now diagnosed in check instead of in parse.
---------
Co-authored-by: Jon Ross-Perkins <jperkins@google.com>
Modifies the core package and literal handling to use factory functions
for standard type literals.
Updates function/builtin/import.carbon to stop depending on the prelude,
since it would list all the impls in the core file. Updates
alias/builtins.carbon to be failing (the type values cannot be aliased).
Note, I assume this doesn't affect all the TODOs (like for i32.carbon),
but does cut some things down (and requires updating some tests that
have prelude name conflicts, but I think they were intended to be
updated this way).
Starts warning on duplicate "export name" because I'm updating name
lookup to make it work.
Need to think harder about how to handle cross-package "export import".
But adding some failing tests to sketch out what *should* work.
Change syntax for package declaration to put the `impl` keyword at the
start and remove the `api` keyword.
To support this, rearrange processing of package, library, and import
declarations to use the general modifier handling support in declaration
parsing rather than special-case logic.
There is an ambiguity in `impl package.Foo as Bar`, which we resolve by
treating `package` as an introducer after a modifier only if it's not
followed by `.`.
The toolchain was iterating a map of package name to import list in a
couple of places to do things beyond counting or other order-invariant
operations. The result was that the specific hashtable iteration order
influenced the order of SemIR generated (and other behaviors like
diagnostic emission I suspect, but the source location sorting probably
hid this). Switching to a hashtable implementation with any order
seeding that changes run-to-run immediately shows the SemIR order
fluctuating without this.
This PR fixes that by instead accumulating the package imports data in a
vector and using a map to vector indices. This is also slightly more
efficient, although that seems unlikely to be an important factor here.
There was only one insertion point so I've just hand coded the
management of the indices and map, but happy to take a different
approach or use an abstraction here if desired.
One awkward aspect of this is that some of the loops need access to the
package name as well. I've just added storage for that as these seem
unlikely to be huge arrays of 10s of 1000s of imported packages, so the
double storage of the identifier ID seems likely OK. But again, happy to
take a different approach here if desired. It also seems like it might
be possible to work out the identifier from the node, but I kept the
patch more direct for simplicity.
I've also not used the LLVM `MapVector` abstraction of this pattern.
This was mostly to avoid adding another layer of abstractions to our
data structures, and because this is the first time we've hit this
really. My experience is also that it is reasonably often that there is
a more efficient way to orient the vector and map than what is
automatically provided. But that may just be my experience.
This adds a `BindExport` instruction in order to better track the
location of the `export` itself, but a `bind_name_id` is also added to
`ImportRef` so that we know quickly where to put it in name lookup.
Merging identical names is a TODO. I haven't quite decided how best to
achieve that, because I do think the BindExport should be what's
actually added to name lookup.
Also, I will probably add a mode to DeclNameStack that blocks
non-namespace scopes. This seems to already be an error, but the wrong
one (maybe due to lack of support for cross-file decl/def support).
Richard Smith
Jon Ross-PerkinsandRichard Smith
Chandler CarruthandRichard Smith
