Bad Comparison of Upcasted Ints Fix #42

README.md

@@ -14,7 +14,7 @@ Table of contents:
 * [License](#license)
 
 ##Lints
-There are 135 lints included in this crate:
+There are 136 lints included in this crate:
 
 name                                                                                                                 | default | meaning
 ---------------------------------------------------------------------------------------------------------------------|---------|------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
@@ -67,6 +67,7 @@ name
 [ineffective_bit_mask](https://github.com/Manishearth/rust-clippy/wiki#ineffective_bit_mask)                         | warn    | expressions where a bit mask will be rendered useless by a comparison, e.g. `(x | 1) > 2`
 [inline_always](https://github.com/Manishearth/rust-clippy/wiki#inline_always)                                       | warn    | `#[inline(always)]` is a bad idea in most cases
 [invalid_regex](https://github.com/Manishearth/rust-clippy/wiki#invalid_regex)                                       | deny    | finds invalid regular expressions in `Regex::new(_)` invocations
+[invalid_upcast_comparisons](https://github.com/Manishearth/rust-clippy/wiki#invalid_upcast_comparisons)             | warn    | a comparison involving an term's upcasting to be within the range of the other side of the term is always true or false
 [items_after_statements](https://github.com/Manishearth/rust-clippy/wiki#items_after_statements)                     | warn    | finds blocks where an item comes after a statement
 [iter_next_loop](https://github.com/Manishearth/rust-clippy/wiki#iter_next_loop)                                     | warn    | for-looping over `_.next()` which is probably not intended
 [len_without_is_empty](https://github.com/Manishearth/rust-clippy/wiki#len_without_is_empty)                         | warn    | traits and impls that have `.len()` but not `.is_empty()`

src/lib.rs

@@ -213,6 +213,7 @@ pub fn plugin_registrar(reg: &mut Registry) {
     });
     reg.register_late_lint_pass(box drop_ref::DropRefPass);
     reg.register_late_lint_pass(box types::AbsurdExtremeComparisons);
+    reg.register_late_lint_pass(box types::InvalidUpcastComparisons);
     reg.register_late_lint_pass(box regex::RegexPass::default());
     reg.register_late_lint_pass(box copies::CopyAndPaste);
     reg.register_late_lint_pass(box format::FormatMacLint);
@@ -356,6 +357,7 @@ pub fn plugin_registrar(reg: &mut Registry) {
         types::ABSURD_EXTREME_COMPARISONS,
         types::BOX_VEC,
         types::CHAR_LIT_AS_U8,
+        types::INVALID_UPCAST_COMPARISONS,
         types::LET_UNIT_VALUE,
         types::LINKEDLIST,
         types::TYPE_COMPLEXITY,

src/types.rs

@@ -1,6 +1,7 @@
 use reexport::*;
 use rustc::lint::*;
 use rustc::middle::const_eval;
+use rustc::middle::const_eval::ConstVal::Integral;
 use rustc::middle::ty;
 use rustc_front::hir::*;
 use rustc_front::intravisit::{FnKind, Visitor, walk_ty};
@@ -9,6 +10,7 @@ use syntax::ast::{IntTy, UintTy, FloatTy};
 use syntax::codemap::Span;
 use utils::*;
 
+
 /// Handles all the linting of funky types
 #[allow(missing_copy_implementations)]
 pub struct TypePass;
@@ -627,24 +629,32 @@ enum AbsurdComparisonResult {
     InequalityImpossible,
 }
 
+enum Rel {
+    Lt,
+    Le,
+}
+
+// Put the expression in the form lhs < rhs or lhs <= rhs.
+fn normalize_comparison<'a>(op: BinOp_, lhs: &'a Expr, rhs: &'a Expr)
+                                -> Option<(Rel, &'a Expr, &'a Expr)> {
+    match op {
+        BiLt => Some((Rel::Lt, lhs, rhs)),
+        BiLe => Some((Rel::Le, lhs, rhs)),
+        BiGt => Some((Rel::Lt, rhs, lhs)),
+        BiGe => Some((Rel::Le, rhs, lhs)),
+        _ => None,
+    }
+}
+
 fn detect_absurd_comparison<'a>(cx: &LateContext, op: BinOp_, lhs: &'a Expr, rhs: &'a Expr)
                                 -> Option<(ExtremeExpr<'a>, AbsurdComparisonResult)> {
     use types::ExtremeType::*;
     use types::AbsurdComparisonResult::*;
     type Extr<'a> = ExtremeExpr<'a>;
 
-    // Put the expression in the form lhs < rhs or lhs <= rhs.
-    enum Rel {
-        Lt,
-        Le,
-    };
-    let (rel, normalized_lhs, normalized_rhs) = match op {
-        BiLt => (Rel::Lt, lhs, rhs),
-        BiLe => (Rel::Le, lhs, rhs),
-        BiGt => (Rel::Lt, rhs, lhs),
-        BiGe => (Rel::Le, rhs, lhs),
-        _ => return None,
-    };
+    let normalized = normalize_comparison(op, lhs, rhs);
+    if normalized.is_none() { return None; } // Could be an if let, but this prevents rightward drift
+    let (rel, normalized_lhs, normalized_rhs) = normalized.expect("Unreachable-- is none check above");
 
     let lx = detect_extreme_expr(cx, normalized_lhs);
     let rx = detect_extreme_expr(cx, normalized_rhs);
@@ -764,3 +774,190 @@ impl LateLintPass for AbsurdExtremeComparisons {
         }
     }
 }
+
+/// **What it does:** This lint 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:** None
+///
+/// **Example:** `let x : u8 = ...; (x as u32) > 300`
+declare_lint! {
+    pub INVALID_UPCAST_COMPARISONS, Warn,
+    "a comparison involving an term's upcasting to be within the range of the other side of the \
+    term is always true or false"
+}
+
+pub struct InvalidUpcastComparisons;
+
+impl LintPass for InvalidUpcastComparisons {
+    fn get_lints(&self) -> LintArray {
+        lint_array!(INVALID_UPCAST_COMPARISONS)
+    }
+}
+
+enum FullInt {
+    S(i64),
+    U(u64),
+}
+
+use std;
+use std::cmp::Ordering;
+
+impl FullInt {
+    #[allow(cast_sign_loss)]
+    fn cmp_s_u(s: &i64, u: &u64) -> std::cmp::Ordering {
+        if *s < 0 {
+            Ordering::Less
+        } else if *u > (i64::max_value() as u64) {
+            Ordering::Greater
+        } else {
+            (*s as u64).cmp(u)
+        }
+    }
+}
+
+impl PartialEq for FullInt {
+    fn eq(&self, other: &Self) -> bool {
+        self.cmp(other) == Ordering::Equal
+    }
+}
+impl Eq for FullInt {}
+
+impl PartialOrd for FullInt {
+    fn partial_cmp(&self, other: &Self) -> Option<std::cmp::Ordering> {
+        Some(match (self, other) {
+            (&FullInt::S(ref s), &FullInt::S(ref o)) => s.cmp(o),
+            (&FullInt::U(ref s), &FullInt::U(ref o)) => s.cmp(o),
+            (&FullInt::S(ref s), &FullInt::U(ref o)) => Self::cmp_s_u(s, o),
+            (&FullInt::U(ref s), &FullInt::S(ref o)) => Self::cmp_s_u(o, s).reverse(),
+        })
+    }
+}
+impl Ord for FullInt {
+    fn cmp(&self, other: &Self) -> std::cmp::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)> {
+    use rustc::middle::ty::TypeVariants::{TyInt, TyUint};
+    use syntax::ast::UintTy;
+    use syntax::ast::IntTy;
+    use std::*;
+
+    if let ExprCast(ref cast_exp,_) = expr.node {
+        match cx.tcx.expr_ty(cast_exp).sty {
+            TyInt(int_ty) => Some(match int_ty {
+                IntTy::I8 => (FullInt::S(i8::min_value() as i64), FullInt::S(i8::max_value() as i64)),
+                IntTy::I16 => (FullInt::S(i16::min_value() as i64), FullInt::S(i16::max_value() as i64)),
+                IntTy::I32 => (FullInt::S(i32::min_value() as i64), FullInt::S(i32::max_value() as i64)),
+                IntTy::I64 => (FullInt::S(i64::min_value() as i64), FullInt::S(i64::max_value() as i64)),
+                IntTy::Is => (FullInt::S(isize::min_value() as i64), FullInt::S(isize::max_value() as i64)),
+            }),
+            TyUint(uint_ty) => Some(match uint_ty {
+                UintTy::U8 => (FullInt::U(u8::min_value() as u64), FullInt::U(u8::max_value() as u64)),
+                UintTy::U16 => (FullInt::U(u16::min_value() as u64), FullInt::U(u16::max_value() as u64)),
+                UintTy::U32 => (FullInt::U(u32::min_value() as u64), FullInt::U(u32::max_value() as u64)),
+                UintTy::U64 => (FullInt::U(u64::min_value() as u64), FullInt::U(u64::max_value() as u64)),
+                UintTy::Us => (FullInt::U(usize::min_value() as u64), FullInt::U(usize::max_value() as u64)),
+            }),
+            _ => None,
+        }
+    } else {
+        None
+    }
+}
+
+fn node_as_const_fullint(cx: &LateContext, expr: &Expr) -> Option<FullInt> {
+    use rustc::middle::const_eval::EvalHint::ExprTypeChecked;
+    use rustc_const_eval::*;
+
+    match const_eval::eval_const_expr_partial(cx.tcx, expr, ExprTypeChecked, None) {
+        Ok(val) => {
+            if let Integral(const_int) = val {
+                Some(match const_int {
+                    I8(x) => FullInt::S(x as i64),
+                    I16(x) => FullInt::S(x as i64),
+                    I32(x) => FullInt::S(x as i64),
+                    Isize(x) => FullInt::S(match x {
+                        Is32(x_) => x_ as i64,
+                        Is64(x_) => x_
+                    }),
+                    I64(x) => FullInt::S(x),
+                    InferSigned(x) => FullInt::S(x as i64),
+                    U8(x) => FullInt::U(x as u64),
+                    U16(x) => FullInt::U(x as u64),
+                    U32(x) => FullInt::U(x as u64),
+                    Usize(x) => FullInt::U(match x {
+                        Us32(x_) => x_ as u64,
+                        Us64(x_) => x_,
+                    }),
+                    U64(x) => FullInt::U(x),
+                    Infer(x) => FullInt::U(x as u64),
+                })
+            } else {
+                None
+            }
+        },
+        Err(_) => None,
+    }
+}
+
+impl LateLintPass for InvalidUpcastComparisons {
+    fn check_expr(&mut self, cx: &LateContext, expr: &Expr) {
+        if let ExprBinary(ref cmp, ref lhs, ref rhs) = expr.node {
+            let normalized = normalize_comparison(cmp.node, lhs, rhs);
+            if normalized.is_none() { return; }
+            let (rel, normalized_lhs, normalized_rhs) = normalized.expect("Unreachable-- is none check above");
+
+            let lhs_bounds = numeric_cast_precast_bounds(cx, normalized_lhs);
+            let rhs_bounds = numeric_cast_precast_bounds(cx, normalized_rhs);
+
+            let msg = "Because of the numeric bounds prior to casting, this expression is always ";
+
+            if let Some(nlb) = lhs_bounds {
+                if let Some(norm_rhs_val) = node_as_const_fullint(cx, normalized_rhs) {
+                    if match rel {
+                        Rel::Lt => nlb.1 < norm_rhs_val,
+                        Rel::Le => nlb.1 <= norm_rhs_val,
+                    } {
+                        // Expression is always true
+                        cx.span_lint(INVALID_UPCAST_COMPARISONS,
+                                     expr.span,
+                                     &format!("{}{}.", msg, "true"));
+                    } else if match rel {
+                        Rel::Lt => nlb.0 >= norm_rhs_val,
+                        Rel::Le => nlb.0 > norm_rhs_val,
+                    } {
+                        // Expression is always false
+                        cx.span_lint(INVALID_UPCAST_COMPARISONS,
+                                     expr.span,
+                                     &format!("{}{}.", msg, "false"));
+                    }
+                }
+            } else if let Some(nrb) = rhs_bounds {
+                if let Some(norm_lhs_val) = node_as_const_fullint(cx, normalized_lhs) {
+                    if match rel {
+                        Rel::Lt => norm_lhs_val < nrb.0,
+                        Rel::Le => norm_lhs_val <= nrb.0,
+                    } {
+                        // Expression is always true
+                        cx.span_lint(INVALID_UPCAST_COMPARISONS,
+                                     expr.span,
+                                     &format!("{}{}.", msg, "true"));
+                    } else if match rel {
+                        Rel::Lt => norm_lhs_val >= nrb.1,
+                        Rel::Le => norm_lhs_val > nrb.1,
+                    } {
+                        // Expression is always false
+                        cx.span_lint(INVALID_UPCAST_COMPARISONS,
+                                     expr.span,
+                                     &format!("{}{}.", msg, "false"));
+                    }
+                }
+            }
+        }
+    }
+}

tests/compile-fail/invalid_upcast_comparisons.rs

@@ -0,0 +1,19 @@
+#![feature(plugin)]
+#![plugin(clippy)]
+
+#![deny(invalid_upcast_comparisons)]
+#![allow(unused, eq_op, no_effect)]
+fn main() {
+    let zero: u32 = 0;
+    let u8_max: u8 = 255;
+
+    (u8_max as u32) > 300; //~ERROR Because of the numeric bounds prior to casting, this expression is always false.
+    (u8_max as u32) > 20;
+
+    (zero as i32) < -5; //~ERROR Because of the numeric bounds prior to casting, this expression is always false.
+    (zero as i32) < 10;
+
+    -5 < (zero as i32); //~ERROR Because of the numeric bounds prior to casting, this expression is always true.
+    0 <= (zero as i32); //~ERROR Because of the numeric bounds prior to casting, this expression is always true.
+    0 < (zero as i32);
+}