Add simple async drop glue generation #121801

zetanumbers · 2024-02-29T13:41:37Z

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:

rustbot · 2024-02-29T13:41:45Z

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 · 2024-02-29T14:36:47Z

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

petrochenkov · 2024-03-01T16:11:43Z

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.

petrochenkov · 2024-03-01T16:15:06Z

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.

RalfJung · 2024-03-02T11:19:17Z

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

zetanumbers · 2024-03-04T13:31:32Z

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<T> 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.

oli-obk · 2024-03-04T14:38:55Z

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

src/tools/rustfmt/config_proc_macro/src/attrs.rs

compiler/rustc_middle/src/ty/instance.rs

library/core/src/future/async_drop.rs

compiler/rustc_ty_utils/src/instance.rs

compiler/rustc_trait_selection/src/traits/project.rs

oli-obk · 2024-03-05T09:51:42Z

compiler/rustc_mir_transform/src/shim.rs

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


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

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.

May be relevant: #121801 (comment)

compiler/rustc_mir_transform/src/shim.rs

oli-obk · 2024-03-05T09:56:40Z

compiler/rustc_mir_transform/src/shim.rs

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


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?

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.

oli-obk · 2024-03-05T09:57:50Z

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

zetanumbers · 2024-03-05T14:10:13Z

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.

zetanumbers · 2024-03-05T14:50:13Z

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 :)

zetanumbers · 2024-03-05T14:56:42Z

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

traviscross · 2024-03-07T01:08:42Z

@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.

Also no longer export noop async_drop_in_place_raw

zetanumbers · 2024-04-18T12:21:44Z

I should have made "not yet implemented" match branch to take a wildcard instead of specifying every TyKind variant. Oh well

It is better this way, even if it causes breakage for you, it will make the life of ppl adding new variants easier

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

rustbot · 2024-04-18T12:21:48Z

This PR changes Stable MIR

cc @oli-obk, @celinval, @ouz-a

zetanumbers · 2024-04-18T12:37:46Z

Uh-oh

This PR changes Stable MIR

zetanumbers · 2024-04-18T13:12:40Z

Sorry for adding changes last moment.

celinval · 2024-04-18T18:52:06Z

Uh-oh

This PR changes Stable MIR

The StableMIR change looks fine. Thanks for addressing it.

oli-obk · 2024-04-19T13:57:54Z

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

why? The equivalent matches on Drop things elsewhere are exhaustive

zetanumbers · 2024-04-19T15:08:25Z

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.

oli-obk · 2024-04-19T15:19:20Z

compiler/rustc_middle/src/ty/sty.rs

@@ -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) || matches!(self.kind(), ty::Error(_)) {


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

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

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:

rust/compiler/rustc_trait_selection/src/traits/project.rs

Line 1111 in e239e73

| ty::Error(_) => false,

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

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

compiler/rustc_middle/src/ty/util.rs

oli-obk · 2024-04-19T15:28:39Z

compiler/rustc_middle/src/ty/util.rs

+    /// 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 {


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

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.

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

compiler/rustc_trait_selection/src/solve/normalizes_to/mod.rs

…ruct_candidate

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

oli-obk · 2024-04-22T14:04:47Z

@bors r+

bors · 2024-04-22T14:04:50Z

📌 Commit 67980dd has been approved by oli-obk

It is now in the queue for this repository.

compiler-errors · 2024-04-22T14:54:37Z

@bors rollup=never

bors · 2024-04-23T02:10:27Z

⌛ Testing commit 67980dd with merge aca749e...

bors · 2024-04-23T04:12:26Z

☀️ Test successful - checks-actions
Approved by: oli-obk
Pushing aca749e to master...

Explainer: https://zetanumbers.github.io/book/async-drop-design.html rust-lang/rust#121801

rust-timer · 2024-04-23T05:28:35Z

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.