don't fail on recoverable parser errors in ignored files

[calebcartwright]

Resolves #3779 and refs #3777

This updates how rustfmt handles parser errors when the parser was able to recover from all the errors, and all those errors came from ignored files:

1. Don't emit recovered parser error text for ignored files
2. Don't fail rustfmt when the only parse errors originated in ignored files and were recovered by the parser

This was implemented via a custom Emitter that checks each of the parser errors as they roll through, and keeps track of whether all of the parser errors originated from ignored files. If the parser encountered errors, but was able to recover from them all, then that indicator is checked to determine if the error count can be reset.

AFAICT it is necessary to track that status within the emitter as neither the Parser, ParseSess, nor Handler publically expose details about the parser errors encountered; only a true/false flag indicating whether any errors were encountered.

I also investigated whether we could cancel/remove parser errors that met the ignore file condition from within the emitter, but this was not viable either as a mutable reference is required to be able to cancel the DiagnosticBuilder, reseting the DB's Handler error count from the emitter does not remove the error from the parser (see below #3782 (comment) for why this is the case), etc.

[calebcartwright]

this is the emitter that Handler::with_tty_emitter creates internally, so creating it here and including it in the custom Emitter info so that parser errors that occur in non-ignored files are still displayed as usual

[topecongiro]

Would you mind explaining the purpose of can_reset? I could not see what this field is used for.

[calebcartwright]

Sure! I was trying to avoid having to do a borrow_mut and update the value unless absolutely necessary, and that was accomplished via both can_reset and has_non_ignorable_parser_errors. If there was only has_non_ignorable_parser_errors, then there could be a lot of repetitive borrow_mut setting the value to true over and over.

I'm not sure if/how problematic that would be, but that was my thought process anyway. I could add some inline comments to explain that, or I could remove it altogether if you'd prefer

[topecongiro]

Ah, so is can_reset used to avoid the overhead of calling borrow_mut multiple times? If that is the case, IMHO, this seems premature optimization to me. RefMut::borrow_mut is quite cheap (some non-atomic counter operations), and the code path is not hot anyway (it's only run on parse error).

[topecongiro]

Is it possible to remove can_reset and has_non_ignorable_parser_errors without changing the semantic? If possible, then I prefer to remove them to keep the code simple.

[calebcartwright]

Ah, so is can_reset used to avoid the overhead of calling borrow_mut multiple times?

Basically yes, and also because I wasn't sure if there'd ever be any competing borrow attempts which I'd read would panic. Sounds like there's no issues there so I'll remove that 👍

Is it possible to remove can_reset and has_non_ignorable_parser_errors without changing the semantic?

I can remove one of them, but AFAICT we'll need at least one flag to indicate whether parser errors from non-ignored files were encountered to ensure that we still fail in such a scenario. For example, if there are two files that have parser errors, foo.rs and bar.rs, and bar.rs is ignored, and foo is processed first and then bar. We should still fail due to the parser error in foo.rs

[calebcartwright]

I've updated this to only use the one flag. The only way I could see to remove them both would be to set the top level reset value to true, and then updating the emitter logic.

rustfmt/src/formatting.rs

Line 79 in 0531683

let can_reset_parser_errors = Rc::new(RefCell::new(false));

However, I don't think that the default behavior should be to ignore/reset parse errors encountered while parsing the crate. I fear that could result in spurious ignores of parser errors which would later cause problems when trying to parse/format the modules.

IMO it's better to have the emitter keep track of whether it saw a parser error from a non-ignored file via a flag, but please let me know if you'd prefer I change to the other approach!

[topecongiro]

Sorry for the late review. The code looks good to me.

For further refactoring, we better start with updating the libsyntax to expose its internal data structures.

[topecongiro]

I first thought that we could simply call db.handler.reset_err_count(); here instead, but looks like the rustc parser bumps the error count after calling emit_diagnostic:

https://github.com/rust-lang/rust/blob/f2023ac599c38a59f86552089e6791c5a73412d3/src/librustc_errors/lib.rs#L781-L791

Also like you mentioned, resetting the error count may cause skipping important parsing errors.

[calebcartwright]

I first thought that we could simply call db.handler.reset_err_count();

That's what I was expecting originally too 👍

here instead, but looks like the rustc parser bumps the error count after calling emit_diagnostic:

Thank you so much for finding that! I was baffled how calling reset_err_count() inside emit_diagnostic seemed to be doing nothing, but that makes much more sense now

[topecongiro]

@calebcartwright Working on #3835, I found that there were some refactorings around rustc_errors and breaking changes to its API. As #3835 is required to fix the broken build on the latest nightly, I am going to merge it first, but that would likely cause conflicts with this PR. Sorry for the inconvenience.

[calebcartwright]

@topecongiro - no worries! I'll update this to address any merge conflicts afterwards

[calebcartwright]

Fortunately looks like the merge conflicts are fairly minimal. I need to grab nightly-2019-10-05-x86_64-unknown-linux-gnu to make sure this is still working as expected since there were changes in rustc_errors. Will push the merge resolution after 👍

[topecongiro]

Perfect, thank you for all the work 🎉