diff --git a/README.md b/README.md index cbb8646c672..4d655c85951 100644 --- a/README.md +++ b/README.md @@ -10,7 +10,7 @@ There are 117 lints included in this crate: name | default | meaning ---------------------------------------------------------------------------------------------------------------|---------|------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ -[absurd_unsigned_comparisons](https://github.com/Manishearth/rust-clippy/wiki#absurd_unsigned_comparisons) | warn | testing whether an unsigned integer is non-positive +[absurd_extreme_comparisons](https://github.com/Manishearth/rust-clippy/wiki#absurd_extreme_comparisons) | warn | a comparison involving a maximum or minimum value involves a case that is always true or always false [approx_constant](https://github.com/Manishearth/rust-clippy/wiki#approx_constant) | warn | the approximate of a known float constant (in `std::f64::consts` or `std::f32::consts`) is found; suggests to use the constant [bad_bit_mask](https://github.com/Manishearth/rust-clippy/wiki#bad_bit_mask) | warn | expressions of the form `_ & mask == select` that will only ever return `true` or `false` (because in the example `select` containing bits that `mask` doesn't have) [block_in_if_condition_expr](https://github.com/Manishearth/rust-clippy/wiki#block_in_if_condition_expr) | warn | braces can be eliminated in conditions that are expressions, e.g `if { true } ...` diff --git a/src/lib.rs b/src/lib.rs index 728aa124a18..cd9ac226321 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -156,7 +156,7 @@ pub fn plugin_registrar(reg: &mut Registry) { reg.register_late_lint_pass(box print::PrintLint); reg.register_late_lint_pass(box vec::UselessVec); reg.register_late_lint_pass(box drop_ref::DropRefPass); - reg.register_late_lint_pass(box types::AbsurdUnsignedComparisons); + reg.register_late_lint_pass(box types::AbsurdExtremeComparisons); reg.register_late_lint_pass(box regex::RegexPass); reg.register_late_lint_pass(box copies::CopyAndPaste); @@ -271,7 +271,7 @@ pub fn plugin_registrar(reg: &mut Registry) { strings::STRING_LIT_AS_BYTES, temporary_assignment::TEMPORARY_ASSIGNMENT, transmute::USELESS_TRANSMUTE, - types::ABSURD_UNSIGNED_COMPARISONS, + types::ABSURD_EXTREME_COMPARISONS, types::BOX_VEC, types::CHAR_LIT_AS_U8, types::LET_UNIT_VALUE, diff --git a/src/types.rs b/src/types.rs index 7034b0d2ddb..a48172c5980 100644 --- a/src/types.rs +++ b/src/types.rs @@ -5,6 +5,7 @@ use rustc_front::util::{is_comparison_binop, binop_to_string}; use syntax::codemap::Span; use rustc_front::intravisit::{FnKind, Visitor, walk_ty}; use rustc::middle::ty; +use rustc::middle::const_eval; use syntax::ast::{IntTy, UintTy, FloatTy}; use utils::*; @@ -579,54 +580,160 @@ impl LateLintPass for CharLitAsU8 { } } -/// **What it does:** This lint checks for expressions where an unsigned integer is tested to be non-positive and suggests testing for equality with zero instead. +/// **What it does:** This lint checks for comparisons where one side of the relation is either the minimum or maximum value for its type and warns if it involves a case that is always true or always false. Only integer and boolean types are checked. /// -/// **Why is this bad?** `x <= 0` may mislead the reader into thinking `x` can be negative. `x == 0` makes explicit that zero is the only possibility. +/// **Why is this bad?** An expression like `min <= x` may misleadingly imply that is is possible for `x` to be less than the minimum. Expressions like `max < x` are probably mistakes. /// /// **Known problems:** None /// -/// **Example:** `vec.len() <= 0` +/// **Example:** `vec.len() <= 0`, `100 > std::i32::MAX` declare_lint! { - pub ABSURD_UNSIGNED_COMPARISONS, Warn, - "testing whether an unsigned integer is non-positive" + pub ABSURD_EXTREME_COMPARISONS, Warn, + "a comparison involving a maximum or minimum value involves a case that is always \ + true or always false" } -pub struct AbsurdUnsignedComparisons; +pub struct AbsurdExtremeComparisons; -impl LintPass for AbsurdUnsignedComparisons { +impl LintPass for AbsurdExtremeComparisons { fn get_lints(&self) -> LintArray { - lint_array!(ABSURD_UNSIGNED_COMPARISONS) + lint_array!(ABSURD_EXTREME_COMPARISONS) } } -fn is_zero_lit(expr: &Expr) -> bool { - use syntax::ast::Lit_; +enum ExtremeType { + Minimum, + Maximum, +} - if let ExprLit(ref l) = expr.node { - if let Lit_::LitInt(val, _) = l.node { - return val == 0; +struct ExtremeExpr<'a> { + which: ExtremeType, + expr: &'a Expr, +} + +enum AbsurdComparisonResult { + AlwaysFalse, + AlwaysTrue, + 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::*; + type Extr<'a> = ExtremeExpr<'a>; + + // Put the expression in the form lhs < rhs or lhs <= rhs. + enum Rel { Lt, Le }; + let (rel, lhs2, rhs2) = 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 lx = detect_extreme_expr(cx, lhs2); + let rx = detect_extreme_expr(cx, rhs2); + + Some(match rel { + Rel::Lt => { + match (lx, rx) { + (Some(l @ Extr { which: Maximum, ..}), _) => (l, AlwaysFalse), // max < x + (_, Some(r @ Extr { which: Minimum, ..})) => (r, AlwaysFalse), // x < min + _ => return None, + } } - } - false + Rel::Le => { + match (lx, rx) { + (Some(l @ Extr { which: Minimum, ..}), _) => (l, AlwaysTrue), // min <= x + (Some(l @ Extr { which: Maximum, ..}), _) => (l, InequalityImpossible), //max <= x + (_, Some(r @ Extr { which: Minimum, ..})) => (r, InequalityImpossible), // x <= min + (_, Some(r @ Extr { which: Maximum, ..})) => (r, AlwaysTrue), // x <= max + _ => return None, + } + } + }) } -impl LateLintPass for AbsurdUnsignedComparisons { +fn detect_extreme_expr<'a>(cx: &LateContext, expr: &'a Expr) -> Option> { + use rustc::middle::const_eval::EvalHint::ExprTypeChecked; + use types::ExtremeType::*; + use rustc::middle::const_eval::ConstVal::*; + + let ty = &cx.tcx.expr_ty(expr).sty; + + match *ty { + ty::TyBool | ty::TyInt(_) | ty::TyUint(_) => (), + _ => return None, + }; + + let cv = match const_eval::eval_const_expr_partial(cx.tcx, expr, ExprTypeChecked, None) { + Ok(val) => val, + Err(_) => return None, + }; + + let which = match (ty, cv) { + (&ty::TyBool, Bool(false)) => Minimum, + + (&ty::TyInt(IntTy::TyIs), Int(x)) if x == ::std::isize::MIN as i64 => Minimum, + (&ty::TyInt(IntTy::TyI8), Int(x)) if x == ::std::i8::MIN as i64 => Minimum, + (&ty::TyInt(IntTy::TyI16), Int(x)) if x == ::std::i16::MIN as i64 => Minimum, + (&ty::TyInt(IntTy::TyI32), Int(x)) if x == ::std::i32::MIN as i64 => Minimum, + (&ty::TyInt(IntTy::TyI64), Int(x)) if x == ::std::i64::MIN as i64 => Minimum, + + (&ty::TyUint(UintTy::TyUs), Uint(x)) if x == ::std::usize::MIN as u64 => Minimum, + (&ty::TyUint(UintTy::TyU8), Uint(x)) if x == ::std::u8::MIN as u64 => Minimum, + (&ty::TyUint(UintTy::TyU16), Uint(x)) if x == ::std::u16::MIN as u64 => Minimum, + (&ty::TyUint(UintTy::TyU32), Uint(x)) if x == ::std::u32::MIN as u64 => Minimum, + (&ty::TyUint(UintTy::TyU64), Uint(x)) if x == ::std::u64::MIN as u64 => Minimum, + + (&ty::TyBool, Bool(true)) => Maximum, + + (&ty::TyInt(IntTy::TyIs), Int(x)) if x == ::std::isize::MAX as i64 => Maximum, + (&ty::TyInt(IntTy::TyI8), Int(x)) if x == ::std::i8::MAX as i64 => Maximum, + (&ty::TyInt(IntTy::TyI16), Int(x)) if x == ::std::i16::MAX as i64 => Maximum, + (&ty::TyInt(IntTy::TyI32), Int(x)) if x == ::std::i32::MAX as i64 => Maximum, + (&ty::TyInt(IntTy::TyI64), Int(x)) if x == ::std::i64::MAX as i64 => Maximum, + + (&ty::TyUint(UintTy::TyUs), Uint(x)) if x == ::std::usize::MAX as u64 => Maximum, + (&ty::TyUint(UintTy::TyU8), Uint(x)) if x == ::std::u8::MAX as u64 => Maximum, + (&ty::TyUint(UintTy::TyU16), Uint(x)) if x == ::std::u16::MAX as u64 => Maximum, + (&ty::TyUint(UintTy::TyU32), Uint(x)) if x == ::std::u32::MAX as u64 => Maximum, + (&ty::TyUint(UintTy::TyU64), Uint(x)) if x == ::std::u64::MAX as u64 => Maximum, + + _ => return None, + }; + Some(ExtremeExpr { which: which, expr: expr }) +} + +impl LateLintPass for AbsurdExtremeComparisons { fn check_expr(&mut self, cx: &LateContext, expr: &Expr) { + use types::ExtremeType::*; + use types::AbsurdComparisonResult::*; + if let ExprBinary(ref cmp, ref lhs, ref rhs) = expr.node { - let op = cmp.node; - - let comparee = match op { - BiLe if is_zero_lit(rhs) => lhs, // x <= 0 - BiGe if is_zero_lit(lhs) => rhs, // 0 >= x - _ => return, - }; - - if let ty::TyUint(_) = cx.tcx.expr_ty(comparee).sty { + if let Some((culprit, result)) = detect_absurd_comparison(cx, cmp.node, lhs, rhs) { if !in_macro(cx, expr.span) { - let msg = "testing whether an unsigned integer is non-positive"; - let help = format!("consider using {} == 0 instead", - snippet(cx, comparee.span, "x")); - span_help_and_lint(cx, ABSURD_UNSIGNED_COMPARISONS, expr.span, msg, &help); + let msg = "this comparison involving the minimum or maximum element for this \ + type contains a case that is always true or always false"; + + let conclusion = match result { + AlwaysFalse => "this comparison is always false".to_owned(), + AlwaysTrue => "this comparison is always true".to_owned(), + InequalityImpossible => + format!("the case where the two sides are not equal never occurs, \ + consider using {} == {} instead", + snippet(cx, lhs.span, "lhs"), + snippet(cx, rhs.span, "rhs")), + }; + + let help = format!("because {} is the {} value for this type, {}", + snippet(cx, culprit.expr.span, "x"), + match culprit.which { Minimum => "minimum", Maximum => "maximum" }, + conclusion); + + span_help_and_lint(cx, ABSURD_EXTREME_COMPARISONS, expr.span, msg, &help); } } } diff --git a/tests/compile-fail/absurd-extreme-comparisons.rs b/tests/compile-fail/absurd-extreme-comparisons.rs new file mode 100644 index 00000000000..9718225d203 --- /dev/null +++ b/tests/compile-fail/absurd-extreme-comparisons.rs @@ -0,0 +1,44 @@ +#![feature(plugin)] +#![plugin(clippy)] + +#![deny(absurd_extreme_comparisons)] +#![allow(unused, eq_op)] +fn main() { + const Z: u32 = 0; + + let u: u32 = 42; + + u <= 0; //~ERROR this comparison involving the minimum or maximum element for this type contains a case that is always true or always false + u <= Z; //~ERROR this comparison involving + u < Z; //~ERROR this comparison involving + Z >= u; //~ERROR this comparison involving + Z > u; //~ERROR this comparison involving + u > std::u32::MAX; //~ERROR this comparison involving + u >= std::u32::MAX; //~ERROR this comparison involving + std::u32::MAX < u; //~ERROR this comparison involving + std::u32::MAX <= u; //~ERROR this comparison involving + + 1-1 > u; + //~^ ERROR this comparison involving + //~| HELP because 1-1 is the minimum value for this type, this comparison is always false + u >= !0; + //~^ ERROR this comparison involving + //~| HELP because !0 is the maximum value for this type, the case where the two sides are not equal never occurs, consider using u == !0 instead + u <= 12 - 2*6; + //~^ ERROR this comparison involving + //~| HELP because 12 - 2*6 is the minimum value for this type, the case where the two sides are not equal never occurs, consider using u == 12 - 2*6 instead + + let i: i8 = 0; + i < -127 - 1; //~ERROR this comparison involving + std::i8::MAX >= i; //~ERROR this comparison involving + 3-7 < std::i32::MIN; //~ERROR this comparison involving + + let b = false; + b >= true; //~ERROR this comparison involving + false > b; //~ERROR this comparison involving + + u > 0; // ok + + // this is handled by unit_cmp + () < {}; //~WARNING <-comparison of unit values detected. +} diff --git a/tests/compile-fail/absurd_unsigned_comparisons.rs b/tests/compile-fail/absurd_unsigned_comparisons.rs deleted file mode 100644 index d7817daf204..00000000000 --- a/tests/compile-fail/absurd_unsigned_comparisons.rs +++ /dev/null @@ -1,14 +0,0 @@ -#![feature(plugin)] -#![plugin(clippy)] - -#![allow(unused)] - -#[deny(absurd_unsigned_comparisons)] -fn main() { - 1u32 <= 0; //~ERROR testing whether an unsigned integer is non-positive - 1u8 <= 0; //~ERROR testing whether an unsigned integer is non-positive - 1i32 <= 0; - 0 >= 1u32; //~ERROR testing whether an unsigned integer is non-positive - 0 >= 1; - 1u32 > 0; -}