New lint: prevent dropping Results (and other must_use types)

[piegamesde]

What it does

I just found the following code:

drop(env_logger::try_init());

Basically someone got a warning on an unused must_use type (in this case a Result), and thus "used" it … by dropping it. Tada, the warning's gone 🎉.

Categories (optional)

• Kind: clippy::correctness

Drawbacks

None.

Example

drop(crate::try_do_stuff());

Could be written as:

crate::try_do_stuff().unwrap();

If the drop was intentional, it should be:

drop(crate::try_do_stuff().unwrap());

[lebensterben]

This is related to the following lints:

• clippy::correctness::drop_copy
• clippy::correctness::drop_ref

I think these two shall take precedence over the proposed lint, because they are more important.

I think we should only implement the proposed new lint on non-Copy owned types, and call it drop_must_use, in clippy::style as I don't think this is "incorrect".

[piegamesde]

Sounds good. However, I'm think dropping a Result and thus not doing error handling is "incorrect" to me. I can't think of any use case where dropping a Result instead of handling it would be desired semantics. Note that I come from a pure Result perspective and don't know of similar situations with other must_use types.

[ebroto]

I don't think we should suggest unwraping the result as that would change semantics (a dropped error is not unwrapped).

Wouldn't a more idiomatic way to write that be:

let _ = crate::try_do_stuff();

This would apply to non-Result types too. Note that the behavior would be the same, as binding to _ destroys the assigned value at the end of the statement.

[camsteffen]

I think that drop is a good way to ignore a Result because it is explicit. Some would prefer it over let _ = for that reason, but I think the jury is still out on which is more idiomatic. If someone changes code to "fix" a warning without thinking about what the change does, that seems like a different problem to me.