From 03833f666fccad492746304d818a4afbe679ccf3 Mon Sep 17 00:00:00 2001 From: Oliver Schneider Date: Thu, 24 Mar 2016 10:45:24 +0100 Subject: [PATCH] differentiate between logic bugs and optimizable expressions --- src/booleans.rs | 71 +++++++++++++++++++++++++++++++--- tests/compile-fail/booleans.rs | 10 +++-- 2 files changed, 72 insertions(+), 9 deletions(-) diff --git a/src/booleans.rs b/src/booleans.rs index 1b4d661da84..49371de5c9d 100644 --- a/src/booleans.rs +++ b/src/booleans.rs @@ -10,18 +10,30 @@ use utils::{span_lint_and_then, in_macro, snippet_opt, SpanlessEq}; /// /// **Known problems:** Ignores short circuting behavior, bitwise and/or and xor. Ends up suggesting things like !(a == b) /// -/// **Example:** `if a && b || a` should be `if a` +/// **Example:** `if a && true` should be `if a` declare_lint! { pub NONMINIMAL_BOOL, Allow, "checks for boolean expressions that can be written more concisely" } +/// **What it does:** This lint checks for boolean expressions that contain terminals that can be eliminated +/// +/// **Why is this bad?** This is most likely a logic bug +/// +/// **Known problems:** Ignores short circuiting behavior +/// +/// **Example:** The `b` in `if a && b || a` is unnecessary because the expression is equivalent to `if a` +declare_lint! { + pub LOGIC_BUG, Warn, + "checks for boolean expressions that contain terminals which can be eliminated" +} + #[derive(Copy,Clone)] pub struct NonminimalBool; impl LintPass for NonminimalBool { fn get_lints(&self) -> LintArray { - lint_array!(NONMINIMAL_BOOL) + lint_array!(NONMINIMAL_BOOL, LOGIC_BUG) } } @@ -168,6 +180,23 @@ fn simple_negate(b: Bool) -> Bool { } } +fn terminal_stats(b: &Bool) -> [usize; 32] { + fn recurse(b: &Bool, stats: &mut [usize; 32]) { + match *b { + True | False => {}, + Not(ref inner) => recurse(inner, stats), + And(ref v) | Or(ref v) => for inner in v { + recurse(inner, stats) + }, + Term(n) => stats[n as usize] += 1, + } + } + use quine_mc_cluskey::Bool::*; + let mut stats = [0; 32]; + recurse(b, &mut stats); + stats +} + impl<'a, 'tcx> NonminimalBoolVisitor<'a, 'tcx> { fn bool_expr(&self, e: &Expr) { let mut h2q = Hir2Qmm { @@ -175,6 +204,7 @@ impl<'a, 'tcx> NonminimalBoolVisitor<'a, 'tcx> { cx: self.0, }; if let Ok(expr) = h2q.run(e) { + let stats = terminal_stats(&expr); let mut simplified = expr.simplify(); for simple in Bool::Not(Box::new(expr.clone())).simplify() { let simple_negated = simple_negate(simple); @@ -184,11 +214,40 @@ impl<'a, 'tcx> NonminimalBoolVisitor<'a, 'tcx> { simplified.push(simple_negated); } if !simplified.iter().any(|s| *s == expr) { - span_lint_and_then(self.0, NONMINIMAL_BOOL, e.span, "this boolean expression can be simplified", |db| { - for suggestion in &simplified { - db.span_suggestion(e.span, "try", suggest(self.0, suggestion, &h2q.terminals)); + let mut improvements = Vec::new(); + 'simplified: for suggestion in &simplified { + let simplified_stats = terminal_stats(&suggestion); + let mut improvement = false; + for i in 0..32 { + // ignore any "simplifications" that end up requiring a terminal more often than in the original expression + if stats[i] < simplified_stats[i] { + continue 'simplified; + } + // if the number of occurrences of a terminal decreases, this expression is a candidate for improvement + if stats[i] >= simplified_stats[i] { + improvement = true; + } + if stats[i] != 0 && simplified_stats[i] == 0 { + span_lint_and_then(self.0, LOGIC_BUG, e.span, "this boolean expression contains a logic bug", |db| { + db.span_help(h2q.terminals[i].span, "this expression can be optimized out by applying boolean operations to the outer expression"); + db.span_suggestion(e.span, "it would look like the following", suggest(self.0, suggestion, &h2q.terminals)); + }); + // don't also lint `NONMINIMAL_BOOL` + improvements.clear(); + break 'simplified; + } } - }); + if improvement { + improvements.push(suggestion); + } + } + if !improvements.is_empty() { + span_lint_and_then(self.0, NONMINIMAL_BOOL, e.span, "this boolean expression can be simplified", |db| { + for suggestion in &improvements { + db.span_suggestion(e.span, "try", suggest(self.0, suggestion, &h2q.terminals)); + } + }); + } } } } diff --git a/tests/compile-fail/booleans.rs b/tests/compile-fail/booleans.rs index f9bf1a15f18..31c160980fc 100644 --- a/tests/compile-fail/booleans.rs +++ b/tests/compile-fail/booleans.rs @@ -1,13 +1,15 @@ #![feature(plugin)] #![plugin(clippy)] -#![deny(nonminimal_bool)] +#![deny(nonminimal_bool, logic_bug)] #[allow(unused)] fn main() { let a: bool = unimplemented!(); let b: bool = unimplemented!(); - let _ = a && b || a; //~ ERROR this boolean expression can be simplified + let _ = a && b || a; //~ ERROR this boolean expression contains a logic bug //|~ HELP for further information visit + //|~ HELP this expression can be optimized out + //|~ HELP it would look like the following //|~ SUGGESTION let _ = a; let _ = !(a && b); //~ ERROR this boolean expression can be simplified //|~ HELP for further information visit @@ -22,8 +24,10 @@ fn main() { //|~ HELP for further information visit //|~ SUGGESTION let _ = a; - let _ = false && a; //~ ERROR this boolean expression can be simplified + let _ = false && a; //~ ERROR this boolean expression contains a logic bug //|~ HELP for further information visit + //|~ HELP this expression can be optimized out + //|~ HELP it would look like the following //|~ SUGGESTION let _ = false; let _ = false || a; //~ ERROR this boolean expression can be simplified