diff --git a/README.md b/README.md index c81fb9b52..fee6d066e 100644 --- a/README.md +++ b/README.md @@ -68,7 +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 term's upcasting to be within the range of the other side of the term is always true or false +[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()` diff --git a/src/types.rs b/src/types.rs index 8fede2593..5e4b3e673 100644 --- a/src/types.rs +++ b/src/types.rs @@ -643,32 +643,21 @@ 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::*; + use utils::comparisons::*; type Extr<'a> = ExtremeExpr<'a>; 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 (rel, normalized_lhs, normalized_rhs) = if let Some(val) = normalized { + val + } else { + return None; + }; let lx = detect_extreme_expr(cx, normalized_lhs); let rx = detect_extreme_expr(cx, normalized_rhs); @@ -799,8 +788,7 @@ impl LateLintPass for AbsurdExtremeComparisons { /// **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" + "a comparison involving an upcast which is always true or false" } pub struct InvalidUpcastComparisons; @@ -811,46 +799,39 @@ impl LintPass for InvalidUpcastComparisons { } } +#[derive(Copy, Clone, Debug, Eq, PartialEq)] 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 { + fn cmp_s_u(s: i64, u: u64) -> Ordering { + if s < 0 { Ordering::Less - } else if *u > (i64::max_value() as u64) { + } else if u > (i64::max_value() as u64) { Ordering::Greater } else { - (*s as u64).cmp(u) + (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 { + fn partial_cmp(&self, other: &Self) -> Option { 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(), + (&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) -> std::cmp::Ordering { + fn cmp(&self, other: &Self) -> Ordering { self.partial_cmp(other).expect("partial_cmp for FullInt can never return None") } } @@ -896,19 +877,15 @@ fn node_as_const_fullint(cx: &LateContext, expr: &Expr) -> Option { 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_ - }), + Isize(Is32(x)) => FullInt::S(x as i64), + Isize(Is64(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_, - }), + Usize(Us32(x)) => FullInt::U(x as u64), + Usize(Us64(x)) | U64(x) => FullInt::U(x), Infer(x) => FullInt::U(x as u64), }) @@ -920,59 +897,59 @@ fn node_as_const_fullint(cx: &LateContext, expr: &Expr) -> Option { } } -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"); +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" }, + ) + ); + } +} - let lhs_bounds = numeric_cast_precast_bounds(cx, normalized_lhs); - let rhs_bounds = numeric_cast_precast_bounds(cx, normalized_rhs); +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::*; - 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")); - } - } + if let Some(nlb) = lhs_bounds { + if let Some(norm_rhs_val) = node_as_const_fullint(cx, rhs) { + if match rel { + Rel::Lt => if invert { norm_rhs_val < nlb.0 } else { nlb.1 < norm_rhs_val }, + Rel::Le => if invert { norm_rhs_val <= nlb.0 } else { nlb.1 <= norm_rhs_val }, + } { + err_upcast_comparison(cx, &span, lhs, true) + } else if match rel { + Rel::Lt => if invert { norm_rhs_val >= nlb.1 } else { nlb.0 >= norm_rhs_val }, + Rel::Le => if invert { norm_rhs_val > nlb.1 } else { nlb.0 > norm_rhs_val }, + } { + 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); + } + } +} diff --git a/src/utils/comparisons.rs b/src/utils/comparisons.rs new file mode 100644 index 000000000..2222c31a4 --- /dev/null +++ b/src/utils/comparisons.rs @@ -0,0 +1,19 @@ +use rustc_front::hir::{BinOp_, Expr}; + +#[derive(PartialEq, Eq, Debug, Copy, Clone)] +pub enum Rel { + Lt, + Le, +} + +/// 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)), + _ => None, + } +} diff --git a/src/utils/mod.rs b/src/utils/mod.rs index 300cb8df0..7607ef314 100644 --- a/src/utils/mod.rs +++ b/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}; diff --git a/tests/compile-fail/invalid_upcast_comparisons.rs b/tests/compile-fail/invalid_upcast_comparisons.rs index d5849420e..f94b49592 100644 --- a/tests/compile-fail/invalid_upcast_comparisons.rs +++ b/tests/compile-fail/invalid_upcast_comparisons.rs @@ -7,13 +7,13 @@ 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) > 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 prior to casting, this expression is always false. + (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 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. + -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); }