Auto merge of #4220 - d-dorazio:4182-needless-return-void-functions, …

…r=flip1995 make needless_return work with void functions fixes #4181. changelog: make needless_return work with void functions. I don't think the failure is related to my changes, but I'm not sure 🤔
rust-lang · Jun 22, 2019 · 469f73d · 469f73d
2 parents 9cae8aa + 316a9f2
commit 469f73d
Show file tree

Hide file tree

Showing 7 changed files with 143 additions and 49 deletions.
diff --git a/clippy_lints/src/invalid_ref.rs b/clippy_lints/src/invalid_ref.rs
@@ -51,6 +51,5 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for InvalidRef {
                 span_help_and_lint(cx, INVALID_REF, expr.span, msg, HELP);
             }
         }
-        return;
     }
 }
diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs
@@ -1648,16 +1648,15 @@ fn lint_unnecessary_fold(cx: &LateContext<'_, '_>, expr: &hir::Expr, fold_args:
     );
 
     // Check if the first argument to .fold is a suitable literal
-    match fold_args[1].node {
-        hir::ExprKind::Lit(ref lit) => match lit.node {
+    if let hir::ExprKind::Lit(ref lit) = fold_args[1].node {
+        match lit.node {
             ast::LitKind::Bool(false) => check_fold_with_op(cx, fold_args, hir::BinOpKind::Or, "any", true),
             ast::LitKind::Bool(true) => check_fold_with_op(cx, fold_args, hir::BinOpKind::And, "all", true),
             ast::LitKind::Int(0, _) => check_fold_with_op(cx, fold_args, hir::BinOpKind::Add, "sum", false),
             ast::LitKind::Int(1, _) => check_fold_with_op(cx, fold_args, hir::BinOpKind::Mul, "product", false),
-            _ => return,
-        },
-        _ => return,
-    };
+            _ => (),
+        }
+    }
 }
 
 fn lint_iter_nth<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, expr: &hir::Expr, iter_args: &'tcx [hir::Expr], is_mut: bool) {

diff --git a/clippy_lints/src/non_expressive_names.rs b/clippy_lints/src/non_expressive_names.rs
@@ -169,7 +169,9 @@ impl<'a, 'tcx, 'b> SimilarNamesNameVisitor<'a, 'tcx, 'b> {
             .any(|id| id.name == ident.name)
         {
             return;
-        } else if let Some(scope) = &mut self.0.single_char_names.last_mut() {
+        }
+
+        if let Some(scope) = &mut self.0.single_char_names.last_mut() {
             scope.push(ident);
         }
     }

diff --git a/clippy_lints/src/redundant_pattern_matching.rs b/clippy_lints/src/redundant_pattern_matching.rs
@@ -95,8 +95,6 @@ fn find_sugg_for_if_let<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, expr: &'tcx Expr,
                 );
             },
         );
-    } else {
-        return;
     }
 }
 
@@ -161,8 +159,6 @@ fn find_sugg_for_match<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, expr: &'tcx Expr, o
                 },
             );
         }
-    } else {
-        return;
     }
 }
 

diff --git a/clippy_lints/src/returns.rs b/clippy_lints/src/returns.rs
@@ -83,6 +83,12 @@ declare_clippy_lint! {
     "needless unit expression"
 }
 
