Skip to content

Commit

Permalink
Rollup merge of #5871 - wiomoc:feature/methodcall-minmax, r=flip1995
Browse files Browse the repository at this point in the history 
Lint .min(x).max(y) with x < y

Fixes  #5854

changelog: Also lint `ord.min(a).max(b)`, where `a < b` in [`min_max`] lint
  • Loading branch information
@flip1995
flip1995 authored Aug 10, 2020
2 parents 08ab29b + 87e7409 commit ee8db50
Show file tree
Hide file tree
Showing 3 changed files with 115 additions and 24 deletions.
55 changes: 39 additions & 16 deletions clippy_lints/src/minmax.rs
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
use crate::consts::{constant_simple, Constant};
use crate::utils::{match_def_path, paths, span_lint};
use crate::utils::{match_def_path, match_trait_method, paths, span_lint};
use if_chain::if_chain;
use rustc_hir::{Expr, ExprKind};
use rustc_lint::{LateContext, LateLintPass};
use rustc_session::{declare_lint_pass, declare_tool_lint};
Expand All @@ -18,6 +19,10 @@ declare_clippy_lint! {
/// ```ignore
/// min(0, max(100, x))
/// ```
/// or
/// ```ignore
/// x.max(100).min(0)
/// ```
/// It will always be equal to `0`. Probably the author meant to clamp the value
/// between 0 and 100, but has erroneously swapped `min` and `max`.
pub MIN_MAX,
Expand Down Expand Up @@ -60,25 +65,43 @@ enum MinMax {
}

fn min_max<'a>(cx: &LateContext<'_>, expr: &'a Expr<'a>) -> Option<(MinMax, Constant, &'a Expr<'a>)> {
if let ExprKind::Call(ref path, ref args) = expr.kind {
if let ExprKind::Path(ref qpath) = path.kind {
cx.typeck_results()
.qpath_res(qpath, path.hir_id)
.opt_def_id()
.and_then(|def_id| {
if match_def_path(cx, def_id, &paths::CMP_MIN) {
fetch_const(cx, args, MinMax::Min)
} else if match_def_path(cx, def_id, &paths::CMP_MAX) {
match expr.kind {
ExprKind::Call(ref path, ref args) => {
if let ExprKind::Path(ref qpath) = path.kind {
cx.typeck_results()
.qpath_res(qpath, path.hir_id)
.opt_def_id()
.and_then(|def_id| {
if match_def_path(cx, def_id, &paths::CMP_MIN) {
fetch_const(cx, args, MinMax::Min)
} else if match_def_path(cx, def_id, &paths::CMP_MAX) {
fetch_const(cx, args, MinMax::Max)
} else {
None
}
})
} else {
None
}
},
ExprKind::MethodCall(ref path, _, ref args, _) => {
if_chain! {
if let [obj, _] = args;
if cx.typeck_results().expr_ty(obj).is_floating_point() || match_trait_method(cx, expr, &paths::ORD);
then {
if path.ident.as_str() == sym!(max).as_str() {
fetch_const(cx, args, MinMax::Max)
} else if path.ident.as_str() == sym!(min).as_str() {
fetch_const(cx, args, MinMax::Min)
} else {
None
}
})
} else {
None
}
} else {
None
} else {
None
}
}
},
_ => None,
}
}

Expand Down
32 changes: 32 additions & 0 deletions tests/ui/min_max.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,18 @@ use std::cmp::{max, min};

const LARGE: usize = 3;

struct NotOrd(u64);

impl NotOrd {
fn min(self, x: u64) -> NotOrd {
NotOrd(x)
}

fn max(self, x: u64) -> NotOrd {
NotOrd(x)
}
}

fn main() {
let x;
x = 2usize;
Expand All @@ -30,4 +42,24 @@ fn main() {
max(min(s, "Apple"), "Zoo");

max("Apple", min(s, "Zoo")); // ok

let f = 3f32;
x.min(1).max(3);
x.max(3).min(1);
f.max(3f32).min(1f32);

x.max(1).min(3); // ok
x.min(3).max(1); // ok
f.min(3f32).max(1f32); // ok

max(x.min(1), 3);
min(x.max(1), 3); // ok

s.max("Zoo").min("Apple");
s.min("Apple").max("Zoo");

s.min("Zoo").max("Apple"); // ok

let not_ord = NotOrd(1);
not_ord.min(1).max(3); // ok
}
52 changes: 44 additions & 8 deletions tests/ui/min_max.stderr
View file
Original file line number Diff line number Diff line change
@@ -1,46 +1,82 @@
error: this `min`/`max` combination leads to constant result
--> $DIR/min_max.rs:12:5
--> $DIR/min_max.rs:24:5
|
LL | min(1, max(3, x));
| ^^^^^^^^^^^^^^^^^
|
= note: `-D clippy::min-max` implied by `-D warnings`

error: this `min`/`max` combination leads to constant result
--> $DIR/min_max.rs:13:5
--> $DIR/min_max.rs:25:5
|
LL | min(max(3, x), 1);
| ^^^^^^^^^^^^^^^^^

error: this `min`/`max` combination leads to constant result
--> $DIR/min_max.rs:14:5
--> $DIR/min_max.rs:26:5
|
LL | max(min(x, 1), 3);
| ^^^^^^^^^^^^^^^^^

error: this `min`/`max` combination leads to constant result
--> $DIR/min_max.rs:15:5
--> $DIR/min_max.rs:27:5
|
LL | max(3, min(x, 1));
| ^^^^^^^^^^^^^^^^^

error: this `min`/`max` combination leads to constant result
--> $DIR/min_max.rs:17:5
--> $DIR/min_max.rs:29:5
|
LL | my_max(3, my_min(x, 1));
| ^^^^^^^^^^^^^^^^^^^^^^^

error: this `min`/`max` combination leads to constant result
--> $DIR/min_max.rs:29:5
--> $DIR/min_max.rs:41:5
|
LL | min("Apple", max("Zoo", s));
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^

error: this `min`/`max` combination leads to constant result
--> $DIR/min_max.rs:30:5
--> $DIR/min_max.rs:42:5
|
LL | max(min(s, "Apple"), "Zoo");
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^

error: aborting due to 7 previous errors
error: this `min`/`max` combination leads to constant result
--> $DIR/min_max.rs:47:5
|
LL | x.min(1).max(3);
| ^^^^^^^^^^^^^^^

error: this `min`/`max` combination leads to constant result
--> $DIR/min_max.rs:48:5
|
LL | x.max(3).min(1);
| ^^^^^^^^^^^^^^^

error: this `min`/`max` combination leads to constant result
--> $DIR/min_max.rs:49:5
|
LL | f.max(3f32).min(1f32);
| ^^^^^^^^^^^^^^^^^^^^^

error: this `min`/`max` combination leads to constant result
--> $DIR/min_max.rs:55:5
|
LL | max(x.min(1), 3);
| ^^^^^^^^^^^^^^^^

error: this `min`/`max` combination leads to constant result
--> $DIR/min_max.rs:58:5
|
LL | s.max("Zoo").min("Apple");
| ^^^^^^^^^^^^^^^^^^^^^^^^^

error: this `min`/`max` combination leads to constant result
--> $DIR/min_max.rs:59:5
|
LL | s.min("Apple").max("Zoo");
| ^^^^^^^^^^^^^^^^^^^^^^^^^

error: aborting due to 13 previous errors

0 comments on commit ee8db50

Please sign in to comment.