-
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
Show the values and computation that would overflow a const evaluation or propagation #73513
Conversation
r? @estebank (rust_highfive has picked a reviewer for you, use r? to override) |
r=me on the changes with green CI I have one nitpick to change the output slightly, but I'm not blocking this PR on it:
or some other wording that mentions the type. |
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 |
Unfortunately we don't have the type available at that time. I could probably get |
cc @RalfJung this touches a little bit of miri-engine error handling |
5f24aa3
to
a1b92b4
Compare
@bors try @rust-timer queue |
Awaiting bors try build completion |
⌛ Trying commit a1b92b4456b18789a1d8decfd9136a752c811a1a with merge 54b2e11bab628c62703dc119d93e1807bba5e4c7... |
src/test/mir-opt/const_prop/bad_op_div_by_zero/rustc.main.ConstProp.diff
Show resolved
Hide resolved
|
||
error: this arithmetic operation will overflow | ||
--> $DIR/issue-69020-assoc-const-arith-overflow.rs:34:22 | ||
| | ||
LL | const ADD: i32 = (i32::MAX+1) + T::ADD; | ||
| ^^^^^^^^^^^^ attempt to add with overflow | ||
| ^^^^^^^^^^^^ attempt to compute `2147483647 + 1` which would overflow |
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.
So computations are enclosed in `
but constants are not? Are we doing that consistently?
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.
no
This comment has been minimized.
This comment has been minimized.
src/librustc_middle/ty/int.rs
Outdated
(2, _) => write!(fmt, "_u16")?, | ||
(4, _) => write!(fmt, "_u32")?, | ||
(8, _) => write!(fmt, "_u64")?, | ||
(16, _) => write!(fmt, "_u128")?, |
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.
Why the _
? Is there precedent for that?
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.
I was under the impression that that's the proper way to do this. I think clippy also suggests everything with an underscore
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.
No strong opinion, I just don't think I have seen this before.
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.
Are we emitting any other diagnostics in rustc like this? I haven't seen any either, but I could be out of touch.
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.
I haven't checked
src/librustc_middle/ty/int.rs
Outdated
@@ -0,0 +1,111 @@ | |||
use crate::mir::interpret::truncate; | |||
use rustc_target::abi::Size; |
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.
Now this file is even more removed from ConstValue
, which it seems closely related to.^^
Maybe call this ty/const.rs
, with the plan to move ConstValue
here as part of #72396?
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.
const
isn't possible, but consts
.
This comment has been minimized.
This comment has been minimized.
942aaa1
to
369e693
Compare
Heh, the PR already bitrotted. I rebased and sqashed the commits |
This comment has been minimized.
This comment has been minimized.
☔ The latest upstream changes (presumably #71911) made this pull request unmergeable. Please resolve the merge conflicts. |
2372049
to
23fa01f
Compare
@estebank there were significant changes to the test output, can you have another look to see if you're still ok with this? |
This comment has been minimized.
This comment has been minimized.
Only nitpicks would ve that we always say overflow even when it underflows and that we could potentially recommend changing types, but these things are very minor and shouldn't block this pr. I worry slightly that overflow is jargon that we never introduce, which is why I suggested the wordier labels before, but at the same time it is very common wording for people coming from other langs. None of these things needs to be addressed now. |
(FWIW I feel like "underflow" is much stranger jargon than "overflow". I'd call it "overflow" even when it's wrapping around negatively. But maybe I'm the odd one out there.) |
3b47280
to
9ed651f
Compare
@bors r=estebank |
📌 Commit 78b549f6607be49c6b6a49c3a2465a9756bf56f7 has been approved by |
⌛ Testing commit 78b549f6607be49c6b6a49c3a2465a9756bf56f7 with merge f7a7b6b77c6f06c2d97eed7b96e34156a753505b... |
💔 Test failed - checks-azure |
78b549f
to
819cde5
Compare
@bors r=estebank |
📌 Commit 819cde5 has been approved by |
☀️ Test successful - checks-azure |
Tested on commit rust-lang/rust@7750c3d. Direct link to PR: <rust-lang/rust#73513> 💔 miri on windows: test-pass → test-fail (cc @oli-obk @eddyb @RalfJung). 💔 miri on linux: test-pass → test-fail (cc @oli-obk @eddyb @RalfJung).
Fixes #71134
In contrast to the example in the issue it doesn't use individual spans for each operand. The effort required to implement that is quite high compared to the little (if at all) benefit it would bring to diagnostics.
cc @shepmaster
The way this is implemented it is also fairly easy to do the same for overflow panics at runtime, but that should be done in a separate PR since it may have runtime performance implications.