From 2e1d53454f698dfb5635a35933c5fe4ec0f36112 Mon Sep 17 00:00:00 2001 From: Bhargav Voleti Date: Mon, 9 Nov 2020 09:19:51 -0800 Subject: [PATCH 1/5] Adds `#[must_not_await_lint]` RFC --- text/0000-must-not-await-lint.md | 152 +++++++++++++++++++++++++++++++ 1 file changed, 152 insertions(+) create mode 100644 text/0000-must-not-await-lint.md diff --git a/text/0000-must-not-await-lint.md b/text/0000-must-not-await-lint.md new file mode 100644 index 00000000000..fa1a52a2283 --- /dev/null +++ b/text/0000-must-not-await-lint.md @@ -0,0 +1,152 @@ +- Feature Name: `must_not_await_lint` +- Start Date: 2020-11-09 +- 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 + +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 +[motivation]: #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. + +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. + +# Guide-level explanation +[guide-level-explanation]: #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 = "Your error message here"] +struct MyStruct {} +``` + +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); +} +``` + +The compiler might output something along the lines of: + +``` +warning: `MyStruct` should not be held across an await point. +``` + +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 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) + +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 +[reference-level-explanation]: #reference-level-explanation + +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. + +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 a value exists across an await point, then this lint is violated. + + +```rust +#[must_not_await = "Your error message here"] +struct MyStruct {} + +async fn foo() { + let my_struct = MyStruct {}; + my_async_op.await; + println!("{:?}", my_struct); +} +``` + +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] +fn foo() -> i32 { 5i32 } + +async fn foo() { + let bar = foo(); + my_async_op.await; + println!("{:?}", bar); +} +``` + +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 { + #[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 +[trait declaration]: https://doc.rust-lang.org/reference/items/traits.html + +# Drawbacks +[drawbacks]: #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. + +# Rationale and alternatives +[rationale-and-alternatives]: #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. + +### 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 +[prior-art]: #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/ +# Unresolved questions +[unresolved-questions]: #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 af126524175b90d4064ea4548a7e85af82ae4cda Mon Sep 17 00:00:00 2001 From: Bhargav Voleti Date: Thu, 10 Dec 2020 21:59:48 -0800 Subject: [PATCH 2/5] Address more RFC review feedback --- text/0000-must-not-await-lint.md | 42 +++++++++++++++++++++++++++++--- 1 file changed, 38 insertions(+), 4 deletions(-) diff --git a/text/0000-must-not-await-lint.md b/text/0000-must-not-await-lint.md index fa1a52a2283..44a9dc677b3 100644 --- a/text/0000-must-not-await-lint.md +++ b/text/0000-must-not-await-lint.md @@ -1,6 +1,6 @@ - Feature Name: `must_not_await_lint` - Start Date: 2020-11-09 -- RFC PR: [rust-lang/rfcs#0000](https://github.com/rust-lang/rfcs/pull/0000) +- RFC PR: [rust-lang/rfcs#3014](https://github.com/rust-lang/rfcs/pull/3014) - Rust Issue: [rust-lang/rust#0000](https://github.com/rust-lang/rust/issues/0000) # Summary @@ -13,7 +13,7 @@ Introduce a `#[must_not_await]` lint in the compiler that will warn the user whe 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. +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 lock 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. @@ -96,6 +96,26 @@ async fn foo() { When used on a [trait declaration], if the value implementing that trait is held across an await point, the lint is violated. +```rust +#[must_not_await] +trait Lock { + fn foo(&self) -> i32; +} + +fn get_lock() -> impl Lock { + 1i32 +} + +async fn foo() { + // violates the #[must_not_await] lint + let bar = get_lock(); + my_async_op.await; + println!("{:?}", bar); +} +``` + +When used on a function in a trait declaration, then the behavior also applies when the call expression is a function from the implementation of the trait. + ```rust trait Trait { #[must_not_await] @@ -113,6 +133,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 @@ -121,7 +142,9 @@ When used on a function in a trait implementation, the attribute does nothing. # Drawbacks [drawbacks]: #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. +- 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 [`warn-by-default`] lints. One thing to note, unlike the `#[must_use]` lint, users cannot silence this warning by using `let _ = bar()` where `bar()` returns a type which has a `#[must_use]` attribute. The `#[allow]` attribute will be the only way to silence the warning. + +[`warn-by-default`]: https://doc.rust-lang.org/rustc/lints/listing/warn-by-default.html # Rationale and alternatives [rationale-and-alternatives]: #rationale-and-alternatives @@ -149,4 +172,15 @@ The `#[must_use]` attribute ensures that if a type or the result of a function i # Unresolved questions [unresolved-questions]: #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. + +# Common behavior with `#[must_use]` lint + +Both `#[must_use]` and `#[must_not_await]` are [`warn-by-default`] lints, and are applied to types decorated with the attribute. Currently the `#[must_use]` lint does not automatically propagate the lint in nested structures/enums due to the additional complexity that it adds on top of the possible breaking changes introduced in the wider ecosystem + +Automatically propagating the lint for types containing a type marked by one of these attributes would make for a more ergonomic user experience, and would reduce syntactic noise. + +While tradeoffs exist for both approaches, in either case, both lints should exhibit the same behavior. + +The `#[must_use]` lint is being used in stable rust for a long time now(The earliest reference I could find was in the release notes for [1.27]) with existing behavior. + +[1.27]: https://github.com/rust-lang/rust/blob/master/RELEASES.md#version-1270-2018-06-21 From 6532fdcda57b3d360d77c6e7a6af48d10baf511c Mon Sep 17 00:00:00 2001 From: Bhargav Voleti Date: Fri, 29 Jan 2021 13:26:19 -0800 Subject: [PATCH 3/5] Rename feature to must_not_suspend --- ...-lint.md => 0000-must-not-suspend-lint.md} | 26 +++++++++---------- 1 file changed, 13 insertions(+), 13 deletions(-) rename text/{0000-must-not-await-lint.md => 0000-must-not-suspend-lint.md} (84%) diff --git a/text/0000-must-not-await-lint.md b/text/0000-must-not-suspend-lint.md similarity index 84% rename from text/0000-must-not-await-lint.md rename to text/0000-must-not-suspend-lint.md index 44a9dc677b3..9a712a249d4 100644 --- a/text/0000-must-not-await-lint.md +++ b/text/0000-must-not-suspend-lint.md @@ -1,4 +1,4 @@ -- Feature Name: `must_not_await_lint` +- Feature Name: `must_not_suspend_lint` - Start Date: 2020-11-09 - RFC PR: [rust-lang/rfcs#3014](https://github.com/rust-lang/rfcs/pull/3014) - Rust Issue: [rust-lang/rust#0000](https://github.com/rust-lang/rust/issues/0000) @@ -6,7 +6,7 @@ # Summary [summary]: #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. +Introduce a `#[must_not_suspend]` lint in the compiler that will warn the user when they are incorrectly holding a struct across an await boundary. # Motivation [motivation]: #motivation @@ -23,7 +23,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 across an await boundary. ```rust -#[must_not_await = "Your error message here"] +#[must_not_suspend = "Your error message here"] struct MyStruct {} ``` @@ -63,15 +63,15 @@ This will be a best effort lint to signal the user about unintended side-effects # Reference-level explanation [reference-level-explanation]: #reference-level-explanation -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. +The `must_not_suspend` 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. -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. +The `must_not_suspend` attribute may include a message by using the [`MetaNameValueStr`] syntax such as `#[must_not_suspend = "example message"]`. The message will be given alongside the warning. When used on a user-defined composite type, if a value exists across an await point, then this lint is violated. ```rust -#[must_not_await = "Your error message here"] +#[must_not_suspend = "Your error message here"] struct MyStruct {} async fn foo() { @@ -84,7 +84,7 @@ async fn foo() { 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] +#[must_not_suspend] fn foo() -> i32 { 5i32 } async fn foo() { @@ -97,7 +97,7 @@ async fn foo() { When used on a [trait declaration], if the value implementing that trait is held across an await point, the lint is violated. ```rust -#[must_not_await] +#[must_not_suspend] trait Lock { fn foo(&self) -> i32; } @@ -107,7 +107,7 @@ fn get_lock() -> impl Lock { } async fn foo() { - // violates the #[must_not_await] lint + // violates the #[must_not_suspend] lint let bar = get_lock(); my_async_op.await; println!("{:?}", bar); @@ -118,7 +118,7 @@ When used on a function in a trait declaration, then the behavior also applies w ```rust trait Trait { - #[must_not_await] + #[must_not_suspend] fn foo(&self) -> i32; } @@ -152,10 +152,10 @@ 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 -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. +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_suspend]` 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_suspend]` 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. +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_suspend]` 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. @@ -175,7 +175,7 @@ The `#[must_use]` attribute ensures that if a type or the result of a function i # Common behavior with `#[must_use]` lint -Both `#[must_use]` and `#[must_not_await]` are [`warn-by-default`] lints, and are applied to types decorated with the attribute. Currently the `#[must_use]` lint does not automatically propagate the lint in nested structures/enums due to the additional complexity that it adds on top of the possible breaking changes introduced in the wider ecosystem +Both `#[must_use]` and `#[must_not_suspend]` are [`warn-by-default`] lints, and are applied to types decorated with the attribute. Currently the `#[must_use]` lint does not automatically propagate the lint in nested structures/enums due to the additional complexity that it adds on top of the possible breaking changes introduced in the wider ecosystem Automatically propagating the lint for types containing a type marked by one of these attributes would make for a more ergonomic user experience, and would reduce syntactic noise. From 6590ef40210d19098738855fb2f8325ec51a324c Mon Sep 17 00:00:00 2001 From: Bhargav Voleti Date: Thu, 11 Feb 2021 20:53:35 -0800 Subject: [PATCH 4/5] address review --- text/0000-must-not-suspend-lint.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/text/0000-must-not-suspend-lint.md b/text/0000-must-not-suspend-lint.md index 9a712a249d4..70ff2478dae 100644 --- a/text/0000-must-not-suspend-lint.md +++ b/text/0000-must-not-suspend-lint.md @@ -27,7 +27,7 @@ Provide a lint that can be attached to structs to let the compiler know that thi struct MyStruct {} ``` -This struct if held across an await boundary would cause a deny-by-default warning: +This struct if held across an await boundary would cause a warn-by-default warning: ```rust async fn foo() { From 7f3e4519fcbf767004094b94cde8ec6ea3db6715 Mon Sep 17 00:00:00 2001 From: Bhargav Voleti Date: Tue, 2 Mar 2021 09:20:25 -0800 Subject: [PATCH 5/5] Address feedback. Remove capability to apply lint to functions. --- text/0000-must-not-suspend-lint.md | 37 ++---------------------------- 1 file changed, 2 insertions(+), 35 deletions(-) diff --git a/text/0000-must-not-suspend-lint.md b/text/0000-must-not-suspend-lint.md index 70ff2478dae..b930485d675 100644 --- a/text/0000-must-not-suspend-lint.md +++ b/text/0000-must-not-suspend-lint.md @@ -63,7 +63,7 @@ This will be a best effort lint to signal the user about unintended side-effects # Reference-level explanation [reference-level-explanation]: #reference-level-explanation -The `must_not_suspend` 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. +The `must_not_suspend` 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), traits. The `must_not_suspend` attribute may include a message by using the [`MetaNameValueStr`] syntax such as `#[must_not_suspend = "example message"]`. The message will be given alongside the warning. @@ -81,19 +81,6 @@ async fn foo() { } ``` -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_suspend] -fn foo() -> i32 { 5i32 } - -async fn foo() { - let bar = foo(); - my_async_op.await; - println!("{:?}", bar); -} -``` - When used on a [trait declaration], if the value implementing that trait is held across an await point, the lint is violated. ```rust @@ -114,26 +101,6 @@ async fn foo() { } ``` -When used on a function in a trait declaration, then the behavior also applies when the call expression is a function from the implementation of the trait. - -```rust -trait Trait { - #[must_not_suspend] - 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 @@ -149,7 +116,7 @@ When used on a function in a trait implementation, the attribute does nothing. # Rationale and alternatives [rationale-and-alternatives]: #rationale-and-alternatives -Going through the prior are we see two systems currently which provide simailar/semantically similar behavior: +Going through the prior art we see two systems currently which provide similar/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_suspend]` 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_suspend]` type inside of it.