Rename `ErrorReported` -> `ErrorGuaranteed` by mark-i-m · Pull Request #93244 · rust-lang/rust

mark-i-m · 2022-01-23T18:38:38Z

The idea is that we would like to use it for both errors and delay_span_bug. Its semantics indicate a guarantee that compilation will fail.

eddyb · 2022-01-23T19:49:29Z

~~It was pretty late when I suggested this, and it amused me, so take that with a grain of salt.~~
(EDIT: the above sentence was written when the PR was using the DoomedCompilation name, it's basically resolved now - however, feedback on the new name is still welcome)

We could probably let this sit to see if anyone else from @rust-lang/compiler has opinions on the bikeshed.

jackh726 · 2022-01-23T19:58:46Z

Honestly, I think ErrorReported is more clear. Even with delay_span_bug, an error does get emitted (or it might be an ICE, I don't recall) even if a diagnostic isn't emitted.

eddyb · 2022-01-23T22:02:36Z

Even with delay_span_bug, an error does get emitted (or it might be an ICE, I don't recall) even if a diagnostic isn't emitted.

The problem with the name is one of ordering/causality. With delay_span_bug, we want a value of this token, but nothing happened yet. In that case, it's a promise, that an error will be reported.

ErrorInevitable is probably closer, but it sort of gives the opposite (and also wrong) impression of "all errors are deferred". Being extremely literal, like ErrorWasOrWillBeReported, seems somewhat undesirable.

CompileFailGuaranteed (or the DoomedCompilation not-entirely-serious suggestion) makes more sense to me, since the purpose of the token isn't to indicate that small self-contained part of the compiler errored itself, but rather than compilation will not succeed, which gives leeway to do things that would be too expensive, or unsound, otherwise (or, alternatively, skip steps that serve no purpose for diagnostics).

mark-i-m · 2022-01-23T23:10:13Z

I don't have strong feeling here myself. I understand Eddy's point, but also think the usage with DelaySpanBugEmitted is uncommon enough that it's not too important.

bors · 2022-01-24T03:06:41Z

☔ The latest upstream changes (presumably #93245) made this pull request unmergeable. Please resolve the merge conflicts.

eddyb · 2022-01-24T08:39:21Z

but also think the usage with DelaySpanBugEmitted is uncommon enough that it's not too important

I wouldn't say that, given that ty::Error is based on "delay_span_bug is enough". That is, ty::Errortoday contains a "proof token" that an error will be emitted, not that one was.
AFAIK that's more pervasive than ErrorReported from actual errors (before #93222, at least).

eddyb · 2022-01-24T08:40:58Z

@jackh726 Do you think ErrorGuaranteed or something similar, that doesn't imply when the error was/will be reported, is enough? I don't have a big issue with Error in the name, more with Reported, really.

jackh726 · 2022-01-24T13:52:13Z

ErrorGuaranteed sounds good

eddyb · 2022-01-25T15:50:58Z

Forgot to leave a comment, but I meant to say that this LGTM, I just want more @rust-lang/compiler eyes on it.
You can r=me in a few days if nobody argues against it - this feels too small for a FCP, but there might still be disagreements.

nikomatsakis · 2022-01-25T17:26:50Z

I am nervous about this. In fact, this precise usage has gotten us into trouble before.

nikomatsakis · 2022-01-25T17:28:07Z

The problem is this:

You have some code that gets into a situation where an error should be reported later. It emits span_bug and returns ErrorGuaranteed.
The code which should report the error comes later -- it somehow observes this ErrorGuaranteed and says "well, I guess I can supresss reporting this error since the user has already been inundated". Or perhaps the code which should report the error never even runs!
Then you get the delay-span-bug.

mark-i-m · 2022-01-25T17:52:35Z

@nikomatsakis That seems like an abuse of ErrorGuaranteed to me... I don't think it is meant as a signal about what kind of failure will happen -- just a static guarantee that compilation will fail. i.e. it is meant to prove that the compiler will not do something, rather than a marker that the compiler has done something.

nikomatsakis · 2022-01-25T17:55:22Z

Well, that's the opposite of ErrorReported =) which is meant precisely to be a marker of something that has happened, which allows us to do arbitrary things later (like: skip parts of our safety checks or suppress potential duplicate errors) without fear of unsoundness.

eddyb · 2022-01-25T18:41:12Z

without fear of unsoundness

Isn't that precisely why we have delay_span_bug? I don't think an ICE preventing unsound compilation is as bad as the respective unsoundness. It's suboptimal from an UX point of view, sure, but the only kind of "proof" we can encode today has to be obtainable from delay_span_bug, to be meaningful.

To avoid delay_span_bug, we would pretty much have to track every reported error that could possibly be relevant to something else, and propagate them that way (e.g. parse/name-resolution errors into HIR).

There's currently over 150 delay_span_bugs in the compiler, and some of them underpin {Ty,Const}Kind::Error, which were made to guarantee at leastdelay_span_bug, a while back (#69426 representing the finalization of that work, basically).

It seems much more work than what we can do more immediately, but maybe I'm missing a subtle detail.
I am curious about your dislike of delay_span_bug, since I view it as a relatively-low-overhead (infrastructure-wise) way to guarantee unsoundness in the presence of diagnostics that can't talk to eachother easily.

jackh726 · 2022-01-25T19:18:52Z

I'm a bit confused about the case @nikomatsakis is worried about; is it when we've already emitted a delay_span_bug, but later on emit an error anyways (but now wouldn't because of some "stronger" guarantee?)? That feels not great, but I'm be curious if there are any places that we actually do that.

I guess one "cheap" way we can solve this is to make ErrorGuaranteed an enum: Reported or NotReported. It's not great, since to "propagate" that you've reported an error when you've already seen an ErrorGuaranteed::NotReported would be to replace the existing Ty, through folding or some other means.

Another alternative would be some kind of error code enum, for a little bit more precise matching. A benefit of this is we could in theory use that to deduplicate previously-emitted errors (so, if we slowly change delay_span_bugs to actually errors, we don't start getting duplicates).

I think a key difference between delay_span_bugs and real errors is their ability to "stop" compilation; the latter will stop wfchecking if coherence checking fails, for example, while the former wouldn't. Along these lines, delay_span_bugs are often used as a sort of "we're in a weird state, but otherwise okay".

I can understand @nikomatsakis's point, kind of. But I don't know the best way to solve it. Maybe a solution is to move any errors that are emitted solely from a delay_span_bug to be propagated separately, and leave delay_span_bug only as a "silent" check on unsoundness.

nikomatsakis · 2022-01-25T19:36:58Z

@jackh726

I'm a bit confused about the case @nikomatsakis is worried about; is it when we've already emitted a delay_span_bug, but later on emit an error anyways (but now wouldn't because of some "stronger" guarantee?)? That feels not great, but I'm be curious if there are any places that we actually do that.

To be clear, my example is not theoretical: we actually had this bug. I forget the precise details, but it went something like this:

Some parts of the code ran at many times. At some points they could see that a given situation would result in a region error, so they returned ErrorReported, which eventually became ty::Err.

But region inference had not run yet. When it did go to run, it observed the ty::Err and opted not to run, because there were other trait resolution errors and it was deemed highly likely that the region errors would be confusing and derivative.

nikomatsakis · 2022-01-25T19:38:35Z

@eddyb

Isn't that precisely why we have delay_span_bug? I don't think an ICE preventing unsound compilation is as bad as the respective unsoundness. It's suboptimal from an UX point of view, sure, but the only kind of "proof" we can encode today has to be obtainable from delay_span_bug, to be meaningful.

Hmm, ok, I agree that delay_span_bug is different from the case I am mentioning. It may be that I'm ok with this change but I think we should be very careful to document the limitations here and to not go further into "this will generate an error later".

Generating an ICE is obviously suboptimal, but it's not unsoundness.

nagisa · 2022-01-25T19:41:04Z

I'm personally quite happy with the name, but an alternative could be FailureGuaranteed. It does not necessarily imply the graceful error as much, I feel; maybe it'd be enough to convince Niko ^^

nikomatsakis · 2022-01-25T19:43:32Z

I think I am happy so long as we have a rustc-dev-guide chapter explaining when it is ok to use "error guaranteed" (and outlining when it is not -- i.e., the scenario I wrote above).

mark-i-m · 2022-02-25T22:44:04Z

I have rebased this PR and open a PR on the rustc-dev-guide to document ErrorGuaranteed, as per Niko's request above: rust-lang/rustc-dev-guide#1316

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

oli-obk · 2022-02-26T08:54:28Z

r? @oli-obk

Assigning to myself as it was previously unassigned

bors · 2022-02-26T10:02:44Z

☔ The latest upstream changes (presumably #94392) made this pull request unmergeable. Please resolve the merge conflicts.

oli-obk · 2022-03-02T09:45:37Z

r=me after a rebase

@bors p=1 bitrotty

mark-i-m · 2022-03-02T15:45:42Z

@oli-obk rebased!

oli-obk · 2022-03-02T15:46:31Z

@bors r+

bors · 2022-03-02T15:46:33Z

📌 Commit e489a94 has been approved by oli-obk

bors · 2022-03-02T16:13:41Z

⌛ Testing commit e489a94 with merge 08504c6...

bors · 2022-03-02T18:37:27Z

☀️ Test successful - checks-actions
Approved by: oli-obk
Pushing 08504c6 to master...

rust-timer · 2022-03-02T20:08:18Z

Finished benchmarking commit (08504c6): 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.

@rustbot label: -perf-regression

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`

rustbot added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Jan 23, 2022

This comment has been minimized.
Sign in to view

bors added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Jan 24, 2022

mark-i-m changed the title ~~rename ErrorReported -> DoomedCompilation~~Rename ErrorReported -> ErrorGuaranteedJan 24, 2022

mark-i-m changed the title ~~Rename ErrorReported -> ErrorGuaranteed~~Rename ErrorReported -> ErrorGuaranteedJan 24, 2022

mark-i-m force-pushed the doomed branch from 69c3cbf to 36b25eeCompare January 24, 2022 21:46

eddyb approved these changes Jan 25, 2022
View reviewed changes

oli-obk approved these changes Jan 25, 2022
View reviewed changes

lcnr approved these changes Jan 25, 2022
View reviewed changes

wesleywiser approved these changes Jan 25, 2022
View reviewed changes

mark-i-m force-pushed the doomed branch from 3d97c14 to 324524fCompare February 25, 2022 21:47

mark-i-m mentioned this pull request Feb 25, 2022
Document ErrorGuaranteed rust-lang/rustc-dev-guide#1316
Merged

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 25, 2022

rust-highfive assigned oli-obk Feb 26, 2022

rename ErrorReported -> ErrorGuaranteed
e489a94

mark-i-m force-pushed the doomed branch from 324524f to e489a94Compare March 2, 2022 15:45

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 Mar 2, 2022

bors added the merged-by-bors This PR was explicitly merged by bors. label Mar 2, 2022

bors merged commit 08504c6 into rust-lang:masterMar 2, 2022

rustbot added this to the 1.61.0 milestone Mar 2, 2022

Uh oh!

Uh oh!

Conversation

mark-i-m commented Jan 23, 2022

Uh oh!

This comment has been minimized.

eddyb commented Jan 23, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jackh726 commented Jan 23, 2022

Uh oh!

eddyb commented Jan 23, 2022

Uh oh!

mark-i-m commented Jan 23, 2022

Uh oh!

bors commented Jan 24, 2022

Uh oh!

eddyb commented Jan 24, 2022

Uh oh!

eddyb commented Jan 24, 2022

Uh oh!

jackh726 commented Jan 24, 2022

Uh oh!

eddyb commented Jan 25, 2022

Uh oh!

nikomatsakis commented Jan 25, 2022

Uh oh!

nikomatsakis commented Jan 25, 2022

Uh oh!

mark-i-m commented Jan 25, 2022

Uh oh!

nikomatsakis commented Jan 25, 2022

Uh oh!

eddyb commented Jan 25, 2022

Uh oh!

jackh726 commented Jan 25, 2022

Uh oh!

nikomatsakis commented Jan 25, 2022

Uh oh!

nikomatsakis commented Jan 25, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nagisa commented Jan 25, 2022

Uh oh!

nikomatsakis commented Jan 25, 2022

Uh oh!

mark-i-m commented Feb 25, 2022

Uh oh!

oli-obk commented Feb 26, 2022

Uh oh!

bors commented Feb 26, 2022

Uh oh!

oli-obk commented Mar 2, 2022

Uh oh!

mark-i-m commented Mar 2, 2022

Uh oh!

oli-obk commented Mar 2, 2022

Uh oh!

bors commented Mar 2, 2022

Uh oh!

bors commented Mar 2, 2022

Uh oh!

bors commented Mar 2, 2022

Uh oh!

rust-timer commented Mar 2, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

12 participants

eddyb commented Jan 23, 2022•
edited
Loading

nikomatsakis commented Jan 25, 2022•
edited
Loading