Skip to content
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

no warning for div by zero: let x = 1 / false as u8; #49760

Closed
matthiaskrgr opened this issue Apr 7, 2018 · 4 comments · Fixed by #52498
Closed

no warning for div by zero: let x = 1 / false as u8; #49760

matthiaskrgr opened this issue Apr 7, 2018 · 4 comments · Fixed by #52498
Assignees
Labels
A-diagnostics Area: Messages for errors, warnings, and lints C-bug Category: This is a bug. P-medium Medium priority T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@matthiaskrgr
Copy link
Member

inlined inside a println!(), I get a warning:

fn main() {
    println!("{}", 1/false as u8);
}
error[E0080]: constant evaluation error
 --> src/main.rs:4:17
  |
4 |     println!("{}", 1/false as u8);
  |                    ^^^^^^^^^^^^^ attempted to do overflowing math
error: aborting due to previous error

however, when I involve a variable

fn main() {
	let x = 1 / false as u8;
	println!("{}", x);
}

there is no warning at all but looking at the generated code it is quite clear that this panics:
https://godbolt.org/g/NQmSFM

@hanna-kruppe
Copy link
Contributor

hanna-kruppe commented Apr 7, 2018

It warns on current stable, but not on beta and nightly. Might be miri fallout? cc @oli-obk

Curiously, when you take a reference (to trigger promotion to static) you get a hard error on beta and nightly (stable is still a warning). Isn't that a regression?

@oli-obk oli-obk self-assigned this Apr 7, 2018
@oli-obk oli-obk added regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. regression-from-stable-to-beta Performance or correctness regression from stable to beta. labels Apr 7, 2018
@pnkfelix pnkfelix added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. P-high High priority labels Apr 12, 2018
bors added a commit that referenced this issue Apr 12, 2018
Don't report compile-time errors for promoteds

Fixes the regression part of #49760, the missing warnings still are missing

r? @eddyb
bors added a commit that referenced this issue Apr 23, 2018
Don't report compile-time errors for promoteds

Fixes the regression part of #49760, the missing warnings still are missing

r? @eddyb
@oli-obk oli-obk added A-diagnostics Area: Messages for errors, warnings, and lints and removed regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. P-high High priority regression-from-stable-to-beta Performance or correctness regression from stable to beta. labels Apr 24, 2018
@nikomatsakis
Copy link
Contributor

triage: P-medium

Still want to get those warnings, but no longer P-high

@nikomatsakis nikomatsakis added the P-medium Medium priority label Apr 26, 2018
@matthiaskrgr
Copy link
Member Author

Hm, what is the status of this?

The PR was merged, but with rustc 1.27.0-nightly (e5f80f2a4 2018-05-09), I don't get a compiler warning for the first example.

For the second one I get two though? 😕

   Compiling zero1 v0.1.0 (file:///tmp/zero1)
warning: constant evaluation error
 --> src/main.rs:3:17
  |
3 |     println!("{}", 1 / false as u8);
  |                    ^^^^^^^^^^^^^^^ attempt to divide by zero
  |
  = note: #[warn(const_err)] on by default
warning: constant evaluation error
 --> src/main.rs:3:17
  |
3 |     println!("{}", 1 / false as u8);
  |                    ^^^^^^^^^^^^^^^ attempt to divide by zero
    Finished dev [unoptimized + debuginfo] target(s) in 0.48s

@oli-obk
Copy link
Contributor

oli-obk commented May 11, 2018

The PR was just for the compilation error. The missing warnings just need an impl for this branch:

https://github.com/rust-lang/rust/blob/master/src/librustc_mir/transform/const_prop.rs#L236

The warning duplication is unfortunate, but if we deduplicate it, we loose warnings in other edge cases. We could deduplicate it similar to the way the notes about where the lint level was set are deduplicated (note how the second warning has no note: #[warn(const_err)] on by default message). That would probably be the cleanest solution.

@XAMPPRocky XAMPPRocky added the C-bug Category: This is a bug. label Jul 7, 2018
bors added a commit that referenced this issue Jul 20, 2018
Const propagate casts

fixes #49760

So... This fixes the original issue about the missing warnings.

But our test suite contains fun things like

```rust
fn foo() {}
assert_eq!(foo as i16, foo as usize as i16);
```

Which, will result in

> a raw memory access tried to access part of a pointer value as raw bytes

on both sides of the assertion. Because well... that's exactly what's going on! We're ripping out 16 bits of a pointer.
bors added a commit that referenced this issue Jul 23, 2018
Abort if a promoted fails to be const evaluable and its runtime checks didn't trigger

r? @eddyb

cc @RalfJung @nagisa

cc #49760
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-diagnostics Area: Messages for errors, warnings, and lints C-bug Category: This is a bug. P-medium Medium priority T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants