rustc_errors: take `self` by value in `DiagnosticBuilder::cancel`.

4 files changed, 99 insertions, 55 deletions

diff --git a/compiler/rustc_errors/src/annotate_snippet_emitter_writer.rs b/compiler/rustc_errors/src/annotate_snippet_emitter_writer.rs
index 2835afb0208..7d7ab1ed4e5 100644
--- a/compiler/rustc_errors/src/annotate_snippet_emitter_writer.rs
+++ b/compiler/rustc_errors/src/annotate_snippet_emitter_writer.rs
@@ -72,8 +72,8 @@ fn annotation_type_for_level(level: Level) -> AnnotationType {
         Level::Warning => AnnotationType::Warning,
         Level::Note => AnnotationType::Note,
         Level::Help => AnnotationType::Help,
-        // FIXME(#59346): Not sure how to map these two levels
-        Level::Cancelled | Level::FailureNote => AnnotationType::Error,
+        // FIXME(#59346): Not sure how to map this level
+        Level::FailureNote => AnnotationType::Error,
         Level::Allow => panic!("Should not call with Allow"),
     }
 }
diff --git a/compiler/rustc_errors/src/diagnostic.rs b/compiler/rustc_errors/src/diagnostic.rs
index f32b11e33c8..6d6ada86428 100644
--- a/compiler/rustc_errors/src/diagnostic.rs
+++ b/compiler/rustc_errors/src/diagnostic.rs
@@ -133,7 +133,7 @@ impl Diagnostic {
             | Level::Error { .. }
             | Level::FailureNote => true,
 
-            Level::Warning | Level::Note | Level::Help | Level::Cancelled | Level::Allow => false,
+            Level::Warning | Level::Note | Level::Help | Level::Allow => false,
         }
     }
 
@@ -151,17 +151,6 @@ impl Diagnostic {
         }
     }
 
-    /// Cancel the diagnostic (a structured diagnostic must either be emitted or
-    /// canceled or it will panic when dropped).
-    pub fn cancel(&mut self) {
-        self.level = Level::Cancelled;
-    }
-
-    /// Check if this diagnostic [was cancelled][Self::cancel()].
-    pub fn cancelled(&self) -> bool {
-        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
@@ -174,17 +163,12 @@ impl Diagnostic {
     /// 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;
-        }
+        assert!(
+            self.is_error(),
+            "downgrade_to_delayed_bug: cannot downgrade {:?} to DelayedBug: not an error",
+            self.level
+        );
+        self.level = Level::DelayedBug;
 
         self
     }
diff --git a/compiler/rustc_errors/src/diagnostic_builder.rs b/compiler/rustc_errors/src/diagnostic_builder.rs
index e0a5e6ef089..7978e1cc162 100644
--- a/compiler/rustc_errors/src/diagnostic_builder.rs
+++ b/compiler/rustc_errors/src/diagnostic_builder.rs
@@ -16,7 +16,7 @@ use tracing::debug;
 #[must_use]
 #[derive(Clone)]
 pub struct DiagnosticBuilder<'a> {
-    handler: &'a Handler,
+    state: DiagnosticBuilderState<'a>,
 
     /// `Diagnostic` is a large type, and `DiagnosticBuilder` is often used as a
     /// return value, especially within the frequently-used `PResult` type.
@@ -25,6 +25,34 @@ pub struct DiagnosticBuilder<'a> {
     diagnostic: Box<Diagnostic>,
 }
 
+#[derive(Clone)]
+enum DiagnosticBuilderState<'a> {
+    /// Initial state of a `DiagnosticBuilder`, before `.emit()` or `.cancel()`.
+    ///
+    /// The `Diagnostic` will be emitted through this `Handler`.
+    Emittable(&'a Handler),
+
+    /// State of a `DiagnosticBuilder`, after `.emit()` or *during* `.cancel()`.
+    ///
+    /// The `Diagnostic` will be ignored when calling `.emit()`, and it can be
+    /// assumed that `.emit()` was previously called, to end up in this state.
+    ///
+    /// While this is also used by `.cancel()`, this state is only observed by
+    /// the `Drop` `impl` of `DiagnosticBuilder`, as `.cancel()` takes `self`
+    /// by-value specifically to prevent any attempts to `.emit()`.
+    ///
+    // FIXME(eddyb) currently this doesn't prevent extending the `Diagnostic`,
+    // despite that being potentially lossy, if important information is added
+    // *after* the original `.emit()` call.
+    AlreadyEmittedOrDuringCancellation,
+}
+
+// `DiagnosticBuilderState` should be pointer-sized.
+rustc_data_structures::static_assert_size!(
+    DiagnosticBuilderState<'_>,
+    std::mem::size_of::<&Handler>()
+);
+
 /// In general, the `DiagnosticBuilder` uses deref to allow access to
 /// the fields and methods of the embedded `diagnostic` in a
 /// transparent way. *However,* many of the methods are intended to
