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>
This implements the leads decision made in #6821, proposal #6910. The
proposal is pending, but I figured it's relatively safe to just do given
the decision.
Assisted-by: Google Antigravity with Gemini
---------
Co-authored-by: Chandler Carruth <chandlerc@gmail.com>
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.
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.
I was suggesting this because `FloatingPoint` is pretty long. `int` and
`float` should be familiar abbreviations. `unsigned` should be familiar
to developers too, but `UnsignedInt` still feels usefully clearer for
the additional chars.
Specifically, rather than nesting them in `Carbon::Testing`, nest them
in `Carbon::Foo` for whatever component they're benchmarking. All our
current benchmarks are lexer benchmarks so its `Carbon::Lex`.
This makes even more sense for benchmarks than unittests I think.
Continuing with #3070. Just a dir and file rename (only prefix change is
lexer_file_test). Everything in the lex dir should be marked as a move.
Note, I think this closes#3070. There may still be further cleanup
later, but the organizational changes suggested there are being
completed.
---------
Co-authored-by: Chandler Carruth <chandlerc@gmail.com>
Chandler CarruthandRichard Smith
Jon Ross-PerkinsandChandler Carruth