Instead of tracking the cleanup scope depth on entry to each scope,
track an "ambient" cleanup scope depth that's *after* the destructors of
local variables in that scope. This gets increased to include the
destructors of local variables when we create a name-binding
declaration. Then, when we reach a point where temporaries should be
destroyed, run cleanups that are after the ambient cleanup scope depth
on the stack. This happens:
* At the `;` of a statement expression.
* At the `)` of an `if` or `while` statement.
* After performing the implied `HasValue()` call in a `for` statement.
Per informal agreement with leads, this means we lifetime-extend all
temporaries created in the initializer of a name-binding declaration to
the full scope of that declaration, but that temporaries created in an
expression statement are destroyed at the `;`.
No functionality change right now: we reject thunks where the signature
has no return type and the callee has a return type. But discarding the
expression is still the right thing to do.
Remove calls to `InstStore::GetLocId()` to build a LocId from an InstId
now that they can be constructed directly from the InstId. Most uses of
LocId are just plumbing, so this does not affect them. However places
that want to look inside the LocId do not want to work with the InstId
form. In these places, introduce `InstStore::GetResolvedLocId()` which
converts a LocId (or an InstId as an optimization) into a LocId which is
not backed by an InstId. These locations can be printed (they have a
line and column when they are a NodeId), they can have flags added to
them (`ToImplicit`, `ToTokenOnly`), they can be converted to an
underlying ImportIRInstId, or they may be `None`.
`Dump()` is made to print a resolved location instead of printing the
InstId in the location, since (at least in my experience) the resolved
location is what is interesting in debugging, and this saves manual
`MakeInstId` steps in the debugger every time a location is of interest.
The LocId constructor from InstId is made `explicit` to add clarity to
function calls passing an `inst_id` now directly instead of calling
`context.insts().GetLocId(inst_id)`. To avoid needing to construct
`SemIR::LocId(...)` explicitly in all cases though, the diagnostics code
in Check uses `DiagnosticLocId` as its template parameter which accepts
InstId as well and does the construction of LocId from it.
Because LocId now requires an explicit construction from InstId, any
callers to `AddInst()` functions will have to explicitly convert to
LocId if they had an InstId, but not if they pass a NodeId. To make this
difference clear to callers, we `requires` that the input type can be
converted to LocId. This ensures that passing an InstId results in an
error at the callsite where the InstId is passed, instead of generating
a compiler error when trying to construct `LocIdAndInst` inside
`AddInst()`, which is less clear about what went wrong and doesn't seem
entirely intentional.
---------
Co-authored-by: Jon Ross-Perkins <jperkins@google.com>
This is for consistency with #4120. Similar to that, we can use
overloads on the typed NodeId rather than individually named handlers.
There isn't the same caller benefit here though, since the calls from
check.cpp are already boilerplate.
Most of these are places where we failed to include a header file and
simply never got an error about this. The fix is to include the header
file.
Most other cases are functions that should have been marked `static` but
were not. Finding all of these was a main motivation for me enabling the
warning despite how much work it is.
One complicating factor was that we weren't including the `handle.h` for
all the state-based handler functions. While this isn't a tiny amount of
code, it is just declarations and doesn't add any extra dependencies. It
also lets us have the checking for which functions need to be `static`
and which don't. For the `parse` library I had to add the `handle.h`
header as well, I tried to match the design of it in `check`.
I have also had to work around a bug in the warning, but given the value
it seems to be providing, that seems reasonable. I've filed the bug
upstream: https://github.com/llvm/llvm-project/issues/94138
I also had to use some hacks to work around limitations of Bazel rules
that wrap `cc_library` rules and don't expose `copts`. I filed a bug for
`cc_proto_library` specifically:
~https://github.com/bazelbuild/bazel/issues/22610~https://github.com/bazelbuild/bazel/issues/4446
The purpose of this change is to allow something such as a FunctionDecl
instruction to note an imported instruction as the "loc_id". Note that
doesn't occur here: this change is already very sweeping in edits. There
is no testdata affected, intended to show equivalent behavior.
We might want to consolidate NodeId references towards LocationId, but
if that's preferred, I'd still like to split it out. A lot of this just
piping through LocationId where it's a build error otherwise, enough
that imports should be able to start using it for diagnostics.
ValueStores are added but still unused -- just flushing out structure
for review.
Restructuring SemIRLocation is necessary to use LocationId this way. For
TokenOnly, it's not getting used in Parse, so I migrated it to Check and
it's now specific to SemIRLocation.
I also considered making LocationId reference an InstId (which would
need to be an ImportRef) instead of an ImportIRInstId. However, that
would've required import.cpp to add instructions for decls which are
reached during resolution -- we typically don't have an inst ready for
use. An extra inst is essentially 16 bytes in InstId's ValueStore + 4
bytes in LocationId's ValueStore, whereas this is 8 bytes per.
This was previously discussed at
https://discord.com/channels/655572317891461132/655578254970716160/1209975051588210729.
I'm initiating this mainly because we typically use "id" suffixes to
indicate an `IdBase` being passed around and the non-id suffix of
`parse_node` suggests at it carrying more data than it actually does.
There used to be more reason for avoiding `node_id` because
`SemIR::InstId` used to be named `NodeId`, but that's no longer
necessary. As a consequence, I'd like to rename `parse_node` to more
precisely reflect its type.
In full, this is doing:
```
parse_node_kind -> node_kind
parse_node -> node_id
ParseNodeCategory -> NodeCategory
ParseNodeKind -> NodeKind
ParseNode -> NodeId
```
This is primarily in check and sem_ir, but with some `parse_node_kind`
references in parse too.
Pluralization is consistent with name forms on both sides, so that
wasn't part of my replacements.
The parse nodes are still tracked as part of the same value store
interface in order to ensure parity, but they're split out from Inst
itself in order to reduce the size of Inst -- the expectation is that
they don't need to be passed around quite as much.
This change doesn't actually reduce the passing very much, although
there are hints of it: AddInstAndPush doesn't typically need a separate
parse node from the one on the Inst itself, for example. In a couple
spots I changed code to rely a little more on the InstId until the
ParseNode is needed, but it's very low hanging fruit where done. I think
convert could do more to not eagerly fetch the parse node before its
use, but more cleanup felt it would be easier to handle separately. I'm
currently viewing this as making such cleanup _possible_ rather than
executing on it up-front.
But also, I want to make sure there's a consensus to head in this
direction before pulling the trigger. We speculated that this would
result in the parse node being passed around less, and I do think that's
the case, although it's a bit fuzzy in the change.
---------
Co-authored-by: Richard Smith <richard@metafoo.co.uk>
Goal is to increase type safety, though more work needs to be done (see
added TODOs).
Note that, after this change, check handlers corresponding to deleted
parse node kinds will no longer compile.
---------
Co-authored-by: Chandler Carruth <chandlerc@gmail.com>
Richard Smith
Dana JansensandJon Ross-Perkins
Chandler Carruth
josh11bandChandler Carruth