arithmetic_overflow lint not triggered for overflowing shifts in promoteds

[cchudant]

I tried this code:

format_args!("{}", 1 << 32)

I expected to see this happen:

Same output as

let a = 1 << 32;
format_args!("{}", a)

which would be

error: this arithmetic operation will overflow
 --> src/main.rs:3:9
  |
3 | let a = 1 << 32;
  |         ^^^^^^^ attempt to shift left by `32_i32`, which would overflow
  |
  = note: `#[deny(arithmetic_overflow)]` on by default

Instead, this happened:

The code compiles with no error, the lint is not produced.
When running it, it overflows: panics at debug mode, and gives the wrong output in release.

Meta

Tested with the current stable, nightly and beta from the playground.

I have tried finding whether this has already been reported, it seems it hasn't

[clubby789]

I think the issue here is that the format_args macro implicitly creates a reference to its arguments. &(1 << 32) gets promoted to a constant, and the MIR visitor which emits this lint doesn't seem to be traversing into the constant.

fn main() {
    let _a = &(1 << 32);
}

has the same issue

[gurry]

@rustbot claim

[gurry]

The issue replicates with any code that is run through const eval, promoted or not. The following snippet doesn't show the lint either:

fn main() {
    let _x = [3; { 
        1 << 32
    }];
}

Perhaps its because the lint is simply not invoked during const eval MIR.

Edit
Ignore the above. No overflow actually occurs in that code since 1_usize << 32_i32 cannot overflow on a 64 bit machine. It occurs with the following code and in that case the lint does fire:

fn main() {
    let _x = [3; { 
        1 << 64
    }];
}

So this issue is only with promoted consts.

[gurry]

The MIR visitor @clubby789 mentioned above is ConstPropLint. It is responsible for emitting the overflow error. However ConstPropLint::run_lint() has the following check right at the beginning:

rust/compiler/rustc_mir_transform/src/const_prop_lint.rs

Lines 45 to 48 in 33f6af8

	// will be evaluated by miri and produce its errors there
	if body.source.promoted.is_some() {
	return;
	}

Due to it the lint doesn't do anything for promoted MIR bodies. Removing the check fixes the issue and the expected overflow error now appears. So indeed the check is a direct cause of the issue.

Further, note the comment on top of the check. It says the lint doesn't need to fire for promoteds because they are checked during MIR interpretation anyway. However, that may not always happen. In our case, for example, the MIR emitted is as below:

promoted[0] in main: &i32 = {
    let mut _0: &i32;
    let mut _1: i32;

    bb0: {
        _1 = Shl(const 1_i32, const 32_i32);
        _0 = &_1;
        return;
    }
}

As you can see there is no assert statement following the Shl statement. As a result, no error will be emitted during interpretation.

I guess the reason for the missing assert is that we want to be conservative when it comes to promoteds as explained here: https://github.com/rust-lang/const-eval/blob/master/promotion.md#promotion-and-fallability-of-const-evaluation.

Anyway, given all of the above I see two options for solving the issue:

1. Remove the check in ConstPropLint::run_lint() or
2. Add an assert after the Shl during MIR generation (this should be fine from the fallibility of promoteds perspective as the overflowing Shl would have failed to compile even if it weren't promoted)

Being super new to this area, I'm not sure which one is better and will cause less collateral damage. Someone with experience in this area please advise.

[saethlin]

The example in the issue description now produces an error about arithmetic overflow. Other variants do not: https://godbolt.org/z/4b8seYe9d

[gurry]

Before we can fix this issue, we need to understand the desired behaviour with regard to promoteds. As per this doc and the discussion in #74696 we don't want overflowing promoteds in dead code to emit an error. However, in the present issue the overflowing promoted occurs in non-dead code and therefore it should ideally emit an error. Although, that in turn conflicts with the expectation in this test case

rust/tests/ui/consts/promotion.rs

Lines 28 to 30 in 84a554c

	// make sure that these do not cause trouble despite overflowing
	assert_static(&(0-1));
	assert_static(&-i32::MIN);

where we expect even non-dead code to not result in an error (the calls to assert_static() here are not dead code). So I'm a bit confused as to what the expected behaviour is.

@RalfJung can you please weigh in?

[RalfJung]

I'm not entirely sure what is being asked. The key point is that the following programs must behave the same way:

if b {
  let _val = 1 << 32;
}

and

if b {
  let _val = &(1 << 32);
}

Replacing b by true or false (in both program consistently) should also still lead to a pair of programs that emit the same diagnostics.

In particular, all of these programs must built successfully with --cap-lints, i.e. they may trigger lints, even err-by-default lints, but not hard errors.

The docs you link are about not making the second program (the one with promotion) a hard error. That does not exclude the possibility of an err-by-default lint, those are very different. If the arithmetic_overflow lint is err-by-default (I don't remember right now whether it is), it should trigger consistently inside and outside of promoteds.

Doing this correctly, and consistently across builds with and without debug assertions, is tricky due to the way we generate MIR. For instance we had issues with duplicate lints being emitted for a long time. Seems like right now we have the opposite issue where the lint is not firing often enough.

[RalfJung]

Interestingly, this does lint as it should, both with and without the &:

pub fn foo(b: bool) {
    if b {
      let _val = &(3u8 + 255u8);
    }
}

I don't know where that lint comes from, if it's not const_prop_lint.