From 82793768b79ef6415c79df0f0385ae5a5d3b00c5 Mon Sep 17 00:00:00 2001 From: laurent Date: Tue, 7 Nov 2017 21:43:24 +0000 Subject: [PATCH 1/5] Handle methods with an immediate negation in the non-minimal boolean lint, fixes #1930. --- clippy_lints/src/booleans.rs | 30 ++++++++++++++++++++++++++++++ tests/ui/booleans.rs | 14 ++++++++++++++ tests/ui/booleans.stderr | 24 ++++++++++++++++++++++++ 3 files changed, 68 insertions(+) diff --git a/clippy_lints/src/booleans.rs b/clippy_lints/src/booleans.rs index ca3fb4017df..86d1f2fb9ee 100644 --- a/clippy_lints/src/booleans.rs +++ b/clippy_lints/src/booleans.rs @@ -44,6 +44,13 @@ declare_lint! { "boolean expressions that contain terminals which can be eliminated" } +const METHODS_WITH_NEGATION: [(&str, &str); 4] = [ + ("is_some", "is_none"), + ("is_none", "is_some"), + ("is_err", "is_ok"), + ("is_ok", "is_err"), +]; + #[derive(Copy, Clone)] pub struct NonminimalBool; @@ -396,6 +403,28 @@ impl<'a, 'tcx> NonminimalBoolVisitor<'a, 'tcx> { } } } + + fn handle_method_call_in_not(&mut self, e: &'tcx Expr, inner: &'tcx Expr) { + if let ExprMethodCall(ref path, _, _) = inner.node { + METHODS_WITH_NEGATION.iter().for_each(|&(method, negation_method)| { + if method == path.name.as_str() { + span_lint_and_then( + self.cx, + NONMINIMAL_BOOL, + e.span, + "this boolean expression can be simplified", + |db| { + db.span_suggestion( + e.span, + "try", + negation_method.to_owned() + ); + } + ) + } + }) + } + } } impl<'a, 'tcx> Visitor<'tcx> for NonminimalBoolVisitor<'a, 'tcx> { @@ -406,6 +435,7 @@ impl<'a, 'tcx> Visitor<'tcx> for NonminimalBoolVisitor<'a, 'tcx> { match e.node { ExprBinary(binop, _, _) if binop.node == BiOr || binop.node == BiAnd => self.bool_expr(e), ExprUnary(UnNot, ref inner) => if self.cx.tables.node_types()[inner.hir_id].is_bool() { + self.handle_method_call_in_not(e, inner); self.bool_expr(e); } else { walk_expr(self, e); diff --git a/tests/ui/booleans.rs b/tests/ui/booleans.rs index 0434285a523..a3c37fecfdc 100644 --- a/tests/ui/booleans.rs +++ b/tests/ui/booleans.rs @@ -38,3 +38,17 @@ fn equality_stuff() { let _ = a > b && a == b; let _ = a != b || !(a != b || c == d); } + +#[allow(unused, many_single_char_names)] +fn methods_with_negation() { + let a: Option = unimplemented!(); + let b: Result = unimplemented!(); + let _ = a.is_some(); + let _ = !a.is_some(); + let _ = a.is_none(); + let _ = !a.is_none(); + let _ = b.is_err(); + let _ = !b.is_err(); + let _ = b.is_ok(); + let _ = !b.is_ok(); +} diff --git a/tests/ui/booleans.stderr b/tests/ui/booleans.stderr index 0311e95a4f1..b7256ee0f3a 100644 --- a/tests/ui/booleans.stderr +++ b/tests/ui/booleans.stderr @@ -130,3 +130,27 @@ help: try 39 | let _ = !(a == b && c == d); | ^^^^^^^^^^^^^^^^^^^ +error: this boolean expression can be simplified + --> $DIR/booleans.rs:47:13 + | +47 | let _ = !a.is_some(); + | ^^^^^^^^^^^^ help: try: `is_none` + +error: this boolean expression can be simplified + --> $DIR/booleans.rs:49:13 + | +49 | let _ = !a.is_none(); + | ^^^^^^^^^^^^ help: try: `is_some` + +error: this boolean expression can be simplified + --> $DIR/booleans.rs:51:13 + | +51 | let _ = !b.is_err(); + | ^^^^^^^^^^^ help: try: `is_ok` + +error: this boolean expression can be simplified + --> $DIR/booleans.rs:53:13 + | +53 | let _ = !b.is_ok(); + | ^^^^^^^^^^ help: try: `is_err` + From 67aeb2eaeb9910d2177a6fc0e675c6bc84261c3d Mon Sep 17 00:00:00 2001 From: laurent Date: Tue, 7 Nov 2017 21:49:30 +0000 Subject: [PATCH 2/5] Only apply when there is a single argument. --- clippy_lints/src/booleans.rs | 38 +++++++++++++++++++----------------- 1 file changed, 20 insertions(+), 18 deletions(-) diff --git a/clippy_lints/src/booleans.rs b/clippy_lints/src/booleans.rs index 86d1f2fb9ee..7fe3b878233 100644 --- a/clippy_lints/src/booleans.rs +++ b/clippy_lints/src/booleans.rs @@ -405,24 +405,26 @@ impl<'a, 'tcx> NonminimalBoolVisitor<'a, 'tcx> { } fn handle_method_call_in_not(&mut self, e: &'tcx Expr, inner: &'tcx Expr) { - if let ExprMethodCall(ref path, _, _) = inner.node { - METHODS_WITH_NEGATION.iter().for_each(|&(method, negation_method)| { - if method == path.name.as_str() { - span_lint_and_then( - self.cx, - NONMINIMAL_BOOL, - e.span, - "this boolean expression can be simplified", - |db| { - db.span_suggestion( - e.span, - "try", - negation_method.to_owned() - ); - } - ) - } - }) + if let ExprMethodCall(ref path, _, ref args) = inner.node { + if args.len() == 1 { + METHODS_WITH_NEGATION.iter().for_each(|&(method, negation_method)| { + if method == path.name.as_str() { + span_lint_and_then( + self.cx, + NONMINIMAL_BOOL, + e.span, + "this boolean expression can be simplified", + |db| { + db.span_suggestion( + e.span, + "try", + negation_method.to_owned() + ); + } + ) + } + }) + } } } } From 14d50133141d3c728c4497375dab7de14f19dbf5 Mon Sep 17 00:00:00 2001 From: laurent Date: Fri, 10 Nov 2017 19:55:15 +0000 Subject: [PATCH 3/5] Use both pair orders. --- clippy_lints/src/booleans.rs | 37 ++++++++++++++++++------------------ 1 file changed, 19 insertions(+), 18 deletions(-) diff --git a/clippy_lints/src/booleans.rs b/clippy_lints/src/booleans.rs index 7fe3b878233..e1e8bce1f7d 100644 --- a/clippy_lints/src/booleans.rs +++ b/clippy_lints/src/booleans.rs @@ -44,11 +44,10 @@ declare_lint! { "boolean expressions that contain terminals which can be eliminated" } -const METHODS_WITH_NEGATION: [(&str, &str); 4] = [ +// For each pairs, both orders are considered. +const METHODS_WITH_NEGATION: [(&str, &str); 2] = [ ("is_some", "is_none"), - ("is_none", "is_some"), ("is_err", "is_ok"), - ("is_ok", "is_err"), ]; #[derive(Copy, Clone)] @@ -407,21 +406,23 @@ impl<'a, 'tcx> NonminimalBoolVisitor<'a, 'tcx> { fn handle_method_call_in_not(&mut self, e: &'tcx Expr, inner: &'tcx Expr) { if let ExprMethodCall(ref path, _, ref args) = inner.node { if args.len() == 1 { - METHODS_WITH_NEGATION.iter().for_each(|&(method, negation_method)| { - if method == path.name.as_str() { - span_lint_and_then( - self.cx, - NONMINIMAL_BOOL, - e.span, - "this boolean expression can be simplified", - |db| { - db.span_suggestion( - e.span, - "try", - negation_method.to_owned() - ); - } - ) + METHODS_WITH_NEGATION.iter().for_each(|&(method1, method2)| { + for &(method, negation_method) in &[(method1, method2), (method2, method1)] { + if method == path.name.as_str() { + span_lint_and_then( + self.cx, + NONMINIMAL_BOOL, + e.span, + "this boolean expression can be simplified", + |db| { + db.span_suggestion( + e.span, + "try", + negation_method.to_owned() + ); + } + ) + } } }) } From bdf3887d22a7352c20d97cef9cf9f8f4ac41ccff Mon Sep 17 00:00:00 2001 From: Oliver Schneider Date: Tue, 14 Nov 2017 17:07:04 +0100 Subject: [PATCH 4/5] Move 'handle_method_call_in_not' code into 'suggest' --- clippy_lints/src/booleans.rs | 84 +++++++++++++---------------- tests/ui/booleans.rs | 2 + tests/ui/booleans.stderr | 24 ++------- tests/ui/format.stderr | 12 ----- tests/ui/needless_range_loop.stderr | 4 +- tests/ui/print_with_newline.stderr | 18 ------- 6 files changed, 45 insertions(+), 99 deletions(-) diff --git a/clippy_lints/src/booleans.rs b/clippy_lints/src/booleans.rs index e1e8bce1f7d..5de3246f1a9 100644 --- a/clippy_lints/src/booleans.rs +++ b/clippy_lints/src/booleans.rs @@ -177,26 +177,45 @@ fn suggest(cx: &LateContext, suggestion: &Bool, terminals: &[&Expr]) -> String { s.push('!'); recurse(true, cx, inner, terminals, s) }, - Term(n) => if let ExprBinary(binop, ref lhs, ref rhs) = terminals[n as usize].node { - let op = match binop.node { - BiEq => " != ", - BiNe => " == ", - BiLt => " >= ", - BiGt => " <= ", - BiLe => " > ", - BiGe => " < ", - _ => { + Term(n) => match terminals[n as usize].node { + ExprBinary(binop, ref lhs, ref rhs) => { + let op = match binop.node { + BiEq => " != ", + BiNe => " == ", + BiLt => " >= ", + BiGt => " <= ", + BiLe => " > ", + BiGe => " < ", + _ => { + s.push('!'); + return recurse(true, cx, inner, terminals, s); + }, + }; + s.push_str(&snip(lhs)); + s.push_str(op); + s.push_str(&snip(rhs)); + s + }, + ExprMethodCall(ref path, _, ref args) if args.len() == 1 => { + let negation = METHODS_WITH_NEGATION + .iter().cloned() + .flat_map(|(a, b)| vec![(a, b), (b, a)]) + .find(|&(a, _)| a == path.name.as_str()); + if let Some((_, negation_method)) = negation { + s.push_str(&snip(&args[0])); + s.push('.'); + s.push_str(negation_method); + s.push_str("()"); + s + } else { s.push('!'); - return recurse(true, cx, inner, terminals, s); - }, - }; - s.push_str(&snip(lhs)); - s.push_str(op); - s.push_str(&snip(rhs)); - s - } else { - s.push('!'); - recurse(false, cx, inner, terminals, s) + recurse(false, cx, inner, terminals, s) + } + }, + _ => { + s.push('!'); + recurse(false, cx, inner, terminals, s) + }, }, _ => { s.push('!'); @@ -402,32 +421,6 @@ impl<'a, 'tcx> NonminimalBoolVisitor<'a, 'tcx> { } } } - - fn handle_method_call_in_not(&mut self, e: &'tcx Expr, inner: &'tcx Expr) { - if let ExprMethodCall(ref path, _, ref args) = inner.node { - if args.len() == 1 { - METHODS_WITH_NEGATION.iter().for_each(|&(method1, method2)| { - for &(method, negation_method) in &[(method1, method2), (method2, method1)] { - if method == path.name.as_str() { - span_lint_and_then( - self.cx, - NONMINIMAL_BOOL, - e.span, - "this boolean expression can be simplified", - |db| { - db.span_suggestion( - e.span, - "try", - negation_method.to_owned() - ); - } - ) - } - } - }) - } - } - } } impl<'a, 'tcx> Visitor<'tcx> for NonminimalBoolVisitor<'a, 'tcx> { @@ -438,7 +431,6 @@ impl<'a, 'tcx> Visitor<'tcx> for NonminimalBoolVisitor<'a, 'tcx> { match e.node { ExprBinary(binop, _, _) if binop.node == BiOr || binop.node == BiAnd => self.bool_expr(e), ExprUnary(UnNot, ref inner) => if self.cx.tables.node_types()[inner.hir_id].is_bool() { - self.handle_method_call_in_not(e, inner); self.bool_expr(e); } else { walk_expr(self, e); diff --git a/tests/ui/booleans.rs b/tests/ui/booleans.rs index a3c37fecfdc..52ce90dd63d 100644 --- a/tests/ui/booleans.rs +++ b/tests/ui/booleans.rs @@ -51,4 +51,6 @@ fn methods_with_negation() { let _ = !b.is_err(); let _ = b.is_ok(); let _ = !b.is_ok(); + let c = false; + let _ = !(a.is_some() && !c); } diff --git a/tests/ui/booleans.stderr b/tests/ui/booleans.stderr index b7256ee0f3a..e50961323e5 100644 --- a/tests/ui/booleans.stderr +++ b/tests/ui/booleans.stderr @@ -131,26 +131,8 @@ help: try | ^^^^^^^^^^^^^^^^^^^ error: this boolean expression can be simplified - --> $DIR/booleans.rs:47:13 + --> $DIR/booleans.rs:55:13 | -47 | let _ = !a.is_some(); - | ^^^^^^^^^^^^ help: try: `is_none` - -error: this boolean expression can be simplified - --> $DIR/booleans.rs:49:13 - | -49 | let _ = !a.is_none(); - | ^^^^^^^^^^^^ help: try: `is_some` - -error: this boolean expression can be simplified - --> $DIR/booleans.rs:51:13 - | -51 | let _ = !b.is_err(); - | ^^^^^^^^^^^ help: try: `is_ok` - -error: this boolean expression can be simplified - --> $DIR/booleans.rs:53:13 - | -53 | let _ = !b.is_ok(); - | ^^^^^^^^^^ help: try: `is_err` +55 | let _ = !(a.is_some() && !c); + | ^^^^^^^^^^^^^^^^^^^^ help: try: `c || a.is_none()` diff --git a/tests/ui/format.stderr b/tests/ui/format.stderr index 67d97f295d8..558e9e83c33 100644 --- a/tests/ui/format.stderr +++ b/tests/ui/format.stderr @@ -6,15 +6,3 @@ error: useless use of `format!` | = note: `-D useless-format` implied by `-D warnings` -error: useless use of `format!` - --> $DIR/format.rs:8:5 - | -8 | format!("{}", "foo"); - | ^^^^^^^^^^^^^^^^^^^^^ - -error: useless use of `format!` - --> $DIR/format.rs:15:5 - | -15 | format!("{}", arg); - | ^^^^^^^^^^^^^^^^^^^ - diff --git a/tests/ui/needless_range_loop.stderr b/tests/ui/needless_range_loop.stderr index 97328f3d4d1..94ee5f613fc 100644 --- a/tests/ui/needless_range_loop.stderr +++ b/tests/ui/needless_range_loop.stderr @@ -23,7 +23,7 @@ error: the loop variable `i` is only used to index `ms`. help: consider using an iterator | 29 | for in &mut ms { - | ^^^^^^ + | error: the loop variable `i` is only used to index `ms`. --> $DIR/needless_range_loop.rs:35:5 @@ -37,5 +37,5 @@ error: the loop variable `i` is only used to index `ms`. help: consider using an iterator | 35 | for in &mut ms { - | ^^^^^^ + | diff --git a/tests/ui/print_with_newline.stderr b/tests/ui/print_with_newline.stderr index 2ade3ae4ef5..0148a470e0d 100644 --- a/tests/ui/print_with_newline.stderr +++ b/tests/ui/print_with_newline.stderr @@ -6,21 +6,3 @@ error: using `print!()` with a format string that ends in a newline, consider us | = note: `-D print-with-newline` implied by `-D warnings` -error: using `print!()` with a format string that ends in a newline, consider using `println!()` instead - --> $DIR/print_with_newline.rs:7:5 - | -7 | print!("Hello {}/n", "world"); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ - -error: using `print!()` with a format string that ends in a newline, consider using `println!()` instead - --> $DIR/print_with_newline.rs:8:5 - | -8 | print!("Hello {} {}/n/n", "world", "#2"); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ - -error: using `print!()` with a format string that ends in a newline, consider using `println!()` instead - --> $DIR/print_with_newline.rs:9:5 - | -9 | print!("{}/n", 1265); - | ^^^^^^^^^^^^^^^^^^^^^ - From 25783fa485933158870260b68bff94998d95ca48 Mon Sep 17 00:00:00 2001 From: laurent Date: Tue, 14 Nov 2017 21:14:08 +0000 Subject: [PATCH 5/5] Raise a lint when suggest has simplified the expression. --- clippy_lints/src/booleans.rs | 68 ++++++++++++++++++++++-------------- tests/ui/booleans.stderr | 24 +++++++++++++ 2 files changed, 66 insertions(+), 26 deletions(-) diff --git a/clippy_lints/src/booleans.rs b/clippy_lints/src/booleans.rs index 5de3246f1a9..9310cca4aee 100644 --- a/clippy_lints/src/booleans.rs +++ b/clippy_lints/src/booleans.rs @@ -159,8 +159,16 @@ impl<'a, 'tcx, 'v> Hir2Qmm<'a, 'tcx, 'v> { } } -fn suggest(cx: &LateContext, suggestion: &Bool, terminals: &[&Expr]) -> String { - fn recurse(brackets: bool, cx: &LateContext, suggestion: &Bool, terminals: &[&Expr], mut s: String) -> String { +// The boolean part of the return indicates whether some simplifications have been applied. +fn suggest(cx: &LateContext, suggestion: &Bool, terminals: &[&Expr]) -> (String, bool) { + fn recurse( + brackets: bool, + cx: &LateContext, + suggestion: &Bool, + terminals: &[&Expr], + mut s: String, + simplified: &mut bool, + ) -> String { use quine_mc_cluskey::Bool::*; let snip = |e: &Expr| snippet_opt(cx, e.span).expect("don't try to improve booleans created by macros"); match *suggestion { @@ -175,7 +183,7 @@ fn suggest(cx: &LateContext, suggestion: &Bool, terminals: &[&Expr]) -> String { Not(ref inner) => match **inner { And(_) | Or(_) => { s.push('!'); - recurse(true, cx, inner, terminals, s) + recurse(true, cx, inner, terminals, s, simplified) }, Term(n) => match terminals[n as usize].node { ExprBinary(binop, ref lhs, ref rhs) => { @@ -188,9 +196,10 @@ fn suggest(cx: &LateContext, suggestion: &Bool, terminals: &[&Expr]) -> String { BiGe => " < ", _ => { s.push('!'); - return recurse(true, cx, inner, terminals, s); + return recurse(true, cx, inner, terminals, s, simplified); }, }; + *simplified = true; s.push_str(&snip(lhs)); s.push_str(op); s.push_str(&snip(rhs)); @@ -202,6 +211,7 @@ fn suggest(cx: &LateContext, suggestion: &Bool, terminals: &[&Expr]) -> String { .flat_map(|(a, b)| vec![(a, b), (b, a)]) .find(|&(a, _)| a == path.name.as_str()); if let Some((_, negation_method)) = negation { + *simplified = true; s.push_str(&snip(&args[0])); s.push('.'); s.push_str(negation_method); @@ -209,17 +219,17 @@ fn suggest(cx: &LateContext, suggestion: &Bool, terminals: &[&Expr]) -> String { s } else { s.push('!'); - recurse(false, cx, inner, terminals, s) + recurse(false, cx, inner, terminals, s, simplified) } }, _ => { s.push('!'); - recurse(false, cx, inner, terminals, s) + recurse(false, cx, inner, terminals, s, simplified) }, }, _ => { s.push('!'); - recurse(false, cx, inner, terminals, s) + recurse(false, cx, inner, terminals, s, simplified) }, }, And(ref v) => { @@ -227,16 +237,16 @@ fn suggest(cx: &LateContext, suggestion: &Bool, terminals: &[&Expr]) -> String { s.push('('); } if let Or(_) = v[0] { - s = recurse(true, cx, &v[0], terminals, s); + s = recurse(true, cx, &v[0], terminals, s, simplified); } else { - s = recurse(false, cx, &v[0], terminals, s); + s = recurse(false, cx, &v[0], terminals, s, simplified); } for inner in &v[1..] { s.push_str(" && "); if let Or(_) = *inner { - s = recurse(true, cx, inner, terminals, s); + s = recurse(true, cx, inner, terminals, s, simplified); } else { - s = recurse(false, cx, inner, terminals, s); + s = recurse(false, cx, inner, terminals, s, simplified); } } if brackets { @@ -248,10 +258,10 @@ fn suggest(cx: &LateContext, suggestion: &Bool, terminals: &[&Expr]) -> String { if brackets { s.push('('); } - s = recurse(false, cx, &v[0], terminals, s); + s = recurse(false, cx, &v[0], terminals, s, simplified); for inner in &v[1..] { s.push_str(" || "); - s = recurse(false, cx, inner, terminals, s); + s = recurse(false, cx, inner, terminals, s, simplified); } if brackets { s.push(')'); @@ -274,7 +284,9 @@ fn suggest(cx: &LateContext, suggestion: &Bool, terminals: &[&Expr]) -> String { }, } } - recurse(false, cx, suggestion, terminals, String::new()) + let mut simplified = false; + let s = recurse(false, cx, suggestion, terminals, String::new(), &mut simplified); + (s, simplified) } fn simple_negate(b: Bool) -> Bool { @@ -384,7 +396,7 @@ impl<'a, 'tcx> NonminimalBoolVisitor<'a, 'tcx> { db.span_suggestion( e.span, "it would look like the following", - suggest(self.cx, suggestion, &h2q.terminals), + suggest(self.cx, suggestion, &h2q.terminals).0, ); }, ); @@ -401,22 +413,26 @@ impl<'a, 'tcx> NonminimalBoolVisitor<'a, 'tcx> { improvements.push(suggestion); } } - if !improvements.is_empty() { + let nonminimal_bool_lint = |suggestions| { span_lint_and_then( self.cx, NONMINIMAL_BOOL, e.span, "this boolean expression can be simplified", - |db| { - db.span_suggestions( - e.span, - "try", - improvements - .into_iter() - .map(|suggestion| suggest(self.cx, suggestion, &h2q.terminals)) - .collect(), - ); - }, + |db| { db.span_suggestions(e.span, "try", suggestions); }, + ); + }; + if improvements.is_empty() { + let suggest = suggest(self.cx, &expr, &h2q.terminals); + if suggest.1 { + nonminimal_bool_lint(vec![suggest.0]) + } + } else { + nonminimal_bool_lint( + improvements + .into_iter() + .map(|suggestion| suggest(self.cx, suggestion, &h2q.terminals).0) + .collect() ); } } diff --git a/tests/ui/booleans.stderr b/tests/ui/booleans.stderr index e50961323e5..05696ba0f59 100644 --- a/tests/ui/booleans.stderr +++ b/tests/ui/booleans.stderr @@ -130,6 +130,30 @@ help: try 39 | let _ = !(a == b && c == d); | ^^^^^^^^^^^^^^^^^^^ +error: this boolean expression can be simplified + --> $DIR/booleans.rs:47:13 + | +47 | let _ = !a.is_some(); + | ^^^^^^^^^^^^ help: try: `a.is_none()` + +error: this boolean expression can be simplified + --> $DIR/booleans.rs:49:13 + | +49 | let _ = !a.is_none(); + | ^^^^^^^^^^^^ help: try: `a.is_some()` + +error: this boolean expression can be simplified + --> $DIR/booleans.rs:51:13 + | +51 | let _ = !b.is_err(); + | ^^^^^^^^^^^ help: try: `b.is_ok()` + +error: this boolean expression can be simplified + --> $DIR/booleans.rs:53:13 + | +53 | let _ = !b.is_ok(); + | ^^^^^^^^^^ help: try: `b.is_err()` + error: this boolean expression can be simplified --> $DIR/booleans.rs:55:13 |