Move parse error recovery for expression operators "out of line" & refactor in the area - #162591
Open
fmease wants to merge 9 commits into
Open
Move parse error recovery for expression operators "out of line" & refactor in the area#162591fmease wants to merge 9 commits into
fmease wants to merge 9 commits into
Conversation
rustbot
added
S-waiting-on-review
Collaborator
|
r? @JohnTitor rustbot has assigned @JohnTitor. Use Why was this reviewer chosen?The reviewer was selected based on:
|
fmease
force-pushed
the
out-of-line-recovery
branch
from
September 10, 2026 12:09
f26a1f9 to
4b51f2d
Compare
fmease
added
the
C-cleanup
…value These functions didn't actually modifiy the operand or return a new or different expression. So essentially the "`fn(Box<Expr>) -> Box<Expr>` part" was an identity function. Just change it to "fn(&Expr)".
fmease
force-pushed
the
out-of-line-recovery
branch
from
September 11, 2026 08:12
4b51f2d to
55f7d6b
Compare
`recover_from_inc_dec` *always* returns a (fatal) `Err(_)` *except* if
the increment/decrement operator is a subexpression *and* the source of
the operand is not available in which case it emits the diagnostic and
returns `Ok(_)` (rendering it non-fatal).
This makes no sense whatsoever. For illustration purposes, listed below
are steps that would make us reach this case:
1. `rustc a.rs --crate-type=lib` where `a.rs` contains:
`#[macro_export] macro_rules! m { () => { i++ } }`.
2. Move or remove `a.rs`
3. `rustc b.rs --edition 2018 --extern a -L.` where
`b.rs` contains:
`fn main() { (a::m!()); }`.
Just make the error unconditionally fatal and add a FIXME to make it non
fatal in the future which would allow us to report name resolution errors
and what not. However, since that would be slightly more involved and
represent a behavior change (in the error path), this is out of scope for
a mere cleanup commit like this one.
There's literally no upside to use it and only downsides:
It's not more concise, only adds code and obfuscates.
Its `MultiSugg::emit{,_verbose}` didn't even *emit* the diagnostic,
they merely *decorated* it!
fmease
force-pushed
the
out-of-line-recovery
branch
from
September 11, 2026 08:55
55f7d6b to
129a2b9
Compare
1. Remove unnecessary rebindings (`op_span` and `op = op.node`) 2. Remove binding `cur_op_span` as it's equal to `op.span` 3. Merge two `match`es on `op.node` into one to make the control flow more obvious and to render everything more legible. Moreover, it allows us to drop an ungly `unreachable!()`
Previously we would check if the current operator was `Binary(Lt)` and the current token was `>` to determine if we're looking at `<>`. However, since `AssocOp::from_token` also treats `<-` as `Binary(Lt)` for better error recovery, the condition would also hold for `<->` (`<-`, `>`) which is not what we want. E.g., given `1 <-> 2` we would previously emit diagnostic "invalid comparison operator `<>`". --- Also update `recover_from_spaceship_cmp_op` to do something similar -- not to fix anything but simply to eliminate param `op: Spanned<AssocOp>`.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Background: While we do have 3k-line module
rustc_parse/src/parser/diagnostics.rs1 dedicated to syntax error diagnostics & parse error recovery, the rest of the parser is still "littered" or "interwoven" through and through with complex or verbose diagnostic code. We support numerous recoveries from syntaxes found in other languages right next to code that actually decides what is and what isn't part of Rust syntactically. This makes the parser code very messy, illegible, bug prone and otherwise hard to maintain.So my long-term plan is to push all this diagnostic & recovery code "out of line", namely into new
rustc_parse/src/parser/$fragment/error.rsfiles, to keep the "inline code" of parsing routines in$fragment.rsfocused on actually parsing Rust code.Why not just use the pre-existing
parser/diagnostics.rsmodule? Well, making diagnostic code for expressions, patterns, types etc. share the same module is the definition of a hodgepodge. Moreover, moving all diagnostic code there would make the file way too large. Of course, if there turns out to be common code, a newparser/errors.rswould do the trick2.Best reviewed commit by commit.
Commit Refactor check_assoc_op to make it more legible was cherry-picked from my PR #161775.
(No LLM was or will be used by me during the entire creation process of this PR)
Footnotes
Not to be confused with
rustc_parse/src/diagnostics.rswhich holds diagnostic structs. ↩I'd like to see
parser/diagnostics.rsgone since its name makes it easy to confuse with the module that holds the diagnostic structs & sincediagnostics/{mod,impl}.rsisn't an option here (cc MCP 1003). ↩