Move emit_stashed_diagnostic call in rustfmt.

nnethercote · nnethercote · commit bf7d1b54e8a3 · 2024-02-26T10:45:45.000+11:00
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.
diff --git a/src/tools/rustfmt/src/formatting.rs b/src/tools/rustfmt/src/formatting.rs
@@ -109,19 +109,29 @@ fn format_project<T: FormatHandler>(
     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());
     }
 
     // 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
@@ -162,22 +162,14 @@ impl<'a> Parser<'a> {
 
     fn parse_crate_mod(&mut self) -> Result<ast::Crate, ParserError> {
         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
@@ -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<ErrorGuaranteed> {
+        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
@@ -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
@@ -0,0 +1,3 @@
+trait Trait<'1> { s> {}
+
+fn main() {}

Original file line number	Diff line number	Diff line change
`@@ -0,0 +1,3 @@`
	`1`	`+trait Trait<'1> { s> {}`
	`2`	`+`
	`3`	`+fn main() {}`