better simplification

Cargo.toml

@@ -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"
 
 [dev-dependencies]
 compiletest_rs = "0.1.0"

README.md

@@ -14,7 +14,7 @@ Table of contents:
 * [License](#license)
 
 ##Lints
-There are 137 lints included in this crate:
+There are 138 lints included in this crate:
 
 name                                                                                                                 | default | meaning
 ---------------------------------------------------------------------------------------------------------------------|---------|------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
@@ -97,6 +97,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)                                   | warn    | 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)`

src/booleans.rs

@@ -0,0 +1,155 @@
+use rustc::lint::*;
+use rustc_front::hir::*;
+use rustc_front::intravisit::*;
+use syntax::ast::LitKind;
+use utils::{span_lint_and_then, in_macro, snippet_opt};
+
+/// **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:** None
+///
+/// **Example:** `if a && b || a` should be `if a`
+declare_lint! {
+    pub NONMINIMAL_BOOL, Warn,
+    "checks for boolean expressions that can be written more concisely"
+}
+
+#[derive(Copy,Clone)]
+pub struct NonminimalBool;
+
+impl LintPass for NonminimalBool {
+    fn get_lints(&self) -> LintArray {
+        lint_array!(NONMINIMAL_BOOL)
+    }
+}
+
+impl LateLintPass for NonminimalBool {
+    fn check_crate(&mut self, cx: &LateContext, krate: &Crate) {
+        krate.visit_all_items(&mut NonminimalBoolVisitor(cx))
+    }
+}
+
+struct NonminimalBoolVisitor<'a, 'tcx: 'a>(&'a LateContext<'a, 'tcx>);
+
+use quine_mc_cluskey::Bool;
+struct Hir2Qmm<'tcx>(Vec<&'tcx Expr>);
+
+impl<'tcx> Hir2Qmm<'tcx> {
+    fn extract(&mut self, op: BinOp_, a: &[&'tcx 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: &'tcx Expr) -> Result<Bool, String> {
+        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),
+                    _ => {},
+                }
+            },
+            _ => {},
+        }
+        let n = self.0.len();
+        self.0.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(cx: &LateContext, suggestion: &Bool, terminals: &[&Expr], mut s: String) -> String {
+        use quine_mc_cluskey::Bool::*;
+        match *suggestion {
+            True => {
+                s.extend("true".chars());
+                s
+            },
+            False => {
+                s.extend("false".chars());
+                s
+            },
+            Not(ref inner) => {
+                s.push('!');
+                recurse(cx, inner, terminals, s)
+            },
+            And(ref v) => {
+                s = recurse(cx, &v[0], terminals, s);
+                for inner in &v[1..] {
+                    s.extend(" && ".chars());
+                    s = recurse(cx, inner, terminals, s);
+                }
+                s
+            },
+            Or(ref v) => {
+                s = recurse(cx, &v[0], terminals, s);
+                for inner in &v[1..] {
+                    s.extend(" || ".chars());
+                    s = recurse(cx, inner, terminals, s);
+                }
+                s
+            },
+            Term(n) => {
+                s.extend(snippet_opt(cx, terminals[n as usize].span).expect("don't try to improve booleans created by macros").chars());
+                s
+            }
+        }
+    }
+    recurse(cx, suggestion, terminals, String::new())
+}
+
+impl<'a, 'tcx> NonminimalBoolVisitor<'a, 'tcx> {
+    fn bool_expr(&self, e: &Expr) {
+        let mut h2q = Hir2Qmm(Vec::new());
+        if let Ok(expr) = h2q.run(e) {
+            let simplified = expr.simplify();
+            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.0));
+                    }
+                });
+            }
+        }
+    }
+}
+
+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),
+        }
+    }
+}

src/lib.rs

@@ -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, it’s 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);
@@ -260,6 +267,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::NONMINIMAL_BOOL,
         collapsible_if::COLLAPSIBLE_IF,
         copies::IF_SAME_THEN_ELSE,
         copies::IFS_SAME_COND,

tests/compile-fail/block_in_if_condition.rs

@@ -67,7 +67,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

tests/compile-fail/eq_op.rs

@@ -38,7 +38,9 @@ fn main() {
     1 - 1; //~ERROR equal expressions
     1 / 1; //~ERROR equal expressions
     true && true; //~ERROR equal expressions
+    //~|WARN this boolean expression can be simplified
     true || true; //~ERROR equal expressions
+    //~|WARN this boolean expression can be simplified
 
     let mut a = vec![1];
     a == a; //~ERROR equal expressions