This is a preparation change for adding name poisoning support
(https://github.com/carbon-language/carbon-lang/issues/4622), which is
expected to require more elaborate logic around NameScope since a name
can be not defined yet, defined, or poisoned.
The API separates looking up a name from getting the full entry since we
have cases where the entries are invalidated between the time we're
looking for the name and when we access (and sometimes modify) the
entry.
This change has the following benefits:
* `names` and `name_map` are internal to `NameScope` and are guaranteed
to match.
* `extended_scopes` and `import_ir_scopes` can not be manipulated (only
new scopes can be added).
* `inst_id`, `name_id` and `parent_scope_id` are constants.
* `has_error` can only be mutated from false to true.
---------
Co-authored-by: jonmeow <jperkins@google.com>
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)
Actually presenting two options in this one review - if you look at the
specific
commits in this PR, the first commit represents my first attempt - and
if you
look at the overall PR change for the second attempt.
But I'm totally open to completely different approaches/ideas - these
were just
my rough guesses.
Extends the set of function signatures that support being given a
builtin definition to include cases where a parameter or return type is
an adapter for a supported type. For example, if we can give a builtin
definition to `Add(a: i32, b: i32) -> i32`, then we can also give a
builtin definition to `Add(a: MyI32, b: MyI32) - >MyI32` where `MyI32`
adapts `i32`.
This is a prerequisite for changing `Core.Int` to be a class type that
adapts the builtin int type.
Also make `-gsimple-template-names` lldb-only due to it tripping up gdb
in some cases (I came across it breaking SmallVector pretty printing
where gdb wouldn't associate a simplified type named declaration with a
simplified type named definition in another translation unit - seems gdb
can associate a type decl/def when it sees both (if you step into both
translation units or otherwise trigger gdb loading/parsing them) but it
doesn't seem able to /search/ for the type definition). Filed
https://sourceware.org/bugzilla/show_bug.cgi?id=32421 for this.
A couple of other things I'd like to do, but don't know how:
* It might be nice to allow opting into or out of gdb_index (with the
default being 'on' for gdb_flags, but you could opt out). But doesn't
seem super important.
* We should turn off fission by default, it seems - bazel has trouble
making the .dwo files available at the same path as is in the binary
especially on partial rebuilds. (not sure if we can do that, I guess
we can make fission a no-op/doesn't add any flags, even if we can't
change the fission default in bazel itself)
* can we have gdb_flags imply/disable lldb_flags? (so you can use
--features=gdb_flags without always having to add
--features=-lldb_flags)
This slightly improves the handling of adapters, by making them copyable
in some cases when their adapted type is copyable.
Refactor some of the repeated checks for properties of value
representations.
In passing, move all the adapter tests in check/testdata/class to a
subdirectory since we now have quite a few of them.
I believe our flags enable the warning by default, it's just that it
doesn't catch this in clang-16 (maybe more; I reproduced with clang-18
and didn't keep digging). For example:
```
toolchain/parse/tree_test.cpp:86:28: error: ISO C++20 considers use of overloaded operator '==' (with operand types 'value_type' (aka 'Carbon::Parse::NodeIdInCategory<Carbon::Parse::NodeCategory::Decl>') and 'AnyDeclId' (aka 'NodeIdInCategory<NodeCategory::Decl>')) to be ambiguous despite there being a unique best viable function [-Werror,-Wambiguous-reversed-operator]
86 | EXPECT_TRUE(*any_decl_id == any_decl_id2);
| ~~~~~~~~~~~~ ^ ~~~~~~~~~~~~
```
The different `operator==` approach works except for with
`Parse::NodeId::Invalid`, which seems easy to replace with a
`.is_valid()` check.
This removes a lot of boilerplate `Print` functions in favor of a
CRTP-based approach that uses a `Label` field as an automatic prefix.
This `Label` is also made available for other purposes, particularly
`IdKind` crash messages in this change. In particular, for
`RequireIdKind` in node_stack.h from using numeric IdKinds (e.g., 5 and
24) to something that will print `IdKind(<label>)` (this came up
recently on #toolchain).
While I'm in here, also doing some other tinkering:
- Moving operators to be `friend` members, to reduce the extra
templating now that the base types are templated.
- Adjusts IntId diagnostics from `int [...]` to `int(...)` for
consistency with other id printing.
- Changes InstBlockId's label from "block" to "inst_block", since we
have multiple blocks now.
- Fixes StructTypeFieldsId to use "struct_type_fields" instead of
"type_block" (from `TypeBlockId`)
- Does some more adjustments from camelCase to snake_case for
consistency
Issue was not properly handling `ImportRef` instructions in
`AddAssociatedEntities` in `check/import_ref.cpp`. Using `CARBON_CHECK`
instead of `CARBON_FATAL` was hiding the error.
---------
Co-authored-by: Josh L <josh11b@users.noreply.github.com>
`ids.h` and `ids.cpp` are the manual edits, everything else is
search-and-replace.
The full list of things moved is:
- `TypeId::TypeType`
- `TypeId::AutoType`
- `TypeId::Error`
- `ConstantId::Error`
This is to unblock removing `InstId::Builtin*`.
* Rewrite constraints are stored in a facet type, substituted, imported,
and formatted.
* We now distinguish `.Self` from other symbolic bindings in two ways:
* `.Self` itself now has an invalid compile time binding index (since it
doesn't bind to any of the generic parameters). As a result, we no
longer need to create a generic region in `handle_where.cpp`.
* There is a new phase tracking values that are only symbolic because
they transitively depend on `.Self`. This allows us to give the result
of a `where` expression template phase as long as it doesn't use any
symbolic constants other than `.Self` or other designators.
* `AddConstant` has been removed from `check/context` since it was only
used from `eval`. This meant less plumbing of the phase change.
* Evaluation of `BindSymbolicName` now also performs substitution into
its type.
* Include a bit more information in some diagnostics.
* `StringifyTypeExpr` outputs rewrites, which required adding support
for associated entities as well.
* Associated entities now have an entity name set when importing.
* Adds tests for some interesting cases with rewrites and uses of
`.Self` mixed with other symbolic constants.
Still to do:
* There is no validation that any particular type satisfies rewrite
constraints.
* Access to members of a facet type do not see the rewritten values.
* Impls don't recognize whether associated constants have rewrites
setting their values.
* No support for resolving facet types.
---------
Co-authored-by: Josh L <josh11b@users.noreply.github.com>
Co-authored-by: Richard Smith <richard@metafoo.co.uk>
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>
The Any[...] instruction group structs have a field layout that matches
the specific typed instructions that it groups together. However those
specific instructions may have different field types, or different
numbers of fields, making it impossible for the Any[...] instruction
group to match them all at once.
In this case, it can use the AnyRawId field type to represent that the
specific typed instructions have different field types in that position,
or may not have a field at all.
Previously we used `int32_t` as this polymorphic placeholder type, which
was implicit and worked somewhat magically:
- It was not listed in IdKind
- It was was not implemented explicitly for either FromRaw or ToRaw
However it happened to work because:
- FromRaw<T> was implemented for every T, and would build as long as T
was constructible from the raw id value, which is int32_t.
- In As<AnyGroupInstruction> for converting a specific instruction to a
group instruction: For any given id field type T in the specific type
struct, it would be converted to its raw (int32_t) value, then the
AnyGroupInstruction field would be constructed with FromRaw<int32_t>
since the polymorphic field type was int32_t.
- Of course int32_t is constructible from int32_t.
- And if the specific instruction did not have a field in the matching
position, the AnyGroupInstruction's field would be default
constructed, and int32_t is default constructible.
This allowed As<AnyGroupInstruction> to construct the
AnyGroupInstruction type
from each of its specific instruction types regardless of what field
types they had.
We can make this more explicit by using a type other than int32_t. We
introduce the AnyRawId specifically for the purpose of being a
polymorphic field type in Any[...] instruction groups.
The AnyRawId type _is_ part of IdKind, removing the need for a special
case in the documentation and rules about field types. And this allows
us to `require` that T is in IdKind for FromRaw<T>.
- This also pointed out that FromRaw and ToRaw do not need to be
implemented for BuiltinTypeKind anymore, as this is no longer a field
type in any typed instructions, further simplifying the rules to not
need any exceptions for what is a valid field type.
The AnyRawId type participates in FromRaw by being a member of IdKind
and being constructible from int32_t.
The AnyRawId type is default constuctible so that when the specific
typed instruction has no field in the matching position, it will be
default-constructed with the InstId::InvalidIndex value.
The AnyRawId type does _not_ participate in ToRaw, and this is
documented on the type. This is because conversion from specific typed
instruction to an instruction group is lossy (due to the polymorphism
which requires the AnyRawId type). As such conversion from the
instruction group to a specific instruction is not possible, and thus
the Any[...] instruction group does not need to support being converted
to raw id values.
This is rebased on #4604.
---------
Co-authored-by: Richard Smith <richard@metafoo.co.uk>
In llvm 17 the _LIBCPP_ENABLE_ASSERTIONS flag was split into two:
- _LIBCPP_ENABLE_HARDENED_MODE for fast checks
- _LIBCPP_ENABLE_DEBUG_MODE for expensive checks
We kept HARDENED_MODE enabled in debug, but we can also turn it on for
opt builds.
In llvm 18, the _LIBCPP_ENABLE_HARDENED_MODE was further split into 4
settings, NONE, FAST, EXTENSIVE, DEBUG. As seen in the recent blog post
https://security.googleblog.com/2024/11/retrofitting-spatial-safety-to-hundreds.html
the FAST hardening mode is indeed very fast and has minimal impact,
while helping to catch a lot of bugs.
So we enable the FAST checks in opt builds, and EXTENSIVE checks in
debug builds.
The EXTENSIVE checks are the same that Chrome enables in every build
configuration, so we could consider enabling it in opt builds as well:
https://source.chromium.org/chromium/chromium/src/+/main:build/config/compiler/BUILD.gn;l=1127;drc=a8260dee097dde71ca4464c0c8d897a80c353db2
Adds a singleton framework, and converts `File` and `InstId::Print` to
demonstrate functionality. Moves various functions from `ids.h` to
`ids.cpp` because `Inst::Print` needs the file if `singleton_insts.h` is
split out, so the small bit of cleanup feels consistent.
I added `Inst::MakeSingleton` because getting the type of the
instruction to make from an `InstKind` felt too hard. Singleton
instructions follow a basic structure, so I'm just putting that
instruction structure into `Inst`. Previously we required macros to do
this, and I'm trying to remove macro dependencies.
I'm trying to remove builtin/singleton-related functionality from
`InstId` in order to get a clearer boundary for the functionality. The
other builtin functions should be removed as part of the bigger
migration, but I'm trying to carefully scope changes to verify agreement
on the singleton approach in use first.
This provides `InstT::SingletonInstId` because that'll often be written
as `SemIR::TypeType::SingletonInstId`. The alternative of something like
`SemIR::SingletonInstId<SemIR::TypeType>` is just a little more verbose
due to the repeated `SemIR`, and it's more consistent with
`TypeType::Kind`.
An alternative I considered was consolidating singleton information to
`InstKind`. This felt challenging because of the
`InstId::BuiltinTypeType` and similar values. Maintaining those would
turn into something like `InstKind::IsSingleton()` and
`InstId::Singleton<TypeType>`, which didn't feel like as good a split.
`InstId::Print` remains aware of singletons, but I'm hoping to remove
other builtin-related calls from `InstId`.
Another thing I considered was adding `.is_singleton = true` to
`InstKind::Definition`. I don't think we could rely on that to get
`InstId::Singleton<TypeType>` set up as `constexpr`, though. At that
point, I think it'd mainly be _just_ a comment-like annotation, maybe
validated with `CHECK` but not having any effect on its own. So I
decided not to do that, just adding comments instead.
Sidenotes:
- "singleton" naming was discussed [on
#toolchain](https://discord.com/channels/655572317891461132/655578254970716160/1308870233729269781).
- The TODO in file.h about possibly excluding other things than
singletons seems moot. That's for raw IR, and we're much more focused on
textual IR these days.
---------
Co-authored-by: David Blaikie <dblaikie@gmail.com>
Co-authored-by: Dana Jansens <danakj@orodu.net>
Co-authored-by: Richard Smith <richard@metafoo.co.uk>
Co-authored-by: josh11b <15258583+josh11b@users.noreply.github.com>
Co-authored-by: Josh L <josh11b@users.noreply.github.com>
Co-authored-by: Chandler Carruth <chandlerc@gmail.com>
Co-authored-by: Carbon Infra Bot <carbon-external-infra@google.com>
Co-authored-by: Boaz Brickner <brickner@google.com>
Co-authored-by: Geoff Romer <gromer@google.com>
The documentation still referred to typed instructions having a
Parse::Node field, however that was removed and moved to the InstStore
in f197219c10.
Then GetParseNode() was renamed to GetNodeId() in 86a7c9ff45 and
then GetLocationId() in b079acd86f and finally GetLocId() in
b5d28f2c4b.
The comment in typed_inst.h mentions only three fields now, but some
types still have four, thanks to the unmentioned `ElementIndex index`
field. Normally this field comes last, after the `[...]Id` fields except
for in one case, AssociatedEntity. Rather than write ambiguously ordered
documentation, update the comment to and docs to say that the
ElementIndex comes last, and move it to the last position in
AssociatedEntity. Tests are rebased accordingly.
---------
Co-authored-by: Richard Smith <richard@metafoo.co.uk>
The current representation has two nested presence indicatators (the
optional bool, the null pointer). Currently the pointer is never null,
but the style guide suggests that T* should be used for parameters that
may or may not be present, so we do not need the optional here.
> When passing an object's address as an argument, use a reference
> unless one of the following cases applies:
>
> - If the parameter is optional, use a pointer and document that it
> may be null.
Once the parameter is just a pointer, the ScopedTiming field does not
need an optional either, and can just store the pointer.
It would be more preferable to have an optional representation of a
sometimes-null pointer like optional<T&> to describe a sometimes-null
pointer, as this would allow clearer runtime diagnostics when used
incorrectly (a check failure in unwrapping) and would be better
self-documenting through syntax instead of a comment. But we do not
currently have such a primitive.
Use it in the remaining few places where we currently hardcode `i32`: as
the index type in array indexing, as the type for literals in `if`
expressions, and as a valid return type for `Run`.
In preparation for changing `Core.Int` to be a class.
---------
Co-authored-by: Jon Ross-Perkins <jperkins@google.com>
The only documentation I can find for `.bazelversion` is in
https://github.com/bazelbuild/bazelisk/blob/master/README.md. However,
`bazel` will error out if it doesn't match the `.bazelversion`. For
example:
```
╚╡/usr/bin/bazel build :all
ERROR: The project you're trying to build requires Bazel 7.3.0 (specified in [elided]/carbon-lang/.bazelversion), but it wasn't found in /usr/bin.
```
Switching to this because the `MODULE.bazel` requires tighter version
pinning in order to avoid churn, and this should help catch mistakes
early.
In `MODULE.bazel`, demote mention of the version issue because it should
only really occur now when the version is being deliberately changed, so
the connection should be more apparent.
If they were called twice for a CompilationUnit, they would destroy
objects that they created and returned a pointer to, leaving a dangling
pointer somewhere else.
The Inst type will type erase a specific typed instruction by storing
the kind as an integer. It does this by calling InstKind::AsInt on a
runtime or compile-time InstKind. Then it returns the kind as InstKind
by reconstituting it from the integer.
Currently it does a cast to a raw enumerator and then calls
InstKind::Make. However Make is designed to be more of an internal
detail. The more clearly paired inverse operation is InstKind::FromInt,
which is documented as being intended to be exposed by derived classes
like InstKind.
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>
* Make the step stack into a class
* Make the operations on the stack (pushing, popping, test for done)
into methods on the stack class.
* Add more kinds of steps (array bound and name).
* Rewrite cases to use the new kinds of steps. Afterward, none use
`Step::Next()` or the step index, so those get removed.
* Add another convenience method `PushTypeId`.
* Remove the `SemIR::File&` member from the steps, since it doesn't
change.
Hopefully using `step_stack.Push`... calls makes it clear that they are
resolved in the reverse order they are executed.
---------
Co-authored-by: Josh L <josh11b@users.noreply.github.com>
Boaz Bricknerandjonmeow
David Blaikie
Richard Smith
josh11bandJosh L
Geoff RomerandRichard Smith
Dana JansensandJon Ross-Perkins
