Skip to content
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

Skip over args when determining if async-closure's inner coroutine consumes its upvars #128520

Merged
merged 1 commit into from
Aug 8, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
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
66 changes: 53 additions & 13 deletions compiler/rustc_hir_typeck/src/upvar.rs
Original file line number Diff line number Diff line change
Expand Up @@ -219,19 +219,15 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
// `async-await/async-closures/force-move-due-to-inferred-kind.rs`.
//
// 2. If the coroutine-closure is forced to be `FnOnce` due to the way it
// uses its upvars, but not *all* upvars would force the closure to `FnOnce`.
// uses its upvars (e.g. it consumes a non-copy value), but not *all* upvars
// would force the closure to `FnOnce`.
// See the test: `async-await/async-closures/force-move-due-to-actually-fnonce.rs`.
//
// This would lead to an impossible to satisfy situation, since `AsyncFnOnce`
// coroutine bodies can't borrow from their parent closure. To fix this,
// we force the inner coroutine to also be `move`. This only matters for
// coroutine-closures that are `move` since otherwise they themselves will
// be borrowing from the outer environment, so there's no self-borrows occuring.
//
// One *important* note is that we do a call to `process_collected_capture_information`
// to eagerly test whether the coroutine would end up `FnOnce`, but we do this
// *before* capturing all the closure args by-value below, since that would always
// cause the analysis to return `FnOnce`.
if let UpvarArgs::Coroutine(..) = args
&& let hir::CoroutineKind::Desugared(_, hir::CoroutineSource::Closure) =
self.tcx.coroutine_kind(closure_def_id).expect("coroutine should have kind")
Expand All @@ -246,19 +242,16 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
capture_clause = hir::CaptureBy::Value { move_kw };
}
// (2.) The way that the closure uses its upvars means it's `FnOnce`.
else if let (_, ty::ClosureKind::FnOnce, _) = self
.process_collected_capture_information(
capture_clause,
&delegate.capture_information,
)
{
else if self.coroutine_body_consumes_upvars(closure_def_id, body) {
capture_clause = hir::CaptureBy::Value { move_kw };
}
}

// As noted in `lower_coroutine_body_with_moved_arguments`, we default the capture mode
// to `ByRef` for the `async {}` block internal to async fns/closure. This means
// that we would *not* be moving all of the parameters into the async block by default.
// that we would *not* be moving all of the parameters into the async block in all cases.
// For example, when one of the arguments is `Copy`, we turn a consuming use into a copy of
// a reference, so for `async fn x(t: i32) {}`, we'd only take a reference to `t`.
//
// We force all of these arguments to be captured by move before we do expr use analysis.
//
Expand Down Expand Up @@ -535,6 +528,53 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
}
}

/// Determines whether the body of the coroutine uses its upvars in a way that
/// consumes (i.e. moves) the value, which would force the coroutine to `FnOnce`.
/// In a more detailed comment above, we care whether this happens, since if
/// this happens, we want to force the coroutine to move all of the upvars it
/// would've borrowed from the parent coroutine-closure.
///
/// This only really makes sense to be called on the child coroutine of a
/// coroutine-closure.
fn coroutine_body_consumes_upvars(
&self,
coroutine_def_id: LocalDefId,
body: &'tcx hir::Body<'tcx>,
) -> bool {
// This block contains argument capturing details. Since arguments
// aren't upvars, we do not care about them for determining if the
// coroutine body actually consumes its upvars.
let hir::ExprKind::Block(&hir::Block { expr: Some(body), .. }, None) = body.value.kind
else {
bug!();
};
// Specifically, we only care about the *real* body of the coroutine.
// We skip out into the drop-temps within the block of the body in order
// to skip over the args of the desugaring.
let hir::ExprKind::DropTemps(body) = body.kind else {
bug!();
};

let mut delegate = InferBorrowKind {
closure_def_id: coroutine_def_id,
capture_information: Default::default(),
fake_reads: Default::default(),
};

let _ = euv::ExprUseVisitor::new(
&FnCtxt::new(self, self.tcx.param_env(coroutine_def_id), coroutine_def_id),
&mut delegate,
)
.consume_expr(body);

let (_, kind, _) = self.process_collected_capture_information(
hir::CaptureBy::Ref,
&delegate.capture_information,
);

matches!(kind, ty::ClosureKind::FnOnce)
}

// Returns a list of `Ty`s for each upvar.
fn final_upvar_tys(&self, closure_id: LocalDefId) -> Vec<Ty<'tcx>> {
self.typeck_results
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
//@ aux-build:block-on.rs
//@ edition:2021
//@ build-pass

#![feature(async_closure)]

extern crate block_on;

fn wrapper(f: impl Fn(String)) -> impl async Fn(String) {
async move |s| f(s)
}
Comment on lines +9 to +11
Copy link
Member

Choose a reason for hiding this comment

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

So, the closure captures f by value, and then the future captures the upvar by ref?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep.


fn main() {
block_on::block_on(async {
wrapper(|who| println!("Hello, {who}!"))(String::from("world")).await;
});
}
Loading