Option::as_mut followed by Option::take is a foot-gun

[thomaseizinger]

What it does

Warns users that calling take on an Option created by as_mut does not clear the original Option.

It is useless to take an Option that is only holding a reference, one might as well pattern match on it directly.

Advantage

It makes it clearer that the original option is not modified.

Drawbacks

No response

Example

let mut option = Some("foo");
let maybe_foo = option.as_mut().take();

"Calling take on an Option with a reference does not clear the original Option. Remove as_mut or pattern-match on the option directly if you did not intend to clear it."

Could be written as:

let mut option = Some("foo");
let maybe_foo = option.take();

let mut option = Some("foo");
if let Some(a) = option.as_mut() {
    // ...
}

[thomaseizinger]

FWIW, this inspired by a real-world bug: firezone/firezone#7288

[ericwu17]

@rustbot claim

[ericwu17]

Hi Thomas, thanks for reporting this!

It turns out clippy already has a lint that checks for usages of the form option.as_ref().take() which is a similar case to this one (the only difference being mutability). This lint, called needless_option_take will actually assume that the programmer did not intent to modify the original option, but this not true in practice.

I think we should remove the suggestions emitted by needless_option_take, and instead just emit warnings/errors.

Should the lint be moved to the clippy::correctness category, since calling take() on a temporary value is certainly something the programmer did not intend to do? Or are there edge cases I'm not aware of where it makes sense to do this?

[ericwu17]

To be more general, if f is any function that returns some type Option<T> then we should warn against expressions of the form
f().take()
or
f().take_if(predicate)
since these expressions will be calling take() on a temporary value. The second one with take_if should be replaced with filter().

[thomaseizinger]

Should the lint be moved to the clippy::correctness category, since calling take() on a temporary value is certainly something the programmer did not intend to do?

I think for as_mut, it is definitely a correctness thing. Not sure about as_ref.

To be more general, if f is any function that returns some type Option<T> then we should warn against expressions of the form f().take() or f().take_if(predicate) since these expressions will be calling take() on a temporary value. The second one with take_if should be replaced with filter().

I think you are on the money there! I think replace might also be affected? The new value put passed to replace will instantly be dropped if called on an owned Option returned from a function.