Allow #[must_use] on functions, rather than just types. Mark Result::{ok,err} #[must_use].

[huonw]

Rendered.

[Gankra]

s/were/where

[Gankra]

Is it within scope for this RFC to try to settle on a convention?

[mahkoh]

-1. I, and most code-bases, use ok() to ignore Result.

[huonw]

As stated in the RFC, there's not a huge amount of evidence for "most".

[mahkoh]

You know what they say: Absence of evidence is not evidence of absence.

[eddyb]

👍 Seeing .ok() anywhere is still quite confusing for me.
.ok().unwrap() makes me wonder what is wrong with their error type that results in .unwrap() not being used.
And foo().ok(); was "obviously" meant to be used and the author must have forgotten to do something with it.
Very rarely have I seen an use I consider legitinately, and even then a match or if-let would be clearer.
@nikomatsakis I know you've been using Result in rustc algorithms a lot lately, what are your thoughts on the matter?

[huonw]

@mahkoh I have looked, I have found evidence of absence. If you can find a wealth of libraries from a variety of authors that use .ok() for ignoring Results the position of the RFC can be reconsidered, but, at the moment, using .ok(); for this purpose seems to be quite unidiomatic (or, at least, uncommon).

Even then, I would prefer having an explicit .ignore() method than abusing .ok() for this purpose, since the former clearly states what it is for while the latter has the problems described in the motivation section.

[mahkoh]

I have looked, I have found evidence of absence.

That's great! I'm looking forward to your analysis of most code-bases on crates.io. Btw: Looking at two trees and not finding any squirrels does not extrapolate to "there are no squirrels in the forrest."

If you can find

Let's not try to move the burden of proof. You're the one proposing a change and making claims about "ok" not being used for this purpose in most code-bases.

Even then, I would prefer having an explicit .ignore()

Then we agree. Unfortunately there doesn't seem to be a consensus for changing ok or adding ignore.

[huonw]

$ ack-grep --type rust 'let _ =' ~/.cargo ~/.multirust/toolchains/*/cargo | wc -l
533
$ ack-grep --type rust '\.ok\(\);' ~/.cargo ~/.multirust/toolchains/*/cargo | grep -v '=.*ok' | wc -l
64

[mahkoh]

Is that your detailed analysis of most code-bases on crates.io?

[huonw]

Clearly not, but the claim "most code-bases use ok() to ignore Result." is looking pretty unlikely.

[mahkoh]

~/rust$ grep "let _ =" $(find -name "*.rs") | wc -l 
17
~/rust$ grep "\.ok()" $(find -name "*.rs") | wc -l 
30

Please focus on proving your claims for now. In case you have forgotten:

Result::ok is occasionally used for silencing the #[must_use]
error of Result, i.e. the ignoring of foo().ok(); is
intentional. However, the most common way do ignore such things is
with let _ =, and ok() is rarely used in comparison, in most
code-bases

[mahkoh]

