Skip to content

Commit

Permalink
Rollup merge of #120959 - nnethercote:rm-good_path, r=oli-obk
Browse files Browse the repository at this point in the history
Remove good path delayed bugs

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

r? ```@compiler-errors```
  • Loading branch information
matthiaskrgr authored Feb 13, 2024
2 parents 65ab663 + 9f2aa09 commit 93e9579
Show file tree
Hide file tree
Showing 10 changed files with 56 additions and 133 deletions.
4 changes: 2 additions & 2 deletions compiler/rustc_const_eval/src/interpret/eval_context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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 }
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_error_messages/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
6 changes: 1 addition & 5 deletions compiler/rustc_errors/src/annotate_snippet_emitter_writer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
3 changes: 1 addition & 2 deletions compiler/rustc_errors/src/diagnostic.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
106 changes: 40 additions & 66 deletions compiler/rustc_errors/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -435,7 +435,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.
Expand All @@ -446,13 +445,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
Expand Down Expand Up @@ -533,11 +537,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.
Expand All @@ -563,11 +562,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 {
Expand Down Expand Up @@ -609,12 +613,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(),
Expand Down Expand Up @@ -666,13 +670,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();
Expand Down Expand Up @@ -934,7 +939,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;
}
}

Expand Down Expand Up @@ -1108,13 +1119,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<'_, ()> {
Expand Down Expand Up @@ -1266,19 +1270,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;
Expand All @@ -1297,12 +1299,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 |_| {});
Expand Down Expand Up @@ -1414,23 +1410,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() {
Expand All @@ -1454,6 +1441,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));
}
Expand All @@ -1462,7 +1451,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 {
Expand Down Expand Up @@ -1534,7 +1523,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 -
Expand Down Expand Up @@ -1567,20 +1555,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.
///
Expand Down Expand Up @@ -1625,7 +1599,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 => {
Expand All @@ -1645,7 +1619,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",
Expand All @@ -1670,8 +1644,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),

Expand Down
14 changes: 0 additions & 14 deletions compiler/rustc_infer/src/infer/error_reporting/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down
11 changes: 5 additions & 6 deletions compiler/rustc_middle/src/ty/print/pretty.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
13 changes: 6 additions & 7 deletions compiler/rustc_session/src/session.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand All @@ -336,7 +335,7 @@ impl Session {
return;
}

self.dcx().good_path_delayed_bug(msg)
self.dcx().set_must_produce_diag()
}

#[inline]
Expand Down Expand Up @@ -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;
Expand Down
10 changes: 0 additions & 10 deletions tests/ui/treat-err-as-bug/eagerly-emit.rs

This file was deleted.

Loading

0 comments on commit 93e9579

Please sign in to comment.