Repository navigation
Conversation
Since I didn't want to use a tempdir/tempfile just for piping a simple source file.
This ICEs on latest main.
This has to be a binary test not the usual {source, target} test, since
the latter doesn't have an installed ICE hook and the test would just
vacuously pass.
rustc's parser can produce an `Ok(expr)` with recoveries in the form of
diagnostics even if the `expr` is technically invalid. One such case is
an fn item that forgot the arg list parentheses, i.e. `fn foo {}`.
To properly handle this, we check for actual parser errors and abort if
there are parser errors (i.e. we re-emit the original macro invocation
unchanged).
We also reset error counts to avoid tainting other parsing sessions
(just in case).
ICE reported in <rust-lang#7087>.
…nt leak Since rustc's parser session state is global. We don't reset it properly in `parse_items_from_cfg_select_inner`, so an valid `cfg_select!` followed by an invalid `cfg_select!` can inhibit formatting.
Since error count is a parser session global state, this was accidentally leaking previously. This causes weird behavior: if you have a `cfg_select!` containing valid item syntax followed by an `cfg_select!` containing invalid item syntax, the invalid `cfg_select!` instance unintentionally inhibits the formatting of the earlier valid one. So fix this by properly resetting error counts.
jieyouxu
added
the
llm-assisted
rustbot
added
the
S-waiting-on-review
| if parser.psess.dcx().has_errors().is_some() { | ||
| parser.psess.dcx().reset_err_count(); | ||
| debug!("cfg entry parsed with recovery in cfg_select! predicate"); | ||
| return None; | ||
| } |
There was a problem hiding this comment.
Remark: yes, I am fully aware this is fragile and basically setting us up to get wrong (other similar macro formatting also need to do this).
This in my opinion is more fundamental to the fact that we reuse rustc's AST and parser. rustc's parser, being positioned as a "compiler with good diagnostics and user experience", attempts a lot of recoveries to give good suggestions. The downside of the good experience is that it necessarily requires more impl complexity, and for rustc's parser it means using error counts as out-of-band bookkeeping and not just via parser Result<..> return values.1
That works reasonable well for rustc. Not so fortunate for rustfmt since rustfmt (well, us) need to also account for parser recoveries even if we "don't actually care" as an AST pretty-printer.
Footnotes
-
Overall, I still think that the upsides of sharing the rustc AST / parser significantly outweighs the downsides considering our available bandwidth and resources. ↩
jieyouxu
added
A-cfg_select
|
Note that |
This branch has not been deployed
2 participants
Overview
This PR fixes two bugs:
cfg_select!parsing which cause us to ICE, cf. [ICE]: bad span:):cfg_select!invalid arm expr "successfully" formats but produces an ICE report #7087, andcfg_select!parsing where we unintentionally leak parser session error count, causing us to incorrectly give up formatting on a program consisting ofcfg_select!with valid body followed bycfg_select!with invalid body.Fixes #7087.
Bug 1: incorrect parser recovery handling
Reported in #7087. rustc's parser can produce recoveries in the form of an
Ok(expr)(and other non-terminals) alongside diagnostics even if theexpris technically invalid. One such case is an function item that forgot the argument list parentheses, i.e.fn foo {}. That mistake pattern is common enough that the parser has recovery for it. Indeed,To properly handle this, we check for actual parser errors and bail if there are parser errors (i.e. we re-emit the original macro invocation unchanged). We also reset error counts to avoid tainting other parsing calls.
Bug 2: parser session error count leak
We use the same rustc parser session when trying to parse multiple
cfg_select!in the same file. The error count is shared state on said parser session. We don't properly reset the error count when we encounter an recovery inparse_items_from_cfg_select_innerwhich causes examples liketo give up (note first instance is valid body, second is invalid body) and not format the first instance.
To fix this, we need to properly reset error counts upon encountering potential recoveries.
Notes on testing strategy
Notes for the reviewer
This PR is structured to 5 commits:
rustfmt_with_extrawith stdin, so that I can runrustfmtwithout needing to use tempdir/tempfiles unnecessarily.main(i.e. ICEs). Commit 3 is the fix where the test now passes.cfg_select!instance with valid body.I suggest the reviewer checkout commits 2 and 4 specifically and observe outcome locally.
Changelog
):cfg_select!invalid arm expr "successfully" formats but produces an ICE report #7087 was authored by me. I used LLM to help review my changes, since the parser recovery paths has some hard to notice edge cases.cfg_select!-with-invalid-item case, but also found thatparse_items_from_cfg_select_innerhas a slightly different parser session error count leak bug. I didn't notice this during the core fix, but indeed I double-checked this by hand (in the form of the test) and confirm that there is indeed a bug here. (I elaborate on the analysis earlier.)