If the first part of this RFC (#[must_use] on functions) is implemented, then #[must_use] should be removed from Result.

[oli-obk]

I think it's irrelevant whether ok() is used often or not. It has a purpose: converting a Result into an Option. let _ = has no other purpose than ignoring results. A rule of thumb in safety is to keep dangerous and safe actions as far away from each other as possible. Exaggerated: "Don't put the nuclear bomb launch button next to the coffee button".

[oli-obk]

If the first part of this RFC (#[must_use] on functions) is implemented, then #[must_use] should be removed from Result.

That would require every function that returns a Result to be annotated. Some might be missed. The Result type exists explicitly for two things: error reporting and ensuring errors don't get lost.

[mahkoh]

That would require every function that returns a Result to be annotated

That's obviously not true. It only requires functions where the return value must be used to be annotated. E.g. if a function is changed from

fn f() -> Option<T>;

to

fn f() -> Result<T, ()>;

then that does not imply that ignoring the return value suddenly becomes more or less dangerous.

[huonw]

~/rust$ grep "let _ =" $(find -name "*.rs") | wc -l

AFAIK, that is the code of a single author who is clearly very attached to using ok() for this purpose (why else would you be fighting so hard?). Further, ok() has actual non-ignoring uses (when someone wants and uses the returned Option), which your grep does not distinguish. I've been careful to use .ok(); (with the semicolon) to try to focus on the interesting cases.

If "most" code bases use it for this purpose, it shouldn't be too hard for you to find a few on github to point me to.

I did my own random sample of 50 projects on Rust CI, and added a write up, with statistical analysis, to the end of the RFC. I'm now convinced that this will have minimal impact on most code. Unless I see some very strong evidence to the contrary, I'm considering the objection that ok is commonly used for this purpose just outright wrong.

If the first part of this RFC (#[must_use] on functions) is implemented, then #[must_use] should be removed from Result.

I'm slow, and your customarily terse comments are not at all self-explanatory. Please expand.

[fenhl]

See #1812 for some motivation.

[Philipp91]

More motivation: In an FFI, you get lots of c_ints as return values. As far as I can tell, you're in no position to change that (make it a Result<c_int, ()> or something like that) and you cannot flag c_int as #[must_use] in general. Still some of those return values are safe to ignore whereas others aren't. And in some cases (particularly when the FFI is generated from C code that contains some __attribute__ ((warn_unused_result)) attributes) the information is easily available. Allowing #[must_use] would make this information usable and useful.

[joshtriplett]

This came up again in the context of #1812, which led to a proposal to close #1812 and re-open this. Anyone interested in working on a revived RFC?

I would suggest focusing first on support for #[must_use] on functions, without specifying any particular functions to mark with that attribute. We can then handle adding that attribute to specific functions afterward, likely with associated crater runs to determine the impact of doing so.

[withoutboats]

We have found a motivation for this in #1812 - PartialEq::partial_eq. We'd have to check the impact, but it seems clear that foo == bar; is the result of either some confusion on the author's part or a mistyped assignment.

[ssokolow]

@withoutboats Wow... if that doesn't have prohibitive impacts, it'd feel like a direct act of one-upmanship to Python's decision to sacrifice if let's ancestors (eg. if (entry = thing.pull()) {) on the altar of typo-prevention... and that's a good thing.

(And if it is blocked by the stability promise, I think it's good reason to start explicitly maintaining a list of "we wish we'd thought of this sooner" features that people writing "Rust-inspired languages" (ie. Rust 2.0 without the Perl6/Py3k/etc. drama) should consider.)

Also, I think the point made by @Philipp91 deserves more attention.

Even without automatic translation of __attribute__ ((warn_unused_result)), #[must_use] on functions feels like a great way to counterbalance the effects of C's primitive type system on -sys crate internals.

With automatic translation, it would help binding generators to have that "go above and beyond" feel that already draws praise in areas like rustc's error messages.

[withoutboats]

@ssokolow since a) its a warn and not an error, b) you can fix it with let _ = foo == bar;, c) why would anyone ever write foo == bar;?, I'm optimistic that the impact will be small and acceptable.

We'd need an implementation and a crater run. I agree with @joshtriplett that we should accept & implement this without specifying which functions to apply it to, and then just do crater runs on applying it things (IMO without RFCs but YMMV).

[jmacdonald]

I'd love to see this, especially as a means to make a new ignore method on Result the idiomatic way to suppress these warnings. The syntax is much more intention revealing than ok() and let _ = ...;, and feels like a great fit, especially for Rust applications, where typical error propagation practices may not be the best fit (and the very existence of these other idioms seems to hint at a need for this sort of syntax).

[Thiez]

So you would especially like to have this feature so that a new method could be introduced specifically to avoid it? That doesn't sound very convincing to me 😛

What exactly would make foo().ignore(); more intention-revealing that let _ = foo();? At first sight it looks more obvious, but let _ = ... means "ignore" everywhere, while .ignore() would mean ignore only on Result. So it seems to me that the let _ = ... form is actually more intention revealing, because it means the same thing in all possible contexts.

[jmacdonald]

No, the benefit of this feature is discouraging the use of ok()/err() to suppress must-use warnings (or warning about it as an unintended side-effect of their usage). 😄

As for the let _ = ... idiom, I'd argue that it's just the underscore that means ignore, and that its use is best suited to individual positional arguments, tuple element destructuring, or match branches that you'd want to ignore (where a placeholder must exist). There's already an idiom for discarding a return value from an expression: the semi-colon! Introducing let to bind the result to a discarded variable feels odd, at least to me. If that's the idiom we want to encourage, it'd be great to add a reference to it in the Error Handling doc section, or in the upcoming book.

Great point about the ignore() solution only applying to Result, though; you'd need something that applies to any type or function that could raise this warning.

[withoutboats]

I'm intrigued by the idea of ignore() instead of let _ = , because I think its much clearer, but it seems like its an idea to consider separately from this RFC.

This RFC needs some edits to get it into the current idea behind this proposal:

• Remove the references to Result::ok and Result::err (except maybe as a possible motivation)
• Add the partial_eq example to the motivation.

However, @huonw is no longer a contributor, even if someone else makes the edits I don't think we can get them onto this branch without his merging them. I don't know what the procedure here is, we may need to open a new PR after all.

If anyone wants to volunteer to make the edits (possibly @crumblingstatue ?) we could probably get this into FCP soon; based on the discussion here and on #1812, this seems pretty well-discussed & without outstanding issues.

[Wyverald]

There's already an idiom for discarding a return value from an expression: the semi-colon!

Isn't that exactly the source of mistakes, though? (because the semi-colon is easily used to discard return values that we normally wouldn't want to discard)

A more radical way to look at this, IMO, would be to enforce that the expression preceding ; has to have type (). This essentially means that nothing can be implicitly discarded by chaining ;.

[arielb1]

A more radical way to look at this, IMO, would be to enforce that the expression preceding ; has to have type (). This essentially means that nothing can be implicitly discarded by chaining ;.

There are several functions that return an "optional" result (IIRC some versions of Vec::push used to return a reference to the new element), and this would make using them hard.

[Wyverald]

Indeed, the "no implicit discard" rule wouldn't work very well with this kind of APIs, but maybe those APIs are the exception instead of the norm, and we should be annotating them with #[can_ignore] or something.

Just to clarify, I'm not pushing for the adoption of this rule -- it would be such a breaking change, after all. But I still think we might be overestimating how often return values need to be discarded implicitly, and if we were to design everything differently, maybe this rule could've been considered.

[iopq]

I also came here from #1812 - it's great motivation. I just saw a bug in Android source code in the form of modem_reset_flag == 0;

I realized Rust does NOT do better in this case

[joshtriplett]

I'd like to avoid derailing this RFC with even stricter proposals. I think this RFC makes sense as a concrete step that would have minimal impact or invasiveness for existing code and projects.

Please, by all means, go ahead and propose a separate RFC (or perhaps a pre-RFC first) on a more universal "never ignore return values" mechanism; at a minimum, that might make sense as an off-by-default lint, even if not an on-by-default one.

[withoutboats]

As I said before, I'd like to fcp this rfc as soon as it's been amended to reflect the current status. If anyone can write up an amendment it would be great.

[nikomatsakis]

Historical note:

A more radical way to look at this, IMO, would be to enforce that the expression preceding ; has to have type (). This essentially means that nothing can be implicitly discarded by chaining ;.

Many of us have been in favor of this in the past, but (imo) we definitively decided against it when we allowed APIs like insert() or push(). The notion of #[must_use] types was intended as the compromise. I do not think we should relitigate that decision but rather stay within the spirit (as this RFC indeed proposes to do) of generally permitting unused values but allowing types/functions to declare when we should warn.

[withoutboats]

Is anyone interested in updating this RFC to reflect the learnings from #1812 ? @crumblingstatue ? @joshtriplett ?

[withoutboats]

Just to make things simpler I'm closing this and making #1940 the PR for tracking this RFC.