Merge pull request #506 from yati-sagade/master

Lint for pointing out needless continue statements, fixes #111

CHANGELOG.md

@@ -417,6 +417,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

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

clippy_lints/src/lib.rs

@@ -106,6 +106,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;
@@ -218,6 +219,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);
@@ -460,6 +462,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,

clippy_lints/src/needless_continue.rs

@@ -0,0 +1,424 @@
+//! 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 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, 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
+/// `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(expr.span) {
+            check_and_warn(ctx, expr);
+        }
+    }
+}
+
+/* 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 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 {
+    match else_expr.node {
+        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 {
+    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
+/// the AST object representing the loop block of `expr`.
+fn with_loop_block<F>(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` expression itself,
+/// - The `if` condition expression,
+/// - The `then` block, and
+/// - The `else` expression.
+///
+fn with_if_expr<F>(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.
+#[derive(Copy, Clone, Debug)]
+enum LintType {
+    ContinueInsideElseBlock,
+    ContinueInsideThenBlock,
+}
+
+/// Data we pass around for construction of help messages.
+struct LintData<'a> {
+    /// 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";
+
+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 {
+        LintType::ContinueInsideElseBlock => {
+            (suggestion_snippet_for_continue_inside_else(ctx, data, header),
+             MSG_REDUNDANT_ELSE_BLOCK,
+             data.else_expr)
+        },
+        LintType::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, &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, "..");
+
+    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_multiline(Cow::from(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, "..");
+    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_multiline(Cow::from(block_code), false);
+
+    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(stmt.span, DUMMY_SP)
+                    })
+                   .map(|span| snippet_block(ctx, span, "..").into_owned())
+                   .collect::<Vec<_>>().join("\n");
+
+    let mut ret = String::from(header);
+
+    ret.push_str(&if_code);
+    ret.push_str("\n// Merged code follows...");
+    ret.push_str(&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, LintType::ContinueInsideElseBlock);
+                } else if is_first_block_stmt_continue(then_block) {
+                    emit_warning(ctx, data, DROP_ELSE_BLOCK_MSG, LintType::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;"
+/// ```
+///
+/// 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() {
+        if !c.is_whitespace() {
+            ret.push(c);
+            break;
+        }
+    }
+    ret
+}
+
+/// 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 == '{')
+     .skip_while(|c| *c == '\n')
+     .collect::<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))
+}

clippy_lints/src/utils/mod.rs

@@ -983,3 +983,4 @@ 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()))
 }
+

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);
+}
+

tests/ui/copies.stderr

@@ -1,3 +1,39 @@
+warning: This else block is redundant.
+
+   --> $DIR/copies.rs:128:20
+    |
+128 |               } else {
+    |  ____________________^
+129 | |                 continue;
+130 | |             }
+    | |_____________^
+    |
+    = 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 {
+    |  ____________________^
+139 | |                 continue;
+140 | |             }
+    | |_____________^
+    |
+    = 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
    |

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");
+    }
+}

tests/ui/needless_continue.stderr

@@ -0,0 +1,63 @@
+error: This else block is redundant.
+
+  --> $DIR/needless_continue.rs:26:16
+   |
+26 |           } else {
+   |  ________________^
+27 | |             continue;
+28 | |         }
+   | |_________^
+   |
+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 {
+42 | |             continue;
+43 | |         } else {
+44 | |             println!("Blabber");
+45 | |             println!("Jabber");
+46 | |         }
+   | |_________^
+   |
+   = 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
+