@@ -78,8 +106,18 @@ impl<'a> DerefMut for DiagnosticBuilder<'a> {
 impl<'a> DiagnosticBuilder<'a> {
     /// Emit the diagnostic.
     pub fn emit(&mut self) {
-        self.handler.emit_diagnostic(&self);
-        self.cancel();
+        match self.state {
+            // First `.emit()` call, the `&Handler` is still available.
+            DiagnosticBuilderState::Emittable(handler) => {
+                handler.emit_diagnostic(&self);
+                self.state = DiagnosticBuilderState::AlreadyEmittedOrDuringCancellation;
+            }
+            // `.emit()` was previously called, disallowed from repeating it.
+            DiagnosticBuilderState::AlreadyEmittedOrDuringCancellation => {
+                // FIXME(eddyb) rely on this to return a "proof" that an error
+                // was/will be emitted, despite doing no emission *here and now*.
+            }
+        }
     }
 
     /// Emit the diagnostic unless `delay` is true,
@@ -93,6 +131,17 @@ impl<'a> DiagnosticBuilder<'a> {
         self.emit();
     }
 
+    /// Cancel the diagnostic (a structured diagnostic must either be emitted or
+    /// cancelled or it will panic when dropped).
+    ///
+    /// This method takes `self` by-value to disallow calling `.emit()` on it,
+    /// which may be expected to *guarantee* the emission of an error, either
+    /// at the time of the call, or through a prior `.emit()` call.
+    pub fn cancel(mut self) {
+        self.state = DiagnosticBuilderState::AlreadyEmittedOrDuringCancellation;
+        drop(self);
+    }
+
     /// Stashes diagnostic for possible later improvement in a different,
     /// later stage of the compiler. The diagnostic can be accessed with
     /// the provided `span` and `key` through [`Handler::steal_diagnostic()`].
@@ -105,22 +154,29 @@ impl<'a> DiagnosticBuilder<'a> {
     }
 
     /// Converts the builder to a `Diagnostic` for later emission,
-    /// unless handler has disabled such buffering.
+    /// unless handler has disabled such buffering, or `.emit()` was called.
     pub fn into_diagnostic(mut self) -> Option<(Diagnostic, &'a Handler)> {
-        if self.handler.flags.dont_buffer_diagnostics
-            || self.handler.flags.treat_err_as_bug.is_some()
-        {
+        let handler = match self.state {
+            // No `.emit()` calls, the `&Handler` is still available.
+            DiagnosticBuilderState::Emittable(handler) => handler,
+            // `.emit()` was previously called, nothing we can do.
+            DiagnosticBuilderState::AlreadyEmittedOrDuringCancellation => {
+                return None;
+            }
+        };
+
+        if handler.flags.dont_buffer_diagnostics || handler.flags.treat_err_as_bug.is_some() {
             self.emit();
             return None;
         }
 
-        let handler = self.handler;
-
-        // We must use `Level::Cancelled` for `dummy` to avoid an ICE about an
-        // unused diagnostic.
-        let dummy = Diagnostic::new(Level::Cancelled, "");
+        // Take the `Diagnostic` by replacing it with a dummy.
+        let dummy = Diagnostic::new(Level::Allow, "");
         let diagnostic = std::mem::replace(&mut *self.diagnostic, dummy);
 
+        // Disable the ICE on `Drop`.
+        self.cancel();
+
         // Logging here is useful to help track down where in logs an error was
         // actually emitted.
         debug!("buffer: diagnostic={:?}", diagnostic);
@@ -314,7 +370,10 @@ impl<'a> DiagnosticBuilder<'a> {
     /// diagnostic.
     crate fn new_diagnostic(handler: &'a Handler, diagnostic: Diagnostic) -> DiagnosticBuilder<'a> {
         debug!("Created new diagnostic");
-        DiagnosticBuilder { handler, diagnostic: Box::new(diagnostic) }
+        DiagnosticBuilder {
+            state: DiagnosticBuilderState::Emittable(handler),
+            diagnostic: Box::new(diagnostic),
+        }
     }
 }
 
@@ -324,19 +383,26 @@ impl<'a> Debug for DiagnosticBuilder<'a> {
     }
 }
 
-/// Destructor bomb - a `DiagnosticBuilder` must be either emitted or canceled
+/// Destructor bomb - a `DiagnosticBuilder` must be either emitted or cancelled
 /// or we emit a bug.
 impl<'a> Drop for DiagnosticBuilder<'a> {
     fn drop(&mut self) {
-        if !panicking() && !self.cancelled() {
-            let mut db = DiagnosticBuilder::new(
-                self.handler,
-                Level::Bug,
-                "the following error was constructed but not emitted",
-            );
-            db.emit();
-            self.emit();
-            panic!();
+        match self.state {
+            // No `.emit()` or `.cancel()` calls.
+            DiagnosticBuilderState::Emittable(handler) => {
+                if !panicking() {
+                    let mut db = DiagnosticBuilder::new(
+                        handler,
+                        Level::Bug,
+                        "the following error was constructed but not emitted",
+                    );
+                    db.emit();
+                    handler.emit_diagnostic(&self);
+                    panic!();
+                }
+            }
+            // `.emit()` was previously called, or maybe we're during `.cancel()`.
+            DiagnosticBuilderState::AlreadyEmittedOrDuringCancellation => {}
         }
     }
 }
diff --git a/compiler/rustc_errors/src/lib.rs b/compiler/rustc_errors/src/lib.rs
index 34f52d78ec9..b92b1cac2e8 100644
--- a/compiler/rustc_errors/src/lib.rs
+++ b/compiler/rustc_errors/src/lib.rs
@@ -911,10 +911,6 @@ impl HandlerInner {
 
     // 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`
@@ -1238,7 +1234,6 @@ pub enum Level {
     Warning,
     Note,
     Help,
-    Cancelled,
     FailureNote,
     Allow,
 }
@@ -1266,7 +1261,7 @@ impl Level {
                 spec.set_fg(Some(Color::Cyan)).set_intense(true);
             }
             FailureNote => {}
-            Allow | Cancelled => unreachable!(),
+            Allow => unreachable!(),
         }
         spec
     }
@@ -1279,7 +1274,6 @@ impl Level {
             Note => "note",
             Help => "help",
             FailureNote => "failure-note",
-            Cancelled => panic!("Shouldn't call on cancelled error"),
             Allow => panic!("Shouldn't call on allowed error"),
         }
     }