From cd91110ec0ca4f823145a13033cfd45f74ce72c6 Mon Sep 17 00:00:00 2001 From: llogiq Date: Sat, 5 Sep 2015 12:46:34 +0200 Subject: [PATCH 1/5] new lint: min_max --- README.md | 3 +- src/consts.rs | 8 ++-- src/lib.rs | 3 ++ src/minmax.rs | 90 +++++++++++++++++++++++++++++++++++ tests/compile-fail/min_max.rs | 25 ++++++++++ 5 files changed, 124 insertions(+), 5 deletions(-) create mode 100644 src/minmax.rs create mode 100644 tests/compile-fail/min_max.rs diff --git a/README.md b/README.md index 0046d8911290..590ba60d552c 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 a8a446a703fa..5e7cd200d0fb 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 a4aee0c27fd6..7665d06b193a 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 000000000000..2e4c92566572 --- /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 000000000000..18f415ddc0bd --- /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 +} From b90e4c7bd51e3193504d7acf8cfc3220933cd5ee Mon Sep 17 00:00:00 2001 From: llogiq Date: Sat, 5 Sep 2015 13:15:18 +0200 Subject: [PATCH 2/5] hir naming, removed lookup, match full path --- src/consts.rs | 2 +- src/minmax.rs | 51 +++++++++++++++++---------------------------------- 2 files changed, 18 insertions(+), 35 deletions(-) diff --git a/src/consts.rs b/src/consts.rs index 5e7cd200d0fb..0c32dc5efadf 100644 --- a/src/consts.rs +++ b/src/consts.rs @@ -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()), diff --git a/src/minmax.rs b/src/minmax.rs index 2e4c92566572..d7a74aa8c8b6 100644 --- a/src/minmax.rs +++ b/src/minmax.rs @@ -1,11 +1,10 @@ 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 consts::{Constant, constant_simple}; use utils::{match_path, span_lint}; use self::MinMax::{Min, Max}; @@ -22,8 +21,8 @@ impl LintPass for MinMaxPass { } 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 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)) => (), @@ -43,47 +42,31 @@ enum MinMax { 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) +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 name == "max" { - fetch_const(cx, args, Max) + if match_path(path, &["std", "cmp", "max"]) { + fetch_const(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, - } + } else { None } + } else { None } } -fn fetch_const<'e>(cx: &Context, args: &'e Vec>, m: MinMax) -> - Option<(MinMax, Constant, &'e Expr)> { +fn fetch_const(args: &[P], m: MinMax) -> + Option<(MinMax, Constant, &Expr)> { if args.len() != 2 { return None } - if let Some((c, _)) = constant(cx, &args[0]) { - if let None = constant(cx, &args[1]) { // otherwise ignore + 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(cx, &args[1]) { + if let Some(c) = constant_simple(&args[1]) { Some((m, c, &args[0])) } else { None } } From 3848756be09744d0947f7472a340764e7ecd2249 Mon Sep 17 00:00:00 2001 From: llogiq Date: Sat, 5 Sep 2015 14:20:35 +0200 Subject: [PATCH 3/5] Made min_max `Warn` by default --- src/minmax.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/minmax.rs b/src/minmax.rs index d7a74aa8c8b6..72190d70e2e5 100644 --- a/src/minmax.rs +++ b/src/minmax.rs @@ -8,7 +8,7 @@ use consts::{Constant, constant_simple}; use utils::{match_path, span_lint}; use self::MinMax::{Min, Max}; -declare_lint!(pub MIN_MAX, Deny, +declare_lint!(pub MIN_MAX, Warn, "`min(_, max(_, _))` (or vice versa) with bounds clamping the result \ to a constant"); From b66bccc45a750169dadf9e93396791e32126e7c8 Mon Sep 17 00:00:00 2001 From: llogiq Date: Sat, 5 Sep 2015 14:22:33 +0200 Subject: [PATCH 4/5] update_lints --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index 590ba60d552c..c7071688e10b 100644 --- a/README.md +++ b/README.md @@ -31,7 +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 +[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 }` From 79bf820170faf257e68540d43cdf40112822a87d Mon Sep 17 00:00:00 2001 From: llogiq Date: Sat, 5 Sep 2015 16:24:41 +0200 Subject: [PATCH 5/5] added test against const lookup --- tests/compile-fail/min_max.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/tests/compile-fail/min_max.rs b/tests/compile-fail/min_max.rs index 18f415ddc0bd..5a5fae4930d3 100644 --- a/tests/compile-fail/min_max.rs +++ b/tests/compile-fail/min_max.rs @@ -5,6 +5,8 @@ use std::cmp::{min, max}; +const LARGE : usize = 3; + fn main() { let x; x = 2usize; @@ -15,6 +17,8 @@ fn main() { 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";