gracefully handle recoverable parser errors in ignored files

[calebcartwright]

Refs #3777 (comment)

[calebcartwright]

AFACIT, when there's a parser error in the main file, we hit the Err(ErrorKind::ParseError) arm (line 91) after calling parse_crate and don't even make it down to the ModResolver in such a scenario:

rustfmt/src/formatting.rs

Lines 82 to 92 in 1ded995

	let krate = match parse_crate(
	input,
	&parse_session,
	config,
	&mut report,
	directory_ownership,
	) {
	Ok(krate) => krate,
	// Surface parse error via Session (errors are merged there from report)
	Err(ErrorKind::ParseError) => return Ok(report),
	Err(e) => return Err(e),

This is the case for the error we are seeing with the ice file in #3777.

If I've got that right, then I see a couple options (let me know if there are others/better ones!):

1. Don't attempt to parse the input at all in format_project if the main_file is a match in the ignore_path_set. This could be done conditionally, such as only applying if the main_file is under tests/ subdirectories and/or only if a new flag for rustfmt is provided. Basically changing
   

   rustfmt/src/formatting.rs

   Lines 73 to 75 in 1ded995

   	if config.skip_children() && ignore_path_set.is_match(&main_file) {
   	return Ok(FormatReport::new());
   	}

to something like

   // new config setting do_not_parse_ignore_matches
    if ignore_path_set.is_match(&main_file)
        && (config.skip_children() || config.do_not_parse_ignore_matches())
    {
        return Ok(FormatReport::new());
    }

2. Update the Err(ErrorKind::ParseError) arm (line 91) on the parse_crate result to behave differently if the parse error came from a file that's a match in the ignore_path_set. Perhaps something like

        // Surface parse error via Session (errors are merged there from report)
        Err(ErrorKind::ParseError) => {
            if ignore_path_set.is_match(&main_file) {
                return Ok(FormatReport::new());
            }
            return Ok(report)
        },

Between those two options, I'd prefer option 1

[topecongiro]

Thank you for delving into this issue!

when there's a parser error in the main file, we hit the Err(ErrorKind::ParseError) arm (line 91) after calling parse_crate and don't even make it down to the ModResolver in such a scenario

I think that you are right here.

I personally prefer the option 2, as that way we can avoid adding a new config option. If we go that route, we may also need to wrap parse_crate with catch_unwind as the rustc parser sometimes dies with panic on certain parse errors 🤷‍♂

[calebcartwright]

I've got option 2 working, and getting the desired zero exit code. There is already a catch_unwind within parse_crate:

rustfmt/src/formatting.rs

Line 660 in 1ded995

catch_unwind(move || parser.0.parse_crate_mod().map_err(|d| vec![d]))

However, the parse error is still displaying on the terminal. Is that still the desired behavior?

I suppose if we wanted to suppress we could use panic::set_hook with a no op closure and then take_hook after the catch_unwind call.

Is wrapping the parse_crate call in a catch_unwind still necessary with that inner catch_unwind?

[topecongiro]

There is already a catch_unwind within parse_crate:

Great! Thank you for checking.

However, the parse error is still displaying on the terminal. Is that still the desired behavior?

It would be better if we could not display the parser error from files that are specified in ignore. IIRC the code which displays parser errors is this:

rustfmt/src/formatting.rs

Line 671 in 1ded995

Ok(Err(mut diagnostics)) => diagnostics.iter_mut().for_each(DiagnosticBuilder::emit),

We can suppress these error messages by explicitly canceling it. rustfmt already does this when parsing macro arguments from the token stream:

rustfmt/src/macros.rs

Lines 108 to 109 in 1ded995

	e.cancel();
	parser.sess.span_diagnostic.reset_err_count();

I think we can do the same thing here against errors from files that matches the ignore pattern.

[calebcartwright]

IIRC the code which displays parser errors is this:

At least on my machine, the error is being printed to the console directly from that catch_unwind line. When I first started looking for the source I started on line 671 too, but all my debugging/testing showed it was already printed before that point.

I'll take another look at it though 👍

[topecongiro]

At least on my machine, the error is being printed to the console directly from that catch_unwind line.

Ok, now I recall it correctly 😄
The rustc parser handles an error by either returning Err or panicking (sigh). Which is used depends on the kind of syntax error and we cannot tell beforehand. #3779 (comment) handles the former, and the catch_unwind implicitly handles the latter.

E.g, the parser will return Err against this snippet:

abc

but panics against this snippet:

fn main() {
    1x;
}

So if we want to completely silence the parser error against the skipped main file, we need to do both adding panic::set_hook and #3779 (comment).

[calebcartwright]

This one turned out to be more involved than I'd originally thought 😄

I tried panic::set_hook no avail, but that lead me to take a closer look at the emitter being used for the parse session which is how I ended up solving this over in #3782