Lint overflowing integer casts in const prop

[wesleywiser]

This extends the invalid cases we catch in const prop to include
overflowing integer casts using the same machinery as the overflowing
binary and unary operation logic.

r? @oli-obk

[wesleywiser]

Force pushed with new error message

[wesleywiser]

Blessed tests

[oli-obk]

r=me with a comment about the weird zst situation added

[wesleywiser]

@bors r=oli-bok

[bors]

📌 Commit d2eddfe7d139051df4ad569224b5d97dee395698 has been approved by oli-bok

[wesleywiser]

Rebased

@bors r=oli-obk

[bors]

📌 Commit 001cea4 has been approved by oli-obk

[Dylan-DPC-zz]

Failed in rollup

@bors r-

[bors]

⌛ Testing commit e8c1c4c with merge 0ec3706...

[bors]

☀️ Test successful - checks-azure
Approved by: oli-obk
Pushing 0ec3706 to master...

[rust-highfive]

📣 Toolstate changed by #67676!

Tested on commit 0ec3706.
Direct link to PR: #67676

💔 miri on windows: test-pass → test-fail (cc @oli-obk @eddyb @RalfJung, @rust-lang/infra).
💔 miri on linux: test-pass → test-fail (cc @oli-obk @eddyb @RalfJung, @rust-lang/infra).

[RalfJung]

To me it looks like the lint is handling enum variants incorrectly:

enum Signed {
    Bar = -42,
    Baz,
    Quux = 100,
}
fn signed() -> [i8; 3] {
    let baz = Signed::Baz; // let-expansion changes the MIR significantly
    [Signed::Bar as i8, baz as i8, Signed::Quux as i8]
}

This errors at Signed::Bar as i8, but why would it?

Cc https://github.com/rust-lang/miri/pull/1138/files#r362443894

[wesleywiser]

@RalfJung Yeah, you're totally correct. I'm surprised we don't have any tests that exercise this code in rustc for enums with unsigned values. I'm investigating a fix or potentially a revert for this PR since I'm on vacation next week.

[SimonSapin]

The premise of this PR seems flawed. Truncating is in some cases the legitimate desired behavior, and as is the only way available (so far) to convert to a smaller integer type.

Compare with overflow checking in +, where wrapping_add has existed since before Rust 1.0, possibly for as long as this overflow checking.

[wesleywiser]

@SimonSapin The lint does not fire on every instance of as. The intent is to only lint situations where the as cast results in a different value, not just a smaller integer type.

To draw a comparison with the wrapping_add function, if the intent is to truly discard the higher-order bits, masking them off first will not cause the lint to fire:

    let x = 1026u16 as u8; // lint fires here
    let y = (1026u16 & 0x00FFu16) as u8; // lint does not fire here

[SimonSapin]

(1026u16 & 0x00FFu16) as u8 is a work-around designed specifically for this lint. 1026u16 as u8 has been the idiomatic way to truncate for many years. Making it an error an telling everyone to just use the work-around doesn’t seem a good approach to me, given Rust’s stability goal.

It is unfortunate that there is no way to tell intentional truncation apart from accidental truncation, and https://internals.rust-lang.org/t/pre-rfc-add-explicitly-named-numeric-conversion-apis/11395 tries to fix that, but it is a fact of today’s Rust.

[oli-obk]

It's not an error and not a stability problem because it's just a lint that you can turn off (even locally). I suggest you disable the lint for your code instead of using a workaround.

[SimonSapin]

https://perf.rust-lang.org/status.html currently shows:

Benchmarks for last commit:

SHA: 0a58f5864659ddfe1d95c122abaa75c88220aed0, date: 2020-01-02T10:20:09+00:00

[…]

error: truncating cast: the value 4294967292 requires 32 bits but the target type is only 16 bits
     --> /tmp/.tmpmCOmcQ/target/debug/build/style-f46d78432390fdd0/out/gecko_properties.rs:10050:21
      |
10050 |                     structs::NS_FONT_STRETCH_ULTRA_CONDENSED as i16,
      |                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
      |
      = note: `#[deny(const_err)]` on by default

(I don’t know if there is a permanent URL for this log.)

https://raw.githubusercontent.com/rust-lang-nursery/rustc-perf/06e8bd4fb5648f6c8c03265150b1d50c529415e1/collector/benchmarks/style-servo/components/style/gecko/generated/structs.rs defines:

    pub const NS_FONT_STRETCH_ULTRA_CONDENSED: ::std::os::raw::c_int = -4;

However I didn’t manage to reproduce the error with this reduction:

pub const A: ::std::os::raw::c_int = -4;
pub const B: i16 = A as i16;

fn main() {
   dbg!(B);
}

