diff --git a/README.md b/README.md index 478bb25272f..92a03b31530 100644 --- a/README.md +++ b/README.md @@ -4,7 +4,7 @@ A collection of lints that give helpful tips to newbies and catch oversights. ##Lints -There are 49 lints included in this crate: +There are 50 lints included in this crate: name | default | meaning -----------------------------------------------------------------------------------------------------|---------|----------------------------------------------------------------------------------------------------------------------------------------------------------------------------- @@ -56,6 +56,7 @@ name [toplevel_ref_arg](https://github.com/Manishearth/rust-clippy/wiki#toplevel_ref_arg) | warn | a function argument is declared `ref` (i.e. `fn foo(ref x: u8)`, but not `fn foo((ref x, ref y): (u8, u8))`) [type_complexity](https://github.com/Manishearth/rust-clippy/wiki#type_complexity) | warn | usage of very complex types; recommends factoring out parts into `type` definitions [unit_cmp](https://github.com/Manishearth/rust-clippy/wiki#unit_cmp) | warn | comparing unit values (which is always `true` or `false`, respectively) +[while_let_loop](https://github.com/Manishearth/rust-clippy/wiki#while_let_loop) | warn | `loop { if let { ... } else break }` can be written as a `while let` loop [zero_width_space](https://github.com/Manishearth/rust-clippy/wiki#zero_width_space) | deny | using a zero-width space in a string literal, which is confusing More to come, please [file an issue](https://github.com/Manishearth/rust-clippy/issues) if you have ideas! diff --git a/src/lib.rs b/src/lib.rs index 32fc953d1c3..d69d352abc6 100755 --- a/src/lib.rs +++ b/src/lib.rs @@ -96,6 +96,7 @@ pub fn plugin_registrar(reg: &mut Registry) { loops::EXPLICIT_ITER_LOOP, loops::ITER_NEXT_LOOP, loops::NEEDLESS_RANGE_LOOP, + loops::WHILE_LET_LOOP, matches::MATCH_REF_PATS, matches::SINGLE_MATCH, methods::OPTION_UNWRAP_USED, diff --git a/src/loops.rs b/src/loops.rs index d12393dba68..eba706ed7e1 100644 --- a/src/loops.rs +++ b/src/loops.rs @@ -4,7 +4,8 @@ use syntax::visit::{Visitor, walk_expr}; use rustc::middle::ty; use std::collections::HashSet; -use utils::{snippet, span_lint, get_parent_expr, match_trait_method, match_type, walk_ptrs_ty}; +use utils::{snippet, span_lint, get_parent_expr, match_trait_method, match_type, walk_ptrs_ty, + in_external_macro, expr_block, span_help_and_lint}; use utils::{VEC_PATH, LL_PATH}; declare_lint!{ pub NEEDLESS_RANGE_LOOP, Warn, @@ -16,12 +17,16 @@ declare_lint!{ pub EXPLICIT_ITER_LOOP, Warn, declare_lint!{ pub ITER_NEXT_LOOP, Warn, "for-looping over `_.next()` which is probably not intended" } +declare_lint!{ pub WHILE_LET_LOOP, Warn, + "`loop { if let { ... } else break }` can be written as a `while let` loop" } + #[derive(Copy, Clone)] pub struct LoopsPass; impl LintPass for LoopsPass { fn get_lints(&self) -> LintArray { - lint_array!(NEEDLESS_RANGE_LOOP, EXPLICIT_ITER_LOOP, ITER_NEXT_LOOP) + lint_array!(NEEDLESS_RANGE_LOOP, EXPLICIT_ITER_LOOP, ITER_NEXT_LOOP, + WHILE_LET_LOOP) } fn check_expr(&mut self, cx: &Context, expr: &Expr) { @@ -36,7 +41,8 @@ impl LintPass for LoopsPass { walk_expr(&mut visitor, body); // linting condition: we only indexed one variable if visitor.indexed.len() == 1 { - let indexed = visitor.indexed.into_iter().next().expect("Len was nonzero, but no contents found"); + let indexed = visitor.indexed.into_iter().next().expect( + "Len was nonzero, but no contents found"); if visitor.nonindex { span_lint(cx, NEEDLESS_RANGE_LOOP, expr.span, &format!( "the loop variable `{}` is used to index `{}`. Consider using \ @@ -77,6 +83,34 @@ impl LintPass for LoopsPass { } } } + // check for `loop { if let {} else break }` that could be `while let` + // (also matches explicit "match" instead of "if let") + if let ExprLoop(ref block, _) = expr.node { + // extract a single expression + if let Some(inner) = extract_single_expr(block) { + if let ExprMatch(ref matchexpr, ref arms, ref source) = inner.node { + // ensure "if let" compatible match structure + match *source { + MatchSource::Normal | MatchSource::IfLetDesugar{..} => if + arms.len() == 2 && + arms[0].pats.len() == 1 && arms[0].guard.is_none() && + arms[1].pats.len() == 1 && arms[1].guard.is_none() && + // finally, check for "break" in the second clause + is_break_expr(&arms[1].body) + { + if in_external_macro(cx, expr.span) { return; } + span_help_and_lint(cx, WHILE_LET_LOOP, expr.span, + "this loop could be written as a `while let` loop", + &format!("try\nwhile let {} = {} {}", + snippet(cx, arms[0].pats[0].span, ".."), + snippet(cx, matchexpr.span, ".."), + expr_block(cx, &arms[0].body, ".."))); + }, + _ => () + } + } + } + } } } @@ -158,3 +192,28 @@ fn is_array(ty: ty::Ty) -> bool { _ => false } } + +/// If block consists of a single expression (with or without semicolon), return it. +fn extract_single_expr(block: &Block) -> Option<&Expr> { + match (&block.stmts.len(), &block.expr) { + (&1, &None) => match block.stmts[0].node { + StmtExpr(ref expr, _) | + StmtSemi(ref expr, _) => Some(expr), + _ => None, + }, + (&0, &Some(ref expr)) => Some(expr), + _ => None + } +} + +/// Return true if expr contains a single break expr (maybe within a block). +fn is_break_expr(expr: &Expr) -> bool { + match expr.node { + ExprBreak(None) => true, + ExprBlock(ref b) => match extract_single_expr(b) { + Some(ref subexpr) => is_break_expr(subexpr), + None => false, + }, + _ => false, + } +} diff --git a/src/matches.rs b/src/matches.rs index 1afb61e9b9e..3d04c9210ce 100644 --- a/src/matches.rs +++ b/src/matches.rs @@ -1,10 +1,8 @@ use rustc::lint::*; use syntax::ast; use syntax::ast::*; -use std::borrow::Cow; -use utils::{snippet, snippet_block}; -use utils::{span_lint, span_help_and_lint, in_external_macro}; +use utils::{snippet, span_lint, span_help_and_lint, in_external_macro, expr_block}; declare_lint!(pub SINGLE_MATCH, Warn, "a match statement with a single nontrivial arm (i.e, where the other arm \ @@ -38,12 +36,6 @@ impl LintPass for MatchPass { is_unit_expr(&arms[1].body) { if in_external_macro(cx, expr.span) {return;} - let body_code = snippet_block(cx, arms[0].body.span, ".."); - let body_code = if let ExprBlock(_) = arms[0].body.node { - body_code - } else { - Cow::Owned(format!("{{ {} }}", body_code)) - }; span_help_and_lint(cx, SINGLE_MATCH, expr.span, "you seem to be trying to use match for \ destructuring a single pattern. Did you mean to \ @@ -51,8 +43,7 @@ impl LintPass for MatchPass { &format!("try\nif let {} = {} {}", snippet(cx, arms[0].pats[0].span, ".."), snippet(cx, ex.span, ".."), - body_code) - ); + expr_block(cx, &arms[0].body, ".."))); } // check preconditions for MATCH_REF_PATS diff --git a/src/utils.rs b/src/utils.rs index b6fae89fc31..f16387f606d 100644 --- a/src/utils.rs +++ b/src/utils.rs @@ -103,6 +103,16 @@ pub fn snippet_block<'a>(cx: &Context, span: Span, default: &'a str) -> Cow<'a, trim_multiline(snip, true) } +/// Like snippet_block, but add braces if the expr is not an ExprBlock +pub fn expr_block<'a>(cx: &Context, expr: &Expr, default: &'a str) -> Cow<'a, str> { + let code = snippet_block(cx, expr.span, default); + if let ExprBlock(_) = expr.node { + code + } else { + Cow::Owned(format!("{{ {} }}", code)) + } +} + /// Trim indentation from a multiline string /// with possibility of ignoring the first line pub fn trim_multiline(s: Cow, ignore_first: bool) -> Cow { diff --git a/tests/compile-fail/while_loop.rs b/tests/compile-fail/while_loop.rs new file mode 100755 index 00000000000..bc09168fad0 --- /dev/null +++ b/tests/compile-fail/while_loop.rs @@ -0,0 +1,52 @@ +#![feature(plugin)] +#![plugin(clippy)] + +#[deny(while_let_loop)] +fn main() { + let y = Some(true); + loop { //~ERROR + if let Some(_x) = y { + let _v = 1; + } else { + break; + } + } + loop { //~ERROR + if let Some(_x) = y { + let _v = 1; + } else { + break + } + } + loop { // no error, break is not in else clause + if let Some(_x) = y { + let _v = 1; + } + break; + } + loop { //~ERROR + match y { + Some(_x) => true, + None => break + }; + } + loop { // no error, match is not the only statement + match y { + Some(_x) => true, + None => break + }; + let _x = 1; + } + loop { // no error, else branch does something other than break + match y { + Some(_x) => true, + _ => { + let _z = 1; + break; + } + }; + } + while let Some(x) = y { // no error, obviously + println!("{}", x); + } +}