+#[derive(PartialEq, Eq, Copy, Clone)]
+enum RetReplacement {
+    Empty,
+    Unit,
+}
+
 declare_lint_pass!(Return => [NEEDLESS_RETURN, LET_AND_RETURN, UNUSED_UNIT]);
 
 impl Return {
@@ -91,21 +97,32 @@ impl Return {
         if let Some(stmt) = block.stmts.last() {
             match stmt.node {
                 ast::StmtKind::Expr(ref expr) | ast::StmtKind::Semi(ref expr) => {
-                    self.check_final_expr(cx, expr, Some(stmt.span));
+                    self.check_final_expr(cx, expr, Some(stmt.span), RetReplacement::Empty);
                 },
                 _ => (),
             }
         }
     }
 
     // Check a the final expression in a block if it's a return.
-    fn check_final_expr(&mut self, cx: &EarlyContext<'_>, expr: &ast::Expr, span: Option<Span>) {
+    fn check_final_expr(
+        &mut self,
+        cx: &EarlyContext<'_>,
+        expr: &ast::Expr,
+        span: Option<Span>,
+        replacement: RetReplacement,
+    ) {
         match expr.node {
             // simple return is always "bad"
-            ast::ExprKind::Ret(Some(ref inner)) => {
+            ast::ExprKind::Ret(ref inner) => {
                 // allow `#[cfg(a)] return a; #[cfg(b)] return b;`
                 if !expr.attrs.iter().any(attr_is_cfg) {
-                    self.emit_return_lint(cx, span.expect("`else return` is not possible"), inner.span);
+                    self.emit_return_lint(
+                        cx,
+                        span.expect("`else return` is not possible"),
+                        inner.as_ref().map(|i| i.span),
+                        replacement,
+                    );
                 }
             },
             // a whole block? check it!
@@ -117,32 +134,60 @@ impl Return {
             // (except for unit type functions) so we don't match it
             ast::ExprKind::If(_, ref ifblock, Some(ref elsexpr)) => {
                 self.check_block_return(cx, ifblock);
-                self.check_final_expr(cx, elsexpr, None);
+                self.check_final_expr(cx, elsexpr, None, RetReplacement::Empty);
             },
             // a match expr, check all arms
             ast::ExprKind::Match(_, ref arms) => {
                 for arm in arms {
-                    self.check_final_expr(cx, &arm.body, Some(arm.body.span));
+                    self.check_final_expr(cx, &arm.body, Some(arm.body.span), RetReplacement::Unit);
                 }
             },
             _ => (),
         }
     }
 
-    fn emit_return_lint(&mut self, cx: &EarlyContext<'_>, ret_span: Span, inner_span: Span) {
-        if in_external_macro(cx.sess(), inner_span) || in_macro_or_desugar(inner_span) {
-            return;
+    fn emit_return_lint(
+        &mut self,
+        cx: &EarlyContext<'_>,
+        ret_span: Span,
+        inner_span: Option<Span>,
+        replacement: RetReplacement,
+    ) {
+        match inner_span {
+            Some(inner_span) => {
+                if in_external_macro(cx.sess(), inner_span) || in_macro_or_desugar(inner_span) {
+                    return;
+                }
+
+                span_lint_and_then(cx, NEEDLESS_RETURN, ret_span, "unneeded return statement", |db| {
+                    if let Some(snippet) = snippet_opt(cx, inner_span) {
+                        db.span_suggestion(ret_span, "remove `return`", snippet, Applicability::MachineApplicable);
+                    }
+                })
+            },
+            None => match replacement {
+                RetReplacement::Empty => {
+                    span_lint_and_then(cx, NEEDLESS_RETURN, ret_span, "unneeded return statement", |db| {
+                        db.span_suggestion(
+                            ret_span,
+                            "remove `return`",
+                            String::new(),
+                            Applicability::MachineApplicable,
+                        );
+                    });
+                },
+                RetReplacement::Unit => {
+                    span_lint_and_then(cx, NEEDLESS_RETURN, ret_span, "unneeded return statement", |db| {
+                        db.span_suggestion(
+                            ret_span,
+                            "replace `return` with the unit type",
+                            "()".to_string(),
+                            Applicability::MachineApplicable,
+                        );
+                    });
+                },
+            },
         }
-        span_lint_and_then(cx, NEEDLESS_RETURN, ret_span, "unneeded return statement", |db| {
-            if let Some(snippet) = snippet_opt(cx, inner_span) {
-                db.span_suggestion(
-                    ret_span,
-                    "remove `return` as shown",
-                    snippet,
-                    Applicability::MachineApplicable,
-                );
-            }
-        });
     }
 
     // Check for "let x = EXPR; x"
@@ -195,7 +240,7 @@ impl EarlyLintPass for Return {
     fn check_fn(&mut self, cx: &EarlyContext<'_>, kind: FnKind<'_>, decl: &ast::FnDecl, span: Span, _: ast::NodeId) {
         match kind {
             FnKind::ItemFn(.., block) | FnKind::Method(.., block) => self.check_block_return(cx, block),
-            FnKind::Closure(body) => self.check_final_expr(cx, body, Some(body.span)),
+            FnKind::Closure(body) => self.check_final_expr(cx, body, Some(body.span), RetReplacement::Empty),
         }
         if_chain! {
             if let ast::FunctionRetTy::Ty(ref ty) = decl.output;

diff --git a/tests/ui/needless_return.rs b/tests/ui/needless_return.rs
@@ -1,5 +1,11 @@
 #![warn(clippy::needless_return)]
 
+macro_rules! the_answer {
+    () => {
+        42
+    };
+}
+
 fn test_end_of_fn() -> bool {
     if true {
         // no error!
@@ -36,6 +42,29 @@ fn test_closure() {
     let _ = || return true;
 }
 
+fn test_macro_call() -> i32 {
+    return the_answer!();
+}
+
+fn test_void_fun() {
+    return;
+}
+
+fn test_void_if_fun(b: bool) {
+    if b {
+        return;
+    } else {
+        return;
+    }
+}
+
+fn test_void_match(x: u32) {
+    match x {
+        0 => (),
+        _ => return,
+    }
+}
+
 fn main() {
     let _ = test_end_of_fn();
     let _ = test_no_semicolon();

diff --git a/tests/ui/needless_return.stderr b/tests/ui/needless_return.stderr
@@ -1,52 +1,76 @@
 error: unneeded return statement
-  --> $DIR/needless_return.rs:8:5
+  --> $DIR/needless_return.rs:14:5
    |
 LL |     return true;
-   |     ^^^^^^^^^^^^ help: remove `return` as shown: `true`
+   |     ^^^^^^^^^^^^ help: remove `return`: `true`
    |
    = note: `-D clippy::needless-return` implied by `-D warnings`
 
 error: unneeded return statement
-  --> $DIR/needless_return.rs:12:5
+  --> $DIR/needless_return.rs:18:5
    |
 LL |     return true;
-   |     ^^^^^^^^^^^^ help: remove `return` as shown: `true`
+   |     ^^^^^^^^^^^^ help: remove `return`: `true`
 
 error: unneeded return statement
-  --> $DIR/needless_return.rs:17:9
+  --> $DIR/needless_return.rs:23:9
    |
 LL |         return true;
-   |         ^^^^^^^^^^^^ help: remove `return` as shown: `true`
+   |         ^^^^^^^^^^^^ help: remove `return`: `true`
 
 error: unneeded return statement
-  --> $DIR/needless_return.rs:19:9
+  --> $DIR/needless_return.rs:25:9
    |
 LL |         return false;
-   |         ^^^^^^^^^^^^^ help: remove `return` as shown: `false`
+   |         ^^^^^^^^^^^^^ help: remove `return`: `false`
 
 error: unneeded return statement
-  --> $DIR/needless_return.rs:25:17
+  --> $DIR/needless_return.rs:31:17
    |
 LL |         true => return false,
-   |                 ^^^^^^^^^^^^ help: remove `return` as shown: `false`
+   |                 ^^^^^^^^^^^^ help: remove `return`: `false`
 
 error: unneeded return statement
-  --> $DIR/needless_return.rs:27:13
+  --> $DIR/needless_return.rs:33:13
    |
 LL |             return true;
-   |             ^^^^^^^^^^^^ help: remove `return` as shown: `true`
+   |             ^^^^^^^^^^^^ help: remove `return`: `true`
 
 error: unneeded return statement
-  --> $DIR/needless_return.rs:34:9
+  --> $DIR/needless_return.rs:40:9
    |
 LL |         return true;
-   |         ^^^^^^^^^^^^ help: remove `return` as shown: `true`
+   |         ^^^^^^^^^^^^ help: remove `return`: `true`
 
 error: unneeded return statement
-  --> $DIR/needless_return.rs:36:16
+  --> $DIR/needless_return.rs:42:16
    |
 LL |     let _ = || return true;
-   |                ^^^^^^^^^^^ help: remove `return` as shown: `true`
+   |                ^^^^^^^^^^^ help: remove `return`: `true`
 
-error: aborting due to 8 previous errors
+error: unneeded return statement
+  --> $DIR/needless_return.rs:50:5
+   |
+LL |     return;
+   |     ^^^^^^^ help: remove `return`
+
+error: unneeded return statement
+  --> $DIR/needless_return.rs:55:9
+   |
+LL |         return;
+   |         ^^^^^^^ help: remove `return`
+
+error: unneeded return statement
+  --> $DIR/needless_return.rs:57:9
+   |
+LL |         return;
+   |         ^^^^^^^ help: remove `return`
+
+error: unneeded return statement
+  --> $DIR/needless_return.rs:64:14
+   |
+LL |         _ => return,
+   |              ^^^^^^ help: replace `return` with the unit type: `()`
+
+error: aborting due to 12 previous errors