Hey All. Someone just sent in a PR for one of my open-source crates asking to add the #[must_use] attribute to every functions in the library that returns a non-unit type. Kinda like for a collection:
I can kind of see the point for pure functions like this: If there are no side effects and the return value is ignored, what’s the point of calling the function at all?
That said, it does seem a bit excessive to me. #[must_use] is primarily intended to make sure you actually handle errors, and adding these when ignoring the output doesn’t cause any real problems may lead to error message fatigue.
Looking at the standard library, identical methods on slices are marked as #[must_use]. I'd generally recommend adding these annotations when they make sense, but it's not essential. I'd imagine Clippy should warn about methods that take &self and return a non-unit type anyway.
Yeah, it was the somewhat inconsistent use in the std library that had me post the question here.
For example, the exact examples I posted, len() and is_empty() are marked #[must_use] for slices, but not marked for Vec. I suppose though that Vec may add them at some point to be more consistent? Maybe.
The suggestion that pure functions could/should be tagged makes a lot of sense. But the rest seems pretty vague. Sounds like it definitely needs a more nuanced approach than just tagging everything.
I guess one thing that I haven't tried yet is to just build my own downstream applications that use the library against the new PR branch. If I get a lot of useless warnings, that might answer my own question.
These are outdated. Last year there were a series of PRs to std which made almost a thousand functions #[must_use]. Felt pretty much like every function had #[must_use] dumped on it, although looking at it now plenty more remain. Honestly, I no longer see any pattern. Why are e.g. ptr::offset_from or ptr::cast not marked as #[must_use], but ptr::wrapping_offset is marked?
I'm not a fan of must-using everything. It's noise in the source, and noise at use sites. In my opinion, unless forgetting to use the value is likely to be a bug (i.e. both it's likely and it's likely a bug), adding #[must_use] is just busywork. At that point I wish Rust would just make all functions #[must_use] by default, with an opt-out for functions used for side effects.
It's worth noting that Rust does have the off-by-default option to #![warn(unused_results)] for any non-unit return value. Unfortunately, this is necessarily noisy since it doesn't consider whether the receiver could be mutated.
I personally agree that in hindsight it probably would have been nicer to have #[discardable_result] and warn by default, but in the current world I think it's fair to say #[must_use] should be used if
ignoring the return value is more likely to be a bug rather than just wasteful (and the type isn't itself #[must_use]), or
you're used by practically everyone (i.e. you're std).
I think a lot of this comes from people who use #![warn(clippy::pedantic)] and zealously apply the fixes mechanically without thinking about the reasoning behind it.
Even the notes for must_use_candidate say you don't need to apply it everywhere:
Why is this bad?
Not bad at all, this lint just shows places where you could add the attribute.
In my opinion, the only reason you should use the #[must_use] attribute is if your function returns something implementing Drop and that drop implementation is "non-trivial" (i.e. we have performance implications) or if they need to handle the return value (i.e. because it is a Result and errors should be handled for correctness).
What qualifies will totally depend on the context, but adding #[must_use] to methods like len() or is_empty() seems like a maintenance issue - now I need to either add the attribute to everything, or I'll have a codebase which uses the attribute inconsistently.
My general feeling is that things shouldn't have the attribute unless they have something specific to say.
For example, .split_off(10) mentions that if you don't actually need those items, you want .truncate(10) instead, which is very useful. Similarly, if you have a pair of methods like .normalized() and .normalize(), marking the non-mutating one as #[must_use] with a pointer to the in-place one is very useful to catch people forgetting which one is which.
But the "spam it on everything" is, IMHO, not worth doing, and more an indication of a missing rust feature (like CAD97 said) to do what people actually want.
For a significant period, the language only allowed applying #[must_use] to types, which aligns well with this approach. If following this approach, you shouldn't mark any functions as #[must_use], just the types with nontrivial Drop.
If I recall then F# does that by default. In F# you can not call a function that returns something and not bind it with a let for instance. F# views this always as an over all return for the function you called your function in.
So in F# you must use |> ignore behind the function call.
Writing x.len(); where the len method has no side effects most certainly is a bug. Thus it's justified to mark such method as #[must_use], in my opionion.
I don't think adding [#must_use] for functions like len generally causes "error message fatigue" because it's unlikely you accidentally write x.len(); and if you do, you'd want to be warned about it.
However, as a counter-argument, I see ergonomics when providing library functions/methods. I would hate having to add #[must_use] to every function without side effects. Thus, maybe it's also wrong to rely on such functions throwing an error when the result is ignored.
That said, I think std could go down a special path and consistently use [#must_use] almost everywhere while third party crates are not expected to do so.
Overall a complex situation where I feel like it's difficult to get a consistent picture and/or find a consistent strategy.
I personally would use [#must_use] sparingly unless I would write some libraries fundamental to Rust – assuming there is a common agreement to use #[must_use] on most side-effect-free functions/methods for std (which doesn't seem clear yet).
"must use" seems like the wrong diagnostic for this "bug" though (if the program will function as expected it's not even a bug IMO, just "bad style" and "suspicious").
Should we have a new lint, say, no_effect, for this case (and also a.saturating_add(b);)?
Motivation:
We get a dedicated warning for "This function call has no effect" instead of "Result must be used", which isn't always an appropriate description.
The compiler can automatically tag every method which has no side effects with #[no_effect]. Looking at the prototype is not enough, but the compiler can examine function code to automatically tag those without side-effects, so no need to tag functions like len and saturating_add
Thus, for example, fn into_iter will now be tagged with #[no_effect] while std::iter::Map will keep its #[must_use] attribute (with custom message).
A drawback is that in some cases both #[no_effect] and #[must_use] will apply (in which case the former lint could be omitted). And there will probably be some cases where the distinction is unclear.
I don't see a point in calling a function with no side effect when you don't use its result. Or is there any example where it makes sense?
I.e. my point is: you must-use the result of a function without side effects, as otherwise you're likely introducing a bug in your program. Same as ignoring a Result. To me, [#no_effect] is a special case of [#must_use].
This might introduce extra warnings while developing a program. (Though this also happens with unused variables, so it's nothing unexpected.) Perhaps it's an interesting idea, but not sure.
I would assume that these extra checks add to the compile time, but not sure if that's a huge share.
Because it has a custom error message — though it could be #[no_effect] instead. Yes, the distinction is not always clear.
It's semantics, but I disagree: adding a stray my_list.len(); call to your program isn't a bug, it's just bad style (and an indication that you probably got distracted).
Good point.
The alternatives:
Try to specify more precisely where #[must_use] should be used (see above); this is roughly the same problem (not always clear).
Change stance and specify that #[must_use] should be added to all methods without side effects... this appears to be the ultimate conclusion of the must_use_candidate Clippy lint, though the lint itself warns "Expect many false positives."
Give up: the current status where #[must_use] is applied inconsistently.
Some other mechanism to warn about "ineffective function calls".
But in case of ignoring a Result, there might be good reasons to do so, opposed to ignoring a result of a function with no side effects.
So I would argue the other way around: Ignoring a result of a function with no side effects is always a bug (or at least a waste of computing power), while ignoring a potential error may be done on purpose.
Just like in Java, writing if false { some_code(); } is an error. In Rust, errors are reserved for cases where the compiler doesn't know to continue, which is massively preferable in my opinion (since hacking code up during debugging sessions is common).
Lets not get into wasted computing power; that's pervasive.
As for warnings: yes, they should be issued. But where, and how? Today, (0..10).into_iter(); does not yield a warning because into_iter() does not have #[must_use] and (going by the above) it is not considered desirable to put #[must_use] on all such functions. So, time to discuss alternatives?
#[must_use] warns by default. Sorry even if I said that I consider it to be a "bug" or "mistake" or whatever, I didn't want to imply the compiler should reject this, but could issue a warning.
Anyway, I don't feel unhappy with anything. I'm fine with how things are. That said, I think there could be improvements on tracking side effects and giving extra warnings. But I really have no stakes in that at the moment.
I don't think so, there are many programs that could compile but won't because of the strict type system:
let x = 1.0;
let y = 2;
let sum = x + y; // error[E0277]: cannot add an integer to a float
let iter = [1,2,3,4].into_iter();
let collected = iter.collect(); // error[E0282]: type annotations needed
Both of these cases could be handled by so-called reasonable defaults, i.e. converting an int to a float where necessary automatically or collecting to a Vec by default.
Rust purposefully chooses to not do that, which I think of as a good thing - you have to be explicit about conversions and it won't do "the wrong thing on accident".
So I think it makes more sense to say "The compiler does know how to continue, but it purposefully chooses not to because of correctness / explicitness / ...".
