rustc_errors: add downgrade_to_delayed_bug to Diagnostic itself.

Author: eddyb

compiler/rustc_codegen_ssa/src/back/write.rs

@@ -1709,7 +1709,7 @@ impl Emitter for SharedEmitter {
         drop(self.sender.send(SharedEmitterMessage::Diagnostic(Diagnostic {
             msg: diag.message(),
             code: diag.code.clone(),
-            lvl: diag.level,
+            lvl: diag.level(),
         })));
         for child in &diag.children {
             drop(self.sender.send(SharedEmitterMessage::Diagnostic(Diagnostic {

compiler/rustc_errors/src/annotate_snippet_emitter_writer.rs

@@ -66,7 +66,9 @@ fn source_string(file: Lrc<SourceFile>, line: &Line) -> String {
 /// Maps `Diagnostic::Level` to `snippet::AnnotationType`
 fn annotation_type_for_level(level: Level) -> AnnotationType {
     match level {
-        Level::Bug | Level::Fatal | Level::Error { .. } => AnnotationType::Error,
+        Level::Bug | Level::DelayedBug | Level::Fatal | Level::Error { .. } => {
+            AnnotationType::Error
+        }
         Level::Warning => AnnotationType::Warning,
         Level::Note => AnnotationType::Note,
         Level::Help => AnnotationType::Help,

compiler/rustc_errors/src/diagnostic.rs

@@ -19,7 +19,10 @@ pub struct SuggestionsDisabled;
 #[must_use]
 #[derive(Clone, Debug, Encodable, Decodable)]
 pub struct Diagnostic {
-    pub level: Level,
+    // NOTE(eddyb) this is private to disallow arbitrary after-the-fact changes,
+    // outside of what methods in this crate themselves allow.
+    crate level: Level,
+
     pub message: Vec<(String, Style)>,
     pub code: Option<DiagnosticId>,
     pub span: MultiSpan,
@@ -117,9 +120,18 @@ impl Diagnostic {
         }
     }
 
+    #[inline(always)]
+    pub fn level(&self) -> Level {
+        self.level
+    }
+
     pub fn is_error(&self) -> bool {
         match self.level {
-            Level::Bug | Level::Fatal | Level::Error { .. } | Level::FailureNote => true,
+            Level::Bug
+            | Level::DelayedBug
+            | Level::Fatal
+            | Level::Error { .. }
+            | Level::FailureNote => true,
 
             Level::Warning | Level::Note | Level::Help | Level::Cancelled | Level::Allow => false,
         }
@@ -150,6 +162,33 @@ impl Diagnostic {
         self.level == Level::Cancelled
     }
 
+    /// Delay emission of this diagnostic as a bug.
+    ///
+    /// This can be useful in contexts where an error indicates a bug but
+    /// typically this only happens when other compilation errors have already
+    /// happened. In those cases this can be used to defer emission of this
+    /// diagnostic as a bug in the compiler only if no other errors have been
+    /// emitted.
+    ///
+    /// In the meantime, though, callsites are required to deal with the "bug"
+    /// locally in whichever way makes the most sense.
+    #[track_caller]
+    pub fn downgrade_to_delayed_bug(&mut self) -> &mut Self {
+        // FIXME(eddyb) this check is only necessary because cancellation exists,
+        // but hopefully that can be removed in the future, if enough callers
+        // of `.cancel()` can take `DiagnosticBuilder`, and by-value.
+        if !self.cancelled() {
+            assert!(
+                self.is_error(),
+                "downgrade_to_delayed_bug: cannot downgrade {:?} to DelayedBug: not an error",
+                self.level
+            );
+            self.level = Level::DelayedBug;
+        }
+
+        self
+    }
+
     /// Adds a span/label to be included in the resulting snippet.
     ///
     /// This is pushed onto the [`MultiSpan`] that was created when the diagnostic

compiler/rustc_errors/src/diagnostic_builder.rs

@@ -105,10 +105,9 @@ impl<'a> DiagnosticBuilder<'a> {
     /// See `emit` and `delay_as_bug` for details.
     pub fn emit_unless(&mut self, delay: bool) {
         if delay {
-            self.delay_as_bug();
-        } else {
-            self.emit();
+            self.downgrade_to_delayed_bug();
         }
+        self.emit();
     }
 
     /// Stashes diagnostic for possible later improvement in a different,
@@ -162,12 +161,17 @@ impl<'a> DiagnosticBuilder<'a> {
     ///
     /// In the meantime, though, callsites are required to deal with the "bug"
     /// locally in whichever way makes the most sense.
+    #[track_caller]
     pub fn delay_as_bug(&mut self) {
-        self.level = Level::Bug;
-        self.handler.delay_as_bug((*self.diagnostic).clone());
-        self.cancel();
+        self.downgrade_to_delayed_bug();
+        self.emit();
     }
 
+    forward!(
+        #[track_caller]
+        pub fn downgrade_to_delayed_bug(&mut self,) -> &mut Self
+    );
+
     /// Appends a labeled span to the diagnostic.
     ///
     /// Labels are used to convey additional context for the diagnostic's primary span. They will

compiler/rustc_errors/src/lib.rs

@@ -491,10 +491,15 @@ impl Drop for HandlerInner {
             self.flush_delayed(bugs, "no errors encountered even though `delay_span_bug` issued");
         }
 
+        // FIXME(eddyb) this explains what `delayed_good_path_bugs` are!
+        // They're `delayed_span_bugs` but for "require some diagnostic happened"
+        // instead of "require some error happened". Sadly that isn't ideal, as
+        // lints can be `#[allow]`'d, potentially leading to this triggering.
+        // Also, "good path" should be replaced with a better naming.
         if !self.has_any_message() {
             let bugs = std::mem::replace(&mut self.delayed_good_path_bugs, Vec::new());
             self.flush_delayed(
-                bugs.into_iter().map(DelayedDiagnostic::decorate).collect(),
+                bugs.into_iter().map(DelayedDiagnostic::decorate),
                 "no warnings or errors encountered even though `delayed_good_path_bugs` issued",
             );
         }
@@ -815,6 +820,8 @@ impl Handler {
         self.inner.borrow_mut().delay_span_bug(span, msg)
     }
 
+    // FIXME(eddyb) note the comment inside `impl Drop for HandlerInner`, that's
+    // where the explanation of what "good path" is (also, it should be renamed).
     pub fn delay_good_path_bug(&self, msg: &str) {
         self.inner.borrow_mut().delay_good_path_bug(msg)
     }
@@ -915,10 +922,6 @@ impl Handler {
     pub fn emit_unused_externs(&self, lint_level: &str, unused_externs: &[&str]) {
         self.inner.borrow_mut().emit_unused_externs(lint_level, unused_externs)
     }
-
-    pub fn delay_as_bug(&self, diagnostic: Diagnostic) {
-        self.inner.borrow_mut().delay_as_bug(diagnostic)
-    }
 }
 
 impl HandlerInner {
@@ -936,11 +939,24 @@ impl HandlerInner {
         diags.iter().for_each(|diag| self.emit_diagnostic(diag));
     }
 
+    // FIXME(eddyb) this should ideally take `diagnostic` by value.
     fn emit_diagnostic(&mut self, diagnostic: &Diagnostic) {
         if diagnostic.cancelled() {
             return;
         }
 
+        if diagnostic.level == Level::DelayedBug {
+            // FIXME(eddyb) this should check for `has_errors` and stop pushing
+            // once *any* errors were emitted (and truncate `delayed_span_bugs`
+            // when an error is first emitted, also), but maybe there's a case
+            // in which that's not sound? otherwise this is really inefficient.
+            self.delayed_span_bugs.push(diagnostic.clone());
+
+            if !self.flags.report_delayed_bugs {
+                return;
+            }
+        }
+
         if diagnostic.has_future_breakage() {
             self.future_breakage_diagnostics.push(diagnostic.clone());
         }
@@ -1119,14 +1135,16 @@ impl HandlerInner {
             // FIXME: don't abort here if report_delayed_bugs is off
             self.span_bug(sp, msg);
         }
-        let mut diagnostic = Diagnostic::new(Level::Bug, msg);
+        let mut diagnostic = Diagnostic::new(Level::DelayedBug, msg);
         diagnostic.set_span(sp.into());
         diagnostic.note(&format!("delayed at {}", std::panic::Location::caller()));
-        self.delay_as_bug(diagnostic)
+        self.emit_diagnostic(&diagnostic)
     }
 
+    // FIXME(eddyb) note the comment inside `impl Drop for HandlerInner`, that's
+    // where the explanation of what "good path" is (also, it should be renamed).
     fn delay_good_path_bug(&mut self, msg: &str) {
-        let diagnostic = Diagnostic::new(Level::Bug, msg);
+        let diagnostic = Diagnostic::new(Level::DelayedBug, msg);
         if self.flags.report_delayed_bugs {
             self.emit_diagnostic(&diagnostic);
         }
@@ -1160,20 +1178,34 @@ impl HandlerInner {
         panic::panic_any(ExplicitBug);
     }
 
-    fn delay_as_bug(&mut self, diagnostic: Diagnostic) {
-        if self.flags.report_delayed_bugs {
-            self.emit_diagnostic(&diagnostic);
-        }
-        self.delayed_span_bugs.push(diagnostic);
-    }
+    fn flush_delayed(&mut self, bugs: impl IntoIterator<Item = Diagnostic>, explanation: &str) {
+        let mut no_bugs = true;
+        for mut bug in bugs {
+            if no_bugs {
+                // Put the overall explanation before the `DelayedBug`s, to
+                // frame them better (e.g. separate warnings from them).
+                self.emit_diagnostic(&Diagnostic::new(Bug, explanation));
+                no_bugs = false;
+            }
+
+            // "Undelay" the `DelayedBug`s (into plain `Bug`s).
+            if bug.level != Level::DelayedBug {
+                // NOTE(eddyb) not panicking here because we're already producing
+                // an ICE, and the more information the merrier.
+                bug.note(&format!(
+                    "`flushed_delayed` got diagnostic with level {:?}, \
+                     instead of the expected `DelayedBug`",
+                    bug.level,
+                ));
+            }
+            bug.level = Level::Bug;
 
-    fn flush_delayed(&mut self, bugs: Vec<Diagnostic>, explanation: &str) {
-        let has_bugs = !bugs.is_empty();
-        for bug in bugs {
             self.emit_diagnostic(&bug);
         }
-        if has_bugs {
-            panic!("{}", explanation);
+
+        // Panic with `ExplicitBug` to avoid "unexpected panic" messages.
+        if !no_bugs {
+            panic::panic_any(ExplicitBug);
         }
     }
 
@@ -1227,6 +1259,7 @@ impl DelayedDiagnostic {
 #[derive(Copy, PartialEq, Clone, Hash, Debug, Encodable, Decodable)]
 pub enum Level {
     Bug,
+    DelayedBug,
     Fatal,
     Error {
         /// If this error comes from a lint, don't abort compilation even when abort_if_errors() is called.
@@ -1250,7 +1283,7 @@ impl Level {
     fn color(self) -> ColorSpec {
         let mut spec = ColorSpec::new();
         match self {
-            Bug | Fatal | Error { .. } => {
+            Bug | DelayedBug | Fatal | Error { .. } => {
                 spec.set_fg(Some(Color::Red)).set_intense(true);
             }
             Warning => {
@@ -1270,7 +1303,7 @@ impl Level {
 
     pub fn to_str(self) -> &'static str {
         match self {
-            Bug => "error: internal compiler error",
+            Bug | DelayedBug => "error: internal compiler error",
             Fatal | Error { .. } => "error",
             Warning => "warning",
             Note => "note",

compiler/rustc_resolve/src/late/diagnostics.rs

@@ -1066,7 +1066,7 @@ impl<'a: 'ast, 'ast> LateResolutionVisitor<'a, '_, 'ast> {
                     })
                     .unwrap_or(false)
                 {
-                    err.delay_as_bug();
+                    err.downgrade_to_delayed_bug();
                     // We already suggested changing `:` into `::` during parsing.
                     return false;
                 }
@@ -1472,7 +1472,7 @@ impl<'a: 'ast, 'ast> LateResolutionVisitor<'a, '_, 'ast> {
                                 .borrow_mut()
                                 .insert(colon_sp)
                             {
-                                err.delay_as_bug();
+                                err.downgrade_to_delayed_bug();
                             }
                         }
                         if let Ok(base_snippet) = base_snippet {

compiler/rustc_typeck/src/check/demand.rs

@@ -251,7 +251,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
                 if !lhs.is_syntactic_place_expr() {
                     // We already emitted E0070 "invalid left-hand side of assignment", so we
                     // silence this.
-                    err.delay_as_bug();
+                    err.downgrade_to_delayed_bug();
                 }
             }
             _ => {}

compiler/rustc_typeck/src/check/fn_ctxt/checks.rs

@@ -837,7 +837,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
                                             kind: hir::ExprKind::Loop(_, _, hir::LoopSource::While, _),
                                             ..
                                         })) => {
-                                            err.delay_as_bug();
+                                            err.downgrade_to_delayed_bug();
                                         }
                                         _ => {}
                                     }

src/librustdoc/passes/collect_intra_doc_links.rs

@@ -2098,8 +2098,15 @@ fn resolution_failure(
                         )
                     }
                     ResolutionFailure::NoParentItem => {
-                        diag.level = rustc_errors::Level::Bug;
-                        "all intra-doc links should have a parent item".to_owned()
+                        // FIXME(eddyb) this doesn't belong here, whatever made
+                        // the `ResolutionFailure::NoParentItem` should emit an
+                        // immediate or delayed `span_bug` about the issue.
+                        tcx.sess.delay_span_bug(
+                            sp.unwrap_or(DUMMY_SP),
+                            "intra-doc link missing parent item",
+                        );
+
+                        "BUG: all intra-doc links should have a parent item".to_owned()
                     }
                     ResolutionFailure::MalformedGenerics(variant) => match variant {
                         MalformedGenerics::UnbalancedAngleBrackets => {

src/tools/rustfmt/src/parse/session.rs

@@ -60,7 +60,7 @@ impl Emitter for SilentOnIgnoredFilesEmitter {
         None
     }
     fn emit_diagnostic(&mut self, db: &Diagnostic) {
-        if db.level == DiagnosticLevel::Fatal {
+        if db.level() == DiagnosticLevel::Fatal {
             return self.handle_non_ignoreable_error(db);
         }
         if let Some(primary_span) = &db.span.primary_span() {
@@ -292,7 +292,7 @@ mod tests {
         use super::*;
         use crate::config::IgnoreList;
         use crate::utils::mk_sp;
-        use rustc_span::{FileName as SourceMapFileName, MultiSpan, RealFileName, DUMMY_SP};
+        use rustc_span::{FileName as SourceMapFileName, MultiSpan, RealFileName};
         use std::path::PathBuf;
         use std::sync::atomic::AtomicU32;
 
@@ -310,16 +310,12 @@ mod tests {
         }
 
         fn build_diagnostic(level: DiagnosticLevel, span: Option<MultiSpan>) -> Diagnostic {
-            Diagnostic {
-                level,
-                code: None,
-                message: vec![],
-                children: vec![],
-                suggestions: Ok(vec![]),
-                span: span.unwrap_or_else(MultiSpan::new),
-                sort_span: DUMMY_SP,
-                is_lint: false,
+            let mut diag = Diagnostic::new(level, "");
+            diag.message.clear();
+            if let Some(span) = span {
+                diag.span = span;
             }
+            diag
         }
 
         fn build_emitter(