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.

Sign up for GitHub

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

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3330940
Avoid repetition in `flush_delayed` calls.
nnethercote Jan 11, 2024
eb79bc0
Good path bugs are just a flavor of delayed bug
compiler-errors Jan 12, 2024
7df43d3
Give me a way to emit all the delayed bugs
compiler-errors Jan 12, 2024
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
2 changes: 1 addition & 1 deletion compiler/rustc_errors/src/annotate_snippet_emitter_writer.rs
View file
Open in desktop
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
View file
Open in desktop
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
89 changes: 53 additions & 36 deletions compiler/rustc_errors/src/lib.rs
View file
Open in desktop
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 @@ -541,8 +547,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 +556,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 +866,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 +884,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 •
edited
Loading

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 +1219,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 +1269,27 @@ 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(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 +1405,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 +1449,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 +1537,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 +1614,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 +1634,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