From e71b46f14f56901f30125d1ba646f915f4308795 Mon Sep 17 00:00:00 2001 From: Ada Alakbarova Date: Sun, 26 Oct 2025 11:04:22 +0100 Subject: [PATCH] refactor(invalid_upcast_comparisons): move to under `operators` and clean up a bit: - put the lint functions in the order they're called - simplify `upcast_comparison_bounds_err` - It first handled `Eq`/`Ne`, then one pair of cases with `Lt`/Le`, then another pair of cases with `Lt`/`Le` -- turn that into a single `match rel`. - It used a pattern of `if invert { a } else { b }`, which I found to be hard because of the need to keep in mind the implicit `invert &&` and `!invert &&` when looking at `a` and `b`. The form `(invert && a) || (!invert && b)` should be a bit more explicit, while compiling down to the same thing - The conditions were already partially written in the order `lb`, `norm_rhs_val`, `ub` -- change the ones that weren't --- clippy_lints/src/declared_lints.rs | 2 +- clippy_lints/src/lib.rs | 2 - .../invalid_upcast_comparisons.rs | 160 +++++++----------- clippy_lints/src/operators/mod.rs | 25 +++ 4 files changed, 88 insertions(+), 101 deletions(-) rename clippy_lints/src/{ => operators}/invalid_upcast_comparisons.rs (56%) diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs index bd2971fa150d..1603114b3103 100644 --- 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 index 6342b80ab134..5628caeb0b78 100644 --- 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; @@ -542,7 +541,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::::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/invalid_upcast_comparisons.rs b/clippy_lints/src/operators/invalid_upcast_comparisons.rs similarity index 56% rename from clippy_lints/src/invalid_upcast_comparisons.rs rename to clippy_lints/src/operators/invalid_upcast_comparisons.rs index 885649074ab6..b32848a32337 100644 --- a/clippy_lints/src/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; -declare_lint_pass!(InvalidUpcastComparisons => [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; + }; + + 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,6 +64,47 @@ fn numeric_cast_precast_bounds(cx: &LateContext<'_>, expr: &Expr<'_>) -> Option< } } +fn upcast_comparison_bounds_err<'tcx>( + cx: &LateContext<'tcx>, + span: Span, + rel: Rel, + lhs_bounds: Option<(FullInt, FullInt)>, + lhs: &'tcx Expr<'_>, + rhs: &'tcx Expr<'_>, + invert: bool, +) { + if let Some((lb, ub)) = lhs_bounds + && let Some(norm_rhs_val) = ConstEvalCtxt::new(cx).eval_full_int(rhs, span.ctxt()) + { + match rel { + Rel::Eq => { + if norm_rhs_val < lb || ub < norm_rhs_val { + err_upcast_comparison(cx, span, lhs, false); + } + }, + Rel::Ne => { + if norm_rhs_val < lb || ub < norm_rhs_val { + err_upcast_comparison(cx, span, lhs, true); + } + }, + Rel::Lt => { + 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 <= 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); + } + }, + } + } +} + 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; @@ -90,76 +127,3 @@ fn err_upcast_comparison(cx: &LateContext<'_>, span: Span, expr: &Expr<'_>, alwa ); } } - -fn upcast_comparison_bounds_err<'tcx>( - cx: &LateContext<'tcx>, - span: Span, - rel: Rel, - lhs_bounds: Option<(FullInt, FullInt)>, - lhs: &'tcx Expr<'_>, - rhs: &'tcx Expr<'_>, - invert: bool, -) { - 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 - } - }, - Rel::Le => { - if invert { - norm_rhs_val <= lb - } else { - ub <= norm_rhs_val - } - }, - 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 - } - }, - Rel::Le => { - if invert { - norm_rhs_val > ub - } else { - lb > norm_rhs_val - } - }, - 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); - } - } -} diff --git a/clippy_lints/src/operators/mod.rs b/clippy_lints/src/operators/mod.rs index 75a6d57efb3c..b960f4ddffa3 100644 --- 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);