Make ErrorReported impossible to construct outside rustc_errors
#93222
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rustbot
added
T-compiler
|
r? @wesleywiser (rust-highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
This comment has been minimized.
This comment has been minimized.
camelid
reviewed
Jan 23, 2022
|
@mark-i-m I think your formatter isn't working correctly. Can you run |
eddyb
reviewed
Jan 23, 2022
eddyb
reviewed
Jan 23, 2022
eddyb
reviewed
Jan 23, 2022
Comment on lines
362
to
365
| if recursion_limit_hit { | ||
| // If we hit a recursion limit, exit early to avoid later passes getting overwhelmed | ||
| // with a large AST | ||
| Err(ErrorReported) | ||
| Err(ErrorReported::new_i_know_what_im_doing()) |
There was a problem hiding this comment.
Looks like reduced_recursion_limit could contain an ErrorReported.
There was a problem hiding this comment.
From whatever set it, which presumably produced an error in the process.
There was a problem hiding this comment.
IIUC, it starts out set and only becomes None if there is no error... so there may not be another good place to generate an ErrorReported...
eddyb
reviewed
Jan 23, 2022
eddyb
reviewed
Jan 23, 2022
compiler/rustc_driver/src/lib.rs
Outdated
Comment on lines
1152
to
1145
| if value.is::<rustc_errors::FatalErrorMarker>() { | ||
| ErrorReported | ||
| ErrorReported::new_i_know_what_im_doing() |
There was a problem hiding this comment.
This should be a downcast, not just .is::<...>(), and FatalErrorMarker should contain ErrorReported.
There was a problem hiding this comment.
Unfortunately, this is difficult because FatalError is defined in rustc_span
eddyb
reviewed
Jan 23, 2022
eddyb
reviewed
Jan 23, 2022
eddyb
reviewed
Jan 23, 2022
eddyb
reviewed
Jan 23, 2022
eddyb
reviewed
Jan 23, 2022
eddyb
reviewed
Jan 23, 2022
eddyb
reviewed
Jan 23, 2022
eddyb
reviewed
Jan 23, 2022
compiler/rustc_errors/src/lib.rs
Outdated
| @@ -1000,6 +1003,8 @@ impl HandlerInner { | |||
| } else { | |||
| self.bump_warn_count(); | |||
| } | |||
|
|
|||
| ErrorReported::new_i_know_what_im_doing() | |||
There was a problem hiding this comment.
This needs to check diagnostic.is_error().
There was a problem hiding this comment.
Overall, it feels like the nice thing would be to seal ErrorReported and have an impl in the same "sealed" module that ErrorReported would be in, with a wrapper method for self.emitter.emit_diagnostic(diagnostic); that checks for the conditions for ErrorReported and creates it if necessary.
Another wrapper could be added for self.delayed_span_bugs.push(diagnostic);. Those should be the only cases in which you can guarantee ErrorReported. Error deduplication can keep a ErrorReported around for reuse (in general, this should be favored because it's free, being a ZST).
eddyb
reviewed
Jan 23, 2022
eddyb
reviewed
Jan 23, 2022
eddyb
reviewed
Jan 23, 2022
mark-i-m
force-pushed
the
errorreported
branch
from
9a12cc5
to
caf4e6a
Compare
eddyb
reviewed
Jan 23, 2022
eddyb
reviewed
Jan 23, 2022
eddyb
reviewed
Jan 23, 2022
| if self.err_count() == old_count { | ||
| Ok(result) | ||
| } else { | ||
| Err(ErrorReported::new_i_know_what_im_doing()) |
There was a problem hiding this comment.
err_count can be something like Option<(NonZeroUsize, ErrorReported)>, making this only slightly harder than simply ==.
There was a problem hiding this comment.
Tried this, but it creates a lot of noise everywhere else that err_count is used...
|
📌 Commit bb8d430 has been approved by |
bors
added
S-waiting-on-bors
|
@bors p=100 (testing to see if ci works fine now after the outage earlier) |
|
⌛ Testing commit bb8d430 with merge 5f69cbe7530c8201946c38966e9e4dc424aa0ed6... |
|
@bors retry |
|
⌛ Testing commit bb8d430 with merge d4c27e66acfef0714867a438bad92a8501dc3ac4... |
|
💔 Test failed - checks-actions |
bors
added
S-waiting-on-review
|
The job Click to see the possible cause of the failure (guessed by this bot) |
|
@bors retry Ran out of space. For some reason, the runner had an unusually small root partition (84G instead of the normal 667G). Investigation is needed, but tree is closed for now. |
bors
added
S-waiting-on-bors
|
⌛ Testing commit bb8d430 with merge c1e4de654ee31e9f368af371fe8bd269c0ec546e... |
|
💔 Test failed - checks-actions |
bors
added
S-waiting-on-review
|
@bors retry p=0 Back to the docker cache/cmake problem. |
bors
added
S-waiting-on-bors
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Mar 18, 2022
Rollup of 10 pull requests Successful merges: - rust-lang#91133 (Improve `unsafe` diagnostic) - rust-lang#93222 (Make ErrorReported impossible to construct outside `rustc_errors`) - rust-lang#93745 (Stabilize ADX target feature) - rust-lang#94309 ([generator_interior] Be more precise with scopes of borrowed places) - rust-lang#94698 (Remove redundant code from copy-suggestions) - rust-lang#94731 (Suggest adding `{ .. }` around a const function call with arguments) - rust-lang#94960 (Fix many spelling mistakes) - rust-lang#94982 (Add deprecated_safe feature gate and attribute, cc rust-lang#94978) - rust-lang#94997 (debuginfo: Fix ICE when generating name for type that produces a layout error.) - rust-lang#95000 (Fixed wrong type name in comment) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There are a few places were we have to construct it, though, and a few
places that are more invasive to change. To do this, we create a
constructor with a long obvious name.
cc #69426 @varkor @eddyb @estebank
I actually didn't see that I was assigned to this issue until now...