mem::replace -> mem::take lint is too aggressive

[benesch]

Clippy will suggest replacing

let x = mem::replace(&mut thing, Thing::default());

with:

let x = mem::take(&mut thing);

This is great! But it will also suggest replacing

mem::replace(&mut thing, Thing::default());

with:

mem::take(&mut thing);

This is not great. The construction using mem::replace in the second case is much clearer IMO as it indicates that the function is being executed for its side effects. The construction using mem::take looks like a mistake.

Would you folks be open to making the lint not apply in the second case?

[flip1995]

So basically only suggest using mem::take when the replaced thing is not assigned to anything?

[benesch]

Exactly. Arguably clippy should suggest *thing = Thing::default() in the latter case.

[Luro02]

This is great! But it will also suggest replacing

mem::replace(&mut thing, Thing::default());

with:

mem::take(&mut thing);

It might be useful to have a clippy lint that suggests replacing the latter with the former, because like you said the latter looks like a mistake (it is harder to read, because there is an implicit replacement of the field with default that is not obvious by the method name).

[taiki-e]

mem::replace(&mut thing, Thing::default());

Given that mem::replace is #[must_use] (rust-lang/rust#71256), the usage that ignores mem::replace's return value is probably not the recommended way.

[benesch]

Yeah, looks like the upstream recommended way is just to assign:

*thing = Thing::default()

[camsteffen]

Since mem::replace was given #[must_use], perhaps the mem_replace_with_default lint should simply ignore cases where the result isn't used. The #[must_use] lint will provide the correct suggestion.

[GabrielBFern]

@rustbot claim