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

compiler-errors · 2024-01-12T00:32:53Z

This is probably a better way to inspect all the delayed bugs in a program that what exists currently (and therefore makes it very easy to choose the right number N with -Zemit-err-as-bug=N, though I guess the naming is a bit ironic when you pair both of the flags together, but that feels like naming bikeshed more than anything).

This pacifies my only concern with #119871 (comment), because (afaict?) that PR doesn't allow you to intercept a delayed bug's stack trace anymore, which as someone who debugs the compiler a lot, is something that I can promise that I do.

r? @nnethercote or @oli-obk

There are two places that handle normal delayed bugs. This commit factors out some repeated code. Also, we can use `std::mem::take` instead of `std::mem::replace`.

compiler-errors · 2024-01-12T00:35:46Z

tests/ui/treat-err-as-bug/eagerly-emit.stderr

+LL | trait Foo {}
+   | ^^^^^^^^^
+
+error: expected fulfillment errors


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

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).

nnethercote · 2024-01-12T02:35:35Z

Just last week I removed -Zreport-delayed-bugs in #119567. How did it compare to this PR's approach?

On the bad side: it emitted the bugs immediately but also stored them for possible later printing, which was much uglier than the approach in this PR. Removing it helped pave the way for Consuming emit #119606.
On the good side: it handled both normal delayed bugs and good path delayed bugs.

