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.

[gurry]

Thanks for the input @RalfJung .

I tested the various types of overflow using the following code:

fn main() {
    let flag = true;
    // let flag = false;
    if flag {
        let _shl = 1 << 32; 
        let _shl_prom = &(1 << 32); 

        let _shr = 1 >> 32; 
        let _shr_prom = &(1 >> 32); 

        let _sum = 3u8 + 255u8;
        let _sum_prom = &(3u8 + 255u8);

        let _dif = 0u8 - 1u8;
        let _dif_prom = &(0u8 - 1u8);

        let _ele = [1, 2, 3][4];
        let _ele_prom = &([1, 2, 3][4]);
    }
}

When flag=false the compilation succeeds with no diagnostics emitted whatsoever.

When flag=true we get this output:

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

error: this arithmetic operation will overflow
 --> src/main.rs:8:20
  |
8 |         let _shr = 1 >> 32;
  |                    ^^^^^^^ attempt to shift right by `32_i32`, which would overflow

error: this arithmetic operation will overflow
  --> src/main.rs:11:20
   |
11 |         let _sum = 3u8 + 255u8;
   |                    ^^^^^^^^^^^ attempt to compute `3_u8 + u8::MAX`, which would overflow        

error: this arithmetic operation will overflow
  --> src/main.rs:12:26
   |
12 |         let _sum_prom = &(3u8 + 255u8);
   |                          ^^^^^^^^^^^^^ attempt to compute `3_u8 + u8::MAX`, which would overflow

error: this arithmetic operation will overflow
  --> src/main.rs:14:20
   |
14 |         let _dif = 0u8 - 1u8;
   |                    ^^^^^^^^^ attempt to compute `0_u8 - 1_u8`, which would overflow

error: this arithmetic operation will overflow
  --> src/main.rs:15:26
   |
15 |         let _dif_prom = &(0u8 - 1u8);
   |                          ^^^^^^^^^^^ attempt to compute `0_u8 - 1_u8`, which would overflow     

error: this operation will panic at runtime
  --> src/main.rs:17:20
   |
17 |         let _ele = [1, 2, 3][4];
   |                    ^^^^^^^^^^^^ index out of bounds: the length is 3 but the index is 4
   |
   = note: `#[deny(unconditional_panic)]` on by default

error: this operation will panic at runtime
  --> src/main.rs:18:26
   |
18 |         let _ele_prom = &([1, 2, 3][4]);
   |                          ^^^^^^^^^^^^^^ index out of bounds: the length is 3 but the index is 4 

error: aborting due to 8 previous errors

As you can see, for shl and shr no diagnostics are emitted for promoteds but they are emitted for normal consts. With all other types of overflow the same diagnotics are emitted both for promoteds and consts.

So clearly there's an inconsistency between promoteds and consts diagnostics only for shl and shr overflows.

The above observations are for opt-level=0 (i.e. debug build). I also tested with opt-level=3 in which case the build succeeded with no diagnostics emitted whatsoever regardless of the value of flag, which is what is expected.

Metadata:

rustc 1.72.1 (d5c2e9c34 2023-09-13)
binary: rustc
commit-hash: d5c2e9c342b358556da91d61ed4133f6f50fc0c3
commit-date: 2023-09-13
host: x86_64-pc-windows-msvc
release: 1.72.1
LLVM version: 16.0.5

Given the above, and @RalfJung's comments that the diagnostics between normal consts and promoteds must be identical, we need to fix the issue for shl and shr.

[RalfJung]

The dependency on the value of flag and the optimization level is unfortunate but somewhat expected. If we want to track this that should be a separate issue.

But for the shifts, I agree, we should be consistent there.

[gurry]

---

Regression in nightly-2023-03-17

---

found 8 bors merge commits in the specified range
commit[0] 2023-03-15: Auto merge of #109169 - bjorn3:sync_cg_clif-2023-03-15, r=bjorn3
commit[1] 2023-03-15: Auto merge of #108282 - cjgillot:mir-checked-sh, r=tmiasko
commit[2] 2023-03-16: Auto merge of #109183 - lqd:revert-107376, r=compiler-errors
commit[3] 2023-03-16: Auto merge of #108809 - lqd:fix-ignore, r=pietroalbini
commit[4] 2023-03-16: Auto merge of #109206 - matthiaskrgr:rollup-oev8ax6, r=matthiaskrgr
commit[5] 2023-03-16: Auto merge of #106824 - m-ou-se:format-args-flatten, r=oli-obk
commit[6] 2023-03-16: Auto merge of #107270 - cjgillot:remove-zst, r=oli-obk
commit[7] 2023-03-16: Auto merge of #108944 - cjgillot:clear-local-info, r=oli-obk
ERROR: no CI builds available between ab65486 and 511364e within last 167 days

[RalfJung]

Probably introduced by #108282 then.

[gurry]

Yes, it is most likely the one.

[gurry]

#108282 is indeed the PR that introduced this issue. But before we go into how it did that, let's understand the mechanics of the issue itself in more detail.

As alluded to above, ConstPropLint is responsible for emitting all overflow lints. It runs only on non-promoted MIR thanks to this check:

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;
	}

