diff --git a/clippy_lints/src/booleans.rs b/clippy_lints/src/booleans.rs index ca3fb4017df..9310cca4aee 100644 --- a/clippy_lints/src/booleans.rs +++ b/clippy_lints/src/booleans.rs @@ -44,6 +44,12 @@ declare_lint! { "boolean expressions that contain terminals which can be eliminated" } +// For each pairs, both orders are considered. +const METHODS_WITH_NEGATION: [(&str, &str); 2] = [ + ("is_some", "is_none"), + ("is_err", "is_ok"), +]; + #[derive(Copy, Clone)] pub struct NonminimalBool; @@ -153,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 { @@ -169,32 +183,53 @@ 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) => 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, simplified); + }, + }; + *simplified = true; + 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 { + *simplified = true; + 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, simplified) + } + }, + _ => { + s.push('!'); + 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) => { @@ -202,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 { @@ -223,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(')'); @@ -249,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 { @@ -359,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, ); }, ); @@ -376,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.rs b/tests/ui/booleans.rs index 0434285a523..52ce90dd63d 100644 --- a/tests/ui/booleans.rs +++ b/tests/ui/booleans.rs @@ -38,3 +38,19 @@ 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(); + let c = false; + let _ = !(a.is_some() && !c); +} diff --git a/tests/ui/booleans.stderr b/tests/ui/booleans.stderr index 0311e95a4f1..05696ba0f59 100644 --- a/tests/ui/booleans.stderr +++ b/tests/ui/booleans.stderr @@ -130,3 +130,33 @@ 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 + | +55 | let _ = !(a.is_some() && !c); + | ^^^^^^^^^^^^^^^^^^^^ help: try: `c || a.is_none()` +