From 6d8aaf1e0a0097119e146898f1a39074b62a6c04 Mon Sep 17 00:00:00 2001 From: Lucio Franco Date: Wed, 8 Jul 2020 14:30:55 -0400 Subject: [PATCH 01/11] Draft RFC to add a yield safe lint --- rfc-drafts/yield_safe_lint.md | 89 +++++++++++++++++++++++++++++++++++ 1 file changed, 89 insertions(+) create mode 100644 rfc-drafts/yield_safe_lint.md diff --git a/rfc-drafts/yield_safe_lint.md b/rfc-drafts/yield_safe_lint.md new file mode 100644 index 00000000..7e1a18ca --- /dev/null +++ b/rfc-drafts/yield_safe_lint.md @@ -0,0 +1,89 @@ +# RFC: yield safe lint + +# Summary + +Introduce a `#[yield_unsafe]` lint in the compiler that will warn the user when they unsafely hold an unsafe yield struct across a yield 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 a yield 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 yield 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. + +# Guide-level explanation + +Provide a lint that can be attached to structs to let the compiler know that this struct is unsafe to be held across a yield boundary. + +```rust + #[yield_unsafe] + struct MyUnsafeYieldStruct {} +``` + +This struct if held across a yield boundary would cause a warning: + +```rust + async fn foo() { + let my_struct = MyUnsafeYieldStruct {}; + my_async_op.await; + println!("{:?}", my_struct); + } +``` + +The compiler might output something along the lines of: + + + warning: Holding `MyUnsafeYieldStruct` across the await bound on line 3 might cause side effects. + +Examples 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 ParkingLot. + - `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 during + +This lint will enable the compiler to warn the user that the generated MIR could produce unforeseen side effects. +An unsafe yield struct is some struct that may cause side effects when it is stored internally to the generator. Some examples of this are + +This will be a best effort lint to signal to the user about unsafety of using certain types across yield points. + +(somewhere go into that async fn/generators generate new “code” that is different than what the users sees, this causes the problem) + +Signal incorrect usage of `!Send` types across yield points. + +Examples: + +- `[MutexGuard](https://doc.rust-lang.org/std/sync/struct.MutexGuard.html)` +- https://docs.rs/tracing/0.1.15/tracing/span/struct.Entered.html + +# Reference-level explanation + +**TODO** + +- [ ] Draw ideas from `#[must_use]` implementation + - [ ] Go through generator implementation to understand how values are captured in state machine. + +# 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. + - This is probab + + +# Rationale and alternatives + + +# Prior art + + +# Unresolved questions + +- What should we call a struct that should not be held across yield boundaries? `unsafe` is quite an overloaded term but is somewhat correct in this sense. That said, rust does not consider deadlocks as unsafe so technically the term is incorrect from rust’s definition. +- Better ways to disable the lint rather than using a `#[allow(yield_unsafe)]` at the top of the module? This would disable the lint for the entire module possibly masking other issues. +- What happens in this situation + struct MyType { + item: usize, + lock: MutexGuard // known to be unsafe to use across yield points + } + - Is it the user’s responsibility to add `#[allow(yield_unsafe)]` to the struct? + - Is it possible to “auto implement” the rule to the struct containing any fields marked `[allow(yield_unsafe)]`? + From a usability perspective, the second option is much more desirable, as the user automatically gets this warning for “free”. + +# Future possibilities + + + From dac69800c3e895cd550e761edd577e51fabbd403 Mon Sep 17 00:00:00 2001 From: Bhargav Voleti Date: Mon, 13 Jul 2020 21:15:02 -0700 Subject: [PATCH 02/11] Address a few comments about copy. --- rfc-drafts/yield_safe_lint.md | 28 ++++++++++++---------------- 1 file changed, 12 insertions(+), 16 deletions(-) diff --git a/rfc-drafts/yield_safe_lint.md b/rfc-drafts/yield_safe_lint.md index 7e1a18ca..7aae2bd2 100644 --- a/rfc-drafts/yield_safe_lint.md +++ b/rfc-drafts/yield_safe_lint.md @@ -6,7 +6,7 @@ Introduce a `#[yield_unsafe]` lint in the compiler that will warn the user when # 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 a yield 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 yield 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. +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 a yield 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. # Guide-level explanation @@ -36,10 +36,12 @@ Examples 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 ParkingLot. - - `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 during + - `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. -This lint will enable the compiler to warn the user that the generated MIR could produce unforeseen side effects. -An unsafe yield struct is some struct that may cause side effects when it is stored internally to the generator. Some examples of this are +This lint will enable the compiler to warn the user that the generated MIR could produce unforeseen side effects. An unsafe yield struct is some struct that may cause side effects when it is stored internally to the generator. Some examples of this are: + +- [`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 unsafety of using certain types across yield points. @@ -47,40 +49,34 @@ This will be a best effort lint to signal to the user about unsafety of using ce Signal incorrect usage of `!Send` types across yield points. -Examples: - -- `[MutexGuard](https://doc.rust-lang.org/std/sync/struct.MutexGuard.html)` -- https://docs.rs/tracing/0.1.15/tracing/span/struct.Entered.html - # Reference-level explanation **TODO** -- [ ] Draw ideas from `#[must_use]` implementation + - [ ] Draw ideas from `#[must_use]` implementation - [ ] Go through generator implementation to understand how values are captured in state machine. # 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. - - This is probab - # Rationale and alternatives # Prior art +* [Clippy lint for holding locks across await points](https://github.com/rust-lang/rust-clippy/pull/5439) # Unresolved questions - What should we call a struct that should not be held across yield boundaries? `unsafe` is quite an overloaded term but is somewhat correct in this sense. That said, rust does not consider deadlocks as unsafe so technically the term is incorrect from rust’s definition. -- Better ways to disable the lint rather than using a `#[allow(yield_unsafe)]` at the top of the module? This would disable the lint for the entire module possibly masking other issues. -- What happens in this situation +- Better ways to disable the lint rather than using a `#[allow(yield_unsafe)]` at the top of the module? This would disable the lint for the entire module, possibly masking other issues. +- What happens in this situation?: struct MyType { item: usize, - lock: MutexGuard // known to be unsafe to use across yield points + lock: MutexGuard, // known to be unsafe to use across yield points } - Is it the user’s responsibility to add `#[allow(yield_unsafe)]` to the struct? - - Is it possible to “auto implement” the rule to the struct containing any fields marked `[allow(yield_unsafe)]`? + - Is it possible to “auto implement” the rule for structs containing any fields marked `[allow(yield_unsafe)]`? From a usability perspective, the second option is much more desirable, as the user automatically gets this warning for “free”. # Future possibilities From 031d93ac61306f7f29fa6269226e3f06fc73ad9c Mon Sep 17 00:00:00 2001 From: Lucio Franco Date: Tue, 14 Jul 2020 14:39:51 -0400 Subject: [PATCH 03/11] Rename rfc to deny_await_lint and update text --- rfc-drafts/deny_await_lint.md | 80 +++++++++++++++++++++++++++++++++ rfc-drafts/yield_safe_lint.md | 85 ----------------------------------- 2 files changed, 80 insertions(+), 85 deletions(-) create mode 100644 rfc-drafts/deny_await_lint.md delete mode 100644 rfc-drafts/yield_safe_lint.md diff --git a/rfc-drafts/deny_await_lint.md b/rfc-drafts/deny_await_lint.md new file mode 100644 index 00000000..0679bad1 --- /dev/null +++ b/rfc-drafts/deny_await_lint.md @@ -0,0 +1,80 @@ +# RFC: Deny await lint + +# Summary + +Introduce a `#[deny_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 accross an await boundary. + +```rust + #[deny_await] + struct MyStruct {} +``` + +This struct if held across an await boundary would cause a warning: + +```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. +``` +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 accross 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: + +- [`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. + +# Reference-level explanation + +**TODO** + + - [ ] Draw ideas from `#[must_use]` implementation + - [ ] Go through generator implementation to understand how values are captured in state machine. + + - 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. + +# 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. + + diff --git a/rfc-drafts/yield_safe_lint.md b/rfc-drafts/yield_safe_lint.md deleted file mode 100644 index 7aae2bd2..00000000 --- a/rfc-drafts/yield_safe_lint.md +++ /dev/null @@ -1,85 +0,0 @@ -# RFC: yield safe lint - -# Summary - -Introduce a `#[yield_unsafe]` lint in the compiler that will warn the user when they unsafely hold an unsafe yield struct across a yield 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 a yield 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. - -# Guide-level explanation - -Provide a lint that can be attached to structs to let the compiler know that this struct is unsafe to be held across a yield boundary. - -```rust - #[yield_unsafe] - struct MyUnsafeYieldStruct {} -``` - -This struct if held across a yield boundary would cause a warning: - -```rust - async fn foo() { - let my_struct = MyUnsafeYieldStruct {}; - my_async_op.await; - println!("{:?}", my_struct); - } -``` - -The compiler might output something along the lines of: - - - warning: Holding `MyUnsafeYieldStruct` across the await bound on line 3 might cause side effects. - -Examples 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 ParkingLot. - - `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. - -This lint will enable the compiler to warn the user that the generated MIR could produce unforeseen side effects. An unsafe yield struct is some struct that may cause side effects when it is stored internally to the generator. Some examples of this are: - -- [`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 unsafety of using certain types across yield points. - -(somewhere go into that async fn/generators generate new “code” that is different than what the users sees, this causes the problem) - -Signal incorrect usage of `!Send` types across yield points. - -# Reference-level explanation - -**TODO** - - - [ ] Draw ideas from `#[must_use]` implementation - - [ ] Go through generator implementation to understand how values are captured in state machine. - -# 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. - -# Rationale and alternatives - - -# Prior art - -* [Clippy lint for holding locks across await points](https://github.com/rust-lang/rust-clippy/pull/5439) - -# Unresolved questions - -- What should we call a struct that should not be held across yield boundaries? `unsafe` is quite an overloaded term but is somewhat correct in this sense. That said, rust does not consider deadlocks as unsafe so technically the term is incorrect from rust’s definition. -- Better ways to disable the lint rather than using a `#[allow(yield_unsafe)]` at the top of the module? This would disable the lint for the entire module, possibly masking other issues. -- What happens in this situation?: - struct MyType { - item: usize, - lock: MutexGuard, // known to be unsafe to use across yield points - } - - Is it the user’s responsibility to add `#[allow(yield_unsafe)]` to the struct? - - Is it possible to “auto implement” the rule for structs containing any fields marked `[allow(yield_unsafe)]`? - From a usability perspective, the second option is much more desirable, as the user automatically gets this warning for “free”. - -# Future possibilities - - - From c027fb2ec8115bed98c63159a7929de839eb01a9 Mon Sep 17 00:00:00 2001 From: Bhargav Voleti Date: Thu, 16 Jul 2020 20:53:03 -0700 Subject: [PATCH 04/11] Rename RFC to must_not_await_lint --- rfc-drafts/{deny_await_lint.md => must_not_await_lint.md} | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) rename rfc-drafts/{deny_await_lint.md => must_not_await_lint.md} (95%) diff --git a/rfc-drafts/deny_await_lint.md b/rfc-drafts/must_not_await_lint.md similarity index 95% rename from rfc-drafts/deny_await_lint.md rename to rfc-drafts/must_not_await_lint.md index 0679bad1..c179a685 100644 --- a/rfc-drafts/deny_await_lint.md +++ b/rfc-drafts/must_not_await_lint.md @@ -1,8 +1,8 @@ -# RFC: Deny await lint +# RFC: Must not await lint # Summary -Introduce a `#[deny_await]` lint in the compiler that will warn the user when they are incorrectly holding a struct across an await boundary. +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 @@ -15,7 +15,7 @@ The big reason for including a lint like this is because under the hood the comp Provide a lint that can be attached to structs to let the compiler know that this struct can not be held accross an await boundary. ```rust - #[deny_await] + #[must_not_await] struct MyStruct {} ``` From 50ba6a348211cae809b6e81e41d449de54bb2478 Mon Sep 17 00:00:00 2001 From: Bhargav Voleti Date: Fri, 17 Jul 2020 11:07:09 -0700 Subject: [PATCH 05/11] Updated reference level explanation --- rfc-drafts/must_not_await_lint.md | 21 ++++++++++++++++++--- 1 file changed, 18 insertions(+), 3 deletions(-) diff --git a/rfc-drafts/must_not_await_lint.md b/rfc-drafts/must_not_await_lint.md index c179a685..585bb84f 100644 --- a/rfc-drafts/must_not_await_lint.md +++ b/rfc-drafts/must_not_await_lint.md @@ -12,7 +12,7 @@ The big reason for including a lint like this is because under the hood the comp # Guide-level explanation -Provide a lint that can be attached to structs to let the compiler know that this struct can not be held accross an await boundary. +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] @@ -44,7 +44,7 @@ Example use cases for this lint: - `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 accross an await boundary. +- 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: @@ -55,6 +55,21 @@ This will be a best effort lint to signal to the user about unintended side-effe # Reference-level explanation +Going throuogh the prior are we see two systems currently which provide simailar/semantically similar behavior: + +## 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. + +[linear-types]: https://gankra.github.io/blah/linear-rust/ +[UnwindSafe]: https://doc.rust-lang.org/std/panic/trait.UnwindSafe.html + + **TODO** - [ ] Draw ideas from `#[must_use]` implementation @@ -76,5 +91,5 @@ This will be a best effort lint to signal to the user about unintended side-effe # 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. - + From 7e0787e0ac9b872e027b3fc899eba9ed6255abe9 Mon Sep 17 00:00:00 2001 From: Bhargav Voleti Date: Thu, 27 Aug 2020 22:13:07 -0700 Subject: [PATCH 06/11] More reference level details --- rfc-drafts/must_not_await_lint.md | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/rfc-drafts/must_not_await_lint.md b/rfc-drafts/must_not_await_lint.md index 585bb84f..4d9429ab 100644 --- a/rfc-drafts/must_not_await_lint.md +++ b/rfc-drafts/must_not_await_lint.md @@ -66,8 +66,17 @@ The `#[must_use]` attribute ensures that if a type or the result of a function i ### 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 + +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. This is done as a part of the [typechecking phase][typechk] of the compiler. 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. + [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 **TODO** From 7ce3b3e196fcc7a41e78eac9896083a940fa1c20 Mon Sep 17 00:00:00 2001 From: Bhargav Voleti Date: Fri, 28 Aug 2020 16:18:06 -0700 Subject: [PATCH 07/11] Add notes about type flag optimization --- rfc-drafts/must_not_await_lint.md | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/rfc-drafts/must_not_await_lint.md b/rfc-drafts/must_not_await_lint.md index 4d9429ab..ee313735 100644 --- a/rfc-drafts/must_not_await_lint.md +++ b/rfc-drafts/must_not_await_lint.md @@ -69,21 +69,18 @@ The `#[must_use]` attribute ensures that if a type or the result of a function i ## High level design overview -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. This is done as a part of the [typechecking phase][typechk] of the compiler. 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 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 - -**TODO** - - - [ ] Draw ideas from `#[must_use]` implementation - - [ ] Go through generator implementation to understand how values are captured in state machine. - - Reference link on how mir transfroms async fn https://tmandry.gitlab.io/blog/posts/optimizing-await-2/ # Drawbacks From 0d511488198233d35b1c0f9d7770fd3c66849e5b Mon Sep 17 00:00:00 2001 From: Bhargav Voleti Date: Sat, 19 Sep 2020 14:19:49 -0700 Subject: [PATCH 08/11] Address RFC review feedback --- rfc-drafts/must_not_await_lint.md | 98 +++++++++++++++++++++++-------- 1 file changed, 74 insertions(+), 24 deletions(-) diff --git a/rfc-drafts/must_not_await_lint.md b/rfc-drafts/must_not_await_lint.md index ee313735..50ea6cb0 100644 --- a/rfc-drafts/must_not_await_lint.md +++ b/rfc-drafts/must_not_await_lint.md @@ -6,7 +6,9 @@ Introduce a `#[must_not_await]` lint in the compiler that will warn the user whe # 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. +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. + +One example of these side effects is holding a `MutexGuard` across an await bound. This opens up the possibility of causing a deadlock since the future holding onto the lock did not relinquish it back before it yielded control. This is a problem for futures that run on single-threaded runtimes (`!Send`) where holding a local after a yield will result in a deadlock. Even on multi-threaded runtimes, it would be nice to provide a custom error message that explains why the user doesn't want to do this instead of only a generic message about their future not being `Send`. Any other kind of RAII guard which depends on behavior similar to that of a `MutexGuard` will have the same issue. 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. @@ -15,18 +17,18 @@ The big reason for including a lint like this is because under the hood the comp 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] - struct MyStruct {} +#[must_not_await = "Your error message here"] +struct MyStruct {} ``` -This struct if held across an await boundary would cause a warning: +This struct if held across an await boundary would cause a deny-by-default warning: ```rust - async fn foo() { - let my_struct = MyStruct {}; - my_async_op.await; - println!("{:?}", my_struct); - } +async fn foo() { + let my_struct = MyStruct {}; + my_async_op.await; + println!("{:?}", my_struct); +} ``` The compiler might output something along the lines of: @@ -51,43 +53,91 @@ This lint will enable the compiler to warn the user that the generated MIR could - [`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. +This will be a best effort lint to signal the user about unintended side-effects of using certain types across an await boundary. # Reference-level explanation -Going throuogh the prior are we see two systems currently which provide simailar/semantically similar behavior: +The `must_not_await` attribute is used to issue a diagnostic warning when a value is not "used". It can be applied to user-defined composite types (structs, enums and unions), functions and traits. -## 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. +The `must_not_await` attribute may include a message by using the [`MetaNameValueStr`] syntax such as `#[must_not_await = "example message"]`. The message will be given alongside the warning. -## `#[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. +When used on a user-defined composite type, if the [expression] of an [expression statement] has this type and is used across an await point, then this lint is violated. -### 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. +```rust +#[must_not_await = "Your error message here"] +struct MyStruct {} + +async fn foo() { + let my_struct = MyStruct {}; + my_async_op.await; + println!("{:?}", my_struct); +} +``` -## High level design overview +When used on a function, if the [expression] of an [expression statement] is a [call expression] to that function, and the expression is held across an await point, this lint is violated. +```rust +#[must_not_await] +fn foo() -> i32 { 5i32 } + +async fn foo() { + let bar = foo(); + my_async_op.await; + println!("{:?}", bar); +} +``` -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. +When used on a [trait declaration], a [call expression] of an [expression statement] to a function that returns an [impl trait] of that trait and if the value is held across an await point, the lint is violated. -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. +```rust +trait Trait { + #[must_not_await] + fn foo(&self) -> i32; +} + +impl Trait for i32 { + fn foo(&self) -> i32 { 0i32 } +} + +async fn foo() { + let bar = 5i32.foo(); + my_async_op.await; + println!("{:?}", bar); +} +``` + +When used on a function in a trait implementation, the attribute does nothing. + +[`MetaNameValueStr`]: https://doc.rust-lang.org/reference/attributes.html#meta-item-attribute-syntax +[expression]: https://doc.rust-lang.org/reference/expressions.html +[expression statement]: https://doc.rust-lang.org/reference/statements.html#expression-statements +[call expression]: https://doc.rust-lang.org/reference/expressions/call-expr.html +[trait declaration]: https://doc.rust-lang.org/reference/items/traits.html +[impl trait]: https://doc.rust-lang.org/reference/types/impl-trait.html + +### 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. -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 is a possibility it can produce a false positive warning and it could get noisy. But using the `allow` attribute would work similar to other `deny-by-default` lints. # Rationale and alternatives +Going through the prior are we see two systems currently which provide simailar/semantically similar behavior: + +## 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. + # Prior art From 6b423e0ca89b707534d942ac7d667a9d5f9af2f8 Mon Sep 17 00:00:00 2001 From: Bhargav Voleti Date: Fri, 25 Sep 2020 09:13:18 -0700 Subject: [PATCH 09/11] Address more minor review feedback --- rfc-drafts/must_not_await_lint.md | 20 +++++++++----------- 1 file changed, 9 insertions(+), 11 deletions(-) diff --git a/rfc-drafts/must_not_await_lint.md b/rfc-drafts/must_not_await_lint.md index 50ea6cb0..813699d6 100644 --- a/rfc-drafts/must_not_await_lint.md +++ b/rfc-drafts/must_not_await_lint.md @@ -33,9 +33,8 @@ async fn foo() { The compiler might output something along the lines of: -TODO: Write a better error message. ``` -warning: Holding `MyStruct` across the await bound on line 3 might cause side effects. +warning: `MyStruct` should not be held across an await point. ``` Example use cases for this lint: @@ -116,14 +115,6 @@ When used on a function in a trait implementation, the attribute does nothing. [trait declaration]: https://doc.rust-lang.org/reference/items/traits.html [impl trait]: https://doc.rust-lang.org/reference/types/impl-trait.html -### 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. - - -[linear-types]: https://gankra.github.io/blah/linear-rust/ -[UnwindSafe]: https://doc.rust-lang.org/std/panic/trait.UnwindSafe.html - - - 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. But using the `allow` attribute would work similar to other `deny-by-default` lints. @@ -138,13 +129,20 @@ This lint goes through all types in `generator_interior_types` looking for `Mute ## `#[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. + +[linear-types]: https://gankra.github.io/blah/linear-rust/ +[UnwindSafe]: https://doc.rust-lang.org/std/panic/trait.UnwindSafe.html + # 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) +* Reference link on how mir transfroms async fn https://tmandry.gitlab.io/blog/posts/optimizing-await-2/ -# Future possibilities +# Unresolved questions - Propagate the lint in nested structs/enums. Similar to the use case for the `must_use` attribute. These likely should be solved together. From 9b25a4a818f43d9116e9722d0ad2a9ca94efbacf Mon Sep 17 00:00:00 2001 From: Bhargav Voleti Date: Fri, 9 Oct 2020 13:43:52 -0700 Subject: [PATCH 10/11] Clean up reference section --- rfc-drafts/must_not_await_lint.md | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/rfc-drafts/must_not_await_lint.md b/rfc-drafts/must_not_await_lint.md index 813699d6..be6b6ad1 100644 --- a/rfc-drafts/must_not_await_lint.md +++ b/rfc-drafts/must_not_await_lint.md @@ -47,7 +47,7 @@ Example use cases for this lint: - 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: +This lint will enable the compiler to warn the user that the code could produce unforeseen side effects. Some examples of this are: - [`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) @@ -60,7 +60,7 @@ The `must_not_await` attribute is used to issue a diagnostic warning when a valu The `must_not_await` attribute may include a message by using the [`MetaNameValueStr`] syntax such as `#[must_not_await = "example message"]`. The message will be given alongside the warning. -When used on a user-defined composite type, if the [expression] of an [expression statement] has this type and is used across an await point, then this lint is violated. +When used on a user-defined composite type, if a value exists across an await point, then this lint is violated. ```rust @@ -74,7 +74,7 @@ async fn foo() { } ``` -When used on a function, if the [expression] of an [expression statement] is a [call expression] to that function, and the expression is held across an await point, this lint is violated. +When used on a function, if the value returned by a function is held across an await point, this lint is violated. ```rust #[must_not_await] @@ -87,7 +87,7 @@ async fn foo() { } ``` -When used on a [trait declaration], a [call expression] of an [expression statement] to a function that returns an [impl trait] of that trait and if the value is held across an await point, the lint is violated. +When used on a [trait declaration], if the value implementing that trait is held across an await point, the lint is violated. ```rust trait Trait { @@ -109,11 +109,7 @@ async fn foo() { When used on a function in a trait implementation, the attribute does nothing. [`MetaNameValueStr`]: https://doc.rust-lang.org/reference/attributes.html#meta-item-attribute-syntax -[expression]: https://doc.rust-lang.org/reference/expressions.html -[expression statement]: https://doc.rust-lang.org/reference/statements.html#expression-statements -[call expression]: https://doc.rust-lang.org/reference/expressions/call-expr.html [trait declaration]: https://doc.rust-lang.org/reference/items/traits.html -[impl trait]: https://doc.rust-lang.org/reference/types/impl-trait.html # Drawbacks From 82cb98bb70155b2713d7269c34e7c882f676228e Mon Sep 17 00:00:00 2001 From: Bhargav Voleti Date: Fri, 9 Oct 2020 13:44:56 -0700 Subject: [PATCH 11/11] Fix incorrect heading --- rfc-drafts/must_not_await_lint.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rfc-drafts/must_not_await_lint.md b/rfc-drafts/must_not_await_lint.md index be6b6ad1..71c06097 100644 --- a/rfc-drafts/must_not_await_lint.md +++ b/rfc-drafts/must_not_await_lint.md @@ -119,7 +119,7 @@ When used on a function in a trait implementation, the attribute does nothing. Going through the prior are we see two systems currently which provide simailar/semantically similar behavior: -## Clippy `#[await_holding_lock]` lint +## 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