Remove DiagnosticBuilderState.

nnethercote · nnethercote · commit 2d91c6d1bf90 · 2024-01-08T16:18:54.000+11:00
Currently it's used for two dynamic checks:
- When a diagnostic is emitted, has it been emitted before?
- When a diagnostic is dropped, has it been emitted/cancelled?

The first check is no longer need, because `emit` is consuming, so it's
impossible to emit a `DiagnosticBuilder` twice. The second check is
still needed.

This commit replaces `DiagnosticBuilderState` with a simpler
`Option&lt;Box&lt;Diagnostic&gt;&gt;`, which is enough for the second check:
functions like `emit` and `cancel` can take the `Diagnostic` and then
`drop` can check that the `Diagnostic` was taken.

The `DiagCtxt` reference from `DiagnosticBuilderState` is now stored as
its own field, removing the need for the `dcx` method.

As well as making the code shorter and simpler, the commit removes:
- One (deprecated) `ErrorGuaranteed::unchecked_claim_error_was_emitted`
  call.
- Two `FIXME(eddyb)` comments that are no longer relevant.
- The use of a dummy `Diagnostic` in `into_diagnostic`.

Nice!
diff --git a/compiler/rustc_errors/src/diagnostic_builder.rs b/compiler/rustc_errors/src/diagnostic_builder.rs
@@ -35,19 +35,28 @@ where
 }
 
 /// Used for emitting structured error messages and other diagnostic information.
+/// Each constructed `DiagnosticBuilder` must be consumed by a function such as
+/// `emit`, `cancel`, `delay_as_bug`, or `into_diagnostic`. A panic occurrs if a
+/// `DiagnosticBuilder` is dropped without being consumed by one of these
+/// functions.
 ///
 /// If there is some state in a downstream crate you would like to
 /// access in the methods of `DiagnosticBuilder` here, consider
 /// extending `DiagCtxtFlags`.
 #[must_use]
 pub struct DiagnosticBuilder<'a, G: EmissionGuarantee = ErrorGuaranteed> {
-    state: DiagnosticBuilderState<'a>,
+    pub dcx: &'a DiagCtxt,
 
-    /// `Diagnostic` is a large type, and `DiagnosticBuilder` is often used as a
-    /// return value, especially within the frequently-used `PResult` type.
-    /// In theory, return value optimization (RVO) should avoid unnecessary
-    /// copying. In practice, it does not (at the time of writing).
-    diagnostic: Box<Diagnostic>,
+    /// Why the `Option`? It is always `Some` until the `DiagnosticBuilder` is
+    /// consumed via `emit`, `cancel`, etc. At that point it is consumed and
+    /// replaced with `None`. Then `drop` checks that it is `None`; if not, it
+    /// panics because a diagnostic was built but not used.
+    ///
+    /// Why the Box? `Diagnostic` is a large type, and `DiagnosticBuilder` is
+    /// often used as a return value, especially within the frequently-used
+    /// `PResult` type. In theory, return value optimization (RVO) should avoid
+    /// unnecessary copying. In practice, it does not (at the time of writing).
+    diag: Option<Box<Diagnostic>>,
 
     _marker: PhantomData<G>,
 }
@@ -56,32 +65,9 @@ pub struct DiagnosticBuilder<'a, G: EmissionGuarantee = ErrorGuaranteed> {
 // twice, which would be bad.
 impl<G> !Clone for DiagnosticBuilder<'_, G> {}
 
-#[derive(Clone)]
-enum DiagnosticBuilderState<'a> {
-    /// Initial state of a `DiagnosticBuilder`, before `.emit()` or `.cancel()`.
-    ///
-    /// The `Diagnostic` will be emitted through this `DiagCtxt`.
-    Emittable(&'a DiagCtxt),
-
-    /// 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`, because `.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::<&DiagCtxt>()
+    DiagnosticBuilder<'_, ()>,
+    2 * std::mem::size_of::<usize>()
 );
 
 /// Trait for types that `DiagnosticBuilder::emit` can return as a "guarantee"
