lint suggestion: unnecessary_move

[sgued]

What it does

Lints against uses of move before a closure or async block that captures nothing by reference. 

Advantage

This is likely the result of a mistake from the implementer, which does not have the proper intuition about how closure borrow their contents. Sometimes it can be hard to reason about it.

For example the developer might not realise that a capture that is passed by value is always captured by value.

Drawbacks

No response

Example

    let a = String::new();
    let closure = move || {
        drop(a);  
    };

Could be written as:

    let a = String::new();
    let closure = || {
        drop(a);  
    };

Because the String is always captured by value, since it needs to be passed by value to drop.

[kpreid]

This is likely the result of a mistake from the implementer, which does not have the proper intuition about how closure borrow their contents. Sometimes it can be hard to reason about it.

I think this is not a good principle to follow. A programmer should not be obligated to understand every nuance of the language's DWIM behaviors; they should be able to follow simpler principles which produce the right answer, where that does not harm readability, and move's behavior is simpler to describe (move/copy everything) than non-move (borrow or move according to usage sites).

Furthermore, this is at odds with reasonable consistency principles, like “always write spawn(move || { ... }), not spawn(|| { ... }), because either it's necessary or it's moot”. I'd rather read

spawn(move || job_1(...));
spawn(move || job_2(...));

than

spawn(move || job_1(...));
spawn(|| job_2(...)); // this one has only by-move arguments

I understand that this is being implemented as a pedantic lint and therefore not enabled by default. Still, I think this goes beyond pedantic territory into “arguably the opposite of this is better” — it's putting a greater obligation on the programmer to comprehend every detail of their program and put it in a form that is only optimal in the direction of “fewer tokens”, not “clearer”. Readability and good style is not just about using fewer tokens.

Please consider putting this in the restriction category (it even has an obvious possible opposite restriction lint, of "write all closures as move closures"), not adding it at all, or at least putting the lack of consistency as a documented caveat.

[dnbln]

I understand that this is being implemented as a pedantic lint and therefore not enabled by default. Still, I think this goes beyond pedantic territory into “arguably the opposite of this is better” — it's putting a greater obligation on the programmer to comprehend every detail of their program and put it in a form that is only optimal in the direction of “fewer tokens”, not “clearer”. Readability and good style is not just about using fewer tokens.

Please consider putting this in the restriction category (it even has an obvious possible opposite restriction lint, of "write all closures as move closures"), not adding it at all, or at least putting the lack of consistency as a documented caveat.

When I initially added it, I wasn't sure of which category would fit it better, as it seems like a candidate for both pedantic as well as restriction, although I went with pedantic because of the "lints which are rather strict" part in the description. I'd be happy with changing it to restriction if it would be a better fit.

[sgued]

It definitely doesn't make sense as a warn by default lint.

Restriction seems fitting.

The use case that made me think of that was when trying to force a closure to not implement Copy to make sure it's not called twice (in addition to FnOnce).

A case where this lint would trigger:

fn testing<T:Copy>(value: T) {
    todo!()
}

fn main() {
    let prevent_copy = String::new();
    testing(move || {
        let _ = prevent_copy;
        println!("Copy");
    })
}

It compiles, but I initially expected it wouldn't.

If you replace let _ = with let _a it did what I expected it to do.

Maybe this is more about let _ =   and _ = than about the move keyword thinking about it. There could be a suspicious lint for closures that have reference something only through let _ = or_ = ` and therefore don't actually capture it.