invalid_upcast_comparisons gives false positive on u8 to i8 wrapping cast

[shssoichiro]

Given this minimal test case:

fn test() {
    let new = 128u8;
    if (new as i8) < 0 {
        println!("greater than 127");
    }
}

Clippy prints the warning:

src/lib.rs:3:8: 3:23 warning: because of the numeric bounds on `new` prior to casting, this expression is always false, #[warn(invalid_upcast_comparisons)] on by default
src/lib.rs:3     if (new as i8) < 0 {
                    ^~~~~~~~~~~~~~~
src/lib.rs:3:8: 3:23 help: for further information visit https://github.com/Manishearth/rust-clippy/wiki#invalid_upcast_comparisons

This is incorrect, as new, if greater than 127, will wrap around to -128 as specified in RFC 560.

[shssoichiro]

I think this may be a flaw in the design of this lint. I see in the tests for this lint:

let zero: u32 = 0;
...
(zero as i32) < -5; //~ERROR because of the numeric bounds on `zero` prior to casting, this expression is always false

So it seems this was intended behavior. However, this isn't a correct warning. A u32 cast to i32 can be less than 0 because of wrapping. (Note that if I change the declaration to let zero: u32 = 4_294_967_200 the test still passes, even though (zero as i32) would be -96)

[mcarton]

I think avoiding implicit wrapping is the point of the lint.

[shssoichiro]

I think the warning message in this case should be different, then. The error message, "this expression is always false", is misleading in this case. (I also think that should be an allow lint by default, because there are cases where this sort of wrapping is desired.)

[shssoichiro]

On the other hand, I guess I can't think of a case where this would be desirable/necessary to use in an if statement/comparison... but I still think the error message may need to be modified for the unsigned-to-signed wraps.

[shssoichiro]

I went back and found the original source that caused this warning:

(s.as_bytes()[index] as i8) >= -0x40

This is equivalent to:

let foo = s.as_bytes()[index];
foo < 0x80 || foo >= 0xC0

So while the latter makes the intention more clear, the unsigned-to-signed wrap appears to be a legitimate optimization that prevents an additional comparison. With this in mind, my opinion is that unsigned-to-signed casting should be moved to a separate, allow-by-default lint.

[oli-obk]

@shssoichiro you can also use a match and see if rustc produces good code:

match s.as_bytes()[index] {
    0...0x7F | 0xC0...0xFF => /* true action */,
    _ => /* false action */
}

[Manishearth]

22:31 < ubsan> warning: because of the numeric bounds on `tmp` prior to casting, this expression is always false, 
               #[warn(invalid_upcast_comparisons)] on by default
22:31 < ubsan> src/main.rs:338     self.r.flags.set_s((tmp as i8) < 0);

we should probably Allow-ify the lint or fix it

[oli-obk]

We do have the cast_possible_wrap lint, so there's no need for this lint to trigger in those cases. Since we now (in the rustup) have a case in clippy itself, I'm fixing it along with the rustup.

[phansch]

This seems to have been fixed: https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=6d192a511daa0151d7afa0eb2f68e536