and

rustup-toolchain-install-master 0ec370670220b712b042ee09aab067ec7e5878d5
rustc +0ec370670220b712b042ee09aab067ec7e5878d5 a.rs

It's not an error and not a stability problem because it's just a lint that you can turn off (even locally). I suggest you disable the lint for your code instead of using a workaround.

The above shows there’s at least one case in the wild of previously-valid code where this lint triggers. This suggests that Crater is needed to determine how much of a stability issue it is to make this lint deny by default.

[lqd]

This may be a case similar to the bug Ralf pointed out earlier

[oli-obk]

Yes, the lint has a signedness bug

[SimonSapin]

Two separate things:

1. The lint as implemented is buggy. Unless a fix can be ready very soon, please consider reverting for now.

   fn main() {
       let _ = -1_i16 as i8;
   }

   error: truncating cast: the value 65535 requires 16 bits but the target type is only 8 bits
    --> a.rs:2:13
     |
   2 |     let _ = -1_i16 as i8;
     |             ^^^^^^^^^^^^
     |
     = note: `#[deny(const_err)]` on by default

2. Even with that bug fixed, I believe that making it deny by default is not acceptable under our stability policy since it triggers for a valid idiomatic pattern.

   Even making it warn by default feels dubious in my opinion. I’d prefer to see an alternative available for intentional truncation (better than manually masking), and an evaluation (perhaps with Crater?) of negative impact of churn in cases of intentional truncation v.s. positive impact in cases the lint catches real bugs.

[wesleywiser]

1. I'm going to revert the lint tonight.

2. I'm not sure I see how 256u16 as u8 is any less "wrong" than, for example, 255u8 + 1u8. This seems like exactly the same category of issue to me. It would be helpful to have an example of code which is triggering this lint (modulo the bug with negative numbers) which you believe should be allowed.

[SimonSapin]

Thank you for taking care of the revert.

Intentional cases are not gonna look like literally 256u16 as u8. Consider for example:

const BITS: u16 = 0x1234;
let upper = (BITS >> 8) as u8;
let lower = BITS as u8;

[oli-obk]

it triggers for a valid idiomatic pattern.

I disagree that value as u8 is an idiomatic way to truncate (and some ppl seem to think it should in fact panic: https://users.rust-lang.org/t/safe-casting-of-primitives/24240). Also in your example, you should be using to_be_bytes to get the components if you want the u8 components. Ok, maybe your real example truncates u32 to u16, but that discussion is not relevant here imo. The relevant point here is twofold. First:

I’d prefer to see an alternative available for intentional truncation (better than manually masking),

Something like Into and TryInto for explicitly casting losslessly? So rust-lang/rfcs#2484

negative impact of churn in cases of intentional truncation v.s. positive impact in cases the lint catches real bugs.

That seems nearly impossible to do. Just like it's super hard to really prove with data that using Into over as will catch accidental changes to types that would have made the as cast lossy.

Though since we are doing that in clippy, if we had a LossyInto, clippy could also take care of complaining about any kind of integral as cast instead of just the few it does complain about right now.

Second: So... while there are ways to move forward for general truncation casts, the situation we have here is that we statically know that you are truncating a value. We are allowed to add more lints for statically knowing that the user is doing something buggy as per RFC 1229. The example from that RFC is

let x = 5u32 << 42;

which actually used to be a hard error and we weakened it to the const_err lint for consistency.

That example from the RFC is in fact also a truncation causing the bitshift to be 5u32 << 10 because the higher bits are subtracted. I'm sure someone has a use case for using a constant instead of the 10 and the silent trucation working. They can allow the lint and keep doing that. The differences to your example are that

1. There's no way to truncate an int other than as casting
2. The bitshift is also checked with debug assertions at runtime in debug mode
   • we also emit the lint since 1.0 in release mode for unary negation overflow, binary math overflow and bit shift truncation
3. The lint about it did not exist since 1.0
   • this point by itself in isolation is moot due to RFC 1227

So the big difference between this new trigger of the lint is that it's not triggering on behaviour that panics at runtime with debug assertions.

Which brings us back to the question "should as panic on truncation?". I don't have an answer to that, but since you can in fact mask and then cast without this causing worse assembly, I think we'd be able to do this.

For now, let's leave such lints to clippy until we have a way to truncate via LossyInto.

[SimonSapin]

That seems nearly impossible to do. Just like it's super hard to really prove with data that using Into over as will catch accidental changes to types that would have made the as cast lossy.

Yes, and we’re not making lossless as a deny-by-default lint in rustc.

For now, let's leave such lints to clippy until we have a way to truncate via LossyInto.

This is pretty much my point, but not what this PR does.