Merge pull request #300 from Manishearth/minmax

new lint: min_max

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)                                   | warn    | `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 }`

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),
+                    (true, true) => rv.cmp(lv),
-                    (false, false) => rv.cmp(lv),
+                    (false, false) => lv.cmp(rv),
-                    (true, false) => Greater,
+                    (true, false) => Less,
-                    (false, true) => Less,
+                    (false, true) => Greater,
                 }),
             (&ConstantFloat(ref ls, lw), &ConstantFloat(ref rs, rw)) =>
                 if match (lw, rw) {
@@ -154,8 +154,8 @@ impl PartialOrd for Constant {
 fn lit_to_constant(lit: &Lit_) -> Constant {
     match *lit {
         LitStr(ref is, style) => ConstantStr(is.to_string(), style),
-        LitBinary(ref blob) => ConstantBinary(blob.clone()),
         LitByte(b) => ConstantByte(b),
+        LitByteStr(ref s) => ConstantBinary(s.clone()),
         LitChar(c) => ConstantChar(c),
         LitInt(value, ty) => ConstantInt(value, ty),
         LitFloat(ref is, ty) => ConstantFloat(is.to_string(), ty.into()),

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,

src/minmax.rs

@@ -0,0 +1,73 @@
+use rustc::lint::{Context, LintPass, LintArray};
+use rustc_front::hir::*;
+use syntax::ptr::P;
+use std::cmp::PartialOrd;
+use std::cmp::Ordering::*;
+
+use consts::{Constant, constant_simple};
+use utils::{match_path, span_lint};
+use self::MinMax::{Min, Max};
+
+declare_lint!(pub MIN_MAX, Warn,
+    "`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(expr) {
+            if let Some((inner_max, inner_c, _)) = min_max(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(expr: &Expr) -> Option<(MinMax, Constant, &Expr)> {
+    if let ExprCall(ref path, ref args) = expr.node {
+        if let ExprPath(None, ref path) = path.node {
+            if match_path(path, &["std", "cmp", "min"]) {
+                fetch_const(args, Min)
+            } else {
+                if match_path(path, &["std", "cmp", "max"]) {
+                    fetch_const(args, Max)
+                } else {
+                    None
+                }
+            }
+        } else { None }
+    } else { None }
+ }
+
+fn fetch_const(args: &[P<Expr>], m: MinMax) ->
+        Option<(MinMax, Constant, &Expr)> {
+    if args.len() != 2 { return None }
+    if let Some(c) = constant_simple(&args[0]) {
+        if let None = constant_simple(&args[1]) { // otherwise ignore
+            Some((m, c, &args[1]))
+        } else { None }
+    } else {
+        if let Some(c) = constant_simple(&args[1]) {
+            Some((m, c, &args[0]))
+        } else { None }
+    }
+}

tests/compile-fail/min_max.rs

@@ -0,0 +1,29 @@
+#![feature(plugin)]
+
+#![plugin(clippy)]
+#![deny(clippy)]
+
+use std::cmp::{min, max};
+
+const LARGE : usize = 3;
+
+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
+
+    min(1, max(LARGE, x)); // no error, we don't lookup consts here
+
+    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
+}