diff --git a/README.md b/README.md index 0046d891129..590ba60d552 100644 --- a/README.md +++ b/README.md @@ -4,7 +4,7 @@ A collection of lints that give helpful tips to newbies and catch oversights. ##Lints -There are 54 lints included in this crate: +There are 55 lints included in this crate: name | default | meaning -----------------------------------------------------------------------------------------------------|---------|----------------------------------------------------------------------------------------------------------------------------------------------------------------------------- @@ -31,6 +31,7 @@ name [let_unit_value](https://github.com/Manishearth/rust-clippy/wiki#let_unit_value) | warn | creating a let binding to a value of unit type, which usually can't be used afterwards [linkedlist](https://github.com/Manishearth/rust-clippy/wiki#linkedlist) | warn | usage of LinkedList, usually a vector is faster, or a more specialized data structure like a RingBuf [match_ref_pats](https://github.com/Manishearth/rust-clippy/wiki#match_ref_pats) | warn | a match has all arms prefixed with `&`; the match expression can be dereferenced instead +[min_max](https://github.com/Manishearth/rust-clippy/wiki#min_max) | deny | `min(_, max(_, _))` (or vice versa) with bounds clamping the result to a constant [modulo_one](https://github.com/Manishearth/rust-clippy/wiki#modulo_one) | warn | taking a number modulo 1, which always returns 0 [mut_mut](https://github.com/Manishearth/rust-clippy/wiki#mut_mut) | warn | usage of double-mut refs, e.g. `&mut &mut ...` (either copy'n'paste error, or shows a fundamental misunderstanding of references) [needless_bool](https://github.com/Manishearth/rust-clippy/wiki#needless_bool) | warn | if-statements with plain booleans in the then- and else-clause, e.g. `if p { true } else { false }` diff --git a/src/consts.rs b/src/consts.rs index a8a446a703f..5e7cd200d0f 100644 --- a/src/consts.rs +++ b/src/consts.rs @@ -121,10 +121,10 @@ impl PartialOrd for Constant { (&ConstantInt(ref lv, lty), &ConstantInt(ref rv, rty)) => Some(match (is_negative(lty) && *lv != 0, is_negative(rty) && *rv != 0) { - (true, true) => lv.cmp(rv), - (false, false) => rv.cmp(lv), - (true, false) => Greater, - (false, true) => Less, + (true, true) => rv.cmp(lv), + (false, false) => lv.cmp(rv), + (true, false) => Less, + (false, true) => Greater, }), (&ConstantFloat(ref ls, lw), &ConstantFloat(ref rs, rw)) => if match (lw, rw) { diff --git a/src/lib.rs b/src/lib.rs index a4aee0c27fd..7665d06b193 100755 --- a/src/lib.rs +++ b/src/lib.rs @@ -32,6 +32,7 @@ pub mod needless_bool; pub mod approx_const; pub mod eta_reduction; pub mod identity_op; +pub mod minmax; pub mod mut_mut; pub mod len_zero; pub mod attrs; @@ -85,6 +86,7 @@ pub fn plugin_registrar(reg: &mut Registry) { reg.register_lint_pass(box types::TypeComplexityPass as LintPassObject); reg.register_lint_pass(box matches::MatchPass as LintPassObject); reg.register_lint_pass(box misc::PatternPass as LintPassObject); + reg.register_lint_pass(box minmax::MinMaxPass as LintPassObject); reg.register_lint_group("clippy_pedantic", vec![ methods::OPTION_UNWRAP_USED, @@ -125,6 +127,7 @@ pub fn plugin_registrar(reg: &mut Registry) { methods::STR_TO_STRING, methods::STRING_TO_STRING, methods::WRONG_SELF_CONVENTION, + minmax::MIN_MAX, misc::CMP_NAN, misc::CMP_OWNED, misc::FLOAT_CMP, diff --git a/src/minmax.rs b/src/minmax.rs new file mode 100644 index 00000000000..2e4c9256657 --- /dev/null +++ b/src/minmax.rs @@ -0,0 +1,90 @@ +use rustc::lint::{Context, LintPass, LintArray}; +use rustc_front::hir::*; +use syntax::codemap::Spanned; +use syntax::ptr::P; +use std::cmp::PartialOrd; +use std::cmp::Ordering::*; + +use consts::{Constant, constant}; +use utils::{match_path, span_lint}; +use self::MinMax::{Min, Max}; + +declare_lint!(pub MIN_MAX, Deny, + "`min(_, max(_, _))` (or vice versa) with bounds clamping the result \ + to a constant"); + +#[allow(missing_copy_implementations)] +pub struct MinMaxPass; + +impl LintPass for MinMaxPass { + fn get_lints(&self) -> LintArray { + lint_array!(MIN_MAX) + } + + fn check_expr(&mut self, cx: &Context, expr: &Expr) { + if let Some((outer_max, outer_c, oe)) = min_max(cx, expr) { + if let Some((inner_max, inner_c, _)) = min_max(cx, oe) { + if outer_max == inner_max { return; } + match (outer_max, outer_c.partial_cmp(&inner_c)) { + (_, None) | (Max, Some(Less)) | (Min, Some(Greater)) => (), + _ => { + span_lint(cx, MIN_MAX, expr.span, + "this min/max combination leads to constant result") + }, + } + } + } + } +} + +#[derive(PartialEq, Eq, Debug)] +enum MinMax { + Min, + Max, +} + +fn min_max<'e>(cx: &Context, expr: &'e Expr) -> + Option<(MinMax, Constant, &'e Expr)> { + match expr.node { + ExprMethodCall(Spanned{node: ref ident, ..}, _, ref args) => { + let name = ident.name; + if name == "min" { + fetch_const(cx, args, Min) + } else { + if name == "max" { + fetch_const(cx, args, Max) + } else { + None + } + } + }, + ExprCall(ref path, ref args) => { + if let &ExprPath(None, ref path) = &path.node { + if match_path(path, &["min"]) { + fetch_const(cx, args, Min) + } else { + if match_path(path, &["max"]) { + fetch_const(cx, args, Max) + } else { + None + } + } + } else { None } + }, + _ => None, + } + } + +fn fetch_const<'e>(cx: &Context, args: &'e Vec>, m: MinMax) -> + Option<(MinMax, Constant, &'e Expr)> { + if args.len() != 2 { return None } + if let Some((c, _)) = constant(cx, &args[0]) { + if let None = constant(cx, &args[1]) { // otherwise ignore + Some((m, c, &args[1])) + } else { None } + } else { + if let Some((c, _)) = constant(cx, &args[1]) { + Some((m, c, &args[0])) + } else { None } + } +} diff --git a/tests/compile-fail/min_max.rs b/tests/compile-fail/min_max.rs new file mode 100644 index 00000000000..18f415ddc0b --- /dev/null +++ b/tests/compile-fail/min_max.rs @@ -0,0 +1,25 @@ +#![feature(plugin)] + +#![plugin(clippy)] +#![deny(clippy)] + +use std::cmp::{min, max}; + +fn main() { + let x; + x = 2usize; + min(1, max(3, x)); //~ERROR this min/max combination leads to constant result + min(max(3, x), 1); //~ERROR this min/max combination leads to constant result + max(min(x, 1), 3); //~ERROR this min/max combination leads to constant result + max(3, min(x, 1)); //~ERROR this min/max combination leads to constant result + + min(3, max(1, x)); // ok, could be 1, 2 or 3 depending on x + + let s; + s = "Hello"; + + min("Apple", max("Zoo", s)); //~ERROR this min/max combination leads to constant result + max(min(s, "Apple"), "Zoo"); //~ERROR this min/max combination leads to constant result + + max("Apple", min(s, "Zoo")); // ok +}