Make let_and_return a late lint pass

4 files changed, 91 insertions, 83 deletions

diff --git a/clippy_lints/src/let_and_return.rs b/clippy_lints/src/let_and_return.rs
new file mode 100644
index 00000000000..8b877f696af
--- /dev/null
+++ b/clippy_lints/src/let_and_return.rs
@@ -0,0 +1,82 @@
+use if_chain::if_chain;
+use rustc_errors::Applicability;
+use rustc_hir::{Block, ExprKind, PatKind, StmtKind};
+use rustc_lint::{LateContext, LateLintPass, LintContext};
+use rustc_middle::lint::in_external_macro;
+use rustc_session::{declare_lint_pass, declare_tool_lint};
+
+use crate::utils::{in_macro, match_qpath, snippet_opt, span_lint_and_then};
+
+declare_clippy_lint! {
+    /// **What it does:** Checks for `let`-bindings, which are subsequently
+    /// returned.
+    ///
+    /// **Why is this bad?** It is just extraneous code. Remove it to make your code
+    /// more rusty.
+    ///
+    /// **Known problems:** None.
+    ///
+    /// **Example:**
+    /// ```rust
+    /// fn foo() -> String {
+    ///     let x = String::new();
+    ///     x
+    /// }
+    /// ```
+    /// instead, use
+    /// ```
+    /// fn foo() -> String {
+    ///     String::new()
+    /// }
+    /// ```
+    pub LET_AND_RETURN,
+    style,
+    "creating a let-binding and then immediately returning it like `let x = expr; x` at the end of a block"
+}
+
+declare_lint_pass!(LetReturn => [LET_AND_RETURN]);
+
+impl<'a, 'tcx> LateLintPass<'a, 'tcx> for LetReturn {
+    fn check_block(&mut self, cx: &LateContext<'a, 'tcx>, block: &'tcx Block<'_>) {
+        // we need both a let-binding stmt and an expr
+        if_chain! {
+            if let Some(retexpr) = block.expr;
+            if let Some(stmt) = block.stmts.iter().last();
+            if let StmtKind::Local(local) = &stmt.kind;
+            if local.ty.is_none();
+            if local.attrs.is_empty();
+            if let Some(initexpr) = &local.init;
+            if let PatKind::Binding(.., ident, _) = local.pat.kind;
+            if let ExprKind::Path(qpath) = &retexpr.kind;
+            if match_qpath(qpath, &[&*ident.name.as_str()]);
+            if !in_external_macro(cx.sess(), initexpr.span);
+            if !in_external_macro(cx.sess(), retexpr.span);
+            if !in_external_macro(cx.sess(), local.span);
+            if !in_macro(local.span);
+            then {
+                span_lint_and_then(
+                    cx,
+                    LET_AND_RETURN,
+                    retexpr.span,
+                    "returning the result of a `let` binding from a block",
+                    |err| {
+                        err.span_label(local.span, "unnecessary `let` binding");
+
+                        if let Some(snippet) = snippet_opt(cx, initexpr.span) {
+                            err.multipart_suggestion(
+                                "return the expression directly",
+                                vec![
+                                    (local.span, String::new()),
+                                    (retexpr.span, snippet),
+                                ],
+                                Applicability::MachineApplicable,
+                            );
+                        } else {
+                            err.span_help(initexpr.span, "this expression can be directly returned");
+                        }
+                    },
+                );
+            }
+        }
+    }
+}
diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs
index 6f8923b2660..6bc9e23bac5 100644
--- a/clippy_lints/src/lib.rs
+++ b/clippy_lints/src/lib.rs
@@ -239,6 +239,7 @@ mod large_const_arrays;
 mod large_enum_variant;
 mod large_stack_arrays;
 mod len_zero;
+mod let_and_return;
 mod let_if_seq;
 mod let_underscore;
 mod lifetimes;
@@ -596,6 +597,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         &large_stack_arrays::LARGE_STACK_ARRAYS,
         &len_zero::LEN_WITHOUT_IS_EMPTY,
         &len_zero::LEN_ZERO,
+        &let_and_return::LET_AND_RETURN,
         &let_if_seq::USELESS_LET_IF_SEQ,
         &let_underscore::LET_UNDERSCORE_LOCK,
         &let_underscore::LET_UNDERSCORE_MUST_USE,
