Skip to content

Add simple async drop glue generation #121801

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

Merged
merged 6 commits into from
Apr 23, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion compiler/rustc_codegen_ssa/src/back/symbol_export.rs
Original file line number Diff line number Diff line change
Expand Up @@ -364,7 +364,7 @@ fn exported_symbols_provider_local(
));
}
MonoItem::Fn(Instance {
def: InstanceDef::AsyncDropGlueCtorShim(def_id, ty),
def: InstanceDef::AsyncDropGlueCtorShim(def_id, Some(ty)),
args,
}) => {
// A little sanity-check
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_middle/src/mir/pretty.rs
Original file line number Diff line number Diff line change
Expand Up @@ -187,7 +187,7 @@ fn dump_path<'tcx>(
}));
s
}
ty::InstanceDef::AsyncDropGlueCtorShim(_, ty) => {
ty::InstanceDef::AsyncDropGlueCtorShim(_, Some(ty)) => {
// Unfortunately, pretty-printed typed are not very filename-friendly.
// We dome some filtering.
let mut s = ".".to_owned();
Expand Down
3 changes: 2 additions & 1 deletion compiler/rustc_middle/src/mir/visit.rs
Original file line number Diff line number Diff line change
Expand Up @@ -350,13 +350,14 @@ macro_rules! make_mir_visitor {
receiver_by_ref: _,
} |
ty::InstanceDef::CoroutineKindShim { coroutine_def_id: _def_id } |
ty::InstanceDef::AsyncDropGlueCtorShim(_def_id, None) |
ty::InstanceDef::DropGlue(_def_id, None) => {}

ty::InstanceDef::FnPtrShim(_def_id, ty) |
ty::InstanceDef::DropGlue(_def_id, Some(ty)) |
ty::InstanceDef::CloneShim(_def_id, ty) |
ty::InstanceDef::FnPtrAddrShim(_def_id, ty) |
ty::InstanceDef::AsyncDropGlueCtorShim(_def_id, ty) => {
ty::InstanceDef::AsyncDropGlueCtorShim(_def_id, Some(ty)) => {
// FIXME(eddyb) use a better `TyContext` here.
self.visit_ty($(& $mutability)? *ty, TyContext::Location(location));
}
Expand Down
5 changes: 3 additions & 2 deletions compiler/rustc_middle/src/ty/instance.rs
Original file line number Diff line number Diff line change
Expand Up @@ -173,7 +173,7 @@ pub enum InstanceDef<'tcx> {
///
/// The `DefId` is for `core::future::async_drop::async_drop_in_place`, the `Ty`
/// is the type `T`.
AsyncDropGlueCtorShim(DefId, Ty<'tcx>),
AsyncDropGlueCtorShim(DefId, Option<Ty<'tcx>>),
}

impl<'tcx> Instance<'tcx> {
Expand Down Expand Up @@ -406,7 +406,8 @@ fn fmt_instance(
InstanceDef::DropGlue(_, Some(ty)) => write!(f, " - shim(Some({ty}))"),
InstanceDef::CloneShim(_, ty) => write!(f, " - shim({ty})"),
InstanceDef::FnPtrAddrShim(_, ty) => write!(f, " - shim({ty})"),
InstanceDef::AsyncDropGlueCtorShim(_, ty) => write!(f, " - shim({ty})"),
InstanceDef::AsyncDropGlueCtorShim(_, None) => write!(f, " - shim(None)"),
InstanceDef::AsyncDropGlueCtorShim(_, Some(ty)) => write!(f, " - shim(Some({ty}))"),
}
}

Expand Down
43 changes: 12 additions & 31 deletions compiler/rustc_middle/src/ty/sty.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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(_)) {
Copy link
Contributor

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

Copy link
Contributor Author

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.

Copy link
Contributor Author

@zetanumbers zetanumbers Apr 19, 2024

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?

Copy link
Contributor

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

return Ty::async_destructor_combinator(tcx, LangItem::AsyncDropNoop)
.instantiate_identity();
}
match *self.kind() {
ty::Param(_) | ty::Alias(..) | ty::Infer(ty::TyVar(_)) => {
let assoc_items = tcx
Expand All @@ -2333,9 +2337,6 @@ impl<'tcx> Ty<'tcx> {
.instantiate(tcx, &[dtor.into()])
}

ty::Adt(adt_def, _) if adt_def.is_manually_drop() => {
Ty::async_destructor_combinator(tcx, LangItem::AsyncDropNoop).instantiate_identity()
}
ty::Adt(adt_def, args) if adt_def.is_enum() || adt_def.is_struct() => self
.adt_async_destructor_ty(
tcx,
Expand All @@ -2357,34 +2358,10 @@ impl<'tcx> Ty<'tcx> {
ty::Adt(adt_def, _) => {
assert!(adt_def.is_union());

match self.surface_async_dropper_ty(tcx, param_env) {
None => Ty::async_destructor_combinator(tcx, LangItem::AsyncDropNoop)
.instantiate_identity(),
Some(surface_drop) => {
Ty::async_destructor_combinator(tcx, LangItem::AsyncDropFuse)
.instantiate(tcx, &[surface_drop.into()])
}
}
}

ty::Never
| ty::Bool
| ty::Char
| ty::Int(_)
| ty::Uint(_)
| ty::Float(_)
| ty::Str
| ty::RawPtr(_, _)
| ty::Ref(..)
| ty::FnDef(..)
| ty::FnPtr(..)
| ty::Infer(ty::IntVar(_) | ty::FloatVar(_))
| ty::Error(_) => {
Ty::async_destructor_combinator(tcx, LangItem::AsyncDropNoop).instantiate_identity()
}
let surface_drop = self.surface_async_dropper_ty(tcx, param_env).unwrap();

ty::Dynamic(..) | ty::CoroutineWitness(..) | ty::Coroutine(..) | ty::Pat(..) => {
bug!("`async_destructor_ty` is not yet implemented for type: {self:?}")
Ty::async_destructor_combinator(tcx, LangItem::AsyncDropFuse)
.instantiate(tcx, &[surface_drop.into()])
}

ty::Bound(..)
Expand All @@ -2393,6 +2370,8 @@ impl<'tcx> Ty<'tcx> {
| ty::Infer(ty::FreshTy(_) | ty::FreshIntTy(_) | ty::FreshFloatTy(_)) => {
bug!("`async_destructor_ty` applied to unexpected type: {self:?}")
}

_ => bug!("`async_destructor_ty` is not yet implemented for type: {self:?}"),
}
}

Expand All @@ -2406,6 +2385,8 @@ impl<'tcx> Ty<'tcx> {
I: Iterator + ExactSizeIterator,
I::Item: IntoIterator<Item = Ty<'tcx>>,
{
debug_assert!(!self.is_async_destructor_noop(tcx, param_env));

let defer = Ty::async_destructor_combinator(tcx, LangItem::AsyncDropDefer);
let chain = Ty::async_destructor_combinator(tcx, LangItem::AsyncDropChain);

Expand All @@ -2425,7 +2406,7 @@ impl<'tcx> Ty<'tcx> {
.reduce(|other, matched| {
either.instantiate(tcx, &[other.into(), matched.into(), self.into()])
})
.unwrap_or(noop);
.unwrap();

let dtor = if let Some(dropper_ty) = self.surface_async_dropper_ty(tcx, param_env) {
Ty::async_destructor_combinator(tcx, LangItem::AsyncDropChain)
Expand Down
117 changes: 59 additions & 58 deletions compiler/rustc_middle/src/ty/util.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1306,8 +1306,7 @@ impl<'tcx> Ty<'tcx> {
/// Checks whether values of this type `T` implements the `AsyncDrop`
/// trait.
pub fn has_surface_async_drop(self, tcx: TyCtxt<'tcx>, param_env: ty::ParamEnv<'tcx>) -> bool {
self.trivially_has_surface_async_drop()
&& tcx.has_surface_async_drop_raw(param_env.and(self))
self.could_have_surface_async_drop() && tcx.has_surface_async_drop_raw(param_env.and(self))
}

/// Fast path helper for testing if a type has `AsyncDrop`
Expand All @@ -1316,52 +1315,68 @@ impl<'tcx> Ty<'tcx> {
/// Returning `false` means the type is known to not have `AsyncDrop`
/// implementation. Returning `true` means nothing -- could be
/// `AsyncDrop`, might not be.
fn trivially_has_surface_async_drop(self) -> bool {
match self.kind() {
ty::Int(_)
| ty::Uint(_)
| ty::Float(_)
| ty::Bool
| ty::Char
| ty::Str
| ty::Never
| ty::Ref(..)
| ty::RawPtr(_, _)
| ty::FnDef(..)
| ty::FnPtr(_)
| ty::Error(_)
| ty::Tuple(_)
| ty::Slice(_)
| ty::Array(_, _)
| ty::Closure(..)
| ty::CoroutineClosure(..)
| ty::Coroutine(..)
| ty::CoroutineWitness(..)
| ty::Pat(..) => false,
ty::Adt(..)
| ty::Bound(..)
| ty::Dynamic(..)
| ty::Foreign(_)
| ty::Infer(_)
| ty::Alias(..)
| ty::Param(_)
| ty::Placeholder(_) => true,
}
fn could_have_surface_async_drop(self) -> bool {
!self.is_async_destructor_trivially_noop()
&& !matches!(
self.kind(),
ty::Tuple(_)
| ty::Slice(_)
| ty::Array(_, _)
| ty::Closure(..)
| ty::CoroutineClosure(..)
| ty::Coroutine(..)
)
}

/// Checks whether values of this type `T` implements the `AsyncDrop`
/// trait.
pub fn has_surface_drop(self, tcx: TyCtxt<'tcx>, param_env: ty::ParamEnv<'tcx>) -> bool {
self.trivially_has_surface_drop() && tcx.has_surface_drop_raw(param_env.and(self))
self.could_have_surface_drop() && tcx.has_surface_drop_raw(param_env.and(self))
}

/// Fast path helper for testing if a type has `AsyncDrop`
/// implementation.
/// Fast path helper for testing if a type has `Drop` implementation.
///
/// Returning `false` means the type is known to not have `AsyncDrop`
/// Returning `false` means the type is known to not have `Drop`
/// implementation. Returning `true` means nothing -- could be
/// `AsyncDrop`, might not be.
fn trivially_has_surface_drop(self) -> bool {
/// `Drop`, might not be.
fn could_have_surface_drop(self) -> bool {
Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

self.is_async_destructor_trivially_noop()
&& !matches!(
self.kind(),
ty::Tuple(_)
| ty::Slice(_)
| ty::Array(_, _)
| ty::Closure(..)
| ty::CoroutineClosure(..)
| ty::Coroutine(..)
)
}

/// Checks whether values of this type `T` implement has noop async destructor.
//
// FIXME: implement optimization to make ADTs, which do not need drop,
// to skip fields or to have noop async destructor.
pub fn is_async_destructor_noop(
self,
tcx: TyCtxt<'tcx>,
param_env: ty::ParamEnv<'tcx>,
) -> bool {
self.is_async_destructor_trivially_noop()
|| if let ty::Adt(adt_def, _) = self.kind() {
(adt_def.is_union() || adt_def.is_payloadfree())
&& !self.has_surface_async_drop(tcx, param_env)
&& !self.has_surface_drop(tcx, param_env)
} else {
false
}
}

/// Fast path helper for testing if a type has noop async destructor.
///
/// Returning `true` means the type is known to have noop async destructor
/// implementation. Returning `true` means nothing -- could be
/// `Drop`, might not be.
fn is_async_destructor_trivially_noop(self) -> bool {
match self.kind() {
ty::Int(_)
| ty::Uint(_)
Expand All @@ -1371,26 +1386,12 @@ impl<'tcx> Ty<'tcx> {
| ty::Str
| ty::Never
| ty::Ref(..)
| ty::RawPtr(_, _)
| ty::RawPtr(..)
| ty::FnDef(..)
| ty::FnPtr(_)
| ty::Error(_)
| ty::Tuple(_)
| ty::Slice(_)
| ty::Array(_, _)
| ty::Closure(..)
| ty::CoroutineClosure(..)
| ty::Coroutine(..)
| ty::CoroutineWitness(..)
| ty::Pat(..) => false,
ty::Adt(..)
| ty::Bound(..)
| ty::Dynamic(..)
| ty::Foreign(_)
| ty::Infer(_)
| ty::Alias(..)
| ty::Param(_)
| ty::Placeholder(_) => true,
| ty::FnPtr(_) => true,
ty::Tuple(tys) => tys.is_empty(),
ty::Adt(adt_def, _) => adt_def.is_manually_drop(),
_ => false,
}
}

Expand Down
4 changes: 2 additions & 2 deletions compiler/rustc_mir_transform/src/inline.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1072,8 +1072,8 @@ fn try_instance_mir<'tcx>(
tcx: TyCtxt<'tcx>,
instance: InstanceDef<'tcx>,
) -> Result<&'tcx Body<'tcx>, &'static str> {
if let ty::InstanceDef::DropGlue(_, Some(ty)) | ty::InstanceDef::AsyncDropGlueCtorShim(_, ty) =
instance
if let ty::InstanceDef::DropGlue(_, Some(ty))
| ty::InstanceDef::AsyncDropGlueCtorShim(_, Some(ty)) = instance
&& let ty::Adt(def, args) = ty.kind()
{
let fields = def.all_fields();
Expand Down
Loading
Loading