redundant_closure false positive when interacting with FnMut closures

[guswynn]

Summary

This is related to #8073 but I believe it is sufficiently different:

Lint Name

redundant_closure

Reproducer

I tried this code:

#![allow(unused)]
fn minimal<L>(mut logic: L)
where
    L: FnMut(()),
{
    let _: Box<dyn FnMut()> = Box::new(move || {
        Vec::new().into_iter().map(|d| logic(d));
    });
}

I saw this happen:

> it compiled

Clippy suggests this closure is a false positive, but if you take its suggestion, it fails to compile:

cannot move out of `logic`, a captured variable in an `FnMut` closure

because you are moving logic into the box, instead of moving a closure that references logic into it

playground: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=7d085db100dcb0f863e3e6f72dd3523a

Version

rustc 1.57

Additional Labels

No response

[rben01]

I think the correct fix-it here is to insert a (mutable) reference to the closure? So that |d| logic(d) becomes &mut logic (and not just logic, which is the current fix-it) if L: FnMut(...) and &logic if L: Fn(...). And logic only if L: FnOnce(...)

Also given that this is quick-fixable, I think the false-positive label should be removed and suggestion-causes-error applied. This is a true positive lint, but the wrong solution is applied when a correct one exists.

[guswynn]

@rben01 I think &mut <a function name> is a bit of a weird construct, and clippy pushing beginners towards that is probably not the best

[rben01]

@guswynn In my opinion, if a beginner is already using the FnOnce, Fn, and FnMut traits, it would be both simplest and most correct for them to learn that (and why/how) those traits are nothing more than the callable analogs of T, &T, and &mut T, respectively. If anything, I find it more confusing that |d| logic(d) is magically FnMut — you sort of have to just know that unlike other expressions, closures only move variables into themselves if written with the move keyword. It's much simpler to me (and beginners?) to reason that logic captures its state by mutable reference and so, transitively, can only be used by mutable reference itself.

Also &mut logic directly address the compiler message you get when you try to use logic.

[guswynn]

@rben01 hmm, I suppose I haven't thought of that parallel, that is fairly convincing!

[pervognsen]

I am constantly running into this when browsing existing codebases. I'm personally in favor of the &mut f fix (rather than disabling the redundant_closure warning in these cases) since that's effectively what the wrapping closure is already doing when borrowing the FnMut, and it's also what rustc recommends as a borrow checker fix. Example:

fn binary_search_by<T>(
    deque: &VecDeque<T>,
    mut cmp: impl FnMut(&T) -> Ordering,
) -> Result<usize, usize> {
    let (front, back) = deque.as_slices();
    match back.first().map(cmp) { // <- rustc: consider mutably borrowing `cmp`: `&mut`
        Some(Ordering::Equal) => Ok(front.len()),
        Some(Ordering::Less) => {
            let shift = |i| i + front.len();
            back.binary_search_by(cmp).map(shift).map_err(shift)
        }
        _ => front.binary_search_by(cmp),
    }
}

The same issue also exists for Fn wrappers (in which case the corresponding fix is a non-mutable reference) but Fn is less commonplace which is presumably why there are fewer reports for that.

[gilescope]

The &mut and & fix suggestions would be good as they are (as pointed out above) things that would not usually occur to rust devs so it's helpful to have that as a suggestion.

[tbillington]

@guswynn In my opinion, if a beginner is already using the FnOnce, Fn, and FnMut traits, it would be both simplest and most correct for them to learn that (and why/how) those traits are nothing more than the callable analogs of T, &T, and &mut T, respectively. If anything, I find it more confusing that |d| logic(d) is magically FnMut — you sort of have to just know that unlike other expressions, closures only move variables into themselves if written with the move keyword. It's much simpler to me (and beginners?) to reason that logic captures its state by mutable reference and so, transitively, can only be used by mutable reference itself.

Also &mut logic directly address the compiler message you get when you try to use logic.

This was confusing at first (Why do I need to make my Fn(A) -> B variable mut?) but I didn't know that's what closures were doing, so it was actually a great learning opportunity. Could this be the clippy "fallback" option if the first fix is not valid? Including a similar explanation.