Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Give me a way to emit all the delayed bugs as errors (add -Zeagerly-emit-delayed-bugs) #119872

Merged
merged 3 commits into from
Jan 12, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +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::DelayedBug | Level::Fatal | Level::Error => AnnotationType::Error,
Level::Bug | Level::DelayedBug(_) | Level::Fatal | Level::Error => AnnotationType::Error,
Level::ForceWarning(_) | Level::Warning => AnnotationType::Warning,
Level::Note | Level::OnceNote => AnnotationType::Note,
Level::Help | Level::OnceHelp => AnnotationType::Help,
Expand Down
15 changes: 9 additions & 6 deletions compiler/rustc_errors/src/diagnostic.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
use crate::snippet::Style;
use crate::{
CodeSuggestion, DiagnosticBuilder, DiagnosticMessage, EmissionGuarantee, Level, MultiSpan,
SubdiagnosticMessage, Substitution, SubstitutionPart, SuggestionStyle,
CodeSuggestion, DelayedBugKind, DiagnosticBuilder, DiagnosticMessage, EmissionGuarantee, Level,
MultiSpan, SubdiagnosticMessage, Substitution, SubstitutionPart, SuggestionStyle,
};
use rustc_data_structures::fx::{FxHashMap, FxIndexMap};
use rustc_error_messages::fluent_value_from_str_list_sep_by_and;
Expand Down Expand Up @@ -243,12 +243,15 @@ impl Diagnostic {

pub fn is_error(&self) -> bool {
match self.level {
Level::Bug | Level::DelayedBug | Level::Fatal | Level::Error | Level::FailureNote => {
true
}
Level::Bug
| Level::DelayedBug(DelayedBugKind::Normal)
| Level::Fatal
| Level::Error
| Level::FailureNote => true,

Level::ForceWarning(_)
| Level::Warning
| Level::DelayedBug(DelayedBugKind::GoodPath)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems wrong. Good path delayed bugs, when emitted, are errors just as much as normal delayed bugs.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No. Good path bugs are not errors, since they are defused by emitting warnings or error diagnostics.

Returning an ErrorGuaranteed here would be promising that compilation fails.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you're looking at the wrong end. If you fail to emit a warning or an error, the the good path delayed bug is emitted as a bug. It has "bug" in the name!

| Level::Note
| Level::OnceNote
| Level::Help
Expand Down Expand Up @@ -318,7 +321,7 @@ impl Diagnostic {
"downgrade_to_delayed_bug: cannot downgrade {:?} to DelayedBug: not an error",
self.level
);
self.level = Level::DelayedBug;
self.level = Level::DelayedBug(DelayedBugKind::Normal);
}

/// Appends a labeled span to the diagnostic.
Expand Down
95 changes: 59 additions & 36 deletions compiler/rustc_errors/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -519,6 +519,12 @@ 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 _));

#[derive(Copy, PartialEq, Eq, Clone, Hash, Debug, Encodable, Decodable)]
pub enum DelayedBugKind {
Normal,
GoodPath,
}

