From 7f1595e18f542f2caf209a05dd090b4a2ebd3b8a Mon Sep 17 00:00:00 2001
From: Philip Hayes <philiphayes@fb.com>
Date: Fri, 29 Jan 2021 22:17:18 -0800
Subject: [PATCH] Fix let_and_return false positive

The issue:

See this Rust playground link: https://play.rust-lang.org/?edition=2018&gist=12cb5d1e7527f8c37743b87fc4a53748

Run the above with clippy to see the following warning:

```
warning: returning the result of a `let` binding from a block
  --> src/main.rs:24:5
   |
23 |     let value = Foo::new(&x).value();
   |     --------------------------------- unnecessary `let` binding
24 |     value
   |     ^^^^^
   |
   = note: `#[warn(clippy::let_and_return)]` on by default
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#let_and_return
help: return the expression directly
   |
23 |
24 |     Foo::new(&x).value()
   |
```

Implementing the suggested fix, removing the temporary let binding,
yields a compiler error:

```
error[E0597]: `x` does not live long enough
  --> src/main.rs:23:14
   |
23 |     Foo::new(&x).value()
   |     ---------^^-
   |     |        |
   |     |        borrowed value does not live long enough
   |     a temporary with access to the borrow is created here ...
24 | }
   | -
   | |
   | `x` dropped here while still borrowed
   | ... and the borrow might be used here, when that temporary is dropped and runs the `Drop` code for type `Foo`
   |
   = note: the temporary is part of an expression at the end of a block;
           consider forcing this temporary to be dropped sooner, before the block's local variables are dropped
help: for example, you could save the expression's value in a new local variable `x` and then make `x` be the expression at the end of the block
   |
23 |     let x = Foo::new(&x).value(); x
   |     ^^^^^^^                     ^^^
```

The fix:

Of course, clippy looks like it should already handle this edge case;
however, it appears `utils::fn_def_id` is not returning a `DefId` for
`Foo::new`. Changing the `qpath_res` lookup to use the child Path
`hir_id` instead of the parent Call `hir_id` fixes the issue.
---
 clippy_lints/src/utils/mod.rs  |  3 ++-
 tests/ui/let_and_return.rs     | 12 +++++++++++-
 tests/ui/let_and_return.stderr |  2 +-
 3 files changed, 14 insertions(+), 3 deletions(-)

diff --git a/clippy_lints/src/utils/mod.rs b/clippy_lints/src/utils/mod.rs
index 991fae6b1aa..caad7c88bcf 100644
--- a/clippy_lints/src/utils/mod.rs
+++ b/clippy_lints/src/utils/mod.rs
@@ -1546,10 +1546,11 @@ pub fn fn_def_id(cx: &LateContext<'_>, expr: &Expr<'_>) -> Option<DefId> {
         ExprKind::Call(
             Expr {
                 kind: ExprKind::Path(qpath),
+                hir_id: path_hir_id,
                 ..
             },
             ..,
-        ) => cx.typeck_results().qpath_res(qpath, expr.hir_id).opt_def_id(),
+        ) => cx.typeck_results().qpath_res(qpath, *path_hir_id).opt_def_id(),
         _ => None,
     }
 }
diff --git a/tests/ui/let_and_return.rs b/tests/ui/let_and_return.rs
index 73e550b3df8..e3561863c1e 100644
--- a/tests/ui/let_and_return.rs
+++ b/tests/ui/let_and_return.rs
@@ -117,7 +117,11 @@ mod no_lint_if_stmt_borrows {
             fn drop(&mut self) {}
         }
 
-        impl Foo<'_> {
+        impl<'a> Foo<'a> {
+            fn new(inner: &'a Inner) -> Self {
+                Self { inner }
+            }
+
             fn value(&self) -> i32 {
                 42
             }
@@ -132,6 +136,12 @@ mod no_lint_if_stmt_borrows {
             let value = some_foo(&x).value();
             value
         }
+
+        fn test2() -> i32 {
+            let x = Inner {};
+            let value = Foo::new(&x).value();
+            value
+        }
     }
 }
 
diff --git a/tests/ui/let_and_return.stderr b/tests/ui/let_and_return.stderr
index fe878e5f206..a6941dabeb8 100644
--- a/tests/ui/let_and_return.stderr
+++ b/tests/ui/let_and_return.stderr
@@ -28,7 +28,7 @@ LL |         5
    |
 
 error: returning the result of a `let` binding from a block
-  --> $DIR/let_and_return.rs:154:13
+  --> $DIR/let_and_return.rs:164:13
    |
 LL |             let clone = Arc::clone(&self.foo);
    |             ---------------------------------- unnecessary `let` binding