Skip to content

Commit

Permalink
Remove DiagnosticBuilderState.
Browse files Browse the repository at this point in the history 
Currently it's used to record whether a `DiagnosticBuilder` has been
emitted or cancelled. But now that `emit` is consuming, it's impossible
to emit a `DiagnosticBuilder` twice. So we don't need
`DiagnosticBuilderState` any more.

This commit replaces it with `Option<Box<Diagnostic>>`. We need the
`Option` so that functions like `emit` and `cancel` can take the
`Diagnostic` and then `drop` can check that the `Diagnostic` was taken.
Basically, a simplified version of `DiagnosticBuilderState`.

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

As well as make the code shorter and simpler, the commit removes:
- One `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`.
  • Loading branch information
@nnethercote
nnethercote committed Jan 8, 2024
1 parent b37c73d commit b22053f
Show file tree
Hide file tree
Showing 3 changed files with 75 additions and 144 deletions.
215 changes: 73 additions & 142 deletions compiler/rustc_errors/src/diagnostic_builder.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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>,
}
Expand All @@ -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"
Expand All @@ -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();
Expand Down Expand Up @@ -206,7 +174,7 @@ macro_rules! forward0 {
$(#[$attrs])*
#[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
}
};
Expand All @@ -227,12 +195,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_mv), "()`].")]
pub fn $n_mv(mut self, $($name: $ty),*) -> Self {
self.diagnostic.$n($($name),*);
self.diag.as_mut().unwrap().$n($($name),*);
self
}
};
Expand All @@ -242,13 +210,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()
}
}

Expand All @@ -262,13 +230,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.
Expand All @@ -289,14 +253,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);
}

Expand All @@ -312,44 +272,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,
Expand Down Expand Up @@ -488,30 +425,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: a `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 => {}
_ => {}
}
}
}
Expand Down
1 change: 1 addition & 0 deletions compiler/rustc_errors/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)]
Expand Down
3 changes: 1 addition & 2 deletions compiler/rustc_mir_transform/src/errors.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down

0 comments on commit b22053f

Please sign in to comment.