improve [cast_sign_loss], to skip warning on always positive expressions
#11883
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
r? @dswij (rustbot has picked a reviewer for you, use r? to override) |
rustbot
added
the
S-waiting-on-review
J-ZhengLi
force-pushed
the
issue11642
branch
from
4eb16d8 to
51fdafb
Compare
dswij
reviewed
Dec 13, 2023
J-ZhengLi
force-pushed
the
issue11642
branch
from
51fdafb to
eae2317
Compare
… that is always positive
Yup, lol |
|
Is this ready for a new review? |
dswij
approved these changes
Jan 6, 2024
|
@bors r+ |
|
☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test |
bors
added a commit
that referenced
this pull request
Feb 27, 2024
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.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
fixes: #11642
changelog: improve [
cast_sign_loss] to skip warning on always positive expressionsTurns out this is change became quite big, and I still can't cover all the cases, like method calls such as
POSITIVE_NUM.mul(POSITIVE_NUM), orNEGATIVE_NUM.div(NEGATIVE_NUM)... but well, if I do, I'm scared that this will goes forever, so I stopped, unless it needs to be done, lol.