Add a new "short_circuit_statement" lint (fixes #1194)

CHANGELOG.md

@@ -372,6 +372,7 @@ All notable changes to this project will be documented in this file.
 [`shadow_reuse`]: https://github.com/Manishearth/rust-clippy/wiki#shadow_reuse
 [`shadow_same`]: https://github.com/Manishearth/rust-clippy/wiki#shadow_same
 [`shadow_unrelated`]: https://github.com/Manishearth/rust-clippy/wiki#shadow_unrelated
+[`short_circuit_statement`]: https://github.com/Manishearth/rust-clippy/wiki#short_circuit_statement
 [`should_implement_trait`]: https://github.com/Manishearth/rust-clippy/wiki#should_implement_trait
 [`similar_names`]: https://github.com/Manishearth/rust-clippy/wiki#similar_names
 [`single_char_pattern`]: https://github.com/Manishearth/rust-clippy/wiki#single_char_pattern

README.md

@@ -179,7 +179,7 @@ transparently:
 
 ## Lints
 
-There are 181 lints included in this crate:
+There are 182 lints included in this crate:
 
 name                                                                                                                   | default | triggers on
 -----------------------------------------------------------------------------------------------------------------------|---------|----------------------------------------------------------------------------------------------------------------------------------
@@ -320,6 +320,7 @@ name
 [shadow_reuse](https://github.com/Manishearth/rust-clippy/wiki#shadow_reuse)                                           | allow   | rebinding a name to an expression that re-uses the original value, e.g. `let x = x + 1`
 [shadow_same](https://github.com/Manishearth/rust-clippy/wiki#shadow_same)                                             | allow   | rebinding a name to itself, e.g. `let mut x = &mut x`
 [shadow_unrelated](https://github.com/Manishearth/rust-clippy/wiki#shadow_unrelated)                                   | allow   | rebinding a name without even using the original value
+[short_circuit_statement](https://github.com/Manishearth/rust-clippy/wiki#short_circuit_statement)                     | warn    | using a short circuit boolean condition as a statement
 [should_implement_trait](https://github.com/Manishearth/rust-clippy/wiki#should_implement_trait)                       | warn    | defining a method that should be implementing a std trait
 [similar_names](https://github.com/Manishearth/rust-clippy/wiki#similar_names)                                         | allow   | similarly named items and bindings
 [single_char_pattern](https://github.com/Manishearth/rust-clippy/wiki#single_char_pattern)                             | warn    | using a single-character str where a char could be used, e.g. `_.split("x")`

clippy_lints/src/lib.rs

@@ -421,6 +421,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry) {
         misc::FLOAT_CMP,
         misc::MODULO_ONE,
         misc::REDUNDANT_PATTERN,
+        misc::SHORT_CIRCUIT_STATEMENT,
         misc::TOPLEVEL_REF_ARG,
         misc_early::BUILTIN_TYPE_SHADOW,
         misc_early::DOUBLE_NEG,

clippy_lints/src/misc.rs

@@ -154,6 +154,25 @@ declare_lint! {
     "using a binding which is prefixed with an underscore"
 }
 
+/// **What it does:** Checks for the use of short circuit boolean conditions as a
+/// statement.
+///
+/// **Why is this bad?** Using a short circuit boolean condition as a statement may
+/// hide the fact that the second part is executed or not depending on the outcome of
+/// the first part.
+///
+/// **Known problems:** None.
+///
+/// **Example:**
+/// ```rust
+/// f() && g();  // We should write `if f() { g(); }`.
+/// ```
+declare_lint! {
+    pub SHORT_CIRCUIT_STATEMENT,
+    Warn,
+    "using a short circuit boolean condition as a statement"
+}
+
 #[derive(Copy, Clone)]
 pub struct Pass;
 
@@ -165,7 +184,8 @@ impl LintPass for Pass {
                     CMP_OWNED,
                     MODULO_ONE,
                     REDUNDANT_PATTERN,
-                    USED_UNDERSCORE_BINDING)
+                    USED_UNDERSCORE_BINDING,
+                    SHORT_CIRCUIT_STATEMENT)
     }
 }
 
@@ -224,7 +244,23 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Pass {
                                                initref=initref));
                 }
             );
-        }}
+        }};
+        if_let_chain! {[
+            let StmtSemi(ref expr, _) = s.node,
+            let Expr_::ExprBinary(ref binop, ref a, ref b) = expr.node,
+            binop.node == BiAnd || binop.node == BiOr,
+            let Some(sugg) = Sugg::hir_opt(cx, a),
+        ], {
+            span_lint_and_then(cx,
+                SHORT_CIRCUIT_STATEMENT,
+                s.span,
+                "boolean short circuit operator in statement may be clearer using an explicit test",
+                |db| {
+                    let sugg = if binop.node == BiOr { !sugg } else { sugg };
+                    db.span_suggestion(s.span, "replace it with",
+                                       format!("if {} {{ {}; }}", sugg, &snippet(cx, b.span, "..")));
+                });
+        }};
     }
 
     fn check_expr(&mut self, cx: &LateContext<'a, 'tcx>, expr: &'tcx Expr) {

clippy_lints/src/no_effect.rs

@@ -1,6 +1,6 @@
 use rustc::lint::{LateContext, LateLintPass, LintArray, LintPass};
 use rustc::hir::def::Def;
-use rustc::hir::{Expr, Expr_, Stmt, StmtSemi, BlockCheckMode, UnsafeSource};
+use rustc::hir::{Expr, Expr_, Stmt, StmtSemi, BlockCheckMode, UnsafeSource, BiAnd, BiOr};
 use utils::{in_macro, span_lint, snippet_opt, span_lint_and_then};
 use std::ops::Deref;
 
@@ -134,8 +134,10 @@ fn reduce_expression<'a>(cx: &LateContext, expr: &'a Expr) -> Option<Vec<&'a Exp
         return None;
     }
     match expr.node {
-        Expr_::ExprIndex(ref a, ref b) |
+        Expr_::ExprIndex(ref a, ref b) => Some(vec![&**a, &**b]),
-        Expr_::ExprBinary(_, ref a, ref b) => Some(vec![&**a, &**b]),
+        Expr_::ExprBinary(ref binop, ref a, ref b) if binop.node != BiAnd && binop.node != BiOr => {
+            Some(vec![&**a, &**b])
+        },
         Expr_::ExprArray(ref v) |
         Expr_::ExprTup(ref v) => Some(v.iter().collect()),
         Expr_::ExprRepeat(ref inner, _) |

tests/compile-fail/diverging_sub_expression.rs

@@ -12,7 +12,7 @@ impl A {
     fn foo(&self) -> ! { diverge() }
 }
 
-#[allow(unused_variables, unnecessary_operation)]
+#[allow(unused_variables, unnecessary_operation, short_circuit_statement)]
 fn main() {
     let b = true;
     b || diverge(); //~ ERROR sub-expression diverges

tests/compile-fail/eq_op.rs

@@ -3,7 +3,7 @@
 
 #[deny(eq_op)]
 #[allow(identity_op, double_parens)]
-#[allow(no_effect, unused_variables, unnecessary_operation)]
+#[allow(no_effect, unused_variables, unnecessary_operation, short_circuit_statement)]
 #[deny(nonminimal_bool)]
 fn main() {
     // simple values and comparisons

tests/compile-fail/short_circuit_statement.rs

@@ -0,0 +1,27 @@
+#![feature(plugin)]
+#![plugin(clippy)]
+
+#![deny(short_circuit_statement)]
+
+fn main() {
+    f() && g();
+    //~^ ERROR boolean short circuit operator
+    //~| HELP replace it with
+    //~| SUGGESTION if f() { g(); }
+    f() || g();
+    //~^ ERROR boolean short circuit operator
+    //~| HELP replace it with
+    //~| SUGGESTION if !f() { g(); }
+    1 == 2 || g();
+    //~^ ERROR boolean short circuit operator
+    //~| HELP replace it with
+    //~| SUGGESTION if !(1 == 2) { g(); }
+}
+
+fn f() -> bool {
+    true
+}
+
+fn g() -> bool {
+    false
+}