mirror of
https://github.com/rust-lang/rust-clippy
synced 2024-11-23 21:23:56 +00:00
e33cba523a
Fix sign-handling bugs and false negatives in `cast_sign_loss` **Note: anyone should feel free to move this PR forward, I might not see notifications from reviewers.** changelog: [`cast_sign_loss`]: Fix sign-handling bugs and false negatives This PR fixes some arithmetic bugs and false negatives in PR #11883 (and maybe earlier PRs). Cc `@J-ZhengLi` I haven't updated the tests yet. I was hoping for some initial feedback before adding tests to cover the cases listed below. Here are the issues I've attempted to fix: #### `abs()` can return a negative value in release builds Example: ```rust i32::MIN.abs() ``` https://play.rust-lang.org/?version=stable&mode=release&edition=2021&gist=022d200f9ef6ee72f629c0c9c1af11b8 Docs: https://doc.rust-lang.org/std/primitive.i32.html#method.abs Other overflows that produce negative values could cause false negatives (and underflows could produce false positives), but they're harder to detect. #### Values with uncertain signs can be positive or negative Any number of values with uncertain signs cause the whole expression to have an uncertain sign, because an uncertain sign can be positive or negative. Example (from UI tests): ```rust fn main() { foo(a: i32, b: i32, c: i32) -> u32 { (a * b * c * c) as u32 //~^ ERROR: casting `i32` to `u32` may lose the sign of the value } println!("{}", foo(1, -1, 1)); } ``` https://play.rust-lang.org/?version=nightly&mode=debug&edition=2021&gist=165d2e2676ee8343b1b9fe60db32aadd #### Handle `expect()` the same way as `unwrap()` Since we're ignoring `unwrap()` we might as well do the same with `expect()`. This doesn't seem to have tests but I'm happy to add some like `Some(existing_test).unwrap() as u32`. #### A negative base to an odd exponent is guaranteed to be negative An integer `pow()`'s sign is only uncertain when its operants are uncertain. (Ignoring overflow.) Example: ```rust ((-2_i32).pow(3) * -2) as u32 ``` This offsets some of the false positives created by one or more uncertain signs producing an uncertain sign. (Rather than just an odd number of uncertain signs.) #### Both sides of a multiply or divide should be peeled recursively I'm not sure why the lhs was peeled recursively, and the rhs was left intact. But the sign of any sequence of multiplies and divides is determined by the signs of its operands. (Ignoring overflow.) I'm not sure what to use as an example here, because most expressions I want to use are const-evaluable. But if `p()` is [a non-const function that returns a positive value](https://doc.rust-lang.org/std/primitive.i32.html#method.isqrt), and if the lint handles unary negation, these should all lint: ```rust fn peel_all(x: i32) { (-p(x) * -p(x) * -p(x)) as u32; ((-p(x) * -p(x)) * -p(x)) as u32; (-p(x) * (-p(x) * -p(x))) as u32; } ``` #### The right hand side of a Rem doesn't change the sign Unlike Mul and Div, > Given remainder = dividend % divisor, the remainder will have the same sign as the dividend. https://doc.rust-lang.org/reference/expressions/operator-expr.html#arithmetic-and-logical-binary-operators I'm not sure what to use as an example here, because most expressions I want to use are const-evaluable. But if `p()` is [a non-const function that returns a positive value](https://doc.rust-lang.org/std/primitive.i32.html#method.isqrt), and if the lint handles unary negation, only the first six expressions should lint. The expressions that start with a constant should lint (or not lint) regardless of whether the lint supports `p()` or unary negation, because only the dividend's sign matters. Example: ```rust fn rem_lhs(x: i32) { (-p(x) % -1) as u32; (-p(x) % 1) as u32; (-1 % -p(x)) as u32; (-1 % p(x)) as u32; (-1 % -x) as u32; (-1 % x) as u32; // These shouldn't lint: (p(x) % -1) as u32; (p(x) % 1) as u32; (1 % -p(x)) as u32; (1 % p(x)) as u32; (1 % -x) as u32; (1 % x) as u32; } ``` #### There's no need to bail on other expressions When peeling, any other operators or expressions can be left intact and sent to the constant evaluator. If these expressions can be evaluated, this offsets some of the false positives created by one or more uncertain signs producing an uncertain sign. If not, they end up marked as having uncertain sign. |
||
|---|---|---|
| .. | ||
| src | ||
| Cargo.toml | ||
| README.md | ||