From 498e0fba7f5d614c8165b41204e05c5ad461d25d Mon Sep 17 00:00:00 2001 From: Taylor Cramer Date: Fri, 25 Mar 2016 02:42:27 -0700 Subject: [PATCH] Initial attempt at linting invalid upcast comparisons --- src/lib.rs | 2 + src/types.rs | 223 +++++++++++++++++- .../invalid_upcast_comparisons.rs | 15 ++ 3 files changed, 228 insertions(+), 12 deletions(-) create mode 100644 tests/compile-fail/invalid_upcast_comparisons.rs diff --git a/src/lib.rs b/src/lib.rs index 75970449582..6834753adf0 100644 --- a/src/lib.rs +++ b/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); @@ -367,6 +368,7 @@ pub fn plugin_registrar(reg: &mut Registry) { transmute::TRANSMUTE_PTR_TO_REF, transmute::USELESS_TRANSMUTE, types::ABSURD_EXTREME_COMPARISONS, + types::INVALID_UPCAST_COMPARISONS, types::BOX_VEC, types::CHAR_LIT_AS_U8, types::LET_UNIT_VALUE, diff --git a/src/types.rs b/src/types.rs index 8e9ac1217f0..6f08b57b55a 100644 --- a/src/types.rs +++ b/src/types.rs @@ -1,7 +1,10 @@ use reexport::*; +use rustc_const_eval::*; use rustc::lint::*; use rustc::middle::def; use rustc::ty; +use rustc::middle::const_eval::ConstVal::Integral; +use rustc_const_eval; use rustc_front::hir::*; use rustc_front::intravisit::{FnKind, Visitor, walk_ty}; use rustc_front::util::{is_comparison_binop, binop_to_string}; @@ -9,6 +12,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; @@ -640,24 +644,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)), + _ => return 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.unwrap(); let lx = detect_extreme_expr(cx, normalized_lhs); let rx = detect_extreme_expr(cx, normalized_rhs); @@ -778,3 +790,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 self::FullInt::*; +use std::cmp::Ordering::*; + +impl FullInt { + fn cmp_s_u(s: &i64, u: &u64) -> std::cmp::Ordering { + if *s < 0 { + Less + } else if *u > (i64::max_value() as u64) { + Greater + } else { + (*s as u64).cmp(u) + } + } +} + +impl PartialEq for FullInt { + fn eq(&self, other: &Self) -> bool { + self.cmp(other) == Equal + } +} +impl Eq for FullInt {} + +impl PartialOrd for FullInt { + fn partial_cmp(&self, other: &Self) -> Option { + Some(match (self, other) { + (&S(ref s), &S(ref o)) => s.cmp(o), + (&U(ref s), &U(ref o)) => s.cmp(o), + (&S(ref s), &U(ref o)) => Self::cmp_s_u(s, o), + (&U(ref s), &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).unwrap() + } +} + + +fn numeric_cast_precast_bounds<'a>(cx: &LateContext, expr: &'a Expr) -> Option<(FullInt, FullInt)> { + use rustc::middle::const_eval::EvalHint::ExprTypeChecked; + + if let ExprCast(ref cast_exp,_) = expr.node { + let cv = match const_eval::eval_const_expr_partial(cx.tcx, cast_exp, ExprTypeChecked, None) { + Ok(val) => val, + Err(_) => return None, + }; + + if let Integral(const_int) = cv { + Some(match const_int { + I8(_) => (S(i8::min_value() as i64), S(i8::max_value() as i64)), + I16(_) => (S(i16::min_value() as i64), S(i16::max_value() as i64)), + I32(_) => (S(i32::min_value() as i64), S(i32::max_value() as i64)), + Isize(_) | + I64(_) | + InferSigned(_) => (S(i64::max_value()), S(i64::max_value())), + U8(_) => (U(u8::min_value() as u64), U(u8::max_value() as u64)), + U16(_) => (U(u16::min_value() as u64), U(u16::max_value() as u64)), + U32(_) => (U(u32::min_value() as u64), U(u32::max_value() as u64)), + Usize(_) | + U64(_) | + Infer(_) => (U(u64::max_value()), U(u64::max_value())), + }) + } else { + None + } + } else { + None + } +} + +fn node_as_const_fullint(cx: &LateContext, expr: &Expr) -> Option { + use rustc::middle::const_eval::EvalHint::ExprTypeChecked; + + 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) => S(x as i64), + I16(x) => S(x as i64), + I32(x) => S(x as i64), + Isize(x) => S(match x { + Is32(x_) => x_ as i64, + Is64(x_) => x_ + }), + I64(x) => S(x), + InferSigned(x) => S(x as i64), + U8(x) => U(x as u64), + U16(x) => U(x as u64), + U32(x) => U(x as u64), + Usize(x) => U(match x { + Us32(x_) => x_ as u64, + Us64(x_) => x_, + }), + U64(x) => U(x), + Infer(x) => U(x as u64), + }) + } else { + None + } + }, + Err(_) => return 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.unwrap(); + + let norm_lhs_bounds = numeric_cast_precast_bounds(cx, normalized_lhs); + let norm_rhs_bounds = numeric_cast_precast_bounds(cx, normalized_rhs); + + if let Some(nlb) = norm_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!("")); + } 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!("")); + } + } + } else if let Some(nrb) = norm_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!("")); + } 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!("")); + } + } + } + } + } +} diff --git a/tests/compile-fail/invalid_upcast_comparisons.rs b/tests/compile-fail/invalid_upcast_comparisons.rs new file mode 100644 index 00000000000..63ccb6efd9d --- /dev/null +++ b/tests/compile-fail/invalid_upcast_comparisons.rs @@ -0,0 +1,15 @@ +#![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 + (u8_max as u32) > 20; + + (zero as i32) < -5; //~ERROR + (zero as i32) < 10; +}