In order for the lint to the be emitted, the non-promoted MIR being checked must contain a potentially overflowing op such as Add, CheckedAdd, CheckedSub, Add, Sub, CheckedShl, Shl etc.. If it does not, no lint will be emitted.

Now, before ConstPropLint has a chance to run, PromoteTemps runs and performs promotion. PromoteTemps's behaviour is that if the op being promoted is a non-checked one it removes the op from the original MIR. So if your original MIR contained Shl, Shr (or I guess even Add, Sub) they will be removed. But if it contains CheckedShl, CheckedShr or CheckedAdd etc. they will be left in.

The end result of this is, if your original MIR contained non-checked ops ConstPropLint won't fire for them because they simply won't be found in the MIR.

This is the root cause of the issue.

What #108282 did is that it changed CheckedShl and CheckedShr in the original MIR to Shl and Shr respectively. And given the above, PromoteTemps now started removing them. Consequently, the lint stopped firing for these ops. The PR did not change any other ops like CheckedAdd, CheckedSub etc. so the lint continues to fire for them even today.

[gurry]

So how can we fix this?

The simpler option is to stop PromoteTemps from removing the likes of Shl & Shr from original MIR. This can be easily accomplished. All we need to do is ensure keep_original gets set to true for these ops over here:

rust/compiler/rustc_const_eval/src/transform/promote_consts.rs

Lines 737 to 739 in 3166bbe

	if uses > 1 {
	self.keep_original = true;
	}

.

But I feel that would be a hack. Why should the promotion code be coupled to the requirements of some lint? In fact for efficiency we would not want to leave any redundant ops behind when we have promoted them.

The right solution therefore is to run ConstPropLint on the promoted MIR itself and catch issues there. This can be enabled easily as well by removing this check:

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;
	}

However, if you do that, lints start firing even for dead code (i.e. even when flag is false in the above test code) which is something we don't want.

So I'll dig a bit further and see if we can enable ConstPropLint selectively only on those promoted bodies that do not constitute dead code.

[RalfJung]

With #108282 what used to be CheckedShl turns into multiple operations: an assertion on the right operand, and the actual Shl. Only the Shl gets promoted. The assertion stays in the main MIR body, and that is what ConstPropLint should use on to emit its lint.

The promoted will never emit an error when executed, so it'd not make a lot of sense to make it emit lints either.

[RalfJung]

That said, we do have another bug: if we run your test in release mode, then the warning disappears for all the promoteds (except the array indexing).

If we want to fix that then we have to change this more fundamentally and have the lint check for the arithmetic operation rather than the assertion. And then indeed we have to check in promoteds as well (for regular arithmetic and for shifts).

[gurry]

The promoted will never emit an error when executed, so it'd not make a lot of sense to make it emit lints either.

I was thinking I could make the promoted selectively emit the lint only when it is not under dead code. However, a given promoted could be used at multiple places I presume, some of them dead and some not. So indeed taking that path is a non starter.

The assertion stays in the main MIR body, and that is what ConstPropLint should use on to emit its lint.

Earlier I had thought of predicating the lint on the assert, but dismissed it because an assert could exist in the code for reasons other than overflow as well. I'll see if we can make it work by adding some extra checks or doing some narrow eval in that area.

That said, we do have another bug: if we run your test in release mode, then the warning disappears for all the promoteds (except the array indexing).

If we want to fix that then we have to change this more fundamentally and have the lint check for the arithmetic operation rather than the assertion. And then indeed we have to check in promoteds as well (for regular arithmetic and for shifts).

Yeah, I had an inkling sooner or later we'll need something like this. That's why I too felt a more fundamental change is needed wherein we start linting the promoteds too.

That said, I'll first tackle the immediate issue of shifts in debug builds by using the assert and return to the larger problem later.

[gurry]

However, a given promoted could be used at multiple places I presume

This statement is not true. I was assuming that If we use the exact same expression (like &(1 << 32)) at multiple places in the code, it results in a single promoted body. Turns out it will result in multiple different promoted bodies.

So yeah, we can then be selective about which one we lint and which one we don't depending on whether it originates from under dead code or not.

(Please forgive if this doesn't make sense. I'm still learning about this area.)

[RalfJung]

Earlier I had thought of predicating the lint on the assert, but dismissed it because an assert could exist in the code for reasons other than overflow as well. I'll see if we can make it work by adding some extra checks or doing some narrow eval in that area.

No, the assert is very specific. You can assume it only exists for shift overflow. This is the MIR terminator you are looking for, and you can check the AssertMessage to see if this is for an overflow and which operand.

So yeah, we can then be selective about which one we lint and which one we don't depending on whether it originates from under dead code or not.

I wouldn't worry about dead code. It's okay if the lint fires in dead code.

[gurry]

No, the assert is very specific. You can assume it only exists for shift overflow. This is the MIR terminator you are looking for, and you can check the AssertMessage to see if this is for an overflow and which operand.

Nice. That will work. Thanks for the suggestion.

I wouldn't worry about dead code. It's okay if the lint fires in dead code.

