Skip to content

Add a lint for forgetting futures - #17054

Open
tdittr wants to merge 8 commits into
rust-lang:masterfrom
tdittr:mem_forget_impl_future
Open

Add a lint for forgetting futures#17054
tdittr wants to merge 8 commits into
rust-lang:masterfrom
tdittr:mem_forget_impl_future

Conversation

@tdittr

@tdittr tdittr commented May 22, 2026

Copy link
Copy Markdown

This adds a new lint to check when a Future is passed to mem::forget, thus not running any cancellation logic.

changelog: Add [mem_forget_future] lint

Open question: Should it be called forget_future instead? Similar to forget_non_drop.

@rustbot rustbot added the needs-fcp PRs that add, remove, or rename lints and need an FCP label May 22, 2026
@tdittr
tdittr force-pushed the mem_forget_impl_future branch from 3c86ae2 to 1a5451f Compare May 22, 2026 15:22
@tdittr

tdittr commented May 22, 2026

Copy link
Copy Markdown
Author

r? @llogiq

@tdittr

tdittr commented May 22, 2026

Copy link
Copy Markdown
Author

CC: @datdenkikniet we talked about this at the embedded unconf

@tdittr
tdittr force-pushed the mem_forget_impl_future branch 3 times, most recently from d0848b3 to b9bd4f3 Compare May 22, 2026 15:58
@tdittr
tdittr marked this pull request as ready for review May 23, 2026 09:10
@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label May 23, 2026
Comment thread clippy_lints/src/drop_forget_ref.rs Outdated
Comment on lines +136 to +142
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}`"));
});
}

@samueltardieu samueltardieu May 23, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aren't you also linting Future types even if they don't implement Drop? What would be the problem there?

View changes since the review

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh yes that is a very good point. I'll change that

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Weirdly the future created by an async block or async fn always implements Drop? No matter if it captures anything that implements Drop.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Weirdly the future created by an async block or async fn always implements Drop? No matter if it captures anything that implements Drop.

I'd expect that, at least if any object inside the async block implements Drop and can be alive around any .await. You should try and check if this is easy to determine.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like the drop glue is always generated, but no explicit Drop impl is around. So for example this will fail to compile:

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?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you by any chance also at RustWeek at the moment?

Yes, but leaving right after lunch.

Comment thread clippy_lints/src/drop_forget_ref.rs
Comment thread clippy_lints/src/drop_forget_ref.rs Outdated
Comment thread clippy_lints/src/drop_forget_ref.rs Outdated
@rustbot

This comment has been minimized.

@tdittr
tdittr force-pushed the mem_forget_impl_future branch from 49380f2 to ab5baf5 Compare August 3, 2026 09:33
@rustbot

rustbot commented Aug 3, 2026

Copy link
Copy Markdown
Collaborator

This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

@tdittr
tdittr force-pushed the mem_forget_impl_future branch from ab5baf5 to 5e2808c Compare August 3, 2026 09:49
@tdittr

tdittr commented Aug 3, 2026

Copy link
Copy Markdown
Author

I finally found some time again to work on this after RustWeek and addressed the suggestions by @datdenkikniet (thanks those texts were much better than what I came up with).

The Drop glue that is generated still makes this lint complain about more cases than it strictly needs to.

@tdittr
tdittr force-pushed the mem_forget_impl_future branch from 5e2808c to aeced90 Compare August 3, 2026 10:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-fcp PRs that add, remove, or rename lints and need an FCP S-waiting-on-review Status: Awaiting review from the assignee but also interested parties

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants