Auto merge of #5680 - ebroto:3792_let_return, r=Manishearth

let_and_return: avoid "does not live long enough" errors

EDIT: Add #3324 to the list of fixes

<details>
<summary>Description of old impl</summary>
<br>
Avoid suggesting turning the RHS expression of the last statement into the block tail expression if a temporary borrows from a local that would be destroyed before.

This is my first incursion into MIR so there's probably room for improvement!
</details>

Avoid linting if the return type of some method or function called in the last statement has a lifetime parameter.

changelog: Fix false positive in [`let_and_return`]

Fixes #3792
Fixes #3324

clippy_lints/src/let_and_return.rs

@@ -0,0 +1,141 @@
+use if_chain::if_chain;
+use rustc_errors::Applicability;
+use rustc_hir::def_id::DefId;
+use rustc_hir::intravisit::{walk_expr, NestedVisitorMap, Visitor};
+use rustc_hir::{Block, Expr, ExprKind, PatKind, StmtKind};
+use rustc_lint::{LateContext, LateLintPass, LintContext};
+use rustc_middle::hir::map::Map;
+use rustc_middle::lint::in_external_macro;
+use rustc_middle::ty::subst::GenericArgKind;
+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 !last_statement_borrows(cx, initexpr);
+            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");
+                        }
+                    },
+                );
+            }
+        }
+    }
+}
+
+fn last_statement_borrows<'tcx>(cx: &LateContext<'_, 'tcx>, expr: &'tcx Expr<'tcx>) -> bool {
+    let mut visitor = BorrowVisitor { cx, borrows: false };
+    walk_expr(&mut visitor, expr);
+    visitor.borrows
+}
+
+struct BorrowVisitor<'a, 'tcx> {
+    cx: &'a LateContext<'a, 'tcx>,
+    borrows: bool,
+}
+
+impl BorrowVisitor<'_, '_> {
+    fn fn_def_id(&self, expr: &Expr<'_>) -> Option<DefId> {
+        match &expr.kind {
+            ExprKind::MethodCall(..) => self.cx.tables.type_dependent_def_id(expr.hir_id),
+            ExprKind::Call(
+                Expr {
+                    kind: ExprKind::Path(qpath),
+                    ..
+                },
+                ..,
+            ) => self.cx.tables.qpath_res(qpath, expr.hir_id).opt_def_id(),
+            _ => None,
+        }
+    }
+}
+
+impl<'tcx> Visitor<'tcx> for BorrowVisitor<'_, 'tcx> {
+    type Map = Map<'tcx>;
+
+    fn visit_expr(&mut self, expr: &'tcx Expr<'_>) {
+        if self.borrows {
+            return;
+        }
+
+        if let Some(def_id) = self.fn_def_id(expr) {
+            self.borrows = self
+                .cx
+                .tcx
+                .fn_sig(def_id)
+                .output()
+                .skip_binder()
+                .walk()
+                .any(|arg| matches!(arg.unpack(), GenericArgKind::Lifetime(_)));
+        }
+
+        walk_expr(self, expr);
+    }
+
+    fn nested_visit_map(&mut self) -> NestedVisitorMap<Self::Map> {
+        NestedVisitorMap::None
+    }
+}

clippy_lints/src/lib.rs

@@ -241,6 +241,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;
@@ -599,6 +600,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,
@@ -775,7 +777,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,
@@ -1026,6 +1027,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);
@@ -1270,6 +1272,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),
@@ -1395,7 +1398,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),
@@ -1480,6 +1482,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),
@@ -1532,7 +1535,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),

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.
@@ -36,33 +36,6 @@ declare_clippy_lint! {
     "using a return statement like `return expr;` where an expression would suffice"
 }
 
-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.
     ///
@@ -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;

clippy_lints/src/utils/mod.rs

@@ -398,7 +398,7 @@ pub fn method_calls<'tcx>(
 /// Matches an `Expr` against a chain of methods, and return the matched `Expr`s.
 ///
 /// For example, if `expr` represents the `.baz()` in `foo.bar().baz()`,
-/// `matched_method_chain(expr, &["bar", "baz"])` will return a `Vec`
+/// `method_chain_args(expr, &["bar", "baz"])` will return a `Vec`
 /// containing the `Expr`s for
 /// `.bar()` and `.baz()`
 pub fn method_chain_args<'a>(expr: &'a Expr<'_>, methods: &[&str]) -> Option<Vec<&'a [Expr<'a>]>> {

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",

tests/ui/let_and_return.rs

@@ -0,0 +1,138 @@
+#![allow(unused)]
+#![warn(clippy::let_and_return)]
+
+fn test() -> i32 {
+    let _y = 0; // no warning
+    let x = 5;
+    x
+}
+
+fn test_inner() -> i32 {
+    if true {
+        let x = 5;
+        x
+    } else {
+        0
+    }
+}
+
+fn test_nowarn_1() -> i32 {
+    let mut x = 5;
+    x += 1;
+    x
+}
+
+fn test_nowarn_2() -> i32 {
+    let x = 5;
+    x + 1
+}
+
+fn test_nowarn_3() -> (i32, i32) {
+    // this should technically warn, but we do not compare complex patterns
+    let (x, y) = (5, 9);
+    (x, y)
+}
+
+fn test_nowarn_4() -> i32 {
+    // this should technically warn, but not b/c of clippy::let_and_return, but b/c of useless type
+    let x: i32 = 5;
+    x
+}
+
+fn test_nowarn_5(x: i16) -> u16 {
+    #[allow(clippy::cast_possible_truncation, clippy::cast_sign_loss)]
+    let x = x as u16;
+    x
+}
+
+// False positive example
+trait Decode {
+    fn decode<D: std::io::Read>(d: D) -> Result<Self, ()>
+    where
+        Self: Sized;
+}
+
+macro_rules! tuple_encode {
+    ($($x:ident),*) => (
+        impl<$($x: Decode),*> Decode for ($($x),*) {
+            #[inline]
+            #[allow(non_snake_case)]
+            fn decode<D: std::io::Read>(mut d: D) -> Result<Self, ()> {
+                // Shouldn't trigger lint
+                Ok(($({let $x = Decode::decode(&mut d)?; $x }),*))
+            }
+        }
+    );
+}
+
+tuple_encode!(T0, T1, T2, T3, T4, T5, T6, T7);
+
+mod no_lint_if_stmt_borrows {
+    mod issue_3792 {
+        use std::io::{self, BufRead, Stdin};
+
+        fn read_line() -> String {
+            let stdin = io::stdin();
+            let line = stdin.lock().lines().next().unwrap().unwrap();
+            line
+        }
+    }
+
+    mod issue_3324 {
+        use std::cell::RefCell;
+        use std::rc::{Rc, Weak};
+
+        fn test(value: Weak<RefCell<Bar>>) -> u32 {
+            let value = value.upgrade().unwrap();
+            let ret = value.borrow().baz();
+            ret
+        }
+
+        struct Bar {}
+
+        impl Bar {
+            fn new() -> Self {
+                Bar {}
+            }
+            fn baz(&self) -> u32 {
+                0
+            }
+        }
+
+        fn main() {
+            let a = Rc::new(RefCell::new(Bar::new()));
+            let b = Rc::downgrade(&a);
+            test(b);
+        }
+    }
+
+    mod free_function {
+        struct Inner;
+
+        struct Foo<'a> {
+            inner: &'a Inner,
+        }
+
+        impl Drop for Foo<'_> {
+            fn drop(&mut self) {}
+        }
+
+        impl Foo<'_> {
+            fn value(&self) -> i32 {
+                42
+            }
+        }
+
+        fn some_foo(inner: &Inner) -> Foo<'_> {
+            Foo { inner }
+        }
+
+        fn test() -> i32 {
+            let x = Inner {};
+            let value = some_foo(&x).value();
+            value
+        }
+    }
+}
+
+fn main() {}

tests/ui/let_and_return.stderr

@@ -1,5 +1,5 @@
 error: returning the result of a `let` binding from a block
-  --> $DIR/let_return.rs:7:5
+  --> $DIR/let_and_return.rs:7:5
    |
 LL |     let x = 5;
    |     ---------- unnecessary `let` binding
@@ -14,7 +14,7 @@ LL |     5
    |
 
 error: returning the result of a `let` binding from a block
-  --> $DIR/let_return.rs:13:9
+  --> $DIR/let_and_return.rs:13:9
    |
 LL |         let x = 5;
    |         ---------- unnecessary `let` binding

tests/ui/let_return.rs

@@ -1,70 +0,0 @@
-#![allow(unused)]
-#![warn(clippy::let_and_return)]
-
-fn test() -> i32 {
-    let _y = 0; // no warning
-    let x = 5;
-    x
-}
-
-fn test_inner() -> i32 {
-    if true {
-        let x = 5;
-        x
-    } else {
-        0
-    }
-}
-
-fn test_nowarn_1() -> i32 {
-    let mut x = 5;
-    x += 1;
-    x
-}
-
-fn test_nowarn_2() -> i32 {
-    let x = 5;
-    x + 1
-}
-
-fn test_nowarn_3() -> (i32, i32) {
-    // this should technically warn, but we do not compare complex patterns
-    let (x, y) = (5, 9);
-    (x, y)
-}
-
-fn test_nowarn_4() -> i32 {
-    // this should technically warn, but not b/c of clippy::let_and_return, but b/c of useless type
-    let x: i32 = 5;
-    x
-}
-
-fn test_nowarn_5(x: i16) -> u16 {
-    #[allow(clippy::cast_possible_truncation, clippy::cast_sign_loss)]
-    let x = x as u16;
-    x
-}
-
-// False positive example
-trait Decode {
-    fn decode<D: std::io::Read>(d: D) -> Result<Self, ()>
-    where
-        Self: Sized;
-}
-
-macro_rules! tuple_encode {
-    ($($x:ident),*) => (
-        impl<$($x: Decode),*> Decode for ($($x),*) {
-            #[inline]
-            #[allow(non_snake_case)]
-            fn decode<D: std::io::Read>(mut d: D) -> Result<Self, ()> {
-                // Shouldn't trigger lint
-                Ok(($({let $x = Decode::decode(&mut d)?; $x }),*))
-            }
-        }
-    );
-}
-
-tuple_encode!(T0, T1, T2, T3, T4, T5, T6, T7);
-
-fn main() {}