Skip to content

rustc_borrowck: fix async closure error note to report FnOnce rather than Fn #148713

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

rustc_borrowck: fix async closure error note to report FnOnce rather than Fn #148713

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
10 changes: 8 additions & 2 deletions compiler/rustc_borrowck/src/diagnostics/region_name.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -331,7 +331,12 @@ impl<'tcx> MirBorrowckCtxt<'_, '_, 'tcx> {

let closure_kind = match def_ty {
DefiningTy::Closure(_, args) => args.as_closure().kind(),
DefiningTy::CoroutineClosure(_, args) => args.as_coroutine_closure().kind(),
DefiningTy::CoroutineClosure(_, args) => {
match args.as_coroutine_closure().kind() {
ty::ClosureKind::Fn => ty::ClosureKind::FnOnce,
Copy link
Contributor

@lcnr lcnr Nov 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

didn't look into this myself, but this seems... quite suboptimal.

Why are we not properly updating the kind for coroutine closures instead so that we don't have to deal with this weird behavior here?

Copy link
Contributor Author

@InvalidPathException InvalidPathException Nov 10, 2025

Choose a reason for hiding this comment

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

rust/compiler/rustc_hir_typeck/src/closure.rs

Lines 206 to 252 in a7b3715

}
};
// Compute all of the variables that will be used to populate the coroutine.
let resume_ty = self.next_ty_var(expr_span);
let closure_kind_ty = match expected_kind {
Some(kind) => Ty::from_closure_kind(tcx, kind),
// Create a type variable (for now) to represent the closure kind.
// It will be unified during the upvar inference phase (`upvar.rs`)
None => self.next_ty_var(expr_span),
};
let coroutine_captures_by_ref_ty = self.next_ty_var(expr_span);
let closure_args = ty::CoroutineClosureArgs::new(
tcx,
ty::CoroutineClosureArgsParts {
parent_args,
closure_kind_ty,
signature_parts_ty: Ty::new_fn_ptr(
tcx,
bound_sig.map_bound(|sig| {
tcx.mk_fn_sig(
[
resume_ty,
Ty::new_tup_from_iter(tcx, sig.inputs().iter().copied()),
],
Ty::new_tup(tcx, &[bound_yield_ty, bound_return_ty]),
sig.c_variadic,
sig.safety,
sig.abi,
)
}),
),
tupled_upvars_ty,
coroutine_captures_by_ref_ty,
},
);
let coroutine_kind_ty = match expected_kind {
Some(kind) => Ty::from_coroutine_closure_kind(tcx, kind),
// Create a type variable (for now) to represent the closure kind.
// It will be unified during the upvar inference phase (`upvar.rs`)
None => self.next_ty_var(expr_span),
};

It is marked as Fn at line 212, I don't know if I am wording it correctly but my understanding is the existing code conflates calling capability with the actual type/behavior. This is the root cause.

rust/compiler/rustc_hir_typeck/src/upvar.rs

Lines 251 to 261 in a7b3715

// 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 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.
//
// FIXME(async_closures): This could be cleaned up. It's a bit janky that we're just
// moving all of the `LocalSource::AsyncFn` locals here.
if let Some(hir::CoroutineKind::Desugared(

This fixme seems related. It could be a way larger refactor and I am not sure if we want to have the right behavior first, or do the refactor as well. It probably warrants more discussion.

rust/src/doc/rustc-dev-guide/src/coroutine-closures.md

Lines 269 to 287 in a7b3715

### Instance resolution
If a coroutine-closure has a closure-kind of `FnOnce`, then its `AsyncFnOnce::call_once` and `FnOnce::call_once` implementations resolve to the coroutine-closure's body[^res1], and the `Future::poll` of the coroutine that gets returned resolves to the body of the child closure.
[^res1]: <https://github.com/rust-lang/rust/blob/705cfe0e966399e061d64dd3661bfbc57553ed87/compiler/rustc_ty_utils/src/instance.rs#L351>
If a coroutine-closure has a closure-kind of `FnMut`/`Fn`, then the same applies to `AsyncFn` and the corresponding `Future` implementation of the coroutine that gets returned.[^res1] However, we use a MIR shim to generate the implementation of `AsyncFnOnce::call_once`/`FnOnce::call_once`[^res2], and `Fn::call`/`FnMut::call_mut` instances if they exist[^res3].
[^res2]: <https://github.com/rust-lang/rust/blob/705cfe0e966399e061d64dd3661bfbc57553ed87/compiler/rustc_ty_utils/src/instance.rs#L341-L349>
[^res3]: <https://github.com/rust-lang/rust/blob/705cfe0e966399e061d64dd3661bfbc57553ed87/compiler/rustc_ty_utils/src/instance.rs#L312-L326>
This is represented by the `ConstructCoroutineInClosureShim`[^i1]. The `receiver_by_ref` bool will be true if this is the instance of `Fn::call`/`FnMut::call_mut`.[^i2] The coroutine that all of these instances returns corresponds to the by-move body we will have synthesized by this point.[^i3]
[^i1]: <https://github.com/rust-lang/rust/blob/705cfe0e966399e061d64dd3661bfbc57553ed87/compiler/rustc_middle/src/ty/instance.rs#L129-L134>
[^i2]: <https://github.com/rust-lang/rust/blob/705cfe0e966399e061d64dd3661bfbc57553ed87/compiler/rustc_middle/src/ty/instance.rs#L136-L141>
[^i3]: <https://github.com/rust-lang/rust/blob/07cbbdd69363da97075650e9be24b78af0bcdd23/compiler/rustc_middle/src/ty/instance.rs#L841>

Might need to update the docs too if we do that.

kind => kind,
}
}
_ => {
// Can't have BrEnv in functions, constants or coroutines.
bug!("BrEnv outside of closure.");
Expand All @@ -353,7 +358,8 @@ impl<'tcx> MirBorrowckCtxt<'_, '_, 'tcx> {
can't escape the closure"
}
ty::ClosureKind::FnOnce => {
bug!("BrEnv in a `FnOnce` closure");
"closure implements `FnOnce`, so references to captured variables \
can't escape the closure"
}
};
Copy link
Contributor

@cjgillot cjgillot Nov 8, 2025

Choose a reason for hiding this comment

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

Any way this can look like

let note = format!("closure implements `{}`, so references to captured variables can't escape the closure", closure_kind.as_str());

Copy link
Contributor Author

@InvalidPathException InvalidPathException Nov 9, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I did think about this. Correct me if I'm wrong, RegionNameSource stores these as &'static str but format! gives String, so we cannot really get around it (at least in this way). If we do refactor then it might be a lot of diff for something we do not touch for years.


Expand Down
4 changes: 2 additions & 2 deletions tests/ui/async-await/async-closures/not-lending.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ LL | let x = async move || -> &String { &s };
| | return type of async closure `{async closure body@$DIR/not-lending.rs:12:42: 12:48}` contains a lifetime `'2`
| lifetime `'1` represents this closure's body
|
= note: closure implements `Fn`, so references to captured variables can't escape the closure
= note: closure implements `FnOnce`, so references to captured variables can't escape the closure

error: lifetime may not live long enough
--> $DIR/not-lending.rs:16:31
Expand All @@ -18,7 +18,7 @@ LL | let x = async move || { &s };
| | return type of async closure `{async closure body@$DIR/not-lending.rs:16:31: 16:37}` contains a lifetime `'2`
| lifetime `'1` represents this closure's body
|
= note: closure implements `Fn`, so references to captured variables can't escape the closure
= note: closure implements `FnOnce`, so references to captured variables can't escape the closure

error: aborting due to 2 previous errors

Loading