Skip to content

Commit e878ab4

Browse files
llogiqllogiq
committed
Merge pull request #790 from oli-obk/bool_opt
optimize booleans
2 parents cf95374 + 2917484 commit e878ab4

File tree

9 files changed

+500
-5
lines changed

9 files changed

+500
-5
lines changed

‎Cargo.toml

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@ regex_macros = { version = "0.1.33", optional = true }
2323
semver = "0.2.1"
2424
toml = "0.1"
2525
unicode-normalization = "0.1"
26+
quine-mc_cluskey = "0.2.2"
2627

2728
[dev-dependencies]
2829
compiletest_rs = "0.1.0"

‎README.md

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@ Table of contents:
1414
* [License](#license)
1515

1616
##Lints
17-
There are 137 lints included in this crate:
17+
There are 139 lints included in this crate:
1818

1919
name | default | meaning
2020
---------------------------------------------------------------------------------------------------------------------|---------|------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
@@ -75,6 +75,7 @@ name
7575
[let_and_return](https://github.com/Manishearth/rust-clippy/wiki#let_and_return) | warn | creating a let-binding and then immediately returning it like `let x = expr; x` at the end of a block
7676
[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
7777
[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 VecDeque
78+
[logic_bug](https://github.com/Manishearth/rust-clippy/wiki#logic_bug) | warn | checks for boolean expressions that contain terminals which can be eliminated
7879
[manual_swap](https://github.com/Manishearth/rust-clippy/wiki#manual_swap) | warn | manual swap
7980
[many_single_char_names](https://github.com/Manishearth/rust-clippy/wiki#many_single_char_names) | warn | too many single character bindings
8081
[map_clone](https://github.com/Manishearth/rust-clippy/wiki#map_clone) | warn | using `.map(|x| x.clone())` to clone an iterator or option's contents (recommends `.cloned()` instead)
@@ -97,6 +98,7 @@ name
9798
[new_without_default](https://github.com/Manishearth/rust-clippy/wiki#new_without_default) | warn | `fn new() -> Self` method without `Default` implementation
9899
[no_effect](https://github.com/Manishearth/rust-clippy/wiki#no_effect) | warn | statements with no effect
99100
[non_ascii_literal](https://github.com/Manishearth/rust-clippy/wiki#non_ascii_literal) | allow | using any literal non-ASCII chars in a string literal; suggests using the \\u escape instead
101+
[nonminimal_bool](https://github.com/Manishearth/rust-clippy/wiki#nonminimal_bool) | allow | checks for boolean expressions that can be written more concisely
100102
[nonsensical_open_options](https://github.com/Manishearth/rust-clippy/wiki#nonsensical_open_options) | warn | nonsensical combination of options for opening a file
101103
[ok_expect](https://github.com/Manishearth/rust-clippy/wiki#ok_expect) | warn | using `ok().expect()`, which gives worse error messages than calling `expect` directly on the Result
102104
[option_map_unwrap_or](https://github.com/Manishearth/rust-clippy/wiki#option_map_unwrap_or) | warn | using `Option.map(f).unwrap_or(a)`, which is more succinctly expressed as `map_or(a, f)`

‎src/booleans.rs

Lines changed: 362 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,362 @@
1+
use rustc::lint::{LintArray, LateLintPass, LateContext, LintPass};
2+
use rustc_front::hir::*;
3+
use rustc_front::intravisit::*;
4+
use syntax::ast::{LitKind, DUMMY_NODE_ID};
5+
use syntax::codemap::{DUMMY_SP, dummy_spanned};
6+
use utils::{span_lint_and_then, in_macro, snippet_opt, SpanlessEq};
7+
8+
/// **What it does:** This lint checks for boolean expressions that can be written more concisely
9+
///
10+
/// **Why is this bad?** Readability of boolean expressions suffers from unnecesessary duplication
11+
///
12+
/// **Known problems:** Ignores short circuting behavior of `||` and `&&`. Ignores `|`, `&` and `^`.
13+
///
14+
/// **Example:** `if a && true` should be `if a` and `!(a == b)` should be `a != b`
15+
declare_lint! {
16+
pub NONMINIMAL_BOOL, Allow,
17+
"checks for boolean expressions that can be written more concisely"
18+
}
19+
20+
/// **What it does:** This lint checks for boolean expressions that contain terminals that can be eliminated
21+
///
22+
/// **Why is this bad?** This is most likely a logic bug
23+
///
24+
/// **Known problems:** Ignores short circuiting behavior
25+
///
26+
/// **Example:** The `b` in `if a && b || a` is unnecessary because the expression is equivalent to `if a`
27+
declare_lint! {
28+
pub LOGIC_BUG, Warn,
29+
"checks for boolean expressions that contain terminals which can be eliminated"
30+
}
31+
32+
#[derive(Copy,Clone)]
33+
pub struct NonminimalBool;
34+
35+
impl LintPass for NonminimalBool {
36+
fn get_lints(&self) -> LintArray {
37+
lint_array!(NONMINIMAL_BOOL, LOGIC_BUG)
38+
}
39+
}
40+
41+
impl LateLintPass for NonminimalBool {
42+
fn check_item(&mut self, cx: &LateContext, item: &Item) {
43+
NonminimalBoolVisitor(cx).visit_item(item)
44+
}
45+
}
46+
47+
struct NonminimalBoolVisitor<'a, 'tcx: 'a>(&'a LateContext<'a, 'tcx>);
48+
49+
use quine_mc_cluskey::Bool;
50+
struct Hir2Qmm<'a, 'tcx: 'a, 'v> {
51+
terminals: Vec<&'v Expr>,
52+
cx: &'a LateContext<'a, 'tcx>
53+
}
54+
55+
impl<'a, 'tcx, 'v> Hir2Qmm<'a, 'tcx, 'v> {
56+
fn extract(&mut self, op: BinOp_, a: &[&'v Expr], mut v: Vec<Bool>) -> Result<Vec<Bool>, String> {
57+
for a in a {
58+
if let ExprBinary(binop, ref lhs, ref rhs) = a.node {
59+
if binop.node == op {
60+
v = self.extract(op, &[lhs, rhs], v)?;
61+
continue;
62+
}
63+
}
64+
v.push(self.run(a)?);
65+
}
66+
Ok(v)
67+
}
68+
69+
fn run(&mut self, e: &'v Expr) -> Result<Bool, String> {
70+
// prevent folding of `cfg!` macros and the like
71+
if !in_macro(self.cx, e.span) {
72+
match e.node {
73+
ExprUnary(UnNot, ref inner) => return Ok(Bool::Not(box self.run(inner)?)),
74+
ExprBinary(binop, ref lhs, ref rhs) => {
75+
match binop.node {
76+
BiOr => return Ok(Bool::Or(self.extract(BiOr, &[lhs, rhs], Vec::new())?)),
77+
BiAnd => return Ok(Bool::And(self.extract(BiAnd, &[lhs, rhs], Vec::new())?)),
78+
_ => {},
79+
}
80+
},
81+
ExprLit(ref lit) => {
82+
match lit.node {
83+
LitKind::Bool(true) => return Ok(Bool::True),
84+
LitKind::Bool(false) => return Ok(Bool::False),
85+
_ => {},
86+
}
87+
},
88+
_ => {},
89+
}
90+
}
91+
for (n, expr) in self.terminals.iter().enumerate() {
92+
if SpanlessEq::new(self.cx).ignore_fn().eq_expr(e, expr) {
93+
#[allow(cast_possible_truncation)]
94+
return Ok(Bool::Term(n as u8));
95+
}
96+
let negated = match e.node {
97+
ExprBinary(binop, ref lhs, ref rhs) => {
98+
let mk_expr = |op| Expr {
99+
id: DUMMY_NODE_ID,
100+
span: DUMMY_SP,
101+
attrs: None,
102+
node: ExprBinary(dummy_spanned(op), lhs.clone(), rhs.clone()),
103+
};
104+
match binop.node {
105+
BiEq => mk_expr(BiNe),
106+
BiNe => mk_expr(BiEq),
107+
BiGt => mk_expr(BiLe),
108+
BiGe => mk_expr(BiLt),
109+
BiLt => mk_expr(BiGe),
110+
BiLe => mk_expr(BiGt),
111+
_ => continue,
112+
}
113+
},
114+
_ => continue,
115+
};
116+
if SpanlessEq::new(self.cx).ignore_fn().eq_expr(&negated, expr) {
117+
#[allow(cast_possible_truncation)]
118+
return Ok(Bool::Not(Box::new(Bool::Term(n as u8))));
119+
}
120+
}
121+
let n = self.terminals.len();
122+
self.terminals.push(e);
123+
if n < 32 {
124+
#[allow(cast_possible_truncation)]
125+
Ok(Bool::Term(n as u8))
126+
} else {
127+
Err("too many literals".to_owned())
128+
}
129+
}
130+
}
131+
132+
fn suggest(cx: &LateContext, suggestion: &Bool, terminals: &[&Expr]) -> String {
133+
fn recurse(brackets: bool, cx: &LateContext, suggestion: &Bool, terminals: &[&Expr], mut s: String) -> String {
134+
use quine_mc_cluskey::Bool::*;
135+
let snip = |e: &Expr| snippet_opt(cx, e.span).expect("don't try to improve booleans created by macros");
136+
match *suggestion {
137+
True => {
138+
s.push_str("true");
139+
s
140+
},
141+
False => {
142+
s.push_str("false");
143+
s
144+
},
145+
Not(ref inner) => {
146+
match **inner {
147+
And(_) | Or(_) => {
148+
s.push('!');
149+
recurse(true, cx, inner, terminals, s)
150+
},
151+
Term(n) => {
152+
if let ExprBinary(binop, ref lhs, ref rhs) = terminals[n as usize].node {
153+
let op = match binop.node {
154+
BiEq => " != ",
155+
BiNe => " == ",
156+
BiLt => " >= ",
157+
BiGt => " <= ",
158+
BiLe => " > ",
159+
BiGe => " < ",
160+
_ => {
161+
s.push('!');
162+
return recurse(true, cx, inner, terminals, s)
163+
},
164+
};
165+
s.push_str(&snip(lhs));
166+
s.push_str(op);
167+
s.push_str(&snip(rhs));
168+
s
169+
} else {
170+
s.push('!');
171+
recurse(false, cx, inner, terminals, s)
172+
}
173+
},
174+
_ => {
175+
s.push('!');
176+
recurse(false, cx, inner, terminals, s)
177+
},
178+
}
179+
},
180+
And(ref v) => {
181+
if brackets {
182+
s.push('(');
183+
}
184+
if let Or(_) = v[0] {
185+
s = recurse(true, cx, &v[0], terminals, s);
186+
} else {
187+
s = recurse(false, cx, &v[0], terminals, s);
188+
}
189+
for inner in &v[1..] {
190+
s.push_str(" && ");
191+
if let Or(_) = *inner {
192+
s = recurse(true, cx, inner, terminals, s);
193+
} else {
194+
s = recurse(false, cx, inner, terminals, s);
195+
}
196+
}
197+
if brackets {
198+
s.push(')');
199+
}
200+
s
201+
},
202+
Or(ref v) => {
203+
if brackets {
204+
s.push('(');
205+
}
206+
s = recurse(false, cx, &v[0], terminals, s);
207+
for inner in &v[1..] {
208+
s.push_str(" || ");
209+
s = recurse(false, cx, inner, terminals, s);
210+
}
211+
if brackets {
212+
s.push(')');
213+
}
214+
s
215+
},
216+
Term(n) => {
217+
if brackets {
218+
if let ExprBinary(..) = terminals[n as usize].node {
219+
s.push('(');
220+
}
221+
}
222+
s.push_str(&snip(&terminals[n as usize]));
223+
if brackets {
224+
if let ExprBinary(..) = terminals[n as usize].node {
225+
s.push(')');
226+
}
227+
}
228+
s
229+
}
230+
}
231+
}
232+
recurse(false, cx, suggestion, terminals, String::new())
233+
}
234+
235+
fn simple_negate(b: Bool) -> Bool {
236+
use quine_mc_cluskey::Bool::*;
237+
match b {
238+
True => False,
239+
False => True,
240+
t @ Term(_) => Not(Box::new(t)),
241+
And(mut v) => {
242+
for el in &mut v {
243+
*el = simple_negate(::std::mem::replace(el, True));
244+
}
245+
Or(v)
246+
},
247+
Or(mut v) => {
248+
for el in &mut v {
249+
*el = simple_negate(::std::mem::replace(el, True));
250+
}
251+
And(v)
252+
},
253+
Not(inner) => *inner,
254+
}
255+
}
256+
257+
#[derive(Default)]
258+
struct Stats {
259+
terminals: [usize; 32],
260+
negations: usize,
261+
ops: usize,
262+
}
263+
264+
fn terminal_stats(b: &Bool) -> Stats {
265+
fn recurse(b: &Bool, stats: &mut Stats) {
266+
match *b {
267+
True | False => stats.ops += 1,
268+
Not(ref inner) => {
269+
match **inner {
270+
And(_) | Or(_) => stats.ops += 1, // brackets are also operations
271+
_ => stats.negations += 1,
272+
}
273+
recurse(inner, stats);
274+
},
275+
And(ref v) | Or(ref v) => {
276+
stats.ops += v.len() - 1;
277+
for inner in v {
278+
recurse(inner, stats);
279+
}
280+
},
281+
Term(n) => stats.terminals[n as usize] += 1,
282+
}
283+
}
284+
use quine_mc_cluskey::Bool::*;
285+
let mut stats = Stats::default();
286+
recurse(b, &mut stats);
287+
stats
288+
}
289+
290+
impl<'a, 'tcx> NonminimalBoolVisitor<'a, 'tcx> {
291+
fn bool_expr(&self, e: &Expr) {
292+
let mut h2q = Hir2Qmm {
293+
terminals: Vec::new(),
294+
cx: self.0,
295+
};
296+
if let Ok(expr) = h2q.run(e) {
297+
let stats = terminal_stats(&expr);
298+
let mut simplified = expr.simplify();
299+
for simple in Bool::Not(Box::new(expr.clone())).simplify() {
300+
match simple {
301+
Bool::Not(_) | Bool::True | Bool::False => {},
302+
_ => simplified.push(Bool::Not(Box::new(simple.clone()))),
303+
}
304+
let simple_negated = simple_negate(simple);
305+
if simplified.iter().any(|s| *s == simple_negated) {
306+
continue;
307+
}
308+
simplified.push(simple_negated);
309+
}
310+
let mut improvements = Vec::new();
311+
'simplified: for suggestion in &simplified {
312+
let simplified_stats = terminal_stats(&suggestion);
313+
let mut improvement = false;
314+
for i in 0..32 {
315+
// ignore any "simplifications" that end up requiring a terminal more often than in the original expression
316+
if stats.terminals[i] < simplified_stats.terminals[i] {
317+
continue 'simplified;
318+
}
319+
if stats.terminals[i] != 0 && simplified_stats.terminals[i] == 0 {
320+
span_lint_and_then(self.0, LOGIC_BUG, e.span, "this boolean expression contains a logic bug", |db| {
321+
db.span_help(h2q.terminals[i].span, "this expression can be optimized out by applying boolean operations to the outer expression");
322+
db.span_suggestion(e.span, "it would look like the following", suggest(self.0, suggestion, &h2q.terminals));
323+
});
324+
// don't also lint `NONMINIMAL_BOOL`
325+
return;
326+
}
327+
// if the number of occurrences of a terminal decreases or any of the stats decreases while none increases
328+
improvement |= (stats.terminals[i] > simplified_stats.terminals[i]) ||
329+
(stats.negations > simplified_stats.negations && stats.ops == simplified_stats.ops) ||
330+
(stats.ops > simplified_stats.ops && stats.negations == simplified_stats.negations);
331+
}
332+
if improvement {
333+
improvements.push(suggestion);
334+
}
335+
}
336+
if !improvements.is_empty() {
337+
span_lint_and_then(self.0, NONMINIMAL_BOOL, e.span, "this boolean expression can be simplified", |db| {
338+
for suggestion in &improvements {
339+
db.span_suggestion(e.span, "try", suggest(self.0, suggestion, &h2q.terminals));
340+
}
341+
});
342+
}
343+
}
344+
}
345+
}
346+
347+
impl<'a, 'v, 'tcx> Visitor<'v> for NonminimalBoolVisitor<'a, 'tcx> {
348+
fn visit_expr(&mut self, e: &'v Expr) {
349+
if in_macro(self.0, e.span) { return }
350+
match e.node {
351+
ExprBinary(binop, _, _) if binop.node == BiOr || binop.node == BiAnd => self.bool_expr(e),
352+
ExprUnary(UnNot, ref inner) => {
353+
if self.0.tcx.node_types()[&inner.id].is_bool() {
354+
self.bool_expr(e);
355+
} else {
356+
walk_expr(self, e);
357+
}
358+
},
359+
_ => walk_expr(self, e),
360+
}
361+
}
362+
}

0 commit comments

Comments
 (0)