needless_return false positive when a temporary borrows a local variable

[ebroto]

I tried this code:

#![allow(unused)]

fn read_line() -> String {
    use std::io::BufRead;
    let stdin = ::std::io::stdin();
    return stdin.lock().lines().next().unwrap().unwrap();
}

I expected to see this happen: needless_return does not fire because the call to lock() borrows stdin and the temporary would be destroyed after the borrowed local, resulting in a does not live long enough compiler error.

Instead, this happened: it fired

The same fix that was applied for let_and_return in #5680 should work for this case. The known problems section of the lint description could be updated too.

Meta

• cargo clippy -V: clippy 0.0.212 (05762e3 2020-08-01)
• rustc -Vv:

  rustc 1.45.1 (c367798cf 2020-07-26)
  binary: rustc
  commit-hash: c367798cfd3817ca6ae908ce675d1d99242af148
  commit-date: 2020-07-26
  host: x86_64-unknown-linux-gnu
  release: 1.45.1
  LLVM version: 10.0
  

[jrqc]

I'd like to give this issue a try

[ebroto]

Don't hesitate to ask for support if you get stuck!

The general idea is to move the lint from early to late lint pass, probably reusing the let_and_return module, adapting the name.

[jrqc]

Great, thanks!

[jrqc]

Hi @ebroto,
I made the needless_return a late pass and added a borrow checker following fix #5680.
Also, added the mentioned function read_line() to the test cases. All tests are passing locally.
I have read the PR guideline and now I am opening a PR.

[categulario]

I just found a possible instance of this with clippy 0.1.63 (4b91a6e 2022-08-08) and the following snippet:

use rusqlite::{Connection, ToSql, Error};

pub fn entry_query(query: &str, params: &[&dyn ToSql]) -> Result<Vec<String>, Error> {
    let conn = Connection::open_in_memory().unwrap();
    let mut stmt = conn.prepare(query).unwrap();

    let entries = stmt.query_and_then(params, |row| {
        row.get("id")
    })?.collect();

    entries
}

Which gives this result:

warning: returning the result of a `let` binding from a block
  --> src/lib.rs:11:5
   |
7  | /     let entries = stmt.query_and_then(params, |row| {
8  | |         row.get("id")
9  | |     })?.collect();
   | |__________________- unnecessary `let` binding
10 |
11 |       entries
   |       ^^^^^^^
   |
   = 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
   |
7  ~
8  |
9  ~     stmt.query_and_then(params, |row| {
10 +         row.get("id")
11 +     })?.collect()
   |

If the suggestion is used the code fails to compile:

error[E0597]: `conn` does not live long enough
  --> src/lib.rs:5:20
   |
5  |       let mut stmt = conn.prepare(query).unwrap();
   |                      ^^^^^^^^^^^^^^^^^^^ borrowed value does not live long enough
6  |
7  | /     stmt.query_and_then(params, |row| {
8  | |         row.get("id")
9  | |     })?.collect()
   | |_______- a temporary with access to the borrow is created here ...
10 |   }
   |   -
   |   |
   |   `conn` dropped here while still borrowed
   |   ... and the borrow might be used here, when that temporary is dropped and runs the destructor for type `ControlFlow<Result<Infallible, rusqlite::Error>, AndThenRows<'_, [closure@src/lib.rs:7:33: 9:6]>>`
   |
   = 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
   |
7  ~     let x = stmt.query_and_then(params, |row| {
8  |         row.get("id")
9  ~     })?.collect(); x
   |

and of course the suggested code is the code that clippy complained about in the first place.