From 4679eb3a0d09219b4bd815c636eb55e5b05c5a5e Mon Sep 17 00:00:00 2001 From: Michael Wright Date: Wed, 20 Oct 2021 06:13:42 +0200 Subject: [PATCH 1/3] Move const int value code to utils --- .../src/invalid_upcast_comparisons.rs | 68 +------------------ clippy_utils/src/consts.rs | 68 +++++++++++++++++++ 2 files changed, 71 insertions(+), 65 deletions(-) diff --git a/clippy_lints/src/invalid_upcast_comparisons.rs b/clippy_lints/src/invalid_upcast_comparisons.rs index b1f70b30c12..82438d85c7a 100644 --- a/clippy_lints/src/invalid_upcast_comparisons.rs +++ b/clippy_lints/src/invalid_upcast_comparisons.rs @@ -1,5 +1,3 @@ -use std::cmp::Ordering; - use rustc_hir::{Expr, ExprKind}; use rustc_lint::{LateContext, LateLintPass}; use rustc_middle::ty::layout::LayoutOf; @@ -7,11 +5,11 @@ use rustc_middle::ty::{self, IntTy, UintTy}; use rustc_session::{declare_lint_pass, declare_tool_lint}; use rustc_span::Span; +use clippy_utils::comparisons; use clippy_utils::comparisons::Rel; -use clippy_utils::consts::{constant, Constant}; +use clippy_utils::consts::{constant_full_int, FullInt}; use clippy_utils::diagnostics::span_lint; use clippy_utils::source::snippet; -use clippy_utils::{comparisons, sext}; declare_clippy_lint! { /// ### What it does @@ -39,53 +37,6 @@ declare_clippy_lint! { declare_lint_pass!(InvalidUpcastComparisons => [INVALID_UPCAST_COMPARISONS]); -#[derive(Copy, Clone, Debug, Eq)] -enum FullInt { - S(i128), - U(u128), -} - -impl FullInt { - #[allow(clippy::cast_sign_loss)] - #[must_use] - fn cmp_s_u(s: i128, u: u128) -> Ordering { - if s < 0 { - Ordering::Less - } else if u > (i128::MAX as u128) { - Ordering::Greater - } else { - (s as u128).cmp(&u) - } - } -} - -impl PartialEq for FullInt { - #[must_use] - fn eq(&self, other: &Self) -> bool { - self.partial_cmp(other).expect("`partial_cmp` only returns `Some(_)`") == Ordering::Equal - } -} - -impl PartialOrd for FullInt { - #[must_use] - fn partial_cmp(&self, other: &Self) -> Option { - Some(match (self, other) { - (&Self::S(s), &Self::S(o)) => s.cmp(&o), - (&Self::U(s), &Self::U(o)) => s.cmp(&o), - (&Self::S(s), &Self::U(o)) => Self::cmp_s_u(s, o), - (&Self::U(s), &Self::S(o)) => Self::cmp_s_u(o, s).reverse(), - }) - } -} - -impl Ord for FullInt { - #[must_use] - fn cmp(&self, other: &Self) -> Ordering { - self.partial_cmp(other) - .expect("`partial_cmp` for FullInt can never return `None`") - } -} - fn numeric_cast_precast_bounds<'a>(cx: &LateContext<'_>, expr: &'a Expr<'_>) -> Option<(FullInt, FullInt)> { if let ExprKind::Cast(cast_exp, _) = expr.kind { let pre_cast_ty = cx.typeck_results().expr_ty(cast_exp); @@ -118,19 +69,6 @@ fn numeric_cast_precast_bounds<'a>(cx: &LateContext<'_>, expr: &'a Expr<'_>) -> } } -fn node_as_const_fullint<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) -> Option { - let val = constant(cx, cx.typeck_results(), expr)?.0; - if let Constant::Int(const_int) = val { - match *cx.typeck_results().expr_ty(expr).kind() { - ty::Int(ity) => Some(FullInt::S(sext(cx.tcx, const_int, ity))), - ty::Uint(_) => Some(FullInt::U(const_int)), - _ => None, - } - } else { - None - } -} - fn err_upcast_comparison(cx: &LateContext<'_>, span: Span, expr: &Expr<'_>, always: bool) { if let ExprKind::Cast(cast_val, _) = expr.kind { span_lint( @@ -156,7 +94,7 @@ fn upcast_comparison_bounds_err<'tcx>( invert: bool, ) { if let Some((lb, ub)) = lhs_bounds { - if let Some(norm_rhs_val) = node_as_const_fullint(cx, rhs) { + if let Some(norm_rhs_val) = constant_full_int(cx, cx.typeck_results(), rhs) { 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); diff --git a/clippy_utils/src/consts.rs b/clippy_utils/src/consts.rs index 8bf31807d55..3e5d74a66f4 100644 --- a/clippy_utils/src/consts.rs +++ b/clippy_utils/src/consts.rs @@ -155,6 +155,19 @@ impl Constant { _ => None, } } + + /// Returns the integer value or `None` if `self` or `val_type` is not integer type. + pub fn int_value(&self, cx: &LateContext<'_>, val_type: Ty<'_>) -> Option { + if let Constant::Int(const_int) = *self { + match *val_type.kind() { + ty::Int(ity) => Some(FullInt::S(sext(cx.tcx, const_int, ity))), + ty::Uint(_) => Some(FullInt::U(const_int)), + _ => None, + } + } else { + None + } + } } /// Parses a `LitKind` to a `Constant`. @@ -202,6 +215,61 @@ pub fn constant_simple<'tcx>( constant(lcx, typeck_results, e).and_then(|(cst, res)| if res { None } else { Some(cst) }) } +pub fn constant_full_int( + lcx: &LateContext<'tcx>, + typeck_results: &ty::TypeckResults<'tcx>, + e: &Expr<'_>, +) -> Option { + constant_simple(lcx, typeck_results, e)?.int_value(lcx, typeck_results.expr_ty(e)) +} + +#[derive(Copy, Clone, Debug, Eq)] +pub enum FullInt { + S(i128), + U(u128), +} + +impl FullInt { + #[allow(clippy::cast_sign_loss)] + #[must_use] + fn cmp_s_u(s: i128, u: u128) -> Ordering { + if s < 0 { + Ordering::Less + } else if u > (i128::MAX as u128) { + Ordering::Greater + } else { + (s as u128).cmp(&u) + } + } +} + +impl PartialEq for FullInt { + #[must_use] + fn eq(&self, other: &Self) -> bool { + self.partial_cmp(other).expect("`partial_cmp` only returns `Some(_)`") == Ordering::Equal + } +} + +impl PartialOrd for FullInt { + #[must_use] + fn partial_cmp(&self, other: &Self) -> Option { + Some(match (self, other) { + (&Self::S(s), &Self::S(o)) => s.cmp(&o), + (&Self::U(s), &Self::U(o)) => s.cmp(&o), + (&Self::S(s), &Self::U(o)) => Self::cmp_s_u(s, o), + (&Self::U(s), &Self::S(o)) => Self::cmp_s_u(o, s).reverse(), + }) + } +} + +impl Ord for FullInt { + #[must_use] + fn cmp(&self, other: &Self) -> Ordering { + self.partial_cmp(other) + .expect("`partial_cmp` for FullInt can never return `None`") + } +} + /// Creates a `ConstEvalLateContext` from the given `LateContext` and `TypeckResults`. pub fn constant_context<'a, 'tcx>( lcx: &'a LateContext<'tcx>, From c4d5471a45dbdddccb74bb505864cab284df21fd Mon Sep 17 00:00:00 2001 From: Michael Wright Date: Wed, 20 Oct 2021 06:13:42 +0200 Subject: [PATCH 2/3] Add test for #7829 --- tests/ui/match_overlapping_arm.rs | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/tests/ui/match_overlapping_arm.rs b/tests/ui/match_overlapping_arm.rs index ff91c4498ec..845986a4ead 100644 --- a/tests/ui/match_overlapping_arm.rs +++ b/tests/ui/match_overlapping_arm.rs @@ -100,6 +100,13 @@ fn overlapping() { _ => (), } + // Issue #7829 + match 0 { + -1..=1 => (), + -2..=2 => (), + _ => (), + } + if let None = Some(42) { // nothing } else if let None = Some(42) { From 1ede540b2135ce56546cb5f28fbf5c000646363f Mon Sep 17 00:00:00 2001 From: Michael Wright Date: Wed, 20 Oct 2021 06:13:42 +0200 Subject: [PATCH 3/3] Fix false positive in `match_overlapping_arm` The bug was dues to the constant bytes being compared instead of their values. This meant that negative values were being treated as larger than some positive values. Fixes #7829 --- clippy_lints/src/matches.rs | 51 ++++++++++--------------------------- 1 file changed, 14 insertions(+), 37 deletions(-) diff --git a/clippy_lints/src/matches.rs b/clippy_lints/src/matches.rs index b643fba5d32..f1289a36e77 100644 --- a/clippy_lints/src/matches.rs +++ b/clippy_lints/src/matches.rs @@ -1,4 +1,4 @@ -use clippy_utils::consts::{constant, miri_to_const, Constant}; +use clippy_utils::consts::{constant, constant_full_int, miri_to_const, FullInt}; use clippy_utils::diagnostics::{ multispan_sugg, span_lint_and_help, span_lint_and_note, span_lint_and_sugg, span_lint_and_then, }; @@ -930,9 +930,8 @@ fn check_match_bool(cx: &LateContext<'_>, ex: &Expr<'_>, arms: &[Arm<'_>], expr: fn check_overlapping_arms<'tcx>(cx: &LateContext<'tcx>, ex: &'tcx Expr<'_>, arms: &'tcx [Arm<'_>]) { if arms.len() >= 2 && cx.typeck_results().expr_ty(ex).is_integral() { let ranges = all_ranges(cx, arms, cx.typeck_results().expr_ty(ex)); - let type_ranges = type_ranges(&ranges); - if !type_ranges.is_empty() { - if let Some((start, end)) = overlapping(&type_ranges) { + if !ranges.is_empty() { + if let Some((start, end)) = overlapping(&ranges) { span_lint_and_note( cx, MATCH_OVERLAPPING_ARM, @@ -1601,7 +1600,7 @@ fn opt_parent_let<'a>(cx: &LateContext<'a>, ex: &Expr<'a>) -> Option<&'a Local<' } /// Gets all arms that are unbounded `PatRange`s. -fn all_ranges<'tcx>(cx: &LateContext<'tcx>, arms: &'tcx [Arm<'_>], ty: Ty<'tcx>) -> Vec> { +fn all_ranges<'tcx>(cx: &LateContext<'tcx>, arms: &'tcx [Arm<'_>], ty: Ty<'tcx>) -> Vec> { arms.iter() .filter_map(|arm| { if let Arm { pat, guard: None, .. } = *arm { @@ -1614,21 +1613,25 @@ fn all_ranges<'tcx>(cx: &LateContext<'tcx>, arms: &'tcx [Arm<'_>], ty: Ty<'tcx>) Some(rhs) => constant(cx, cx.typeck_results(), rhs)?.0, None => miri_to_const(ty.numeric_max_val(cx.tcx)?)?, }; - let rhs = match range_end { - RangeEnd::Included => Bound::Included(rhs), - RangeEnd::Excluded => Bound::Excluded(rhs), + + let lhs_val = lhs.int_value(cx, ty)?; + let rhs_val = rhs.int_value(cx, ty)?; + + let rhs_bound = match range_end { + RangeEnd::Included => Bound::Included(rhs_val), + RangeEnd::Excluded => Bound::Excluded(rhs_val), }; return Some(SpannedRange { span: pat.span, - node: (lhs, rhs), + node: (lhs_val, rhs_bound), }); } if let PatKind::Lit(value) = pat.kind { - let value = constant(cx, cx.typeck_results(), value)?.0; + let value = constant_full_int(cx, cx.typeck_results(), value)?; return Some(SpannedRange { span: pat.span, - node: (value.clone(), Bound::Included(value)), + node: (value, Bound::Included(value)), }); } } @@ -1643,32 +1646,6 @@ pub struct SpannedRange { pub node: (T, Bound), } -type TypedRanges = Vec>; - -/// Gets all `Int` ranges or all `Uint` ranges. Mixed types are an error anyway -/// and other types than -/// `Uint` and `Int` probably don't make sense. -fn type_ranges(ranges: &[SpannedRange]) -> TypedRanges { - ranges - .iter() - .filter_map(|range| match range.node { - (Constant::Int(start), Bound::Included(Constant::Int(end))) => Some(SpannedRange { - span: range.span, - node: (start, Bound::Included(end)), - }), - (Constant::Int(start), Bound::Excluded(Constant::Int(end))) => Some(SpannedRange { - span: range.span, - node: (start, Bound::Excluded(end)), - }), - (Constant::Int(start), Bound::Unbounded) => Some(SpannedRange { - span: range.span, - node: (start, Bound::Unbounded), - }), - _ => None, - }) - .collect() -} - // Checks if arm has the form `None => None` fn is_none_arm(cx: &LateContext<'_>, arm: &Arm<'_>) -> bool { matches!(arm.pat.kind, PatKind::Path(ref qpath) if is_lang_ctor(cx, qpath, OptionNone))