`#[must_use]` for `Option::take`

language design
21

In my experience it is almost always a bug to ignore the result, with the exception of some specific cases. In a new language (or in a new edition) it could make sense to reverse the logic. Some specific cases I can think of:

  • () as mentioned
  • Insert / remove operations on collections and containers that return information about the previous state (often you don't care)

And that is about it, most of the time not caring is a bug: either the value should be used (correctness) or it is a left over thing from refactoring and no longer needed (efficiency). And Rustc cannot really tell those two cases apart, so it should assume the worst (safest) and treat it as a correctness issue.

3 Likes
22

I'll note that there's long been a https://doc.rust-lang.org/rustc/lints/listing/allowed-by-default.html#unused-results that people can turn on if they really want to told about everything (except, I think, ()s).

I've never seen any code which turns that on, though, which says to me that it's less true than often asserted that people really want to be told about everything.

5 Likes
23

Didn't know that existed. I'll give it a try and see how usable it is. I suspect that is the issue here: no one knows about it. But I could be wrong.

UPDATE: I tried this on two of my projects and the option isn't really usable or ergonomic as is. Here is a breakdown of what I found:

  • Insert/remove/update from various containers was as I predicted a case of wanting to ignore the results.
  • A case I didn't think of is things like read_to_end()? where that returns a result (handled by the ?) of the size read. Unlike normal read, where you really do need to handle the size, for many use cases we don't care other than "we read it all". The same applies to std::io::copy and std::fs::copy.
  • Builders taking and returning &mut Self. If you don't use these in the chained style, this lint will make you suffer.
  • Lots of reports from inside the guts of proc macros (specifically those for the Rune embeddable scripting language in my case). I have no idea if these are false positives or real issues though, probably the former.

Did I find any actual bugs? One, where a function really didn't need to return anything any longer (so efficiency group, except all it returned was a bool, so hardly that either).

As is I don't think this is a usable lint in Rust. All of that (apart from the proc macro) could be worked around by explicit _ = if you were determined enough. But I do think that in a different language that made different decisions from the start (making warning opt out per function / type) this could have worked quite well.

4 Likes
24

Note that #[must_use] on fallible functions is not airtight. It warns when the error is ignored, but does not warn when you handle the error with ? but ignore the result:

#[must_use]
fn foo() -> Result<i32, ()> {...}

foo()?; // no warning
3 Likes
25

I don't think making #[must_use] the default (or even just overusing it) is a good idea. I'd rather have a few false negatives than have many false positives that'll desensitize us to the cases that really need it.

Today, if I see let _ = foo();, it tells me something - it tells me that foo() was returning something that was generally important but in this particular case the programmer who wrote that statement concluded that in this particular case it's not important. But if #[must_use] was the default, I'd lose that information. I'll gain the information that foo() was returning something, but that's not an equal trade - it'll become too common an no longer scream that the returned value is generally of importance. I will not bother trying to understand why the boy cried wolf the value was discarded this time, and just ignore it.

Even worse - foo() was returning something that can be used but can also be safely discarded. That's the usual case with most functions. Now, say that we make a change where foo() can fail. To we change it's return type to a Result. Today, since #[must_use] is not the default, the calls to foo() that don't use the value are just foo(); - so Rust will yell at us for not using the results, and refer us to all the lines where we need to handle the error. But if #[must_use] was the default, these calls would be let _ = foo(); - which will just as easily swallow the Result as they did the original return value. We'll get no warning.

6 Likes
26

Is it really? How often do you define a function with an ignorable return value that isn't ()?

I personally suspect (but have not actually gathered any data) that examples like HashMap::insert() are common to use and therefore salient, but that defining such functions is actually quite uncommon[1] and it would not be a large burden to annotate such functions with a #[may_discard] (or, preferably, a more precise mechanism that lets you express "this returns a Result<bool> and you may discard the bool but not the Result”). I entirely agree with you that it would be bad if programs came to include a lot of let _ =, but I think it is possible to avoid that.

  1. except for &mut self-style builder methods ↩ī¸Ž

6 Likes
27

I said "can be safely discarded" - which is a lower hurdle that what you are probably thinking of.

Say you have a function that calculates something:

fn big_heavy_calc() -> i32;

So you want to run the calculation and send it to the server:

big_heavy_calc();
send_to_server(result_of_big_heavy_calculation);

Do we need a default #[must_use] to realize the result of the function was discarded? No! We call it because we wanted to use the value, which means that not being able to use the value is a bug that's going to surface immediately.

#[must_use] is good for values that you don't need for whatever it is you are trying to program - but you should handle them anyways. Like the Result returned by send_to_server. I don't need it for the logic, but ignoring it means that I'll be ignoring errors - which makes them not "safe to be discarded".

BTW - here are some functions in the wild that return values that really are not (usually) needed. Some of them return Result/Future, which are needed, but the value inside is not needed so I still think they server as good examples:

But again - even if the value is always needed, that doesn't mean that not having it linted is going to hide a bug. Even if not using it will cause a bug - that bug will be exposed as soon much sooner than what the hurdle for #[must_use] needs to be (in my opinion)

3 Likes
28

Slightly off topic, but you really don't want to ignore the returned value from Write::write without thinking it through. A successful return only guarantees that a single byte has been written, so you need the returned usize to know whether most of your input has been discarded. (Or you can use write_all and not worry about this stuff.)

5 Likes
29

What is the harm in getting the warning in this case? Here it's clearly a bug to ignore the return value, so it wouldn't be a false positive.

The potential bug that the warning will save you from is when you forget the send_to_server call:

big_heavy_calc();

instead of:

send_to_server(big_heavy_calc());
1 Like
30

I was just looking at the example and did not consider this. But that actually supports my point. Currently we can't mark with #[must_use] a method that returns Result in a way that will penetrate the error handling (technically we could use a newtype, but that would be a breaking change). But suppose we could. Since I was aware that write returns a usize but ignored it because I thought that's just an artifact of the low level API and the Result was already covering my bases - wouldn't I (and many others) would still make the same mistake if #[must_use] was the default and just swallowed it?

Ideally I'd want #[must_use] to not be the default, and somehow mark it so that f.write(&buf)?; would yell at me for not using that value. That will be an indicator for "this is not a regular return value, this is a value you actually need to pay attention to". If #[must_use] was the default, that would be like looking at some highlighted words in a page where all the text is highlighted because "it's all important".

31

That was not an example for a false positive. That was an example for just how mild the the false negatives are. Yes, if you wouldn't call send_to_server at all that would be a bug - but since you'll be expecting to see the result on the server that bug would be surfaced immediately. The lint will not be offering much protection - not enough to justify the cost of the actual false positives.

1 Like
32

I think this function is bad API. It should have been called HashMap::replace, it's just like Option::replace. insert should panic or return an error on collision.

1 Like
33

You could say that about almost any bug or any warning. "It will be caught immediately by integration tests". The whole point of a type system is to catch errors at compile time rather than by testing.

7 Likes
34

There is a significant qualitative difference between the two types of bugs, though, and thus in the value of catching the bugs earlier.

If you omit send_to_server, then the code does not perform its intended function, and the fact that it's faulty is obvious. This is a relatively low-value bug to catch at compile-time, because it will be caught very quickly in testing, since it is a deterministic and predictable failure.

In contrast, failing to consider what happens when a write to a network socket fails is a high-value bug to catch at compile-time; it is very hard for a test to cause a write to a network socket to fail at a precise point in the code. Thus, a bug caused by a failure to consider this is unlikely to be caught in testing - and even if it is caught in testing, it's likely to be a non-deterministic failure.

Given this difference in value, we should bias the type system towards catching bugs that are hard to find in testing, and away from catching bugs that would show up in even cursory testing. This way, we minimise total effort involved in creating a bug-free program - there's no point making it take an extra 8 hours effort to have all bugs caught at compile time in order to save one hour of effort catching the bugs in automated tests, for example.

8 Likes
35

No - I can only say it about happy-flow bugs. There is a world of difference between "you should run the code at least once to see how it works in practice" and "you should write an extensive suite of tests that checks every possible condition".

5 Likes
36

I just used Option::get_or_insert() to update an option if and only if it is None. I do not need the existing value in my case and didn't want to wrap it inside an if option.is_none() {}, since that would require me to access the same option twice.

    pub fn set_status(&mut self, status: Status) {
        match status {
            Status::Pending => {}
            Status::Running => {
                self.started.get_or_insert(SystemTime::now());
            }
            Status::Completed | Status::Failed(_) => {
                self.finished.get_or_insert(SystemTime::now());
            }
        }

        self.status = status;
    }
37

To this point, I would expect that in a world where #[must_use] was the default since the beginning hash maps would have two methods, one of which would return the old value and one which just returns () (panicking or otherwise).

I know. I just mean that something like fn f() -> Result<...> has no extant annotation, but gets that behavior anyway.

1 Like
38

What about defaulting to #[must_use] being on for any functions which meet all of the following requirements (and allowing manual #[must_use] as currently used for cases which don't meet the requirement):

  1. The function return type is not () (no point in doing anything with the return value).
  2. No arguments are behind exclusive (mutable) references.
  3. No arguments with interior mutability (e.g. Mutex) are behind shared references.

If all three are met, then the function probably does nothing other than returning a value you should use, but if requirements 2 and 3 are not met, then the caller might just be interested in the function for its change to the mutable state of its arguments.

Obviously, there will be exceptions both ways (e.g. what if a function takes an Arc by value and manipulates the pointed-to value, producing changes to other systems holding copies of the Arc?), but this seems like a reasonable set of states in which to by-default assume #[must_use] should apply, which mostly gets it right. This gets the &mut self -> &mut Self builder pattern right, and data structure operations.

39

this is sort of a thing in a lot of C old apis, returning a local variable that's only useful only 10% of the time.

also you could use IndexMut instead of insert

40

This applies to all expressions, not just function results, so it does catch other things mentioned like f.write(..)?. Maybe that should be split into fine-grained lints -- unused_try_results etc.

Or that particular case could be made transitive, so if the Try type is #[must_use], then its ? output is considered #[must_use] too. Then that would apply to ? on Result but not Option.