Fix uncertain sign and remainder op handling in cast_sign_loss.rs

1 files changed, 60 insertions, 30 deletions

diff --git a/clippy_lints/src/casts/cast_sign_loss.rs b/clippy_lints/src/casts/cast_sign_loss.rs
index 1df5a25f674..1b907d82570 100644
--- a/clippy_lints/src/casts/cast_sign_loss.rs
+++ b/clippy_lints/src/casts/cast_sign_loss.rs
@@ -7,7 +7,12 @@ use rustc_middle::ty::{self, Ty, UintTy};
 
 use super::CAST_SIGN_LOSS;
 
-const METHODS_RET_POSITIVE: &[&str] = &["abs", "checked_abs", "rem_euclid", "checked_rem_euclid"];
+/// A list of methods that can never return a negative value.
+/// Includes methods that panic rather than returning a negative value.
+///
+/// Methods that can overflow and return a negative value must not be included in this list,
+/// because checking for negative return values from those functions can be useful.
+const METHODS_RET_POSITIVE: &[&str] = &["checked_abs", "rem_euclid", "checked_rem_euclid"];
 
 pub(super) fn check(cx: &LateContext<'_>, expr: &Expr<'_>, cast_op: &Expr<'_>, cast_from: Ty<'_>, cast_to: Ty<'_>) {
     if should_lint(cx, cast_op, cast_from, cast_to) {
@@ -27,13 +32,15 @@ fn should_lint(cx: &LateContext<'_>, cast_op: &Expr<'_>, cast_from: Ty<'_>, cast
                 return false;
             }
 
-            // Don't lint if `cast_op` is known to be positive.
+            // Don't lint if `cast_op` is known to be positive, ignoring overflow.
             if let Sign::ZeroOrPositive = expr_sign(cx, cast_op, cast_from) {
                 return false;
             }
 
             let (mut uncertain_count, mut negative_count) = (0, 0);
-            // Peel off possible binary expressions, e.g. x * x * y => [x, x, y]
+            // Peel off possible binary expressions, for example:
+            // x * x * y => [x, x, y]
+            // a % b => [a]
             let Some(exprs) = exprs_with_selected_binop_peeled(cast_op) else {
                 // Assume cast sign lose if we cannot determine the sign of `cast_op`
                 return true;
@@ -47,8 +54,9 @@ fn should_lint(cx: &LateContext<'_>, cast_op: &Expr<'_>, cast_from: Ty<'_>, cast
                 };
             }
 
-            // Lint if there are odd number of uncertain or negative results
-            uncertain_count % 2 == 1 || negative_count % 2 == 1
+            // Lint if there are any uncertain results (because they could be negative or positive),
+            // or an odd number of negative results.
+            uncertain_count > 0 || negative_count % 2 == 1
         },
 
         (false, true) => !cast_to.is_signed(),
@@ -87,6 +95,12 @@ fn expr_sign(cx: &LateContext<'_>, expr: &Expr<'_>, ty: Ty<'_>) -> Sign {
         {
             method_name = inner_path.ident.name.as_str();
         }
+        if method_name == "expect"
+            && let Some(arglist) = method_chain_args(expr, &["expect"])
+            && let ExprKind::MethodCall(inner_path, ..) = &arglist[0].0.kind
+        {
+            method_name = inner_path.ident.name.as_str();
+        }
 
         if method_name == "pow"
             && let [arg] = args
@@ -100,53 +114,69 @@ fn expr_sign(cx: &LateContext<'_>, expr: &Expr<'_>, ty: Ty<'_>) -> Sign {
     Sign::Uncertain
 }
 
-/// Return the sign of the `pow` call's result.
+/// Return the sign of the `pow` call's result, ignoring overflow.
+///
+/// If the base is positive, the result is always positive.
+/// If the base is negative, and the exponent is a even number, the result is always positive,
+/// otherwise if the exponent is an odd number, the result is always negative.
 ///
-/// If the caller is a positive number, the result is always positive,
-/// If the `power_of` is a even number, the result is always positive as well,
-/// Otherwise a [`Sign::Uncertain`] will be returned.
+/// If either value can't be evaluated, [`Sign::Uncertain`] will be returned.
 fn pow_call_result_sign(cx: &LateContext<'_>, caller: &Expr<'_>, power_of: &Expr<'_>) -> Sign {
     let caller_ty = cx.typeck_results().expr_ty(caller);
-    if let Some(caller_val) = get_const_int_eval(cx, caller, caller_ty)
-        && caller_val >= 0
-    {
+    let Some(caller_val) = get_const_int_eval(cx, caller, caller_ty) else {
+        return Sign::Uncertain;
+    }
+    // Non-negative values raised to non-negative exponents are always non-negative, ignoring overflow.
+    // (Rust's integer pow() function takes an unsigned exponent.)
+    if caller_val >= 0 {
         return Sign::ZeroOrPositive;
     }
 
-    if let Some(Constant::Int(n)) = constant(cx, cx.typeck_results(), power_of)
-        && clip(cx.tcx, n, UintTy::U32) % 2 == 0
-    {
+    let Some(Constant::Int(n)) = constant(cx, cx.typeck_results(), power_of) else {
+        return Sign::Uncertain;
+    }
+    // A negative value raised to an even exponent is non-negative, and an odd exponent
+    // is negative, ignoring overflow.
+    if clip(cx.tcx, n, UintTy::U32) % 2 == 0 0 {
         return Sign::ZeroOrPositive;
+    } else {
+        return Sign::Negative;
     }
-
-    Sign::Uncertain
 }
 
 /// Peels binary operators such as [`BinOpKind::Mul`], [`BinOpKind::Div`] or [`BinOpKind::Rem`],
-/// which the result could always be positive under certain condition.
+/// which the result could always be positive under certain conditions, ignoring overflow.
 ///
-/// Other operators such as `+`/`-` causing the result's sign hard to determine, which we will
-/// return `None`
-fn exprs_with_selected_binop_peeled<'a>(expr: &'a Expr<'_>) -> Option<Vec<&'a Expr<'a>>> {
+/// Expressions using other operators are preserved, so we can try to evaluate them later.
+fn exprs_with_selected_binop_peeled<'a>(expr: &'a Expr<'_>) -> Vec<&'a Expr<'a>> {
     #[inline]
-    fn collect_operands<'a>(expr: &'a Expr<'a>, operands: &mut Vec<&'a Expr<'a>>) -> Option<()> {
+    fn collect_operands<'a>(expr: &'a Expr<'a>, operands: &mut Vec<&'a Expr<'a>>) {
         match expr.kind {
             ExprKind::Binary(op, lhs, rhs) => {
-                if matches!(op.node, BinOpKind::Mul | BinOpKind::Div | BinOpKind::Rem) {
+                if matches!(op.node, BinOpKind::Mul | BinOpKind::Div) {
+                    // For binary operators which both contribute to the sign of the result,
+                    // collect all their operands, recursively. This ignores overflow.
+                    collect_operands(lhs, operands);
+                    collect_operands(rhs, operands);
+                } else if matches!(op.node, BinOpKind::Rem) {
+                    // For binary operators where the left hand side determines the sign of the result,
+                    // only collect that side, recursively. Overflow panics, so this always holds.
+                    //
+                    // > 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
                     collect_operands(lhs, operands);
-                    operands.push(rhs);
                 } else {
-                    // Things are complicated when there are other binary ops exist,
-                    // abort checking by returning `None` for now.
-                    return None;
+                    // The sign of the result of other binary operators depends on the values of the operands,
+                    // so try to evaluate the expression.
+                    operands.push(expr);
                 }
             },
+            // For other expressions, including unary operators and constants, try to evaluate the expression.
             _ => operands.push(expr),
         }
-        Some(())
     }
 
     let mut res = vec![];
-    collect_operands(expr, &mut res)?;
-    Some(res)
+    collect_operands(expr, &mut res);
+    res
 }