#5445 updates to bazel 8.2.1, this does more updates (including to
buildifier, which does autofixes like the `sh_test` loads in the other
PR).
Note I'm using the latest available clang-format wheel. That's not
really something I expect people to have installed, but should mostly be
consistent. I'm specifically skipping clang-format 18 because it had
some broad regressions, and 19 got really confused by a `requires` on a
trailing return. Using the latest seemed probably okay since most people
won't see the difference. Do note that trailing returns in macros,
https://github.com/llvm/llvm-project/issues/47664, seems to be cropping
up again as an issue.
The current behavior hits UBSAN and ASAN issues.
Note, `RequiresNullTerminator` is already set to `false` in
`source_buffer.cpp`; setting it in `compile_helper.cpp` is making things
more consistent. The related logic is an [assert
fail](https://github.com/llvm/llvm-project/blob/main/llvm/lib/Support/MemoryBuffer.cpp#L52).
This was fuzzer-discovered.
This consolidates Lex::TokenizedBuffer::DumpSemIRRange and
Parse::TreeAndSubtrees::TokenRange into a single InclusiveTokenRange,
also making the OverlapsWithDumpSemIRRange function take the new struct.
I considered switching to `llvm::iterator_range<Lex::TokenIterator>`,
but we often want to see if the range is size one. Using `TokenIterator`
just looked like it'd add a bunch of offsetting to make it work; I view
that as low-value overhead.
For example:
```
Lex::InclusiveTokenRange token_range = GetSubtreeTokenRange(node_id);
auto begin_loc = tree_->tokens().TokenToDiagnosticLoc(token_range.begin);
if (token_range.begin == token_range.end) {
return begin_loc;
}
auto end_loc = tree_->tokens().TokenToDiagnosticLoc(token_range.end);
```
would become:
```
llvm::iterator_range<Lex::TokenIterator> token_range = GetSubtreeTokenRange(node_id);
auto begin_loc = tree_->tokens().TokenToDiagnosticLoc(*token_range.begin());
if (token_range.begin() + 1 == token_range.end()) {
return begin_loc;
}
auto end_loc = tree_->tokens().TokenToDiagnosticLoc(*(token_range.end() - 1));
```
So I'm keeping the bespoke struct.
I was thinking about this while working on parent generation on #5394
(which does a similar loop), it's just a simplification.
Also remove surplus spaces in the preorder print.
`FindIfOrNull` returns a pointer to the element in the range if it's
found, and nullptr otherwise. `FindIfOrNone` returns a copy of the
element in the range if it's found, and `T::None` (for a range of
elements of type `T`) otherwise. `Contains` returns a bool indicating
whether the element in the range is found.
These functions replace `llvm::find()` and `llvm::find_if()` when you
want a single answer back instead of an iterator. This avoids the need
to check against `end()`, allowing the return condition to be tested as
a standard bool.
We replace uses of `find()` and `find_if()` that did not require an
iterator with these new helpers.
Note that the return type of `FindIfOrNull` is a pointer since we can
not write `optional<T&>`, which must be tested for null. If the null
check is omitted, UB occurs and the resulting code may end up with an
incorrect pointer (https://crbug.com/40153300) into the range (or
elsewhere), rather than a null dereference. And this would be very
confusing to debug. Hopefully debug builds and sanitizers keep this from
being an issue we sink a bunch of time into debugging.
Rules executed by bazel don't necessarily have the right environment to
find the symbolizer, which was the intent of `cc_env` setting
`LLVM_SYMBOLIZER_PATH`. So far, this has kind of been a case-by-case
fix, but every so often I'm trying to debug a crash in a test that
doesn't provide it. Rather continuing down this route, instead add
drop-in wrappers for cc rules so that it's hard to forget.
Note `bazel/cc_rules` is intended to mirror `bazel/carbon_rules` and
`bazel/cc_toolchains`, rather than `@rules_cc`.
AFAICT there isn't a great way to add this as a default for the `bazel
run` environment. It's not typically going to be set on its own,
forwarding `$PATH` would be too broad, and the [action
`env_sets`](https://bazel.build/docs/cc-toolchain-config-reference#using-action-config)
I think are not quite what we need (I think those don't include output
execution, only compilation).
This is trying to document the status quo. Note
https://github.com/carbon-language/carbon-lang/pull/4497 placed
restrictions on a bit, and this is in part flowing back to restrictions
on both NodeId and ImportIRInstId limits.
Also renames variables of that type to match. A follow-up PR will rename
`Parse::StateStackEntry` to `Parse::State`. This is more consistent with
the naming of similar enums elsewhere in the toolchain, and with the
prevailing practice of using `state` rather than e.g. `entry` as the
name of a `StateStackEntry`.
See also discussion
[here](https://discord.com/channels/655572317891461132/963846118964350976/1326280585592700990)
Given the namespacing of `Diagnostics` in #5173, now we can use
`DiagnosticEmitter` for phase-specific emitters. This is consistent with
how we do `Context`, and also check had started this with
`DiagnosticBuilder` in anticipation of the namespacing.
Also renames `Emitter::DiagnosticBuilder` to `Emitter::Builder` for
consistency with other `Diagnostics` entities.
In check, I'm still splitting `DiagnosticEmitterBase` and
`DiagnosticEmitter` just to keep the emitter definition separate from
the context.
Also cleans up some incorrect check diagnostic emitter dependencies in
lower.
I was thinking about this after `seq` changes in #5182, and looked for
other uses that might be replaceable. Here's the resulting cleanup
around `seq`:
- Switch to `enumerate` or `zip` when possible.
- `int _` -> `auto _` (it's typically a `size_t`, but there's no reason
to cast when unused)
- Fix a case of cast style `(size_t)...` -> `static_cast<size_t>(...)`
- Switch `(void)close_children_count` to `[[maybe_unused]]`
What this really does is avoids shadowing names, so that we can
comfortable have things like `Check::DiagnosticEmitter` or
`Check::DiagnosticLoc` without shadowing being a concern.
Note, down this path I'm also thinking about:
- Renaming misc DiagnosticConsumer/DiagnosticEmitter classes, possibly
just to DiagnosticConsumer/DiagnosticEmitter (so
`Check::DiagnosticEmitter` instead of `SemIRLocDiagnosticEmitter`).
- Dropping `Diagnostic` from `Emitter::DiagnosticBuilder`.
- But not for `Check::DiagnosticBuilder`, because `Check::Builder` would
be ambiguous.
- Renaming diagnostics/diagnostic_* to drop "diagnostic".
[Discussion about SemIRLoc ->
DiagnosticLoc](https://discord.com/channels/655572317891461132/655578254970716160/1353771570463768698)
reminded me of this (in particular the older [Check::DiagnosticBuilder
discussion](https://discord.com/channels/655572317891461132/655578254970716160/1344363562608627763)),
but I'd only do that rename if there's matching consensus about a path
forward where we keep SemIRLoc, and in a way that it's only ever used
for diagnostics (the divergence from which is at the root of current
LocId discussion).
I'm trying to keep that separate from a namespace addition for clarity.
Adds subset conversion of `NodeIdOneOf` due to the choice usage, plus
the pre-existing TODO. Fixes incorrect information about nodes on
StructLiteral and TupleLiteral.
After this change, `UncheckedLoc` is only used in a couple import
contexts (hard to verify) plus `InstStore::GetWithLocId`.
Building on #5120, make a variant of `AddNode` that returns typed nodes,
and replace `UnsafeMake` uses with it. This switches to templating in
import parsing so that we can get type validation.
With this change, `UnsafeMake` ends up used in three places: `Tree::As`,
`Tree::TryAs`, and `Context::AddNode`. That should mean that all typed
nodes are verified.
This flows out of #5084 and trying to reduce UnsafeMake use. It turns
out imports and namespaces were using unexpected node kinds (previously
ImportIntroducer instead of ImportDecl, for example). This fixes and
adds validation.
I was uncertain about whether to just remove the is_convertible check,
since I don't see it as motivating creation of a conversion between
NodeIdOneOf types. So I've just left a TODO for now.
According to approved syntax at
https://github.com/carbon-language/carbon-lang/blob/trunk/proposals/p3848.md#syntax-defined,
`fn F[]` without explicit parameters should be valid. This makes it
work, then adds some validation to prevent `class C[]` in check.
Note that for `fn`, positional parameters are a TODO -- but this allows
me to test validation in `fn destroy[]` which is rejected, not just a
TODO.
This doesn't change functionality, but I was seeing better diagnostics
in VS Code.
This also changes the NodeId constructors for related types (also
NodeCategory and NodeIdForKind) to use UnsafeMake for construction. That
originated from avoiding ambiguity coming from `requires`, but the
constructor mode is also one we should typically avoid (e.g., preferring
`Parse::Tree::As`).
Use CRTP to eliminate per-type declarations of `None`. Note this adds
`None` to a few that may not need it, but eliminates a lot off
boilerplate.
Note this leaves `GenericInstIndex::None` because it has a more complex
construction.
Also fix `InstId::InitTombstone` to be `NoneIndex - 1`
We had a long discussion of this, so trying to document what seems to be
the conclusion... and also clean up the exceptions that I could find.
---------
Co-authored-by: Dana Jansens <danakj@orodu.net>
Noted CopyNameFromImportIR while glancing around (this one's interesting
because it's NameId, not void nor auto), did a scan just for a few other
cases. Not an exhaustive fix, and TBH assuming we'd prefer `auto ... ->
auto` since equivalent Carbon syntax would probably be `fn ... -> auto`
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>
Rearrange `NodeCategory` printing so we get a compile-time error for
missing switch cases if it's missing any categories. Add several missing
categories.
In passing, fix some minor things in the `NodeCategory` class
definition, and fix an overly-permissive typed node.
This adds support for choice types at a similar level to that of a C
enum, where each alternative has a name but no additional
data/parameters attached to it. We generate a TODO diagnostic if
parameters are specified.
Because there's no extra data, the storage is a simple unsigned integer
discriminant of the smallest possible size.
A choice without any alternatives is not constructible. A choice with a
single alternative is, and has an empty tuple in place of a discriminant
since it has only one state. The empty tuple is used to make the class
non-constructible. This can be improved.
Each alternative is turned into a let binding on the choice that is a
value of the choice with that alternative set as the active one in the
discriminant. This isn't possible to write in user code with a class
right now, since the let binding has the same type as the choice
(which is a class) it is within. It's possible to generate it in semir
however by adding the binding after the class is marked complete.
When I open a .def file, there are often 4 errors:
- The #error
- The #define is not defined
- Missing `;`
- Identifier naming
This PR is meant to disable all of these, since they can be distracting
from fixable diagnostics.
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.
To do this, we restructure the parse tree to make `forall` a leaf node
that comes before the parameter list.
---------
Co-authored-by: Jon Ross-Perkins <jperkins@google.com>
Change representation of package names from `IdentifierId` to
`PackageNameId`, and add a special value `PackageNameId::Core` for the
Core package. Add a `Core` expression to name the Core package, and
support for parsing the `Core` keyword in `package` and `import`
declarations.
For now, I've made no changes to instruction fingerprinting or name
mangling. This means that fingerprints and mangled names will collide
between names in the `Core` package and names in a `r#Core` package. See
#4908.
---------
Co-authored-by: Jon Ross-Perkins <jperkins@google.com>
Also makes the style guide explicitly comment on void, but this was the
intent IIRC because it matches Carbon's `-> ()` (and "always" versus
"except for void", which we definitely went back and forth on).
Includes adjusting function pointers, which I definitely forget this
syntax works sometimes.
Excludes utils/tree_sitter/src/scanner.c because it claims to be C, but
really we should probably fix that to be cpp.
When a node kind's Id::Kind is determined from its category, don't also
require it to be listed in the switch over all node kinds. This was both
redundant and also error prone -- and in practice for several node
kinds, the Id::Kind computed in the two different ways was different.
Instead, have the switch over node kinds handle only special cases that
can't be handled by their category, and enforce that each node kind has
an Id::Kind specified in exactly one way via checks in the .cpp file.
This refines the previous change in #4280 -- we still get the improved
errors for missing updates, but now also don't require redundant
additions to the switch.
At present, we typically define a DiagnosticConverter, then store an
instance of it and a DiagnosticEmitter that wraps it. This is relatively
minor in general, but I've been trying to create more self-contained
DiagnosticEmitter classes (which hold their own DiagnosticConverter,
similar to NullDiagnosticEmitter), and there it just gets in the way.
Since we don't reuse DiagnosticConverter instances, this combines the
definition into DiagnosticEmitter. Mainly this means we don't have a
separate object in play, and less to carry around.
The most impact is probably to SemIRDiagnosticConverter, which was also
the most complex. Now `SemIRLocDiagnosticEmitter`, this gets some
different construction flow. Note in the PR I've split the file rename
to its own commit, to try to help delta views. However, the most
substantial parts of the refactoring are split into #4876, which this
depends upon.
Jon Ross-Perkins
Boaz Brickner
josh11bandJosh L
Geoff Romerandjosh11b
Dana Jansens
Thomas Köppe
czapiga