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

Duplicate errors for overflowing division / remainder #69020

Closed
RalfJung opened this issue Feb 10, 2020 · 1 comment · Fixed by #69185
Closed

Duplicate errors for overflowing division / remainder #69020

RalfJung opened this issue Feb 10, 2020 · 1 comment · Fixed by #69185
Labels
A-const-eval Area: constant evaluation (mir interpretation) C-bug Category: This is a bug. D-verbose Diagnostics: Too much output caused by a single piece of incorrect code. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@RalfJung
Copy link
Member

RalfJung commented Feb 10, 2020

When there is an overflowing division or remainder operation that const_prop can detect, the error is duplicated in release mode only:

#![deny(const_err)]

use std::i32;

fn main() {
    let _ = i32::MIN / -1;
}

shows

error: attempt to divide with overflow
 --> src/main.rs:6:13
  |
6 |     let _ = i32::MIN / -1;
  |             ^^^^^^^^^^^^^
  |
note: the lint level is defined here
 --> src/main.rs:1:9
  |
1 | #![deny(const_err)]
  |         ^^^^^^^^^

error: this expression will panic at runtime
 --> src/main.rs:6:13
  |
6 |     let _ = i32::MIN / -1;
  |             ^^^^^^^^^^^^^ attempt to divide with overflow

This is because !overflow_check is true in release mode but divison and remainder still get overflow checks.

(This test case already covers the problem, so a fix does not need a new test case, it just needs to adjust the existing test case to no longer expect two ERROR per line.)

Cc @oli-obk @wesleywiser

@jonas-schievink jonas-schievink added A-const-eval Area: constant evaluation (mir interpretation) C-bug Category: This is a bug. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Feb 10, 2020
@RalfJung
Copy link
Member Author

RalfJung commented Feb 10, 2020

Basically the same issue also leads to duplicate errors for Neg, though that requires associated consts to actually trigger:

use std::i32;

pub trait Foo {
    const N: i32;
}

impl<T: Foo> Foo for Vec<T> {
    const N: i32 = -i32::MIN + T::N;
}

shows

error: attempt to negate with overflow
 --> src/lib.rs:8:20
  |
8 |     const N: i32 = -i32::MIN + T::N;
  |                    ^^^^^^^^^
  |
  = note: `#[deny(const_err)]` on by default

error: this expression will panic at runtime
 --> src/lib.rs:8:20
  |
8 |     const N: i32 = -i32::MIN + T::N;
  |                    ^^^^^^^^^ attempt to negate with overflow

Here, the cause is that even in release builds, const bodies use checked arithmetic. (The assoc const is needed because otherwise the const body just gets evaluated, not analyzed statically by const-prop.)

@estebank estebank added the D-verbose Diagnostics: Too much output caused by a single piece of incorrect code. label Feb 10, 2020
@bors bors closed this as completed in d237e0f Feb 20, 2020
jumbatm added a commit to jumbatm/rust that referenced this issue Apr 2, 2020
Make sure we check the case where the generic operand comes first, in
case any future changes make this ordering matter.
jumbatm added a commit to jumbatm/rust that referenced this issue Apr 2, 2020
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Apr 3, 2020
…RalfJung

Extend rust-lang#69020 test to include reversed operand order.

Make sure we still emit if a lint if the generic argument comes first. See rust-lang#70566 (comment).

r? @RalfJung
bors added a commit to rust-lang-ci/rust that referenced this issue Apr 3, 2020
Rollup of 6 pull requests

Successful merges:

 - rust-lang#70696 (Extend rust-lang#69020 test to include reversed operand order.)
 - rust-lang#70706 (Minor cleanup in rustdoc --check-theme)
 - rust-lang#70725 (Avoid `.unwrap()`s on `.span_to_snippet(...)`s)
 - rust-lang#70728 (Minor doc improvements on `AllocRef`)
 - rust-lang#70730 (Fix link in task::Wake docs)
 - rust-lang#70731 (Minor follow-up after renaming librustc(_middle))

Failed merges:

r? @ghost
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-const-eval Area: constant evaluation (mir interpretation) C-bug Category: This is a bug. D-verbose Diagnostics: Too much output caused by a single piece of incorrect code. 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.

3 participants