From 240471f92662b556a94284d7cc52f82827f6a331 Mon Sep 17 00:00:00 2001 From: Yati Sagade Date: Sun, 21 Aug 2016 23:18:58 +0200 Subject: [PATCH 01/13] Needless continue: This is a complete rewrite of this lint as an early-pass lint. The previous version had troubles computing suggestions without macro expansions creeping in. This fixes it by using original_sp, which works on AST nodes. --- CHANGELOG.md | 1 + clippy_lints/src/lib.rs | 3 + clippy_lints/src/needless_continue.rs | 468 ++++++++++++++++++++++++++ 3 files changed, 472 insertions(+) create mode 100644 clippy_lints/src/needless_continue.rs diff --git a/CHANGELOG.md b/CHANGELOG.md index 1aee256f8bba..e02f9cd1b7b6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -405,6 +405,7 @@ All notable changes to this project will be documented in this file. [`mutex_integer`]: https://github.com/Manishearth/rust-clippy/wiki#mutex_integer [`needless_bool`]: https://github.com/Manishearth/rust-clippy/wiki#needless_bool [`needless_borrow`]: https://github.com/Manishearth/rust-clippy/wiki#needless_borrow +[`needless_continue`]: https://github.com/Manishearth/rust-clippy/wiki#needless_continue [`needless_lifetimes`]: https://github.com/Manishearth/rust-clippy/wiki#needless_lifetimes [`needless_pass_by_value`]: https://github.com/Manishearth/rust-clippy/wiki#needless_pass_by_value [`needless_range_loop`]: https://github.com/Manishearth/rust-clippy/wiki#needless_range_loop diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index f84b023c3d8c..ef85b029617d 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -107,6 +107,7 @@ pub mod mutex_atomic; pub mod needless_bool; pub mod needless_borrow; pub mod needless_pass_by_value; +pub mod needless_continue; pub mod needless_update; pub mod neg_multiply; pub mod new_without_default; @@ -219,6 +220,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry) { reg.register_late_lint_pass(box approx_const::Pass); reg.register_late_lint_pass(box misc::Pass); reg.register_early_lint_pass(box precedence::Precedence); + reg.register_early_lint_pass(box needless_continue::NeedlessContinue); reg.register_late_lint_pass(box eta_reduction::EtaPass); reg.register_late_lint_pass(box identity_op::IdentityOp); reg.register_early_lint_pass(box items_after_statements::ItemsAfterStatements); @@ -461,6 +463,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry) { needless_bool::NEEDLESS_BOOL, needless_borrow::NEEDLESS_BORROW, needless_pass_by_value::NEEDLESS_PASS_BY_VALUE, + needless_continue::NEEDLESS_CONTINUE, needless_update::NEEDLESS_UPDATE, neg_multiply::NEG_MULTIPLY, new_without_default::NEW_WITHOUT_DEFAULT, diff --git a/clippy_lints/src/needless_continue.rs b/clippy_lints/src/needless_continue.rs new file mode 100644 index 000000000000..042f45cefcf1 --- /dev/null +++ b/clippy_lints/src/needless_continue.rs @@ -0,0 +1,468 @@ +//! Checks for continue statements in loops that are redundant. +//! +//! For example, the lint would catch +//! +//! ``` +//! while condition() { +//! update_condition(); +//! if x { +//! // ... +//! } else { +//! continue; +//! } +//! println!("Hello, world"); +//! } +//! ``` +//! +//! And suggest something like this: +//! +//! ``` +//! while condition() { +//! update_condition(); +//! if x { +//! // ... +//! println!("Hello, world"); +//! } +//! } +//! ``` +//! +//! This lint is **warn** by default. +use std; +use rustc::lint::*; +use syntax::ast; +use syntax::codemap::{original_sp,DUMMY_SP}; + +use utils::{in_macro, span_help_and_lint, snippet_block, snippet}; +use self::LintType::*; + +/// **What it does:** The lint checks for `if`-statements appearing in loops +/// that contain a `continue` statement in either their main blocks or their +/// `else`-blocks, when omitting the `else`-block possibly with some +/// rearrangement of code can make the code easier to understand. +/// +/// **Why is this bad?** Having explicit `else` blocks for `if` statements +/// containing `continue` in their THEN branch adds unnecessary branching and +/// nesting to the code. Having an else block containing just `continue` can +/// also be better written by grouping the statements following the whole `if` +/// statement within the THEN block and omitting the else block completely. +/// +/// **Known problems:** None +/// +/// **Example:** +/// ```rust +/// while condition() { +/// update_condition(); +/// if x { +/// // ... +/// } else { +/// continue; +/// } +/// println!("Hello, world"); +/// } +/// ``` +/// +/// Could be rewritten as +/// +/// ```rust +/// while condition() { +/// update_condition(); +/// if x { +/// // ... +/// println!("Hello, world"); +/// } +/// } +/// ``` +/// +/// As another example, the following code +/// +/// ```rust +/// loop { +/// if waiting() { +/// continue; +/// } else { +/// // Do something useful +/// } +/// } +/// ``` +/// Could be rewritten as +/// +/// ```rust +/// loop { +/// if waiting() { +/// continue; +/// } +/// // Do something useful +/// } +/// ``` +declare_lint! { + pub NEEDLESS_CONTINUE, + Warn, + "`continue` statements that can be replaced by a rearrangement of code" +} + +#[derive(Copy,Clone)] +pub struct NeedlessContinue; + +impl LintPass for NeedlessContinue { + fn get_lints(&self) -> LintArray { + lint_array!(NEEDLESS_CONTINUE) + } +} + +impl EarlyLintPass for NeedlessContinue { + fn check_expr(&mut self, ctx: &EarlyContext, expr: &ast::Expr) { + if !in_macro(ctx, expr.span) { + check_and_warn(ctx, expr); + } + } +} + +/// Given an Expr, returns true if either of the following is true +/// +/// - The Expr is a `continue` node. +/// - The Expr node is a block with the first statement being a `continue`. +/// +fn needless_continue_in_else(else_expr: &ast::Expr) -> bool { + let mut found = false; + match else_expr.node { + ast::ExprKind::Block(ref else_block) => { + found = is_first_block_stmt_continue(else_block); + }, + ast::ExprKind::Continue(_) => { found = true }, + _ => { }, + }; + found +} + +fn is_first_block_stmt_continue(block: &ast::Block) -> bool { + let mut ret = false; + block.stmts.get(0).map(|stmt| { + if_let_chain! {[ + let ast::StmtKind::Semi(ref e) = stmt.node, + let ast::ExprKind::Continue(_) = e.node, + ], { + ret = true; + }} + }).unwrap_or(()); + ret +} + +/// If `expr` is a loop expression (while/while let/for/loop), calls `func` with +/// the AST object representing the loop block of `expr`. +fn with_loop_block(expr: &ast::Expr, mut func: F) where F: FnMut(&ast::Block) { + match expr.node { + ast::ExprKind::While(_, ref loop_block, _) | + ast::ExprKind::WhileLet(_, _, ref loop_block, _) | + ast::ExprKind::ForLoop( _, _, ref loop_block, _) | + ast::ExprKind::Loop(ref loop_block, _) => func(loop_block), + _ => {}, + } +} + +/// If `stmt` is an if expression node with an else branch, calls func with the +/// following: +/// +/// - The if Expr, +/// - The if condition Expr, +/// - The then block of this if Expr, and +/// - The else expr. +/// +fn with_if_expr(stmt: &ast::Stmt, mut func: F) + where F: FnMut(&ast::Expr, &ast::Expr, &ast::Block, &ast::Expr) { + match stmt.node { + ast::StmtKind::Semi(ref e) | + ast::StmtKind::Expr(ref e) => { + if let ast::ExprKind::If(ref cond, ref if_block, Some(ref else_expr)) = e.node { + func(e, cond, if_block, else_expr); + } + }, + _ => { }, + } +} + +/// A type to distinguish between the two distinct cases this lint handles. +enum LintType { + ContinueInsideElseBlock, + ContinueInsideThenBlock, +} + +/// Data we pass around for construction of help messages. +struct LintData<'a> { + if_expr: &'a ast::Expr, // The `if` expr encountered in the above loop. + if_cond: &'a ast::Expr, // The condition expression for the above `if`. + if_block: &'a ast::Block, // The `then` block of the `if` statement. + else_expr: &'a ast::Expr, /* The `else` block of the `if` statement. + Note that we only work with `if` exprs that + have an `else` branch. */ + stmt_idx: usize, /* The 0-based index of the `if` statement in + the containing loop block. */ + block_stmts: &'a [ast::Stmt], // The statements of the loop block. +} + +const MSG_REDUNDANT_ELSE_BLOCK: &'static str = "This else block is redundant.\n"; + +const MSG_ELSE_BLOCK_NOT_NEEDED: &'static str = "There is no need for an explicit `else` block for this `if` expression\n"; + +const DROP_ELSE_BLOCK_AND_MERGE_MSG: &'static str = + "Consider dropping the else clause and merging the code that follows (in the loop) with the if block, like so:\n"; + +const DROP_ELSE_BLOCK_MSG: &'static str = + "Consider dropping the else clause, and moving out the code in the else block, like so:\n"; + + +fn emit_warning<'a>(ctx: &EarlyContext, + data: &'a LintData, + header: &str, + typ: LintType) { + + // snip is the whole *help* message that appears after the warning. + // message is the warning message. + // expr is the expression which the lint warning message refers to. + let (snip, message, expr) = match typ { + ContinueInsideElseBlock => { + (suggestion_snippet_for_continue_inside_else(ctx, data, header), + MSG_REDUNDANT_ELSE_BLOCK, + data.else_expr) + }, + ContinueInsideThenBlock => { + (suggestion_snippet_for_continue_inside_if(ctx, data, header), + MSG_ELSE_BLOCK_NOT_NEEDED, + data.if_expr) + } + }; + span_help_and_lint(ctx, NEEDLESS_CONTINUE, expr.span, message, + &format!("{}", snip)); +} + +fn suggestion_snippet_for_continue_inside_if<'a>(ctx: &EarlyContext, + data: &'a LintData, + header: &str) -> String { + let cond_code = &snippet(ctx, data.if_cond.span, "..").into_owned(); + + let if_code = &format!("if {} {{\n continue;\n}}\n", cond_code); + /* ^^^^--- Four spaces of indentation. */ + // region B + let else_code = &snippet(ctx, data.else_expr.span, "..").into_owned(); + let else_code = erode_block(&else_code); + let else_code = trim_indent(&else_code, false); + + let mut ret = String::from(header); + ret.push_str(&if_code); + ret.push_str(&else_code); + ret.push_str("\n..."); + ret +} + +fn suggestion_snippet_for_continue_inside_else<'a>(ctx: &EarlyContext, + data: &'a LintData, + header: &str) -> String +{ + let cond_code = &snippet(ctx, data.if_cond.span, "..").into_owned(); + let mut if_code = format!("if {} {{\n", cond_code); + + // Region B + let block_code = &snippet(ctx, data.if_block.span, "..").into_owned(); + let block_code = erode_block(block_code); + let block_code = trim_indent(&block_code, false); + let block_code = left_pad_lines_with_spaces(&block_code, 4usize); + + if_code.push_str(&block_code); + + // Region C + // These is the code in the loop block that follows the if/else construction + // we are complaining about. We want to pull all of this code into the + // `then` block of the `if` statement. + let to_annex = data.block_stmts[data.stmt_idx+1..] + .iter() + .map(|stmt| { + original_sp(ctx.sess().codemap(), stmt.span, DUMMY_SP) + }) + .map(|span| snippet_block(ctx, span, "..").into_owned()) + .collect::>().join("\n"); + + let mut ret = String::from(header); + ret.push_str(&align_snippets(&[&if_code, + "\n// Merged code follows...", + &to_annex])); + ret.push_str("\n}\n"); + ret +} + +fn check_and_warn<'a>(ctx: &EarlyContext, expr: &'a ast::Expr) { + with_loop_block(expr, |loop_block| { + for (i, stmt) in loop_block.stmts.iter().enumerate() { + with_if_expr(stmt, |if_expr, cond, then_block, else_expr| { + let data = &LintData { + stmt_idx: i, + if_expr: if_expr, + if_cond: cond, + if_block: then_block, + else_expr: else_expr, + block_stmts: &loop_block.stmts, + }; + if needless_continue_in_else(else_expr) { + emit_warning(ctx, data, DROP_ELSE_BLOCK_AND_MERGE_MSG, ContinueInsideElseBlock); + } else if is_first_block_stmt_continue(then_block) { + emit_warning(ctx, data, DROP_ELSE_BLOCK_MSG, ContinueInsideThenBlock); + } + }); + } + }); +} + +/// Eats at `s` from the end till a closing brace `}` is encountered, and then +/// continues eating till a non-whitespace character is found. +/// e.g., the string +/// +/// " +/// { +/// let x = 5; +/// } +/// " +/// +/// is transformed to +/// +/// " +/// { +/// let x = 5;" +/// +fn erode_from_back(s: &str) -> String { + let mut ret = String::from(s); + while ret.pop().map_or(false, |c| c != '}') { } + while let Some(c) = ret.pop() { + if !c.is_whitespace() { + ret.push(c); + break; + } + } + ret +} + +fn erode_from_front(s: &str) -> String { + s.chars() + .skip_while(|c| c.is_whitespace()) + .skip_while(|c| *c == '{') + .skip_while(|c| *c == '\n') + .collect::() +} + +fn erode_block(s: &str) -> String { + erode_from_back(&erode_from_front(s)) +} + +fn is_all_whitespace(s: &str) -> bool { s.chars().all(|c| c.is_whitespace()) } + +/// Returns true if a string is empty or just spaces. +fn is_null(s: &str) -> bool { s.is_empty() || is_all_whitespace(s) } + +/// Returns the indentation level of a string. It just returns the count of +/// whitespace characters in the string before a non-whitespace character +/// is encountered. +fn indent_level(s: &str) -> usize { + s.chars() + .enumerate() + .find(|&(_, c)| !c.is_whitespace()) + .map_or(0usize, |(i, _)| i) +} + +/// Trims indentation from a snippet such that the line with the minimum +/// indentation has no indentation after the trasformation. +fn trim_indent(s: &str, skip_first_line: bool) -> String { + let min_indent_level = s.lines() + .filter(|line| !is_null(line)) + .skip(skip_first_line as usize) + .map(indent_level) + .min() + .unwrap_or(0usize); + let ret = s.lines().map(|line| { + if is_null(line) { + String::from(line) + } else { + line.chars() + .enumerate() + .skip_while(|&(i, c)| c.is_whitespace() && i < min_indent_level) + .map(|pair| pair.1) + .collect::() + } + }).collect::>(); + ret.join("\n") +} + +/// Add `n` spaces to the left of `s`. +fn left_pad_with_spaces(s: &str, n: usize) -> String { + let mut new_s = std::iter::repeat(' '/* <-space */).take(n).collect::(); + new_s.push_str(s); + new_s +} + +/// Add `n` spaces to the left of each line in `s` and return the result +/// in a new String. +fn left_pad_lines_with_spaces(s: &str, n: usize) -> String { + s.lines() + .map(|line| left_pad_with_spaces(line, n)) + .collect::>() + .join("\n") +} + +/// Remove upto `n` whitespace characters from the beginning of `s`. +fn remove_whitespace_from_left(s: &str, n: usize) -> String { + s.chars() + .enumerate() + .skip_while(|&(i, c)| i < n && c.is_whitespace()) + .map(|(_, c)| c) + .collect::() +} + +/// Aligns two snippets such that the indentation level of the last non-empty, +/// non-space line of the first snippet matches the first non-empty, non-space +/// line of the second. +fn align_two_snippets(s: &str, t: &str) -> String { + // indent level of the last nonempty, non-whitespace line of s. + let target_ilevel = s.lines() + .rev() + .skip_while(|line| line.is_empty() || is_all_whitespace(line)) + .next() + .map_or(0usize, indent_level); + + // We want to align the first nonempty, non-all-whitespace line of t to + // have the same indent level as target_ilevel + let level = t.lines() + .skip_while(|line| line.is_empty() || is_all_whitespace(line)) + .next() + .map_or(0usize, indent_level); + + let add_or_not_remove = target_ilevel > level; /* when true, we add spaces, + otherwise eat. */ + + let delta = if add_or_not_remove { + target_ilevel - level + } else { + level - target_ilevel + }; + + let new_t = t.lines() + .filter(|line| !is_null(line)) + .map(|line| if add_or_not_remove { + left_pad_with_spaces(line, delta) + } else { + remove_whitespace_from_left(line, delta) + }) + .collect::>().join("\n"); + + format!("{}\n{}", s, new_t) +} + +fn align_snippets(xs: &[&str]) -> String { + match xs.len() { + 0 => String::from(""), + _ => { + let mut ret = String::new(); + ret.push_str(xs[0]); + for x in xs.iter().skip(1usize) { + ret = align_two_snippets(&ret, x); + } + ret + } + } +} + From 7b4a12fd212046578060378f4b4e081286891ac4 Mon Sep 17 00:00:00 2001 From: Yati Sagade Date: Sun, 21 Aug 2016 23:39:24 +0200 Subject: [PATCH 02/13] Fix lint warnings from dogfooding tests. --- clippy_lints/src/needless_continue.rs | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/clippy_lints/src/needless_continue.rs b/clippy_lints/src/needless_continue.rs index 042f45cefcf1..6a4f98f19b26 100644 --- a/clippy_lints/src/needless_continue.rs +++ b/clippy_lints/src/needless_continue.rs @@ -143,7 +143,7 @@ fn is_first_block_stmt_continue(block: &ast::Block) -> bool { ], { ret = true; }} - }).unwrap_or(()); + }); ret } @@ -230,8 +230,7 @@ fn emit_warning<'a>(ctx: &EarlyContext, data.if_expr) } }; - span_help_and_lint(ctx, NEEDLESS_CONTINUE, expr.span, message, - &format!("{}", snip)); + span_help_and_lint(ctx, NEEDLESS_CONTINUE, expr.span, message, &snip); } fn suggestion_snippet_for_continue_inside_if<'a>(ctx: &EarlyContext, @@ -242,7 +241,7 @@ fn suggestion_snippet_for_continue_inside_if<'a>(ctx: &EarlyContext, let if_code = &format!("if {} {{\n continue;\n}}\n", cond_code); /* ^^^^--- Four spaces of indentation. */ // region B - let else_code = &snippet(ctx, data.else_expr.span, "..").into_owned(); + let else_code = snippet(ctx, data.else_expr.span, "..").into_owned(); let else_code = erode_block(&else_code); let else_code = trim_indent(&else_code, false); From 9396120008745a1caf3027c6f8badaf4aef3587c Mon Sep 17 00:00:00 2001 From: Yati Sagade Date: Sun, 21 Aug 2016 23:46:17 +0200 Subject: [PATCH 03/13] More dogfood test fixes. --- clippy_lints/src/needless_continue.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/clippy_lints/src/needless_continue.rs b/clippy_lints/src/needless_continue.rs index 6a4f98f19b26..74b9386e31ce 100644 --- a/clippy_lints/src/needless_continue.rs +++ b/clippy_lints/src/needless_continue.rs @@ -238,8 +238,8 @@ fn suggestion_snippet_for_continue_inside_if<'a>(ctx: &EarlyContext, header: &str) -> String { let cond_code = &snippet(ctx, data.if_cond.span, "..").into_owned(); - let if_code = &format!("if {} {{\n continue;\n}}\n", cond_code); - /* ^^^^--- Four spaces of indentation. */ + let if_code = format!("if {} {{\n continue;\n}}\n", cond_code); + /* ^^^^--- Four spaces of indentation. */ // region B let else_code = snippet(ctx, data.else_expr.span, "..").into_owned(); let else_code = erode_block(&else_code); From 38238f576debeb035a20d88c13d37a769f9c4437 Mon Sep 17 00:00:00 2001 From: Yati Sagade Date: Sat, 31 Dec 2016 22:35:39 +0100 Subject: [PATCH 04/13] [needless_continue] Add comments explaining terminology used thoughout in the code. --- clippy_lints/src/needless_continue.rs | 170 +++++++++++++++++--------- 1 file changed, 109 insertions(+), 61 deletions(-) diff --git a/clippy_lints/src/needless_continue.rs b/clippy_lints/src/needless_continue.rs index 74b9386e31ce..c6ae0113f91f 100644 --- a/clippy_lints/src/needless_continue.rs +++ b/clippy_lints/src/needless_continue.rs @@ -33,7 +33,6 @@ use syntax::ast; use syntax::codemap::{original_sp,DUMMY_SP}; use utils::{in_macro, span_help_and_lint, snippet_block, snippet}; -use self::LintType::*; /// **What it does:** The lint checks for `if`-statements appearing in loops /// that contain a `continue` statement in either their main blocks or their @@ -117,34 +116,78 @@ impl EarlyLintPass for NeedlessContinue { } } -/// Given an Expr, returns true if either of the following is true +/* This lint has to mainly deal with two cases of needless continue statements. + * + * Case 1 [Continue inside else block]: + * + * loop { + * // region A + * if cond { + * // region B + * } else { + * continue; + * } + * // region C + * } + * + * This code can better be written as follows: + * + * loop { + * // region A + * if cond { + * // region B + * // region C + * } + * } + * + * Case 2 [Continue inside then block]: + * + * loop { + * // region A + * if cond { + * continue; + * // potentially more code here. + * } else { + * // region B + * } + * // region C + * } + * + * + * This snippet can be refactored to: + * + * loop { + * // region A + * if !cond { + * // region B + * // region C + * } + * } + */ + +/// Given an expression, returns true if either of the following is true /// -/// - The Expr is a `continue` node. -/// - The Expr node is a block with the first statement being a `continue`. +/// - The expression is a `continue` node. +/// - The expression node is a block with the first statement being a `continue`. /// fn needless_continue_in_else(else_expr: &ast::Expr) -> bool { - let mut found = false; match else_expr.node { - ast::ExprKind::Block(ref else_block) => { - found = is_first_block_stmt_continue(else_block); - }, - ast::ExprKind::Continue(_) => { found = true }, - _ => { }, - }; - found + ast::ExprKind::Block(ref else_block) => is_first_block_stmt_continue(else_block), + ast::ExprKind::Continue(_) => true, + _ => false, + } } fn is_first_block_stmt_continue(block: &ast::Block) -> bool { - let mut ret = false; - block.stmts.get(0).map(|stmt| { - if_let_chain! {[ - let ast::StmtKind::Semi(ref e) = stmt.node, - let ast::ExprKind::Continue(_) = e.node, - ], { - ret = true; - }} - }); - ret + block.stmts.get(0).map_or(false, |stmt| match stmt.node { + ast::StmtKind::Semi(ref e) | + ast::StmtKind::Expr(ref e) => if let ast::ExprKind::Continue(_) = e.node { + true + } else { + false + }, + _ => false, + }) } /// If `expr` is a loop expression (while/while let/for/loop), calls `func` with @@ -159,13 +202,13 @@ fn with_loop_block(expr: &ast::Expr, mut func: F) where F: FnMut(&ast::Block) } } -/// If `stmt` is an if expression node with an else branch, calls func with the +/// If `stmt` is an if expression node with an `else` branch, calls func with the /// following: /// -/// - The if Expr, -/// - The if condition Expr, -/// - The then block of this if Expr, and -/// - The else expr. +/// - The `if` expression itself, +/// - The `if` condition expression, +/// - The `then` block, and +/// - The `else` expression. /// fn with_if_expr(stmt: &ast::Stmt, mut func: F) where F: FnMut(&ast::Expr, &ast::Expr, &ast::Block, &ast::Expr) { @@ -188,15 +231,19 @@ enum LintType { /// Data we pass around for construction of help messages. struct LintData<'a> { - if_expr: &'a ast::Expr, // The `if` expr encountered in the above loop. - if_cond: &'a ast::Expr, // The condition expression for the above `if`. - if_block: &'a ast::Block, // The `then` block of the `if` statement. - else_expr: &'a ast::Expr, /* The `else` block of the `if` statement. - Note that we only work with `if` exprs that - have an `else` branch. */ - stmt_idx: usize, /* The 0-based index of the `if` statement in - the containing loop block. */ - block_stmts: &'a [ast::Stmt], // The statements of the loop block. + /// The `if` expression encountered in the above loop. + if_expr: &'a ast::Expr, + /// The condition expression for the above `if`. + if_cond: &'a ast::Expr, + /// The `then` block of the `if` statement. + if_block: &'a ast::Block, + /// The `else` block of the `if` statement. + /// Note that we only work with `if` exprs that have an `else` branch. + else_expr: &'a ast::Expr, + /// The 0-based index of the `if` statement in the containing loop block. + stmt_idx: usize, + /// The statements of the loop block. + block_stmts: &'a [ast::Stmt], } const MSG_REDUNDANT_ELSE_BLOCK: &'static str = "This else block is redundant.\n"; @@ -219,12 +266,12 @@ fn emit_warning<'a>(ctx: &EarlyContext, // message is the warning message. // expr is the expression which the lint warning message refers to. let (snip, message, expr) = match typ { - ContinueInsideElseBlock => { + LintType::ContinueInsideElseBlock => { (suggestion_snippet_for_continue_inside_else(ctx, data, header), MSG_REDUNDANT_ELSE_BLOCK, data.else_expr) }, - ContinueInsideThenBlock => { + LintType::ContinueInsideThenBlock => { (suggestion_snippet_for_continue_inside_if(ctx, data, header), MSG_ELSE_BLOCK_NOT_NEEDED, data.if_expr) @@ -236,7 +283,7 @@ fn emit_warning<'a>(ctx: &EarlyContext, fn suggestion_snippet_for_continue_inside_if<'a>(ctx: &EarlyContext, data: &'a LintData, header: &str) -> String { - let cond_code = &snippet(ctx, data.if_cond.span, "..").into_owned(); + let cond_code = snippet(ctx, data.if_cond.span, ".."); let if_code = format!("if {} {{\n continue;\n}}\n", cond_code); /* ^^^^--- Four spaces of indentation. */ @@ -256,14 +303,14 @@ fn suggestion_snippet_for_continue_inside_else<'a>(ctx: &EarlyContext, data: &'a LintData, header: &str) -> String { - let cond_code = &snippet(ctx, data.if_cond.span, "..").into_owned(); + let cond_code = snippet(ctx, data.if_cond.span, ".."); let mut if_code = format!("if {} {{\n", cond_code); // Region B let block_code = &snippet(ctx, data.if_block.span, "..").into_owned(); let block_code = erode_block(block_code); let block_code = trim_indent(&block_code, false); - let block_code = left_pad_lines_with_spaces(&block_code, 4usize); + let block_code = left_pad_lines_with_spaces(&block_code, 4_usize); if_code.push_str(&block_code); @@ -300,9 +347,9 @@ fn check_and_warn<'a>(ctx: &EarlyContext, expr: &'a ast::Expr) { block_stmts: &loop_block.stmts, }; if needless_continue_in_else(else_expr) { - emit_warning(ctx, data, DROP_ELSE_BLOCK_AND_MERGE_MSG, ContinueInsideElseBlock); + emit_warning(ctx, data, DROP_ELSE_BLOCK_AND_MERGE_MSG, LintType::ContinueInsideElseBlock); } else if is_first_block_stmt_continue(then_block) { - emit_warning(ctx, data, DROP_ELSE_BLOCK_MSG, ContinueInsideThenBlock); + emit_warning(ctx, data, DROP_ELSE_BLOCK_MSG, LintType::ContinueInsideThenBlock); } }); } @@ -361,7 +408,7 @@ fn indent_level(s: &str) -> usize { s.chars() .enumerate() .find(|&(_, c)| !c.is_whitespace()) - .map_or(0usize, |(i, _)| i) + .map_or(0_usize, |(i, _)| i) } /// Trims indentation from a snippet such that the line with the minimum @@ -372,7 +419,7 @@ fn trim_indent(s: &str, skip_first_line: bool) -> String { .skip(skip_first_line as usize) .map(indent_level) .min() - .unwrap_or(0usize); + .unwrap_or(0_usize); let ret = s.lines().map(|line| { if is_null(line) { String::from(line) @@ -421,14 +468,14 @@ fn align_two_snippets(s: &str, t: &str) -> String { .rev() .skip_while(|line| line.is_empty() || is_all_whitespace(line)) .next() - .map_or(0usize, indent_level); + .map_or(0_usize, indent_level); // We want to align the first nonempty, non-all-whitespace line of t to // have the same indent level as target_ilevel let level = t.lines() .skip_while(|line| line.is_empty() || is_all_whitespace(line)) .next() - .map_or(0usize, indent_level); + .map_or(0_usize, indent_level); let add_or_not_remove = target_ilevel > level; /* when true, we add spaces, otherwise eat. */ @@ -440,11 +487,14 @@ fn align_two_snippets(s: &str, t: &str) -> String { }; let new_t = t.lines() - .filter(|line| !is_null(line)) - .map(|line| if add_or_not_remove { - left_pad_with_spaces(line, delta) - } else { - remove_whitespace_from_left(line, delta) + .filter_map(|line| { + if is_null(line) { + None + } else if add_or_not_remove { + Some(left_pad_with_spaces(line, delta)) + } else { + Some(remove_whitespace_from_left(line, delta)) + } }) .collect::>().join("\n"); @@ -452,16 +502,14 @@ fn align_two_snippets(s: &str, t: &str) -> String { } fn align_snippets(xs: &[&str]) -> String { - match xs.len() { - 0 => String::from(""), - _ => { - let mut ret = String::new(); - ret.push_str(xs[0]); - for x in xs.iter().skip(1usize) { - ret = align_two_snippets(&ret, x); - } - ret + if xs.is_empty() { + String::from("") + } else { + let mut ret = xs[0].to_string(); + for x in xs.iter().skip(1_usize) { + ret = align_two_snippets(&ret, x); } + ret } } From 1ee34e7851362aa8e01afd06fcc824864d10e02f Mon Sep 17 00:00:00 2001 From: Yati Sagade Date: Mon, 6 Feb 2017 14:31:29 +0100 Subject: [PATCH 05/13] Run update_lints.py --- README.md | 1 + 1 file changed, 1 insertion(+) diff --git a/README.md b/README.md index 9058c901d191..f63e7412bb49 100644 --- a/README.md +++ b/README.md @@ -288,6 +288,7 @@ name [mutex_integer](https://github.com/Manishearth/rust-clippy/wiki#mutex_integer) | allow | using a mutex for an integer type [needless_bool](https://github.com/Manishearth/rust-clippy/wiki#needless_bool) | warn | if-statements with plain booleans in the then- and else-clause, e.g. `if p { true } else { false }` [needless_borrow](https://github.com/Manishearth/rust-clippy/wiki#needless_borrow) | warn | taking a reference that is going to be automatically dereferenced +[needless_continue](https://github.com/Manishearth/rust-clippy/wiki#needless_continue) | warn | `continue` statements that can be replaced by a rearrangement of code [needless_lifetimes](https://github.com/Manishearth/rust-clippy/wiki#needless_lifetimes) | warn | using explicit lifetimes for references in function arguments when elision rules would allow omitting them [needless_pass_by_value](https://github.com/Manishearth/rust-clippy/wiki#needless_pass_by_value) | warn | functions taking arguments by value, but not consuming them in its body [needless_range_loop](https://github.com/Manishearth/rust-clippy/wiki#needless_range_loop) | warn | for-looping over a range of indices where an iterator over items would do From 8ea6d6a8d285d8a09885ebb535051346bf22cd79 Mon Sep 17 00:00:00 2001 From: Yati Sagade Date: Sun, 9 Apr 2017 14:07:11 +0200 Subject: [PATCH 06/13] needless_continue: Refactor to use stuff from utils, and move some stuff to utils. I had my own implementation of what `trim_multiline()` seems to be doing, so I just started using `trim_multiline()`. Some other functions, like those block alignment, are general enough to be used elsewhere, so moved them to utils. --- clippy_lints/src/needless_continue.rs | 176 ++++++------------------ clippy_lints/src/utils/mod.rs | 188 ++++++++++++++++++++++++++ 2 files changed, 229 insertions(+), 135 deletions(-) diff --git a/clippy_lints/src/needless_continue.rs b/clippy_lints/src/needless_continue.rs index c6ae0113f91f..5784032b750e 100644 --- a/clippy_lints/src/needless_continue.rs +++ b/clippy_lints/src/needless_continue.rs @@ -27,12 +27,13 @@ //! ``` //! //! This lint is **warn** by default. -use std; use rustc::lint::*; use syntax::ast; use syntax::codemap::{original_sp,DUMMY_SP}; +use std::borrow::Cow; -use utils::{in_macro, span_help_and_lint, snippet_block, snippet}; +use utils::{in_macro, span_help_and_lint, snippet_block, snippet, trim_multiline, + left_pad_lines_with_spaces, align_snippets}; /// **What it does:** The lint checks for `if`-statements appearing in loops /// that contain a `continue` statement in either their main blocks or their @@ -110,7 +111,7 @@ impl LintPass for NeedlessContinue { impl EarlyLintPass for NeedlessContinue { fn check_expr(&mut self, ctx: &EarlyContext, expr: &ast::Expr) { - if !in_macro(ctx, expr.span) { + if !in_macro(expr.span) { check_and_warn(ctx, expr); } } @@ -290,7 +291,7 @@ fn suggestion_snippet_for_continue_inside_if<'a>(ctx: &EarlyContext, // region B let else_code = snippet(ctx, data.else_expr.span, "..").into_owned(); let else_code = erode_block(&else_code); - let else_code = trim_indent(&else_code, false); + let else_code = trim_multiline(Cow::from(else_code), false); let mut ret = String::from(header); ret.push_str(&if_code); @@ -309,7 +310,7 @@ fn suggestion_snippet_for_continue_inside_else<'a>(ctx: &EarlyContext, // Region B let block_code = &snippet(ctx, data.if_block.span, "..").into_owned(); let block_code = erode_block(block_code); - let block_code = trim_indent(&block_code, false); + let block_code = trim_multiline(Cow::from(block_code), false); let block_code = left_pad_lines_with_spaces(&block_code, 4_usize); if_code.push_str(&block_code); @@ -321,7 +322,7 @@ fn suggestion_snippet_for_continue_inside_else<'a>(ctx: &EarlyContext, let to_annex = data.block_stmts[data.stmt_idx+1..] .iter() .map(|stmt| { - original_sp(ctx.sess().codemap(), stmt.span, DUMMY_SP) + original_sp(stmt.span, DUMMY_SP) }) .map(|span| snippet_block(ctx, span, "..").into_owned()) .collect::>().join("\n"); @@ -360,19 +361,21 @@ fn check_and_warn<'a>(ctx: &EarlyContext, expr: &'a ast::Expr) { /// continues eating till a non-whitespace character is found. /// e.g., the string /// -/// " -/// { -/// let x = 5; -/// } -/// " +/// " +/// { +/// let x = 5; +/// } +/// " /// /// is transformed to /// -/// " -/// { -/// let x = 5;" +/// " +/// { +/// let x = 5;" /// -fn erode_from_back(s: &str) -> String { +/// NOTE: when there is no closing brace in `s`, `s` is _not_ preserved, i.e., +/// an empty string will be returned in that case. +pub fn erode_from_back(s: &str) -> String { let mut ret = String::from(s); while ret.pop().map_or(false, |c| c != '}') { } while let Some(c) = ret.pop() { @@ -384,7 +387,25 @@ fn erode_from_back(s: &str) -> String { ret } -fn erode_from_front(s: &str) -> String { +/// Eats at `s` from the front by first skipping all leading whitespace. Then, +/// any number of opening braces are eaten, followed by any number of newlines. +/// e.g., the string +/// +/// " +/// { +/// something(); +/// inside_a_block(); +/// } +/// " +/// +/// is transformed to +/// +/// " something(); +/// inside_a_block(); +/// } +/// " +/// +pub fn erode_from_front(s: &str) -> String { s.chars() .skip_while(|c| c.is_whitespace()) .skip_while(|c| *c == '{') @@ -392,124 +413,9 @@ fn erode_from_front(s: &str) -> String { .collect::() } -fn erode_block(s: &str) -> String { +/// If `s` contains the code for a block, delimited by braces, this function +/// tries to get the contents of the block. If there is no closing brace present, +/// an empty string is returned. +pub fn erode_block(s: &str) -> String { erode_from_back(&erode_from_front(s)) } - -fn is_all_whitespace(s: &str) -> bool { s.chars().all(|c| c.is_whitespace()) } - -/// Returns true if a string is empty or just spaces. -fn is_null(s: &str) -> bool { s.is_empty() || is_all_whitespace(s) } - -/// Returns the indentation level of a string. It just returns the count of -/// whitespace characters in the string before a non-whitespace character -/// is encountered. -fn indent_level(s: &str) -> usize { - s.chars() - .enumerate() - .find(|&(_, c)| !c.is_whitespace()) - .map_or(0_usize, |(i, _)| i) -} - -/// Trims indentation from a snippet such that the line with the minimum -/// indentation has no indentation after the trasformation. -fn trim_indent(s: &str, skip_first_line: bool) -> String { - let min_indent_level = s.lines() - .filter(|line| !is_null(line)) - .skip(skip_first_line as usize) - .map(indent_level) - .min() - .unwrap_or(0_usize); - let ret = s.lines().map(|line| { - if is_null(line) { - String::from(line) - } else { - line.chars() - .enumerate() - .skip_while(|&(i, c)| c.is_whitespace() && i < min_indent_level) - .map(|pair| pair.1) - .collect::() - } - }).collect::>(); - ret.join("\n") -} - -/// Add `n` spaces to the left of `s`. -fn left_pad_with_spaces(s: &str, n: usize) -> String { - let mut new_s = std::iter::repeat(' '/* <-space */).take(n).collect::(); - new_s.push_str(s); - new_s -} - -/// Add `n` spaces to the left of each line in `s` and return the result -/// in a new String. -fn left_pad_lines_with_spaces(s: &str, n: usize) -> String { - s.lines() - .map(|line| left_pad_with_spaces(line, n)) - .collect::>() - .join("\n") -} - -/// Remove upto `n` whitespace characters from the beginning of `s`. -fn remove_whitespace_from_left(s: &str, n: usize) -> String { - s.chars() - .enumerate() - .skip_while(|&(i, c)| i < n && c.is_whitespace()) - .map(|(_, c)| c) - .collect::() -} - -/// Aligns two snippets such that the indentation level of the last non-empty, -/// non-space line of the first snippet matches the first non-empty, non-space -/// line of the second. -fn align_two_snippets(s: &str, t: &str) -> String { - // indent level of the last nonempty, non-whitespace line of s. - let target_ilevel = s.lines() - .rev() - .skip_while(|line| line.is_empty() || is_all_whitespace(line)) - .next() - .map_or(0_usize, indent_level); - - // We want to align the first nonempty, non-all-whitespace line of t to - // have the same indent level as target_ilevel - let level = t.lines() - .skip_while(|line| line.is_empty() || is_all_whitespace(line)) - .next() - .map_or(0_usize, indent_level); - - let add_or_not_remove = target_ilevel > level; /* when true, we add spaces, - otherwise eat. */ - - let delta = if add_or_not_remove { - target_ilevel - level - } else { - level - target_ilevel - }; - - let new_t = t.lines() - .filter_map(|line| { - if is_null(line) { - None - } else if add_or_not_remove { - Some(left_pad_with_spaces(line, delta)) - } else { - Some(remove_whitespace_from_left(line, delta)) - } - }) - .collect::>().join("\n"); - - format!("{}\n{}", s, new_t) -} - -fn align_snippets(xs: &[&str]) -> String { - if xs.is_empty() { - String::from("") - } else { - let mut ret = xs[0].to_string(); - for x in xs.iter().skip(1_usize) { - ret = align_two_snippets(&ret, x); - } - ret - } -} - diff --git a/clippy_lints/src/utils/mod.rs b/clippy_lints/src/utils/mod.rs index db595b079699..2e35140cdf55 100644 --- a/clippy_lints/src/utils/mod.rs +++ b/clippy_lints/src/utils/mod.rs @@ -22,6 +22,7 @@ use syntax::codemap::{ExpnFormat, ExpnInfo, MultiSpan, Span, DUMMY_SP}; use syntax::errors::DiagnosticBuilder; use syntax::ptr::P; use syntax::symbol::keywords; +use std::iter; pub mod comparisons; pub mod conf; @@ -978,3 +979,190 @@ pub fn type_size<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, ty: ty::Ty<'tcx>) -> Opti .infer_ctxt((), Reveal::All) .enter(|infcx| ty.layout(&infcx).ok().map(|lay| lay.size(&TargetDataLayout::parse(cx.sess())).bytes())) } + +/// Add `n` spaces to the left of `s`. +pub fn left_pad_with_spaces(s: &str, n: usize) -> String { + let mut new_s = iter::repeat(' ').take(n).collect::(); + new_s.push_str(s); + new_s +} + +/// Add `n` spaces to the left of each line in `s` and return the result +/// in a new String. +/// e.g., when n = 2, the string +/// +/// " fn foo() { +/// bar() +/// }" +/// +/// becomes +/// +/// " fn foo() { +/// bar() +/// }" +/// +/// # Example +/// +/// ``` +/// use clippy_lints::utils::left_pad_with_spaces; +/// +/// let input = "\ +/// fn main() { +/// println!("hello world!"); +/// }"; +/// +/// let expected = +/// " fn main() { +/// println!("hello world!"); +/// }"; +/// +/// assert_eq!(expected, input); +/// ``` +pub fn left_pad_lines_with_spaces(s: &str, n: usize) -> String { + s.lines() + .map(|line| left_pad_with_spaces(line, n)) + .collect::>() + .join("\n") +} + +/// Remove upto `n` whitespace characters from the beginning of `s`. +/// +/// # Examples +/// +/// ``` +/// let s = " foobar "; +/// assert_eq!("foobar ", remove_whitespace_from_left(s, 100)); +/// assert_eq!(" foobar ", remove_whitespace_from_left(s, 2)); +/// assert_eq("", remove_whitespace_from_left(" ", 50)); +/// ``` +pub fn remove_whitespace_from_left(s: &str, n: usize) -> String { + s.chars() + .enumerate() + .skip_while(|&(i, c)| i < n && c.is_whitespace()) + .map(|(_, c)| c) + .collect::() +} + +/// Aligns two snippets such that the indentation level of the last non-empty, +/// non-space line of the first snippet matches the first non-empty, non-space +/// line of the second. +pub fn align_two_snippets(s: &str, t: &str) -> String { + // indent level of the last nonempty, non-whitespace line of s. + let target_ilevel = s.lines() + .rev() + .skip_while(|line| line.is_empty() || is_all_whitespace(line)) + .next() + .map_or(0_usize, indent_level); + + // We want to align the first nonempty, non-all-whitespace line of t to + // have the same indent level as target_ilevel + let level = t.lines() + .skip_while(|line| line.is_empty() || is_all_whitespace(line)) + .next() + .map_or(0_usize, indent_level); + + // When add_spaces=true, we add spaces, otherwise eat. + let add_spaces = target_ilevel > level; + + let delta = if add_spaces { + target_ilevel - level + } else { + level - target_ilevel + }; + + let new_t = t.lines() + .map(|line| { + if is_null(line) { + // leave empty lines alone + String::from(line) + } else if add_spaces { + left_pad_with_spaces(line, delta) + } else { + remove_whitespace_from_left(line, delta) + } + }) + .collect::>().join("\n"); + + format!("{}\n{}", s, new_t) +} + +/// Aligns strings in `xs` pairwise from the start, such that for any pair of +/// strings, the first string's last line is aligned with the first line of +/// the second string. See `align_two_snippets`. Use this to merge code regions +/// into a reasonably aligned chunk of code. +/// +/// For example, consider +/// +/// let s1 = "\ +/// if (condition()) { +/// do_something()"; +/// +/// let s2 = "\ +/// code_from_somewhere_else();" +/// +/// let s3 = "\ +/// another_piece_of_code(); +/// indented_here();"; +/// +/// +/// +/// +/// Now calling `align_snippets(&[s1, s2, s3])` will yield the following: +/// +/// "\ +/// if (condition()) { +/// do_something(); +/// code_from_somewhere_else(); +/// another_piece_of_code(); +/// indented_here();" +pub fn align_snippets(xs: &[&str]) -> String { + if xs.is_empty() { + String::from("") + } else { + let mut ret = xs[0].to_string(); + for x in xs.iter().skip(1_usize) { + ret = align_two_snippets(&ret, x); + } + ret + } +} + + +/// # Examples +/// ``` +/// use clippy_lints::utils::is_all_whitespace; +/// +/// assert_eq!(true, " \n\t "); +/// assert_eq!(false, ""); +/// assert_eq!(false, "hello world!\n"); +/// ``` +pub fn is_all_whitespace(s: &str) -> bool { + s.chars().all(|c| c.is_whitespace()) +} + +/// Returns true if a string is empty or just spaces. +pub fn is_null(s: &str) -> bool { + s.is_empty() || is_all_whitespace(s) +} + +/// Returns the indentation level of a string. It just returns the count of +/// whitespace characters in the string before a non-whitespace character +/// is encountered. +/// +/// # Examples +/// +/// ``` +/// use clippy_lints::utils::indent_level; +/// +/// let s = " fn foo() { "; +/// assert_eq!(4, indent_level(s)); +/// +/// let s = "fn foo() { "; +/// assert_eq!(0, indent_level(s)); +/// ``` +pub fn indent_level(s: &str) -> usize { + s.chars() + .enumerate() + .find(|&(_, c)| !c.is_whitespace()) + .map_or(0_usize, |(i, _)| i) +} From a0997481133e02ec0034991e913d69b8b21bba64 Mon Sep 17 00:00:00 2001 From: Yati Sagade Date: Sun, 9 Apr 2017 14:09:54 +0200 Subject: [PATCH 07/13] utils: Add tests for the `align_blocks` helper in utils. --- tests/test_align_snippets.rs | 74 ++++++++++++++++++++++++++++++++++++ 1 file changed, 74 insertions(+) create mode 100644 tests/test_align_snippets.rs diff --git a/tests/test_align_snippets.rs b/tests/test_align_snippets.rs new file mode 100644 index 000000000000..f6a80036b351 --- /dev/null +++ b/tests/test_align_snippets.rs @@ -0,0 +1,74 @@ +extern crate clippy_lints; + +use clippy_lints::utils::align_snippets; + +#[test] +fn test_align_snippets_single_line() { + assert_eq!("", align_snippets(&[""])); + assert_eq!("...", align_snippets(&["..."])); +} + +#[test] +#[cfg_attr(rustfmt, rustfmt_skip)] +fn test_align_snippets_multiline() { + let expected = "\ +if condition() { + do_something(); + do_another_thing(); + yet_another_thing(); + { + and_then_an_indented_block(); + } + and_then_something_the_user_indented();"; // expected + + let input = &[ +"\ +if condition() { + do_something();", +" do_another_thing();", +" yet_another_thing(); + { + and_then_an_indented_block(); + } + and_then_something_the_user_indented();", + ]; // input + + let got = align_snippets(input); + assert_eq!(expected, got); + +} + +#[test] +#[cfg_attr(rustfmt, rustfmt_skip)] +fn test_align_snippets_multiline_with_empty_lines() { + let expected = "\ +if condition() { + do_something(); + do_another_thing(); + yet_another_thing(); + { + + and_then_an_indented_block(); + } + + and_then_something_the_user_indented();"; // expected + + let input = &[ +"\ +if condition() { + do_something();", +" do_another_thing();", +" yet_another_thing(); + { + + and_then_an_indented_block(); + } + + and_then_something_the_user_indented();", + ]; // input + + let got = align_snippets(input); + println!("Input: {}\nExpected: {}\nGot: {}", input.join("\n"), &expected, &got); + assert_eq!(expected, got); +} + From 69928906f490e52aed6497f9b8f8e5a3ad924477 Mon Sep 17 00:00:00 2001 From: Yati Sagade Date: Sun, 9 Apr 2017 14:12:01 +0200 Subject: [PATCH 08/13] needless_continue: Add tests for helper functions to the needless_continue lint. Creating a test file per function sounds a bit excessive, so just clubbing all needless_continue specific function tests into this module. --- tests/needless_continue_helpers.rs | 89 ++++++++++++++++++++++++++++++ 1 file changed, 89 insertions(+) create mode 100644 tests/needless_continue_helpers.rs diff --git a/tests/needless_continue_helpers.rs b/tests/needless_continue_helpers.rs new file mode 100644 index 000000000000..0fcef5180302 --- /dev/null +++ b/tests/needless_continue_helpers.rs @@ -0,0 +1,89 @@ +// Tests for the various helper functions used by the needless_continue +// lint that don't belong in utils. +extern crate clippy_lints; +use clippy_lints::needless_continue::{erode_from_back, erode_block, erode_from_front}; + +#[test] +#[cfg_attr(rustfmt, rustfmt_skip)] +fn test_erode_from_back() { + let input = "\ +{ + let x = 5; + let y = format!(\"{}\", 42); +}"; + + let expected = "\ +{ + let x = 5; + let y = format!(\"{}\", 42);"; + + let got = erode_from_back(input); + assert_eq!(expected, got); +} + +#[test] +#[cfg_attr(rustfmt, rustfmt_skip)] +fn test_erode_from_back_no_brace() { + let input = "\ +let x = 5; +let y = something(); +"; + let expected = ""; + let got = erode_from_back(input); + assert_eq!(expected, got); +} + +#[test] +#[cfg_attr(rustfmt, rustfmt_skip)] +fn test_erode_from_front() { + let input = " + { + something(); + inside_a_block(); + } + "; + let expected = +" something(); + inside_a_block(); + } + "; + let got = erode_from_front(input); + println!("input: {}\nexpected:\n{}\ngot:\n{}", input, expected, got); + assert_eq!(expected, got); +} + +#[test] +#[cfg_attr(rustfmt, rustfmt_skip)] +fn test_erode_from_front_no_brace() { + let input = " + something(); + inside_a_block(); + "; + let expected = +"something(); + inside_a_block(); + "; + let got = erode_from_front(input); + println!("input: {}\nexpected:\n{}\ngot:\n{}", input, expected, got); + assert_eq!(expected, got); +} + + +#[test] +#[cfg_attr(rustfmt, rustfmt_skip)] +fn test_erode_block() { + + let input = " + { + something(); + inside_a_block(); + } + "; + let expected = +" something(); + inside_a_block();"; + let got = erode_block(input); + println!("input: {}\nexpected:\n{}\ngot:\n{}", input, expected, got); + assert_eq!(expected, got); +} + From 62548f447c5569bff20ce77ba7f29a7d02e6df6b Mon Sep 17 00:00:00 2001 From: Yati Sagade Date: Sun, 9 Apr 2017 14:20:14 +0200 Subject: [PATCH 09/13] needless_continue: Add ui test The test program contains both conditions tested by the lint, i.e., a redundant continue in the `if` and `else` blocks within a loop. Maybe splitting them out and also having a program that should *not* trigger the lint warning is better. --- tests/ui/needless_continue.rs | 50 +++++++++++++++++++++++ tests/ui/needless_continue.stderr | 66 +++++++++++++++++++++++++++++++ 2 files changed, 116 insertions(+) create mode 100644 tests/ui/needless_continue.rs create mode 100644 tests/ui/needless_continue.stderr diff --git a/tests/ui/needless_continue.rs b/tests/ui/needless_continue.rs new file mode 100644 index 000000000000..b6f9513be448 --- /dev/null +++ b/tests/ui/needless_continue.rs @@ -0,0 +1,50 @@ +#![feature(plugin)] +#![plugin(clippy)] + +macro_rules! zero { + ($x:expr) => ($x == 0); +} + +macro_rules! nonzero { + ($x:expr) => (!zero!($x)); +} + +#[deny(needless_continue)] +fn main() { + let mut i = 1; + while i < 10 { + i += 1; + + if i % 2 == 0 && i % 3 == 0 { + println!("{}", i); + println!("{}", i+1); + if i % 5 == 0 { + println!("{}", i+2); + } + let i = 0; + println!("bar {} ", i); + } else { + continue; + } + + println!("bleh"); + { + println!("blah"); + } + + // some comments that also should ideally be included in the + // output of the lint suggestion if possible. + if !(!(i == 2) || !(i == 5)) { + println!("lama"); + } + + if (zero!(i % 2) || nonzero!(i % 5)) && i % 3 != 0 { + continue; + } else { + println!("Blabber"); + println!("Jabber"); + } + + println!("bleh"); + } +} diff --git a/tests/ui/needless_continue.stderr b/tests/ui/needless_continue.stderr new file mode 100644 index 000000000000..e8433ef9f747 --- /dev/null +++ b/tests/ui/needless_continue.stderr @@ -0,0 +1,66 @@ +error: This else block is redundant. + + --> $DIR/needless_continue.rs:26:16 + | +26 | } else { + | ________________^ starting here... +27 | | continue; +28 | | } + | |_________^ ...ending here + | +note: lint level defined here + --> $DIR/needless_continue.rs:12:8 + | +12 | #[deny(needless_continue)] + | ^^^^^^^^^^^^^^^^^ + = help: Consider dropping the else clause and merging the code that follows (in the loop) with the if block, like so: + if i % 2 == 0 && i % 3 == 0 { + println!("{}", i); + println!("{}", i+1); + if i % 5 == 0 { + println!("{}", i+2); + } + let i = 0; + println!("bar {} ", i); + + // Merged code follows... + println!("bleh"); + { + println!("blah"); + } + if !(!(i == 2) || !(i == 5)) { + println!("lama"); + } + if (zero!(i % 2) || nonzero!(i % 5)) && i % 3 != 0 { + continue; + } else { + println!("Blabber"); + println!("Jabber"); + } + println!("bleh"); + } + + +error: There is no need for an explicit `else` block for this `if` expression + + --> $DIR/needless_continue.rs:41:9 + | +41 | if (zero!(i % 2) || nonzero!(i % 5)) && i % 3 != 0 { + | _________^ starting here... +42 | | continue; +43 | | } else { +44 | | println!("Blabber"); +45 | | println!("Jabber"); +46 | | } + | |_________^ ...ending here + | + = help: Consider dropping the else clause, and moving out the code in the else block, like so: + if (zero!(i % 2) || nonzero!(i % 5)) && i % 3 != 0 { + continue; + } + println!("Blabber"); + println!("Jabber"); + ... + +error: aborting due to 2 previous errors + From 3534149035ea6608bf7f8f60c8ca4dce41934611 Mon Sep 17 00:00:00 2001 From: Oliver Schneider Date: Tue, 11 Apr 2017 16:19:27 +0200 Subject: [PATCH 10/13] Update ui tests --- tests/ui/copies.stderr | 40 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 40 insertions(+) diff --git a/tests/ui/copies.stderr b/tests/ui/copies.stderr index 315ff79ca49c..d67c0e52527d 100644 --- a/tests/ui/copies.stderr +++ b/tests/ui/copies.stderr @@ -1,3 +1,43 @@ +warning: This else block is redundant. + + --> $DIR/copies.rs:128:20 + | +128 | } else { + | ____________________^ starting here... +129 | | continue; +130 | | } + | |_____________^ ...ending here + | + = note: #[warn(needless_continue)] on by default + = help: Consider dropping the else clause and merging the code that follows (in the loop) with the if block, like so: + if true { + break; + + // Merged code follows... + + } + + +warning: This else block is redundant. + + --> $DIR/copies.rs:138:20 + | +138 | } else { + | ____________________^ starting here... +139 | | continue; +140 | | } + | |_____________^ ...ending here + | + = note: #[warn(needless_continue)] on by default + = help: Consider dropping the else clause and merging the code that follows (in the loop) with the if block, like so: + if true { + break; + + // Merged code follows... + + } + + error: this `if` has identical blocks --> $DIR/copies.rs:40:10 | From 7ee0d4f9c249da80ceb92c5878bd00f8764ba921 Mon Sep 17 00:00:00 2001 From: Oliver Schneider Date: Wed, 12 Apr 2017 10:55:34 +0200 Subject: [PATCH 11/13] Dogfood tests --- clippy_lints/src/needless_continue.rs | 18 +++++++++++------- clippy_lints/src/utils/mod.rs | 8 +++++--- 2 files changed, 16 insertions(+), 10 deletions(-) diff --git a/clippy_lints/src/needless_continue.rs b/clippy_lints/src/needless_continue.rs index 5784032b750e..9b248b894a05 100644 --- a/clippy_lints/src/needless_continue.rs +++ b/clippy_lints/src/needless_continue.rs @@ -225,6 +225,7 @@ fn with_if_expr(stmt: &ast::Stmt, mut func: F) } /// A type to distinguish between the two distinct cases this lint handles. +#[derive(Copy, Clone, Debug)] enum LintType { ContinueInsideElseBlock, ContinueInsideThenBlock, @@ -361,17 +362,18 @@ fn check_and_warn<'a>(ctx: &EarlyContext, expr: &'a ast::Expr) { /// continues eating till a non-whitespace character is found. /// e.g., the string /// -/// " +/// ``` /// { /// let x = 5; /// } -/// " +/// ``` /// /// is transformed to /// -/// " +/// ``` /// { /// let x = 5;" +/// ``` /// /// NOTE: when there is no closing brace in `s`, `s` is _not_ preserved, i.e., /// an empty string will be returned in that case. @@ -391,19 +393,21 @@ pub fn erode_from_back(s: &str) -> String { /// any number of opening braces are eaten, followed by any number of newlines. /// e.g., the string /// -/// " +/// ``` /// { /// something(); /// inside_a_block(); /// } -/// " +/// ``` /// /// is transformed to /// -/// " something(); +/// ``` +/// something(); /// inside_a_block(); /// } -/// " +/// +/// ``` /// pub fn erode_from_front(s: &str) -> String { s.chars() diff --git a/clippy_lints/src/utils/mod.rs b/clippy_lints/src/utils/mod.rs index 2e35140cdf55..5782d3ef3a8a 100644 --- a/clippy_lints/src/utils/mod.rs +++ b/clippy_lints/src/utils/mod.rs @@ -1093,6 +1093,7 @@ pub fn align_two_snippets(s: &str, t: &str) -> String { /// /// For example, consider /// +/// ``` /// let s1 = "\ /// if (condition()) { /// do_something()"; @@ -1103,18 +1104,19 @@ pub fn align_two_snippets(s: &str, t: &str) -> String { /// let s3 = "\ /// another_piece_of_code(); /// indented_here();"; -/// +/// ``` /// /// /// /// Now calling `align_snippets(&[s1, s2, s3])` will yield the following: /// -/// "\ +/// ``` /// if (condition()) { /// do_something(); /// code_from_somewhere_else(); /// another_piece_of_code(); -/// indented_here();" +/// indented_here(); +/// ``` pub fn align_snippets(xs: &[&str]) -> String { if xs.is_empty() { String::from("") From cac15d24f9342a8eee2e7446ade2990c9414a8a1 Mon Sep 17 00:00:00 2001 From: Yati Sagade Date: Thu, 13 Apr 2017 21:48:52 +0200 Subject: [PATCH 12/13] needless_continue: Remove indentation of suggestion code. As per a suggestion by Oliver on the PR thread, maintaining indentation in the suggested code is futile because of the changes in the compiler and the messiness of real-world code. rustfmt will do the indentation if required, so we don't need to do it. --- clippy_lints/src/needless_continue.rs | 11 +- clippy_lints/src/utils/mod.rs | 189 -------------------------- tests/test_align_snippets.rs | 74 ---------- tests/ui/needless_continue.stderr | 44 +++--- 4 files changed, 26 insertions(+), 292 deletions(-) delete mode 100644 tests/test_align_snippets.rs diff --git a/clippy_lints/src/needless_continue.rs b/clippy_lints/src/needless_continue.rs index 9b248b894a05..1e7120951cab 100644 --- a/clippy_lints/src/needless_continue.rs +++ b/clippy_lints/src/needless_continue.rs @@ -32,8 +32,7 @@ use syntax::ast; use syntax::codemap::{original_sp,DUMMY_SP}; use std::borrow::Cow; -use utils::{in_macro, span_help_and_lint, snippet_block, snippet, trim_multiline, - left_pad_lines_with_spaces, align_snippets}; +use utils::{in_macro, span_help_and_lint, snippet_block, snippet, trim_multiline}; /// **What it does:** The lint checks for `if`-statements appearing in loops /// that contain a `continue` statement in either their main blocks or their @@ -312,7 +311,6 @@ fn suggestion_snippet_for_continue_inside_else<'a>(ctx: &EarlyContext, let block_code = &snippet(ctx, data.if_block.span, "..").into_owned(); let block_code = erode_block(block_code); let block_code = trim_multiline(Cow::from(block_code), false); - let block_code = left_pad_lines_with_spaces(&block_code, 4_usize); if_code.push_str(&block_code); @@ -329,9 +327,10 @@ fn suggestion_snippet_for_continue_inside_else<'a>(ctx: &EarlyContext, .collect::>().join("\n"); let mut ret = String::from(header); - ret.push_str(&align_snippets(&[&if_code, - "\n// Merged code follows...", - &to_annex])); + + ret.push_str(&if_code); + ret.push_str("\n// Merged code follows..."); + ret.push_str(&to_annex); ret.push_str("\n}\n"); ret } diff --git a/clippy_lints/src/utils/mod.rs b/clippy_lints/src/utils/mod.rs index 5782d3ef3a8a..40dab908614f 100644 --- a/clippy_lints/src/utils/mod.rs +++ b/clippy_lints/src/utils/mod.rs @@ -22,7 +22,6 @@ use syntax::codemap::{ExpnFormat, ExpnInfo, MultiSpan, Span, DUMMY_SP}; use syntax::errors::DiagnosticBuilder; use syntax::ptr::P; use syntax::symbol::keywords; -use std::iter; pub mod comparisons; pub mod conf; @@ -980,191 +979,3 @@ pub fn type_size<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, ty: ty::Ty<'tcx>) -> Opti .enter(|infcx| ty.layout(&infcx).ok().map(|lay| lay.size(&TargetDataLayout::parse(cx.sess())).bytes())) } -/// Add `n` spaces to the left of `s`. -pub fn left_pad_with_spaces(s: &str, n: usize) -> String { - let mut new_s = iter::repeat(' ').take(n).collect::(); - new_s.push_str(s); - new_s -} - -/// Add `n` spaces to the left of each line in `s` and return the result -/// in a new String. -/// e.g., when n = 2, the string -/// -/// " fn foo() { -/// bar() -/// }" -/// -/// becomes -/// -/// " fn foo() { -/// bar() -/// }" -/// -/// # Example -/// -/// ``` -/// use clippy_lints::utils::left_pad_with_spaces; -/// -/// let input = "\ -/// fn main() { -/// println!("hello world!"); -/// }"; -/// -/// let expected = -/// " fn main() { -/// println!("hello world!"); -/// }"; -/// -/// assert_eq!(expected, input); -/// ``` -pub fn left_pad_lines_with_spaces(s: &str, n: usize) -> String { - s.lines() - .map(|line| left_pad_with_spaces(line, n)) - .collect::>() - .join("\n") -} - -/// Remove upto `n` whitespace characters from the beginning of `s`. -/// -/// # Examples -/// -/// ``` -/// let s = " foobar "; -/// assert_eq!("foobar ", remove_whitespace_from_left(s, 100)); -/// assert_eq!(" foobar ", remove_whitespace_from_left(s, 2)); -/// assert_eq("", remove_whitespace_from_left(" ", 50)); -/// ``` -pub fn remove_whitespace_from_left(s: &str, n: usize) -> String { - s.chars() - .enumerate() - .skip_while(|&(i, c)| i < n && c.is_whitespace()) - .map(|(_, c)| c) - .collect::() -} - -/// Aligns two snippets such that the indentation level of the last non-empty, -/// non-space line of the first snippet matches the first non-empty, non-space -/// line of the second. -pub fn align_two_snippets(s: &str, t: &str) -> String { - // indent level of the last nonempty, non-whitespace line of s. - let target_ilevel = s.lines() - .rev() - .skip_while(|line| line.is_empty() || is_all_whitespace(line)) - .next() - .map_or(0_usize, indent_level); - - // We want to align the first nonempty, non-all-whitespace line of t to - // have the same indent level as target_ilevel - let level = t.lines() - .skip_while(|line| line.is_empty() || is_all_whitespace(line)) - .next() - .map_or(0_usize, indent_level); - - // When add_spaces=true, we add spaces, otherwise eat. - let add_spaces = target_ilevel > level; - - let delta = if add_spaces { - target_ilevel - level - } else { - level - target_ilevel - }; - - let new_t = t.lines() - .map(|line| { - if is_null(line) { - // leave empty lines alone - String::from(line) - } else if add_spaces { - left_pad_with_spaces(line, delta) - } else { - remove_whitespace_from_left(line, delta) - } - }) - .collect::>().join("\n"); - - format!("{}\n{}", s, new_t) -} - -/// Aligns strings in `xs` pairwise from the start, such that for any pair of -/// strings, the first string's last line is aligned with the first line of -/// the second string. See `align_two_snippets`. Use this to merge code regions -/// into a reasonably aligned chunk of code. -/// -/// For example, consider -/// -/// ``` -/// let s1 = "\ -/// if (condition()) { -/// do_something()"; -/// -/// let s2 = "\ -/// code_from_somewhere_else();" -/// -/// let s3 = "\ -/// another_piece_of_code(); -/// indented_here();"; -/// ``` -/// -/// -/// -/// Now calling `align_snippets(&[s1, s2, s3])` will yield the following: -/// -/// ``` -/// if (condition()) { -/// do_something(); -/// code_from_somewhere_else(); -/// another_piece_of_code(); -/// indented_here(); -/// ``` -pub fn align_snippets(xs: &[&str]) -> String { - if xs.is_empty() { - String::from("") - } else { - let mut ret = xs[0].to_string(); - for x in xs.iter().skip(1_usize) { - ret = align_two_snippets(&ret, x); - } - ret - } -} - - -/// # Examples -/// ``` -/// use clippy_lints::utils::is_all_whitespace; -/// -/// assert_eq!(true, " \n\t "); -/// assert_eq!(false, ""); -/// assert_eq!(false, "hello world!\n"); -/// ``` -pub fn is_all_whitespace(s: &str) -> bool { - s.chars().all(|c| c.is_whitespace()) -} - -/// Returns true if a string is empty or just spaces. -pub fn is_null(s: &str) -> bool { - s.is_empty() || is_all_whitespace(s) -} - -/// Returns the indentation level of a string. It just returns the count of -/// whitespace characters in the string before a non-whitespace character -/// is encountered. -/// -/// # Examples -/// -/// ``` -/// use clippy_lints::utils::indent_level; -/// -/// let s = " fn foo() { "; -/// assert_eq!(4, indent_level(s)); -/// -/// let s = "fn foo() { "; -/// assert_eq!(0, indent_level(s)); -/// ``` -pub fn indent_level(s: &str) -> usize { - s.chars() - .enumerate() - .find(|&(_, c)| !c.is_whitespace()) - .map_or(0_usize, |(i, _)| i) -} diff --git a/tests/test_align_snippets.rs b/tests/test_align_snippets.rs deleted file mode 100644 index f6a80036b351..000000000000 --- a/tests/test_align_snippets.rs +++ /dev/null @@ -1,74 +0,0 @@ -extern crate clippy_lints; - -use clippy_lints::utils::align_snippets; - -#[test] -fn test_align_snippets_single_line() { - assert_eq!("", align_snippets(&[""])); - assert_eq!("...", align_snippets(&["..."])); -} - -#[test] -#[cfg_attr(rustfmt, rustfmt_skip)] -fn test_align_snippets_multiline() { - let expected = "\ -if condition() { - do_something(); - do_another_thing(); - yet_another_thing(); - { - and_then_an_indented_block(); - } - and_then_something_the_user_indented();"; // expected - - let input = &[ -"\ -if condition() { - do_something();", -" do_another_thing();", -" yet_another_thing(); - { - and_then_an_indented_block(); - } - and_then_something_the_user_indented();", - ]; // input - - let got = align_snippets(input); - assert_eq!(expected, got); - -} - -#[test] -#[cfg_attr(rustfmt, rustfmt_skip)] -fn test_align_snippets_multiline_with_empty_lines() { - let expected = "\ -if condition() { - do_something(); - do_another_thing(); - yet_another_thing(); - { - - and_then_an_indented_block(); - } - - and_then_something_the_user_indented();"; // expected - - let input = &[ -"\ -if condition() { - do_something();", -" do_another_thing();", -" yet_another_thing(); - { - - and_then_an_indented_block(); - } - - and_then_something_the_user_indented();", - ]; // input - - let got = align_snippets(input); - println!("Input: {}\nExpected: {}\nGot: {}", input.join("\n"), &expected, &got); - assert_eq!(expected, got); -} - diff --git a/tests/ui/needless_continue.stderr b/tests/ui/needless_continue.stderr index e8433ef9f747..5a4447aa6940 100644 --- a/tests/ui/needless_continue.stderr +++ b/tests/ui/needless_continue.stderr @@ -15,29 +15,27 @@ note: lint level defined here | ^^^^^^^^^^^^^^^^^ = help: Consider dropping the else clause and merging the code that follows (in the loop) with the if block, like so: if i % 2 == 0 && i % 3 == 0 { - println!("{}", i); - println!("{}", i+1); - if i % 5 == 0 { - println!("{}", i+2); - } - let i = 0; - println!("bar {} ", i); - - // Merged code follows... - println!("bleh"); - { - println!("blah"); - } - if !(!(i == 2) || !(i == 5)) { - println!("lama"); - } - if (zero!(i % 2) || nonzero!(i % 5)) && i % 3 != 0 { - continue; - } else { - println!("Blabber"); - println!("Jabber"); - } - println!("bleh"); + println!("{}", i); + println!("{}", i+1); + if i % 5 == 0 { + println!("{}", i+2); + } + let i = 0; + println!("bar {} ", i); + // Merged code follows...println!("bleh"); + { + println!("blah"); + } + if !(!(i == 2) || !(i == 5)) { + println!("lama"); + } + if (zero!(i % 2) || nonzero!(i % 5)) && i % 3 != 0 { + continue; + } else { + println!("Blabber"); + println!("Jabber"); + } + println!("bleh"); } From 5381c4fcf63171d1dedcf5163426b6f9410d1b03 Mon Sep 17 00:00:00 2001 From: Oliver Schneider Date: Tue, 25 Apr 2017 10:57:44 +0200 Subject: [PATCH 13/13] Update ui tests to new rustc range printing --- tests/ui/copies.stderr | 20 ++++++++------------ tests/ui/needless_continue.stderr | 9 ++++----- 2 files changed, 12 insertions(+), 17 deletions(-) diff --git a/tests/ui/copies.stderr b/tests/ui/copies.stderr index f0dd409e2b21..05c5e92a7faa 100644 --- a/tests/ui/copies.stderr +++ b/tests/ui/copies.stderr @@ -3,18 +3,16 @@ warning: This else block is redundant. --> $DIR/copies.rs:128:20 | 128 | } else { - | ____________________^ starting here... + | ____________________^ 129 | | continue; 130 | | } - | |_____________^ ...ending here + | |_____________^ | = note: #[warn(needless_continue)] on by default = help: Consider dropping the else clause and merging the code that follows (in the loop) with the if block, like so: if true { - break; - - // Merged code follows... - + break; + // Merged code follows... } @@ -23,18 +21,16 @@ warning: This else block is redundant. --> $DIR/copies.rs:138:20 | 138 | } else { - | ____________________^ starting here... + | ____________________^ 139 | | continue; 140 | | } - | |_____________^ ...ending here + | |_____________^ | = note: #[warn(needless_continue)] on by default = help: Consider dropping the else clause and merging the code that follows (in the loop) with the if block, like so: if true { - break; - - // Merged code follows... - + break; + // Merged code follows... } diff --git a/tests/ui/needless_continue.stderr b/tests/ui/needless_continue.stderr index 5a4447aa6940..50be062a67bf 100644 --- a/tests/ui/needless_continue.stderr +++ b/tests/ui/needless_continue.stderr @@ -3,10 +3,10 @@ error: This else block is redundant. --> $DIR/needless_continue.rs:26:16 | 26 | } else { - | ________________^ starting here... + | ________________^ 27 | | continue; 28 | | } - | |_________^ ...ending here + | |_________^ | note: lint level defined here --> $DIR/needless_continue.rs:12:8 @@ -43,14 +43,13 @@ error: There is no need for an explicit `else` block for this `if` expression --> $DIR/needless_continue.rs:41:9 | -41 | if (zero!(i % 2) || nonzero!(i % 5)) && i % 3 != 0 { - | _________^ starting here... +41 | / if (zero!(i % 2) || nonzero!(i % 5)) && i % 3 != 0 { 42 | | continue; 43 | | } else { 44 | | println!("Blabber"); 45 | | println!("Jabber"); 46 | | } - | |_________^ ...ending here + | |_________^ | = help: Consider dropping the else clause, and moving out the code in the else block, like so: if (zero!(i % 2) || nonzero!(i % 5)) && i % 3 != 0 {