From bf7d1b54e8a3195a0f52516d48659d9f1270436d Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Mon, 26 Feb 2024 09:24:03 +1100 Subject: [PATCH] Move `emit_stashed_diagnostic` call in rustfmt. This call was added to `parse_crate_mod` in #121487, to fix a case where a stashed diagnostic wasn't emitted. But there is another path where a stashed diagnostic might fail to be emitted if there's a parse error, if the `build` call in `parse_crate_inner` fails before `parse_crate_mod` is reached. So this commit moves the `emit_stashed_diagnostic` call outwards, from `parse_crate_mod` to `format_project`, just after the `Parser::parse_crate` call. This should be far out enough to catch any parsing errors. Fixes #121517. --- src/tools/rustfmt/src/formatting.rs | 22 ++++++++++++++----- src/tools/rustfmt/src/parse/parser.rs | 16 ++++---------- src/tools/rustfmt/src/parse/session.rs | 6 ++++- src/tools/rustfmt/src/test/parser.rs | 7 ++++++ .../rustfmt/tests/parser/stashed-diag2.rs | 3 +++ 5 files changed, 35 insertions(+), 19 deletions(-) create mode 100644 src/tools/rustfmt/tests/parser/stashed-diag2.rs diff --git a/src/tools/rustfmt/src/formatting.rs b/src/tools/rustfmt/src/formatting.rs index cd57a025b6726..323ae83fe6ede 100644 --- a/src/tools/rustfmt/src/formatting.rs +++ b/src/tools/rustfmt/src/formatting.rs @@ -109,7 +109,7 @@ fn format_project( let main_file = input.file_name(); let input_is_stdin = main_file == FileName::Stdin; - let parse_session = ParseSess::new(config)?; + let mut parse_session = ParseSess::new(config)?; if config.skip_children() && parse_session.ignore_file(&main_file) { return Ok(FormatReport::new()); } @@ -117,11 +117,21 @@ fn format_project( // Parse the crate. let mut report = FormatReport::new(); let directory_ownership = input.to_directory_ownership(); - let krate = match Parser::parse_crate(input, &parse_session) { - Ok(krate) => krate, - // Surface parse error via Session (errors are merged there from report) - Err(e) => { - let forbid_verbose = input_is_stdin || e != ParserError::ParsePanicError; + + // rustfmt doesn't use `run_compiler` like other tools, so it must emit any + // stashed diagnostics itself, otherwise the `DiagCtxt` will assert when + // dropped. The final result here combines the parsing result and the + // `emit_stashed_diagnostics` result. + let parse_res = Parser::parse_crate(input, &parse_session); + let stashed_res = parse_session.emit_stashed_diagnostics(); + let krate = match (parse_res, stashed_res) { + (Ok(krate), None) => krate, + (parse_res, _) => { + // Surface parse error via Session (errors are merged there from report). + let forbid_verbose = match parse_res { + Err(e) if e != ParserError::ParsePanicError => true, + _ => input_is_stdin, + }; should_emit_verbose(forbid_verbose, config, || { eprintln!("The Rust parser panicked"); }); diff --git a/src/tools/rustfmt/src/parse/parser.rs b/src/tools/rustfmt/src/parse/parser.rs index cca14353b5c18..3269fe7c7c706 100644 --- a/src/tools/rustfmt/src/parse/parser.rs +++ b/src/tools/rustfmt/src/parse/parser.rs @@ -162,22 +162,14 @@ impl<'a> Parser<'a> { fn parse_crate_mod(&mut self) -> Result { let mut parser = AssertUnwindSafe(&mut self.parser); - - // rustfmt doesn't use `run_compiler` like other tools, so it must emit - // any stashed diagnostics itself, otherwise the `DiagCtxt` will assert - // when dropped. The final result here combines the parsing result and - // the `emit_stashed_diagnostics` result. - let parse_res = catch_unwind(move || parser.parse_crate_mod()); - let stashed_res = self.parser.dcx().emit_stashed_diagnostics(); let err = Err(ParserError::ParsePanicError); - match (parse_res, stashed_res) { - (Ok(Ok(k)), None) => Ok(k), - (Ok(Ok(_)), Some(_guar)) => err, - (Ok(Err(db)), _) => { + match catch_unwind(move || parser.parse_crate_mod()) { + Ok(Ok(k)) => Ok(k), + Ok(Err(db)) => { db.emit(); err } - (Err(_), _) => err, + Err(_) => err, } } } diff --git a/src/tools/rustfmt/src/parse/session.rs b/src/tools/rustfmt/src/parse/session.rs index cff025cf2ab3a..decd3b167e0a5 100644 --- a/src/tools/rustfmt/src/parse/session.rs +++ b/src/tools/rustfmt/src/parse/session.rs @@ -6,7 +6,7 @@ use rustc_data_structures::sync::{IntoDynSyncSend, Lrc}; use rustc_errors::emitter::{DynEmitter, Emitter, HumanEmitter}; use rustc_errors::translation::Translate; use rustc_errors::{ - ColorConfig, DiagCtxt, Diagnostic, DiagnosticBuilder, Level as DiagnosticLevel, + ColorConfig, DiagCtxt, Diagnostic, DiagnosticBuilder, ErrorGuaranteed, Level as DiagnosticLevel, }; use rustc_session::parse::ParseSess as RawParseSess; use rustc_span::{ @@ -230,6 +230,10 @@ impl ParseSess { self.ignore_path_set.as_ref().is_match(path) } + pub(crate) fn emit_stashed_diagnostics(&mut self) -> Option { + self.parse_sess.dcx.emit_stashed_diagnostics() + } + pub(crate) fn set_silent_emitter(&mut self) { self.parse_sess.dcx = DiagCtxt::with_emitter(silent_emitter()); } diff --git a/src/tools/rustfmt/src/test/parser.rs b/src/tools/rustfmt/src/test/parser.rs index da2a2ba62e009..d951c8469e661 100644 --- a/src/tools/rustfmt/src/test/parser.rs +++ b/src/tools/rustfmt/src/test/parser.rs @@ -62,3 +62,10 @@ fn crate_parsing_stashed_diag() { let filename = "tests/parser/stashed-diag.rs"; assert_parser_error(filename); } + +#[test] +fn crate_parsing_stashed_diag2() { + // See also https://github.com/rust-lang/rust/issues/121517 + let filename = "tests/parser/stashed-diag2.rs"; + assert_parser_error(filename); +} diff --git a/src/tools/rustfmt/tests/parser/stashed-diag2.rs b/src/tools/rustfmt/tests/parser/stashed-diag2.rs new file mode 100644 index 0000000000000..579a69def1633 --- /dev/null +++ b/src/tools/rustfmt/tests/parser/stashed-diag2.rs @@ -0,0 +1,3 @@ +trait Trait<'1> { s> {} + +fn main() {}