Fix cfg_select! parser recovery handling and parser session error count leak

[jieyouxu]

Overview

This PR fixes two bugs:

1. Incorrect parser recovery handling in 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, and
2. A closely related bug in cfg_select! parsing where we unintentionally leak parser session error count. This can unintentionally inhibit formatting of another eligible cfg_select! invocation.

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 the expr is 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,

Once we've got what we think is a valid block expr we go through our regular formatting flow and when we reach fn foo {} things fail because rustfmt (IMO rightfully) assumes that it will always be able to find an opening ( and closing )

-- @ytmimi's comment

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 in parse_items_from_cfg_select_inner which causes examples like

cfg_select! {
    unix   =>   {   fn   bar() {} }
}

cfg_select! { unix => { fn foo {} } }

to 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

• For bug 1, we can't use simple {source, target} test pair since it would vacuously pass as there's no ICE hook that would cause the test to fail (see analysis). I have to use a binary test here.
• For bug 2, there we can for the error count leak, since the target file goes from give up formatting => formatted.

Notes for the reviewer

I suggest the reviewer checkout the test commits specifically and observe outcome locally.

Changelog

Fix `cfg_select!` parser recovery handling and parser session error count leak

---

• I did not use an LLM to create a change in this PR.
• I used an LLM to create a change in this PR, and I have explained below how it was used.

• The core test and impl for fixing [ICE]: bad span: ): ; 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.
• During said review, the LLM found that my fix was sufficient for the single cfg_select!-with-invalid-item case, but also found that parse_items_from_cfg_select_inner has 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.)
• Actual code changes I hand-wrote.
• All communications and analysis are hand-written. (Sorry, I know this is a bit long, but I rather the analysis exists somewhere.)

[jieyouxu]

Note that cfg_select! formatting is still gated on nightly-only, so the fix for Bug 2 doesn't impact stable formatting.

[matthewhughes934]

It's my first time looking over this macro parsing code, so had some questions to help build my understanding

View changes since this review

[matthewhughes934]

could/should we propagate these parser errors? So the user knows we gave up because of invalid code

Conversely: do we ever want to not reset the error count like this? In which case could we just push it up a level

[jieyouxu]

could/should we propagate these parser errors? So the user knows we gave up because of invalid code

It depends. The problem is that, just because say a cfg_select! arm body is not "valid syntax" outside of macro context, it doesn't mean the overall macro itself is invalid syntax. Consider precisely

cfg_select! { unix => { fn foo {} } }

fn foo {} is not valid syntax outside of macro contexts, but the overall cfg_select! is valid syntax. (If that makes sense.)

I don't think it makes sense to emit the give up either, not even under show_parse_errors=true, because macro formatting is essentially conditioned on a set of heuristics of "does this look like non-macro context rust code". That typically involves us synthetically invoking parts of rustc's parser. Unless under some hypothetical debug_why_rustfmt_gives_up_on_macros=true, then maybe.

Conversely: do we ever want to not reset the error count like this? In which case could we just push it up a level

Probably. I'll check.

[jieyouxu]

I pushed the error count up a level.

I'll probably look into a less footgun-ish approach in a follow-up #7180.

[jieyouxu]

@rustbot author

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[jieyouxu]

Note: I believe cfg_if! also has this error count leak bug, cf. #7184, I'll look into that later.

View changes since the review

[jieyouxu]

Changes since last review:

• Pushed the error count book keeping up a level. Note that I also need to the panic-handling in the cfg arms parsing.
• Folded the 2 fix commits into one.

r? matthewhughes934 (since you're already reviewing)

@rustbot review

[jieyouxu]

(I know there's a bit of duplicate code for the panic handling, I'll probably pull out a helper when I look into cfg_if! as well.)

[ytmimi]

Thank you! I'm also in favor of extracting the panic handling code into a helper, but that can be done in a follow up!

View changes since this review