improve [cast_sign_loss], to skip warning on mathmatical expression that is always positive

J-ZhengLi · J-ZhengLi · commit 51fdafb0594f · 2023-11-27T18:07:02.000+08:00
diff --git a/clippy_lints/src/casts/cast_sign_loss.rs b/clippy_lints/src/casts/cast_sign_loss.rs
@@ -1,12 +1,14 @@
 use clippy_utils::consts::{constant, Constant};
 use clippy_utils::diagnostics::span_lint;
-use clippy_utils::{method_chain_args, sext};
-use rustc_hir::{Expr, ExprKind};
+use clippy_utils::{clip, method_chain_args, sext};
+use rustc_hir::{BinOpKind, Expr, ExprKind};
 use rustc_lint::LateContext;
-use rustc_middle::ty::{self, Ty};
+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"];
+
 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) {
         span_lint(
@@ -18,44 +20,150 @@ pub(super) fn check(cx: &LateContext<'_>, expr: &Expr<'_>, cast_op: &Expr<'_>, c
     }
 }
 
+/// (Remove after final code review)
+/// When does a mathmatic expression always resulting a positive value?
+/// 1. Even number of same variables: `x*x`
+/// 2. Odd number of same variables but, ignore the ones that a guarenteed to be positive, there are
+///    even number of the vairables left: `x*x*x.abs()*x.abs()*x*x`
+/// 3. Any number of any variables, but all them are guarenteed to be positive:
+///    `x.abs()*y.abs()*z.abs()`
+/// 4. Any number of any variables, ignoreing the guarenteed ones, there are even number of same
+///    variables left: `x.abs()*y.abs()*x*x*y*y`
+/// 5. Any number that calls `pow*` with even number: `x.pow(2)`
+///
+/// TODO:
+/// 1. ~A function called `expr_is_always_positive`~
+/// 2. ~A function to peel binary op (Skip ops other than Mul, Div)~
+/// 3. ~When peeling binary op, collect each expr~
+/// 4. ~For each expr, check if they are always positive, then count them if not~
+/// 5. ~Detect above cases and call it for the day~
 fn should_lint(cx: &LateContext<'_>, cast_op: &Expr<'_>, cast_from: Ty<'_>, cast_to: Ty<'_>) -> bool {
     match (cast_from.is_integral(), cast_to.is_integral()) {
         (true, true) => {
             if !cast_from.is_signed() || cast_to.is_signed() {
                 return false;
             }
 
-            // Don't lint for positive constants.
-            let const_val = constant(cx, cx.typeck_results(), cast_op);
-            if let Some(Constant::Int(n)) = const_val
-                && let ty::Int(ity) = *cast_from.kind()
-                && sext(cx.tcx, n, ity) >= 0
-            {
+            // Don't lint if `cast_op` is known to be positive.
+            if let Sign::ZeroOrPositive = expr_sign(cx, cast_op, cast_from) {
                 return false;
             }
 
-            // Don't lint for the result of methods that always return non-negative values.
-            if let ExprKind::MethodCall(path, ..) = cast_op.kind {
-                let mut method_name = path.ident.name.as_str();
-                let allowed_methods = ["abs", "checked_abs", "rem_euclid", "checked_rem_euclid"];
-
-                if method_name == "unwrap"
-                    && let Some(arglist) = method_chain_args(cast_op, &["unwrap"])
-                    && let ExprKind::MethodCall(inner_path, ..) = &arglist[0].0.kind
-                {
-                    method_name = inner_path.ident.name.as_str();
-                }
-
-                if allowed_methods.iter().any(|&name| method_name == name) {
-                    return false;
-                }
+            let (mut uncertain_count, mut negative_count) = (0, 0);
+            // Peel off possible binary expressions, e.g. x * x * y => [x, x, y]
+            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;
+            };
+            for expr in exprs {
+                let ty = cx.typeck_results().expr_ty(expr);
+                match expr_sign(cx, expr, ty) {
+                    Sign::Negative => negative_count += 1,
+                    Sign::Uncertain => uncertain_count += 1,
+                    Sign::ZeroOrPositive => (),
+                };
             }
 
-            true
+            // Lint if there are odd number of uncertain or negative results
+            uncertain_count % 2 == 1 || negative_count % 2 == 1
         },
 
         (false, true) => !cast_to.is_signed(),
 
         (_, _) => false,
     }
 }
+
+fn get_const_int_eval(cx: &LateContext<'_>, expr: &Expr<'_>, ty: Ty<'_>) -> Option<i128> {
+    if let Constant::Int(n) = constant(cx, cx.typeck_results(), expr)?
+        && let ty::Int(ity) = *ty.kind()
+    {
+        return Some(sext(cx.tcx, n, ity));
+    }
+    None
+}
+
+enum Sign {
+    ZeroOrPositive,
+    Negative,
+    Uncertain,
+}
+
+fn expr_sign(cx: &LateContext<'_>, expr: &Expr<'_>, ty: Ty<'_>) -> Sign {
+    // Try evaluate this expr first to see if it's positive
+    if let Some(val) = get_const_int_eval(cx, expr, ty) {
+        return if val >= 0 { Sign::ZeroOrPositive } else { Sign::Negative };
+    }
+    // Calling on methods that always return non-negative values.
+    if let ExprKind::MethodCall(path, caller, args, ..) = expr.kind {
+        let mut method_name = path.ident.name.as_str();
+
+        if method_name == "unwrap"
+            && let Some(arglist) = method_chain_args(expr, &["unwrap"])
+            && let ExprKind::MethodCall(inner_path, ..) = &arglist[0].0.kind
+        {
+            method_name = inner_path.ident.name.as_str();
+        }
+
+        if method_name == "pow"
+            && let [arg] = args
+        {
+            return pow_call_result_sign(cx, caller, arg);
+        } else if METHODS_RET_POSITIVE.iter().any(|&name| method_name == name) {
+            return Sign::ZeroOrPositive;
+        }
+    }
+
+    Sign::Uncertain
+}
+
+/// Return the sign of the `pow` call's result.
+///
+/// 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.
+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
+    {
+        return Sign::ZeroOrPositive;
+    }
+
+    if let Some(Constant::Int(n)) = constant(cx, cx.typeck_results(), power_of)
+        && clip(cx.tcx, n, UintTy::U32) % 2 == 0
+    {
+        return Sign::ZeroOrPositive;
+    }
+
+    Sign::Uncertain
+}
+
+/// Peels binary operators such as [`BinOpKind::Mul`], [`BinOpKind::Div`] or [`BinOpKind::Rem`],
+/// which the result could always be positive under certain condition.
+///
+/// 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>>> {
+    #[inline]
+    fn collect_operands<'a>(expr: &'a Expr<'a>, operands: &mut Vec<&'a Expr<'a>>) -> Option<()> {
+        match expr.kind {
+            ExprKind::Binary(op, lhs, rhs) => {
+                if matches!(op.node, BinOpKind::Mul | BinOpKind::Div | BinOpKind::Rem) {
+                    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;
+                }
+            },
+            _ => operands.push(expr),
+        }
+        Some(())
+    }
+
+    let mut res = vec![];
+    collect_operands(expr, &mut res)?;
+    Some(res)
+}
diff --git a/tests/ui/cast.rs b/tests/ui/cast.rs
@@ -365,3 +365,52 @@ fn avoid_subtract_overflow(q: u32) {
 fn issue11426() {
     (&42u8 >> 0xa9008fb6c9d81e42_0e25730562a601c8_u128) as usize;
 }
+
+fn issue11642() {
+    fn square(x: i16) -> u32 {
+        let x = x as i32;
+        (x * x) as u32;
+        x.pow(2) as u32;
+        (-2_i32).pow(2) as u32
+    }
+
+    let _a = |x: i32| -> u32 { (x * x * x * x) as u32 };
+
+    (-2_i32).pow(3) as u32;
+    //~^ ERROR: casting `i32` to `u32` may lose the sign of the value
+
+    let x: i32 = 10;
+    (x * x) as u32;
+    (x * x * x) as u32;
+    //~^ ERROR: casting `i32` to `u32` may lose the sign of the value
+
+    let y: i16 = -2;
+    (y * y * y * y * -2) as u16;
+    //~^ ERROR: casting `i16` to `u16` may lose the sign of the value
+    (y * y * y * y * 2) as u16;
+    (y * y * y * 2) as u16;
+    //~^ ERROR: casting `i16` to `u16` may lose the sign of the value
+    (y * y * y * -2) as u16;
+    //~^ ERROR: casting `i16` to `u16` may lose the sign of the value
+
+    fn foo(a: i32, b: i32, c: i32) -> u32 {
+        (a * a * b * b * c * c) as u32;
+        (a * b * c) as u32;
+        //~^ ERROR: casting `i32` to `u32` may lose the sign of the value
+        (a * -b * c) as u32;
+        //~^ ERROR: casting `i32` to `u32` may lose the sign of the value
+        (a * b * c * c) as u32;
+        (a * -2) as u32;
+        //~^ ERROR: casting `i32` to `u32` may lose the sign of the value
+        (a * b * c * -2) as u32;
+        //~^ ERROR: casting `i32` to `u32` may lose the sign of the value
+        (a / b) as u32;
+        (a / b * c) as u32;
+        //~^ ERROR: casting `i32` to `u32` may lose the sign of the value
+        (a / b + b * c) as u32;
+        //~^ ERROR: casting `i32` to `u32` may lose the sign of the value
+        a.pow(3) as u32;
+        //~^ ERROR: casting `i32` to `u32` may lose the sign of the value
+        (a.abs() * b.pow(2) / c.abs()) as u32
+    }
+}
diff --git a/tests/ui/cast.stderr b/tests/ui/cast.stderr
@@ -444,5 +444,77 @@ help: ... or use `try_from` and handle the error accordingly
 LL |     let c = u8::try_from(q / 1000);
    |             ~~~~~~~~~~~~~~~~~~~~~~
 
-error: aborting due to 51 previous errors
+error: casting `i32` to `u32` may lose the sign of the value
+  --> $DIR/cast.rs:379:5
+   |
+LL |     (-2_i32).pow(3) as u32;
+   |     ^^^^^^^^^^^^^^^^^^^^^^
+
+error: casting `i32` to `u32` may lose the sign of the value
+  --> $DIR/cast.rs:384:5
+   |
+LL |     (x * x * x) as u32;
+   |     ^^^^^^^^^^^^^^^^^^
+
+error: casting `i16` to `u16` may lose the sign of the value
+  --> $DIR/cast.rs:388:5
+   |
+LL |     (y * y * y * y * -2) as u16;
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+error: casting `i16` to `u16` may lose the sign of the value
+  --> $DIR/cast.rs:391:5
+   |
+LL |     (y * y * y * 2) as u16;
+   |     ^^^^^^^^^^^^^^^^^^^^^^
+
+error: casting `i16` to `u16` may lose the sign of the value
+  --> $DIR/cast.rs:393:5
+   |
+LL |     (y * y * y * -2) as u16;
+   |     ^^^^^^^^^^^^^^^^^^^^^^^
+
+error: casting `i32` to `u32` may lose the sign of the value
+  --> $DIR/cast.rs:398:9
+   |
+LL |         (a * b * c) as u32;
+   |         ^^^^^^^^^^^^^^^^^^
+
+error: casting `i32` to `u32` may lose the sign of the value
+  --> $DIR/cast.rs:400:9
+   |
+LL |         (a * -b * c) as u32;
+   |         ^^^^^^^^^^^^^^^^^^^
+
+error: casting `i32` to `u32` may lose the sign of the value
+  --> $DIR/cast.rs:403:9
+   |
+LL |         (a * -2) as u32;
+   |         ^^^^^^^^^^^^^^^
+
+error: casting `i32` to `u32` may lose the sign of the value
+  --> $DIR/cast.rs:405:9
+   |
+LL |         (a * b * c * -2) as u32;
+   |         ^^^^^^^^^^^^^^^^^^^^^^^
+
+error: casting `i32` to `u32` may lose the sign of the value
+  --> $DIR/cast.rs:408:9
+   |
+LL |         (a / b * c) as u32;
+   |         ^^^^^^^^^^^^^^^^^^
+
+error: casting `i32` to `u32` may lose the sign of the value
+  --> $DIR/cast.rs:410:9
+   |
+LL |         (a / b + b * c) as u32;
+   |         ^^^^^^^^^^^^^^^^^^^^^^
+
+error: casting `i32` to `u32` may lose the sign of the value
+  --> $DIR/cast.rs:412:9
+   |
+LL |         a.pow(3) as u32;
+   |         ^^^^^^^^^^^^^^^
+
+error: aborting due to 63 previous errors
 
