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

[InvalidPathException]

Fixes #148391

Root cause:
async closures are treated like plain Fn closures because we never updated their kind after adding the async coroutine path.

Symptom:
The compiler note tells users closure implements Fn…, which is wrong for async closures and made the error message confusing. Notably FnMut does not suffer from this issue.

Fix:
With this fix we treat coroutine closures as FnOnce when building that note.

Testing:
Updated a UI test stderr file to reflect the correct wording. It also clearly demonstrates what was wrong.

[rustbot]

r? @lcnr

rustbot has assigned @lcnr.
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

[cjgillot]

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());

[InvalidPathException]

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.

[lcnr]

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?

[InvalidPathException]

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.

[lcnr]

This is confusing, asking for clarification in #148391

[lcnr]

I feel like mentioning FnOnce is the wrong fix here. While the async closure does not implement Fn, it implements AsyncFn.

The closure kind is actually correct, just somewhat confusingly named... just because kind is Fn, async closures are not guaranteed to impl Fn, but may only implement AsyncFn instead.

This is to support the future which has been returned by calling the async closure borrowing from the closure itself, see #132706.

We should instead change this diagnostic to talk about AsyncFn where applicable

[lcnr]

so the fix here would be to check whether args.has_self_borrows and if so, talk about AsyncFn(Mut) instead