-
Notifications
You must be signed in to change notification settings - Fork 1.7k
optimize booleans #790
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
optimize booleans #790
Changes from all commits
Commits
Show all changes
24 commits
Select commit
Hold shift + click to select a range
93d097e
better simplification
oli-obk 57faa5a
improve bracket display
oli-obk 1f1f09b
also compute minimal product of sum form
oli-obk 25ed62f
improve lint attribute detail
oli-obk 5911cca
merge multiple equal terminals into one
oli-obk 050d7fd
fallout and tests
oli-obk 0a78a79
bugfix in quine-mc_cluskey 0.2.1
oli-obk 288ea79
treat macros as terminals to prevent `cfg!` from giving platform spec…
oli-obk 03833f6
differentiate between logic bugs and optimizable expressions
oli-obk 37cee84
negations around expressions can make things simpler
oli-obk 76ab801
if a < b { ... } if a >= b { ... } what am I doing?
oli-obk e7013a3
update lints
oli-obk 0f92f84
String::extend -> String::push_str
oli-obk dd6bee3
collect stats on bool ops and negations in an expression
oli-obk 6904fd5
add tests showing the current level of minimization with ==
oli-obk 25bbde0
a small refactoring for readability
oli-obk 3a0791e
make sure `a < b` and `a >= b` are considered equal by SpanlessEq
oli-obk 96be287
detect negations of terminals like a != b vs a == b
oli-obk be72883
more tests
oli-obk 216edba
accidentally forgot about improvements if there were multiplie candid…
oli-obk b05dd13
added brackets and fixed compiler comments
oli-obk e9c87c7
`!(a == b)` --> `a != b`
oli-obk fa48ee6
dogfood
oli-obk 2917484
make `nonminimal_bool` allow-by-default
oli-obk File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,362 @@ | ||
| use rustc::lint::{LintArray, LateLintPass, LateContext, LintPass}; | ||
| use rustc_front::hir::*; | ||
| use rustc_front::intravisit::*; | ||
| use syntax::ast::{LitKind, DUMMY_NODE_ID}; | ||
| use syntax::codemap::{DUMMY_SP, dummy_spanned}; | ||
| use utils::{span_lint_and_then, in_macro, snippet_opt, SpanlessEq}; | ||
|
|
||
| /// **What it does:** This lint checks for boolean expressions that can be written more concisely | ||
| /// | ||
| /// **Why is this bad?** Readability of boolean expressions suffers from unnecesessary duplication | ||
| /// | ||
| /// **Known problems:** Ignores short circuting behavior of `||` and `&&`. Ignores `|`, `&` and `^`. | ||
| /// | ||
| /// **Example:** `if a && true` should be `if a` and `!(a == b)` should be `a != b` | ||
| 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, LOGIC_BUG) | ||
| } | ||
| } | ||
|
|
||
| impl LateLintPass for NonminimalBool { | ||
| fn check_item(&mut self, cx: &LateContext, item: &Item) { | ||
| NonminimalBoolVisitor(cx).visit_item(item) | ||
| } | ||
| } | ||
|
|
||
| struct NonminimalBoolVisitor<'a, 'tcx: 'a>(&'a LateContext<'a, 'tcx>); | ||
|
|
||
| use quine_mc_cluskey::Bool; | ||
| struct Hir2Qmm<'a, 'tcx: 'a, 'v> { | ||
| terminals: Vec<&'v Expr>, | ||
| cx: &'a LateContext<'a, 'tcx> | ||
| } | ||
|
|
||
| impl<'a, 'tcx, 'v> Hir2Qmm<'a, 'tcx, 'v> { | ||
| fn extract(&mut self, op: BinOp_, a: &[&'v Expr], mut v: Vec<Bool>) -> Result<Vec<Bool>, String> { | ||
| for a in a { | ||
| if let ExprBinary(binop, ref lhs, ref rhs) = a.node { | ||
| if binop.node == op { | ||
| v = self.extract(op, &[lhs, rhs], v)?; | ||
| continue; | ||
| } | ||
| } | ||
| v.push(self.run(a)?); | ||
| } | ||
| Ok(v) | ||
| } | ||
|
|
||
| fn run(&mut self, e: &'v Expr) -> Result<Bool, String> { | ||
| // prevent folding of `cfg!` macros and the like | ||
| if !in_macro(self.cx, e.span) { | ||
| match e.node { | ||
| ExprUnary(UnNot, ref inner) => return Ok(Bool::Not(box self.run(inner)?)), | ||
| ExprBinary(binop, ref lhs, ref rhs) => { | ||
| match binop.node { | ||
| BiOr => return Ok(Bool::Or(self.extract(BiOr, &[lhs, rhs], Vec::new())?)), | ||
| BiAnd => return Ok(Bool::And(self.extract(BiAnd, &[lhs, rhs], Vec::new())?)), | ||
| _ => {}, | ||
| } | ||
| }, | ||
| ExprLit(ref lit) => { | ||
| match lit.node { | ||
| LitKind::Bool(true) => return Ok(Bool::True), | ||
| LitKind::Bool(false) => return Ok(Bool::False), | ||
| _ => {}, | ||
| } | ||
| }, | ||
| _ => {}, | ||
| } | ||
| } | ||
| for (n, expr) in self.terminals.iter().enumerate() { | ||
| if SpanlessEq::new(self.cx).ignore_fn().eq_expr(e, expr) { | ||
| #[allow(cast_possible_truncation)] | ||
| return Ok(Bool::Term(n as u8)); | ||
| } | ||
| let negated = match e.node { | ||
| ExprBinary(binop, ref lhs, ref rhs) => { | ||
| let mk_expr = |op| Expr { | ||
| id: DUMMY_NODE_ID, | ||
| span: DUMMY_SP, | ||
| attrs: None, | ||
| node: ExprBinary(dummy_spanned(op), lhs.clone(), rhs.clone()), | ||
| }; | ||
| match binop.node { | ||
| BiEq => mk_expr(BiNe), | ||
| BiNe => mk_expr(BiEq), | ||
| BiGt => mk_expr(BiLe), | ||
| BiGe => mk_expr(BiLt), | ||
| BiLt => mk_expr(BiGe), | ||
| BiLe => mk_expr(BiGt), | ||
| _ => continue, | ||
| } | ||
| }, | ||
| _ => continue, | ||
| }; | ||
| if SpanlessEq::new(self.cx).ignore_fn().eq_expr(&negated, expr) { | ||
| #[allow(cast_possible_truncation)] | ||
| return Ok(Bool::Not(Box::new(Bool::Term(n as u8)))); | ||
| } | ||
| } | ||
| let n = self.terminals.len(); | ||
| self.terminals.push(e); | ||
| if n < 32 { | ||
| #[allow(cast_possible_truncation)] | ||
| Ok(Bool::Term(n as u8)) | ||
| } else { | ||
| Err("too many literals".to_owned()) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| fn suggest(cx: &LateContext, suggestion: &Bool, terminals: &[&Expr]) -> String { | ||
| fn recurse(brackets: bool, cx: &LateContext, suggestion: &Bool, terminals: &[&Expr], mut s: String) -> 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 { | ||
| True => { | ||
| s.push_str("true"); | ||
| s | ||
| }, | ||
| False => { | ||
| s.push_str("false"); | ||
| s | ||
| }, | ||
| Not(ref inner) => { | ||
| match **inner { | ||
| And(_) | Or(_) => { | ||
| 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 => " < ", | ||
| _ => { | ||
| 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) | ||
| } | ||
| }, | ||
| _ => { | ||
| s.push('!'); | ||
| recurse(false, cx, inner, terminals, s) | ||
| }, | ||
| } | ||
| }, | ||
| And(ref v) => { | ||
| if brackets { | ||
| s.push('('); | ||
| } | ||
| if let Or(_) = v[0] { | ||
| s = recurse(true, cx, &v[0], terminals, s); | ||
| } else { | ||
| s = recurse(false, cx, &v[0], terminals, s); | ||
| } | ||
| for inner in &v[1..] { | ||
| s.push_str(" && "); | ||
| if let Or(_) = *inner { | ||
| s = recurse(true, cx, inner, terminals, s); | ||
| } else { | ||
| s = recurse(false, cx, inner, terminals, s); | ||
| } | ||
| } | ||
| if brackets { | ||
| s.push(')'); | ||
| } | ||
| s | ||
| }, | ||
| Or(ref v) => { | ||
| if brackets { | ||
| s.push('('); | ||
| } | ||
| s = recurse(false, cx, &v[0], terminals, s); | ||
| for inner in &v[1..] { | ||
| s.push_str(" || "); | ||
| s = recurse(false, cx, inner, terminals, s); | ||
| } | ||
| if brackets { | ||
| s.push(')'); | ||
| } | ||
| s | ||
| }, | ||
| Term(n) => { | ||
| if brackets { | ||
| if let ExprBinary(..) = terminals[n as usize].node { | ||
| s.push('('); | ||
| } | ||
| } | ||
| s.push_str(&snip(&terminals[n as usize])); | ||
| if brackets { | ||
| if let ExprBinary(..) = terminals[n as usize].node { | ||
| s.push(')'); | ||
| } | ||
| } | ||
| s | ||
| } | ||
| } | ||
| } | ||
| recurse(false, cx, suggestion, terminals, String::new()) | ||
| } | ||
|
|
||
| fn simple_negate(b: Bool) -> Bool { | ||
| use quine_mc_cluskey::Bool::*; | ||
| match b { | ||
| True => False, | ||
| False => True, | ||
| t @ Term(_) => Not(Box::new(t)), | ||
| And(mut v) => { | ||
| for el in &mut v { | ||
| *el = simple_negate(::std::mem::replace(el, True)); | ||
| } | ||
| Or(v) | ||
| }, | ||
| Or(mut v) => { | ||
| for el in &mut v { | ||
| *el = simple_negate(::std::mem::replace(el, True)); | ||
| } | ||
| And(v) | ||
| }, | ||
| Not(inner) => *inner, | ||
| } | ||
| } | ||
|
|
||
| #[derive(Default)] | ||
| struct Stats { | ||
| terminals: [usize; 32], | ||
| negations: usize, | ||
| ops: usize, | ||
| } | ||
|
|
||
| fn terminal_stats(b: &Bool) -> Stats { | ||
| fn recurse(b: &Bool, stats: &mut Stats) { | ||
| match *b { | ||
| True | False => stats.ops += 1, | ||
| Not(ref inner) => { | ||
| match **inner { | ||
| And(_) | Or(_) => stats.ops += 1, // brackets are also operations | ||
| _ => stats.negations += 1, | ||
| } | ||
| recurse(inner, stats); | ||
| }, | ||
| And(ref v) | Or(ref v) => { | ||
| stats.ops += v.len() - 1; | ||
| for inner in v { | ||
| recurse(inner, stats); | ||
| } | ||
| }, | ||
| Term(n) => stats.terminals[n as usize] += 1, | ||
| } | ||
| } | ||
| use quine_mc_cluskey::Bool::*; | ||
| let mut stats = Stats::default(); | ||
| recurse(b, &mut stats); | ||
| stats | ||
| } | ||
|
|
||
| impl<'a, 'tcx> NonminimalBoolVisitor<'a, 'tcx> { | ||
| fn bool_expr(&self, e: &Expr) { | ||
| let mut h2q = Hir2Qmm { | ||
| terminals: Vec::new(), | ||
| 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() { | ||
| match simple { | ||
| Bool::Not(_) | Bool::True | Bool::False => {}, | ||
| _ => simplified.push(Bool::Not(Box::new(simple.clone()))), | ||
| } | ||
| let simple_negated = simple_negate(simple); | ||
| if simplified.iter().any(|s| *s == simple_negated) { | ||
| continue; | ||
| } | ||
| simplified.push(simple_negated); | ||
| } | ||
| 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.terminals[i] < simplified_stats.terminals[i] { | ||
| continue 'simplified; | ||
| } | ||
| if stats.terminals[i] != 0 && simplified_stats.terminals[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` | ||
| return; | ||
| } | ||
| // if the number of occurrences of a terminal decreases or any of the stats decreases while none increases | ||
| improvement |= (stats.terminals[i] > simplified_stats.terminals[i]) || | ||
| (stats.negations > simplified_stats.negations && stats.ops == simplified_stats.ops) || | ||
| (stats.ops > simplified_stats.ops && stats.negations == simplified_stats.negations); | ||
| } | ||
| 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)); | ||
| } | ||
| }); | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| impl<'a, 'v, 'tcx> Visitor<'v> for NonminimalBoolVisitor<'a, 'tcx> { | ||
| fn visit_expr(&mut self, e: &'v Expr) { | ||
| if in_macro(self.0, e.span) { return } | ||
| match e.node { | ||
| ExprBinary(binop, _, _) if binop.node == BiOr || binop.node == BiAnd => self.bool_expr(e), | ||
| ExprUnary(UnNot, ref inner) => { | ||
| if self.0.tcx.node_types()[&inner.id].is_bool() { | ||
| self.bool_expr(e); | ||
| } else { | ||
| walk_expr(self, e); | ||
| } | ||
| }, | ||
| _ => walk_expr(self, e), | ||
| } | ||
| } | ||
| } | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is that
usehere?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to annoy enough so someone writes a lint warning about
uses in odd places?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems legit 😄