From ee8fd82f4ca18d154522f65cafd808c8de1ecd9c Mon Sep 17 00:00:00 2001 From: teor Date: Thu, 11 Jan 2024 20:26:07 +1000 Subject: [PATCH 01/19] Fix uncertain sign and remainder op handling in cast_sign_loss.rs --- clippy_lints/src/casts/cast_sign_loss.rs | 90 ++++++++++++++++-------- 1 file 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 1df5a25f674d..1b907d825701 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>> { +/// 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 } From 02214f2ac00986f84fa32e136f0ba81c1f85a495 Mon Sep 17 00:00:00 2001 From: teor Date: Tue, 16 Jan 2024 16:40:32 +1000 Subject: [PATCH 02/19] Expand method lists --- clippy_lints/src/casts/cast_sign_loss.rs | 24 ++++++++++++++++++++++-- 1 file changed, 22 insertions(+), 2 deletions(-) diff --git a/clippy_lints/src/casts/cast_sign_loss.rs b/clippy_lints/src/casts/cast_sign_loss.rs index 1b907d825701..be8f524317ae 100644 --- a/clippy_lints/src/casts/cast_sign_loss.rs +++ b/clippy_lints/src/casts/cast_sign_loss.rs @@ -11,8 +11,28 @@ use super::CAST_SIGN_LOSS; /// 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"]; +/// because casting their return values can still result in sign loss. +const METHODS_RET_POSITIVE: &[&str] = &[ + "checked_abs", + "saturating_abs", + "isqrt", + "checked_isqrt", + "rem_euclid", + "checked_rem_euclid", + "wrapping_rem_euclid", +]; + +/// A list of methods that act like `pow()`, and can never return: +/// - a negative value from a non-negative base +/// - a negative value from a negative base and even exponent +/// - a non-negative value from a negative base and odd exponent +/// +/// Methods that can overflow and return a negative value must not be included in this list, +/// because casting their return values can still result in sign loss. +const METHODS_POW: &[&str] = &["pow", "saturating_pow", "checked_pow"]; + +/// A list of methods that act like `unwrap()`, and don't change the sign of the inner value. +const METHODS_UNWRAP: &[&str] = &["unwrap", "unwrap_unchecked", "expect", "into_ok"]; 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) { From e51fcd03dc6a5b61f98289825961be1bdbb3dd64 Mon Sep 17 00:00:00 2001 From: teor Date: Tue, 16 Jan 2024 16:40:49 +1000 Subject: [PATCH 03/19] Make pow_call_result_sign compile --- clippy_lints/src/casts/cast_sign_loss.rs | 26 +++++++++++------------- 1 file changed, 12 insertions(+), 14 deletions(-) diff --git a/clippy_lints/src/casts/cast_sign_loss.rs b/clippy_lints/src/casts/cast_sign_loss.rs index be8f524317ae..241f523769cc 100644 --- a/clippy_lints/src/casts/cast_sign_loss.rs +++ b/clippy_lints/src/casts/cast_sign_loss.rs @@ -59,12 +59,9 @@ fn should_lint(cx: &LateContext<'_>, cast_op: &Expr<'_>, cast_from: Ty<'_>, cast let (mut uncertain_count, mut negative_count) = (0, 0); // Peel off possible binary expressions, for example: - // x * x * y => [x, x, y] + // 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; - }; + let exprs = exprs_with_selected_binop_peeled(cast_op); for expr in exprs { let ty = cx.typeck_results().expr_ty(expr); match expr_sign(cx, expr, ty) { @@ -141,23 +138,24 @@ fn expr_sign(cx: &LateContext<'_>, expr: &Expr<'_>, ty: Ty<'_>) -> Sign { /// otherwise if the exponent is an odd number, the result is always negative. /// /// 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); - let Some(caller_val) = get_const_int_eval(cx, caller, caller_ty) else { +fn pow_call_result_sign(cx: &LateContext<'_>, base: &Expr<'_>, exponent: &Expr<'_>) -> Sign { + let base_ty = cx.typeck_results().expr_ty(base); + let Some(base_val) = get_const_int_eval(cx, base, base_ty) else { return Sign::Uncertain; - } - // Non-negative values raised to non-negative exponents are always non-negative, ignoring overflow. + }; + // Non-negative bases raised to non-negative exponents are always non-negative, ignoring overflow. // (Rust's integer pow() function takes an unsigned exponent.) - if caller_val >= 0 { + if base_val >= 0 { return Sign::ZeroOrPositive; } - let Some(Constant::Int(n)) = constant(cx, cx.typeck_results(), power_of) else { + let Some(Constant::Int(n)) = constant(cx, cx.typeck_results(), exponent) 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 { + if clip(cx.tcx, n, UintTy::U32) % 2 == 0 { return Sign::ZeroOrPositive; } else { return Sign::Negative; From 28f247e16ce6e311cbbb7208373f27c14a85e97f Mon Sep 17 00:00:00 2001 From: teor Date: Tue, 16 Jan 2024 16:50:16 +1000 Subject: [PATCH 04/19] Use the expanded method lists --- clippy_lints/src/casts/cast_sign_loss.rs | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/clippy_lints/src/casts/cast_sign_loss.rs b/clippy_lints/src/casts/cast_sign_loss.rs index 241f523769cc..690fd6a97118 100644 --- a/clippy_lints/src/casts/cast_sign_loss.rs +++ b/clippy_lints/src/casts/cast_sign_loss.rs @@ -102,24 +102,20 @@ fn expr_sign(cx: &LateContext<'_>, expr: &Expr<'_>, ty: Ty<'_>) -> Sign { 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 == "expect" - && let Some(arglist) = method_chain_args(expr, &["expect"]) + // Peel unwrap(), expect(), etc. + while let Some(&found_name) = METHODS_UNWRAP.iter().find(|&name| &method_name == name) + && let Some(arglist) = method_chain_args(expr, &[found_name]) && let ExprKind::MethodCall(inner_path, ..) = &arglist[0].0.kind { method_name = inner_path.ident.name.as_str(); } - if method_name == "pow" + if METHODS_POW.iter().any(|&name| method_name == name) && let [arg] = args { return pow_call_result_sign(cx, caller, arg); From 47339d01f8e22df7b25d7ac92f3628f4cdc982af Mon Sep 17 00:00:00 2001 From: teor Date: Tue, 16 Jan 2024 16:53:34 +1000 Subject: [PATCH 05/19] Bless fixed cast_sign_loss false negatives --- tests/ui/cast.stderr | 86 +++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 85 insertions(+), 1 deletion(-) diff --git a/tests/ui/cast.stderr b/tests/ui/cast.stderr index e2bcf94734e9..0eb3b95733a7 100644 --- a/tests/ui/cast.stderr +++ b/tests/ui/cast.stderr @@ -321,6 +321,36 @@ error: casting `isize` to `usize` may lose the sign of the value LL | -1isize as usize; | ^^^^^^^^^^^^^^^^ +error: casting `i8` to `u8` may lose the sign of the value + --> tests/ui/cast.rs:119:5 + | +LL | (-1i8).abs() as u8; + | ^^^^^^^^^^^^^^^^^^ + +error: casting `i16` to `u16` may lose the sign of the value + --> tests/ui/cast.rs:120:5 + | +LL | (-1i16).abs() as u16; + | ^^^^^^^^^^^^^^^^^^^^ + +error: casting `i32` to `u32` may lose the sign of the value + --> tests/ui/cast.rs:121:5 + | +LL | (-1i32).abs() as u32; + | ^^^^^^^^^^^^^^^^^^^^ + +error: casting `i64` to `u64` may lose the sign of the value + --> tests/ui/cast.rs:122:5 + | +LL | (-1i64).abs() as u64; + | ^^^^^^^^^^^^^^^^^^^^ + +error: casting `isize` to `usize` may lose the sign of the value + --> tests/ui/cast.rs:123:5 + | +LL | (-1isize).abs() as usize; + | ^^^^^^^^^^^^^^^^^^^^^^^^ + error: casting `i64` to `i8` may truncate the value --> tests/ui/cast.rs:179:5 | @@ -444,12 +474,36 @@ help: ... or use `try_from` and handle the error accordingly LL | let c = u8::try_from(q / 1000); | ~~~~~~~~~~~~~~~~~~~~~~ +error: casting `i32` to `u32` may lose the sign of the value + --> tests/ui/cast.rs:372:9 + | +LL | (x * x) as u32; + | ^^^^^^^^^^^^^^ + +error: casting `i32` to `u32` may lose the sign of the value + --> tests/ui/cast.rs:373:9 + | +LL | x.pow(2) as u32; + | ^^^^^^^^^^^^^^^ + +error: casting `i32` to `u32` may lose the sign of the value + --> tests/ui/cast.rs:377:32 + | +LL | let _a = |x: i32| -> u32 { (x * x * x * x) as u32 }; + | ^^^^^^^^^^^^^^^^^^^^^^ + error: casting `i32` to `u32` may lose the sign of the value --> tests/ui/cast.rs:379:5 | LL | (-2_i32).pow(3) as u32; | ^^^^^^^^^^^^^^^^^^^^^^ +error: casting `i32` to `u32` may lose the sign of the value + --> tests/ui/cast.rs:383:5 + | +LL | (x * x) as u32; + | ^^^^^^^^^^^^^^ + error: casting `i32` to `u32` may lose the sign of the value --> tests/ui/cast.rs:384:5 | @@ -462,6 +516,12 @@ error: casting `i16` to `u16` may lose the sign of the value LL | (y * y * y * y * -2) as u16; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ +error: casting `i16` to `u16` may lose the sign of the value + --> tests/ui/cast.rs:390:5 + | +LL | (y * y * y * y * 2) as u16; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^ + error: casting `i16` to `u16` may lose the sign of the value --> tests/ui/cast.rs:391:5 | @@ -474,6 +534,12 @@ error: casting `i16` to `u16` may lose the sign of the value LL | (y * y * y * -2) as u16; | ^^^^^^^^^^^^^^^^^^^^^^^ +error: casting `i32` to `u32` may lose the sign of the value + --> tests/ui/cast.rs:397:9 + | +LL | (a * a * b * b * c * c) as u32; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + error: casting `i32` to `u32` may lose the sign of the value --> tests/ui/cast.rs:398:9 | @@ -486,6 +552,12 @@ error: casting `i32` to `u32` may lose the sign of the value LL | (a * -b * c) as u32; | ^^^^^^^^^^^^^^^^^^^ +error: casting `i32` to `u32` may lose the sign of the value + --> tests/ui/cast.rs:402:9 + | +LL | (a * b * c * c) as u32; + | ^^^^^^^^^^^^^^^^^^^^^^ + error: casting `i32` to `u32` may lose the sign of the value --> tests/ui/cast.rs:403:9 | @@ -498,6 +570,12 @@ error: casting `i32` to `u32` may lose the sign of the value LL | (a * b * c * -2) as u32; | ^^^^^^^^^^^^^^^^^^^^^^^ +error: casting `i32` to `u32` may lose the sign of the value + --> tests/ui/cast.rs:407:9 + | +LL | (a / b) as u32; + | ^^^^^^^^^^^^^^ + error: casting `i32` to `u32` may lose the sign of the value --> tests/ui/cast.rs:408:9 | @@ -516,5 +594,11 @@ error: casting `i32` to `u32` may lose the sign of the value LL | a.pow(3) as u32; | ^^^^^^^^^^^^^^^ -error: aborting due to 63 previous errors +error: casting `i32` to `u32` may lose the sign of the value + --> tests/ui/cast.rs:414:9 + | +LL | (a.abs() * b.pow(2) / c.abs()) as u32 + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +error: aborting due to 77 previous errors From c5e8487d63290fc38e6261c45425662be9279a36 Mon Sep 17 00:00:00 2001 From: teor Date: Tue, 16 Jan 2024 17:43:57 +1000 Subject: [PATCH 06/19] Fix pow() to return more known signs --- clippy_lints/src/casts/cast_sign_loss.rs | 69 +++++++++++++----------- 1 file changed, 38 insertions(+), 31 deletions(-) diff --git a/clippy_lints/src/casts/cast_sign_loss.rs b/clippy_lints/src/casts/cast_sign_loss.rs index 690fd6a97118..ba8222fc0cd5 100644 --- a/clippy_lints/src/casts/cast_sign_loss.rs +++ b/clippy_lints/src/casts/cast_sign_loss.rs @@ -1,9 +1,9 @@ use clippy_utils::consts::{constant, Constant}; use clippy_utils::diagnostics::span_lint; -use clippy_utils::{clip, method_chain_args, sext}; +use clippy_utils::{method_chain_args, sext}; use rustc_hir::{BinOpKind, Expr, ExprKind}; use rustc_lint::LateContext; -use rustc_middle::ty::{self, Ty, UintTy}; +use rustc_middle::ty::{self, Ty}; use super::CAST_SIGN_LOSS; @@ -22,10 +22,7 @@ const METHODS_RET_POSITIVE: &[&str] = &[ "wrapping_rem_euclid", ]; -/// A list of methods that act like `pow()`, and can never return: -/// - a negative value from a non-negative base -/// - a negative value from a negative base and even exponent -/// - a non-negative value from a negative base and odd exponent +/// A list of methods that act like `pow()`. See `pow_call_result_sign()` for details. /// /// Methods that can overflow and return a negative value must not be included in this list, /// because casting their return values can still result in sign loss. @@ -34,7 +31,13 @@ const METHODS_POW: &[&str] = &["pow", "saturating_pow", "checked_pow"]; /// A list of methods that act like `unwrap()`, and don't change the sign of the inner value. const METHODS_UNWRAP: &[&str] = &["unwrap", "unwrap_unchecked", "expect", "into_ok"]; -pub(super) fn check(cx: &LateContext<'_>, expr: &Expr<'_>, cast_op: &Expr<'_>, cast_from: Ty<'_>, cast_to: Ty<'_>) { +pub(super) fn check<'cx>( + cx: &LateContext<'cx>, + expr: &Expr<'_>, + cast_op: &Expr<'_>, + cast_from: Ty<'cx>, + cast_to: Ty<'_>, +) { if should_lint(cx, cast_op, cast_from, cast_to) { span_lint( cx, @@ -45,7 +48,7 @@ pub(super) fn check(cx: &LateContext<'_>, expr: &Expr<'_>, cast_op: &Expr<'_>, c } } -fn should_lint(cx: &LateContext<'_>, cast_op: &Expr<'_>, cast_from: Ty<'_>, cast_to: Ty<'_>) -> bool { +fn should_lint<'cx>(cx: &LateContext<'cx>, cast_op: &Expr<'_>, cast_from: Ty<'cx>, cast_to: Ty<'_>) -> bool { match (cast_from.is_integral(), cast_to.is_integral()) { (true, true) => { if !cast_from.is_signed() || cast_to.is_signed() { @@ -82,7 +85,9 @@ fn should_lint(cx: &LateContext<'_>, cast_op: &Expr<'_>, cast_from: Ty<'_>, cast } } -fn get_const_int_eval(cx: &LateContext<'_>, expr: &Expr<'_>, ty: Ty<'_>) -> Option { +fn get_const_int_eval<'cx>(cx: &LateContext<'cx>, expr: &Expr<'_>, ty: impl Into>>) -> Option { + let ty = ty.into().unwrap_or_else(|| cx.typeck_results().expr_ty(expr)); + if let Constant::Int(n) = constant(cx, cx.typeck_results(), expr)? && let ty::Int(ity) = *ty.kind() { @@ -97,7 +102,7 @@ enum Sign { Uncertain, } -fn expr_sign(cx: &LateContext<'_>, expr: &Expr<'_>, ty: Ty<'_>) -> Sign { +fn expr_sign<'cx>(cx: &LateContext<'cx>, expr: &Expr<'_>, ty: impl Into>>) -> 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 }; @@ -112,6 +117,8 @@ fn expr_sign(cx: &LateContext<'_>, expr: &Expr<'_>, ty: Ty<'_>) -> Sign { && let Some(arglist) = method_chain_args(expr, &[found_name]) && let ExprKind::MethodCall(inner_path, ..) = &arglist[0].0.kind { + // The original type has changed, but we can't use `ty` here anyway, because it has been + // moved. method_name = inner_path.ident.name.as_str(); } @@ -130,31 +137,31 @@ fn expr_sign(cx: &LateContext<'_>, expr: &Expr<'_>, ty: Ty<'_>) -> Sign { /// 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 exponent is a even number, the result is always positive, +/// Otherwise, if the base is negative, and the exponent is an odd number, the result is always +/// negative. /// -/// If either value can't be evaluated, [`Sign::Uncertain`] will be returned. +/// Otherwise, returns [`Sign::Uncertain`]. fn pow_call_result_sign(cx: &LateContext<'_>, base: &Expr<'_>, exponent: &Expr<'_>) -> Sign { - let base_ty = cx.typeck_results().expr_ty(base); - let Some(base_val) = get_const_int_eval(cx, base, base_ty) else { - return Sign::Uncertain; - }; - // Non-negative bases raised to non-negative exponents are always non-negative, ignoring overflow. - // (Rust's integer pow() function takes an unsigned exponent.) - if base_val >= 0 { - return Sign::ZeroOrPositive; - } + let base_sign = expr_sign(cx, base, None); + let exponent_val = get_const_int_eval(cx, exponent, None); + let exponent_is_even = exponent_val.map(|val| val % 2 == 0); + + match (base_sign, exponent_is_even) { + // Non-negative bases always return non-negative results, ignoring overflow. + // This is because Rust's integer pow() functions take an unsigned exponent. + (Sign::ZeroOrPositive, _) => Sign::ZeroOrPositive, + + // Any base raised to an even exponent is non-negative. + // This is true even if we don't know the value of the base. + (_, Some(true)) => Sign::ZeroOrPositive, - let Some(Constant::Int(n)) = constant(cx, cx.typeck_results(), exponent) else { - return Sign::Uncertain; - }; + // A negative base raised to an odd exponent is non-negative. + (Sign::Negative, Some(false)) => Sign::Negative, - // 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 { - return Sign::ZeroOrPositive; - } else { - return Sign::Negative; + // Negative or unknown base to an unknown exponent, or an unknown base to an odd exponent. + (Sign::Negative | Sign::Uncertain, None) => Sign::Uncertain, + (Sign::Uncertain, Some(false)) => Sign::Uncertain, } } From e74fe4362ac6b79751aa1ddd72dfb256b006fff6 Mon Sep 17 00:00:00 2001 From: teor Date: Tue, 16 Jan 2024 18:03:35 +1000 Subject: [PATCH 07/19] Check for both signed and unsigned constant expressions --- clippy_lints/src/casts/cast_sign_loss.rs | 35 ++++++++++++++++++++---- tests/ui/cast.stderr | 2 +- 2 files changed, 30 insertions(+), 7 deletions(-) diff --git a/clippy_lints/src/casts/cast_sign_loss.rs b/clippy_lints/src/casts/cast_sign_loss.rs index ba8222fc0cd5..530fb0dcde03 100644 --- a/clippy_lints/src/casts/cast_sign_loss.rs +++ b/clippy_lints/src/casts/cast_sign_loss.rs @@ -66,8 +66,7 @@ fn should_lint<'cx>(cx: &LateContext<'cx>, cast_op: &Expr<'_>, cast_from: Ty<'cx // a % b => [a] let exprs = exprs_with_selected_binop_peeled(cast_op); for expr in exprs { - let ty = cx.typeck_results().expr_ty(expr); - match expr_sign(cx, expr, ty) { + match expr_sign(cx, expr, None) { Sign::Negative => negative_count += 1, Sign::Uncertain => uncertain_count += 1, Sign::ZeroOrPositive => (), @@ -85,7 +84,11 @@ fn should_lint<'cx>(cx: &LateContext<'cx>, cast_op: &Expr<'_>, cast_from: Ty<'cx } } -fn get_const_int_eval<'cx>(cx: &LateContext<'cx>, expr: &Expr<'_>, ty: impl Into>>) -> Option { +fn get_const_signed_int_eval<'cx>( + cx: &LateContext<'cx>, + expr: &Expr<'_>, + ty: impl Into>>, +) -> Option { let ty = ty.into().unwrap_or_else(|| cx.typeck_results().expr_ty(expr)); if let Constant::Int(n) = constant(cx, cx.typeck_results(), expr)? @@ -96,6 +99,22 @@ fn get_const_int_eval<'cx>(cx: &LateContext<'cx>, expr: &Expr<'_>, ty: impl Into None } +fn get_const_unsigned_int_eval<'cx>( + cx: &LateContext<'cx>, + expr: &Expr<'_>, + ty: impl Into>>, +) -> Option { + let ty = ty.into().unwrap_or_else(|| cx.typeck_results().expr_ty(expr)); + + if let Constant::Int(n) = constant(cx, cx.typeck_results(), expr)? + && let ty::Uint(_ity) = *ty.kind() + { + return Some(n); + } + None +} + +#[derive(Copy, Clone, Debug, Eq, PartialEq)] enum Sign { ZeroOrPositive, Negative, @@ -104,9 +123,12 @@ enum Sign { fn expr_sign<'cx>(cx: &LateContext<'cx>, expr: &Expr<'_>, ty: impl Into>>) -> Sign { // Try evaluate this expr first to see if it's positive - if let Some(val) = get_const_int_eval(cx, expr, ty) { + if let Some(val) = get_const_signed_int_eval(cx, expr, ty) { return if val >= 0 { Sign::ZeroOrPositive } else { Sign::Negative }; } + if let Some(_val) = get_const_unsigned_int_eval(cx, expr, None) { + return Sign::ZeroOrPositive; + } // Calling on methods that always return non-negative values. if let ExprKind::MethodCall(path, caller, args, ..) = expr.kind { @@ -144,12 +166,13 @@ fn expr_sign<'cx>(cx: &LateContext<'cx>, expr: &Expr<'_>, ty: impl Into, base: &Expr<'_>, exponent: &Expr<'_>) -> Sign { let base_sign = expr_sign(cx, base, None); - let exponent_val = get_const_int_eval(cx, exponent, None); + + // Rust's integer pow() functions take an unsigned exponent. + let exponent_val = get_const_unsigned_int_eval(cx, exponent, None); let exponent_is_even = exponent_val.map(|val| val % 2 == 0); match (base_sign, exponent_is_even) { // Non-negative bases always return non-negative results, ignoring overflow. - // This is because Rust's integer pow() functions take an unsigned exponent. (Sign::ZeroOrPositive, _) => Sign::ZeroOrPositive, // Any base raised to an even exponent is non-negative. diff --git a/tests/ui/cast.stderr b/tests/ui/cast.stderr index 0eb3b95733a7..aaa6b4183e95 100644 --- a/tests/ui/cast.stderr +++ b/tests/ui/cast.stderr @@ -600,5 +600,5 @@ error: casting `i32` to `u32` may lose the sign of the value LL | (a.abs() * b.pow(2) / c.abs()) as u32 | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ -error: aborting due to 77 previous errors +error: aborting due to 76 previous errors From 8b615af7608c243a2be777d36df59e75e2b553df Mon Sep 17 00:00:00 2001 From: teor Date: Tue, 16 Jan 2024 18:09:16 +1000 Subject: [PATCH 08/19] Fix clippy_dogfood --- clippy_lints/src/casts/cast_sign_loss.rs | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/clippy_lints/src/casts/cast_sign_loss.rs b/clippy_lints/src/casts/cast_sign_loss.rs index 530fb0dcde03..b1a0f46bb02f 100644 --- a/clippy_lints/src/casts/cast_sign_loss.rs +++ b/clippy_lints/src/casts/cast_sign_loss.rs @@ -173,17 +173,18 @@ fn pow_call_result_sign(cx: &LateContext<'_>, base: &Expr<'_>, exponent: &Expr<' match (base_sign, exponent_is_even) { // Non-negative bases always return non-negative results, ignoring overflow. - (Sign::ZeroOrPositive, _) => Sign::ZeroOrPositive, - + (Sign::ZeroOrPositive, _) | // Any base raised to an even exponent is non-negative. - // This is true even if we don't know the value of the base. - (_, Some(true)) => Sign::ZeroOrPositive, + // These both hold even if we don't know the value of the base. + (_, Some(true)) + => Sign::ZeroOrPositive, // A negative base raised to an odd exponent is non-negative. (Sign::Negative, Some(false)) => Sign::Negative, - // Negative or unknown base to an unknown exponent, or an unknown base to an odd exponent. - (Sign::Negative | Sign::Uncertain, None) => Sign::Uncertain, + // Negative/unknown base to an unknown exponent, or unknown base to an odd exponent. + // Could be negative or positive depending on the actual values. + (Sign::Negative | Sign::Uncertain, None) | (Sign::Uncertain, Some(false)) => Sign::Uncertain, } } From 11e0d6acca95e3db70ff0cbd474cb3bacd5ad1a7 Mon Sep 17 00:00:00 2001 From: teor Date: Tue, 16 Jan 2024 18:25:38 +1000 Subject: [PATCH 09/19] Move muldiv peeling into expr_sign --- clippy_lints/src/casts/cast_sign_loss.rs | 50 +++++++++++++----------- 1 file changed, 28 insertions(+), 22 deletions(-) diff --git a/clippy_lints/src/casts/cast_sign_loss.rs b/clippy_lints/src/casts/cast_sign_loss.rs index b1a0f46bb02f..7e4efbbc03d6 100644 --- a/clippy_lints/src/casts/cast_sign_loss.rs +++ b/clippy_lints/src/casts/cast_sign_loss.rs @@ -56,26 +56,7 @@ fn should_lint<'cx>(cx: &LateContext<'cx>, cast_op: &Expr<'_>, cast_from: Ty<'cx } // 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, for example: - // x * x / y => [x, x, y] - // a % b => [a] - let exprs = exprs_with_selected_binop_peeled(cast_op); - for expr in exprs { - match expr_sign(cx, expr, None) { - Sign::Negative => negative_count += 1, - Sign::Uncertain => uncertain_count += 1, - Sign::ZeroOrPositive => (), - }; - } - - // 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 + expr_sign(cx, cast_op, cast_from) == Sign::ZeroOrPositive }, (false, true) => !cast_to.is_signed(), @@ -153,7 +134,32 @@ fn expr_sign<'cx>(cx: &LateContext<'cx>, expr: &Expr<'_>, ty: impl Into [x, x, y] + // a % b => [a] + let exprs = exprs_with_muldiv_binop_peeled(expr); + for expr in exprs { + match expr_sign(cx, expr, None) { + Sign::Negative => negative_count += 1, + Sign::Uncertain => uncertain_count += 1, + Sign::ZeroOrPositive => (), + }; + } + + // A mul/div is: + // - uncertain if there are any uncertain values (because they could be negative or positive), + // - negative if there are an odd number of negative values, + // - positive or zero otherwise. + if uncertain_count > 0 { + Sign::Uncertain + } else if negative_count % 2 == 1 { + Sign::Negative + } else { + Sign::ZeroOrPositive + } } /// Return the sign of the `pow` call's result, ignoring overflow. @@ -193,7 +199,7 @@ fn pow_call_result_sign(cx: &LateContext<'_>, base: &Expr<'_>, exponent: &Expr<' /// which the result could always be positive under certain conditions, ignoring overflow. /// /// 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>> { +fn exprs_with_muldiv_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>>) { match expr.kind { From 4ab2ed33a07c9b2c76430b8221f78b6efd9fde79 Mon Sep 17 00:00:00 2001 From: teor Date: Tue, 16 Jan 2024 18:33:20 +1000 Subject: [PATCH 10/19] Put muldiv peeling in its own method --- clippy_lints/src/casts/cast_sign_loss.rs | 74 ++++++++++++++---------- 1 file changed, 45 insertions(+), 29 deletions(-) diff --git a/clippy_lints/src/casts/cast_sign_loss.rs b/clippy_lints/src/casts/cast_sign_loss.rs index 7e4efbbc03d6..db7121928fe8 100644 --- a/clippy_lints/src/casts/cast_sign_loss.rs +++ b/clippy_lints/src/casts/cast_sign_loss.rs @@ -56,7 +56,15 @@ fn should_lint<'cx>(cx: &LateContext<'cx>, cast_op: &Expr<'_>, cast_from: Ty<'cx } // Don't lint if `cast_op` is known to be positive, ignoring overflow. - expr_sign(cx, cast_op, cast_from) == Sign::ZeroOrPositive + if let Sign::ZeroOrPositive = expr_sign(cx, cast_op, cast_from) { + return false; + } + + if let Sign::ZeroOrPositive = expr_muldiv_sign(cx, cast_op) { + return false; + } + + true }, (false, true) => !cast_to.is_signed(), @@ -134,32 +142,7 @@ fn expr_sign<'cx>(cx: &LateContext<'cx>, expr: &Expr<'_>, ty: impl Into [x, x, y] - // a % b => [a] - let exprs = exprs_with_muldiv_binop_peeled(expr); - for expr in exprs { - match expr_sign(cx, expr, None) { - Sign::Negative => negative_count += 1, - Sign::Uncertain => uncertain_count += 1, - Sign::ZeroOrPositive => (), - }; - } - - // A mul/div is: - // - uncertain if there are any uncertain values (because they could be negative or positive), - // - negative if there are an odd number of negative values, - // - positive or zero otherwise. - if uncertain_count > 0 { - Sign::Uncertain - } else if negative_count % 2 == 1 { - Sign::Negative - } else { - Sign::ZeroOrPositive - } + Sign::Uncertain } /// Return the sign of the `pow` call's result, ignoring overflow. @@ -195,13 +178,46 @@ fn pow_call_result_sign(cx: &LateContext<'_>, base: &Expr<'_>, exponent: &Expr<' } } +/// Peels binary operators such as [`BinOpKind::Mul`], [`BinOpKind::Div`] or [`BinOpKind::Rem`], +/// which the result could always be positive under certain conditions, ignoring overflow. +/// +/// Returns the sign of the list of peeled expressions. +fn expr_muldiv_sign(cx: &LateContext<'_>, expr: &Expr<'_>) -> Sign { + let mut uncertain_count = 0; + let mut negative_count = 0; + + // Peel off possible binary expressions, for example: + // x * x / y => [x, x, y] + // a % b => [a] + let exprs = exprs_with_muldiv_binop_peeled(expr); + for expr in exprs { + match expr_sign(cx, expr, None) { + Sign::Negative => negative_count += 1, + Sign::Uncertain => uncertain_count += 1, + Sign::ZeroOrPositive => (), + }; + } + + // A mul/div is: + // - uncertain if there are any uncertain values (because they could be negative or positive), + // - negative if there are an odd number of negative values, + // - positive or zero otherwise. + if uncertain_count > 0 { + Sign::Uncertain + } else if negative_count % 2 == 1 { + Sign::Negative + } else { + Sign::ZeroOrPositive + } +} + /// Peels binary operators such as [`BinOpKind::Mul`], [`BinOpKind::Div`] or [`BinOpKind::Rem`], /// which the result could always be positive under certain conditions, ignoring overflow. /// /// Expressions using other operators are preserved, so we can try to evaluate them later. -fn exprs_with_muldiv_binop_peeled<'a>(expr: &'a Expr<'_>) -> Vec<&'a Expr<'a>> { +fn exprs_with_muldiv_binop_peeled<'e>(expr: &'e Expr<'_>) -> Vec<&'e Expr<'e>> { #[inline] - fn collect_operands<'a>(expr: &'a Expr<'a>, operands: &mut Vec<&'a Expr<'a>>) { + fn collect_operands<'e>(expr: &'e Expr<'e>, operands: &mut Vec<&'e Expr<'e>>) { match expr.kind { ExprKind::Binary(op, lhs, rhs) => { if matches!(op.node, BinOpKind::Mul | BinOpKind::Div) { From 740441fd98090cde764a360587d380dbd66a69f0 Mon Sep 17 00:00:00 2001 From: teor Date: Tue, 16 Jan 2024 18:56:51 +1000 Subject: [PATCH 11/19] Use the visitor pattern instead of recusive functions --- clippy_lints/src/casts/cast_sign_loss.rs | 30 ++++++++++++++---------- 1 file changed, 18 insertions(+), 12 deletions(-) diff --git a/clippy_lints/src/casts/cast_sign_loss.rs b/clippy_lints/src/casts/cast_sign_loss.rs index db7121928fe8..20a7cbd4a1ba 100644 --- a/clippy_lints/src/casts/cast_sign_loss.rs +++ b/clippy_lints/src/casts/cast_sign_loss.rs @@ -1,5 +1,8 @@ +use std::ops::ControlFlow; + use clippy_utils::consts::{constant, Constant}; use clippy_utils::diagnostics::span_lint; +use clippy_utils::visitors::{for_each_expr, Descend}; use clippy_utils::{method_chain_args, sext}; use rustc_hir::{BinOpKind, Expr, ExprKind}; use rustc_lint::LateContext; @@ -216,34 +219,37 @@ fn expr_muldiv_sign(cx: &LateContext<'_>, expr: &Expr<'_>) -> Sign { /// /// Expressions using other operators are preserved, so we can try to evaluate them later. fn exprs_with_muldiv_binop_peeled<'e>(expr: &'e Expr<'_>) -> Vec<&'e Expr<'e>> { - #[inline] - fn collect_operands<'e>(expr: &'e Expr<'e>, operands: &mut Vec<&'e Expr<'e>>) { - match expr.kind { - ExprKind::Binary(op, lhs, rhs) => { + let mut res = vec![]; + + for_each_expr(expr, |sub_expr| { + match sub_expr.kind { + ExprKind::Binary(op, lhs, _rhs) => { 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); + ControlFlow::Continue(Descend::Yes) } 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); + res.push(lhs); + ControlFlow::Break(()) } else { // 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); + res.push(expr); + ControlFlow::Continue(Descend::No) } }, // For other expressions, including unary operators and constants, try to evaluate the expression. - _ => operands.push(expr), + _ => { + res.push(expr); + ControlFlow::Continue(Descend::No) + }, } - } + }); - let mut res = vec![]; - collect_operands(expr, &mut res); res } From 6dfff19090e544c58a9bbb6caaf406da7db3d427 Mon Sep 17 00:00:00 2001 From: teor Date: Tue, 16 Jan 2024 19:00:32 +1000 Subject: [PATCH 12/19] clippy_dogfood --- clippy_lints/src/casts/cast_sign_loss.rs | 45 +++++++++++------------- 1 file changed, 21 insertions(+), 24 deletions(-) diff --git a/clippy_lints/src/casts/cast_sign_loss.rs b/clippy_lints/src/casts/cast_sign_loss.rs index 20a7cbd4a1ba..1d71545913a4 100644 --- a/clippy_lints/src/casts/cast_sign_loss.rs +++ b/clippy_lints/src/casts/cast_sign_loss.rs @@ -222,32 +222,29 @@ fn exprs_with_muldiv_binop_peeled<'e>(expr: &'e Expr<'_>) -> Vec<&'e Expr<'e>> { let mut res = vec![]; for_each_expr(expr, |sub_expr| { - match sub_expr.kind { - ExprKind::Binary(op, lhs, _rhs) => { - 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. - ControlFlow::Continue(Descend::Yes) - } 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 - res.push(lhs); - ControlFlow::Break(()) - } else { - // The sign of the result of other binary operators depends on the values of the operands, - // so try to evaluate the expression. - res.push(expr); - ControlFlow::Continue(Descend::No) - } - }, - // For other expressions, including unary operators and constants, try to evaluate the expression. - _ => { + if let ExprKind::Binary(op, lhs, _rhs) = sub_expr.kind { + 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. + ControlFlow::Continue(Descend::Yes) + } 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 + res.push(lhs); + ControlFlow::Break(()) + } else { + // The sign of the result of other binary operators depends on the values of the operands, + // so try to evaluate the expression. res.push(expr); ControlFlow::Continue(Descend::No) - }, + } + } else { + // For other expressions, including unary operators and constants, try to evaluate the expression. + res.push(expr); + ControlFlow::Continue(Descend::No) } }); From d109e681782d71c6426b155d225b816893538a9c Mon Sep 17 00:00:00 2001 From: teor Date: Tue, 16 Jan 2024 19:04:22 +1000 Subject: [PATCH 13/19] Add some TODO comments --- clippy_lints/src/casts/cast_sign_loss.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/clippy_lints/src/casts/cast_sign_loss.rs b/clippy_lints/src/casts/cast_sign_loss.rs index 1d71545913a4..1100625bc4a9 100644 --- a/clippy_lints/src/casts/cast_sign_loss.rs +++ b/clippy_lints/src/casts/cast_sign_loss.rs @@ -63,6 +63,7 @@ fn should_lint<'cx>(cx: &LateContext<'cx>, cast_op: &Expr<'_>, cast_from: Ty<'cx return false; } + // We don't check for sums of all-positive or all-negative values, but we could. if let Sign::ZeroOrPositive = expr_muldiv_sign(cx, cast_op) { return false; } @@ -222,6 +223,7 @@ fn exprs_with_muldiv_binop_peeled<'e>(expr: &'e Expr<'_>) -> Vec<&'e Expr<'e>> { let mut res = vec![]; for_each_expr(expr, |sub_expr| { + // We don't check for mul/div/rem methods here, but we could. if let ExprKind::Binary(op, lhs, _rhs) = sub_expr.kind { if matches!(op.node, BinOpKind::Mul | BinOpKind::Div) { // For binary operators which both contribute to the sign of the result, From 1cbb58bd061a72f07f087c7f0befad6b0b214538 Mon Sep 17 00:00:00 2001 From: teor Date: Wed, 17 Jan 2024 12:22:23 +1000 Subject: [PATCH 14/19] Fix divmul peeling --- clippy_lints/src/casts/cast_sign_loss.rs | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/clippy_lints/src/casts/cast_sign_loss.rs b/clippy_lints/src/casts/cast_sign_loss.rs index 1100625bc4a9..9c18989701e0 100644 --- a/clippy_lints/src/casts/cast_sign_loss.rs +++ b/clippy_lints/src/casts/cast_sign_loss.rs @@ -229,23 +229,30 @@ fn exprs_with_muldiv_binop_peeled<'e>(expr: &'e Expr<'_>) -> Vec<&'e Expr<'e>> { // For binary operators which both contribute to the sign of the result, // collect all their operands, recursively. This ignores overflow. ControlFlow::Continue(Descend::Yes) - } else if matches!(op.node, BinOpKind::Rem) { + } else if matches!(op.node, BinOpKind::Rem | BinOpKind::Shr) { // 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. // + // Large left shifts turn negatives into zeroes, so we can't use it here. + // // > Given remainder = dividend % divisor, the remainder will have the same sign as the dividend + // > ... + // > Arithmetic right shift on signed integer types // https://doc.rust-lang.org/reference/expressions/operator-expr.html#arithmetic-and-logical-binary-operators + + // We want to descend into the lhs, but skip the rhs. + // That's tricky to do using for_each_expr(), so we just keep the lhs intact. res.push(lhs); - ControlFlow::Break(()) + ControlFlow::Continue(Descend::No) } else { // The sign of the result of other binary operators depends on the values of the operands, // so try to evaluate the expression. - res.push(expr); + res.push(sub_expr); ControlFlow::Continue(Descend::No) } } else { // For other expressions, including unary operators and constants, try to evaluate the expression. - res.push(expr); + res.push(sub_expr); ControlFlow::Continue(Descend::No) } }); From 6670acd2878b2053b8118dcf65056f99d755fe5a Mon Sep 17 00:00:00 2001 From: teor Date: Wed, 17 Jan 2024 12:32:43 +1000 Subject: [PATCH 15/19] Check for all-positive or all-negative sums --- clippy_lints/src/casts/cast_sign_loss.rs | 90 ++++++++++++++++++++++-- 1 file changed, 83 insertions(+), 7 deletions(-) diff --git a/clippy_lints/src/casts/cast_sign_loss.rs b/clippy_lints/src/casts/cast_sign_loss.rs index 9c18989701e0..546cfca21566 100644 --- a/clippy_lints/src/casts/cast_sign_loss.rs +++ b/clippy_lints/src/casts/cast_sign_loss.rs @@ -1,3 +1,4 @@ +use std::convert::Infallible; use std::ops::ControlFlow; use clippy_utils::consts::{constant, Constant}; @@ -63,11 +64,14 @@ fn should_lint<'cx>(cx: &LateContext<'cx>, cast_op: &Expr<'_>, cast_from: Ty<'cx return false; } - // We don't check for sums of all-positive or all-negative values, but we could. if let Sign::ZeroOrPositive = expr_muldiv_sign(cx, cast_op) { return false; } + if let Sign::ZeroOrPositive = expr_add_sign(cx, cast_op) { + return false; + } + true }, @@ -182,13 +186,13 @@ fn pow_call_result_sign(cx: &LateContext<'_>, base: &Expr<'_>, exponent: &Expr<' } } -/// Peels binary operators such as [`BinOpKind::Mul`], [`BinOpKind::Div`] or [`BinOpKind::Rem`], -/// which the result could always be positive under certain conditions, ignoring overflow. +/// Peels binary operators such as [`BinOpKind::Mul`] or [`BinOpKind::Rem`], +/// where the result could always be positive. See [`exprs_with_muldiv_binop_peeled()`] for details. /// /// Returns the sign of the list of peeled expressions. fn expr_muldiv_sign(cx: &LateContext<'_>, expr: &Expr<'_>) -> Sign { - let mut uncertain_count = 0; let mut negative_count = 0; + let mut uncertain_count = 0; // Peel off possible binary expressions, for example: // x * x / y => [x, x, y] @@ -215,18 +219,58 @@ fn expr_muldiv_sign(cx: &LateContext<'_>, expr: &Expr<'_>) -> Sign { } } +/// Peels binary operators such as [`BinOpKind::Add`], where the result could always be positive. +/// See [`exprs_with_add_binop_peeled()`] for details. +/// +/// Returns the sign of the list of peeled expressions. +fn expr_add_sign(cx: &LateContext<'_>, expr: &Expr<'_>) -> Sign { + let mut negative_count = 0; + let mut uncertain_count = 0; + let mut positive_count = 0; + + // Peel off possible binary expressions, for example: + // a + b + c => [a, b, c] + let exprs = exprs_with_add_binop_peeled(expr); + for expr in exprs { + match expr_sign(cx, expr, None) { + Sign::Negative => negative_count += 1, + Sign::Uncertain => uncertain_count += 1, + Sign::ZeroOrPositive => positive_count += 1, + }; + } + + // A sum is: + // - uncertain if there are any uncertain values (because they could be negative or positive), + // - positive or zero if there are only positive (or zero) values, + // - negative if there are only negative (or zero) values. + // We could split Zero out into its own variant, but we don't yet. + if uncertain_count > 0 { + Sign::Uncertain + } else if negative_count == 0 { + Sign::ZeroOrPositive + } else if positive_count == 0 { + Sign::Negative + } else { + Sign::Uncertain + } +} + /// Peels binary operators such as [`BinOpKind::Mul`], [`BinOpKind::Div`] or [`BinOpKind::Rem`], -/// which the result could always be positive under certain conditions, ignoring overflow. +/// where the result depends on: +/// - the number of negative values in the entire expression, or +/// - the number of negative values on the left hand side of the expression. +/// Ignores overflow. +/// /// /// Expressions using other operators are preserved, so we can try to evaluate them later. fn exprs_with_muldiv_binop_peeled<'e>(expr: &'e Expr<'_>) -> Vec<&'e Expr<'e>> { let mut res = vec![]; - for_each_expr(expr, |sub_expr| { + for_each_expr(expr, |sub_expr| -> ControlFlow { // We don't check for mul/div/rem methods here, but we could. if let ExprKind::Binary(op, lhs, _rhs) = sub_expr.kind { if matches!(op.node, BinOpKind::Mul | BinOpKind::Div) { - // For binary operators which both contribute to the sign of the result, + // For binary operators where both sides contribute to the sign of the result, // collect all their operands, recursively. This ignores overflow. ControlFlow::Continue(Descend::Yes) } else if matches!(op.node, BinOpKind::Rem | BinOpKind::Shr) { @@ -259,3 +303,35 @@ fn exprs_with_muldiv_binop_peeled<'e>(expr: &'e Expr<'_>) -> Vec<&'e Expr<'e>> { res } + +/// Peels binary operators such as [`BinOpKind::Add`], where the result depends on: +/// - all the expressions being positive, or +/// - all the expressions being negative. +/// Ignores overflow. +/// +/// Expressions using other operators are preserved, so we can try to evaluate them later. +fn exprs_with_add_binop_peeled<'e>(expr: &'e Expr<'_>) -> Vec<&'e Expr<'e>> { + let mut res = vec![]; + + for_each_expr(expr, |sub_expr| -> ControlFlow { + // We don't check for add methods here, but we could. + if let ExprKind::Binary(op, _lhs, _rhs) = sub_expr.kind { + if matches!(op.node, BinOpKind::Add) { + // For binary operators where both sides contribute to the sign of the result, + // collect all their operands, recursively. This ignores overflow. + ControlFlow::Continue(Descend::Yes) + } else { + // The sign of the result of other binary operators depends on the values of the operands, + // so try to evaluate the expression. + res.push(sub_expr); + ControlFlow::Continue(Descend::No) + } + } else { + // For other expressions, including unary operators and constants, try to evaluate the expression. + res.push(sub_expr); + ControlFlow::Continue(Descend::No) + } + }); + + res +} From f40279ff7dd7e2abaa6d3be3b647a911153c6411 Mon Sep 17 00:00:00 2001 From: teor Date: Wed, 17 Jan 2024 12:55:38 +1000 Subject: [PATCH 16/19] Add some more test cases --- tests/ui/cast.stderr | 6 ------ 1 file changed, 6 deletions(-) diff --git a/tests/ui/cast.stderr b/tests/ui/cast.stderr index aaa6b4183e95..458b2736866b 100644 --- a/tests/ui/cast.stderr +++ b/tests/ui/cast.stderr @@ -480,12 +480,6 @@ error: casting `i32` to `u32` may lose the sign of the value LL | (x * x) as u32; | ^^^^^^^^^^^^^^ -error: casting `i32` to `u32` may lose the sign of the value - --> tests/ui/cast.rs:373:9 - | -LL | x.pow(2) as u32; - | ^^^^^^^^^^^^^^^ - error: casting `i32` to `u32` may lose the sign of the value --> tests/ui/cast.rs:377:32 | From 367a403367566d1ec8d9721c160585f3ebf042d1 Mon Sep 17 00:00:00 2001 From: teor Date: Wed, 17 Jan 2024 12:56:11 +1000 Subject: [PATCH 17/19] Add test coverage for cast_sign_loss changes --- tests/ui/cast.rs | 68 +++++++++++++++++++++++++++++++++++++++--------- 1 file changed, 55 insertions(+), 13 deletions(-) diff --git a/tests/ui/cast.rs b/tests/ui/cast.rs index e9476c80ccb8..6973a574942f 100644 --- a/tests/ui/cast.rs +++ b/tests/ui/cast.rs @@ -116,21 +116,41 @@ fn main() { i64::MAX as u64; i128::MAX as u128; - (-1i8).abs() as u8; - (-1i16).abs() as u16; - (-1i32).abs() as u32; + (-1i8).saturating_abs() as u8; + // abs() can return a negative value in release builds + (i8::MIN).abs() as u8; + //~^ ERROR: casting `i8` to `u8` may lose the sign of the value + (-1i16).saturating_abs() as u16; + (-1i32).saturating_abs() as u32; (-1i64).abs() as u64; (-1isize).abs() as usize; (-1i8).checked_abs().unwrap() as u8; + (i8::MIN).checked_abs().unwrap() as u8; (-1i16).checked_abs().unwrap() as u16; (-1i32).checked_abs().unwrap() as u32; - (-1i64).checked_abs().unwrap() as u64; - (-1isize).checked_abs().unwrap() as usize; + // SAFETY: -1 is a small number which will always return Some + (unsafe { (-1i64).checked_abs().unwrap_unchecked() }) as u64; + (-1isize).checked_abs().expect("-1 is a small number") as usize; + + (-1i8).isqrt() as u8; + (i8::MIN).isqrt() as u8; + (-1i16).isqrt() as u16; + (-1i32).isqrt() as u32; + (-1i64).isqrt() as u64; + (-1isize).isqrt() as usize; + + (-1i8).checked_isqrt().unwrap() as u8; + (i8::MIN).checked_isqrt().unwrap() as u8; + (-1i16).checked_isqrt().unwrap() as u16; + (-1i32).checked_isqrt().unwrap() as u32; + // SAFETY: -1 is a small number which will always return Some + (unsafe { (-1i64).checked_isqrt().unwrap_unchecked() }) as u64; + (-1isize).checked_isqrt().expect("-1 is a small number") as usize; (-1i8).rem_euclid(1i8) as u8; - (-1i8).rem_euclid(1i8) as u16; - (-1i16).rem_euclid(1i16) as u16; + (-1i8).wrapping_rem_euclid(1i8).unwrap() as u16; + (-1i16).rem_euclid(1i16).unwrap() as u16; (-1i16).rem_euclid(1i16) as u32; (-1i32).rem_euclid(1i32) as u32; (-1i32).rem_euclid(1i32) as u64; @@ -138,7 +158,7 @@ fn main() { (-1i64).rem_euclid(1i64) as u128; (-1isize).rem_euclid(1isize) as usize; (1i8).rem_euclid(-1i8) as u8; - (1i8).rem_euclid(-1i8) as u16; + (1i8).wrapping_rem_euclid(-1i8) as u16; (1i16).rem_euclid(-1i16) as u16; (1i16).rem_euclid(-1i16) as u32; (1i32).rem_euclid(-1i32) as u32; @@ -371,14 +391,25 @@ fn issue11642() { let x = x as i32; (x * x) as u32; x.pow(2) as u32; - (-2_i32).pow(2) as u32 + (-2_i32).saturating_pow(2) as u32 } let _a = |x: i32| -> u32 { (x * x * x * x) as u32 }; + (2_i32).checked_pow(3).unwrap() as u32; (-2_i32).pow(3) as u32; //~^ ERROR: casting `i32` to `u32` may lose the sign of the value + (2_i32 % 1) as u32; + (2_i32 % -1) as u32; + (-2_i32 % 1) as u32; + //~^ ERROR: casting `i32` to `u32` may lose the sign of the value + (-2_i32 % -1) as u32; + //~^ ERROR: casting `i32` to `u32` may lose the sign of the value + (2_i32 >> 1) as u32; + (-2_i32 >> 1) 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; @@ -387,12 +418,22 @@ fn issue11642() { 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; + (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; + (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 + (y + y + y + 2) as u16; + //~^ ERROR: casting `i16` to `u16` may lose the sign of the value + + let z: i16 = 2; + (z + -2) as u16; + //~^ ERROR: casting `i16` to `u16` may lose the sign of the value + (z + z + 2) as u16; + fn foo(a: i32, b: i32, c: i32) -> u32 { (a * a * b * b * c * c) as u32; (a * b * c) as u32; @@ -409,8 +450,9 @@ fn issue11642() { //~^ 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; + a.saturating_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 + //~^ ERROR: casting `i32` to `u32` may lose the sign of the value } } From 6bc7c96bb39b4e29c6195e1c24bda1dc97888702 Mon Sep 17 00:00:00 2001 From: teor Date: Wed, 17 Jan 2024 13:02:58 +1000 Subject: [PATCH 18/19] cargo bless --- tests/ui/cast.rs | 13 +-- tests/ui/cast.stderr | 244 ++++++++++++++++++++++++++----------------- 2 files changed, 157 insertions(+), 100 deletions(-) diff --git a/tests/ui/cast.rs b/tests/ui/cast.rs index 6973a574942f..a7331ddc7d0e 100644 --- a/tests/ui/cast.rs +++ b/tests/ui/cast.rs @@ -1,6 +1,7 @@ //@no-rustfix #![feature(repr128)] +#![feature(isqrt)] #![allow(incomplete_features)] #![warn( clippy::cast_precision_loss, @@ -149,8 +150,8 @@ fn main() { (-1isize).checked_isqrt().expect("-1 is a small number") as usize; (-1i8).rem_euclid(1i8) as u8; - (-1i8).wrapping_rem_euclid(1i8).unwrap() as u16; - (-1i16).rem_euclid(1i16).unwrap() as u16; + (-1i8).wrapping_rem_euclid(1i8) as u16; + (-1i16).rem_euclid(1i16) as u16; (-1i16).rem_euclid(1i16) as u32; (-1i32).rem_euclid(1i32) as u32; (-1i32).rem_euclid(1i32) as u64; @@ -400,11 +401,11 @@ fn issue11642() { (-2_i32).pow(3) as u32; //~^ ERROR: casting `i32` to `u32` may lose the sign of the value - (2_i32 % 1) as u32; - (2_i32 % -1) as u32; - (-2_i32 % 1) as u32; + (3_i32 % 2) as u32; + (3_i32 % -2) as u32; + (-5_i32 % 2) as u32; //~^ ERROR: casting `i32` to `u32` may lose the sign of the value - (-2_i32 % -1) as u32; + (-5_i32 % -2) as u32; //~^ ERROR: casting `i32` to `u32` may lose the sign of the value (2_i32 >> 1) as u32; (-2_i32 >> 1) as u32; diff --git a/tests/ui/cast.stderr b/tests/ui/cast.stderr index 458b2736866b..dd5d339b81b2 100644 --- a/tests/ui/cast.stderr +++ b/tests/ui/cast.stderr @@ -1,5 +1,5 @@ error: casting `i32` to `f32` causes a loss of precision (`i32` is 32 bits wide, but `f32`'s mantissa is only 23 bits wide) - --> tests/ui/cast.rs:16:5 + --> tests/ui/cast.rs:17:5 | LL | x0 as f32; | ^^^^^^^^^ @@ -8,37 +8,37 @@ LL | x0 as f32; = help: to override `-D warnings` add `#[allow(clippy::cast_precision_loss)]` error: casting `i64` to `f32` causes a loss of precision (`i64` is 64 bits wide, but `f32`'s mantissa is only 23 bits wide) - --> tests/ui/cast.rs:20:5 + --> tests/ui/cast.rs:21:5 | LL | x1 as f32; | ^^^^^^^^^ error: casting `i64` to `f64` causes a loss of precision (`i64` is 64 bits wide, but `f64`'s mantissa is only 52 bits wide) - --> tests/ui/cast.rs:22:5 + --> tests/ui/cast.rs:23:5 | LL | x1 as f64; | ^^^^^^^^^ error: casting `u32` to `f32` causes a loss of precision (`u32` is 32 bits wide, but `f32`'s mantissa is only 23 bits wide) - --> tests/ui/cast.rs:25:5 + --> tests/ui/cast.rs:26:5 | LL | x2 as f32; | ^^^^^^^^^ error: casting `u64` to `f32` causes a loss of precision (`u64` is 64 bits wide, but `f32`'s mantissa is only 23 bits wide) - --> tests/ui/cast.rs:28:5 + --> tests/ui/cast.rs:29:5 | LL | x3 as f32; | ^^^^^^^^^ error: casting `u64` to `f64` causes a loss of precision (`u64` is 64 bits wide, but `f64`'s mantissa is only 52 bits wide) - --> tests/ui/cast.rs:30:5 + --> tests/ui/cast.rs:31:5 | LL | x3 as f64; | ^^^^^^^^^ error: casting `f32` to `i32` may truncate the value - --> tests/ui/cast.rs:33:5 + --> tests/ui/cast.rs:34:5 | LL | 1f32 as i32; | ^^^^^^^^^^^ @@ -48,7 +48,7 @@ LL | 1f32 as i32; = help: to override `-D warnings` add `#[allow(clippy::cast_possible_truncation)]` error: casting `f32` to `u32` may truncate the value - --> tests/ui/cast.rs:35:5 + --> tests/ui/cast.rs:36:5 | LL | 1f32 as u32; | ^^^^^^^^^^^ @@ -56,7 +56,7 @@ LL | 1f32 as u32; = help: if this is intentional allow the lint with `#[allow(clippy::cast_possible_truncation)]` ... error: casting `f32` to `u32` may lose the sign of the value - --> tests/ui/cast.rs:35:5 + --> tests/ui/cast.rs:36:5 | LL | 1f32 as u32; | ^^^^^^^^^^^ @@ -65,7 +65,7 @@ LL | 1f32 as u32; = help: to override `-D warnings` add `#[allow(clippy::cast_sign_loss)]` error: casting `f64` to `f32` may truncate the value - --> tests/ui/cast.rs:39:5 + --> tests/ui/cast.rs:40:5 | LL | 1f64 as f32; | ^^^^^^^^^^^ @@ -73,7 +73,7 @@ LL | 1f64 as f32; = help: if this is intentional allow the lint with `#[allow(clippy::cast_possible_truncation)]` ... error: casting `i32` to `i8` may truncate the value - --> tests/ui/cast.rs:41:5 + --> tests/ui/cast.rs:42:5 | LL | 1i32 as i8; | ^^^^^^^^^^ @@ -85,7 +85,7 @@ LL | i8::try_from(1i32); | ~~~~~~~~~~~~~~~~~~ error: casting `i32` to `u8` may truncate the value - --> tests/ui/cast.rs:43:5 + --> tests/ui/cast.rs:44:5 | LL | 1i32 as u8; | ^^^^^^^^^^ @@ -97,7 +97,7 @@ LL | u8::try_from(1i32); | ~~~~~~~~~~~~~~~~~~ error: casting `f64` to `isize` may truncate the value - --> tests/ui/cast.rs:45:5 + --> tests/ui/cast.rs:46:5 | LL | 1f64 as isize; | ^^^^^^^^^^^^^ @@ -105,7 +105,7 @@ LL | 1f64 as isize; = help: if this is intentional allow the lint with `#[allow(clippy::cast_possible_truncation)]` ... error: casting `f64` to `usize` may truncate the value - --> tests/ui/cast.rs:47:5 + --> tests/ui/cast.rs:48:5 | LL | 1f64 as usize; | ^^^^^^^^^^^^^ @@ -113,13 +113,13 @@ LL | 1f64 as usize; = help: if this is intentional allow the lint with `#[allow(clippy::cast_possible_truncation)]` ... error: casting `f64` to `usize` may lose the sign of the value - --> tests/ui/cast.rs:47:5 + --> tests/ui/cast.rs:48:5 | LL | 1f64 as usize; | ^^^^^^^^^^^^^ error: casting `u32` to `u16` may truncate the value - --> tests/ui/cast.rs:50:5 + --> tests/ui/cast.rs:51:5 | LL | 1f32 as u32 as u16; | ^^^^^^^^^^^^^^^^^^ @@ -131,7 +131,7 @@ LL | u16::try_from(1f32 as u32); | ~~~~~~~~~~~~~~~~~~~~~~~~~~ error: casting `f32` to `u32` may truncate the value - --> tests/ui/cast.rs:50:5 + --> tests/ui/cast.rs:51:5 | LL | 1f32 as u32 as u16; | ^^^^^^^^^^^ @@ -139,13 +139,13 @@ LL | 1f32 as u32 as u16; = help: if this is intentional allow the lint with `#[allow(clippy::cast_possible_truncation)]` ... error: casting `f32` to `u32` may lose the sign of the value - --> tests/ui/cast.rs:50:5 + --> tests/ui/cast.rs:51:5 | LL | 1f32 as u32 as u16; | ^^^^^^^^^^^ error: casting `i32` to `i8` may truncate the value - --> tests/ui/cast.rs:55:22 + --> tests/ui/cast.rs:56:22 | LL | let _x: i8 = 1i32 as _; | ^^^^^^^^^ @@ -157,7 +157,7 @@ LL | let _x: i8 = 1i32.try_into(); | ~~~~~~~~~~~~~~~ error: casting `f32` to `i32` may truncate the value - --> tests/ui/cast.rs:57:9 + --> tests/ui/cast.rs:58:9 | LL | 1f32 as i32; | ^^^^^^^^^^^ @@ -165,7 +165,7 @@ LL | 1f32 as i32; = help: if this is intentional allow the lint with `#[allow(clippy::cast_possible_truncation)]` ... error: casting `f64` to `i32` may truncate the value - --> tests/ui/cast.rs:59:9 + --> tests/ui/cast.rs:60:9 | LL | 1f64 as i32; | ^^^^^^^^^^^ @@ -173,7 +173,7 @@ LL | 1f64 as i32; = help: if this is intentional allow the lint with `#[allow(clippy::cast_possible_truncation)]` ... error: casting `f32` to `u8` may truncate the value - --> tests/ui/cast.rs:61:9 + --> tests/ui/cast.rs:62:9 | LL | 1f32 as u8; | ^^^^^^^^^^ @@ -181,13 +181,13 @@ LL | 1f32 as u8; = help: if this is intentional allow the lint with `#[allow(clippy::cast_possible_truncation)]` ... error: casting `f32` to `u8` may lose the sign of the value - --> tests/ui/cast.rs:61:9 + --> tests/ui/cast.rs:62:9 | LL | 1f32 as u8; | ^^^^^^^^^^ error: casting `u8` to `i8` may wrap around the value - --> tests/ui/cast.rs:66:5 + --> tests/ui/cast.rs:67:5 | LL | 1u8 as i8; | ^^^^^^^^^ @@ -196,31 +196,31 @@ LL | 1u8 as i8; = help: to override `-D warnings` add `#[allow(clippy::cast_possible_wrap)]` error: casting `u16` to `i16` may wrap around the value - --> tests/ui/cast.rs:69:5 + --> tests/ui/cast.rs:70:5 | LL | 1u16 as i16; | ^^^^^^^^^^^ error: casting `u32` to `i32` may wrap around the value - --> tests/ui/cast.rs:71:5 + --> tests/ui/cast.rs:72:5 | LL | 1u32 as i32; | ^^^^^^^^^^^ error: casting `u64` to `i64` may wrap around the value - --> tests/ui/cast.rs:73:5 + --> tests/ui/cast.rs:74:5 | LL | 1u64 as i64; | ^^^^^^^^^^^ error: casting `usize` to `isize` may wrap around the value - --> tests/ui/cast.rs:75:5 + --> tests/ui/cast.rs:76:5 | LL | 1usize as isize; | ^^^^^^^^^^^^^^^ error: casting `usize` to `i8` may truncate the value - --> tests/ui/cast.rs:78:5 + --> tests/ui/cast.rs:79:5 | LL | 1usize as i8; | ^^^^^^^^^^^^ @@ -232,7 +232,7 @@ LL | i8::try_from(1usize); | ~~~~~~~~~~~~~~~~~~~~ error: casting `usize` to `i16` may truncate the value - --> tests/ui/cast.rs:81:5 + --> tests/ui/cast.rs:82:5 | LL | 1usize as i16; | ^^^^^^^^^^^^^ @@ -244,7 +244,7 @@ LL | i16::try_from(1usize); | ~~~~~~~~~~~~~~~~~~~~~ error: casting `usize` to `i16` may wrap around the value on targets with 16-bit wide pointers - --> tests/ui/cast.rs:81:5 + --> tests/ui/cast.rs:82:5 | LL | 1usize as i16; | ^^^^^^^^^^^^^ @@ -253,7 +253,7 @@ LL | 1usize as i16; = note: for more information see https://doc.rust-lang.org/reference/types/numeric.html#machine-dependent-integer-types error: casting `usize` to `i32` may truncate the value on targets with 64-bit wide pointers - --> tests/ui/cast.rs:86:5 + --> tests/ui/cast.rs:87:5 | LL | 1usize as i32; | ^^^^^^^^^^^^^ @@ -265,19 +265,19 @@ LL | i32::try_from(1usize); | ~~~~~~~~~~~~~~~~~~~~~ error: casting `usize` to `i32` may wrap around the value on targets with 32-bit wide pointers - --> tests/ui/cast.rs:86:5 + --> tests/ui/cast.rs:87:5 | LL | 1usize as i32; | ^^^^^^^^^^^^^ error: casting `usize` to `i64` may wrap around the value on targets with 64-bit wide pointers - --> tests/ui/cast.rs:90:5 + --> tests/ui/cast.rs:91:5 | LL | 1usize as i64; | ^^^^^^^^^^^^^ error: casting `u16` to `isize` may wrap around the value on targets with 16-bit wide pointers - --> tests/ui/cast.rs:95:5 + --> tests/ui/cast.rs:96:5 | LL | 1u16 as isize; | ^^^^^^^^^^^^^ @@ -286,13 +286,13 @@ LL | 1u16 as isize; = note: for more information see https://doc.rust-lang.org/reference/types/numeric.html#machine-dependent-integer-types error: casting `u32` to `isize` may wrap around the value on targets with 32-bit wide pointers - --> tests/ui/cast.rs:99:5 + --> tests/ui/cast.rs:100:5 | LL | 1u32 as isize; | ^^^^^^^^^^^^^ error: casting `u64` to `isize` may truncate the value on targets with 32-bit wide pointers - --> tests/ui/cast.rs:102:5 + --> tests/ui/cast.rs:103:5 | LL | 1u64 as isize; | ^^^^^^^^^^^^^ @@ -304,55 +304,55 @@ LL | isize::try_from(1u64); | ~~~~~~~~~~~~~~~~~~~~~ error: casting `u64` to `isize` may wrap around the value on targets with 64-bit wide pointers - --> tests/ui/cast.rs:102:5 + --> tests/ui/cast.rs:103:5 | LL | 1u64 as isize; | ^^^^^^^^^^^^^ error: casting `i32` to `u32` may lose the sign of the value - --> tests/ui/cast.rs:107:5 + --> tests/ui/cast.rs:108:5 | LL | -1i32 as u32; | ^^^^^^^^^^^^ error: casting `isize` to `usize` may lose the sign of the value - --> tests/ui/cast.rs:110:5 + --> tests/ui/cast.rs:111:5 | LL | -1isize as usize; | ^^^^^^^^^^^^^^^^ error: casting `i8` to `u8` may lose the sign of the value - --> tests/ui/cast.rs:119:5 - | -LL | (-1i8).abs() as u8; - | ^^^^^^^^^^^^^^^^^^ - -error: casting `i16` to `u16` may lose the sign of the value - --> tests/ui/cast.rs:120:5 - | -LL | (-1i16).abs() as u16; - | ^^^^^^^^^^^^^^^^^^^^ - -error: casting `i32` to `u32` may lose the sign of the value - --> tests/ui/cast.rs:121:5 + --> tests/ui/cast.rs:122:5 | -LL | (-1i32).abs() as u32; - | ^^^^^^^^^^^^^^^^^^^^ +LL | (i8::MIN).abs() as u8; + | ^^^^^^^^^^^^^^^^^^^^^ error: casting `i64` to `u64` may lose the sign of the value - --> tests/ui/cast.rs:122:5 + --> tests/ui/cast.rs:126:5 | LL | (-1i64).abs() as u64; | ^^^^^^^^^^^^^^^^^^^^ error: casting `isize` to `usize` may lose the sign of the value - --> tests/ui/cast.rs:123:5 + --> tests/ui/cast.rs:127:5 | LL | (-1isize).abs() as usize; | ^^^^^^^^^^^^^^^^^^^^^^^^ +error: casting `i64` to `u64` may lose the sign of the value + --> tests/ui/cast.rs:134:5 + | +LL | (unsafe { (-1i64).checked_abs().unwrap_unchecked() }) as u64; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +error: casting `i64` to `u64` may lose the sign of the value + --> tests/ui/cast.rs:149:5 + | +LL | (unsafe { (-1i64).checked_isqrt().unwrap_unchecked() }) as u64; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + error: casting `i64` to `i8` may truncate the value - --> tests/ui/cast.rs:179:5 + --> tests/ui/cast.rs:200:5 | LL | (-99999999999i64).min(1) as i8; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ @@ -364,7 +364,7 @@ LL | i8::try_from((-99999999999i64).min(1)); | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ error: casting `u64` to `u8` may truncate the value - --> tests/ui/cast.rs:193:5 + --> tests/ui/cast.rs:214:5 | LL | 999999u64.clamp(0, 256) as u8; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ @@ -376,7 +376,7 @@ LL | u8::try_from(999999u64.clamp(0, 256)); | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ error: casting `main::E2` to `u8` may truncate the value - --> tests/ui/cast.rs:216:21 + --> tests/ui/cast.rs:237:21 | LL | let _ = self as u8; | ^^^^^^^^^^ @@ -388,7 +388,7 @@ LL | let _ = u8::try_from(self); | ~~~~~~~~~~~~~~~~~~ error: casting `main::E2::B` to `u8` will truncate the value - --> tests/ui/cast.rs:218:21 + --> tests/ui/cast.rs:239:21 | LL | let _ = Self::B as u8; | ^^^^^^^^^^^^^ @@ -397,7 +397,7 @@ LL | let _ = Self::B as u8; = help: to override `-D warnings` add `#[allow(clippy::cast_enum_truncation)]` error: casting `main::E5` to `i8` may truncate the value - --> tests/ui/cast.rs:260:21 + --> tests/ui/cast.rs:281:21 | LL | let _ = self as i8; | ^^^^^^^^^^ @@ -409,13 +409,13 @@ LL | let _ = i8::try_from(self); | ~~~~~~~~~~~~~~~~~~ error: casting `main::E5::A` to `i8` will truncate the value - --> tests/ui/cast.rs:262:21 + --> tests/ui/cast.rs:283:21 | LL | let _ = Self::A as i8; | ^^^^^^^^^^^^^ error: casting `main::E6` to `i16` may truncate the value - --> tests/ui/cast.rs:279:21 + --> tests/ui/cast.rs:300:21 | LL | let _ = self as i16; | ^^^^^^^^^^^ @@ -427,7 +427,7 @@ LL | let _ = i16::try_from(self); | ~~~~~~~~~~~~~~~~~~~ error: casting `main::E7` to `usize` may truncate the value on targets with 32-bit wide pointers - --> tests/ui/cast.rs:298:21 + --> tests/ui/cast.rs:319:21 | LL | let _ = self as usize; | ^^^^^^^^^^^^^ @@ -439,7 +439,7 @@ LL | let _ = usize::try_from(self); | ~~~~~~~~~~~~~~~~~~~~~ error: casting `main::E10` to `u16` may truncate the value - --> tests/ui/cast.rs:345:21 + --> tests/ui/cast.rs:366:21 | LL | let _ = self as u16; | ^^^^^^^^^^^ @@ -451,7 +451,7 @@ LL | let _ = u16::try_from(self); | ~~~~~~~~~~~~~~~~~~~ error: casting `u32` to `u8` may truncate the value - --> tests/ui/cast.rs:356:13 + --> tests/ui/cast.rs:377:13 | LL | let c = (q >> 16) as u8; | ^^^^^^^^^^^^^^^ @@ -463,7 +463,7 @@ LL | let c = u8::try_from(q >> 16); | ~~~~~~~~~~~~~~~~~~~~~ error: casting `u32` to `u8` may truncate the value - --> tests/ui/cast.rs:360:13 + --> tests/ui/cast.rs:381:13 | LL | let c = (q / 1000) as u8; | ^^^^^^^^^^^^^^^^ @@ -475,124 +475,180 @@ LL | let c = u8::try_from(q / 1000); | ~~~~~~~~~~~~~~~~~~~~~~ error: casting `i32` to `u32` may lose the sign of the value - --> tests/ui/cast.rs:372:9 + --> tests/ui/cast.rs:393:9 | LL | (x * x) as u32; | ^^^^^^^^^^^^^^ error: casting `i32` to `u32` may lose the sign of the value - --> tests/ui/cast.rs:377:32 + --> tests/ui/cast.rs:398:32 | LL | let _a = |x: i32| -> u32 { (x * x * x * x) as u32 }; | ^^^^^^^^^^^^^^^^^^^^^^ error: casting `i32` to `u32` may lose the sign of the value - --> tests/ui/cast.rs:379:5 + --> tests/ui/cast.rs:400:5 + | +LL | (2_i32).checked_pow(3).unwrap() as u32; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +error: casting `i32` to `u32` may lose the sign of the value + --> tests/ui/cast.rs:401:5 | LL | (-2_i32).pow(3) as u32; | ^^^^^^^^^^^^^^^^^^^^^^ error: casting `i32` to `u32` may lose the sign of the value - --> tests/ui/cast.rs:383:5 + --> tests/ui/cast.rs:406:5 + | +LL | (-5_i32 % 2) as u32; + | ^^^^^^^^^^^^^^^^^^^ + +error: casting `i32` to `u32` may lose the sign of the value + --> tests/ui/cast.rs:408:5 + | +LL | (-5_i32 % -2) as u32; + | ^^^^^^^^^^^^^^^^^^^^ + +error: casting `i32` to `u32` may lose the sign of the value + --> tests/ui/cast.rs:411:5 + | +LL | (-2_i32 >> 1) as u32; + | ^^^^^^^^^^^^^^^^^^^^ + +error: casting `i32` to `u32` may lose the sign of the value + --> tests/ui/cast.rs:415:5 | LL | (x * x) as u32; | ^^^^^^^^^^^^^^ error: casting `i32` to `u32` may lose the sign of the value - --> tests/ui/cast.rs:384:5 + --> tests/ui/cast.rs:416:5 | LL | (x * x * x) as u32; | ^^^^^^^^^^^^^^^^^^ error: casting `i16` to `u16` may lose the sign of the value - --> tests/ui/cast.rs:388:5 + --> tests/ui/cast.rs:420:5 | LL | (y * y * y * y * -2) as u16; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ error: casting `i16` to `u16` may lose the sign of the value - --> tests/ui/cast.rs:390:5 + --> tests/ui/cast.rs:422:5 | -LL | (y * y * y * y * 2) as u16; +LL | (y * y * y / y * 2) as u16; | ^^^^^^^^^^^^^^^^^^^^^^^^^^ error: casting `i16` to `u16` may lose the sign of the value - --> tests/ui/cast.rs:391:5 + --> tests/ui/cast.rs:423:5 | -LL | (y * y * y * 2) as u16; +LL | (y * y / y * 2) as u16; | ^^^^^^^^^^^^^^^^^^^^^^ error: casting `i16` to `u16` may lose the sign of the value - --> tests/ui/cast.rs:393:5 + --> tests/ui/cast.rs:425:5 | -LL | (y * y * y * -2) as u16; +LL | (y / y * y * -2) as u16; | ^^^^^^^^^^^^^^^^^^^^^^^ +error: equal expressions as operands to `/` + --> tests/ui/cast.rs:425:6 + | +LL | (y / y * y * -2) as u16; + | ^^^^^ + | + = note: `#[deny(clippy::eq_op)]` on by default + +error: casting `i16` to `u16` may lose the sign of the value + --> tests/ui/cast.rs:428:5 + | +LL | (y + y + y + -2) as u16; + | ^^^^^^^^^^^^^^^^^^^^^^^ + +error: casting `i16` to `u16` may lose the sign of the value + --> tests/ui/cast.rs:430:5 + | +LL | (y + y + y + 2) as u16; + | ^^^^^^^^^^^^^^^^^^^^^^ + +error: casting `i16` to `u16` may lose the sign of the value + --> tests/ui/cast.rs:434:5 + | +LL | (z + -2) as u16; + | ^^^^^^^^^^^^^^^ + +error: casting `i16` to `u16` may lose the sign of the value + --> tests/ui/cast.rs:436:5 + | +LL | (z + z + 2) as u16; + | ^^^^^^^^^^^^^^^^^^ + error: casting `i32` to `u32` may lose the sign of the value - --> tests/ui/cast.rs:397:9 + --> tests/ui/cast.rs:439:9 | LL | (a * a * b * b * c * c) as u32; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ error: casting `i32` to `u32` may lose the sign of the value - --> tests/ui/cast.rs:398:9 + --> tests/ui/cast.rs:440:9 | LL | (a * b * c) as u32; | ^^^^^^^^^^^^^^^^^^ error: casting `i32` to `u32` may lose the sign of the value - --> tests/ui/cast.rs:400:9 + --> tests/ui/cast.rs:442:9 | LL | (a * -b * c) as u32; | ^^^^^^^^^^^^^^^^^^^ error: casting `i32` to `u32` may lose the sign of the value - --> tests/ui/cast.rs:402:9 + --> tests/ui/cast.rs:444:9 | LL | (a * b * c * c) as u32; | ^^^^^^^^^^^^^^^^^^^^^^ error: casting `i32` to `u32` may lose the sign of the value - --> tests/ui/cast.rs:403:9 + --> tests/ui/cast.rs:445:9 | LL | (a * -2) as u32; | ^^^^^^^^^^^^^^^ error: casting `i32` to `u32` may lose the sign of the value - --> tests/ui/cast.rs:405:9 + --> tests/ui/cast.rs:447:9 | LL | (a * b * c * -2) as u32; | ^^^^^^^^^^^^^^^^^^^^^^^ error: casting `i32` to `u32` may lose the sign of the value - --> tests/ui/cast.rs:407:9 + --> tests/ui/cast.rs:449:9 | LL | (a / b) as u32; | ^^^^^^^^^^^^^^ error: casting `i32` to `u32` may lose the sign of the value - --> tests/ui/cast.rs:408:9 + --> tests/ui/cast.rs:450:9 | LL | (a / b * c) as u32; | ^^^^^^^^^^^^^^^^^^ error: casting `i32` to `u32` may lose the sign of the value - --> tests/ui/cast.rs:410:9 + --> tests/ui/cast.rs:452:9 | LL | (a / b + b * c) as u32; | ^^^^^^^^^^^^^^^^^^^^^^ error: casting `i32` to `u32` may lose the sign of the value - --> tests/ui/cast.rs:412:9 + --> tests/ui/cast.rs:454:9 | -LL | a.pow(3) as u32; - | ^^^^^^^^^^^^^^^ +LL | a.saturating_pow(3) as u32; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^ error: casting `i32` to `u32` may lose the sign of the value - --> tests/ui/cast.rs:414:9 + --> tests/ui/cast.rs:456:9 | LL | (a.abs() * b.pow(2) / c.abs()) as u32 | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ -error: aborting due to 76 previous errors +error: aborting due to 85 previous errors From 1e3c55eea2c61bd82d2ca356442291797b99d5be Mon Sep 17 00:00:00 2001 From: teor Date: Tue, 27 Feb 2024 07:34:18 +1000 Subject: [PATCH 19/19] Remove redundant uncertain_counts --- clippy_lints/src/casts/cast_sign_loss.rs | 23 ++++++++++------------- 1 file changed, 10 insertions(+), 13 deletions(-) diff --git a/clippy_lints/src/casts/cast_sign_loss.rs b/clippy_lints/src/casts/cast_sign_loss.rs index 546cfca21566..8fd95d9654cf 100644 --- a/clippy_lints/src/casts/cast_sign_loss.rs +++ b/clippy_lints/src/casts/cast_sign_loss.rs @@ -192,7 +192,6 @@ fn pow_call_result_sign(cx: &LateContext<'_>, base: &Expr<'_>, exponent: &Expr<' /// Returns the sign of the list of peeled expressions. fn expr_muldiv_sign(cx: &LateContext<'_>, expr: &Expr<'_>) -> Sign { let mut negative_count = 0; - let mut uncertain_count = 0; // Peel off possible binary expressions, for example: // x * x / y => [x, x, y] @@ -201,18 +200,17 @@ fn expr_muldiv_sign(cx: &LateContext<'_>, expr: &Expr<'_>) -> Sign { for expr in exprs { match expr_sign(cx, expr, None) { Sign::Negative => negative_count += 1, - Sign::Uncertain => uncertain_count += 1, + // A mul/div is: + // - uncertain if there are any uncertain values (because they could be negative or positive), + Sign::Uncertain => return Sign::Uncertain, Sign::ZeroOrPositive => (), }; } // A mul/div is: - // - uncertain if there are any uncertain values (because they could be negative or positive), // - negative if there are an odd number of negative values, // - positive or zero otherwise. - if uncertain_count > 0 { - Sign::Uncertain - } else if negative_count % 2 == 1 { + if negative_count % 2 == 1 { Sign::Negative } else { Sign::ZeroOrPositive @@ -225,7 +223,6 @@ fn expr_muldiv_sign(cx: &LateContext<'_>, expr: &Expr<'_>) -> Sign { /// Returns the sign of the list of peeled expressions. fn expr_add_sign(cx: &LateContext<'_>, expr: &Expr<'_>) -> Sign { let mut negative_count = 0; - let mut uncertain_count = 0; let mut positive_count = 0; // Peel off possible binary expressions, for example: @@ -234,19 +231,19 @@ fn expr_add_sign(cx: &LateContext<'_>, expr: &Expr<'_>) -> Sign { for expr in exprs { match expr_sign(cx, expr, None) { Sign::Negative => negative_count += 1, - Sign::Uncertain => uncertain_count += 1, + // A sum is: + // - uncertain if there are any uncertain values (because they could be negative or positive), + Sign::Uncertain => return Sign::Uncertain, Sign::ZeroOrPositive => positive_count += 1, }; } // A sum is: - // - uncertain if there are any uncertain values (because they could be negative or positive), // - positive or zero if there are only positive (or zero) values, - // - negative if there are only negative (or zero) values. + // - negative if there are only negative (or zero) values, or + // - uncertain if there are both. // We could split Zero out into its own variant, but we don't yet. - if uncertain_count > 0 { - Sign::Uncertain - } else if negative_count == 0 { + if negative_count == 0 { Sign::ZeroOrPositive } else if positive_count == 0 { Sign::Negative