-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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.
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
more specific errors in src/librustc/mir/interpret/error.rs #60951
Conversation
r? @estebank (rust_highfive has picked a reviewer for you, use r? to override) |
r? @oli-obk |
@oli-obk I have just copy pasted the enum specified in the RFC, so that we can discuss how to do stuff over here. |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
The current error enum is defined in https://github.com/rust-lang/rust/blob/b4aba66b0577bcd2228afd01421b136c72965b28/src/librustc/mir/interpret/error.rs#L334 You should not copy the one from the RFC issue, but modify the current one, as there may have been significant changes. The RFC issue only serves as a guideline on what to do |
ping from triage @saleemjaffer any updates? |
@Dylan-DPC I was occupied with something over the last two weeks. I can pick this up again in a while. |
ping from triage @saleemjaffer |
@oli-obk @Dylan-DPC I can work on this now :) |
b4aba66
to
efb7457
Compare
@oli-obk Don't know why does it say that I have closed the PR. Could you reopen it? I have tried doing some refactor, but it does not show up on this PR. Github asks me to open a new PR. |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
@oli-obk I have tried to start off by changing the enum fields as per the RFC. How do I proceed now? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The next steps would be to make all uses of the moved variants compile again.
src/librustc/mir/interpret/error.rs
Outdated
@@ -281,6 +284,29 @@ pub enum InterpError<'tcx, O> { | |||
required: Align, | |||
has: Align, | |||
}, | |||
MemoryLockViolation { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these suddenly showed up in this PR
src/librustc/mir/interpret/error.rs
Outdated
@@ -247,6 +247,7 @@ pub enum InterpError<'tcx, O> { | |||
DanglingPointerDeref, | |||
DoubleFree, | |||
InvalidMemoryAccess, | |||
FunctionPointerTyMismatch(FnSig<'tcx>, FnSig<'tcx>), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this wasn't here before this PR, maybe a rebase gone wrong?
src/librustc/mir/interpret/error.rs
Outdated
@@ -319,6 +340,16 @@ pub enum InterpError<'tcx, O> { | |||
InfiniteLoop, | |||
} | |||
|
|||
#[derive(Clone, RustcEncodable, RustcDecodable)] | |||
pub enum EvalErrorPanic<'tcx, O> { | |||
Panic, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this needs the fields of the old Panic
variant from above.
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
61fc12e
to
ed6c5cd
Compare
@oli-obk How does it look? |
ed6c5cd
to
90426ed
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just two small things, then it's ready to go
| OverflowNeg | ||
| DivisionByZero | ||
| RemainderByZero | ||
| Panic(EvalErrorPanic::Panic { .. }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just use Panic(_)
instead of listing all of these.
This code here is one of the reasons we want a Panic
variant
src/librustc/mir/interpret/error.rs
Outdated
@@ -228,6 +228,24 @@ impl<'tcx> From<InterpError<'tcx, u64>> for InterpErrorInfo<'tcx> { | |||
|
|||
pub type AssertMessage<'tcx> = InterpError<'tcx, mir::Operand<'tcx>>; | |||
|
|||
#[derive(Clone, RustcEncodable, RustcDecodable, HashStable)] | |||
pub enum EvalErrorPanic<O> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Name this enum PanicMessage
. This way it makes more sense when we'll start reusing it in TerminatorKind::Assert
's msg
field
@oli-obk Made the changes. |
Thanks! @bors r+ If you are interested in continuing this avenue, you can (on top of this PR, but in a new PR and branch) change the type of the |
📌 Commit 3730ed9 has been approved by |
… r=oli-obk more specific errors in src/librustc/mir/interpret/error.rs Implements [this](rust-lang/const-eval#4)
Rollup of 14 pull requests Successful merges: - #60951 (more specific errors in src/librustc/mir/interpret/error.rs) - #62523 (Delay bug to resolve HRTB ICE) - #62656 (explain how to search in slice without owned data) - #62791 (Handle more cases of typos misinterpreted as type ascription) - #62804 (rustc_typeck: improve diagnostics for _ const/static declarations) - #62808 (Revert "Disable stack probing for gnux32.") - #62817 (Tweak span for variant not found error) - #62842 (Add tests for issue-58887) - #62851 (move unescape module to rustc_lexer) - #62859 (Place::as_place_ref is now Place::as_ref) - #62869 (add rustc_private as a proper language feature gate) - #62880 (normalize use of backticks in compiler messages for librustc_allocator) - #62885 (Change "OSX" to "macOS") - #62889 (Update stage0.txt) Failed merges: r? @ghost
build: Fix build after rust-lang/rust#60951
build: Fix build after rust-lang/rust#60951
Implements this