@@ -772,7 +774,6 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         &regex::INVALID_REGEX,
         &regex::REGEX_MACRO,
         &regex::TRIVIAL_REGEX,
-        &returns::LET_AND_RETURN,
         &returns::NEEDLESS_RETURN,
         &returns::UNUSED_UNIT,
         &serde_api::SERDE_API_MISUSE,
@@ -1022,6 +1023,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
     store.register_early_pass(|| box formatting::Formatting);
     store.register_early_pass(|| box misc_early::MiscEarlyLints);
     store.register_early_pass(|| box returns::Return);
+    store.register_late_pass(|| box let_and_return::LetReturn);
     store.register_early_pass(|| box collapsible_if::CollapsibleIf);
     store.register_early_pass(|| box items_after_statements::ItemsAfterStatements);
     store.register_early_pass(|| box precedence::Precedence);
@@ -1265,6 +1267,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         LintId::of(&large_enum_variant::LARGE_ENUM_VARIANT),
         LintId::of(&len_zero::LEN_WITHOUT_IS_EMPTY),
         LintId::of(&len_zero::LEN_ZERO),
+        LintId::of(&let_and_return::LET_AND_RETURN),
         LintId::of(&let_underscore::LET_UNDERSCORE_LOCK),
         LintId::of(&lifetimes::EXTRA_UNUSED_LIFETIMES),
         LintId::of(&lifetimes::NEEDLESS_LIFETIMES),
@@ -1390,7 +1393,6 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         LintId::of(&regex::INVALID_REGEX),
         LintId::of(&regex::REGEX_MACRO),
         LintId::of(&regex::TRIVIAL_REGEX),
-        LintId::of(&returns::LET_AND_RETURN),
         LintId::of(&returns::NEEDLESS_RETURN),
         LintId::of(&returns::UNUSED_UNIT),
         LintId::of(&serde_api::SERDE_API_MISUSE),
@@ -1474,6 +1476,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         LintId::of(&inherent_to_string::INHERENT_TO_STRING),
         LintId::of(&len_zero::LEN_WITHOUT_IS_EMPTY),
         LintId::of(&len_zero::LEN_ZERO),
+        LintId::of(&let_and_return::LET_AND_RETURN),
         LintId::of(&literal_representation::INCONSISTENT_DIGIT_GROUPING),
         LintId::of(&loops::EMPTY_LOOP),
         LintId::of(&loops::FOR_KV_MAP),
@@ -1526,7 +1529,6 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         LintId::of(&redundant_static_lifetimes::REDUNDANT_STATIC_LIFETIMES),
         LintId::of(&regex::REGEX_MACRO),
         LintId::of(&regex::TRIVIAL_REGEX),
-        LintId::of(&returns::LET_AND_RETURN),
         LintId::of(&returns::NEEDLESS_RETURN),
         LintId::of(&returns::UNUSED_UNIT),
         LintId::of(&single_component_path_imports::SINGLE_COMPONENT_PATH_IMPORTS),
diff --git a/clippy_lints/src/returns.rs b/clippy_lints/src/returns.rs
index 35464f629c3..3c939744173 100644
--- a/clippy_lints/src/returns.rs
+++ b/clippy_lints/src/returns.rs
@@ -8,7 +8,7 @@ use rustc_session::{declare_lint_pass, declare_tool_lint};
 use rustc_span::source_map::Span;
 use rustc_span::BytePos;
 
