Merge pull request #790 from oli-obk/bool_opt

optimize booleans
This commit is contained in:
llogiq 2016-03-30 15:09:49 +02:00
commit e878ab40aa
9 changed files with 500 additions and 5 deletions

View File

@ -23,6 +23,7 @@ regex_macros = { version = "0.1.33", optional = true }
semver = "0.2.1"
toml = "0.1"
unicode-normalization = "0.1"
quine-mc_cluskey = "0.2.2"
[dev-dependencies]
compiletest_rs = "0.1.0"

View File

@ -14,7 +14,7 @@ Table of contents:
* [License](#license)
##Lints
There are 137 lints included in this crate:
There are 139 lints included in this crate:
name | default | meaning
---------------------------------------------------------------------------------------------------------------------|---------|------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
@ -75,6 +75,7 @@ name
[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
[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
[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
[logic_bug](https://github.com/Manishearth/rust-clippy/wiki#logic_bug) | warn | checks for boolean expressions that contain terminals which can be eliminated
[manual_swap](https://github.com/Manishearth/rust-clippy/wiki#manual_swap) | warn | manual swap
[many_single_char_names](https://github.com/Manishearth/rust-clippy/wiki#many_single_char_names) | warn | too many single character bindings
[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
[new_without_default](https://github.com/Manishearth/rust-clippy/wiki#new_without_default) | warn | `fn new() -> Self` method without `Default` implementation
[no_effect](https://github.com/Manishearth/rust-clippy/wiki#no_effect) | warn | statements with no effect
[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
[nonminimal_bool](https://github.com/Manishearth/rust-clippy/wiki#nonminimal_bool) | allow | checks for boolean expressions that can be written more concisely
[nonsensical_open_options](https://github.com/Manishearth/rust-clippy/wiki#nonsensical_open_options) | warn | nonsensical combination of options for opening a file
[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
[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)`

362
src/booleans.rs Normal file
View File

@ -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),
}
}
}

View File

@ -4,6 +4,8 @@
#![feature(iter_arith)]
#![feature(custom_attribute)]
#![feature(slice_patterns)]
#![feature(question_mark)]
#![feature(stmt_expr_attributes)]
#![allow(indexing_slicing, shadow_reuse, unknown_lints)]
// this only exists to allow the "dogfood" integration test to work
@ -35,6 +37,9 @@ extern crate semver;
// for regex checking
extern crate regex_syntax;
// for finding minimal boolean expressions
extern crate quine_mc_cluskey;
extern crate rustc_plugin;
extern crate rustc_const_eval;
use rustc_plugin::Registry;
@ -50,6 +55,7 @@ pub mod attrs;
pub mod bit_mask;
pub mod blacklisted_name;
pub mod block_in_if_condition;
pub mod booleans;
pub mod collapsible_if;
pub mod copies;
pub mod cyclomatic_complexity;
@ -149,6 +155,7 @@ pub fn plugin_registrar(reg: &mut Registry) {
// end deprecated lints, do not remove this comment, its used in `update_lints`
reg.register_late_lint_pass(box types::TypePass);
reg.register_late_lint_pass(box booleans::NonminimalBool);
reg.register_late_lint_pass(box misc::TopLevelRefPass);
reg.register_late_lint_pass(box misc::CmpNan);
reg.register_late_lint_pass(box eq_op::EqOp);
@ -228,6 +235,7 @@ pub fn plugin_registrar(reg: &mut Registry) {
reg.register_lint_group("clippy_pedantic", vec![
array_indexing::INDEXING_SLICING,
booleans::NONMINIMAL_BOOL,
enum_glob_use::ENUM_GLOB_USE,
matches::SINGLE_MATCH_ELSE,
methods::OPTION_UNWRAP_USED,
@ -260,6 +268,7 @@ pub fn plugin_registrar(reg: &mut Registry) {
blacklisted_name::BLACKLISTED_NAME,
block_in_if_condition::BLOCK_IN_IF_CONDITION_EXPR,
block_in_if_condition::BLOCK_IN_IF_CONDITION_STMT,
booleans::LOGIC_BUG,
collapsible_if::COLLAPSIBLE_IF,
copies::IF_SAME_THEN_ELSE,
copies::IFS_SAME_COND,

View File

@ -262,7 +262,7 @@ fn levenstein_not_1(a_name: &str, b_name: &str) -> bool {
}
if let Some(b2) = b_chars.next() {
// check if there's just one character inserted
return !(a == b2 && a_chars.eq(b_chars));
return a != b2 || a_chars.ne(b_chars);
} else {
// tuple
// ntuple

View File

@ -75,7 +75,8 @@ impl<'a, 'tcx: 'a> SpanlessEq<'a, 'tcx> {
}
(&ExprBlock(ref l), &ExprBlock(ref r)) => self.eq_block(l, r),
(&ExprBinary(l_op, ref ll, ref lr), &ExprBinary(r_op, ref rl, ref rr)) => {
l_op.node == r_op.node && self.eq_expr(ll, rl) && self.eq_expr(lr, rr)
l_op.node == r_op.node && self.eq_expr(ll, rl) && self.eq_expr(lr, rr) ||
swap_binop(l_op.node, ll, lr).map_or(false, |(l_op, ll, lr)| l_op == r_op.node && self.eq_expr(ll, rl) && self.eq_expr(lr, rr))
}
(&ExprBreak(li), &ExprBreak(ri)) => both(&li, &ri, |l, r| l.node.name.as_str() == r.node.name.as_str()),
(&ExprBox(ref l), &ExprBox(ref r)) => self.eq_expr(l, r),
@ -197,6 +198,23 @@ impl<'a, 'tcx: 'a> SpanlessEq<'a, 'tcx> {
}
}
fn swap_binop<'a>(binop: BinOp_, lhs: &'a Expr, rhs: &'a Expr) -> Option<(BinOp_, &'a Expr, &'a Expr)> {
match binop {
BiAdd |
BiMul |
BiBitXor |
BiBitAnd |
BiEq |
BiNe |
BiBitOr => Some((binop, rhs, lhs)),
BiLt => Some((BiGt, rhs, lhs)),
BiLe => Some((BiGe, rhs, lhs)),
BiGe => Some((BiLe, rhs, lhs)),
BiGt => Some((BiLt, rhs, lhs)),
BiShl | BiShr | BiRem | BiSub | BiDiv | BiAnd | BiOr => None,
}
}
/// Check if the two `Option`s are both `None` or some equal values as per `eq_fn`.
fn both<X, F>(l: &Option<X>, r: &Option<X>, mut eq_fn: F) -> bool
where F: FnMut(&X, &X) -> bool

View File

@ -4,6 +4,7 @@
#![deny(block_in_if_condition_expr)]
#![deny(block_in_if_condition_stmt)]
#![allow(unused, let_and_return)]
#![warn(nonminimal_bool)]
macro_rules! blocky {
@ -67,7 +68,7 @@ fn pred_test() {
fn condition_is_normal() -> i32 {
let x = 3;
if true && x == 3 {
if true && x == 3 { //~ WARN this boolean expression can be simplified
6
} else {
10

View File

@ -0,0 +1,87 @@
#![feature(plugin)]
#![plugin(clippy)]
#![deny(nonminimal_bool, logic_bug)]
#[allow(unused, many_single_char_names)]
fn main() {
let a: bool = unimplemented!();
let b: bool = unimplemented!();
let c: bool = unimplemented!();
let d: bool = unimplemented!();
let e: bool = unimplemented!();
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);
let _ = !true; //~ ERROR this boolean expression can be simplified
//~| HELP for further information visit
//~| SUGGESTION let _ = false;
let _ = !false; //~ ERROR this boolean expression can be simplified
//~| HELP for further information visit
//~| SUGGESTION let _ = true;
let _ = !!a; //~ ERROR this boolean expression can be simplified
//~| HELP for further information visit
//~| SUGGESTION let _ = a;
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
//~| HELP for further information visit
//~| SUGGESTION let _ = a;
// don't lint on cfgs
let _ = cfg!(you_shall_not_not_pass) && a;
let _ = a || !b || !c || !d || !e;
let _ = !(a && b || c);
let _ = !(!a && b); //~ ERROR this boolean expression can be simplified
//~| HELP for further information visit
//~| SUGGESTION let _ = !b || a;
}
#[allow(unused, many_single_char_names)]
fn equality_stuff() {
let a: i32 = unimplemented!();
let b: i32 = unimplemented!();
let c: i32 = unimplemented!();
let d: i32 = unimplemented!();
let e: i32 = unimplemented!();
let _ = a == b && a != b; //~ 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 _ = a == b && c == 5 && a == b; //~ ERROR this boolean expression can be simplified
//~| HELP for further information visit
//~| SUGGESTION let _ = a == b && c == 5;
let _ = a == b && c == 5 && b == a; //~ ERROR this boolean expression can be simplified
//~| HELP for further information visit
//~| SUGGESTION let _ = a == b && c == 5;
//~| HELP try
//~| SUGGESTION let _ = !(c != 5 || a != b);
let _ = a < b && a >= b; //~ 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 _ = a > b && a <= b; //~ 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 _ = a > b && a == b;
let _ = a != b || !(a != b || c == d); //~ ERROR this boolean expression can be simplified
//~| HELP for further information visit
//~| SUGGESTION let _ = c != d || a != b;
//~| HELP try
//~| SUGGESTION let _ = !(a == b && c == d);
}

View File

@ -3,7 +3,8 @@
#[deny(eq_op)]
#[allow(identity_op)]
#[allow(no_effect)]
#[allow(no_effect, unused_variables)]
#[deny(nonminimal_bool)]
fn main() {
// simple values and comparisons
1 == 1; //~ERROR equal expressions
@ -38,7 +39,21 @@ fn main() {
1 - 1; //~ERROR equal expressions
1 / 1; //~ERROR equal expressions
true && true; //~ERROR equal expressions
//~|ERROR this boolean expression can be simplified
true || true; //~ERROR equal expressions
//~|ERROR this boolean expression can be simplified
let a: u32 = unimplemented!();
let b: u32 = unimplemented!();
a == b && b == a; //~ERROR equal expressions
//~|ERROR this boolean expression can be simplified
a != b && b != a; //~ERROR equal expressions
//~|ERROR this boolean expression can be simplified
a < b && b > a; //~ERROR equal expressions
//~|ERROR this boolean expression can be simplified
a <= b && b >= a; //~ERROR equal expressions
//~|ERROR this boolean expression can be simplified
let mut a = vec![1];
a == a; //~ERROR equal expressions