diff --git a/CHANGELOG.md b/CHANGELOG.md index 30781d3d33fb..d3c30065e866 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6759,6 +6759,7 @@ Released 2018-09-13 [`unsafe_removed_from_name`]: https://rust-lang.github.io/rust-clippy/master/index.html#unsafe_removed_from_name [`unsafe_vector_initialization`]: https://rust-lang.github.io/rust-clippy/master/index.html#unsafe_vector_initialization [`unseparated_literal_suffix`]: https://rust-lang.github.io/rust-clippy/master/index.html#unseparated_literal_suffix +[`unsigned_subtraction_gt_zero`]: https://rust-lang.github.io/rust-clippy/master/index.html#unsigned_subtraction_gt_zero [`unsound_collection_transmute`]: https://rust-lang.github.io/rust-clippy/master/index.html#unsound_collection_transmute [`unstable_as_mut_slice`]: https://rust-lang.github.io/rust-clippy/master/index.html#unstable_as_mut_slice [`unstable_as_slice`]: https://rust-lang.github.io/rust-clippy/master/index.html#unstable_as_slice diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs index 0ec0aaaad453..01de75f442cd 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/clippy_lints/src/declared_lints.rs @@ -600,6 +600,7 @@ pub static LINTS: &[&::declare_clippy_lint::LintInfo] = &[ crate::operators::OP_REF_INFO, crate::operators::REDUNDANT_COMPARISONS_INFO, crate::operators::SELF_ASSIGNMENT_INFO, + crate::operators::UNSIGNED_SUBTRACTION_GT_ZERO_INFO, crate::operators::VERBOSE_BIT_MASK_INFO, crate::option_env_unwrap::OPTION_ENV_UNWRAP_INFO, crate::option_if_let_else::OPTION_IF_LET_ELSE_INFO, diff --git a/clippy_lints/src/operators/mod.rs b/clippy_lints/src/operators/mod.rs index aaea4ff11fc3..3feaeb267057 100644 --- a/clippy_lints/src/operators/mod.rs +++ b/clippy_lints/src/operators/mod.rs @@ -20,6 +20,7 @@ mod needless_bitwise_bool; mod numeric_arithmetic; mod op_ref; mod self_assignment; +mod unsigned_subtraction_gt_zero; mod verbose_bit_mask; pub(crate) mod arithmetic_side_effects; @@ -62,6 +63,34 @@ declare_clippy_lint! { "a comparison with a maximum or minimum value that is always true or false" } +declare_clippy_lint! { + /// ### What it does + /// Lints comparisons of an unsigned integer subtraction against zero, like `(a - b) > 0`. + /// + /// ### Why is this bad? + /// If `a < b`, `a - b` will panic in debug builds and wrap in release builds, making the + /// comparison effectively behave like `a != b`. This is likely unintended; `a > b` is clearer + /// and avoids potential panics and wraparound. + /// + /// ### Example + /// ```no_run + /// let (a, b): (u32, u32) = (1, 2); + /// if a - b > 0 {} + /// ``` + /// + /// Use instead: + /// ```no_run + /// let (a, b): (u32, u32) = (1, 2); + /// if a > b {} + /// // Or, if you meant inequality: + /// if a != b {} + /// ``` + #[clippy::version = "1.92.0"] + pub UNSIGNED_SUBTRACTION_GT_ZERO, + correctness, + "suspicious comparison of unsigned subtraction to zero" +} + declare_clippy_lint! { /// ### What it does /// Checks any kind of arithmetic operation of any type. @@ -906,6 +935,7 @@ impl_lint_pass!(Operators => [ SELF_ASSIGNMENT, MANUAL_MIDPOINT, MANUAL_IS_MULTIPLE_OF, + UNSIGNED_SUBTRACTION_GT_ZERO, ]); impl<'tcx> LateLintPass<'tcx> for Operators { @@ -932,6 +962,7 @@ impl<'tcx> LateLintPass<'tcx> for Operators { const_comparisons::check(cx, op, lhs, rhs, e.span); duration_subsec::check(cx, e, op.node, lhs, rhs); float_equality_without_abs::check(cx, e, op.node, lhs, rhs); + unsigned_subtraction_gt_zero::check(cx, e, op.node, lhs, rhs); integer_division::check(cx, e, op.node, lhs, rhs); cmp_owned::check(cx, op.node, lhs, rhs); float_cmp::check(cx, e, op.node, lhs, rhs); diff --git a/clippy_lints/src/operators/unsigned_subtraction_gt_zero.rs b/clippy_lints/src/operators/unsigned_subtraction_gt_zero.rs new file mode 100644 index 000000000000..b6f565d2b928 --- /dev/null +++ b/clippy_lints/src/operators/unsigned_subtraction_gt_zero.rs @@ -0,0 +1,64 @@ +use clippy_utils::consts::is_zero_integer_const; +use clippy_utils::diagnostics::span_lint_and_then; +use clippy_utils::sugg::Sugg; +use rustc_errors::Applicability; +use rustc_hir::{BinOpKind, Expr, ExprKind}; +use rustc_lint::LateContext; +use rustc_middle::ty; + +use super::UNSIGNED_SUBTRACTION_GT_ZERO; + +pub(super) fn check<'tcx>( + cx: &LateContext<'tcx>, + expr: &'tcx Expr<'tcx>, + op: BinOpKind, + lhs_expr: &'tcx Expr<'tcx>, + rhs_expr: &'tcx Expr<'tcx>, +) { + // Avoid linting macro-generated code to reduce noise + if expr.span.from_expansion() { + return; + } + + // Only consider strict relational comparisons where one side is zero and the other is a subtraction + let sub_expr = match op { + // x > 0 + BinOpKind::Gt if is_zero_integer_const(cx, rhs_expr, expr.span.ctxt()) => lhs_expr, + // 0 < x + BinOpKind::Lt if is_zero_integer_const(cx, lhs_expr, expr.span.ctxt()) => rhs_expr, + + _ => return, + }; + + // Ensure the compared expression is a subtraction + let (lhs, rhs) = match sub_expr.kind { + ExprKind::Binary(sub_op, lhs, rhs) if sub_op.node == BinOpKind::Sub => (lhs, rhs), + _ => return, + }; + + // Subtraction result type must be an unsigned primitive + if !matches!(cx.typeck_results().expr_ty(sub_expr).peel_refs().kind(), ty::Uint(_)) { + return; + } + + // Suggest `a > b` preserving user formatting with parentheses as needed + let mut app = Applicability::MaybeIncorrect; + let (left_sugg, right_sugg) = ( + Sugg::hir_with_applicability(cx, lhs, "_", &mut app).maybe_paren(), + Sugg::hir_with_applicability(cx, rhs, "_", &mut app).maybe_paren(), + ); + let replacement = format!("{left_sugg} > {right_sugg}"); + let neq_suggestion = format!("{left_sugg} != {right_sugg}"); + + span_lint_and_then( + cx, + UNSIGNED_SUBTRACTION_GT_ZERO, + expr.span, + "suspicious comparison of unsigned subtraction to zero", + |diag| { + diag.help(format!("`{left_sugg} - {right_sugg} > 0` will panic in debug mode when `{left_sugg} < {right_sugg}` and wrap in release mode; `{left_sugg} > {right_sugg}` is clearer and will never panic")); + diag.help(format!("if you meant inequality, use `{neq_suggestion}`")); + diag.span_suggestion(expr.span, "try", replacement, app); + }, + ); +} diff --git a/tests/ui/unsigned_subtraction_gt_zero.fixed b/tests/ui/unsigned_subtraction_gt_zero.fixed new file mode 100644 index 000000000000..6698956becc0 --- /dev/null +++ b/tests/ui/unsigned_subtraction_gt_zero.fixed @@ -0,0 +1,29 @@ +#![warn(clippy::unsigned_subtraction_gt_zero)] +#![allow(clippy::needless_if)] + +fn main() { + let (a, b): (u32, u32) = (1, 2); + if a > b {} + //~^ unsigned_subtraction_gt_zero + + if a > b {} + //~^ unsigned_subtraction_gt_zero + + let (x, y): (usize, usize) = (10, 3); + if x > y {} + //~^ unsigned_subtraction_gt_zero + + if x > y {} + //~^ unsigned_subtraction_gt_zero + + // signed: should not lint + let (i, j): (i32, i32) = (1, 2); + if i - j > 0 {} + + // float: should not lint + let (f, g): (f32, f32) = (1.0, 2.0); + if f - g > 0.0 {} + + // using saturating_sub: should not lint + if a.saturating_sub(b) > 0 {} +} diff --git a/tests/ui/unsigned_subtraction_gt_zero.rs b/tests/ui/unsigned_subtraction_gt_zero.rs new file mode 100644 index 000000000000..0dda9274fe28 --- /dev/null +++ b/tests/ui/unsigned_subtraction_gt_zero.rs @@ -0,0 +1,29 @@ +#![warn(clippy::unsigned_subtraction_gt_zero)] +#![allow(clippy::needless_if)] + +fn main() { + let (a, b): (u32, u32) = (1, 2); + if a - b > 0 {} + //~^ unsigned_subtraction_gt_zero + + if 0 < a - b {} + //~^ unsigned_subtraction_gt_zero + + let (x, y): (usize, usize) = (10, 3); + if x - y > 0 {} + //~^ unsigned_subtraction_gt_zero + + if 0 < x - y {} + //~^ unsigned_subtraction_gt_zero + + // signed: should not lint + let (i, j): (i32, i32) = (1, 2); + if i - j > 0 {} + + // float: should not lint + let (f, g): (f32, f32) = (1.0, 2.0); + if f - g > 0.0 {} + + // using saturating_sub: should not lint + if a.saturating_sub(b) > 0 {} +} diff --git a/tests/ui/unsigned_subtraction_gt_zero.stderr b/tests/ui/unsigned_subtraction_gt_zero.stderr new file mode 100644 index 000000000000..1bd227dcd885 --- /dev/null +++ b/tests/ui/unsigned_subtraction_gt_zero.stderr @@ -0,0 +1,40 @@ +error: suspicious comparison of unsigned subtraction to zero + --> tests/ui/unsigned_subtraction_gt_zero.rs:6:8 + | +LL | if a - b > 0 {} + | ^^^^^^^^^ help: try: `a > b` + | + = help: `a - b > 0` will panic in debug mode when `a < b` and wrap in release mode; `a > b` is clearer and will never panic + = help: if you meant inequality, use `a != b` + = note: `-D clippy::unsigned-subtraction-gt-zero` implied by `-D warnings` + = help: to override `-D warnings` add `#[allow(clippy::unsigned_subtraction_gt_zero)]` + +error: suspicious comparison of unsigned subtraction to zero + --> tests/ui/unsigned_subtraction_gt_zero.rs:9:8 + | +LL | if 0 < a - b {} + | ^^^^^^^^^ help: try: `a > b` + | + = help: `a - b > 0` will panic in debug mode when `a < b` and wrap in release mode; `a > b` is clearer and will never panic + = help: if you meant inequality, use `a != b` + +error: suspicious comparison of unsigned subtraction to zero + --> tests/ui/unsigned_subtraction_gt_zero.rs:13:8 + | +LL | if x - y > 0 {} + | ^^^^^^^^^ help: try: `x > y` + | + = help: `x - y > 0` will panic in debug mode when `x < y` and wrap in release mode; `x > y` is clearer and will never panic + = help: if you meant inequality, use `x != y` + +error: suspicious comparison of unsigned subtraction to zero + --> tests/ui/unsigned_subtraction_gt_zero.rs:16:8 + | +LL | if 0 < x - y {} + | ^^^^^^^^^ help: try: `x > y` + | + = help: `x - y > 0` will panic in debug mode when `x < y` and wrap in release mode; `x > y` is clearer and will never panic + = help: if you meant inequality, use `x != y` + +error: aborting due to 4 previous errors +