]> git.lizzy.rs Git - rust.git/blobdiff - clippy_lints/src/needless_continue.rs
Auto merge of #3946 - rchaser53:issue-3920, r=flip1995
[rust.git] / clippy_lints / src / needless_continue.rs
index 60ab0eaae02415a7c041d869b6e4da90fe1a15ab..e37e917ddec88f41c96a84697086f13fabbc9db9 100644 (file)
@@ -2,9 +2,12 @@
 //!
 //! For example, the lint would catch
 //!
-//! ```ignore
-//! while condition() {
-//!     update_condition();
+//! ```rust
+//! let mut a = 1;
+//! let x = true;
+//!
+//! while a < 5 {
+//!     a = 6;
 //!     if x {
 //!         // ...
 //!     } else {
 //!
 //! And suggest something like this:
 //!
-//! ```ignore
-//! while condition() {
-//!     update_condition();
+//! ```rust
+//! let mut a = 1;
+//! let x = true;
+//!
+//! while a < 5 {
+//!     a = 6;
 //!     if x {
 //!         // ...
 //!         println!("Hello, world");
 //! ```
 //!
 //! This lint is **warn** by default.
-use rustc::lint::*;
-use rustc::{declare_lint, lint_array};
-use syntax::ast;
-use syntax::codemap::{original_sp, DUMMY_SP};
+use rustc::lint::{EarlyContext, EarlyLintPass, LintArray, LintPass};
+use rustc::{declare_tool_lint, lint_array};
 use std::borrow::Cow;
+use syntax::ast;
+use syntax::source_map::{original_sp, DUMMY_SP};
 
 use crate::utils::{in_macro, snippet, snippet_block, span_help_and_lint, 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_clippy_lint! {
+    /// **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
+    /// }
+    /// ```
     pub NEEDLESS_CONTINUE,
     pedantic,
     "`continue` statements that can be replaced by a rearrangement of code"
@@ -107,6 +113,10 @@ impl LintPass for NeedlessContinue {
     fn get_lints(&self) -> LintArray {
         lint_array!(NEEDLESS_CONTINUE)
     }
+
+    fn name(&self) -> &'static str {
+        "NeedlessContinue"
+    }
 }
 
 impl EarlyLintPass for NeedlessContinue {
@@ -171,37 +181,50 @@ fn check_expr(&mut self, ctx: &EarlyContext<'_>, expr: &ast::Expr) {
 /// - 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 {
+fn needless_continue_in_else(else_expr: &ast::Expr, label: Option<&ast::Label>) -> bool {
     match else_expr.node {
-        ast::ExprKind::Block(ref else_block, _) => is_first_block_stmt_continue(else_block),
-        ast::ExprKind::Continue(_) => true,
+        ast::ExprKind::Block(ref else_block, _) => is_first_block_stmt_continue(else_block, label),
+        ast::ExprKind::Continue(l) => compare_labels(label, l.as_ref()),
         _ => false,
     }
 }
 
-fn is_first_block_stmt_continue(block: &ast::Block) -> bool {
+fn is_first_block_stmt_continue(block: &ast::Block, label: Option<&ast::Label>) -> 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
+        ast::StmtKind::Semi(ref e) | ast::StmtKind::Expr(ref e) => {
+            if let ast::ExprKind::Continue(ref l) = e.node {
+                compare_labels(label, l.as_ref())
+            } else {
+                false
+            }
         },
         _ => false,
     })
 }
 
+/// If the `continue` has a label, check it matches the label of the loop.
+fn compare_labels(loop_label: Option<&ast::Label>, continue_label: Option<&ast::Label>) -> bool {
+    match (loop_label, continue_label) {
+        // `loop { continue; }` or `'a loop { continue; }`
+        (_, None) => true,
+        // `loop { continue 'a; }`
+        (None, _) => false,
+        // `'a loop { continue 'a; }` or `'a loop { continue 'b; }`
+        (Some(x), Some(y)) => x.ident == y.ident,
+    }
+}
+
 /// 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),
+    F: FnMut(&ast::Block, Option<&ast::Label>),
 {
     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),
+        ast::ExprKind::While(_, ref loop_block, ref label)
+        | ast::ExprKind::WhileLet(_, _, ref loop_block, ref label)
+        | ast::ExprKind::ForLoop(_, _, ref loop_block, ref label)
+        | ast::ExprKind::Loop(ref loop_block, ref label) => func(loop_block, label.as_ref()),
         _ => {},
     }
 }
@@ -214,7 +237,6 @@ fn with_loop_block<F>(expr: &ast::Expr, mut func: F)
 /// - 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),
@@ -264,7 +286,6 @@ struct LintData<'a> {
 const DROP_ELSE_BLOCK_MSG: &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.
@@ -284,7 +305,11 @@ fn emit_warning<'a>(ctx: &EarlyContext<'_>, data: &'a LintData<'_>, header: &str
     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 {
+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);
@@ -301,7 +326,11 @@ fn suggestion_snippet_for_continue_inside_if<'a>(ctx: &EarlyContext<'_>, data: &
     ret
 }
 
-fn suggestion_snippet_for_continue_inside_else<'a>(ctx: &EarlyContext<'_>, data: &'a LintData<'_>, header: &str) -> String {
+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);
 
@@ -333,7 +362,7 @@ fn suggestion_snippet_for_continue_inside_else<'a>(ctx: &EarlyContext<'_>, data:
 }
 
 fn check_and_warn<'a>(ctx: &EarlyContext<'_>, expr: &'a ast::Expr) {
-    with_loop_block(expr, |loop_block| {
+    with_loop_block(expr, |loop_block, label| {
         for (i, stmt) in loop_block.stmts.iter().enumerate() {
             with_if_expr(stmt, |if_expr, cond, then_block, else_expr| {
                 let data = &LintData {
@@ -344,9 +373,14 @@ fn check_and_warn<'a>(ctx: &EarlyContext<'_>, expr: &'a ast::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) {
+                if needless_continue_in_else(else_expr, label) {
+                    emit_warning(
+                        ctx,
+                        data,
+                        DROP_ELSE_BLOCK_AND_MERGE_MSG,
+                        LintType::ContinueInsideElseBlock,
+                    );
+                } else if is_first_block_stmt_continue(then_block, label) {
                     emit_warning(ctx, data, DROP_ELSE_BLOCK_MSG, LintType::ContinueInsideThenBlock);
                 }
             });
@@ -358,10 +392,10 @@ 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;
-///     }
+/// ```rust
+/// {
+///     let x = 5;
+/// }
 /// ```
 ///
 /// is transformed to
@@ -403,7 +437,6 @@ pub fn erode_from_back(s: &str) -> String {
 ///             inside_a_block();
 ///         }
 /// ```
-///
 pub fn erode_from_front(s: &str) -> String {
     s.chars()
         .skip_while(|c| c.is_whitespace())