-
Notifications
You must be signed in to change notification settings - Fork 1.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Allow #[must_use]
on functions, rather than just types. Mark Result::{ok,err}
#[must_use]
.
#886
Changes from 2 commits
763fd5d
cfc59a6
3b0d1e6
e68a4d5
86245c5
f95ee6c
86e3e77
96c62fa
ca877fb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,140 @@ | ||
- Feature Name: none? | ||
- Start Date: 2015-02-18 | ||
- RFC PR: [rust-lang/rfcs#886](https://github.com/rust-lang/rfcs/pull/886) | ||
- Rust Issue: (leave this empty) | ||
|
||
# Summary | ||
|
||
Support the `#[must_use]` attribute on arbitrary functions, to make | ||
the compiler lint when a call to such a function is ignored. Mark | ||
`Result::{ok, err}` `#[must_use]`. | ||
|
||
# Motivation | ||
|
||
The `#[must_use]` lint is extremely useful for ensuring that values | ||
that are likely to be important are handled, even if by just | ||
explicitly ignoring them with, e.g., `let _ = ...;`. This expresses | ||
the programmers intention clearly, so that there is less confusion | ||
about whether, for example, ignoring the possible error from a `write` | ||
call is intentional or just an accidental oversight. | ||
|
||
Rust has got a lot of mileage out connecting the `#[must_use]` lint to | ||
specific types: types like `Result`, `MutexGuard` (any guard, ina | ||
general) and the lazy iterator adapters have narrow enough use cases | ||
that the programmer usually wants to do something with them. These | ||
types are marked `#[must_use]` and the compiler will print an error if | ||
a semicolon ever throws away a value of that type: | ||
|
||
```rust | ||
fn returns_result() -> Result<(), ()> { | ||
Ok(()) | ||
} | ||
|
||
fn ignore_it() { | ||
returns_result(); | ||
} | ||
``` | ||
|
||
``` | ||
test.rs:6:5: 6:11 warning: unused result which must be used, #[warn(unused_must_use)] on by default | ||
test.rs:6 returns_result(); | ||
^~~~~~~~~~~~~~~~~ | ||
``` | ||
|
||
However, not every "important" (or, "usually want to use") result can | ||
be a type that can be marked `#[must_use]`, for example, sometimes | ||
functions return unopinionated type like `Option<...>` or `u8` that | ||
may lead to confusion if they are ignored. For example, the `Result<T, | ||
E>` type provides | ||
|
||
```rust | ||
pub fn ok(self) -> Option<T> { | ||
match self { | ||
Ok(x) => Some(x), | ||
Err(_) => None, | ||
} | ||
} | ||
``` | ||
|
||
to view any data in the `Ok` variant as an `Option`. Notably, this | ||
does no meaningful computation, in particular, it does not *enforce* | ||
that the `Result` is `ok()`. Someone reading a line of code | ||
`returns_result().ok();` where the returned value is unused | ||
cannot easily tell if that behaviour was correct, or if something else | ||
was intended, possibilities include: | ||
|
||
- `let _ = returns_result();` to ignore the result (as | ||
`returns_result().ok();` does), | ||
- `returns_result().unwrap();` to panic if there was an error, | ||
- `returns_result().ok().something_else();` to do more computation. | ||
|
||
This is somewhat problematic in the context of `Result` in particular, | ||
because `.ok()` does not really (in the authors opinion) represent a | ||
meaningful use of the `Result`, but it still silences the | ||
`#[must_use]` error. | ||
|
||
These cases can be addressed by allowing specific functions to | ||
explicitly opt-in to also having important results, e.g. `#[must_use] | ||
fn ok(self) -> Option<T>`. This is a natural generalisation of | ||
`#[must_use]` to allow fine-grained control of context sensitive info. | ||
|
||
# Detailed design | ||
|
||
If a semicolon discards the result of a function or method tagged with | ||
`#[must_use]`, the compiler will emit a lint message (under same lint | ||
as `#[must_use]` on types). An optional message `#[must_use = "..."]` | ||
will be printed, to provide the user with more guidance. | ||
|
||
```rust | ||
#[must_use] | ||
fn foo() -> u8 { 0 } | ||
|
||
|
||
struct Bar; | ||
|
||
impl Bar { | ||
#[must_use = "maybe you meant something else"] | ||
fn baz(&self) -> Option<String> { None } | ||
} | ||
|
||
fn qux() { | ||
foo(); // warning: unused result that must be used | ||
Bar.baz(); // warning: unused result that must be used: maybe you meant something else | ||
} | ||
``` | ||
|
||
|
||
# Drawbacks | ||
|
||
This adds a little more complexity to the `#[must_use]` system. | ||
|
||
The rule stated doesn't cover every instance were a `#[must_use]` | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. s/were/where |
||
function is ignored (it does cover all instances of a `#[must_use]` | ||
type), e.g. `(foo());` and `{ ...; foo() };` will not be picked up | ||
(that is, passing the result through another piece of syntax). This | ||
could be tweaked. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This paragraph is kinda hard to parse (two asides in the same "sentence") There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Adjusted. |
||
|
||
`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: 2 instances in the rust-lang/rust codebase (vs. nearly 400 | ||
text matches for `let _ =`) and 4 in the servo/servo (vs. 55 `let _ | ||
=`). Yet another way to write this is `drop(foo())`, although neither | ||
this nor `let _ =` have the method chaining style. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is it within scope for this RFC to try to settle on a convention? |
||
|
||
# Alternatives | ||
|
||
- Adjust the rule to propagate `#[must_used]`ness through parentheses | ||
and blocks, so that `(foo());`, `{ foo() };` and even `if cond { | ||
foo() } else { 0 };` are linted. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Any idea how complicated this would be to do? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the easiest thing to do would be to track whether the result of evaluating the current expression is obviously unused - that would propagate down through There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So basically automatic derivation of must_use for all types (if you have a sub-element that has must_use, you are must_use)? any expression doing a no-op on the object will also return a must_use object -> error message is forwarded outside the expression. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @oli-obk it doesn't look like this applies to types. |
||
|
||
- Provide an additional method on `Result`, e.g. `fn ignore(self) {}`, so | ||
that users who wish to ignore `Result`s can do so in the method | ||
chaining style: `foo().ignore();`. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 from me. Hooray for semantics (and easy to grep/lint for once you're ready for production!) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed. I like this solution, though am also in favor of the RFC as a generally useful construct. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Although There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We could add an ignore method to all types (with a blanket impl) that's just a method form of drop. |
||
|
||
# Unresolved questions | ||
|
||
- Are there many other functions in the standard library/compiler | ||
would benefit from `#[must_use]`? | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Its been suggested elsewhere that the comparison operators should be linted thusly. No reasonable code should have Would it be possible to mark a trait's method as must_use, affecting all implementers? |
||
- Should this be feature gated? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo at ina