Merge pull request #802 (through #828)

Bad Comparison of Upcasted Ints

Author: mcarton

README.md

@@ -14,7 +14,7 @@ Table of contents:
 * [License](#license)
 
 ##Lints
-There are 139 lints included in this crate:
+There are 140 lints included in this crate:
 
 name                                                                                                                 | default | meaning
 ---------------------------------------------------------------------------------------------------------------------|---------|------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
@@ -68,6 +68,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 upcast which 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

@@ -221,6 +221,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);
@@ -369,6 +370,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

@@ -5,6 +5,7 @@ use rustc::ty;
 use rustc_front::hir::*;
 use rustc_front::intravisit::{FnKind, Visitor, walk_ty};
 use rustc_front::util::{is_comparison_binop, binop_to_string};
+use std::cmp::Ordering;
 use syntax::ast::{IntTy, UintTy, FloatTy};
 use syntax::codemap::Span;
 use utils::*;
@@ -640,23 +641,20 @@ enum AbsurdComparisonResult {
     InequalityImpossible,
 }
 
+
+
 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::*;
+    use utils::comparisons::*;
     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);
+    let (rel, normalized_lhs, normalized_rhs) = if let Some(val) = normalized {
+        val
+    } else {
+        return None;
     };
 
     let lx = detect_extreme_expr(cx, normalized_lhs);
@@ -679,6 +677,7 @@ fn detect_absurd_comparison<'a>(cx: &LateContext, op: BinOp_, lhs: &'a Expr, rhs
                 _ => return None,
             }
         }
+        Rel::Ne | Rel::Eq => return None,
     })
 }
 
@@ -778,3 +777,178 @@ 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 upcast which is always true or false"
+}
+
+pub struct InvalidUpcastComparisons;
+
+impl LintPass for InvalidUpcastComparisons {
+    fn get_lints(&self) -> LintArray {
+        lint_array!(INVALID_UPCAST_COMPARISONS)
+    }
+}
+
+#[derive(Copy, Clone, Debug, Eq)]
+enum FullInt {
+    S(i64),
+    U(u64),
+}
+
+impl FullInt {
+    #[allow(cast_sign_loss)]
+    fn cmp_s_u(s: i64, u: u64) -> 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.partial_cmp(other).expect("partial_cmp only returns Some(_)") == Ordering::Equal
+    }
+}
+
+impl PartialOrd for FullInt {
+    fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
+        Some(match (self, other) {
+            (&FullInt::S(s), &FullInt::S(o)) => s.cmp(&o),
+            (&FullInt::U(s), &FullInt::U(o)) => s.cmp(&o),
+            (&FullInt::S(s), &FullInt::U(o)) => Self::cmp_s_u(s, o),
+            (&FullInt::U(s), &FullInt::S(o)) => Self::cmp_s_u(o, s).reverse(),
+        })
+    }
+}
+impl Ord for FullInt {
+    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)> {
+    use rustc::ty::TypeVariants::{TyInt, TyUint};
+    use syntax::ast::{IntTy, UintTy};
+    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_val::ConstVal::*;
+    use rustc_const_eval::EvalHint::ExprTypeChecked;
+    use rustc_const_eval::eval_const_expr_partial;
+    use rustc_const_math::ConstInt;
+
+    match eval_const_expr_partial(cx.tcx, expr, ExprTypeChecked, None) {
+        Ok(val) => {
+            if let Integral(const_int) = val {
+                Some(match const_int.erase_type() {
+                    ConstInt::InferSigned(x) => FullInt::S(x as i64),
+                    ConstInt::Infer(x) => FullInt::U(x as u64),
+                    _ => unreachable!(),
+                })
+            } else {
+                None
+            }
+        },
+        Err(_) => None,
+    }
+}
+
+fn err_upcast_comparison(cx: &LateContext, span: &Span, expr: &Expr, always: bool) {
+    if let ExprCast(ref cast_val, _) = expr.node {
+        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(
+        cx: &LateContext, span: &Span, rel: comparisons::Rel,
+        lhs_bounds: Option<(FullInt, FullInt)>, lhs: &Expr, rhs: &Expr, invert: bool) {
+    use utils::comparisons::*;
+
+    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 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 = 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);
+        }
+    }
+}

src/utils/comparisons.rs

@@ -0,0 +1,23 @@
+use rustc_front::hir::{BinOp_, Expr};
+
+#[derive(PartialEq, Eq, Debug, Copy, Clone)]
+pub enum Rel {
+    Lt,
+    Le,
+    Eq,
+    Ne,
+}
+
+/// Put the expression in the form  `lhs < rhs` or `lhs <= rhs`.
+pub fn normalize_comparison<'a>(op: BinOp_, lhs: &'a Expr, rhs: &'a Expr)
+                                -> Option<(Rel, &'a Expr, &'a Expr)> {
+    match op {
+        BinOp_::BiLt => Some((Rel::Lt, lhs, rhs)),
+        BinOp_::BiLe => Some((Rel::Le, lhs, rhs)),
+        BinOp_::BiGt => Some((Rel::Lt, rhs, lhs)),
+        BinOp_::BiGe => Some((Rel::Le, rhs, lhs)),
+        BinOp_::BiEq => Some((Rel::Eq, rhs, lhs)),
+        BinOp_::BiNe => Some((Rel::Ne, rhs, lhs)),
+        _ => None,
+    }
+}

src/utils/mod.rs

@@ -19,6 +19,7 @@ use syntax::codemap::{ExpnInfo, Span, ExpnFormat};
 use syntax::errors::DiagnosticBuilder;
 use syntax::ptr::P;
 
+pub mod comparisons;
 pub mod conf;
 mod hir;
 pub use self::hir::{SpanlessEq, SpanlessHash};

tests/compile-fail/invalid_upcast_comparisons.rs

@@ -0,0 +1,35 @@
+#![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 on `u8_max` prior to casting, this expression is always false
+    (u8_max as u32) > 20;
+
+    (zero as i32) < -5; //~ERROR because of the numeric bounds on `zero` prior to casting, this expression is always false
+    (zero as i32) < 10;
+
+    -5 < (zero as i32); //~ERROR because of the numeric bounds on `zero` prior to casting, this expression is always true
+    0 <= (zero as i32); //~ERROR because of the numeric bounds on `zero` prior to casting, this expression is always true
+    0 < (zero as i32);
+
+    -5 > (zero as i32); //~ERROR because of the numeric bounds on `zero` prior to casting, this expression is always false
+    -5 >= (u8_max as i32); //~ERROR because of the numeric bounds on `u8_max` prior to casting, this expression is always false
+    1337 == (u8_max as i32); //~ERROR because of the numeric bounds on `u8_max` prior to casting, this expression is always false
+
+    -5 == (zero as i32); //~ERROR because of the numeric bounds on `zero` prior to casting, this expression is always false
+    -5 != (u8_max as i32); //~ERROR because of the numeric bounds on `u8_max` prior to casting, this expression is always true
+
+    // Those are Ok:
+    42 == (u8_max as i32);
+    42 != (u8_max as i32);
+    42 > (u8_max as i32);
+    (u8_max as i32) == 42;
+    (u8_max as i32) != 42;
+    (u8_max as i32) > 42;
+    (u8_max as i32) < 42;
+}