Move invalid_upcast_comparisons to its own module

clippy_lints/src/invalid_upcast_comparisons.rs

@@ -0,0 +1,221 @@
+use std::cmp::Ordering;
+
+use rustc_hir::{Expr, ExprKind};
+use rustc_lint::{LateContext, LateLintPass};
+use rustc_middle::ty::{self, IntTy, UintTy};
+use rustc_session::{declare_lint_pass, declare_tool_lint};
+use rustc_span::Span;
+use rustc_target::abi::LayoutOf;
+
+use crate::consts::{constant, Constant};
+
+use clippy_utils::comparisons::Rel;
+use clippy_utils::diagnostics::span_lint;
+use clippy_utils::source::snippet;
+use clippy_utils::{comparisons, sext};
+
+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`.
+    ///
+    /// **Known problems:**
+    /// https://github.com/rust-lang/rust-clippy/issues/886
+    ///
+    /// **Example:**
+    /// ```rust
+    /// let x: u8 = 1;
+    /// (x as u32) > 300;
+    /// ```
+    pub INVALID_UPCAST_COMPARISONS,
+    pedantic,
+    "a comparison involving an upcast which is always true or false"
+}
+
+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<Ordering> {
+        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(ref cast_exp, _) = expr.kind {
+        let pre_cast_ty = cx.typeck_results().expr_ty(cast_exp);
+        let cast_ty = cx.typeck_results().expr_ty(expr);
+        // if it's a cast from i32 to u32 wrapping will invalidate all these checks
+        if cx.layout_of(pre_cast_ty).ok().map(|l| l.size) == cx.layout_of(cast_ty).ok().map(|l| l.size) {
+            return None;
+        }
+        match pre_cast_ty.kind() {
+            ty::Int(int_ty) => Some(match int_ty {
+                IntTy::I8 => (FullInt::S(i128::from(i8::MIN)), FullInt::S(i128::from(i8::MAX))),
+                IntTy::I16 => (FullInt::S(i128::from(i16::MIN)), FullInt::S(i128::from(i16::MAX))),
+                IntTy::I32 => (FullInt::S(i128::from(i32::MIN)), FullInt::S(i128::from(i32::MAX))),
+                IntTy::I64 => (FullInt::S(i128::from(i64::MIN)), FullInt::S(i128::from(i64::MAX))),
+                IntTy::I128 => (FullInt::S(i128::MIN), FullInt::S(i128::MAX)),
+                IntTy::Isize => (FullInt::S(isize::MIN as i128), FullInt::S(isize::MAX as i128)),
+            }),
+            ty::Uint(uint_ty) => Some(match uint_ty {
+                UintTy::U8 => (FullInt::U(u128::from(u8::MIN)), FullInt::U(u128::from(u8::MAX))),
+                UintTy::U16 => (FullInt::U(u128::from(u16::MIN)), FullInt::U(u128::from(u16::MAX))),
+                UintTy::U32 => (FullInt::U(u128::from(u32::MIN)), FullInt::U(u128::from(u32::MAX))),
+                UintTy::U64 => (FullInt::U(u128::from(u64::MIN)), FullInt::U(u128::from(u64::MAX))),
+                UintTy::U128 => (FullInt::U(u128::MIN), FullInt::U(u128::MAX)),
+                UintTy::Usize => (FullInt::U(usize::MIN as u128), FullInt::U(usize::MAX as u128)),
+            }),
+            _ => None,
+        }
+    } else {
+        None
+    }
+}
+
+fn node_as_const_fullint<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) -> Option<FullInt> {
+    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(ref cast_val, _) = expr.kind {
+        span_lint(
+            cx,
+            INVALID_UPCAST_COMPARISONS,
+            span,
+            &format!(
+                "because of the numeric bounds on `{}` prior to casting, this expression is always {}",
+                snippet(cx, cast_val.span, "the expression"),
+                if always { "true" } else { "false" },
+            ),
+        );
+    }
+}
+
+fn upcast_comparison_bounds_err<'tcx>(
+    cx: &LateContext<'tcx>,
+    span: Span,
+    rel: comparisons::Rel,
+    lhs_bounds: Option<(FullInt, FullInt)>,
+    lhs: &'tcx Expr<'_>,
+    rhs: &'tcx Expr<'_>,
+    invert: bool,
+) {
+    if let Some((lb, ub)) = lhs_bounds {
+        if let Some(norm_rhs_val) = node_as_const_fullint(cx, 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);
+                }
+            } 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, ref lhs, ref rhs) = expr.kind {
+            let normalized = comparisons::normalize_comparison(cmp.node, lhs, rhs);
+            let (rel, normalized_lhs, normalized_rhs) = if let Some(val) = normalized {
+                val
+            } 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);
+        }
+    }
+}

