From 185da552635321d51be06e66115199f11a586289 Mon Sep 17 00:00:00 2001 From: Ravi Shankar Date: Sun, 27 Sep 2015 13:09:42 +0530 Subject: [PATCH] extending while_let to warn for more statements --- src/loops.rs | 67 +++++++++++++++++++++++++------- src/matches.rs | 2 +- src/utils.rs | 12 ++++-- tests/compile-fail/while_loop.rs | 16 +++----- 4 files changed, 69 insertions(+), 28 deletions(-) diff --git a/src/loops.rs b/src/loops.rs index 4afa31ff5..b48ae13e7 100644 --- a/src/loops.rs +++ b/src/loops.rs @@ -6,6 +6,7 @@ use rustc::middle::ty; use rustc::middle::def::DefLocal; use consts::{constant_simple, Constant}; use rustc::front::map::Node::{NodeBlock}; +use std::borrow::Cow; use std::collections::{HashSet,HashMap}; use syntax::ast::Lit_::*; @@ -159,10 +160,27 @@ impl LateLintPass for LoopsPass { } } // check for `loop { if let {} else break }` that could be `while let` - // (also matches explicit "match" instead of "if let") + // (also matches an explicit "match" instead of "if let") + // (even if the "match" or "if let" is used for declaration) if let ExprLoop(ref block, _) = expr.node { + // extract the first statement (if any) in a block + let inner_stmt = extract_expr_from_first_stmt(block); // extract a single expression - if let Some(inner) = extract_single_expr(block) { + let inner_expr = extract_first_expr(block); + let extracted = match inner_stmt { + Some(_) => inner_stmt, + None => inner_expr, + }; + + if let Some(inner) = extracted { + // collect remaining expressions below the match + let other_stuff = block.stmts + .iter() + .skip(1) + .map(|stmt| { + format!("{}", snippet(cx, stmt.span, "..")) + }).collect::>(); + if let ExprMatch(ref matchexpr, ref arms, ref source) = inner.node { // ensure "if let" compatible match structure match *source { @@ -174,12 +192,19 @@ impl LateLintPass for LoopsPass { is_break_expr(&arms[1].body) { if in_external_macro(cx, expr.span) { return; } + let loop_body = match inner_stmt { + // FIXME: should probably be an ellipsis + // tabbing and newline is probably a bad idea, especially for large blocks + Some(_) => Cow::Owned(format!("{{\n {}\n}}", other_stuff.join("\n "))), + None => expr_block(cx, &arms[0].body, + Some(other_stuff.join("\n ")), ".."), + }; 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, ".."))); + loop_body)); }, _ => () } @@ -276,23 +301,38 @@ fn is_ref_iterable_type(cx: &LateContext, e: &Expr) -> bool { } fn is_iterable_array(ty: ty::Ty) -> bool { - //IntoIterator is currently only implemented for array sizes <= 32 in rustc + // IntoIterator is currently only implemented for array sizes <= 32 in rustc match ty.sty { ty::TyArray(_, 0...32) => true, _ => 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), +/// If a block begins with a statement (possibly a `let` binding) and has an expression, return it. +fn extract_expr_from_first_stmt(block: &Block) -> Option<&Expr> { + match block.expr { + Some(_) => None, + None => match block.stmts[0].node { + StmtDecl(ref decl, _) => match decl.node { + DeclLocal(ref local) => match local.init { + Some(ref expr) => Some(expr), + None => None, + }, + _ => None, + }, + _ => None, + }, + } +} + +/// If a block begins with an expression (with or without semicolon), return it. +fn extract_first_expr(block: &Block) -> Option<&Expr> { + match block.expr { + Some(ref expr) => Some(expr), + None => match block.stmts[0].node { + StmtExpr(ref expr, _) | StmtSemi(ref expr, _) => Some(expr), _ => None, }, - (&0, &Some(ref expr)) => Some(expr), - _ => None } } @@ -300,7 +340,8 @@ fn extract_single_expr(block: &Block) -> Option<&Expr> { fn is_break_expr(expr: &Expr) -> bool { match expr.node { ExprBreak(None) => true, - ExprBlock(ref b) => match extract_single_expr(b) { + // there won't be a `let = break` and so we can safely ignore the StmtDecl case + ExprBlock(ref b) => match extract_first_expr(b) { Some(ref subexpr) => is_break_expr(subexpr), None => false, }, diff --git a/src/matches.rs b/src/matches.rs index 4e49cd3ff..e935a6aa6 100644 --- a/src/matches.rs +++ b/src/matches.rs @@ -43,7 +43,7 @@ impl LateLintPass for MatchPass { &format!("try\nif let {} = {} {}", snippet(cx, arms[0].pats[0].span, ".."), snippet(cx, ex.span, ".."), - expr_block(cx, &arms[0].body, ".."))); + expr_block(cx, &arms[0].body, None, ".."))); } // check preconditions for MATCH_REF_PATS diff --git a/src/utils.rs b/src/utils.rs index 250e24ac6..09924d902 100644 --- a/src/utils.rs +++ b/src/utils.rs @@ -130,12 +130,18 @@ pub fn snippet_block<'a, T: LintContext>(cx: &T, span: Span, default: &'a str) - } /// Like snippet_block, but add braces if the expr is not an ExprBlock -pub fn expr_block<'a, T: LintContext>(cx: &T, expr: &Expr, default: &'a str) -> Cow<'a, str> { +/// Also takes an Option which can be put inside the braces +pub fn expr_block<'a, T: LintContext>(cx: &T, expr: &Expr, + option: Option, + default: &'a str) -> Cow<'a, str> { let code = snippet_block(cx, expr.span, default); + let string = option.map_or("".to_owned(), |s| s); if let ExprBlock(_) = expr.node { - code - } else { + Cow::Owned(format!("{}{}", code, string)) + } else if string.is_empty() { Cow::Owned(format!("{{ {} }}", code)) + } else { + Cow::Owned(format!("{{\n{};\n{}\n}}", code, string)) } } diff --git a/tests/compile-fail/while_loop.rs b/tests/compile-fail/while_loop.rs index bc09168fa..ef798b2a7 100755 --- a/tests/compile-fail/while_loop.rs +++ b/tests/compile-fail/while_loop.rs @@ -4,13 +4,6 @@ #[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; @@ -30,12 +23,13 @@ fn main() { None => break }; } - loop { // no error, match is not the only statement - match y { - Some(_x) => true, + loop { //~ERROR + let x = match y { + Some(x) => x, None => break }; - let _x = 1; + let _x = x; + let _str = "foo"; } loop { // no error, else branch does something other than break match y {