Rollup merge of #120959 - nnethercote:rm-good_path, r=oli-obk

oli-obk · web-flow · commit 56f78e04a2f0 · 2024-02-13T10:45:57.000+01:00
Remove good path delayed bugs

Because they're not that useful, and kind of annoying. Details in the individual commits.

r? ``@compiler-errors``
diff --git a/compiler/rustc_const_eval/src/interpret/eval_context.rs b/compiler/rustc_const_eval/src/interpret/eval_context.rs
@@ -288,7 +288,7 @@ impl<'tcx> fmt::Display for FrameInfo<'tcx> {
             if tcx.def_key(self.instance.def_id()).disambiguated_data.data == DefPathData::Closure {
                 write!(f, "inside closure")
             } else {
-                // Note: this triggers a `good_path_delayed_bug` state, which means that if we ever
+                // Note: this triggers a `must_produce_diag` state, which means that if we ever
                 // get here we must emit a diagnostic. We should never display a `FrameInfo` unless
                 // we actually want to emit a warning or error to the user.
                 write!(f, "inside `{}`", self.instance)
@@ -304,7 +304,7 @@ impl<'tcx> FrameInfo<'tcx> {
             errors::FrameNote { where_: "closure", span, instance: String::new(), times: 0 }
         } else {
             let instance = format!("{}", self.instance);
-            // Note: this triggers a `good_path_delayed_bug` state, which means that if we ever get
+            // Note: this triggers a `must_produce_diag` state, which means that if we ever get
             // here we must emit a diagnostic. We should never display a `FrameInfo` unless we
             // actually want to emit a warning or error to the user.
             errors::FrameNote { where_: "instance", span, instance, times: 0 }
diff --git a/compiler/rustc_error_messages/src/lib.rs b/compiler/rustc_error_messages/src/lib.rs
@@ -376,7 +376,7 @@ impl From<Cow<'static, str>> for DiagnosticMessage {
     }
 }
 
-/// A workaround for good_path_delayed_bug ICEs when formatting types in disabled lints.
+/// A workaround for must_produce_diag ICEs when formatting types in disabled lints.
 ///
 /// Delays formatting until `.into(): DiagnosticMessage` is used.
 pub struct DelayDm<F>(pub F);
diff --git a/compiler/rustc_errors/src/annotate_snippet_emitter_writer.rs b/compiler/rustc_errors/src/annotate_snippet_emitter_writer.rs
@@ -85,11 +85,7 @@ fn source_string(file: Lrc<SourceFile>, line: &Line) -> String {
 /// Maps `Diagnostic::Level` to `snippet::AnnotationType`
 fn annotation_type_for_level(level: Level) -> AnnotationType {
     match level {
-        Level::Bug
-        | Level::Fatal
-        | Level::Error
-        | Level::DelayedBug
-        | Level::GoodPathDelayedBug => AnnotationType::Error,
+        Level::Bug | Level::Fatal | Level::Error | Level::DelayedBug => AnnotationType::Error,
         Level::ForceWarning(_) | Level::Warning => AnnotationType::Warning,
         Level::Note | Level::OnceNote => AnnotationType::Note,
         Level::Help | Level::OnceHelp => AnnotationType::Help,
diff --git a/compiler/rustc_errors/src/diagnostic.rs b/compiler/rustc_errors/src/diagnostic.rs
@@ -237,8 +237,7 @@ impl Diagnostic {
         match self.level {
             Level::Bug | Level::Fatal | Level::Error | Level::DelayedBug => true,
 
-            Level::GoodPathDelayedBug
-            | Level::ForceWarning(_)
+            Level::ForceWarning(_)
             | Level::Warning
             | Level::Note
             | Level::OnceNote
diff --git a/compiler/rustc_errors/src/lib.rs b/compiler/rustc_errors/src/lib.rs
@@ -436,7 +436,6 @@ struct DiagCtxtInner {
     lint_err_guars: Vec<ErrorGuaranteed>,
     /// The delayed bugs and their error guarantees.
     delayed_bugs: Vec<(DelayedDiagnostic, ErrorGuaranteed)>,
-    good_path_delayed_bugs: Vec<DelayedDiagnostic>,
 
     /// The number of stashed errors. Unlike the other counts, this can go up
     /// and down, so it doesn't guarantee anything.
@@ -447,13 +446,18 @@ struct DiagCtxtInner {
     /// The warning count shown to the user at the end.
     deduplicated_warn_count: usize,
 
+    emitter: Box<DynEmitter>,
+
+    /// Must we produce a diagnostic to justify the use of the expensive
+    /// `trimmed_def_paths` function?
+    must_produce_diag: bool,
+
     /// Has this diagnostic context printed any diagnostics? (I.e. has
     /// `self.emitter.emit_diagnostic()` been called?
     has_printed: bool,
 
-    emitter: Box<DynEmitter>,
     /// This flag indicates that an expected diagnostic was emitted and suppressed.
-    /// This is used for the `good_path_delayed_bugs` check.
+    /// This is used for the `must_produce_diag` check.
     suppressed_expected_diag: bool,
 
     /// This set contains the code of all emitted diagnostics to avoid
@@ -534,11 +538,6 @@ fn default_track_diagnostic(diag: Diagnostic, f: &mut dyn FnMut(Diagnostic)) {
 pub static TRACK_DIAGNOSTIC: AtomicRef<fn(Diagnostic, &mut dyn FnMut(Diagnostic))> =
     AtomicRef::new(&(default_track_diagnostic as _));
 
-enum DelayedBugKind {
-    Normal,
-    GoodPath,
-}
-
 #[derive(Copy, Clone, Default)]
 pub struct DiagCtxtFlags {
     /// If false, warning-level lints are suppressed.
@@ -564,11 +563,16 @@ impl Drop for DiagCtxtInner {
         self.emit_stashed_diagnostics();
 
         if self.err_guars.is_empty() {
-            self.flush_delayed(DelayedBugKind::Normal)
+            self.flush_delayed()
         }
 
         if !self.has_printed && !self.suppressed_expected_diag && !std::thread::panicking() {
-            self.flush_delayed(DelayedBugKind::GoodPath);
+            if self.must_produce_diag {
+                panic!(
+                    "must_produce_diag: trimmed_def_paths called but no diagnostics emitted; \
+                       use `DelayDm` for lints or `with_no_trimmed_paths` for debugging"
+                );
+            }
         }
 
         if self.check_unstable_expect_diagnostics {
@@ -610,12 +614,12 @@ impl DiagCtxt {
                 err_guars: Vec::new(),
                 lint_err_guars: Vec::new(),
                 delayed_bugs: Vec::new(),
-                good_path_delayed_bugs: Vec::new(),
                 stashed_err_count: 0,
                 deduplicated_err_count: 0,
                 deduplicated_warn_count: 0,
-                has_printed: false,
                 emitter,
+                must_produce_diag: false,
+                has_printed: false,
                 suppressed_expected_diag: false,
                 taught_diagnostics: Default::default(),
                 emitted_diagnostic_codes: Default::default(),
@@ -667,13 +671,14 @@ impl DiagCtxt {
         inner.stashed_err_count = 0;
         inner.deduplicated_err_count = 0;
         inner.deduplicated_warn_count = 0;
+        inner.must_produce_diag = false;
         inner.has_printed = false;
+        inner.suppressed_expected_diag = false;
 
         // actually free the underlying memory (which `clear` would not do)
         inner.err_guars = Default::default();
         inner.lint_err_guars = Default::default();
         inner.delayed_bugs = Default::default();
-        inner.good_path_delayed_bugs = Default::default();
         inner.taught_diagnostics = Default::default();
         inner.emitted_diagnostic_codes = Default::default();
         inner.emitted_diagnostics = Default::default();
@@ -935,7 +940,13 @@ impl DiagCtxt {
     }
 
     pub fn flush_delayed(&self) {
-        self.inner.borrow_mut().flush_delayed(DelayedBugKind::Normal);
+        self.inner.borrow_mut().flush_delayed();
+    }
+
+    /// Used when trimmed_def_paths is called and we must produce a diagnostic
+    /// to justify its cost.
+    pub fn set_must_produce_diag(&self) {
+        self.inner.borrow_mut().must_produce_diag = true;
     }
 }
 
@@ -1109,13 +1120,6 @@ impl DiagCtxt {
         DiagnosticBuilder::<ErrorGuaranteed>::new(self, DelayedBug, msg).with_span(sp).emit()
     }
 
-    /// Ensures that a diagnostic is printed. See `Level::GoodPathDelayedBug`.
-    // No `#[rustc_lint_diagnostics]` because bug messages aren't user-facing.
-    #[track_caller]
-    pub fn good_path_delayed_bug(&self, msg: impl Into<DiagnosticMessage>) {
-        DiagnosticBuilder::<()>::new(self, GoodPathDelayedBug, msg).emit()
-    }
-
     #[rustc_lint_diagnostics]
     #[track_caller]
     pub fn struct_warn(&self, msg: impl Into<DiagnosticMessage>) -> DiagnosticBuilder<'_, ()> {
@@ -1267,19 +1271,17 @@ impl DiagCtxtInner {
         if diagnostic.has_future_breakage() {
             // Future breakages aren't emitted if they're Level::Allow,
             // but they still need to be constructed and stashed below,
-            // so they'll trigger the good-path bug check.
+            // so they'll trigger the must_produce_diag check.
             self.suppressed_expected_diag = true;
             self.future_breakage_diagnostics.push(diagnostic.clone());
         }
 
-        if matches!(diagnostic.level, DelayedBug | GoodPathDelayedBug)
-            && self.flags.eagerly_emit_delayed_bugs
-        {
+        if diagnostic.level == DelayedBug && self.flags.eagerly_emit_delayed_bugs {
             diagnostic.level = Error;
         }
 
         match diagnostic.level {
-            // This must come after the possible promotion of `DelayedBug`/`GoodPathDelayedBug` to
+            // This must come after the possible promotion of `DelayedBug` to
             // `Error` above.
             Fatal | Error if self.treat_next_err_as_bug() => {
                 diagnostic.level = Bug;
@@ -1298,12 +1300,6 @@ impl DiagCtxtInner {
                     .push((DelayedDiagnostic::with_backtrace(diagnostic, backtrace), guar));
                 return Some(guar);
             }
-            GoodPathDelayedBug => {
-                let backtrace = std::backtrace::Backtrace::capture();
-                self.good_path_delayed_bugs
-                    .push(DelayedDiagnostic::with_backtrace(diagnostic, backtrace));
-                return None;
-            }
             Warning if !self.flags.can_emit_warnings => {
                 if diagnostic.has_future_breakage() {
                     (*TRACK_DIAGNOSTIC)(diagnostic, &mut |_| {});
@@ -1415,23 +1411,14 @@ impl DiagCtxtInner {
         self.emit_diagnostic(Diagnostic::new(FailureNote, msg));
     }
 
-    fn flush_delayed(&mut self, kind: DelayedBugKind) {
-        let (bugs, note1) = match kind {
-            DelayedBugKind::Normal => (
-                std::mem::take(&mut self.delayed_bugs).into_iter().map(|(b, _)| b).collect(),
-                "no errors encountered even though delayed bugs were created",
-            ),
-            DelayedBugKind::GoodPath => (
-                std::mem::take(&mut self.good_path_delayed_bugs),
-                "no warnings or errors encountered even though good path delayed bugs were created",
-            ),
-        };
-        let note2 = "those delayed bugs will now be shown as internal compiler errors";
-
-        if bugs.is_empty() {
+    fn flush_delayed(&mut self) {
+        if self.delayed_bugs.is_empty() {
             return;
         }
 
+        let bugs: Vec<_> =
+            std::mem::take(&mut self.delayed_bugs).into_iter().map(|(b, _)| b).collect();
+
         // If backtraces are enabled, also print the query stack
         let backtrace = std::env::var_os("RUST_BACKTRACE").map_or(true, |x| &x != "0");
         for (i, bug) in bugs.into_iter().enumerate() {
@@ -1455,6 +1442,8 @@ impl DiagCtxtInner {
                 // frame them better (e.g. separate warnings from them). Also,
                 // make it a note so it doesn't count as an error, because that
                 // could trigger `-Ztreat-err-as-bug`, which we don't want.
+                let note1 = "no errors encountered even though delayed bugs were created";
+                let note2 = "those delayed bugs will now be shown as internal compiler errors";
                 self.emit_diagnostic(Diagnostic::new(Note, note1));
                 self.emit_diagnostic(Diagnostic::new(Note, note2));
             }
@@ -1463,7 +1452,7 @@ impl DiagCtxtInner {
                 if backtrace || self.ice_file.is_none() { bug.decorate() } else { bug.inner };
 
             // "Undelay" the delayed bugs (into plain `Bug`s).
-            if !matches!(bug.level, DelayedBug | GoodPathDelayedBug) {
+            if bug.level != DelayedBug {
                 // NOTE(eddyb) not panicking here because we're already producing
                 // an ICE, and the more information the merrier.
                 bug.subdiagnostic(InvalidFlushedDelayedDiagnosticLevel {
@@ -1535,7 +1524,6 @@ impl DelayedDiagnostic {
 /// Fatal              yes       FatalAbort/FatalError(*)  yes        -     -
 /// Error              yes       ErrorGuaranteed           yes        -     yes
 /// DelayedBug         yes       ErrorGuaranteed           yes        -     -
-/// GoodPathDelayedBug -         ()                        yes        -     -
 /// ForceWarning       -         ()                        yes        -     lint-only
 /// Warning            -         ()                        yes        yes   yes
 /// Note               -         ()                        rare       yes   -
@@ -1568,20 +1556,6 @@ pub enum Level {
     /// that should only be reached when compiling erroneous code.
     DelayedBug,
 
-    /// Like `DelayedBug`, but weaker: lets you register an error without emitting it. If
-    /// compilation ends without any other diagnostics being emitted (and without an expected lint
-    /// being suppressed), this will be emitted as a bug. Otherwise, it will be silently dropped.
-    /// I.e. "expect other diagnostics are emitted (or suppressed)" semantics. Useful on code paths
-    /// that should only be reached when emitting diagnostics, e.g. for expensive one-time
-    /// diagnostic formatting operations.
-    ///
-    /// FIXME(nnethercote) good path delayed bugs are semantically strange: if printed they produce
-    /// an ICE, but they don't satisfy `is_error` and they don't guarantee an error is emitted.
-    /// Plus there's the extra complication with expected (suppressed) lints. They have limited
-    /// use, and are used in very few places, and "good path" isn't a good name. It would be good
-    /// to remove them.
-    GoodPathDelayedBug,
-
     /// A `force-warn` lint warning about the code being compiled. Does not prevent compilation
     /// from finishing.
     ///
@@ -1626,7 +1600,7 @@ impl Level {
     fn color(self) -> ColorSpec {
         let mut spec = ColorSpec::new();
         match self {
-            Bug | Fatal | Error | DelayedBug | GoodPathDelayedBug => {
+            Bug | Fatal | Error | DelayedBug => {
                 spec.set_fg(Some(Color::Red)).set_intense(true);
             }
             ForceWarning(_) | Warning => {
@@ -1646,7 +1620,7 @@ impl Level {
 
     pub fn to_str(self) -> &'static str {
         match self {
-            Bug | DelayedBug | GoodPathDelayedBug => "error: internal compiler error",
+            Bug | DelayedBug => "error: internal compiler error",
             Fatal | Error => "error",
             ForceWarning(_) | Warning => "warning",
             Note | OnceNote => "note",
@@ -1671,8 +1645,8 @@ impl Level {
     // subdiagnostic message?
     fn can_be_top_or_sub(&self) -> (bool, bool) {
         match self {
-            Bug | DelayedBug | Fatal | Error | GoodPathDelayedBug | ForceWarning(_)
-            | FailureNote | Allow | Expect(_) => (true, false),
+            Bug | DelayedBug | Fatal | Error | ForceWarning(_) | FailureNote | Allow
+            | Expect(_) => (true, false),
 
             Warning | Note | Help => (true, true),
 
diff --git a/compiler/rustc_infer/src/infer/error_reporting/mod.rs b/compiler/rustc_infer/src/infer/error_reporting/mod.rs
@@ -132,20 +132,6 @@ pub struct TypeErrCtxt<'a, 'tcx> {
         Box<dyn Fn(Ty<'tcx>) -> Vec<(Ty<'tcx>, Vec<PredicateObligation<'tcx>>)> + 'a>,
 }
 
-impl Drop for TypeErrCtxt<'_, '_> {
-    fn drop(&mut self) {
-        if self.dcx().has_errors().is_some() {
-            // Ok, emitted an error.
-        } else {
-            // Didn't emit an error; maybe it was created but not yet emitted.
-            self.infcx
-                .tcx
-                .sess
-                .good_path_delayed_bug("used a `TypeErrCtxt` without raising an error or lint");
-        }
-    }
-}
-
 impl<'a, 'tcx> TypeErrCtxt<'a, 'tcx> {
     pub fn dcx(&self) -> &'tcx DiagCtxt {
         self.infcx.tcx.dcx()
diff --git a/compiler/rustc_middle/src/ty/print/pretty.rs b/compiler/rustc_middle/src/ty/print/pretty.rs
@@ -3156,13 +3156,12 @@ fn for_each_def(tcx: TyCtxt<'_>, mut collect_fn: impl for<'b> FnMut(&'b Ident, N
 // this is pub to be able to intra-doc-link it
 pub fn trimmed_def_paths(tcx: TyCtxt<'_>, (): ()) -> DefIdMap<Symbol> {
     // Trimming paths is expensive and not optimized, since we expect it to only be used for error
-    // reporting.
+    // reporting. Record the fact that we did it, so we can abort if we later found it was
+    // unnecessary.
     //
-    // For good paths causing this bug, the `rustc_middle::ty::print::with_no_trimmed_paths`
-    // wrapper can be used to suppress this query, in exchange for full paths being formatted.
-    tcx.sess.good_path_delayed_bug(
-        "trimmed_def_paths constructed but no error emitted; use `DelayDm` for lints or `with_no_trimmed_paths` for debugging",
-    );
+    // The `rustc_middle::ty::print::with_no_trimmed_paths` wrapper can be used to suppress this
+    // checking, in exchange for full paths being formatted.
+    tcx.sess.record_trimmed_def_paths();
 
     // Once constructed, unique namespace+symbol pairs will have a `Some(_)` entry, while
     // non-unique pairs will have a `None` entry.
diff --git a/compiler/rustc_session/src/session.rs b/compiler/rustc_session/src/session.rs
@@ -322,10 +322,9 @@ impl Session {
         }
     }
 
-    /// Used for code paths of expensive computations that should only take place when
-    /// warnings or errors are emitted. If no messages are emitted ("good path"), then
-    /// it's likely a bug.
-    pub fn good_path_delayed_bug(&self, msg: impl Into<DiagnosticMessage>) {
+    /// Record the fact that we called `trimmed_def_paths`, and do some
+    /// checking about whether its cost was justified.
+    pub fn record_trimmed_def_paths(&self) {
         if self.opts.unstable_opts.print_type_sizes
             || self.opts.unstable_opts.query_dep_graph
             || self.opts.unstable_opts.dump_mir.is_some()
@@ -336,7 +335,7 @@ impl Session {
             return;
         }
 
-        self.dcx().good_path_delayed_bug(msg)
+        self.dcx().set_must_produce_diag()
     }
 
     #[inline]
@@ -546,8 +545,8 @@ impl Session {
                 if fuel.remaining == 0 && !fuel.out_of_fuel {
                     if self.dcx().can_emit_warnings() {
                         // We only call `msg` in case we can actually emit warnings.
-                        // Otherwise, this could cause a `good_path_delayed_bug` to
-                        // trigger (issue #79546).
+                        // Otherwise, this could cause a `must_produce_diag` ICE
+                        // (issue #79546).
                         self.dcx().emit_warn(errors::OptimisationFuelExhausted { msg: msg() });
                     }
                     fuel.out_of_fuel = true;
diff --git a/tests/ui/treat-err-as-bug/eagerly-emit.rs b/tests/ui/treat-err-as-bug/eagerly-emit.rs
diff --git a/tests/ui/treat-err-as-bug/eagerly-emit.stderr b/tests/ui/treat-err-as-bug/eagerly-emit.stderr

Original file line number	Diff line number	Diff line change
`@@ -376,7 +376,7 @@ impl From<Cow<'static, str>> for DiagnosticMessage {`
`376`	`376`	`}`
`377`	`377`	`}`
`378`	`378`
`379`		`-/// A workaround for good_path_delayed_bug ICEs when formatting types in disabled lints.`
	`379`	`+/// A workaround for must_produce_diag ICEs when formatting types in disabled lints.`
`380`	`380`	`///`
`381`	`381`	/// Delays formatting until `.into(): DiagnosticMessage` is used.
`382`	`382`	`pub struct DelayDm<F>(pub F);`
Original file line number	Diff line number	Diff line change
`@@ -322,10 +322,9 @@ impl Session {`
`322`	`322`	`}`
`323`	`323`	`}`
`324`	`324`
`325`		`- /// Used for code paths of expensive computations that should only take place when`
`326`		`- /// warnings or errors are emitted. If no messages are emitted ("good path"), then`
`327`		`- /// it's likely a bug.`
`328`		`- pub fn good_path_delayed_bug(&self, msg: impl Into<DiagnosticMessage>) {`
	`325`	+ /// Record the fact that we called `trimmed_def_paths`, and do some
	`326`	`+ /// checking about whether its cost was justified.`
	`327`	`+ pub fn record_trimmed_def_paths(&self) {`
`329`	`328`	`if self.opts.unstable_opts.print_type_sizes`
`330`	`329`	`\|\| self.opts.unstable_opts.query_dep_graph`
`331`	`330`	`\|\| self.opts.unstable_opts.dump_mir.is_some()`
`@@ -336,7 +335,7 @@ impl Session {`
`336`	`335`	`return;`
`337`	`336`	`}`
`338`	`337`
`339`		`- self.dcx().good_path_delayed_bug(msg)`
	`338`	`+ self.dcx().set_must_produce_diag()`
`340`	`339`	`}`
`341`	`340`
`342`	`341`	`#[inline]`
`@@ -546,8 +545,8 @@ impl Session {`
`546`	`545`	`if fuel.remaining == 0 && !fuel.out_of_fuel {`
`547`	`546`	`if self.dcx().can_emit_warnings() {`
`548`	`547`	// We only call `msg` in case we can actually emit warnings.
`549`		- // Otherwise, this could cause a `good_path_delayed_bug` to
`550`		`- // trigger (issue #79546).`
	`548`	+ // Otherwise, this could cause a `must_produce_diag` ICE
	`549`	`+ // (issue #79546).`
`551`	`550`	`self.dcx().emit_warn(errors::OptimisationFuelExhausted { msg: msg() });`
`552`	`551`	`}`
`553`	`552`	`fuel.out_of_fuel = true;`