const-propagated arithmetic_overflow in unreachable code

[cuviper]

Code

I tried this code: playground

fn main() {
    let x = 2u32;
    let y = 3u32;
    // ...
    if y <= x {
        dbg!(x - y);
    }
}

I expected to see this happen: compiles, nothing printed

Instead, this happened:

error: this arithmetic operation will overflow
 --> src/main.rs:6:14
  |
6 |         dbg!(x - y);
  |              ^^^^^ attempt to compute `2_u32 - 3_u32`, which would overflow
  |
  = note: `#[deny(arithmetic_overflow)]` on by default

The original code comes from macro-generated test cases in num-integer, which failed here:
rust-num/num-integer#54 (comment)

Version it worked on

It most recently worked on: 1.69.0-beta.3

Version with regression

rustc --version --verbose:

rustc 1.70.0-nightly (478cbb42b 2023-03-28)
binary: rustc
commit-hash: 478cbb42b730ba4739351b72ce2aa928e78e2f81
commit-date: 2023-03-28
host: x86_64-unknown-linux-gnu
release: 1.70.0-nightly
LLVM version: 16.0.0

@rustbot modify labels: +regression-from-stable-to-nightly -regression-untriaged

[cuviper]

Note that this only errors in a full build, not check.

cargo bisect-rustc identified b05bb29 -- cc #108872 @cjgillot @oli-obk

[apiraino]

WG-prioritization assigning priority (Zulip discussion).

@rustbot label -I-prioritize +P-high

[cuviper]

@cjgillot I found that this is also failing in some num-traits test cases, and the new change doesn't help. Here's a minimization of that:

use std::cmp::Ordering::*;
fn main() {
    let max = u64::MAX as usize;
    match (usize::MAX as u64).cmp(&u64::MAX) {
        Greater => {
            dbg!(max + 1);
        }
        Equal | Less => {}
    }
}

$ rustc +nightly -V
rustc 1.70.0-nightly (28a29282f 2023-04-06)
$ rustc +nightly test.rs
error: this arithmetic operation will overflow
 --> test.rs:6:18
  |
6 |             dbg!(max + 1);
  |                  ^^^^^^^ attempt to compute `usize::MAX + 1_usize`, which would overflow
  |
  = note: `#[deny(arithmetic_overflow)]` on by default

This doesn't error if the Equal | Less is changed to _, or if the match is changed to if let.

I worry that this is a weakness in determining what is unreachable, especially if we were to expand that scenario to something that is not statically unreachable, only realized at runtime.

[cjgillot]

@cuviper the existing lint does not try to call u64::cmp, so it conservatively assumes that it can return anything, so that the 3 branches are reachable.

Removing false positives from this lint is inherently limited by the halting problem, and pragmatically by the resources we accept to put in the symbolic execution.

I don't think keeping this bug open is very useful, except to redirect possible duplicates. I fixed the previous instance because the 'if false' can realistically happen with consts. There are still 1001 variations that still have the false positive.

Should we suggest to allow the lint?

[cuviper]

IMO, all of this symbolic execution should still behave as-if it were executed at runtime. Introducing a compile-time error is going too far, and worse it doesn't respect -Coverflow-checks either, where it might not have even been a runtime error even if it were totally reachable.

But I have no idea how hard it would be to implement as-if behavior here. Has the current behavior already been discussed and decided, or should we nominate this for compiler discussion?

Should we suggest to allow the lint?

Do you mean suggest to the maintainer of the num crates? That's me, so we don't need to take any further action than this issue. If the change stands, I can and will adapt as necessary.

If you mean in general, having the compiler suggest to allow its own lint, I think that would be a bad outcome. If there are 1001 variations of false positives in a deny-by-default lint, we have a problem.

[cuviper]

BTW, I noticed that each time I try one of these failing tests, it's leaving temporary *.rcgu.o files around. That may have nothing to do with your changes, but it seems like this lint is not cleaning up properly. Should I file another issue?

[pnkfelix]

Discussed briefly in T-compiler triage meeting, but we did not really make progress on trying to help towards a decision here...

[wesleywiser]

Nominating as I think we should continue discussion. I guess one possible outcome would be documenting a philosophy like @jyn514 mentioned which we could include in the error message to help users understand if they run into it:

personally I think we should not treat changes like this as breaking, the code is incorrect even if it's unreachable

[cuviper]

"If a tree falls in a forest and no one is around to hear it, does it make a sound?" Except in this case, we're talking about its potential to fall, and complaining about the sound it would make...

[jyn514]

Yes, I don't think we should try to warn on unreachable code, it seems ok to silence the lint there. I just don't think we should treat it as a regression and breaking change. This is a lint and not a hard error for a reason.

[cuviper]

While it may be decided that this is not considered a regression, for now this has reached beta, so:

@rustbot label -regression-from-stable-to-nightly +regression-from-stable-to-beta

[apiraino]

T-compiler visited this in two meetings on Zulip, posting here the notes for reference: meeting #1 and meeting #2 - we'll need to circle back on the question when a problem with a lint like this is a bug vs when it is just quality of life improvement.

[apiraino]

Steering meeting scheduled for discussion: rust-lang/compiler-team#627

@rustbot label -I-compiler-nominated

[pnkfelix]

We had the rust-lang/compiler-team#627 meeting, but this wasn't completely settled, at least in terms of whether we need to take action for 1.70 or if we're going to let it ship as is. See https://rust-lang.zulipchat.com/#narrow/stream/238009-t-compiler.2Fmeetings/topic/.5Bsteering.5D.202023-05-12.20lint.20bugs.20vs.20QOI.20compiler-team.23627/near/357903013

@rustbot label +I-compiler-nominated

[pnkfelix]

based on further discussion in today's triage meeting and follow-up chat between Felix and Wesley (as well as follow-up on last week's steering meeting), we're going to let this ride the trains, which means we can remove the nomination label.

@rustbot label: -I-compiler-nominated