-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Add must-use-output
attribute
#3773
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
Open
Kixunil
wants to merge
5
commits into
rust-lang:master
Choose a base branch
from
Kixunil:must-use-output
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
d51e8a8
Add initial version of `must-use-output`
Kixunil 0b18cb1
Update the `must-use-output` RFC with PR number
Kixunil 17253d1
Fill in the start date of RFC 3773
Kixunil 49b5c10
Stylistic improvements/clarifications to RFC 3773
Kixunil 073b089
Address most review comments of 3773
Kixunil File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,123 @@ | ||
- Feature Name: `must-use-output` | ||
- Start Date: (fill me in with today's date, YYYY-MM-DD) | ||
- RFC PR: [rust-lang/rfcs#0000](https://github.com/rust-lang/rfcs/pull/0000) | ||
- Rust Issue: [rust-lang/rust#0000](https://github.com/rust-lang/rust/issues/0000) | ||
|
||
# Summary | ||
[summary]: #summary | ||
|
||
Allow adding `#[must_use_output]` attribute on function paramters that triggers a warning if the function was called with a value that is later not used (only dropped). Also add the attribute to relevant library functions such as `Vec::push`. | ||
|
||
# Motivation | ||
[motivation]: #motivation | ||
|
||
Today, if you write this code in Rust: | ||
|
||
```rust | ||
fn main() { | ||
let mut x = 0; | ||
x += 42; | ||
} | ||
``` | ||
|
||
you'll get a warning about unused write. It's likely that either you forgot to use the value of `x` or you wrote a needless operation that has no meaningful effect on the program. | ||
|
||
However this code: | ||
|
||
```rust | ||
fn main() { | ||
let mut vec = Vec::new(); | ||
vec.push(42); | ||
} | ||
``` | ||
|
||
has a similar problem and yet it doesn't trigger a warning. This can be even dangerous in some situations. For instance, if the `Vec` is collecting multiple errors (to report messages similar to `rustc`) and the code is supposed to check if the `Vec` is empty before proceeding. | ||
|
||
In some cases there are even multiple arguments that should be accessed after a call to a function. For instance `core::mem::swap` is almost always useless if both arguments are not accessed - if none of them are it only changes the drop order which doesn't matter in the vast majority of the code, and if only one of them is it's better to use a simple assignment rather than swap. | ||
|
||
Note also that some functions may be useless to call even when they take in truly immutable (`Freeze`) references. For instance `clone` is wasteful if the original is not accessed. | ||
|
||
# Guide-level explanation | ||
[guide-level-explanation]: #guide-level-explanation | ||
|
||
You may use the `#[must_use_output]` attribute in front of any function argument with a reference type (`&T` or `&mut T`) to indicate to the caller that calling the function is useless or dangerous if the referenced value is not later accessed, e.g. reading it to check status. If the caller doesn't access the value later in the function a warning will be emitted. | ||
Kixunil marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
As an example, this code: | ||
|
||
```rust | ||
fn main() { | ||
let mut vec = Vec::new(); | ||
vec.push(42); | ||
} | ||
``` | ||
|
||
will emit a warning saying "`vec` is not used after the call to `push`" because `Vec::push` has `self` marked with this attribute: | ||
|
||
```rust | ||
impl<T> Vec<T> { | ||
pub fn push(#[must_use_output] &mut self, item: T) { /* ... */ } | ||
} | ||
``` | ||
|
||
This is conceptually similar to `#[must_use]` on the function referring to the output value and helps you (or downstream consumers) to catch likely mistakes. | ||
|
||
A common case when this is useful is when the function has no side effects (other than allocation). A typical example is methods modifying collections, builders or similar objects. | ||
|
||
Note that this is currently subject to some limitations: if the reference was passed in as an argument, returned from another function or obtained from a pointer the warning will not trigger. This may change in the future and it may start producing more warnings in some cases. | ||
|
||
# Reference-level explanation | ||
[reference-level-explanation]: #reference-level-explanation | ||
|
||
Whenever a function paramter is marked with `#[must_use_output]` and the caller does not later access the value a warning is emitted similar to using the `+=` operator on integers and later not using the value. | ||
|
||
It is an error to put this attribute on parameters with types other than reference types. Pointers, smart pointers and generics are forbidden mainly for simplicity of initial implementation. They can be implemented and allowed later if it's even possible at all. | ||
|
||
The compiler makes no distinction whether the reference is unique or shared because writes can happen through both, there doesn't seem to be a reason to forbid `Freeze` references and there is at least one case when even `Freeze` reference is useful. | ||
|
||
The standard library functions get annotated with this attribute as well, including but not limited to: | ||
|
||
- Modifications of collections that don't return a value | ||
- `core::mem::swap` | ||
- `Clone::clone` | ||
- `*_assign` methods in `core::ops` traits | ||
- `OpenOptions` and similar builders | ||
|
||
# Drawbacks | ||
[drawbacks]: #drawbacks | ||
|
||
- Adds another atrribute to the language complicating it | ||
- In some cases analysis will fail and perhaps people will have false sene of security | ||
- People could misuse it to attempt to enforce soundness of `unsafe` code (which it cannot do) | ||
|
||
# Rationale and alternatives | ||
[rationale-and-alternatives]: #rationale-and-alternatives | ||
|
||
- We could also simply not do this but the potential mistakes it catches are real. | ||
- The name could be something different and preferably shorter. The name used here was suggested by Josh Triplett and is pretty readable and it can be used before stabilization in the absence of better one. | ||
- Make it a `clippy` lint instead. However not everyone uses `clippy` and the need to communicate which arguments are considered important would diminish its effect. | ||
- Try to somehow analyze the code and not require the attribute. This seems hard and it could lead into libraries accidentally triggering warnings in users code if they change the body of the function. | ||
- Implement a more full feature - e.g. by doing some things in "future posibilities" section. However this feature should be useful even without them. | ||
- Have the atribute on the function instead listing the names of paramters. This could make it nicer to extend to support "or" relationship described in "Future possibilities" | ||
|
||
# Prior art | ||
[prior-art]: #prior-art | ||
|
||
I'm not aware of anything other than the unused write warning and `#[must_use]` attribute which are somewhat related. | ||
|
||
# Unresolved questions | ||
[unresolved-questions]: #unresolved-questions | ||
|
||
- Attribute name. | ||
- Perhaps some limitations are not as difficult as I imagine and they could be lifted right away. | ||
- Should we emit a warning if a function parameter does not have that attribute but is passed to a function with the attribute to encourage propagation? | ||
|
||
# Future possibilities | ||
[future-possibilities]: #future-possibilities | ||
|
||
A custom message could be added just like `#[must_use = "..."]` is already available. | ||
|
||
The limitations above might be lifted somehow. For instance I think that it'd be useful to also emit a warning if the reference was obtained from a function parameter that is itself *not* marked `#[must_use_output]`. | ||
|
||
Have a way to explain to the compiler how smart pointers work so that this can be used on `Pin` as well. | ||
|
||
Have an attribute that can express "or" relationship between parameters and return types. For instance, `Vec::pop` is sometimes used to remove the last element and then pass the vec to something else (e.g. trim `\n` at the end) and sometimes it's used to get the last value and the vec is no longer relevant. Someting like `#[must_use_at_least_one(self, return)]` on function could handle this case. |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
This is true, but I don't think it's on-topic for this RFC, and addressing it would need some additional care to avoid the annoying ripple-effect problem of "don't write the clone on the last use". Can we drop it from this RFC?
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.
How about we mention it but don't propose to mark it just yet? Maybe some library has a use for it in some other context and I don't think adding code to ban
Freeze
references is worth it.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.
@Kixunil I'm not suggesting adding code to ban it, just that it's something different and may not want to use the same handling. Could you move it to one of the later notes sections (e.g. future work), then, along with the note that this isn't proposing to mark such values?
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.
I'm confused. If we allow it right away and there's a clear use case with
clone
-like functions, which people could write how is it a "future possibility"? Note that while I wroteclone
in the list below, I can simply remove it from the list and reword this to:And put "Add the attribute to
clone
" in "Future possibilities"