let mut option = Some(v);
option.take();
is effectively the same as
let mut option = Some(v);
option = None;
, which is why I think Option::take should have a must_use attribute:
#[must_use = "use `option = None` instead"]
pub const fn take(&mut self) -> Option<T> {
// FIXME replace `mem::replace` by `mem::take` when the latter is const ready
mem::replace(self, None)
}
Related: Is "must_use" really needed for Option::insert ? · Issue #130594 · rust-lang/rust · GitHub
In the original PR that annotated Option::insert as #[must_use], it was noted that #[must_use] is used somewhat inconsistently for two unrelated things (pushing users to use a value vs. pushing users towards a more idiomatic approach) and that the same reasoning could apply to Option::take and Option::replace.
Seems reasonable to me, as long as we confirm that that pattern isn't too common.
Echoing the same sentiment expressed in the issue linked above, I personally find using take much cleaner than assigning None. Especially when dealing with a &mut Option that would require dereferencing:
fn takes_mut<T>(opt: &mut Option<T>) {
// rather than deref and assign
// *opt = None;
// I prefer taking
opt.take();
}
Since take does modify the value in place, I don't see that the return value must be used. If it's just a matter of taste, a (pedantic) clippy lint seems more appropriate to me.
I don't think the lint would necessarily push you towards *opt = None. You could equally silence it with drop(opt.take()) or with let _opt = opt.take(), both which are more explicit that the drop order is intentional.
Looking at Option kind of like a collection, we could also add fn clear(&mut self).
I agree #[must_use] is applied inconsistently.
For example, mem::replace() has #[must_use], even though Option::replace() doesn't.
I think it'd make sense for take() to require use of the taken value. It's in the name — it's taking a value, not clearing or resetting the target. Assignment better expresses setting of the value, and doesn't leave it ambiguous whether the previous value was meant to be kept or not.
It has been argued before, but I will just mention that I think it would be reasonable for #[must_use] to be the default for every function that returns anything other than ().
Exceptions could be marked with #[return_value_may_be_unused].
Although I would go even further and say: no exceptions, just drop it if you don't want to use it.
I definitely wouldn't go as far as "no exceptions", but I personally would support "must_use by default, mark things that aren't must_use", over an edition.
Another possible ameliorating incremental step would be "for pub APIs".
I keep hoping for someone to find a reasonable middleground that's easy to understand, so we have both directions too.
Like maybe &mut self isn't #[must_use] by default, but &self is, and there's still, as you said, a #[not_must_use] available for the opt-out if needed.
(That sketch is far from perfect, though, because functions and interior mutability and ...)
Why? Is it more common to want mutable methods as #[not_must_use] than as #[must_use]? I doubt it. Simple rules >> complicated rules.
For example, I went through all the mutable methods on Option:
2 are marked must_use:
x.as_mut_slice()x.insert(y)3 are not but definitely should be because they do nothing if you don't use the return value:
x.as_deref_mut()x.as_mut()x.iter_mut()2 are not but arguably should be because they have better APIs if you don't use the return value:
x.replace(y) => x = Some(y)x.take() => x = None3 could maybe be considered borderline useful without using the return value, but it's still weird to use them that way:
x.get_or_insert(y)x.get_or_insert_with(|| expr)x.take_if(|v| expr)Part of the problem is that there's lots of them that return () that shouldn't.
For example, Vec::push is -> () today, but it really ought to be -> &mut T. And no, I don't want to _ = that.
Also, I don't think that it's usually rustc's place to say "looks, it's not a problem at all to call that trivial method and not use it, but I'm going to whine about it anyway" unless there's something specific it should say about it. Like the reason to lint .to_uppercase() is not that it's a problem to call it, but that you might want an in-place .make_uppercase() instead.
I'd rather "hey, you didn't need this call" was R-A's problem.
I've definitely ended up using this without the return value a few times. If we made it must_use I'd want us to add an Option::insert_with with no return value.
For std specifically a warning could be added to every edition, as long as I can quick fix
#![allow(assume_std_uses_must_use_everywhere)]
For all other libraries it would be really nice to be able to enable this behavior even in pre-edition-2015 code, but the obvious
// enabled by default in edition 2045
#![feature(pretend_everyone_uses_must_use_everywhere)]
perhaps does not have a precedent.
IIRC rustc has a policy that lints should be emitted for (likely) programmer error, not for stylistic reasons.
the one exception i can think of is non-uppercase constants, however those have a genuine reason, as constants can be used in patterns, and this can cause very confusing errors.
IMO stylistic stuff generally belongs in rust-clippy or similar.
The point of #[must_use] is that it represents likely programmer error. The cases we're debating here lie on the fuzzy line between "likely programmer error" and "style".
I'm not convinced anyone would ever use Option::take erroneously instead of... I'm not even sure what possible confusion could happen here. An example of the type of code we're trying to lint against would probably be helpful.
I don't think the issue is particularly relevant to misuse of Option::take. Rather, the issue is that adding #[must_use] to every relevant place is a game of whack-a-mole, and the whole thing should maybe be solved all at once in a very general fashion.
Most of the pain in rust of not having default must_use is already fixed by the fact that returning Result acts as "must use" without the annotation. I still believe it is worth fixing overall.
this is because Result itself is must_use.
If we flipped polarity, () would be a #[may_discard] type. Is there any other type which is a discardable fn result more often than it is something that should be kept? My first heuristic would be needs_drop, since the logic is that if it takes effort to drop, it took some effort to construct, and it'd be better to use API which doesn't construct the value if you don't need it.
Functions where #[must_use] is the most appropriate are functions where discarding the result means discarding the work done by the function and that there's a better option in that case. I concur that this interpretation feels more like a style/efficiency lint than a correctness (or footgun/pitfall) lint.