Fix #2741

Author: Michael Wright

clippy_lints/src/minmax.rs

@@ -1,8 +1,9 @@
 use crate::consts::{constant_simple, Constant};
-use rustc::lint::*;
+use crate::utils::{match_def_path, opt_def_id, paths, sext, span_lint};
 use rustc::hir::*;
+use rustc::lint::*;
+use rustc::ty::{self, TyCtxt};
 use std::cmp::{Ordering, PartialOrd};
-use crate::utils::{match_def_path, opt_def_id, paths, span_lint};
 
 /// **What it does:** Checks for expressions where `std::cmp::min` and `max` are
 /// used to clamp values, but switched so that the result is constant.
@@ -36,21 +37,43 @@ impl LintPass for MinMaxPass {
 impl<'a, 'tcx> LateLintPass<'a, 'tcx> for MinMaxPass {
     fn check_expr(&mut self, cx: &LateContext<'a, 'tcx>, expr: &'tcx 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 let Some((inner_max, inner_c, ie)) = min_max(cx, oe) {
                 if outer_max == inner_max {
                     return;
                 }
-                match (outer_max, outer_c.partial_cmp(&inner_c)) {
+                match (
+                    outer_max,
+                    const_partial_cmp(cx.tcx, &outer_c, &inner_c, &cx.tables.expr_ty(ie).sty),
+                ) {
                     (_, None) | (MinMax::Max, Some(Ordering::Less)) | (MinMax::Min, Some(Ordering::Greater)) => (),
                     _ => {
-                        span_lint(cx, MIN_MAX, expr.span, "this min/max combination leads to constant result");
+                        span_lint(
+                            cx,
+                            MIN_MAX,
+                            expr.span,
+                            "this min/max combination leads to constant result",
+                        );
                     },
                 }
             }
         }
     }
 }
 
+// Constant::partial_cmp incorrectly orders signed integers
+fn const_partial_cmp(tcx: TyCtxt, a: &Constant, b: &Constant, expr_ty: &ty::TypeVariants) -> Option<Ordering> {
+    match *expr_ty {
+        ty::TyInt(int_ty) => {
+            if let (&Constant::Int(a), &Constant::Int(b)) = (a, b) {
+                Some(sext(tcx, a, int_ty).cmp(&sext(tcx, b, int_ty)))
+            } else {
+                None
+            }
+        },
+        _ => a.partial_cmp(&b),
+    }
+}
+
 #[derive(PartialEq, Eq, Debug)]
 enum MinMax {
     Min,

tests/ui/min_max.rs

@@ -23,6 +23,9 @@ fn main() {
 
     min(1, max(LARGE, x)); // no error, we don't lookup consts here
 
+    let y = 2isize;
+    min(max(y, -1), 3);
+
     let s;
     s = "Hello";
 

tests/ui/min_max.stderr

@@ -31,15 +31,15 @@ error: this min/max combination leads to constant result
    |     ^^^^^^^^^^^^^^^^^^^^^^^
 
 error: this min/max combination leads to constant result
-  --> $DIR/min_max.rs:29:5
+  --> $DIR/min_max.rs:32:5
    |
-29 |     min("Apple", max("Zoo", s));
+32 |     min("Apple", max("Zoo", s));
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
 error: this min/max combination leads to constant result
-  --> $DIR/min_max.rs:30:5
+  --> $DIR/min_max.rs:33:5
    |
-30 |     max(min(s, "Apple"), "Zoo");
+33 |     max(min(s, "Apple"), "Zoo");
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
 error: aborting due to 7 previous errors