-use crate::utils::{in_macro, match_path_ast, snippet_opt, span_lint_and_sugg, span_lint_and_then};
+use crate::utils::{snippet_opt, span_lint_and_sugg, span_lint_and_then};
 
 declare_clippy_lint! {
     /// **What it does:** Checks for return statements at the end of a block.
@@ -37,33 +37,6 @@ declare_clippy_lint! {
 }
 
 declare_clippy_lint! {
-    /// **What it does:** Checks for `let`-bindings, which are subsequently
-    /// returned.
-    ///
-    /// **Why is this bad?** It is just extraneous code. Remove it to make your code
-    /// more rusty.
-    ///
-    /// **Known problems:** None.
-    ///
-    /// **Example:**
-    /// ```rust
-    /// fn foo() -> String {
-    ///     let x = String::new();
-    ///     x
-    /// }
-    /// ```
-    /// instead, use
-    /// ```
-    /// fn foo() -> String {
-    ///     String::new()
-    /// }
-    /// ```
-    pub LET_AND_RETURN,
-    style,
-    "creating a let-binding and then immediately returning it like `let x = expr; x` at the end of a block"
-}
-
-declare_clippy_lint! {
     /// **What it does:** Checks for unit (`()`) expressions that can be removed.
     ///
     /// **Why is this bad?** Such expressions add no value, but can make the code
@@ -90,7 +63,7 @@ enum RetReplacement {
     Block,
 }
 
-declare_lint_pass!(Return => [NEEDLESS_RETURN, LET_AND_RETURN, UNUSED_UNIT]);
+declare_lint_pass!(Return => [NEEDLESS_RETURN, UNUSED_UNIT]);
 
 impl Return {
     // Check the final stmt or expr in a block for unnecessary return.
@@ -105,7 +78,7 @@ impl Return {
         }
     }
 
-    // Check a the final expression in a block if it's a return.
+    // Check the final expression in a block if it's a return.
     fn check_final_expr(
         &mut self,
         cx: &EarlyContext<'_>,
@@ -186,54 +159,6 @@ impl Return {
             },
         }
     }
-
-    // Check for "let x = EXPR; x"
-    fn check_let_return(cx: &EarlyContext<'_>, block: &ast::Block) {
-        let mut it = block.stmts.iter();
-
-        // we need both a let-binding stmt and an expr
-        if_chain! {
-            if let Some(retexpr) = it.next_back();
-            if let ast::StmtKind::Expr(ref retexpr) = retexpr.kind;
-            if let Some(stmt) = it.next_back();
-            if let ast::StmtKind::Local(ref local) = stmt.kind;
-            // don't lint in the presence of type inference
-            if local.ty.is_none();
-            if local.attrs.is_empty();
-            if let Some(ref initexpr) = local.init;
-            if let ast::PatKind::Ident(_, ident, _) = local.pat.kind;
-            if let ast::ExprKind::Path(_, ref path) = retexpr.kind;
-            if match_path_ast(path, &[&*ident.name.as_str()]);
-            if !in_external_macro(cx.sess(), initexpr.span);
-            if !in_external_macro(cx.sess(), retexpr.span);
-            if !in_external_macro(cx.sess(), local.span);
-            if !in_macro(local.span);
-            then {
-                span_lint_and_then(
-                    cx,
-                    LET_AND_RETURN,
-                    retexpr.span,
-                    "returning the result of a `let` binding from a block",
-                    |err| {
-                        err.span_label(local.span, "unnecessary `let` binding");
-
-                        if let Some(snippet) = snippet_opt(cx, initexpr.span) {
-                            err.multipart_suggestion(
-                                "return the expression directly",
-                                vec![
-                                    (local.span, String::new()),
-                                    (retexpr.span, snippet),
-                                ],
-                                Applicability::MachineApplicable,
-                            );
-                        } else {
-                            err.span_help(initexpr.span, "this expression can be directly returned");
-                        }
-                    },
-                );
-            }
-        }
-    }
 }
 
 impl EarlyLintPass for Return {
@@ -254,7 +179,6 @@ impl EarlyLintPass for Return {
     }
 
     fn check_block(&mut self, cx: &EarlyContext<'_>, block: &ast::Block) {
-        Self::check_let_return(cx, block);
         if_chain! {
             if let Some(ref stmt) = block.stmts.last();
             if let ast::StmtKind::Expr(ref expr) = stmt.kind;
diff --git a/src/lintlist/mod.rs b/src/lintlist/mod.rs
index d5d07ccb2eb..bb191f9be92 100644
--- a/src/lintlist/mod.rs
+++ b/src/lintlist/mod.rs
@@ -1023,7 +1023,7 @@ pub static ref ALL_LINTS: Vec<Lint> = vec![
         group: "style",
         desc: "creating a let-binding and then immediately returning it like `let x = expr; x` at the end of a block",
         deprecation: None,
-        module: "returns",
+        module: "let_and_return",
     },
     Lint {
         name: "let_underscore_lock",