Merge pull request #3494 from daxpedda/master

Added `IMPLICIT_RETURN` lint.

6 files changed, 249 insertions, 1 deletions

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 320a3511e5c..e691ec9412f 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -706,6 +706,7 @@ All notable changes to this project will be documented in this file.
 [`if_same_then_else`]: https://rust-lang.github.io/rust-clippy/master/index.html#if_same_then_else
 [`ifs_same_cond`]: https://rust-lang.github.io/rust-clippy/master/index.html#ifs_same_cond
 [`implicit_hasher`]: https://rust-lang.github.io/rust-clippy/master/index.html#implicit_hasher
+[`implicit_return`]: https://rust-lang.github.io/rust-clippy/master/index.html#implicit_return
 [`inconsistent_digit_grouping`]: https://rust-lang.github.io/rust-clippy/master/index.html#inconsistent_digit_grouping
 [`indexing_slicing`]: https://rust-lang.github.io/rust-clippy/master/index.html#indexing_slicing
 [`ineffective_bit_mask`]: https://rust-lang.github.io/rust-clippy/master/index.html#ineffective_bit_mask
diff --git a/README.md b/README.md
index 0d83224e3f6..92bb4586688 100644
--- a/README.md
+++ b/README.md
@@ -7,7 +7,7 @@
 
 A collection of lints to catch common mistakes and improve your [Rust](https://github.com/rust-lang/rust) code.
 
-[There are 289 lints included in this crate!](https://rust-lang.github.io/rust-clippy/master/index.html)
+[There are 290 lints included in this crate!](https://rust-lang.github.io/rust-clippy/master/index.html)
 
 We have a bunch of lint categories to allow you to choose how much Clippy is supposed to ~~annoy~~ help you:
 
diff --git a/clippy_lints/src/implicit_return.rs b/clippy_lints/src/implicit_return.rs
new file mode 100644
index 00000000000..664f182c533
--- /dev/null
+++ b/clippy_lints/src/implicit_return.rs
@@ -0,0 +1,135 @@
+// Copyright 2014-2018 The Rust Project Developers. See the COPYRIGHT
+// file at the top-level directory of this distribution.
+//
+// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
+// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
+// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
+// option. This file may not be copied, modified, or distributed
+// except according to those terms.
+
+use crate::rustc::hir::{intravisit::FnKind, Body, ExprKind, FnDecl};
+use crate::rustc::lint::{LateContext, LateLintPass, LintArray, LintPass};
+use crate::rustc::{declare_tool_lint, lint_array};
+use crate::rustc_errors::Applicability;
+use crate::syntax::{ast::NodeId, source_map::Span};
+use crate::utils::{snippet_opt, span_lint_and_then};
+
+/// **What it does:** Checks for missing return statements at the end of a block.
+///
+/// **Why is this bad?** Actually omitting the return keyword is idiomatic Rust code. Programmers
+/// coming from other languages might prefer the expressiveness of `return`. It's possible to miss
+/// the last returning statement because the only difference is a missing `;`. Especially in bigger
+/// code with multiple return paths having a `return` keyword makes it easier to find the
+/// corresponding statements.
+///
+/// **Known problems:** None.
+///
+/// **Example:**
+/// ```rust
+/// fn foo(x: usize) {
+///     x
+/// }
+/// ```
+/// add return
+/// ```rust
+/// fn foo(x: usize) {
+///     return x;
+/// }
+/// ```
+declare_clippy_lint! {
+    pub IMPLICIT_RETURN,
+    restriction,
+    "use a return statement like `return expr` instead of an expression"
+}
+
+pub struct Pass;
+
+impl Pass {
+    fn expr_match(cx: &LateContext<'_, '_>, expr: &rustc::hir::Expr) {
+        match &expr.node {
+            ExprKind::Block(block, ..) => {
+                if let Some(expr) = &block.expr {
+                    Self::expr_match(cx, expr);
+                }
+                // only needed in the case of `break` with `;` at the end
+                else if let Some(stmt) = block.stmts.last() {
+                    if let rustc::hir::StmtKind::Semi(expr, ..) = &stmt.node {
+                        Self::expr_match(cx, expr);
+                    }
+                }
+            },
+            // use `return` instead of `break`
+            ExprKind::Break(.., break_expr) => {
+                if let Some(break_expr) = break_expr {
+                    span_lint_and_then(cx, IMPLICIT_RETURN, expr.span, "missing return statement", |db| {
+                        if let Some(snippet) = snippet_opt(cx, break_expr.span) {
+                            db.span_suggestion_with_applicability(
+                                expr.span,
+                                "change `break` to `return` as shown",
+                                format!("return {}", snippet),
+                                Applicability::MachineApplicable,
+                            );
+                        }
+                    });
+                }
+            },
+            ExprKind::If(.., if_expr, else_expr) => {
+                Self::expr_match(cx, if_expr);
+
+                if let Some(else_expr) = else_expr {
+                    Self::expr_match(cx, else_expr);
+                }
+            },
+            ExprKind::Match(_, arms, ..) => {
+                for arm in arms {
+                    Self::expr_match(cx, &arm.body);
+                }
+            },
+            // loops could be using `break` instead of `return`
+            ExprKind::Loop(block, ..) => {
+                if let Some(expr) = &block.expr {
+                    Self::expr_match(cx, expr);
+                }
+            },
+            // skip if it already has a return statement
+            ExprKind::Ret(..) => (),
+            // everything else is missing `return`
+            _ => span_lint_and_then(cx, IMPLICIT_RETURN, expr.span, "missing return statement", |db| {
+                if let Some(snippet) = snippet_opt(cx, expr.span) {
+                    db.span_suggestion_with_applicability(
+                        expr.span,
+                        "add `return` as shown",
+                        format!("return {}", snippet),
+                        Applicability::MachineApplicable,
+                    );
+                }
+            }),
+        }
+    }
+}
+
+impl LintPass for Pass {
+    fn get_lints(&self) -> LintArray {
+        lint_array!(IMPLICIT_RETURN)
+    }
+}
+
+impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Pass {
+    fn check_fn(
+        &mut self,
+        cx: &LateContext<'a, 'tcx>,
+        _: FnKind<'tcx>,
+        _: &'tcx FnDecl,
+        body: &'tcx Body,
+        _: Span,
+        _: NodeId,
+    ) {
+        let def_id = cx.tcx.hir.body_owner_def_id(body.id());
+        let mir = cx.tcx.optimized_mir(def_id);
+
+        // checking return type through MIR, HIR is not able to determine inferred closure return types
+        if !mir.return_ty().is_unit() {
+            Self::expr_match(cx, &body.value);
+        }
+    }
+}
diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs
index 326bf884e33..ee41c632077 100644
--- a/clippy_lints/src/lib.rs
+++ b/clippy_lints/src/lib.rs
@@ -126,6 +126,7 @@ pub mod functions;
 pub mod identity_conversion;
 pub mod identity_op;
 pub mod if_not_else;
+pub mod implicit_return;
 pub mod indexing_slicing;
 pub mod infallible_destructuring_match;
 pub mod infinite_iter;
@@ -371,6 +372,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry<'_>, conf: &Conf) {
     reg.register_late_lint_pass(box unicode::Unicode);
     reg.register_late_lint_pass(box strings::StringAdd);
     reg.register_early_lint_pass(box returns::ReturnPass);
+    reg.register_late_lint_pass(box implicit_return::Pass);
     reg.register_late_lint_pass(box methods::Pass);
     reg.register_late_lint_pass(box map_clone::Pass);
     reg.register_late_lint_pass(box shadow::Pass);
@@ -485,6 +487,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry<'_>, conf: &Conf) {
         arithmetic::FLOAT_ARITHMETIC,
         arithmetic::INTEGER_ARITHMETIC,
         else_if_without_else::ELSE_IF_WITHOUT_ELSE,
+        implicit_return::IMPLICIT_RETURN,
         indexing_slicing::INDEXING_SLICING,
         inherent_impl::MULTIPLE_INHERENT_IMPL,
         literal_representation::DECIMAL_LITERAL_REPRESENTATION,
diff --git a/tests/ui/implicit_return.rs b/tests/ui/implicit_return.rs
new file mode 100644
index 00000000000..73cf2908833
--- /dev/null
+++ b/tests/ui/implicit_return.rs
@@ -0,0 +1,63 @@
+// Copyright 2014-2018 The Rust Project Developers. See the COPYRIGHT
+// file at the top-level directory of this distribution.
+//
+// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
+// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
+// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
+// option. This file may not be copied, modified, or distributed
+// except according to those terms.
+
+
+
+
+
+#![warn(clippy::implicit_return)]
+
+fn test_end_of_fn() -> bool {
+    if true {
+        // no error!
+        return true;
+    }
+    true
+}
+
+#[allow(clippy::needless_bool)]
+fn test_if_block() -> bool {
+    if true {
+        true
+    } else {
+        false
+    }
+}
+
+#[allow(clippy::match_bool)]
+fn test_match(x: bool) -> bool {
+    match x {
+        true => false,
+        false => {
+            true
+        }
+    }
+}
+
+#[allow(clippy::never_loop)]
+fn test_loop() -> bool {
+    loop {
+        break true;
+    }
+}
+
+fn test_closure() {
+    let _ = || {
+        true
+    };
+    let _ = || true;
+}
+
+fn main() {
+    let _ = test_end_of_fn();
+    let _ = test_if_block();
+    let _ = test_match(true);
+    let _ = test_loop();
+    test_closure();
+}
diff --git a/tests/ui/implicit_return.stderr b/tests/ui/implicit_return.stderr
new file mode 100644
index 00000000000..bba8d942e27
--- /dev/null
+++ b/tests/ui/implicit_return.stderr
@@ -0,0 +1,46 @@
+error: missing return statement
+  --> $DIR/implicit_return.rs:21:5
+   |
+21 |     true
+   |     ^^^^ help: add `return` as shown: `return true`
+   |
+   = note: `-D clippy::implicit-return` implied by `-D warnings`
+
+error: missing return statement
+  --> $DIR/implicit_return.rs:27:9
+   |
+27 |         true
+   |         ^^^^ help: add `return` as shown: `return true`
+
+error: missing return statement
+  --> $DIR/implicit_return.rs:29:9
+   |
+29 |         false
+   |         ^^^^^ help: add `return` as shown: `return false`
+
+error: missing return statement
+  --> $DIR/implicit_return.rs:36:17
+   |
+36 |         true => false,
+   |                 ^^^^^ help: add `return` as shown: `return false`
+
+error: missing return statement
+  --> $DIR/implicit_return.rs:38:13
+   |
+38 |             true
+   |             ^^^^ help: add `return` as shown: `return true`
+
+error: missing return statement
+  --> $DIR/implicit_return.rs:52:9
+   |
+52 |         true
+   |         ^^^^ help: add `return` as shown: `return true`
+
+error: missing return statement
+  --> $DIR/implicit_return.rs:54:16
+   |
+54 |     let _ = || true;
+   |                ^^^^ help: add `return` as shown: `return true`
+
+error: aborting due to 7 previous errors
+