user0/rust
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

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

Browse source 
The last two commits could arguably be split off into a separate PR --
let me know if that's desired.

changelog: none
This commit is contained in:
Samuel Tardieu 2025-10-28 19:41:35 +00:00 • committed by GitHub
commit d74200a6fe
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
4 changed files with 88 additions and 101 deletions
Download patch file Download diff file Expand all files Collapse all files

2
clippy_lints/src/declared_lints.rs
View file

@ -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,

2
clippy_lints/src/lib.rs
View file

@ -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));

160
clippy_lints/src/invalid_upcast_comparisons.rs → clippy_lints/src/operators/invalid_upcast_comparisons.rs
View file

@ -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);
}
}
}

25
clippy_lints/src/operators/mod.rs
View file

@ -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);