diff --git a/CHANGELOG.md b/CHANGELOG.md index af28d6074..1f2608089 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -381,6 +381,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 diff --git a/README.md b/README.md index 94017cd53..85c6136b9 100644 --- a/README.md +++ b/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")` diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index cdcb271d8..3e0aa17f0 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -423,6 +423,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, diff --git a/clippy_lints/src/misc.rs b/clippy_lints/src/misc.rs index 4af47d731..b604c7222 100644 --- a/clippy_lints/src/misc.rs +++ b/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) { diff --git a/clippy_lints/src/no_effect.rs b/clippy_lints/src/no_effect.rs index a87c9d0f7..97317d00c 100644 --- a/clippy_lints/src/no_effect.rs +++ b/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 Some(vec![&**a, &**b]), + Expr_::ExprIndex(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, _) | diff --git a/tests/compile-fail/diverging_sub_expression.rs b/tests/compile-fail/diverging_sub_expression.rs index e7daebfdf..02e6d0693 100644 --- a/tests/compile-fail/diverging_sub_expression.rs +++ b/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 diff --git a/tests/compile-fail/eq_op.rs b/tests/compile-fail/eq_op.rs index 0ec86157d..c133f4227 100644 --- a/tests/compile-fail/eq_op.rs +++ b/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 diff --git a/tests/compile-fail/short_circuit_statement.rs b/tests/compile-fail/short_circuit_statement.rs new file mode 100644 index 000000000..23dfc0ebc --- /dev/null +++ b/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 +}