Add simple async drop glue generation by zetanumbers · Pull Request #121801 · rust-lang/rust (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation131 Commits6 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

This is a prototype of the async drop glue generation for some simple types. Async drop glue is intended to behave very similar to the regular drop glue except for being asynchronous. Currently it does not execute synchronous drops but only calls user implementations of AsyncDrop::async_drop associative function and awaits the returned future. It is not complete as it only recurses into arrays, slices, tuples, and structs and does not have same sensible restrictions as the old Drop trait implementation like having the same bounds as the type definition, while code assumes their existence (requires a future work).

This current design uses a workaround as it does not create any custom async destructor state machine types for ADTs, but instead uses types defined in the std library called future combinators (deferred_async_drop, chain, ready_unit).

Also I recommend reading my explainer.

This is a part of the MCP: Low level components for async drop work.

Feature completeness:

AsyncDrop trait
async_drop_in_place_raw/async drop glue generation support for
- Trivially destructible types (integers, bools, floats, string slices, pointers, references, etc.)
- Arrays and slices (array pointer is unsized into slice pointer)
- ADTs (enums, structs, unions)
- tuple-like types (tuples, closures)
- Dynamic types (dyn Trait, see explainer's proposed design)
- coroutines (Async drop codegen #123948)
Async drop glue includes sync drop glue code
Cleanup branch generation for async_drop_in_place_raw
Union rejects non-trivially async destructible fields
AsyncDrop implementation requires same bounds as type definition
Skip trivially destructible fields (optimization)
New TyKind::AdtAsyncDestructor and get rid of combinators
Synchronously undroppable types
Automatic async drop at the end of the scope in async context

r? @michaelwoerister

rustbot has assigned @michaelwoerister.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

rustbot added S-waiting-on-review

Status: Awaiting review from the assignee but also interested parties.

T-compiler

Relevant to the compiler team, which will review and decide on the PR/issue.

T-libs

Relevant to the library team, which will review and decide on the PR/issue.

WG-trait-system-refactor

The Rustc Trait System Refactor Initiative (-Znext-solver)

labels

Feb 29, 2024

This comment has been minimized.

The Miri subtree was changed

cc @rust-lang/miri

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

Some changes occurred in src/tools/rustfmt

cc @rust-lang/rustfmt

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

Some changes occurred to the core trait solver

cc @rust-lang/initiative-trait-system-refactor

This comment has been minimized.

Some high level comments:

This PR adds many lang items, it would be good to document not just what they do, but why are they added

to avoid generating MIR manually and call something from the library instead (not strictly necessary, but makes life easier)
to get some type information that is not easily calculatable otherwise (?), e.g. why AsyncDestruct and its associated type exist in particular.
to execute something user-defined from the compiler (strictly necessary, like trait AsyncDrop)
etc

It would also be good to add test cases that are not yet supported in a test, to see what happens when they are encountered and add corresponding FIXMEs for people to see.
Preferable with some short comments explaining whether they are trivial to support or not, and why.
E.g. what if AsyncDrop is implemented or called for enums, closures, or generic parameters.

Also add a test for a struct (or tuple) with 2 fields, one with non-trivial async drop, and one with non-trivial sync drop.
This case is not yet supported, but it would be good to cover it with a failing test for visibility.

This needs a review from someone who maintains MIR building/passes/transforms.
It's quite possible that some things here are not done idiomatically, and could be done simpler/better.
I oversaw this work, but neither @zetanumbers nor me had a previous experience with these parts of the compiler.

This comment has been minimized.

Great to see Miri tests already in the initial PR, that's much appreciated. :)

I would say there's one specific problem about this API. async_drop_in_place gets a pointer to some object to construct object's async destructor. This async destructor has a lifetime, that comes from the T: 'a bound of async_drop_in_place, which may not be meaningfull. For example if T: 'static is true, then it may be that 'a = 'static even tho we probably meant lifetime of a reference to this object instead. This 'a argument came from the trivial implementation of async drop purely within the library so we would able to write <T as AsyncDrop>::Dropper<'a> within the implementation of impl<'a, T: AsyncDrop> AsyncDestruct<'a> for T {...}.

To move forward I can either remove this 'a parameter or change function signature to something like a fn async_drop_in_place<'a, T>(to_drop: &'a mut MaybeUninit<T>) .... Second one has the problem with MaybeUninit requiring T: Sized, which is already bad for slices, but perhaps this could be replaced with ManuallyDrop<T>. First variant has been done once, and i'm actually surprised lifetime punning kinda works here, however there could still be a hypothetical problem with making some coroutines Unpin cause technically we still don't borrow anything there thus possibly allowing compiler to mark this coroutine as Unpin, while having a self-referencial pointer.

For now I've decided to stick with removing 'a since Unpin coroutines don't seem to be close in the future.

Did a partial review to get an overview. I think the main comment of this PR should mention that this does not yet do

Component 2: Calling async drop glue at the end of scope

or any other automatic drop glue handling. All that we get is that a type gets dropped asynchronously plus the glue needed for field types and builtin types

Comment on lines 1073 to 1078

/// Empty strategy should generate this:
///
/// ```ignore (illustrative)
/// ready_unit()
/// ```
Empty,

Choose a reason for hiding this comment

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

can we avoid this variant by never requesting the shim for these at all?

Choose a reason for hiding this comment

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

We would still need to be able to directly call async_drop_in_place(_raw)::<()> so we cannot "never request". This is true for regular drop glue too.

Choose a reason for hiding this comment

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

}
}

GlueStrategy::Chain { has_surface_async_drop, elem_tys } => {

Choose a reason for hiding this comment

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

can we write most of this one in surface Rust somehow? Maybe we could generate a tuple of the various types that should be dropped and recursively invoke drop on them?

Choose a reason for hiding this comment

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

Sorry but I don't understand what you are describing. If you imagine a handwritten async destructor future for a tuple it would be an enum, for which each variant of it represents running an async destructor for some field, sequentially ordered one after the other + variants like Unresumed and Done.

rustbot added S-waiting-on-author

Status: This is awaiting some action (such as code changes or more information) from the author.

and removed S-waiting-on-review

Status: Awaiting review from the assignee but also interested parties.

labels

Mar 5, 2024

I have not reviewed the shim impls yet, I'd prefer it if we could avoid such a complex shim

I have not reviewed the shim impls yet, I'd prefer it if we could avoid such a complex shim

When it comes to GlueStrategy::Chain it shouldn't have been complex like that, but since I haven't yet managed to generate async destructor ADTs inside of the compiler I've used nested library types, which is suboptimal.

EDIT: However it will instead introduce ADT async destructors Future::poll shim.

To reduce complexity of shim generation code I can try modeling it as nested expressions instead of directly creating basic blocks. Yeah, I should try doing that.

EDIT: going to prioritize this for now since making changes became a bit unwieldy.

EDIT2: Done in 16010ba :)

Also this "constructor of async destructor" is confusing to say. Not sure what naming convention I should use instead.

This comment has been minimized.

@rustbot labels +T-lang +I-lang-nominated

Let's discuss approval of this as an experiment.

@zetanumbers / @petrochenkov: If you could perhaps write up further details for the lang team to review as part of this nomination about the nature of this part of the experiment, what the motivation for it is, and how this fits into the larger body of work, that work be helpful.

Sorry for adding changes last moment.

Uh-oh

This PR changes Stable MIR

The StableMIR change looks fine. Thanks for addressing it.

Made most matches on TyKind to be non exhaustive in 80c0b7e.

why? The equivalent matches on Drop things elsewhere are exhaustive

Made most matches on TyKind to be non exhaustive in 80c0b7e.

why? The equivalent matches on Drop things elsewhere are exhaustive

Some TyKind variants are not supported yet, so I've made this branch to accept anything that either I didn't account for or is know but should be added in the future. Anyway people would have to figure out what case they should put their type in and it would be most probably the "todo" branch, so I would like to think I've made this choice myself not to spend everyone's time on those. But of course the "todo" branch should be removed in the future.

@@ -2319,6 +2319,10 @@ impl<'tcx> Ty<'tcx> {

/// Returns the type of the async destructor of this type.
pub fn async_destructor_ty(self, tcx: TyCtxt<'tcx>, param_env: ParamEnv<'tcx>) -> Ty<'tcx> {
if self.is_async_destructor_noop(tcx, param_env) \|

Choose a reason for hiding this comment

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

prefer self.references_error() over matching for ty::Error

Choose a reason for hiding this comment

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

Neither drop glue nor discriminant kind do this. I will have to switch every match on ty::Error otherwise compiler may emit type errors.

Choose a reason for hiding this comment

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

I think I cannot add this case to be a part of is_async_destructor_noop because of this line, which differs from other similar types like ints:

But I'm not sure, maybe I should include it?

Choose a reason for hiding this comment

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

Hmm... I guess we can look if there's a general thing to improve here everywhere

/// Returning `false` means the type is known to not have `Drop`
/// implementation. Returning `true` means nothing -- could be
/// `Drop`, might not be.
fn could_have_surface_drop(self) -> bool {

Choose a reason for hiding this comment

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

how does this relate to is_trivially_const_drop and can we deduplicate some code?

Choose a reason for hiding this comment

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

is_trivially_const_drop recurses into array and tuple elements to determine if type can be trivially destructed, while could_have_surface_drop only checks if there's possible impl Drop for T. This function would return false for a tuple with any collection of types, while !is_trivially_const_drop for the same tuple may be true because of some element type could be an ADT. I could use could_have_surface_drop from is_trivially_const_drop since the former's negative implies the latter if it's ok.

Choose a reason for hiding this comment

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

Well, in contrast is_trivially_const_drop does not check/intend to check ManuallyDrop, so those could mean a bit different things after all.

rustbot added the S-waiting-on-author

Status: This is awaiting some action (such as code changes or more information) from the author.

label

Apr 19, 2024

Co-authored-by: Oli Scherer github35764891676564198441@oli-obk.de

📌 Commit 67980dd has been approved by oli-obk

It is now in the queue for this repository.

bors removed the S-waiting-on-author

Status: This is awaiting some action (such as code changes or more information) from the author.

label

Apr 22, 2024

github-actions bot pushed a commit to rust-lang/miri that referenced this pull request

Apr 23, 2024

Finished benchmarking commit (aca749e): comparison URL.

Overall result: ❌ regressions - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	0.1%	[0.1%, 0.1%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-2.5%	[-2.5%, -2.5%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-2.5%	[-2.5%, -2.5%]	1

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 672.713s -> 674.396s (0.25%)
Artifact size: 316.06 MiB -> 315.53 MiB (-0.17%)

bors added a commit to rust-lang-ci/rust that referenced this pull request

May 31, 2024

bors added a commit to rust-lang-ci/rust that referenced this pull request

Apr 28, 2025

Async drop codegen

Async drop implementation using templated coroutine for async drop glue generation.

Scopes changes to generate async_drop_in_place() awaits, when async droppable objects are out-of-scope in async context.

Implementation details: https://github.com/azhogin/posts/blob/main/async-drop-impl.md

New fields in Drop terminator (drop & async_fut). Processing in codegen/miri must validate that those fields are empty (in full version async Drop terminator will be expanded at StateTransform pass or reverted to sync version). Changes in terminator visiting to consider possible new successor (drop field).

ResumedAfterDrop messages for panic when coroutine is resumed after it is started to be async drop'ed.

Lang item for generated coroutine for async function async_drop_in_place. async fn async_drop_in_place<T>()::{{closure0}}.

Scopes processing for generate async drop preparations. Async drop is a hidden Yield, so potentially async drops require the same dropline preparation as for Yield terminators.

Processing in StateTransform: async drops are expanded into yield-point. Generation of async drop of coroutine itself added.

Shims for AsyncDropGlueCtorShim, AsyncDropGlue and FutureDropPoll.

#[lang = "async_drop"]
pub trait AsyncDrop {
    #[allow(async_fn_in_trait)]
    async fn drop(self: Pin<&mut Self>);
}

impl Drop for Foo {
    fn drop(&mut self) {
        println!("Foo::drop({})", self.my_resource_handle);
    }
}

impl AsyncDrop for Foo {
    async fn drop(self: Pin<&mut Self>) {
        println!("Foo::async drop({})", self.my_resource_handle);
    }
}

First async drop glue implementation re-worked to use the same drop elaboration code as for sync drop. async_drop_in_place changed to be async fn. So both async_drop_in_place ctor and produced coroutine have their lang items (AsyncDropInPlace/AsyncDropInPlacePoll) and shim instances (AsyncDropGlueCtorShim/AsyncDropGlue).

pub async unsafe fn async_drop_in_place<T: ?Sized>(_to_drop: *mut T) {
}

AsyncDropGlue shim generation uses elaborate_drops::elaborate_drop to produce drop ladder (in the similar way as for sync drop glue) and then coroutine::StateTransform to convert function into coroutine poll.

AsyncDropGlue coroutine's layout can't be calculated for generic T, it requires known final dropee type to be generated (in StateTransform). So, templated coroutine was introduced here (templated_coroutine_layout(...) etc).

Such approach overrides the first implementation using mixing language-level futures in rust-lang#121801.

github-actions bot pushed a commit to rust-lang/miri that referenced this pull request

Apr 29, 2025

Async drop codegen

Async drop implementation using templated coroutine for async drop glue generation.

Scopes changes to generate async_drop_in_place() awaits, when async droppable objects are out-of-scope in async context.

Implementation details: https://github.com/azhogin/posts/blob/main/async-drop-impl.md

ResumedAfterDrop messages for panic when coroutine is resumed after it is started to be async drop'ed.

Lang item for generated coroutine for async function async_drop_in_place. async fn async_drop_in_place<T>()::{{closure0}}.

Scopes processing for generate async drop preparations. Async drop is a hidden Yield, so potentially async drops require the same dropline preparation as for Yield terminators.

Processing in StateTransform: async drops are expanded into yield-point. Generation of async drop of coroutine itself added.

Shims for AsyncDropGlueCtorShim, AsyncDropGlue and FutureDropPoll.

#[lang = "async_drop"]
pub trait AsyncDrop {
    #[allow(async_fn_in_trait)]
    async fn drop(self: Pin<&mut Self>);
}

impl Drop for Foo {
    fn drop(&mut self) {
        println!("Foo::drop({})", self.my_resource_handle);
    }
}

impl AsyncDrop for Foo {
    async fn drop(self: Pin<&mut Self>) {
        println!("Foo::async drop({})", self.my_resource_handle);
    }
}

pub async unsafe fn async_drop_in_place<T: ?Sized>(_to_drop: *mut T) {
}

Such approach overrides the first implementation using mixing language-level futures in rust-lang/rust#121801.

github-actions bot pushed a commit to model-checking/verify-rust-std that referenced this pull request

May 9, 2025

Async drop codegen

Async drop implementation using templated coroutine for async drop glue generation.

Scopes changes to generate async_drop_in_place() awaits, when async droppable objects are out-of-scope in async context.

Implementation details: https://github.com/azhogin/posts/blob/main/async-drop-impl.md

ResumedAfterDrop messages for panic when coroutine is resumed after it is started to be async drop'ed.

Lang item for generated coroutine for async function async_drop_in_place. async fn async_drop_in_place<T>()::{{closure0}}.

Scopes processing for generate async drop preparations. Async drop is a hidden Yield, so potentially async drops require the same dropline preparation as for Yield terminators.

Processing in StateTransform: async drops are expanded into yield-point. Generation of async drop of coroutine itself added.

Shims for AsyncDropGlueCtorShim, AsyncDropGlue and FutureDropPoll.

#[lang = "async_drop"]
pub trait AsyncDrop {
    #[allow(async_fn_in_trait)]
    async fn drop(self: Pin<&mut Self>);
}

impl Drop for Foo {
    fn drop(&mut self) {
        println!("Foo::drop({})", self.my_resource_handle);
    }
}

impl AsyncDrop for Foo {
    async fn drop(self: Pin<&mut Self>) {
        println!("Foo::async drop({})", self.my_resource_handle);
    }
}

pub async unsafe fn async_drop_in_place<T: ?Sized>(_to_drop: *mut T) {
}

Such approach overrides the first implementation using mixing language-level futures in rust-lang#121801.

Labels

A-async-await

Area: Async & Await

AsyncAwait-Triaged

Async-await issues that have been triaged during a working group meeting.

F-async_drop

`#![feature(async_drop)]`

merged-by-bors

This PR was explicitly merged by bors.

S-waiting-on-bors

Status: Waiting on bors to run and complete tests. Bors will change the label on completion.

T-compiler

Relevant to the compiler team, which will review and decide on the PR/issue.

T-lang

Relevant to the language team

T-libs

Relevant to the library team, which will review and decide on the PR/issue.

WG-async

Working group: Async & await

WG-trait-system-refactor

The Rustc Trait System Refactor Initiative (-Znext-solver)