Lifetime conficts when recommending closures

[Jarcho]

Description

This affects any lint where we recommend moving code into a closure. Off the top of my head are manual_map, option_if_let_else, and map_entry, but there are probably more.

Given the following code:

let mut x = 0;
let ref_x = &mut x;

let _ = match Some(0) {
    Some(y) => Some({
        *ref_x = y;
        some_fn_call(&mut x) + y
    }),
    None => None,
};

Clippy will suggest to use Option::map

let _ = Some(0).map(|y| {
    *ref_x = y;
    some_fn_call(&mut x) + y
});

This will fail with multiple mutable borrows of x due to the closure capturing both ref_x and x. The original code would have the borrow held by ref_x end right before x is borrowed for some_fn_call.

Solving this would require the lifetimes computed by borrowck which I don't believe we have easy access to. It may also require turning these as MIR lints.

Version

No response

Additional Labels

@rustbot label +I-false-positive
@rustbot label +E-hard
@rustbot label +T-MIR

[tamaroning]

@rustbot claim

[tamaroning]

I am thinking of reusing PosibbleBorrowerVisitor used in redundant_closure, which means no need to deal with specific cases (like #7531 )

[Jarcho]

I assume you mean redundant_clone. You'll need to adapt PossibleBorrowerVisitor to track only mutable borrows (easy) and either convert all the lints to MIR lints, or find some way to link HIR nodes with MIR (the less easy part).

This will also introduce false negatives as well. PossibleBorrowerVisitor is not particularly precise at tracking lifetimes.

[tamaroning]

ohh yes. I mean redundant_clone.

find some way to link HIR nodes with MIR (the less easy part).

I think just comparing spans of nodes with spans of mir statements might be enough. (further invastigation is needed)

This will also introduce false negatives as well. PossibleBorrowerVisitor is not particularly precise at tracking lifetimes.

I see. I am going to dig into borrow checker and try to use intermidiate state of it.

[Jarcho]

I think just comparing spans of nodes with spans of mir statements might be enough. (further invastigation is needed)

It might be doable. I can't think of a reason why it couldn't work, but it has to be done as least somewhat separately for each lint.

I see. I am going to dig into borrow checker and try to use intermidiate state of it.

You might need changes on the rustc side to make that work. It might be worth waiting until polonius is ready, but that could be years.

[TimonPost]

I also get a false positive suggestion where clippy suggests faulty code with a borrow issue:

use std::sync::Arc;

fn main() {
    gives_clippy_warning();   
    clippy_suggestion_gives_error();
}

fn my_function(foo: &String) -> String {
    foo.to_string()
}
        
fn gives_clippy_warning() {
    let closure = |foo| my_function(foo);

    let value_a = "a".to_string();
    let value_b = Arc::new("b".to_string());
    
    // clippy: manual_map warning
    let _result = if 1 == 2 {
        Some(closure(&value_a))
    } else if let Some(value_b) = Some(value_b) {
        Some(closure(&value_b))
    } else {
        None
    };
}

fn clippy_suggestion_gives_error() {
    let closure = |foo| my_function(foo);

    let value_a = "a".to_string();
    let value_b = Arc::new("b".to_string());

    // Clippy suggestion but doesnt compile as value_b does not live long enough
    let _result = if 1 == 2 {
        Some(closure(&value_a))
    } else {
        Some(value_b).map(|value_b| closure(&value_b))
    };
}