Hmm... That conflicts with the mental model I'd formed so far from your comment that the following two snippets should emit the same diagnostics for the same value of b:

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

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

I had assumed that when b is false, then the code inside both the if blocks is deemed dead code and the lint shouldn't fire in both cases and when b is true then both blocks' contents aren't dead code and the lint should fire. That's how I sorta made it about dead code vs non-dead code.

I'll go through the docs again to clarify my thinking.

Thanks for all your help :)

[RalfJung]

the following two snippets should emit the same diagnostics for the same value of b

I honestly didn't even know that for non-promoteds, we skip the lint in dead code. But we don't even do that consistently, it depends on optimization levels and might change in the future as optimizations get more clever.

TBH I would prefer if we'd emit the exact same lints in debug and release builds, and that probably can only be achieved by always linting everything in dead code. I don't know if this is a consensus position though. However, we certainly don't have a clear decision to not lint in dead code; the fact that that is how it works today is mostly incidental.

So as far as I'm concerned, the proper behavior for your two snippets is to always lint for all values of b. We could try to see if we have t-compiler consensus for that. But meanwhile, if we need to anyway write some new code (analyze promoteds so that we get the lint even if overflow checks are disabled), then I wouldn't worry much about aligning with the completely incidental behavior of const-prop-lint when it comes to dead code.

[gurry]

@RalfJung I've created a draft PR #119339. But I'm not too pleased with it. Not only is it a bit of a hack (which we already knew), it has also started producing a whole lot of duplicate diagnostics. The user won't see them after deduplication, but it's still not pretty.

I was wondering if we can do a proper fix instead. What if we move the lints earlier so that they run right after the MIR is built. Then the lints would see all ops before they are removed/optimized away. Hence they will not only start firing for all promoted consts but also for release builds.

Thoughts?

[gurry]

What if we move the lints earlier so that they run right after the MIR is built.

I tried this by moving the call to ConstPropLint from here:

rust/compiler/rustc_mir_transform/src/lib.rs

Line 535 in f4d794e

&Lint(const_prop_lint::ConstPropLint),

to here:

rust/compiler/rustc_mir_transform/src/lib.rs

Line 311 in f4d794e

],

i.e. to the query mir_const which is located earlier in the call graph. It solved the problem. The lint started firing for all promoted ops not just Shl & Shr and also for both debug and release builds.

However, when I tried to build the std library with this compiler I ran into cycles like this triggered by opaque types:

error[E0391]: cycle detected when computing type of `iter::traits::iterator::iter_compare::compare::{opaque#0}`
    --> library\core\src\iter\traits\iterator.rs:4145:10
     |
4145 |     ) -> impl FnMut(X) -> ControlFlow<ControlFlow<T, Ordering>> + 'a
     |          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
     |
note: ...which requires computing type of opaque `iter::traits::iterator::iter_compare::compare::{opaque#0}`...
    --> library\core\src\iter\traits\iterator.rs:4145:10
     |
4145 |     ) -> impl FnMut(X) -> ControlFlow<ControlFlow<T, Ordering>> + 'a
     |          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
note: ...which requires borrow-checking `iter::traits::iterator::iter_compare::compare`...
    --> library\core\src\iter\traits\iterator.rs:4142:5
     |
4142 | /     fn compare<'a, B, X, T>(
4143 | |         b: &'a mut B,
4144 | |         mut f: impl FnMut(X, B::Item) -> ControlFlow<T> + 'a,
4145 | |     ) -> impl FnMut(X) -> ControlFlow<ControlFlow<T, Ordering>> + 'a
4146 | |     where
4147 | |         B: Iterator,
     | |____________________^
note: ...which requires promoting constants in MIR for `iter::traits::iterator::iter_compare::compare`...
    --> library\core\src\iter\traits\iterator.rs:4142:5
     |
4142 | /     fn compare<'a, B, X, T>(
4143 | |         b: &'a mut B,
4144 | |         mut f: impl FnMut(X, B::Item) -> ControlFlow<T> + 'a,
4145 | |     ) -> impl FnMut(X) -> ControlFlow<ControlFlow<T, Ordering>> + 'a
4146 | |     where
4147 | |         B: Iterator,
     | |____________________^
     = note: ...which requires computing layout of `iter::traits::iterator::iter_compare::compare::{opaque#0}`...
     = note: ...which requires normalizing `iter::traits::iterator::iter_compare::compare::{opaque#0}`...
     = note: ...which again requires computing type of `iter::traits::iterator::iter_compare::compare::{opaque#0}`, completing the cycle
     = note: cycle used when checking effective visibilities
     = note: see https://rustc-dev-guide.rust-lang.org/overview.html#queries and https://rustc-dev-guide.rust-lang.org/query.html for more information

Probably it is because ConstPropLint itself tries to normalize constants and that leads to the cycle. Maybe with a little tweaking of the lint code we can suppress it.

[gurry]

@RalfJung I've created another PR #119432 taking a different approach wherein we simply allow ConstPropLint to run on promoteds in addition to regular MIR. To me this seems to be the most promising. Please take a look.