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

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) {

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,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<P<Expr>>, 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 }
+    }
+}

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
+}