Fix sign-handling bugs and false negatives in cast_sign_loss

[teor2345]

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:

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):

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:

((-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, and if the lint handles unary negation, these should all lint:

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, 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:

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.

[rustbot]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @llogiq (or someone else) soon.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

• @rustbot author: the review is finished, PR author should check the comments and take action accordingly
• @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

[teor2345]

I've finished working on the code and the existing tests should pass. I still need to add new tests.

[teor2345]

Ok, the tests should be finished now too!

[teor2345]

@rustbot review

[teor2345]

Anyone should feel free to move this PR forward, I might not see notifications from reviewers.

[rustbot]

There are merge commits (commits with multiple parents) in your changes. We have a no merge policy so these commits will need to be removed for this pull request to be merged.

You can start a rebase with the following commands:

$ # rebase
$ git rebase -i master
$ # delete any merge commits in the editor that appears
$ git push --force-with-lease

The following commits are merge commits:

• ede7a3f

[bors]

☔ The latest upstream changes (presumably #12306) made this pull request unmergeable. Please resolve the merge conflicts.

[teor2345]

I re-blessed the tests.

[llogiq]

We don't need an uncertain count. In fact, whenever we encounter any uncertain sign, we can return Uncertain right away in this method.

[teor2345]

Thanks, I removed this and the uncertain_count in the add method in commit 1e3c55e

[llogiq]

Apart from a small nit in the code, this makes sense to me. r=me with the nit fixed.

[teor2345]

@rustbot review

The nit is fixed, r=llogiq

[llogiq]

Thank you!

@bors r+

[bors]

📌 Commit 1e3c55e has been approved by llogiq

It is now in the queue for this repository.

[bors]

⌛ Testing commit 1e3c55e with merge e33cba5...

[bors]

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: llogiq
Pushing e33cba5 to master...