@@ -99,62 +85,44 @@ pub trait EmissionGuarantee: Sized {
 }
 
 impl<'a, G: EmissionGuarantee> DiagnosticBuilder<'a, G> {
+    /// Takes the diagnostic. For use by methods that consume the
+    /// DiagnosticBuilder: `emit`, `cancel`, etc. Afterwards, `drop` is the
+    /// only code that will be run on `self`.
+    fn take_diag(&mut self) -> Diagnostic {
+        Box::into_inner(self.diag.take().unwrap())
+    }
+
     /// Most `emit_producing_guarantee` functions use this as a starting point.
     fn emit_producing_nothing(mut self) {
-        match self.state {
-            // First `.emit()` call, the `&DiagCtxt` is still available.
-            DiagnosticBuilderState::Emittable(dcx) => {
-                self.state = DiagnosticBuilderState::AlreadyEmittedOrDuringCancellation;
-                dcx.emit_diagnostic_without_consuming(&mut self.diagnostic);
-            }
-            // `.emit()` was previously called, disallowed from repeating it.
-            DiagnosticBuilderState::AlreadyEmittedOrDuringCancellation => {}
-        }
+        let diag = self.take_diag();
+        self.dcx.emit_diagnostic(diag);
+    }
+
+    /// `ErrorGuaranteed::emit_producing_guarantee` uses this.
+    // FIXME(eddyb) make `ErrorGuaranteed` impossible to create outside `.emit()`.
+    fn emit_producing_error_guaranteed(mut self) -> ErrorGuaranteed {
+        let diag = self.take_diag();
+
+        // Only allow a guarantee if the `level` wasn't switched to a
+        // non-error. The field isn't `pub`, but the whole `Diagnostic` can be
+        // overwritten with a new one, thanks to `DerefMut`.
+        assert!(
+            diag.is_error(),
+            "emitted non-error ({:?}) diagnostic from `DiagnosticBuilder<ErrorGuaranteed>`",
+            diag.level,
+        );
+
+        let guar = self.dcx.emit_diagnostic(diag);
+        guar.unwrap()
     }
 }
 