clippy_lints/src/lib.rs

@@ -242,6 +242,7 @@ mod inherent_to_string;
 mod inline_fn_without_body;
 mod int_plus_one;
 mod integer_division;
+mod invalid_upcast_comparisons;
 mod items_after_statements;
 mod large_const_arrays;
 mod large_enum_variant;
@@ -697,6 +698,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         &inline_fn_without_body::INLINE_FN_WITHOUT_BODY,
         &int_plus_one::INT_PLUS_ONE,
         &integer_division::INTEGER_DIVISION,
+        &invalid_upcast_comparisons::INVALID_UPCAST_COMPARISONS,
         &items_after_statements::ITEMS_AFTER_STATEMENTS,
         &large_const_arrays::LARGE_CONST_ARRAYS,
         &large_enum_variant::LARGE_ENUM_VARIANT,
@@ -960,7 +962,6 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         &types::BORROWED_BOX,
         &types::BOX_VEC,
         &types::IMPLICIT_HASHER,
-        &types::INVALID_UPCAST_COMPARISONS,
         &types::LINKEDLIST,
         &types::OPTION_OPTION,
         &types::RC_BUFFER,
@@ -1114,7 +1115,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
     store.register_late_pass(|| box drop_forget_ref::DropForgetRef);
     store.register_late_pass(|| box empty_enum::EmptyEnum);
     store.register_late_pass(|| box absurd_extreme_comparisons::AbsurdExtremeComparisons);
-    store.register_late_pass(|| box types::InvalidUpcastComparisons);
+    store.register_late_pass(|| box invalid_upcast_comparisons::InvalidUpcastComparisons);
     store.register_late_pass(|| box regex::Regex::default());
     store.register_late_pass(|| box copies::CopyAndPaste);
     store.register_late_pass(|| box copy_iterator::CopyIterator);
@@ -1374,6 +1375,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         LintId::of(&if_not_else::IF_NOT_ELSE),
         LintId::of(&implicit_saturating_sub::IMPLICIT_SATURATING_SUB),
         LintId::of(&infinite_iter::MAYBE_INFINITE_ITER),
+        LintId::of(&invalid_upcast_comparisons::INVALID_UPCAST_COMPARISONS),
         LintId::of(&items_after_statements::ITEMS_AFTER_STATEMENTS),
         LintId::of(&large_stack_arrays::LARGE_STACK_ARRAYS),
         LintId::of(&let_underscore::LET_UNDERSCORE_DROP),
@@ -1413,7 +1415,6 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         LintId::of(&trait_bounds::TRAIT_DUPLICATION_IN_BOUNDS),
         LintId::of(&trait_bounds::TYPE_REPETITION_IN_BOUNDS),
         LintId::of(&types::IMPLICIT_HASHER),
-        LintId::of(&types::INVALID_UPCAST_COMPARISONS),
         LintId::of(&types::LINKEDLIST),
         LintId::of(&types::OPTION_OPTION),
         LintId::of(&unicode::NON_ASCII_LITERAL),

clippy_lints/src/types/mod.rs

@@ -10,7 +10,6 @@ mod utils;
 mod vec_box;
 
 use std::borrow::Cow;
-use std::cmp::Ordering;
 use std::collections::BTreeMap;
 
 use clippy_utils::diagnostics::{multispan_sugg, span_lint, span_lint_and_then};
