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

Author: eddyb

compiler/rustc_borrowck/src/lib.rs

@@ -379,7 +379,7 @@ fn do_mir_borrowck<'a, 'tcx>(
     // Convert any reservation warnings into lints.
     let reservation_warnings = mem::take(&mut mbcx.reservation_warnings);
     for (_, (place, span, location, bk, borrow)) in reservation_warnings {
-        let mut initial_diag = mbcx.report_conflicting_borrow(location, (place, span), bk, &borrow);
+        let initial_diag = mbcx.report_conflicting_borrow(location, (place, span), bk, &borrow);
 
         let scope = mbcx.body.source_info(location).scope;
         let lint_root = match &mbcx.body.source_scopes[scope].local_data {
@@ -2329,7 +2329,7 @@ mod error {
             move_out_indices: Vec<MoveOutIndex>,
             place_and_err: (PlaceRef<'tcx>, DiagnosticBuilder<'tcx>),
         ) -> bool {
-            if let Some((_, mut diag)) =
+            if let Some((_, diag)) =
                 self.errors.buffered_move_errors.insert(move_out_indices, place_and_err)
             {
                 // Cancel the old diagnostic so we don't ICE

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"),
     }
 }

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
     }

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 => {}
         }
     }
 }

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"),
         }
     }

compiler/rustc_expand/src/mbe/macro_rules.rs

@@ -95,7 +95,7 @@ fn emit_frag_parse_err(
     match kind {
         // Try a statement if an expression is wanted but failed and suggest adding `;` to call.
         AstFragmentKind::Expr => match parse_ast_fragment(orig_parser, AstFragmentKind::Stmts) {
-            Err(mut err) => err.cancel(),
+            Err(err) => err.cancel(),
             Ok(_) => {
                 e.note(
                     "the macro call doesn't expand to an expression, but it can expand to a statement",

compiler/rustc_infer/src/infer/error_reporting/mod.rs

@@ -1598,7 +1598,9 @@ impl<'a, 'tcx> InferCtxt<'a, 'tcx> {
                     Some((expected, found)) => Some((expected, found)),
                     None => {
                         // Derived error. Cancel the emitter.
-                        diag.cancel();
+                        // NOTE(eddyb) this was `.cancel()`, but `diag`
+                        // is borrowed, so we can't fully defuse it.
+                        diag.downgrade_to_delayed_bug();
                         return;
                     }
                 };

compiler/rustc_interface/src/interface.rs

@@ -102,7 +102,7 @@ pub fn parse_cfgspecs(cfgspecs: Vec<String>) -> FxHashSet<(String, Option<String
                 }
 
                 match maybe_new_parser_from_source_str(&sess, filename, s.to_string()) {
-                    Ok(mut parser) => match &mut parser.parse_meta_item() {
+                    Ok(mut parser) => match parser.parse_meta_item() {
                         Ok(meta_item) if parser.token == token::Eof => {
                             if meta_item.path.segments.len() != 1 {
                                 error!("argument key must be an identifier");
@@ -121,7 +121,7 @@ pub fn parse_cfgspecs(cfgspecs: Vec<String>) -> FxHashSet<(String, Option<String
                         Ok(..) => {}
                         Err(err) => err.cancel(),
                     },
-                    Err(errs) => errs.into_iter().for_each(|mut err| err.cancel()),
+                    Err(errs) => drop(errs),
                 }
 
                 // If the user tried to use a key="value" flag, but is missing the quotes, provide
@@ -165,7 +165,7 @@ pub fn parse_check_cfg(specs: Vec<String>) -> CheckCfg {
             }
 
             match maybe_new_parser_from_source_str(&sess, filename, s.to_string()) {
-                Ok(mut parser) => match &mut parser.parse_meta_item() {
+                Ok(mut parser) => match parser.parse_meta_item() {
                     Ok(meta_item) if parser.token == token::Eof => {
                         if let Some(args) = meta_item.meta_item_list() {
                             if meta_item.has_name(sym::names) {
@@ -210,7 +210,7 @@ pub fn parse_check_cfg(specs: Vec<String>) -> CheckCfg {
                     Ok(..) => {}
                     Err(err) => err.cancel(),
                 },
-                Err(errs) => errs.into_iter().for_each(|mut err| err.cancel()),
+                Err(errs) => drop(errs),
             }
 
             error!(

compiler/rustc_parse/src/parser/attr.rs

@@ -165,7 +165,7 @@ impl<'a> Parser<'a> {
         loop {
             // skip any other attributes, we want the item
             if snapshot.token.kind == token::Pound {
-                if let Err(mut err) = snapshot.parse_attribute(InnerAttrPolicy::Permitted) {
+                if let Err(err) = snapshot.parse_attribute(InnerAttrPolicy::Permitted) {
                     err.cancel();
                     return Some(replacement_span);
                 }
@@ -206,7 +206,7 @@ impl<'a> Parser<'a> {
                 );
                 return None;
             }
-            Err(mut item_err) => {
+            Err(item_err) => {
                 item_err.cancel();
             }
             Ok(None) => {}
@@ -412,12 +412,12 @@ impl<'a> Parser<'a> {
     fn parse_meta_item_inner(&mut self) -> PResult<'a, ast::NestedMetaItem> {
         match self.parse_unsuffixed_lit() {
             Ok(lit) => return Ok(ast::NestedMetaItem::Literal(lit)),
-            Err(ref mut err) => err.cancel(),
+            Err(err) => err.cancel(),
         }
 
         match self.parse_meta_item() {
             Ok(mi) => return Ok(ast::NestedMetaItem::MetaItem(mi)),
-            Err(ref mut err) => err.cancel(),
+            Err(err) => err.cancel(),
         }
 
         let found = pprust::token_to_string(&self.token);