I think this new option should handle good path delayed bugs. Currently DiagCtxt::good_path_delayed_bug is implemented differently from DiagCtxt::span_delayed_bug. They could be united by giving Level::DelayedBug a DelayedBugKind value (added in #119871) that indicates whether it's a normal delayed bug or a good path delayed bug, and then emit_diagnostic would consult that flag to choose which bug vector to push onto.

I also wonder if -Ztreat-delayed-bug-as-err would be a better name. It's consistent with -Ztreat-err-as-bug.

And that makes me wonder if the two options could be combined. Could we drop this PR and just change -Ztreat-err-as-bug to also treat delayed bugs as errors? I'm not sure if that's a good or a bad idea.

compiler-errors · 2024-01-12T02:43:14Z

I think this new option should handle good path delayed bugs. [...] They could be united by giving Level::DelayedBug a DelayedBugKind value (added in #119871) that indicates whether it's a normal delayed bug or a good path delayed bug [...]

I'm happy to implement this. I can see why emitting good-path bugs as errors should happen if we're emitting delayed bugs.

I also wonder if -Ztreat-delayed-bug-as-err would be a better name. It's consistent with -Ztreat-err-as-bug.

I don't care about naming really. As long as this helps make sure we preserve the functionality that's being lost in #119871.

And that makes me wonder if the two options could be combined. Could we drop this PR and just change -Ztreat-err-as-bug to also treat delayed bugs as errors? I'm not sure if that's a good or a bad idea.

I have a hunch that it would be unnecessarily noisy to emit all bugs on -Ztreat-err-as-bug. I admit that most people use -Ztreat-err-as-bug to triage visible errors and don't care about delayed bugs, hence this flag existing primarily to bridge the gap for the rarer and intentionally more noisy debugging experience of caring about delayed bugs.

nnethercote · 2024-01-12T02:46:28Z

Can you give an example of how you would use this new flag? I'm not doubting that you would, I just want to understand how it would get used, in case that gives me ideas for alternatives.

compiler-errors · 2024-01-12T02:53:29Z

I can't give a hard example of a specific ICE I've fixed where I've needed to use -Ztreat-err-as-bug=N where N is some delayed bug, but I've often used the backtrace of delayed bugs as part of my debugging workflow. Some areas in particular are: in astconv, where we delay bugs when invariants are broken; in trait error reporting, where we suppress diagnostics in favor of others; in typeck (and throughout the compiler), when we delay a bug on the creation of TyKind::Error and the likes.

Before this PR, I would have acquire such a stack trace by starting at -Ztreat-err-as-bug=1, then incrementing to =2, =3, etc.. until I got to the stack trace that I wanted. This is just counting up the delayed bug, becuase honestly one can't know which N to actually put for -Ztreat-err-as-bug=N until you try it out on a real program¹.

With this PR, I would simply add -Zeagerly-emit-delayed-bugs, count the number of errors that were emitted until the one I wanted, then plug that into -Ztreat-err-as-bug=N. With #119871, such a workflow would continue to work, since these are being eagerly emitted as errors, and it has the added benefit of being able to see all the delayed bugs at once.

additionally, I didn't know about the existence of -Zreport-delayed-bugs. If it works like this PR does, i.e. delayed bugs were reported eagerly and in order, then I'd probably have used it if it still existed, but especially would've needed to use it if Overhaul -Ztreat-err-as-bug #119871 landed. IDK if it does the same thing that this PR does in practice, tho, and I'm too lazy to look. ↩

nnethercote · 2024-01-12T03:12:40Z

Thanks for the info. I think the updated -Ztreat-err-as-bug and the new -Zeagerly-emit-delayed-bugs will be much more flexible and usable than the old -Ztreat-err-as-bug and the old -Zreport-delayed-bugs, and the implementation is simpler as well.

compiler-errors · 2024-01-12T03:33:51Z

I'm not certain that we landed on an obvious correct name for the flag, so I didn't change that yet, but I did cherry-pick 3330940 to consolidate good path bugs into the regular error reporting pathway.

Now -Zeagerly-emit-delayed-bugs reports good path bugs too.

nnethercote · 2024-01-12T03:55:25Z

compiler/rustc_errors/src/diagnostic.rs


            Level::ForceWarning(_)
            | Level::Warning
+            | Level::DelayedBug(DelayedBugKind::GoodPath)


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

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.

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!

nnethercote · 2024-01-12T04:00:08Z

compiler/rustc_errors/src/lib.rs

-        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()


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

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.

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.

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.

nnethercote · 2024-01-12T04:00:46Z

compiler/rustc_errors/src/lib.rs

+                self.good_path_delayed_bugs
+                    .push(DelayedDiagnostic::with_backtrace(diagnostic.clone(), backtrace));
+
+                return None;


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

nnethercote · 2024-01-12T04:02:31Z

compiler/rustc_errors/src/lib.rs

-    /// Its `EmissionGuarantee` is `ErrorGuaranteed`.
-    DelayedBug,
+    /// Its `EmissionGuarantee` is `ErrorGuaranteed` for `Normal` delayed bugs, and `()` for
+    /// `GoodPath` delayed bugs.


Again, good path delayed bugs are also ErrorGuaranteed.

See above, this is not true

nnethercote · 2024-01-12T04:15:03Z

I'm content with the name -Zeagerly-emit-delayed-bugs.

Overall it LGTM, though my distaste for good path delayed bugs has risen a notch. I figure it's worth @oli-obk reviewing it as well, given my confusion above.

oli-obk · 2024-01-12T08:34:53Z

my distaste for good path delayed bugs

oh yea, they are the worst 😆

@bors r=oli-obk,nnethercote

bors · 2024-01-12T08:34:56Z

📌 Commit 7df43d3 has been approved by oli-obk,nnethercote

It is now in the queue for this repository.

…llaumeGomez Rollup of 6 pull requests Successful merges: - rust-lang#119817 (Remove special-casing around `AliasKind::Opaque` when structurally resolving in new solver) - rust-lang#119819 (Check rust lints when an unknown lint is detected) - rust-lang#119872 (Give me a way to emit all the delayed bugs as errors (add `-Zeagerly-emit-delayed-bugs`)) - rust-lang#119877 (Add more information to `visit_projection_elem`) - rust-lang#119884 (Rename `--env` option flag to `--env-set`) - rust-lang#119885 (Revert rust-lang#113923) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of rust-lang#119872 - compiler-errors:eagerly-emit-delayed-bugs, r=oli-obk,nnethercote Give me a way to emit all the delayed bugs as errors (add `-Zeagerly-emit-delayed-bugs`) This is probably a *better* way to inspect all the delayed bugs in a program that what exists currently (and therefore makes it very easy to choose the right number `N` with `-Zemit-err-as-bug=N`, though I guess the naming is a bit ironic when you pair both of the flags together, but that feels like naming bikeshed more than anything). This pacifies my only concern with rust-lang#119871 (comment), because (afaict?) that PR doesn't allow you to intercept a delayed bug's stack trace anymore, which as someone who debugs the compiler a lot, is something that I can *promise* that I do. r? `@nnethercote` or `@oli-obk`

Avoid repetition in flush_delayed calls.

3330940

There are two places that handle normal delayed bugs. This commit factors out some repeated code. Also, we can use `std::mem::take` instead of `std::mem::replace`.

rustbot assigned nnethercote Jan 12, 2024

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jan 12, 2024

compiler-errors commented Jan 12, 2024

View reviewed changes

compiler-errors added 2 commits January 12, 2024 03:29

Good path bugs are just a flavor of delayed bug

eb79bc0

Give me a way to emit all the delayed bugs

7df43d3

rust-cloud-vms bot force-pushed the eagerly-emit-delayed-bugs branch from 04532e9 to 7df43d3 Compare January 12, 2024 03:33

compiler-errors mentioned this pull request Jan 12, 2024

Overhaul -Ztreat-err-as-bug #119871

Merged

nnethercote reviewed Jan 12, 2024

View reviewed changes

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 12, 2024

GuillaumeGomez mentioned this pull request Jan 12, 2024

Rollup of 6 pull requests #119889

Merged

bors merged commit 504794b into rust-lang:master Jan 12, 2024
11 checks passed

rustbot added this to the 1.77.0 milestone Jan 12, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

compiler-errors commented Jan 12, 2024 •

edited

Loading

compiler-errors Jan 12, 2024

fmease Jan 12, 2024 •

edited

Loading

nnethercote commented Jan 12, 2024

compiler-errors commented Jan 12, 2024

nnethercote commented Jan 12, 2024

compiler-errors commented Jan 12, 2024 •

edited

Loading

nnethercote commented Jan 12, 2024

compiler-errors commented Jan 12, 2024 •

edited

Loading

nnethercote Jan 12, 2024

compiler-errors Jan 12, 2024

nnethercote Jan 12, 2024

nnethercote Jan 12, 2024

compiler-errors Jan 12, 2024 •

edited

Loading

nnethercote Jan 12, 2024

compiler-errors Jan 12, 2024

nnethercote Jan 12, 2024

compiler-errors Jan 12, 2024

nnethercote Jan 12, 2024

compiler-errors Jan 12, 2024

nnethercote commented Jan 12, 2024

oli-obk commented Jan 12, 2024

bors commented Jan 12, 2024

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

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

Conversation

compiler-errors commented Jan 12, 2024 • edited Loading

Choose a reason for hiding this comment

fmease Jan 12, 2024 • edited Loading

Choose a reason for hiding this comment

nnethercote commented Jan 12, 2024

compiler-errors commented Jan 12, 2024

nnethercote commented Jan 12, 2024

compiler-errors commented Jan 12, 2024 • edited Loading

Footnotes

nnethercote commented Jan 12, 2024

compiler-errors commented Jan 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

compiler-errors Jan 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nnethercote commented Jan 12, 2024

oli-obk commented Jan 12, 2024

bors commented Jan 12, 2024

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

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

compiler-errors commented Jan 12, 2024 •

edited

Loading

fmease Jan 12, 2024 •

edited

Loading

compiler-errors commented Jan 12, 2024 •

edited

Loading

compiler-errors commented Jan 12, 2024 •

edited

Loading

compiler-errors Jan 12, 2024 •

edited

Loading