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

const_prop_lint can't tell when code guarded with && is dead #113905

Open
SvenKeimpema opened this issue Jul 20, 2023 · 8 comments
Open

const_prop_lint can't tell when code guarded with && is dead #113905

SvenKeimpema opened this issue Jul 20, 2023 · 8 comments
Labels
A-const-prop Area: Constant propagation A-lints Area: Lints (warnings about flaws in source code) such as unused_mut. C-bug Category: This is a bug. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@SvenKeimpema
Copy link

I tried this code:

macro_rules! set_bit {
    ($bb:expr, $sq:expr) => {
        {
            let mut bb = $bb as u64;
            let sq = $sq as i32;
            if sq >= 0 && sq < 64 {
                bb |= (1u64 << $sq);
            }
            bb
        }
    };
}

I expected to see this happen: i expected the code to run if i simply passed sq=64 to the macro as an parameter, however it threw an:

error: this arithmetic operation will overflow
  --> main.rs:10:23
   |
10 |                 bb |= (1u64 << $sq);
   |                       ^^^^^^^^^^^^^ attempt to shift left by `64_i32`, which would overflow
...
63 |     println!("{}", set_bit!(0, 64));
   |                    --------------- in this macro invocation
   |
   = note: `#[deny(arithmetic_overflow)]` on by default
   = note: this error originates in the macro `set_bit` (in Nightly builds, run with -Z macro-backtrace for more info)

error: aborting due to previous error; 4 warnings emitted

which is pretty strange since it will never be able to actually perform the bitwise if sq >= 64.

It's also to note if i change the bitwise operation to bb |= (1u64 << sq); (removing the dollar sign) it won't throw this error.
I thinks this is due to an oversight where if the sq operator is copied to an different variable it won't release a if-statement is done to prevent a arithmetic_overflow.

Meta

rustc --version --verbose:

rustc 1.66.1 (90743e729 2023-01-10) (built from a source tarball)
binary: rustc
commit-hash: 90743e7298aca107ddaa0c202a4d3604e29bfeb6
commit-date: 2023-01-10
host: x86_64-unknown-linux-gnu
release: 1.66.1
LLVM version: 15.0.7

@SvenKeimpema SvenKeimpema added the C-bug Category: This is a bug. label Jul 20, 2023
@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Jul 20, 2023
@saethlin
Copy link
Member

Here's a significantly more silly minimized version:

if false && false {
    let _ = 1u64 << 64;
}

The problem is that the lint here has trouble seeing that code is dead. Something about the way we lower && confuses it.

@saethlin saethlin changed the title Incorrect overflow error in macro const_prop_lint can't tell when code guarded with && is dead Jul 21, 2023
@saethlin saethlin added A-const-prop Area: Constant propagation and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Jul 21, 2023
@cjgillot
Copy link
Contributor

#111752 may help with that. To check once it lands.

@chenyukang
Copy link
Member

chenyukang commented Jul 23, 2023

Even this code also don't have a lint?

if false {
   let _ = 1u64 << 64;
}

I expect something like any code following this block is unreachable.

@saethlin
Copy link
Member

@chenyukang In current stable, that code correctly does not have a lint. In 1.69 and before, it actually did hit this lint. So things have gotten better recently, but it's still too easy to hit these scuffed cases.

@chenyukang
Copy link
Member

The result of 1.67.1 for code:

fn main() {
    if 1 < 2 {
        let _ = 1u64 << 64;
    } else {
        let v = 1u64 << 64;
        eprintln!("v: {:?}", v);
    }
}

is:

~/rust ❯❯❯ rustc ./p/if.rs
error: this arithmetic operation will overflow
 --> ./p/if.rs:3:17
  |
3 |         let _ = 1u64 << 64;
  |                 ^^^^^^^^^^ attempt to shift left by `64_i32`, which would overflow
  |
  = note: `#[deny(arithmetic_overflow)]` on by default

error: this arithmetic operation will overflow
 --> ./p/if.rs:5:17
  |
5 |         let v = 1u64 << 64;
  |                 ^^^^^^^^^^ attempt to shift left by `64_i32`, which would overflow

error: aborting due to 2 previous errors

Should we check the if-else branch with some kind of const-eval checking? I expect the result is something like this:

~/rust ❯❯❯ rustc ./p/if.rs
error: this arithmetic operation will overflow
 --> ./p/if.rs:3:17
  |
3 |         let _ = 1u64 << 64;
  |                 ^^^^^^^^^^ attempt to shift left by `64_i32`, which would overflow
  |
  = note: `#[deny(arithmetic_overflow)]` on by default

error: this block is unreachable
 --> ./p/if.rs:5:17
  |
5 |         let v = 1u64 << 64;
  | 

error: aborting due to 2 previous errors

We have warn_if_unreachable, but seems it only checking expressions following return or panic!.

pub(in super::super) fn warn_if_unreachable(&self, id: hir::HirId, span: Span, kind: &str) {

@saethlin
Copy link
Member

saethlin commented Jul 24, 2023

The lint that we are discussing here lints by running the const eval interpreter.

We definitely should not warn or error on if false {, that's for example what debug_assert! expands to when debug assertions are off. The concern in this issue is that we shouldn't report unconditional panics in unreachable code. Because the panic isn't a problem.

@fmease fmease added A-lints Area: Lints (warnings about flaws in source code) such as unused_mut. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Sep 6, 2023
@RalfJung
Copy link
Member

I think this is a duplicate of #109731?

@veera-sivarajan
Copy link
Contributor

Output on rustc 1.79.0 for code:

fn main() {
    if 1 < 2 {
        let _ = 1u64 << 64;
    } else {
        let v = 1u64 << 64;
        eprintln!("v: {:?}", v);
    }
}

Output:

error: this arithmetic operation will overflow
 --> <source>:3:17
  |
3 |         let _ = 1u64 << 64;
  |                 ^^^^^^^^^^ attempt to shift left by `64_i32`, which would overflow
  |
  = note: `#[deny(arithmetic_overflow)]` on by default

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-const-prop Area: Constant propagation A-lints Area: Lints (warnings about flaws in source code) such as unused_mut. C-bug Category: This is a bug. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

8 participants