-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Add a lint for forgetting futures #17054
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
base: master
Are you sure you want to change the base?
Changes from 6 commits
1a5451f
435f837
b9bd4f3
c7831b8
1044457
70c7249
49380f2
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 |
|---|---|---|
| @@ -1,7 +1,7 @@ | ||
| use clippy_utils::diagnostics::span_lint_and_then; | ||
| use clippy_utils::is_must_use_func_call; | ||
| use clippy_utils::res::MaybeDef; | ||
| use clippy_utils::ty::{is_copy, is_must_use_ty}; | ||
| use clippy_utils::ty::{implements_trait, is_copy, is_must_use_ty}; | ||
| use rustc_hir::{Arm, Expr, ExprKind, LangItem, Node}; | ||
| use rustc_lint::{LateContext, LateLintPass}; | ||
| use rustc_session::declare_lint_pass; | ||
|
|
@@ -28,6 +28,29 @@ declare_clippy_lint! { | |
| "call to `std::mem::drop` with a value which does not implement `Drop`" | ||
| } | ||
|
|
||
| declare_clippy_lint! { | ||
| /// ### What it does | ||
| /// | ||
| /// Checks for not executing `Drop` on a `Future`. | ||
| /// | ||
| /// ### Why is this bad? | ||
| /// | ||
| /// Futures use drop to be signalled that they were cancelled and should clean up their resources. | ||
|
Contributor
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 can be clearer here. The statement is not technically wrong, but it's arguing from the viewpoint of "we want to drop all Additionally, I propose something like: "When dropped, |
||
| /// | ||
| /// ### Example | ||
| /// ```no_run | ||
| /// std::mem::forget(async {}); | ||
| /// ``` | ||
| /// Use instead: | ||
| /// ```no_run | ||
| /// std::mem::drop(async {}); | ||
| /// ``` | ||
| #[clippy::version = "1.97.0"] | ||
| pub FORGET_FUTURE, | ||
| suspicious, | ||
| "`mem::forget` usage on `Future` types, likely to cause problems with cancelation" | ||
|
Contributor
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. Another "can we be more specific" request: " |
||
| } | ||
|
|
||
| declare_clippy_lint! { | ||
| /// ### What it does | ||
| /// Checks for calls to `std::mem::forget` with a value that does not implement `Drop`. | ||
|
|
@@ -70,12 +93,18 @@ declare_clippy_lint! { | |
| "`mem::forget` usage on `Drop` types, likely to cause memory leaks" | ||
| } | ||
|
|
||
| declare_lint_pass!(DropForgetRef => [DROP_NON_DROP, FORGET_NON_DROP, MEM_FORGET]); | ||
| declare_lint_pass!(DropForgetRef => [ | ||
| DROP_NON_DROP, | ||
| FORGET_FUTURE, | ||
| FORGET_NON_DROP, | ||
| MEM_FORGET, | ||
| ]); | ||
|
|
||
| const DROP_NON_DROP_SUMMARY: &str = "call to `std::mem::drop` with a value that does not implement `Drop`. \ | ||
| Dropping such a type only extends its contained lifetimes"; | ||
| const FORGET_NON_DROP_SUMMARY: &str = "call to `std::mem::forget` with a value that does not implement `Drop`. \ | ||
| Forgetting such a type is the same as dropping it"; | ||
| const FORGET_FUTURE_SUMMARY: &str = "forgetting a Future might cause problems with cancelation"; | ||
|
|
||
| impl<'tcx> LateLintPass<'tcx> for DropForgetRef { | ||
| fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) { | ||
|
|
@@ -104,6 +133,14 @@ impl<'tcx> LateLintPass<'tcx> for DropForgetRef { | |
| (DROP_NON_DROP, DROP_NON_DROP_SUMMARY.into(), Some(arg.span)) | ||
| }, | ||
| sym::mem_forget => { | ||
| if let Some(future_trait) = cx.tcx.lang_items().future_trait() | ||
| && implements_trait(cx, arg_ty, future_trait, &[]) | ||
| { | ||
| span_lint_and_then(cx, FORGET_FUTURE, expr.span, FORGET_FUTURE_SUMMARY, |diag| { | ||
| diag.span_note(arg.span, format!("argument has type `{arg_ty}`")); | ||
| }); | ||
| } | ||
|
Member
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. Aren't you also linting
Author
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. Oh yes that is a very good point. I'll change that
Author
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. Weirdly the future created by an
Member
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'd expect that, at least if any object inside the
Author
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. It looks like the drop glue is always generated, but no explicit async fn foo() {
let x = Box::new(());
core::future::ready(()).await;
let _ = x;
}
fn bar() -> impl Future<Output = ()> + Drop {
foo()
}Are you by any chance also at RustWeek at the moment?
Member
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, but leaving right after lunch. |
||
|
|
||
| if arg_ty.needs_drop(cx.tcx, cx.typing_env()) { | ||
| ( | ||
| MEM_FORGET, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,34 @@ | ||
| #![warn(clippy::forget_future)] | ||
|
|
||
| use core::mem; | ||
| use std::pin::Pin; | ||
| use std::task::{Context, Poll}; | ||
|
|
||
| fn main() { | ||
| let fut = foo(); | ||
| mem::forget(fut); | ||
| //~^ forget_future | ||
|
|
||
| mem::forget(async {}); | ||
| //~^ forget_future | ||
|
|
||
| let fut = MyFuture; | ||
| mem::forget(fut); | ||
| //~^ forget_future | ||
| } | ||
|
|
||
| async fn foo() {} | ||
|
|
||
| struct MyFuture; | ||
|
|
||
| impl Future for MyFuture { | ||
| type Output = (); | ||
|
|
||
| fn poll(self: Pin<&mut Self>, _cx: &mut Context<'_>) -> Poll<Self::Output> { | ||
| Poll::Ready(()) | ||
| } | ||
| } | ||
|
|
||
| impl Drop for MyFuture { | ||
| fn drop(&mut self) {} | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,40 @@ | ||
| error: forgetting a Future might cause problems with cancelation | ||
| --> tests/ui/forget_future.rs:9:5 | ||
| | | ||
| LL | mem::forget(fut); | ||
| | ^^^^^^^^^^^^^^^^ | ||
| | | ||
| note: argument has type `impl std::future::Future<Output = ()>` | ||
| --> tests/ui/forget_future.rs:9:17 | ||
| | | ||
| LL | mem::forget(fut); | ||
| | ^^^ | ||
| = note: `-D clippy::forget-future` implied by `-D warnings` | ||
| = help: to override `-D warnings` add `#[allow(clippy::forget_future)]` | ||
|
|
||
| error: forgetting a Future might cause problems with cancelation | ||
| --> tests/ui/forget_future.rs:12:5 | ||
| | | ||
| LL | mem::forget(async {}); | ||
| | ^^^^^^^^^^^^^^^^^^^^^ | ||
| | | ||
| note: argument has type `{async block@tests/ui/forget_future.rs:12:17: 12:22}` | ||
| --> tests/ui/forget_future.rs:12:17 | ||
| | | ||
| LL | mem::forget(async {}); | ||
| | ^^^^^^^^ | ||
|
|
||
| error: forgetting a Future might cause problems with cancelation | ||
| --> tests/ui/forget_future.rs:16:5 | ||
| | | ||
| LL | mem::forget(fut); | ||
| | ^^^^^^^^^^^^^^^^ | ||
| | | ||
| note: argument has type `MyFuture` | ||
| --> tests/ui/forget_future.rs:16:17 | ||
| | | ||
| LL | mem::forget(fut); | ||
| | ^^^ | ||
|
|
||
| error: aborting due to 3 previous errors | ||
|
|
Uh oh!
There was an error while loading. Please reload this page.
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.
Does this also catch other non-drop cases (i.e.
ManuallyDrop<impl Future>)? If not, we should be more specific and say that "Checks whetherstd::mem::forgetis called on aFuturethat implementsDrop" or something like that..View changes since the review
Uh oh!
There was an error while loading. Please reload this page.
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've double-checked, and the following does indeed not generate the lint (which is fine, but supports my request for making this description more specific):
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.
It's fine in the sense that we can always add
ManuallyDrop<impl Future>detection later, but not half-implying that this lint already does it would be good :D