Lexer diagnostics used a single `const char*` as their source location,
which only allowed pointing at a single character rather than a run of
characters. This limited the underlining of unrecognized tokens, and
also made escape sequence diagnostics particularly strange by stopping
at whatever point the lexer discovered the error and pointing only at
that location. For example: `"\w"` was reported at the `w`, and `"\x"`
at the closing `"`.
This change tries to always keep the primary location of a diagnostic
the point at which typing (inserting or deleting) might fix the error.
So for `"\x"`, `\x` is underlined, but the position is at the `"` as
that's where you would type a hex code.
The lexer's emitter now takes a special lexing source location that
represents either a single position _between_ two characters, a single
character, or a range of the source buffer. The lexer diagnostics are
updated to construct these for the character range as appropriate.
Parser diagnostics for a missing token were located at the _next_ token,
which is particularly confusing for terminators like missing `;`s. Now
they are located at a position after the previous token, with or without
a space depending on the token, but typically on the same line.
Diagnostics still sort by how far the parser has read.
The parser diagnostics for unclosed groups also now attaches a label to
the opening token when it is on a separate line. And the diagnostics for
mismatched indent in a multi-line string attaches a label to the closing
`'''` that establishes that indent.
There are likely more location improvements we should do here, but this
seemed like a reasonable incremental improvement and is what Claude
found when doing an initial audit. One example where I think we should
make further improvements is around suggesting raw identifiers when we
find an unexpected `Cpp` or `Core` or `base`. We do this consistently
now, but raw identifiers should be rare. A TODO is added for these
cases.
Assisted-by: Claude Code
---------
Co-authored-by: Richard Smith <richard@metafoo.co.uk>
If a keyword or a sized type literal (eg, `f2`) is used in a context
where we are confident that we are expecting an identifier -- either
before a `:` in a binding pattern or after a `.` in a member access or
designator -- then recover as if a raw identifier was used.
This appears to be a particular stumbling block for coding agents, so
seems worth paying special attention to.
Add a mechanism to the tokenized buffer to track additional tokens
synthesized for error recovery so that we can keep the lexed token
sequence immutable and still satisfy the invariants throughout the rest
of the toolchain for recovery tokens. Thanks to chandlerc for suggesting
this approach!
This change ensures that a function signature always starts with an
`IdentifierNameMaybeBeforeSignature` node (renamed from
`IdentifierNameBeforeParams`), even in the case of function declarations
like `fn F -> T` that have no parameter list. As a consequence, this
ensures that we push new entries onto `pattern_block_stack` and
`full_pattern_stack` when we start processing the function signature.
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)
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.
This change splits `NodeKind::IdentifierName` into separate node kinds
depending on whether the identifier is followed by parameters, and
similarly splits `NameQualifier` based on whether the qualifier has
parameters. This enables us to only push a pattern block when it's
actually needed, rather than "defensively" pushing one when it might be
needed.
---------
Co-authored-by: Jon Ross-Perkins <jperkins@google.com>
Note, this supports plurals, but doesn't apply it anywhere. I'm mainly
doing that to demonstrate the approach regarding syntax. See
format_providers.h for details.
This is a primarily automated change:
- Search & replace for capitalization
-
`(CARBON_DIAGNOSTIC\((?:\n\s+)?\w+,(?:\n\s+)?\s\w+,(?:\n\s+)?\s")([A-Z])`
- `$1\L$2`
- Search & replace for period
-
`(CARBON_DIAGNOSTIC\((?:\n\s+)?\w+,(?:\n\s+)?\s\w+,(?:\n\s+)?\s"(?:[^)]|\n)+)\.("[,)])`
- `$1$2`
- Limited search & replace for `ERROR: ` -> `error: ` in streamed things
- Leaving a TODO for command_line because there's more cleanup that can
be done there
- Modify diagnostic_consumer.cpp
- ERROR -> error
- WARNING -> warning
---------
Co-authored-by: Richard Smith <richard@metafoo.co.uk>
Move subtree sizes over to TreeAndSubtrees, using the different
structure to represent the additional parse work that occurs, as well as
making it clear which functions require the extra information. My intent
is to make it hard to use this by accident.
The subtree size is still tracked during Parse::Tree construction. I
think a lot of that can be cleaned up, although we use it during
placeholder assignment so it may take some work. I wanted to see what
people thought about this before taking action on such a change.
I'm using a 1m line source file generated by #4124 for testing. Command
is `time bazel-bin/toolchain/install/prefix_root/bin/carbon compile
--phase=check --dump-mem-usage ~/tmp/data.carbon`
At head, what I'm seeing is:
```
...
parse_tree_.node_impls_:
used_bytes: 61516116
reserved_bytes: 61516116
...
Total:
used_bytes: 447814230
reserved_bytes: 551663894
...
1.43s user 0.14s system 99% cpu 1.565 total
```
With `Tree::Verify` disabled completely, it looks like:
```
parse_tree_.node_impls_:
used_bytes: 41010744
reserved_bytes: 41010744
...
Total:
used_bytes: 427308858
reserved_bytes: 531158522
...
1.20s user 0.13s system 99% cpu 1.332 total
```
Re-enabling just the basic verification (what is now `Tree::Verify`),
I'm seeing maybe 0.05s slower, but that's within noise for my system. I
do see variability in my timing results, and overall I think this is a
0.2s +/- 0.1s improvement versus the earlier (always testing `Extract`
code) implementation. That's opt; debug builds will be unaffected,
because the same checking occurs as before.
Note, the subtree size is a third of the node representation, which is
why I'm showing the decrease in memory usage here.
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
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>
On the parsing side, we treat `a.(b)` as a member access whose second
operand is a `ParenExpr` rather than a `MemberName`. A new node category
is added for the union of `MemberName` and `ParenExpr` to support this.
Checking is mostly reusing the same pieces we already have for simple
member access. Compound member access is in most ways a simplified form
of simple member access because it doesn't need to do any lookup.
This gets to a lifetime subtlety, particularly with things like the
sorting diagnostic consumer that delay output. In order to reduce the
chance of accidental references, disallow StringRef in the diagnostics.
For example:
```
./toolchain/diagnostics/diagnostic_emitter.h:162:5: error: static_assert failed due to requirement '!std::is_same_v<llvm::StringRef, llvm::StringRef>' "Use std::string or llvm::StringLiteral for diagnostic lifetimes."
static_assert(
^
toolchain/check/convert.cpp:477:11: note: in instantiation of member function 'Carbon::Internal::DiagnosticBase<std::string, std::string, llvm::StringRef>::DiagnosticBase' requested here
CARBON_DIAGNOSTIC(StructInitMissingFieldInConversion, Error,
^
./toolchain/diagnostics/diagnostic_emitter.h:47:7: note: expanded from macro 'CARBON_DIAGNOSTIC'
::Carbon::Internal::DiagnosticBase<__VA_ARGS__>( \
^
```
---------
Co-authored-by: Chandler Carruth <chandlerc@gmail.com>
Continuing with #3070. Just a dir and file rename (mostly removing
prefixes, although for parse_tree_fuzzer and parse_tree_file_test I'm
dropping "tree" instead of "parse"). Everything in the parse dir should
be marked as a move.
Chandler CarruthandRichard Smith
Geoff Romer
Jon Ross-Perkins
Brymer Meneses
josh11bandJosh L