@@ -27,17 +26,15 @@ use rustc_hir::{
 use rustc_lint::{LateContext, LateLintPass, LintContext};
 use rustc_middle::hir::map::Map;
 use rustc_middle::lint::in_external_macro;
-use rustc_middle::ty::{self, IntTy, Ty, TyS, TypeckResults, UintTy};
+use rustc_middle::ty::{Ty, TyS, TypeckResults};
 use rustc_session::{declare_lint_pass, declare_tool_lint, impl_lint_pass};
 use rustc_span::source_map::Span;
 use rustc_span::symbol::sym;
-use rustc_target::abi::LayoutOf;
 use rustc_target::spec::abi::Abi;
 use rustc_typeck::hir_ty_to_ty;
 
-use crate::consts::{constant, Constant};
 use clippy_utils::paths;
-use clippy_utils::{comparisons, differing_macro_contexts, match_path, sext};
+use clippy_utils::{differing_macro_contexts, match_path};
 
 declare_clippy_lint! {
     /// **What it does:** Checks for use of `Box<Vec<_>>` anywhere in the code.
@@ -552,214 +549,6 @@ impl<'tcx> Visitor<'tcx> for TypeComplexityVisitor {
     }
 }
 
-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`.
-    ///
-    /// **Known problems:**
-    /// https://github.com/rust-lang/rust-clippy/issues/886
-    ///
-    /// **Example:**
-    /// ```rust
-    /// let x: u8 = 1;
-    /// (x as u32) > 300;
-    /// ```
-    pub INVALID_UPCAST_COMPARISONS,
-    pedantic,
-    "a comparison involving an upcast which is always true or false"
-}
-
-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<Ordering> {
-        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(ref cast_exp, _) = expr.kind {
-        let pre_cast_ty = cx.typeck_results().expr_ty(cast_exp);
-        let cast_ty = cx.typeck_results().expr_ty(expr);
-        // if it's a cast from i32 to u32 wrapping will invalidate all these checks
-        if cx.layout_of(pre_cast_ty).ok().map(|l| l.size) == cx.layout_of(cast_ty).ok().map(|l| l.size) {
-            return None;
-        }
-        match pre_cast_ty.kind() {
-            ty::Int(int_ty) => Some(match int_ty {
-                IntTy::I8 => (FullInt::S(i128::from(i8::MIN)), FullInt::S(i128::from(i8::MAX))),
-                IntTy::I16 => (FullInt::S(i128::from(i16::MIN)), FullInt::S(i128::from(i16::MAX))),
-                IntTy::I32 => (FullInt::S(i128::from(i32::MIN)), FullInt::S(i128::from(i32::MAX))),
-                IntTy::I64 => (FullInt::S(i128::from(i64::MIN)), FullInt::S(i128::from(i64::MAX))),
-                IntTy::I128 => (FullInt::S(i128::MIN), FullInt::S(i128::MAX)),
-                IntTy::Isize => (FullInt::S(isize::MIN as i128), FullInt::S(isize::MAX as i128)),
-            }),
-            ty::Uint(uint_ty) => Some(match uint_ty {
-                UintTy::U8 => (FullInt::U(u128::from(u8::MIN)), FullInt::U(u128::from(u8::MAX))),
-                UintTy::U16 => (FullInt::U(u128::from(u16::MIN)), FullInt::U(u128::from(u16::MAX))),
-                UintTy::U32 => (FullInt::U(u128::from(u32::MIN)), FullInt::U(u128::from(u32::MAX))),
-                UintTy::U64 => (FullInt::U(u128::from(u64::MIN)), FullInt::U(u128::from(u64::MAX))),
-                UintTy::U128 => (FullInt::U(u128::MIN), FullInt::U(u128::MAX)),
-                UintTy::Usize => (FullInt::U(usize::MIN as u128), FullInt::U(usize::MAX as u128)),
-            }),
-            _ => None,
-        }
-    } else {
-        None
-    }
-}
-
-fn node_as_const_fullint<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) -> Option<FullInt> {
-    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(ref cast_val, _) = expr.kind {
-        span_lint(
-            cx,
-            INVALID_UPCAST_COMPARISONS,
-            span,
-            &format!(
-                "because of the numeric bounds on `{}` prior to casting, this expression is always {}",
-                snippet(cx, cast_val.span, "the expression"),
-                if always { "true" } else { "false" },
-            ),
-        );
-    }
-}
-
-fn upcast_comparison_bounds_err<'tcx>(
-    cx: &LateContext<'tcx>,
-    span: Span,
-    rel: comparisons::Rel,
-    lhs_bounds: Option<(FullInt, FullInt)>,
-    lhs: &'tcx Expr<'_>,
-    rhs: &'tcx Expr<'_>,
-    invert: bool,
-) {
-    use clippy_utils::comparisons::Rel;
-
-    if let Some((lb, ub)) = lhs_bounds {
-        if let Some(norm_rhs_val) = node_as_const_fullint(cx, 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);
-                }
-            } 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, ref lhs, ref rhs) = expr.kind {
-            let normalized = comparisons::normalize_comparison(cmp.node, lhs, rhs);
-            let (rel, normalized_lhs, normalized_rhs) = if let Some(val) = normalized {
-                val
-            } 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);
-        }
-    }
-}
-
 declare_clippy_lint! {
     /// **What it does:** Checks for public `impl` or `fn` missing generalization
     /// over different hashers and implicitly defaulting to the default hashing