-// FIXME(eddyb) make `ErrorGuaranteed` impossible to create outside `.emit()`.
 impl EmissionGuarantee for ErrorGuaranteed {
-    fn emit_producing_guarantee(mut db: DiagnosticBuilder<'_, Self>) -> Self::EmitResult {
-        // Contrast this with `emit_producing_nothing`.
-        match db.state {
-            // First `.emit()` call, the `&DiagCtxt` is still available.
-            DiagnosticBuilderState::Emittable(dcx) => {
-                db.state = DiagnosticBuilderState::AlreadyEmittedOrDuringCancellation;
-                let guar = dcx.emit_diagnostic_without_consuming(&mut db.diagnostic);
-
-                // Only allow a guarantee if the `level` wasn't switched to a
-                // non-error - the field isn't `pub`, but the whole `Diagnostic`
-                // can be overwritten with a new one, thanks to `DerefMut`.
-                assert!(
-                    db.diagnostic.is_error(),
-                    "emitted non-error ({:?}) diagnostic \
-                     from `DiagnosticBuilder<ErrorGuaranteed>`",
-                    db.diagnostic.level,
-                );
-                guar.unwrap()
-            }
-            // `.emit()` was previously called, disallowed from repeating it,
-            // but can take advantage of the previous `.emit()`'s guarantee
-            // still being applicable (i.e. as a form of idempotency).
-            DiagnosticBuilderState::AlreadyEmittedOrDuringCancellation => {
-                // Only allow a guarantee if the `level` wasn't switched to a
-                // non-error - the field isn't `pub`, but the whole `Diagnostic`
-                // can be overwritten with a new one, thanks to `DerefMut`.
-                assert!(
-                    db.diagnostic.is_error(),
-                    "`DiagnosticBuilder<ErrorGuaranteed>`'s diagnostic \
-                     became non-error ({:?}), after original `.emit()`",
-                    db.diagnostic.level,
-                );
-                #[allow(deprecated)]
-                ErrorGuaranteed::unchecked_claim_error_was_emitted()
-            }
-        }
+    fn emit_producing_guarantee(db: DiagnosticBuilder<'_, Self>) -> Self::EmitResult {
+        db.emit_producing_error_guaranteed()
     }
 }
 
-// FIXME(eddyb) should there be a `Option<ErrorGuaranteed>` impl as well?
 impl EmissionGuarantee for () {
     fn emit_producing_guarantee(db: DiagnosticBuilder<'_, Self>) -> Self::EmitResult {
         db.emit_producing_nothing();
@@ -219,12 +187,12 @@ macro_rules! forward {
     ) => {
         #[doc = concat!("See [`Diagnostic::", stringify!($n), "()`].")]
         pub fn $n(&mut self, $($name: $ty),*) -> &mut Self {
-            self.diagnostic.$n($($name),*);
+            self.diag.as_mut().unwrap().$n($($name),*);
             self
         }
         #[doc = concat!("See [`Diagnostic::", stringify!($n), "()`].")]
         pub fn $n_mv(mut self, $($name: $ty),*) -> Self {
-            self.diagnostic.$n($($name),*);
+            self.diag.as_mut().unwrap().$n($($name),*);
             self
         }
     };
@@ -234,13 +202,13 @@ impl<G: EmissionGuarantee> Deref for DiagnosticBuilder<'_, G> {
     type Target = Diagnostic;
 
     fn deref(&self) -> &Diagnostic {
-        &self.diagnostic
+        self.diag.as_ref().unwrap()
     }
 }
 
 impl<G: EmissionGuarantee> DerefMut for DiagnosticBuilder<'_, G> {
     fn deref_mut(&mut self) -> &mut Diagnostic {
-        &mut self.diagnostic
+        self.diag.as_mut().unwrap()
     }
 }
 
@@ -254,13 +222,9 @@ impl<'a, G: EmissionGuarantee> DiagnosticBuilder<'a, G> {
     /// Creates a new `DiagnosticBuilder` with an already constructed
     /// diagnostic.
     #[track_caller]
-    pub(crate) fn new_diagnostic(dcx: &'a DiagCtxt, diagnostic: Diagnostic) -> Self {
+    pub(crate) fn new_diagnostic(dcx: &'a DiagCtxt, diag: Diagnostic) -> Self {
         debug!("Created new diagnostic");
-        Self {
-            state: DiagnosticBuilderState::Emittable(dcx),
-            diagnostic: Box::new(diagnostic),
-            _marker: PhantomData,
-        }
+        Self { dcx, diag: Some(Box::new(diag)), _marker: PhantomData }
     }
 
     /// Emit and consume the diagnostic.
@@ -281,14 +245,10 @@ impl<'a, G: EmissionGuarantee> DiagnosticBuilder<'a, G> {
         self.emit()
     }
 
-    /// Cancel the diagnostic (a structured diagnostic must either be emitted or
+    /// Cancel and consume the diagnostic. (A 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;
+        self.diag = None;
         drop(self);
     }
 
@@ -304,44 +264,21 @@ impl<'a, G: EmissionGuarantee> DiagnosticBuilder<'a, G> {
     }
 
     /// Converts the builder to a `Diagnostic` for later emission,
-    /// unless dcx has disabled such buffering, or `.emit()` was called.
+    /// unless dcx has disabled such buffering.
     pub fn into_diagnostic(mut self) -> Option<(Diagnostic, &'a DiagCtxt)> {
-        let dcx = match self.state {
-            // No `.emit()` calls, the `&DiagCtxt` is still available.
-            DiagnosticBuilderState::Emittable(dcx) => dcx,
-            // `.emit()` was previously called, nothing we can do.
-            DiagnosticBuilderState::AlreadyEmittedOrDuringCancellation => {
-                return None;
-            }
-        };
-
-        if dcx.inner.lock().flags.dont_buffer_diagnostics
-            || dcx.inner.lock().flags.treat_err_as_bug.is_some()
-        {
+        let flags = self.dcx.inner.lock().flags;
+        if flags.dont_buffer_diagnostics || flags.treat_err_as_bug.is_some() {
             self.emit();
             return None;
         }
 
-        // Take the `Diagnostic` by replacing it with a dummy.
-        let dummy = Diagnostic::new(Level::Allow, DiagnosticMessage::from(""));
-        let diagnostic = std::mem::replace(&mut *self.diagnostic, dummy);
-
-        // Disable the ICE on `Drop`.
-        self.cancel();
+        let diag = self.take_diag();
 
         // Logging here is useful to help track down where in logs an error was
         // actually emitted.
-        debug!("buffer: diagnostic={:?}", diagnostic);
+        debug!("buffer: diag={:?}", diag);
 
-        Some((diagnostic, dcx))
-    }
-
-    /// Retrieves the [`DiagCtxt`] if available
-    pub fn dcx(&self) -> Option<&DiagCtxt> {
-        match self.state {
-            DiagnosticBuilderState::Emittable(dcx) => Some(dcx),
-            DiagnosticBuilderState::AlreadyEmittedOrDuringCancellation => None,
-        }
+        Some((diag, self.dcx))
     }
 
     /// Buffers the diagnostic for later emission,
@@ -494,30 +431,24 @@ impl<'a, G: EmissionGuarantee> DiagnosticBuilder<'a, G> {
 
 impl<G: EmissionGuarantee> Debug for DiagnosticBuilder<'_, G> {
     fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
-        self.diagnostic.fmt(f)
+        self.diag.fmt(f)
     }
 }
 
-/// Destructor bomb - a `DiagnosticBuilder` must be either emitted or cancelled
-/// or we emit a bug.
+/// Destructor bomb: every `DiagnosticBuilder` must be consumed (emitted,
+/// cancelled, etc.) or we emit a bug.
 impl<G: EmissionGuarantee> Drop for DiagnosticBuilder<'_, G> {
     fn drop(&mut self) {
-        match self.state {
-            // No `.emit()` or `.cancel()` calls.
-            DiagnosticBuilderState::Emittable(dcx) => {
-                if !panicking() {
-                    dcx.emit_diagnostic(Diagnostic::new(
-                        Level::Bug,
-                        DiagnosticMessage::from(
-                            "the following error was constructed but not emitted",
-                        ),
-                    ));
-                    dcx.emit_diagnostic_without_consuming(&mut self.diagnostic);
-                    panic!("error was constructed but not emitted");
-                }
+        match self.diag.take() {
+            Some(diag) if !panicking() => {
+                self.dcx.emit_diagnostic(Diagnostic::new(
+                    Level::Bug,
+                    DiagnosticMessage::from("the following error was constructed but not emitted"),
+                ));
+                self.dcx.emit_diagnostic(*diag);
+                panic!("error was constructed but not emitted");
             }
-            // `.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
@@ -7,6 +7,7 @@
 #![feature(rustdoc_internals)]
 #![feature(array_windows)]
 #![feature(associated_type_defaults)]
+#![feature(box_into_inner)]
 #![feature(extract_if)]
 #![feature(if_let_guard)]
 #![feature(let_chains)]
diff --git a/compiler/rustc_mir_transform/src/errors.rs b/compiler/rustc_mir_transform/src/errors.rs
@@ -181,8 +181,7 @@ pub(crate) struct UnsafeOpInUnsafeFn {
 impl<'a> DecorateLint<'a, ()> for UnsafeOpInUnsafeFn {
     #[track_caller]
     fn decorate_lint<'b>(self, diag: &'b mut DiagnosticBuilder<'a, ()>) {
-        let dcx = diag.dcx().expect("lint should not yet be emitted");
-        let desc = dcx.eagerly_translate_to_string(self.details.label(), [].into_iter());
+        let desc = diag.dcx.eagerly_translate_to_string(self.details.label(), [].into_iter());
         diag.arg("details", desc);
         diag.span_label(self.details.span, self.details.label());
         self.details.add_subdiagnostics(diag);