#[derive(Copy, Clone, Default)]
pub struct DiagCtxtFlags {
/// If false, warning-level lints are suppressed.
Expand All @@ -527,6 +533,9 @@ pub struct DiagCtxtFlags {
/// If Some, the Nth error-level diagnostic is upgraded to bug-level.
/// (rustc: see `-Z treat-err-as-bug`)
pub treat_err_as_bug: Option<NonZeroUsize>,
/// Eagerly emit delayed bugs as errors, so that the compiler debugger may
/// see all of the errors being emitted at once.
pub eagerly_emit_delayed_bugs: bool,
/// Show macro backtraces.
/// (rustc: see `-Z macro-backtrace`)
pub macro_backtrace: bool,
Expand All @@ -541,8 +550,7 @@ impl Drop for DiagCtxtInner {
self.emit_stashed_diagnostics();

if !self.has_errors() {
let bugs = std::mem::replace(&mut self.span_delayed_bugs, Vec::new());
self.flush_delayed(bugs, "no errors encountered even though `span_delayed_bug` issued");
self.flush_delayed(DelayedBugKind::Normal)
}

// FIXME(eddyb) this explains what `good_path_delayed_bugs` are!
Expand All @@ -551,11 +559,7 @@ impl Drop for DiagCtxtInner {
// lints can be `#[allow]`'d, potentially leading to this triggering.
// Also, "good path" should be replaced with a better naming.
if !self.has_printed && !self.suppressed_expected_diag && !std::thread::panicking() {
let bugs = std::mem::replace(&mut self.good_path_delayed_bugs, Vec::new());
self.flush_delayed(
bugs,
"no warnings or errors encountered even though `good_path_delayed_bugs` issued",
);
self.flush_delayed(DelayedBugKind::GoodPath);
}

if self.check_unstable_expect_diagnostics {
Expand Down Expand Up @@ -865,7 +869,8 @@ impl DiagCtxt {
if treat_next_err_as_bug {
self.bug(msg);
}
DiagnosticBuilder::<ErrorGuaranteed>::new(self, DelayedBug, msg).emit()
DiagnosticBuilder::<ErrorGuaranteed>::new(self, DelayedBug(DelayedBugKind::Normal), msg)
.emit()
}

/// Like `delayed_bug`, but takes an additional span.
Expand All @@ -882,16 +887,15 @@ impl DiagCtxt {
if treat_next_err_as_bug {
self.span_bug(sp, msg);
}
DiagnosticBuilder::<ErrorGuaranteed>::new(self, DelayedBug, msg).with_span(sp).emit()
DiagnosticBuilder::<ErrorGuaranteed>::new(self, DelayedBug(DelayedBugKind::Normal), msg)
.with_span(sp)
.emit()
}

// FIXME(eddyb) note the comment inside `impl Drop for DiagCtxtInner`, that's
// where the explanation of what "good path" is (also, it should be renamed).
pub fn good_path_delayed_bug(&self, msg: impl Into<DiagnosticMessage>) {
let mut inner = self.inner.borrow_mut();
let diagnostic = Diagnostic::new(DelayedBug, msg);
let backtrace = std::backtrace::Backtrace::capture();
inner.good_path_delayed_bugs.push(DelayedDiagnostic::with_backtrace(diagnostic, backtrace));
DiagnosticBuilder::<()>::new(self, DelayedBug(DelayedBugKind::GoodPath), msg).emit()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be ErrorGuaranteed instead of (), and also this method can return ErrorGuaranteed just like span_delayed_bug.

Copy link
Member Author

@compiler-errors compiler-errors Jan 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is not correct to return ErrorGuaranteed from good_path_bug.

If I delayed a good path bug, emitted a warning-level diagnostic to suppress the good path bugs from causing an ICE, then constructed a Ty::new_error from that ErrorGuranteed, I could end up with a TyKind::Error in codegen. That's not sound.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, ok, I see what you're saying. I hate good path delayed bugs! I'd love to get rid of them, I think the one in TypeErrCtxt::drop is bogus and should be something else (though I haven't worked out what, yet) and the other one in trimmed_def_paths I wonder if it's even worth having.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, but for now, I'd rather keep what I have so we don't accidentally expose new ways for people to make ErrorGuaranteeds out of nothing. Creating an ErrorGuaranteed when you shouldn't have one could lead to really bad bugs that can go unnoticed and it's not worth a minor cleanup imo.

It's possible we could construct the diagnostic as a DiagnosticBuilder<ErrorGuranteed>, but the emit call still should return None for clarity, and the fn good_path_bug user-facing API still needs to return () for correctness, so I don't see the harm in just making it act function as a DiagnosticBuilder<()> always.

}

#[track_caller]
Expand Down Expand Up @@ -1218,9 +1222,7 @@ impl DiagCtxt {
}

pub fn flush_delayed(&self) {
let mut inner = self.inner.borrow_mut();
let bugs = std::mem::replace(&mut inner.span_delayed_bugs, Vec::new());
inner.flush_delayed(bugs, "no errors encountered even though `span_delayed_bug` issued");
self.inner.borrow_mut().flush_delayed(DelayedBugKind::Normal);
}
}

Expand Down Expand Up @@ -1270,17 +1272,30 @@ impl DiagCtxtInner {
return None;
}

if diagnostic.level == DelayedBug {
// FIXME(eddyb) this should check for `has_errors` and stop pushing
// once *any* errors were emitted (and truncate `span_delayed_bugs`
// when an error is first emitted, also), but maybe there's a case
// in which that's not sound? otherwise this is really inefficient.
let backtrace = std::backtrace::Backtrace::capture();
self.span_delayed_bugs
.push(DelayedDiagnostic::with_backtrace(diagnostic.clone(), backtrace));
// FIXME(eddyb) this should check for `has_errors` and stop pushing
// once *any* errors were emitted (and truncate `span_delayed_bugs`
// when an error is first emitted, also), but maybe there's a case
// in which that's not sound? otherwise this is really inefficient.
match diagnostic.level {
DelayedBug(_) if self.flags.eagerly_emit_delayed_bugs => {
diagnostic.level = Error;
}
DelayedBug(DelayedBugKind::Normal) => {
let backtrace = std::backtrace::Backtrace::capture();
self.span_delayed_bugs
.push(DelayedDiagnostic::with_backtrace(diagnostic.clone(), backtrace));

#[allow(deprecated)]
return Some(ErrorGuaranteed::unchecked_claim_error_was_emitted());
#[allow(deprecated)]
return Some(ErrorGuaranteed::unchecked_claim_error_was_emitted());
}
DelayedBug(DelayedBugKind::GoodPath) => {
let backtrace = std::backtrace::Backtrace::capture();
self.good_path_delayed_bugs
.push(DelayedDiagnostic::with_backtrace(diagnostic.clone(), backtrace));

return None;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And here, can return an ErrorGuaranteed. So there's scope for more code sharing between these two match arms.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See above

}
_ => {}
}

if diagnostic.has_future_breakage() {
Expand Down Expand Up @@ -1396,11 +1411,18 @@ impl DiagCtxtInner {
self.emit_diagnostic(Diagnostic::new(FailureNote, msg));
}

fn flush_delayed(
&mut self,
bugs: Vec<DelayedDiagnostic>,
explanation: impl Into<DiagnosticMessage> + Copy,
) {
fn flush_delayed(&mut self, kind: DelayedBugKind) {
let (bugs, explanation) = match kind {
DelayedBugKind::Normal => (
std::mem::take(&mut self.span_delayed_bugs),
"no errors encountered even though `span_delayed_bug` issued",
),
DelayedBugKind::GoodPath => (
std::mem::take(&mut self.good_path_delayed_bugs),
"no warnings or errors encountered even though `good_path_delayed_bugs` issued",
),
};

if bugs.is_empty() {
return;
}
Expand Down Expand Up @@ -1433,7 +1455,7 @@ impl DiagCtxtInner {
if backtrace || self.ice_file.is_none() { bug.decorate() } else { bug.inner };

// "Undelay" the `DelayedBug`s (into plain `Bug`s).
if bug.level != DelayedBug {
if !matches!(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 @@ -1521,8 +1543,9 @@ pub enum Level {
/// silently dropped. I.e. "expect other errors are emitted" semantics. Useful on code paths
/// that should only be reached when compiling erroneous code.
///
/// Its `EmissionGuarantee` is `ErrorGuaranteed`.
DelayedBug,
/// Its `EmissionGuarantee` is `ErrorGuaranteed` for `Normal` delayed bugs, and `()` for
/// `GoodPath` delayed bugs.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, good path delayed bugs are also ErrorGuaranteed.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See above, this is not true

DelayedBug(DelayedBugKind),

/// An error that causes an immediate abort. Used for things like configuration errors,
/// internal overflows, some file operation errors.
Expand Down Expand Up @@ -1597,7 +1620,7 @@ impl Level {
fn color(self) -> ColorSpec {
let mut spec = ColorSpec::new();
match self {
Bug | DelayedBug | Fatal | Error => {
Bug | DelayedBug(_) | Fatal | Error => {
spec.set_fg(Some(Color::Red)).set_intense(true);
}
ForceWarning(_) | Warning => {
Expand All @@ -1617,7 +1640,7 @@ impl Level {

pub fn to_str(self) -> &'static str {
match self {
Bug | DelayedBug => "error: internal compiler error",
Bug | DelayedBug(_) => "error: internal compiler error",
Fatal | Error => "error",
ForceWarning(_) | Warning => "warning",
Note | OnceNote => "note",
Expand Down
1 change: 1 addition & 0 deletions compiler/rustc_session/src/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1146,6 +1146,7 @@ impl UnstableOptions {
DiagCtxtFlags {
can_emit_warnings,
treat_err_as_bug: self.treat_err_as_bug,
eagerly_emit_delayed_bugs: self.eagerly_emit_delayed_bugs,
macro_backtrace: self.macro_backtrace,
deduplicate_diagnostics: self.deduplicate_diagnostics,
track_diagnostics: self.track_diagnostics,
Expand Down
3 changes: 3 additions & 0 deletions compiler/rustc_session/src/options.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1583,6 +1583,9 @@ options! {
"version of DWARF debug information to emit (default: 2 or 4, depending on platform)"),
dylib_lto: bool = (false, parse_bool, [UNTRACKED],
"enables LTO for dylib crate type"),
eagerly_emit_delayed_bugs: bool = (false, parse_bool, [UNTRACKED],
"emit delayed bugs eagerly as errors instead of stashing them and emitting \
them only if an error has not been emitted"),
ehcont_guard: bool = (false, parse_bool, [TRACKED],
"generate Windows EHCont Guard tables"),
emit_stack_sizes: bool = (false, parse_bool, [UNTRACKED],
Expand Down
11 changes: 11 additions & 0 deletions tests/ui/treat-err-as-bug/eagerly-emit.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
// compile-flags: -Zeagerly-emit-delayed-bugs

trait Foo {}

fn main() {}

fn f() -> impl Foo {
//~^ ERROR the trait bound `i32: Foo` is not satisfied
//~| ERROR `report_selection_error` did not emit an error
1i32
}
28 changes: 28 additions & 0 deletions tests/ui/treat-err-as-bug/eagerly-emit.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
error: `report_selection_error` did not emit an error
--> $DIR/eagerly-emit.rs:7:11
|
LL | fn f() -> impl Foo {
| ^^^^^^^^

error: trimmed_def_paths constructed but no error emitted; use `DelayDm` for lints or `with_no_trimmed_paths` for debugging

error[E0277]: the trait bound `i32: Foo` is not satisfied
--> $DIR/eagerly-emit.rs:7:11
|
LL | fn f() -> impl Foo {
| ^^^^^^^^ the trait `Foo` is not implemented for `i32`
...
LL | 1i32
| ---- return type was inferred to be `i32` here
|
help: this trait has no implementations, consider adding one
--> $DIR/eagerly-emit.rs:3:1
|
LL | trait Foo {}
| ^^^^^^^^^

error: expected fulfillment errors
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's very strange that compiletest doesn't make you //~ expected fulfillment errors, though I consider that bug to be orthogonal to this PR.

Copy link
Member

@fmease fmease Jan 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not a bug, //~ only checks diagnostics that have a (non-dummy) span.
You should be able to do // error-pattern: expected fulfillment errors at the top of the file (while keeping the other //~s).


error: aborting due to 4 previous errors

For more information about this error, try `rustc --explain E0277`.
Loading