rustc_errors: let DiagnosticBuilder::emit return a "guarantee of emission".
#93368
Conversation
rustbot
added
T-compiler
|
Some changes occurred in src/tools/rustfmt. Some changes occured to the CTFE / Miri engine cc @rust-lang/miri Some changes occurred in src/tools/clippy. cc @rust-lang/clippy |
rust-highfive
added
the
S-waiting-on-review
|
@bors try @rust-timer queue |
|
Awaiting bors try build completion. @rustbot label: +S-waiting-on-perf |
rustbot
added
the
S-waiting-on-perf
|
⌛ Trying commit 0b5259b113998b904e2d715960ccad75cddf7db5 with merge d71c4f293fafbe0f5a5edbb5894e9f489269697b... |
This comment has been minimized.
This comment has been minimized.
|
☀️ Try build successful - checks-actions |
|
Queued d71c4f293fafbe0f5a5edbb5894e9f489269697b with parent 21b4a9c, future comparison URL. |
|
Finished benchmarking commit (d71c4f293fafbe0f5a5edbb5894e9f489269697b): comparison url. Summary: This benchmark run did not return any relevant results. If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf. Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR led to changes in compiler perf. @bors rollup=never |
rustbot
removed
the
S-waiting-on-perf
eddyb
force-pushed
the
diagbld-guarantee
branch
from
0b5259b
to
1291f7a
Compare
|
That "incremental" test failure (not an issue with incremental, to be clear, just output formatting) was interesting. I just fixed it and had to change a few details, which results in this change in how (click to expand before/after)
The difference is even more visible with colors: In particular, the indication that (Also we could probably replace |
This comment has been minimized.
This comment has been minimized.
eddyb
force-pushed
the
diagbld-guarantee
branch
from
1291f7a
to
c01c063
Compare
This comment has been minimized.
This comment has been minimized.
eddyb
force-pushed
the
diagbld-guarantee
branch
from
c01c063
to
3bcb813
Compare
|
☔ The latest upstream changes (presumably #93762) made this pull request unmergeable. Please resolve the merge conflicts. |
|
This is nice =) |
| // FIXME(eddyb) this should check for `has_errors` and stop pushing | ||
| // once *any* errors were emitted (and truncate `delayed_span_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. |
There was a problem hiding this comment.
I think doing that change would cause -Ztreat-err-as-bug=N stop working correctly to stop for a specific N.
There was a problem hiding this comment.
I don't think -Ztreat-err-as-bug cares about delay_span_bug at all? Like, the latter pushes to a separate buffer, my comment is about the inefficiency of keeping those around once they've become irrelevant. You can't actually get any behavior from delay_span_bug unless no errors were emitted at all, and -Ztreat-err-as-bug only affects real errors.
| @@ -121,7 +121,7 @@ pub fn parse_cfgspecs(cfgspecs: Vec<String>) -> FxHashSet<(String, Option<String | |||
| Ok(..) => {} | |||
| Err(err) => err.cancel(), | |||
| }, | |||
| Err(errs) => errs.into_iter().for_each(|mut err| err.cancel()), | |||
| Err(errs) => drop(errs), | |||
There was a problem hiding this comment.
This gives me a second of pause: does it mean that drop(err) is a valid cancelation methodology? Doesn't that go against what you're trying to accomplish?
There was a problem hiding this comment.
.cancel() was being called on a Diagnostic, not a DiagnosticBuilder. It basically was doing absolutely nothing before (other than writing to plain memory nothing will look at afterwards), I could've just as well replaced this with Err(_) => {}.
Perhaps some interface is confused about what types it should've used?
|
That was quite a review ^_^' r=me after rebase |
estebank
added
S-waiting-on-author
|
☀️ Test successful - checks-actions |
Tested on commit rust-lang/rust@d4de1f2. Direct link to PR: <rust-lang/rust#93368> 💔 miri on windows: test-fail → build-fail (cc @eddyb @oli-obk @RalfJung). 💔 miri on linux: test-fail → build-fail (cc @eddyb @oli-obk @RalfJung).
|
Finished benchmarking commit (d4de1f2): comparison url. Summary: This benchmark run shows 1 relevant improvement 🎉 but 4 relevant regressions 😿 to instruction counts.
If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf. Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression |
|
Is the |
|
I saw the exact same So I opened #94373, which makes So I would say it looks like #94373 has minimal perf impact (no regressions), and may eliminate this flakiness, so it might be worth merging. |
[beta] backport fix for rust-lang#94502 this issue was fixed as part of rust-lang#93368, so i extracted the change from there closes rust-lang#94502
rustc_errors: let `DiagnosticBuilder::emit` return a "guarantee of emission".
That is, `DiagnosticBuilder` is now generic over the return type of `.emit()`, so we'll now have:
* `DiagnosticBuilder<ErrorReported>` for error (incl. fatal/bug) diagnostics
* can only be created via a `const L: Level`-generic constructor, that limits allowed variants via a `where` clause, so not even `rustc_errors` can accidentally bypass this limitation
* asserts `diagnostic.is_error()` on emission, just in case the construction restriction was bypassed (e.g. by replacing the whole `Diagnostic` inside `DiagnosticBuilder`)
* `.emit()` returns `ErrorReported`, as a "proof" token that `.emit()` was called
(though note that this isn't a real guarantee until after completing the work on
rust-lang#69426)
* `DiagnosticBuilder<()>` for everything else (warnings, notes, etc.)
* can also be obtained from other `DiagnosticBuilder`s by calling `.forget_guarantee()`
This PR is a companion to other ongoing work, namely:
* rust-lang#69426
and it's ongoing implementation:
rust-lang#93222
the API changes in this PR are needed to get statically-checked "only errors produce `ErrorReported` from `.emit()`", but doesn't itself provide any really strong guarantees without those other `ErrorReported` changes
* rust-lang#93244
would make the choices of API changes (esp. naming) in this PR fit better overall
In order to be able to let `.emit()` return anything trustable, several changes had to be made:
* `Diagnostic`'s `level` field is now private to `rustc_errors`, to disallow arbitrary "downgrade"s from "some kind of error" to "warning" (or anything else that doesn't cause compilation to fail)
* it's still possible to replace the whole `Diagnostic` inside the `DiagnosticBuilder`, sadly, that's harder to fix, but it's unlikely enough that we can paper over it with asserts on `.emit()`
* `.cancel()` now consumes `DiagnosticBuilder`, preventing `.emit()` calls on a cancelled diagnostic
* it's also now done internally, through `DiagnosticBuilder`-private state, instead of having a `Level::Cancelled` variant that can be read (or worse, written) by the user
* this removes a hazard of calling `.cancel()` on an error then continuing to attach details to it, and even expect to be able to `.emit()` it
* warnings were switched to *only* `can_emit_warnings` on emission (instead of pre-cancelling early)
* `struct_dummy` was removed (as it relied on a pre-`Cancelled` `Diagnostic`)
* since `.emit()` doesn't consume the `DiagnosticBuilder` <sub>(I tried and gave up, it's much more work than this PR)</sub>,
we have to make `.emit()` idempotent wrt the guarantees it returns
* thankfully, `err.emit(); err.emit();` can return `ErrorReported` both times, as the second `.emit()` call has no side-effects *only* because the first one did do the appropriate emission
* `&mut Diagnostic` is now used in a lot of function signatures, which used to take `&mut DiagnosticBuilder` (in the interest of not having to make those functions generic)
* the APIs were already mostly identical, allowing for low-effort porting to this new setup
* only some of the suggestion methods needed some rework, to have the extra `DiagnosticBuilder` functionality on the `Diagnostic` methods themselves (that change is also present in rust-lang#93259)
* `.emit()`/`.cancel()` aren't available, but IMO calling them from an "error decorator/annotator" function isn't a good practice, and can lead to strange behavior (from the caller's perspective)
* `.downgrade_to_delayed_bug()` was added, letting you convert any `.is_error()` diagnostic into a `delay_span_bug` one (which works because in both cases the guarantees available are the same)
This PR should ideally be reviewed commit-by-commit, since there is a lot of fallout in each.
r? `@estebank` cc `@Manishearth` `@nikomatsakis` `@mark-i-m`
Change the trait_impls stdout file back, the `Display` impls have gotten changed back in the meantime it seems.
That is,
DiagnosticBuilderis now generic over the return type of.emit(), so we'll now have:DiagnosticBuilder<ErrorReported>for error (incl. fatal/bug) diagnosticsconst L: Level-generic constructor, that limits allowed variants via awhereclause, so not evenrustc_errorscan accidentally bypass this limitationdiagnostic.is_error()on emission, just in case the construction restriction was bypassed (e.g. by replacing the wholeDiagnosticinsideDiagnosticBuilder).emit()returnsErrorReported, as a "proof" token that.emit()was called(though note that this isn't a real guarantee until after completing the work on
Make emitting an error necessary to acquire a
ErrorReportedtoken #69426)DiagnosticBuilder<()>for everything else (warnings, notes, etc.)DiagnosticBuilders by calling.forget_guarantee()This PR is a companion to other ongoing work, namely:
ErrorReportedtoken #69426and it's ongoing implementation:
Make ErrorReported impossible to construct outside
rustc_errors#93222the API changes in this PR are needed to get statically-checked "only errors produce
ErrorReportedfrom.emit()", but doesn't itself provide any really strong guarantees without those otherErrorReportedchangesErrorReported->ErrorGuaranteed#93244would make the choices of API changes (esp. naming) in this PR fit better overall
In order to be able to let
.emit()return anything trustable, several changes had to be made:Diagnostic'slevelfield is now private torustc_errors, to disallow arbitrary "downgrade"s from "some kind of error" to "warning" (or anything else that doesn't cause compilation to fail)Diagnosticinside theDiagnosticBuilder, sadly, that's harder to fix, but it's unlikely enough that we can paper over it with asserts on.emit().cancel()now consumesDiagnosticBuilder, preventing.emit()calls on a cancelled diagnosticDiagnosticBuilder-private state, instead of having aLevel::Cancelledvariant that can be read (or worse, written) by the user.cancel()on an error then continuing to attach details to it, and even expect to be able to.emit()itcan_emit_warningson emission (instead of pre-cancelling early)struct_dummywas removed (as it relied on a pre-CancelledDiagnostic).emit()doesn't consume theDiagnosticBuilder(I tried and gave up, it's much more work than this PR),we have to make
.emit()idempotent wrt the guarantees it returnserr.emit(); err.emit();can returnErrorReportedboth times, as the second.emit()call has no side-effects only because the first one did do the appropriate emission&mut Diagnosticis now used in a lot of function signatures, which used to take&mut DiagnosticBuilder(in the interest of not having to make those functions generic)DiagnosticBuilderfunctionality on theDiagnosticmethods themselves (that change is also present in rustc_errors: only box thediagnosticfield inDiagnosticBuilder. #93259).emit()/.cancel()aren't available, but IMO calling them from an "error decorator/annotator" function isn't a good practice, and can lead to strange behavior (from the caller's perspective).downgrade_to_delayed_bug()was added, letting you convert any.is_error()diagnostic into adelay_span_bugone (which works because in both cases the guarantees available are the same)This PR should ideally be reviewed commit-by-commit, since there is a lot of fallout in each.
r? @estebank cc @Manishearth @nikomatsakis @mark-i-m