refactor(invalid_upcast_comparisons): move to under operators, simplify (#15956)

samueltardieu · web-flow · commit d74200a6feef · 2025-10-28T19:41:35.000Z
The last two commits could arguably be split off into a separate PR --
let me know if that's desired.

changelog: none
diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs
@@ -228,7 +228,6 @@ pub static LINTS: &[&::declare_clippy_lint::LintInfo] = &[
     crate::inline_fn_without_body::INLINE_FN_WITHOUT_BODY_INFO,
     crate::int_plus_one::INT_PLUS_ONE_INFO,
     crate::integer_division_remainder_used::INTEGER_DIVISION_REMAINDER_USED_INFO,
-    crate::invalid_upcast_comparisons::INVALID_UPCAST_COMPARISONS_INFO,
     crate::item_name_repetitions::ENUM_VARIANT_NAMES_INFO,
     crate::item_name_repetitions::MODULE_INCEPTION_INFO,
     crate::item_name_repetitions::MODULE_NAME_REPETITIONS_INFO,
@@ -591,6 +590,7 @@ pub static LINTS: &[&::declare_clippy_lint::LintInfo] = &[
     crate::operators::IMPOSSIBLE_COMPARISONS_INFO,
     crate::operators::INEFFECTIVE_BIT_MASK_INFO,
     crate::operators::INTEGER_DIVISION_INFO,
+    crate::operators::INVALID_UPCAST_COMPARISONS_INFO,
     crate::operators::MANUAL_DIV_CEIL_INFO,
     crate::operators::MANUAL_IS_MULTIPLE_OF_INFO,
     crate::operators::MANUAL_MIDPOINT_INFO,
diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs
@@ -172,7 +172,6 @@ mod init_numbered_fields;
 mod inline_fn_without_body;
 mod int_plus_one;
 mod integer_division_remainder_used;
-mod invalid_upcast_comparisons;
 mod item_name_repetitions;
 mod items_after_statements;
 mod items_after_test_module;
@@ -541,7 +540,6 @@ pub fn register_lint_passes(store: &mut rustc_lint::LintStore, conf: &'static Co
     store.register_late_pass(move |_| Box::new(derivable_impls::DerivableImpls::new(conf)));
     store.register_late_pass(|_| Box::new(drop_forget_ref::DropForgetRef));
     store.register_late_pass(|_| Box::new(empty_enums::EmptyEnums));
-    store.register_late_pass(|_| Box::new(invalid_upcast_comparisons::InvalidUpcastComparisons));
     store.register_late_pass(|_| Box::<regex::Regex>::default());
     store.register_late_pass(move |tcx| Box::new(ifs::CopyAndPaste::new(tcx, conf)));
     store.register_late_pass(|_| Box::new(copy_iterator::CopyIterator));
diff --git a/clippy_lints/src/operators/invalid_upcast_comparisons.rs b/clippy_lints/src/operators/invalid_upcast_comparisons.rs
@@ -1,9 +1,8 @@
 use rustc_errors::Applicability;
-use rustc_hir::{Expr, ExprKind};
-use rustc_lint::{LateContext, LateLintPass};
+use rustc_hir::{BinOpKind, Expr, ExprKind};
+use rustc_lint::LateContext;
 use rustc_middle::ty::layout::LayoutOf;
 use rustc_middle::ty::{self, IntTy, UintTy};
-use rustc_session::declare_lint_pass;
 use rustc_span::Span;
 
 use clippy_utils::comparisons;
@@ -12,29 +11,26 @@ use clippy_utils::consts::{ConstEvalCtxt, FullInt};
 use clippy_utils::diagnostics::span_lint;
 use clippy_utils::source::snippet_with_context;
 
-declare_clippy_lint! {
-    /// ### What it does
-    /// Checks for comparisons where the relation is always either
-    /// true or false, but where one side has been upcast so that the comparison is
-    /// necessary. Only integer types are checked.
-    ///
-    /// ### Why is this bad?
-    /// An expression like `let x : u8 = ...; (x as u32) > 300`
-    /// will mistakenly imply that it is possible for `x` to be outside the range of
-    /// `u8`.
-    ///
-    /// ### Example
-    /// ```no_run
-    /// let x: u8 = 1;
-    /// (x as u32) > 300;
-    /// ```
-    #[clippy::version = "pre 1.29.0"]
-    pub INVALID_UPCAST_COMPARISONS,
-    pedantic,
-    "a comparison involving an upcast which is always true or false"
-}
+use super::INVALID_UPCAST_COMPARISONS;
+
+pub(super) fn check<'tcx>(
+    cx: &LateContext<'tcx>,
+    cmp: BinOpKind,
+    lhs: &'tcx Expr<'_>,
+    rhs: &'tcx Expr<'_>,
+    span: Span,
+) {
+    let normalized = comparisons::normalize_comparison(cmp, lhs, rhs);
+    let Some((rel, normalized_lhs, normalized_rhs)) = normalized else {
+        return;
+    };
 
-declare_lint_pass!(InvalidUpcastComparisons => [INVALID_UPCAST_COMPARISONS]);
+    let lhs_bounds = numeric_cast_precast_bounds(cx, normalized_lhs);
+    let rhs_bounds = numeric_cast_precast_bounds(cx, normalized_rhs);
+
+    upcast_comparison_bounds_err(cx, span, rel, lhs_bounds, normalized_lhs, normalized_rhs, false);
+    upcast_comparison_bounds_err(cx, span, rel, rhs_bounds, normalized_rhs, normalized_lhs, true);
+}
 
 fn numeric_cast_precast_bounds(cx: &LateContext<'_>, expr: &Expr<'_>) -> Option<(FullInt, FullInt)> {
     if let ExprKind::Cast(cast_exp, _) = expr.kind {
@@ -68,29 +64,6 @@ fn numeric_cast_precast_bounds(cx: &LateContext<'_>, expr: &Expr<'_>) -> Option<
     }
 }
 
-fn err_upcast_comparison(cx: &LateContext<'_>, span: Span, expr: &Expr<'_>, always: bool) {
-    if let ExprKind::Cast(cast_val, _) = expr.kind {
-        let mut applicability = Applicability::MachineApplicable;
-        let (cast_val_snip, _) = snippet_with_context(
-            cx,
-            cast_val.span,
-            expr.span.ctxt(),
-            "the expression",
-            &mut applicability,
-        );
-        span_lint(
-            cx,
-            INVALID_UPCAST_COMPARISONS,
-            span,
-            format!(
-                "because of the numeric bounds on `{}` prior to casting, this expression is always {}",
-                cast_val_snip,
-                if always { "true" } else { "false" },
-            ),
-        );
-    }
-}
-
 fn upcast_comparison_bounds_err<'tcx>(
     cx: &LateContext<'tcx>,
     span: Span,
@@ -103,63 +76,54 @@ fn upcast_comparison_bounds_err<'tcx>(
     if let Some((lb, ub)) = lhs_bounds
         && let Some(norm_rhs_val) = ConstEvalCtxt::new(cx).eval_full_int(rhs, span.ctxt())
     {
-        if rel == Rel::Eq || rel == Rel::Ne {
-            if norm_rhs_val < lb || norm_rhs_val > ub {
-                err_upcast_comparison(cx, span, lhs, rel == Rel::Ne);
-            }
-        } else if match rel {
-            Rel::Lt => {
-                if invert {
-                    norm_rhs_val < lb
-                } else {
-                    ub < norm_rhs_val
+        match rel {
+            Rel::Eq => {
+                if norm_rhs_val < lb || ub < norm_rhs_val {
+                    err_upcast_comparison(cx, span, lhs, false);
                 }
             },
-            Rel::Le => {
-                if invert {
-                    norm_rhs_val <= lb
-                } else {
-                    ub <= norm_rhs_val
+            Rel::Ne => {
+                if norm_rhs_val < lb || ub < norm_rhs_val {
+                    err_upcast_comparison(cx, span, lhs, true);
                 }
             },
-            Rel::Eq | Rel::Ne => unreachable!(),
-        } {
-            err_upcast_comparison(cx, span, lhs, true);
-        } else if match rel {
             Rel::Lt => {
-                if invert {
-                    norm_rhs_val >= ub
-                } else {
-                    lb >= norm_rhs_val
+                if (invert && norm_rhs_val < lb) || (!invert && ub < norm_rhs_val) {
+                    err_upcast_comparison(cx, span, lhs, true);
+                } else if (!invert && norm_rhs_val <= lb) || (invert && ub <= norm_rhs_val) {
+                    err_upcast_comparison(cx, span, lhs, false);
                 }
             },
             Rel::Le => {
-                if invert {
-                    norm_rhs_val > ub
-                } else {
-                    lb > norm_rhs_val
+                if (invert && norm_rhs_val <= lb) || (!invert && ub <= norm_rhs_val) {
+                    err_upcast_comparison(cx, span, lhs, true);
+                } else if (!invert && norm_rhs_val < lb) || (invert && ub < norm_rhs_val) {
+                    err_upcast_comparison(cx, span, lhs, false);
                 }
             },
-            Rel::Eq | Rel::Ne => unreachable!(),
-        } {
-            err_upcast_comparison(cx, span, lhs, false);
         }
     }
 }
 
-impl<'tcx> LateLintPass<'tcx> for InvalidUpcastComparisons {
-    fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
-        if let ExprKind::Binary(ref cmp, lhs, rhs) = expr.kind {
-            let normalized = comparisons::normalize_comparison(cmp.node, lhs, rhs);
-            let Some((rel, normalized_lhs, normalized_rhs)) = normalized else {
-                return;
-            };
-
-            let lhs_bounds = numeric_cast_precast_bounds(cx, normalized_lhs);
-            let rhs_bounds = numeric_cast_precast_bounds(cx, normalized_rhs);
-
-            upcast_comparison_bounds_err(cx, expr.span, rel, lhs_bounds, normalized_lhs, normalized_rhs, false);
-            upcast_comparison_bounds_err(cx, expr.span, rel, rhs_bounds, normalized_rhs, normalized_lhs, true);
-        }
+fn err_upcast_comparison(cx: &LateContext<'_>, span: Span, expr: &Expr<'_>, always: bool) {
+    if let ExprKind::Cast(cast_val, _) = expr.kind {
+        let mut applicability = Applicability::MachineApplicable;
+        let (cast_val_snip, _) = snippet_with_context(
+            cx,
+            cast_val.span,
+            expr.span.ctxt(),
+            "the expression",
+            &mut applicability,
+        );
+        span_lint(
+            cx,
+            INVALID_UPCAST_COMPARISONS,
+            span,
+            format!(
+                "because of the numeric bounds on `{}` prior to casting, this expression is always {}",
+                cast_val_snip,
+                if always { "true" } else { "false" },
+            ),
+        );
     }
 }
diff --git a/clippy_lints/src/operators/mod.rs b/clippy_lints/src/operators/mod.rs
@@ -11,6 +11,7 @@ mod float_cmp;
 mod float_equality_without_abs;
 mod identity_op;
 mod integer_division;
+mod invalid_upcast_comparisons;
 mod manual_div_ceil;
 mod manual_is_multiple_of;
 mod manual_midpoint;
@@ -888,6 +889,28 @@ declare_clippy_lint! {
     "manually reimplementing `div_ceil`"
 }
 
+declare_clippy_lint! {
+    /// ### What it does
+    /// Checks for comparisons where the relation is always either
+    /// true or false, but where one side has been upcast so that the comparison is
+    /// necessary. Only integer types are checked.
+    ///
+    /// ### Why is this bad?
+    /// An expression like `let x : u8 = ...; (x as u32) > 300`
+    /// will mistakenly imply that it is possible for `x` to be outside the range of
+    /// `u8`.
+    ///
+    /// ### Example
+    /// ```no_run
+    /// let x: u8 = 1;
+    /// (x as u32) > 300;
+    /// ```
+    #[clippy::version = "pre 1.29.0"]
+    pub INVALID_UPCAST_COMPARISONS,
+    pedantic,
+    "a comparison involving an upcast which is always true or false"
+}
+
 pub struct Operators {
     arithmetic_context: numeric_arithmetic::Context,
     verbose_bit_mask_threshold: u64,
@@ -935,6 +958,7 @@ impl_lint_pass!(Operators => [
     MANUAL_MIDPOINT,
     MANUAL_IS_MULTIPLE_OF,
     MANUAL_DIV_CEIL,
+    INVALID_UPCAST_COMPARISONS,
 ]);
 
 impl<'tcx> LateLintPass<'tcx> for Operators {
@@ -950,6 +974,7 @@ impl<'tcx> LateLintPass<'tcx> for Operators {
                     }
                     erasing_op::check(cx, e, op.node, lhs, rhs);
                     identity_op::check(cx, e, op.node, lhs, rhs);
+                    invalid_upcast_comparisons::check(cx, op.node, lhs, rhs, e.span);
                     needless_bitwise_bool::check(cx, e, op.node, lhs, rhs);
                     manual_midpoint::check(cx, e, op.node, lhs, rhs, self.msrv);
                     manual_is_multiple_of::check(cx, e, op.node, lhs, rhs, self.msrv);
