From 2d91c6d1bf900276de71a7bde89ffe9cffed59fa Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Mon, 8 Jan 2024 07:47:02 +1100 Subject: [PATCH] Remove `DiagnosticBuilderState`. 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>`, 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! --- .../rustc_errors/src/diagnostic_builder.rs | 213 ++++++------------ compiler/rustc_errors/src/lib.rs | 1 + compiler/rustc_mir_transform/src/errors.rs | 3 +- 3 files changed, 74 insertions(+), 143 deletions(-) diff --git a/compiler/rustc_errors/src/diagnostic_builder.rs b/compiler/rustc_errors/src/diagnostic_builder.rs index b81e37501e7e0..e72791d89a292 100644 --- 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, + /// 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>, _marker: PhantomData, } @@ -56,32 +65,9 @@ pub struct DiagnosticBuilder<'a, G: EmissionGuarantee = ErrorGuaranteed> { // twice, which would be bad. impl !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::() ); /// 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`", + 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`", - 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`'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` 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 Deref for DiagnosticBuilder<'_, G> { type Target = Diagnostic; fn deref(&self) -> &Diagnostic { - &self.diagnostic + self.diag.as_ref().unwrap() } } impl 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 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 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 index 4710722c77157..8411e188189d0 100644 --- 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 index bde442049b1d3..446f13feff089 100644 --- 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);