-
Notifications
You must be signed in to change notification settings - Fork 88
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
Draft RFC to add a must_not_await
lint
#16
Changes from 7 commits
6d8aaf1
dac6980
031d93a
c027fb2
50ba6a3
7e0787e
7ce3b3e
0d51148
6b423e0
9b25a4a
82cb98b
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,101 @@ | ||||||
# RFC: Must not await lint | ||||||
|
||||||
# Summary | ||||||
|
||||||
Introduce a `#[must_not_await]` lint in the compiler that will warn the user when they are incorrectly holding a struct across an await boundary. | ||||||
|
||||||
# Motivation | ||||||
|
||||||
Enable users to fearlessly write concurrent async code without the need to understand the internals of runtimes and how their code will be affected. The goal is to provide a best effort warning that will let the user know of a possible side effect that is not visible by reading the code right away. Some examples of these side effects are holding a `MutexGuard` across an await bound in a single threaded runtime. In this case the resulting generated future will resolve to `!Send` but could still hold the lock when the future yields back to the executor. This opens up for the possibility of causing a deadlock since the future holding onto the lock did not relinquish it back before it yielded control. This can become even more problematic for futures that run on single-threaded runtimes (`!Send`) where holding a local after a yield will result in a deadlock. | ||||||
|
||||||
The big reason for including a lint like this is because under the hood the compiler will automatically transform async fn into a state machine which can store locals. This process is invisible to users and will produce code that is different than what is in the actual rust file. Due to this it is important to inform users that their code may not do what they expect. | ||||||
|
||||||
# Guide-level explanation | ||||||
|
||||||
Provide a lint that can be attached to structs to let the compiler know that this struct can not be held across an await boundary. | ||||||
|
||||||
```rust | ||||||
#[must_not_await] | ||||||
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. Like |
||||||
struct MyStruct {} | ||||||
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. nit: can you un-indent these code blocks? |
||||||
``` | ||||||
|
||||||
This struct if held across an await boundary would cause a warning: | ||||||
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.
Suggested change
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.
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. https://doc.rust-lang.org/rustc/lints/listing/deny-by-default.html They are basically errors that you can disable with |
||||||
|
||||||
```rust | ||||||
async fn foo() { | ||||||
let my_struct = MyStruct {}; | ||||||
my_async_op.await; | ||||||
println!("{:?}", my_struct); | ||||||
} | ||||||
``` | ||||||
|
||||||
The compiler might output something along the lines of: | ||||||
|
||||||
TODO: Write a better error message. | ||||||
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. You can show your custom error message here. The generic default could be as straightforward as " |
||||||
``` | ||||||
warning: Holding `MyStruct` across the await bound on line 3 might cause side effects. | ||||||
``` | ||||||
|
||||||
Example use cases for this lint: | ||||||
|
||||||
- `MutexGuard` holding this across a yield boundary in a single threaded executor could cause deadlocks. In a multi-threaded runtime the resulting future would become `!Send` which will stop the user from spawning this future and causing issues. But in a single threaded runtime which accepts `!Send` futures deadlocks could happen. | ||||||
|
||||||
- The same applies to other such synchronization primitives such as locks from `parking-lot`. | ||||||
|
||||||
- `tracing::Span` has the ability to enter the span via the `tracing::span::Entered` guard. While entering a span is totally normal, during an async fn the span only needs to be entered once before the `.await` call, which might potentially yield the execution. | ||||||
|
||||||
- Any RAII guard might possibly create unintended behavior if held across an await boundary. | ||||||
|
||||||
This lint will enable the compiler to warn the user that the generated MIR could produce unforeseen side effects. Some examples of this are: | ||||||
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. nit: The MIR is more of an implementation detail. It's just the defined semantics of async functions, namely the fact that they suspend execution. |
||||||
|
||||||
- [`std::sync::MutexGuard`](https://doc.rust-lang.org/std/sync/struct.MutexGuard.html) | ||||||
- [`tracing::span::Entered`](https://docs.rs/tracing/0.1.15/tracing/span/struct.Entered.html) | ||||||
|
||||||
This will be a best effort lint to signal to the user about unintended side-effects of using certain types across an await boundary. | ||||||
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.
Suggested change
|
||||||
|
||||||
# Reference-level explanation | ||||||
tmandry marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
|
||||||
Going throuogh the prior are we see two systems currently which provide simailar/semantically similar behavior: | ||||||
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 seems more like it belongs in either the motivation or the rationale and alternatives section, to me at least. |
||||||
|
||||||
## Clippy `#[await_holding_lock]` lint | ||||||
This lint goes through all types in `generator_interior_types` looking for `MutexGuard`, `RwLockReadGuard` and `RwLockWriteGuard`. While this is a first great step, we think that this can be further extended to handle not only the hardcoded lock guards, but any type which is should not be held across an await point. By marking a type as `#[must_not_await]` we can warn when any arbitrary type is being held across an await boundary. An additional benefit to this approach is that this behaviour can be extended to any type which holds a `#[must_not_await]` type inside of it. | ||||||
|
||||||
## `#[must_use]` attribute | ||||||
The `#[must_use]` attribute ensures that if a type or the result of a function is not used, a warning is displayed. This ensures that the user is notified about the importance of said value. Currently the attribute does not automatically get applied to any type which contains a type declared as `#[must_use]`, but the implementation for both `#[must_not_await]` and `#[must_use]` should be similar in their behavior. | ||||||
|
||||||
### Auto trait vs attribute | ||||||
`#[must_use]` is implemented as an attribute, and from prior art and [other literature][linear-types], we can gather that the decision was made due to the complexity of implementing true linear types in Rust. [`std::panic::UnwindSafe`][UnwindSafe] on the other hand is implemented as a marker trait with structural composition. | ||||||
|
||||||
|
||||||
## High level design overview | ||||||
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. Maybe there's precedent for this that I don't know about, but I don't feel that implementation notes like this belong in an RFC. |
||||||
|
||||||
|
||||||
The main body of finding the types which are captured in the state machine for an async block are done during the [typechecking][typechk] phase. From a 10000ft view, generators currently analyze the body of the async block to [build the list of values][resolve-interior] which live across a yield point. We can use this list of types to check whether or not any of them have been marked as `#[must_not_await]`. In order to do so, we can leverage the HIR definition of the types which would include the annotation. | ||||||
|
||||||
The attribute can be found by querying the session by the `DefId` of each of the captured type, and a warning can issued based on whether or not the types captured in the generator have the attribute associated with them. | ||||||
|
||||||
We also have the option of precomputing the presence of an attribute on a type during parsing and storing this information on the type flags for the type. In my opinion this would be the more efficient way of implementing this check as queriying the `Session` object for a large list of types could become an expensive operation. | ||||||
|
||||||
[linear-types]: https://gankra.github.io/blah/linear-rust/ | ||||||
[UnwindSafe]: https://doc.rust-lang.org/std/panic/trait.UnwindSafe.html | ||||||
[resolve-interior]: https://github.com/rust-lang/rust/blob/master/src/librustc_typeck/check/generator_interior.rs#L122 | ||||||
[typechk]: https://github.com/rust-lang/rust/blob/3e041cec75c45e78730972194db3401af06b72ef/src/librustc_typeck/check/mod.rs#L1113 | ||||||
|
||||||
- Reference link on how mir transfroms async fn https://tmandry.gitlab.io/blog/posts/optimizing-await-2/ | ||||||
|
||||||
# Drawbacks | ||||||
- There is a possibility it can produce a false positive warning and it could get noisy. We likely want to allow overriding via some sort of module level `allow` attribute. | ||||||
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. Yes we do (and it should work at the item level as well), leveraging the same mechanism as other deny-by-default lints. |
||||||
|
||||||
# Rationale and alternatives | ||||||
|
||||||
|
||||||
# Prior art | ||||||
|
||||||
* [Clippy lint for holding locks across await points](https://github.com/rust-lang/rust-clippy/pull/5439) | ||||||
* [Must use for functions](https://github.com/iopq/rfcs/blob/f4b68532206f0a3e0664877841b407ab1302c79a/text/1940-must-use-functions.md) | ||||||
|
||||||
# Future possibilities | ||||||
|
||||||
- Propagate the lint in nested structs/enums. Similar to the use case for the `must_use` attribute. These likely should be solved together. | ||||||
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 we should answer this question before stabilizing. If we add this afterwards it'll create unnecessary breakage. 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 we should include this in this change. In my opinion, this would be the best experience for the user of this attribute. 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. If we do that it'll be inconsistent with Strawman: This is about what types are safe to include (transitively or not) in a Future object. 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 was brought up before by @nikomatsakis in an earlier comment. There was a PR which introduced the behavior we are speaking of for #[must_use] but was closed because of crater breakages. That particular conversation was biased towards having the same behavior across both attributes. It would certainly increase the scope of this project especially so if it's a breaking change we might have to ship it as a part of the 2021 edition (if it's happening). 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. Maybe for now we can add an "Unresolved questions" section and add this to 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.
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.
You might want to hint in this section that other kinds of RAII guards may want this (which you talk about below). Someone who only reads to here might think this doesn't have enough use cases for a general mechanism.