Print out error message and file when detecting that rust parser panicked

[Mark-Simulacrum]

We're seeing that rustfmt just fails to run with no error message whatsoever without --verbose in rust-lang/rust#65939, and once that was passed managed to track it down to rustfmt printing "Rust parser panicked" (see this log for example). This is my attempt to try and get rustfmt to print some more information in the hope that once this propagates into nightly we can run it by that PR.

If you have thoughts on how to debug the failure I'd love to hear that too!

r? @topecongiro

[Mark-Simulacrum]

CI failure looks spurious.

[calebcartwright]

Any chance there's a parser error in a file that's ignore'd in the rustfmt config file? There was a bug in rustfmt where it wouldn't display the parser error text from ignored files, but would still exit with an error code.

[Mark-Simulacrum]

It is possible (and indeed highly likely) as this is the Rust repo, so e.g. src/test contains probably dozens of tests which might cause the parser to error out. But if the file is ignored, it shouldn't lead to a error code exit, right? Or was the bug that we were exiting despite the ignore applying?

Based on the log there's only two files that are failing like this and it's only on Windows (AFAICT) which is a bit odd too...

[calebcartwright]

But if the file is ignored, it shouldn't lead to a error code exit, right?

Actually it can still lead to rustfmt exiting with an error due to the current approach rustfmt is using to get the AST and create the file/mod association (details in #3930 and #3920 (comment)).

Or was the bug that we were exiting despite the ignore applying?

The bug is covered in more detail in #3920 (comment) (second section under silently erroring on fatal parser errors, even when hide_parse_errors is false)

That bug was fixed via #3920, but hasn't been released yet AFAIK

[calebcartwright]

I'll grab my windows machine and try running the latest version of rustfmt against the rust repo to see if it tells me anything. It could certainly be another issue but it does sound a lot like some of the other parser related issues we've seen

[calebcartwright]

Believe the parser error is coming from src/libcore/lib.rs

running rustfmt --edition 2018 --verbose --check src\libcore\lib.rs on Windows with nightly-i686-pc-windows-gnu will return the same The Rust parser panicked and non-zero exit code.

Parser error details:

error: couldn't read \\?\C:\dev\rust\src\libcore\..\stdarch\crates\core_arch\src\mod.rs: The filename, directory name, or volume label syntax is incorrect. (os error 123)
   --> \\?\C:\dev\rust\src\libcore\lib.rs:250:5
    |
250 | mod core_arch;
    |     ^^^^^^^^^

The Rust parser panicked

[Mark-Simulacrum]

Ah, I see. That is... unexpected, to say the least! I'm glad to hear that it sounds like you believe that this has been fixed on master at least.

I would prefer to land this PR regardless I think since it seems like we should give some indication of why we've encountered a rustc parser panic...

I also think that might not be the actual error, since AFAICT that file should exist on Windows (on our PR builders). You might need to run git submodule update --init to get it locally, though.

[calebcartwright]

that file should exist on Windows (on our PR builders). You might need to run git submodule update --init to get it locally, though.

The file exists on my local env too (I ran src/ci/scripts/checkout-submodules.sh locally) which makes it even more puzzling. The only thing I can think of that may potentially be relevant is that rustfmt is still using v610.0.0 of the rustc-ap-* crates and the associated parser in v610.0.0

I also think that might not be the actual error

Fair enough, though I still suspect that's the parser error rustfmt is seeing. One way that hypothesis could be tested right away (while waiting for this PR to land and become available) would be to run the two below commands in the pipeline:

rustfmt --edition 2018 --verbose --check src\libcore\lib.rs --skip-children
rustfmt --edition 2018 --verbose --check src\libcore\lib.rs

Obviously if they both pass then my guess would definitely be wrong, but if the first one succeeds and the second one errors (with the The Rust parser panicked err) then IMO it'd be highly likely that is indeed the parser error rustfmt is seeing.

[Mark-Simulacrum]

Hm, so --skip-children feels wrong to me, since we do want to eventually format the submodules of every crate. And I think cargo fmt won't run rustfmt on every file, just on the roots today?

But maybe I'm misunderstanding what the flag does.

However, it looks like at least one panic you've successfully tracked down, based on this log libcore does indeed cause that message to be printed when individually formatted.

[calebcartwright]

Hm, so --skip-children feels wrong to me, since we do want to eventually format the submodules of every crate

Makes sense. I was under the (likely incorrect) impression that the pipeline was invoking rustfmt directly based on a cursory review pipeline logs, but it sounds like you are actually running the cargo fmt subcommand?

If you weren't just trying to run rustfmt on those handful of specific files and do want to format/check the formatting for all the crates and submods (other than those that are ignored) then yes, you don't want to use --skip-children

And I think cargo fmt won't run rustfmt on every file, just on the roots today

But maybe I'm misunderstanding what the flag does.

You are correct. cargo fmt inspects the workspace metadata to get the root entry file for each target for each crate/package in the workspace, which it then passes as args to rustfmt. rustfmt then (by default) will format that input file and all the discovered submods. the --skip-children flag results in rustfmt only formatting the explicitly specified input files, and mods defined in other files are ignored

[Mark-Simulacrum]

Okay, so it sounds loosely like rustfmt on master today would "fix" the bug where it doesn't report any errors but silently exits, even though errors are only coming from ignored files. Is that correct? We would not be okay with such a situation in rust-lang/rust unfortunately which may mean that we need to delay integrating rustfmt even more, since I'm not sure it's easily fixable inside rustfmt based on my vague understanding...

[calebcartwright]

Okay, so it sounds loosely like rustfmt on master today would "fix" the bug where it doesn't report any errors but silently exits, even though errors are only coming from ignored files. Is that correct?

Correct. the nightly rustfmt v1.4.11 still has the bug where it suppresses the text of non-recoverable parser errors originating from ignored files, but that bug has been fixed on master (I used that version to track down the parser error text)

[Mark-Simulacrum]

Do you have any insight into whether it would be feasible to fix the underlying bug? That is, it is a deal breaker for the Rust project if ignored files are in fact the cause of breakage here.

[calebcartwright]

Do you have any insight into whether it would be feasible to fix the underlying bug?

Which bug? Are you referring to rustfmt attempting to parse and construct the entire AST, even for ignore'd files?

[Mark-Simulacrum]

Yes -- sorry for not being clear!

[calebcartwright]

No worries! It's definitely feasible and @topecongiro has outlined a plan to resolve the issue (though I've no idea what the timeline is).

ATM I'm looking into updating rustfmt to use the latest rustc-ap-* crates version but am hitting some snags due to things being moved out of libsyntax and into separate crates. After that (assuming no one beats me to it 😄) I was planning to try to work on implementing the alternative parsing/AST construction approach that should resolve the underlying issue you are seeing

[Mark-Simulacrum]

Okay, thanks for the information. I'll leave it up to you (or @topecongiro) whether to close this PR; I think it's plausibly still useful but maybe not on master :)

Sounds like we'll need to hold off on integrating rustfmt into rust-lang/rust until we get this resolved (both the bug with not printing anything when encountering a parser error and not being able to truly ignore files wrt to parser error).

[calebcartwright]

👍 sounds good! #3930 would be the issue to track on the rustfmt side for anyone interested as that seems like it'll be a prereq for rust-lang/rust to be able to integrate rustfmt

[topecongiro]

@Mark-Simulacrum Thank you for the PR, and @calebcartwright, thank you for investigating! I will look into the issue myself once I feel better. Meanwhile, I will merge this as it improves the error report even if the root cause is somewhere else.