-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Unclear lifetime error in closure producing a future #74497
Comments
This note
Is supposed to define the lifetime cc #74072 |
This code is invalid. The closure is given a The error message here is obviously not great. We have specialized errors for these kinds of cases in closures normally, I'm not sure why it didn't trigger in this case. |
There isn't a great workaround that uses stable syntax to make this compile, interestingly. We discussed it some on Zulip. The error message for a comparable scenario without async isn't that great either, actually, I am somewhat surprised (playground):
|
Returning a reference to a local variable is somewhat better (playground):
|
…r=estebank Refactor `region_name`: add `RegionNameHighlight` This PR does not change any diagnostics itself, rather it enables further code changes, but I would like to get approval for the refactoring first before making use of it. In `rustc_mir::borrow_check::diagnostics::region_name`, there is code that allows for, when giving a synthesized name like `'1` to an anonymous lifetime, pointing at e.g. the exact '`&`' that introduces the lifetime. This PR decouples that code from the specific case of arguments, adding a new enum `RegionNameHighlight`, enabling future changes to use it in other places. This allows: * We could change the other `AnonRegionFrom*` variants to use `RegionNameHighlight` to precisely point at where lifetimes are introduced in other locations when they have type annotations, e.g. a closure return `|...| -> &i32`. * Because of how async functions are lowered this affects async functions as well, see rust-lang#74072 * for rust-lang#74597, we could add a second, optional `RegionNameHighlight` to the `AnonRegionFromArgument` variant that highlights a lifetime in the return type of a function when, due to elision, this is the same as the argument lifetime. * in rust-lang#74497 (comment) I noticed that a diagnostic was trying to introduce a lifetime `'2` in the opaque type `impl std::future::Future`. The code for the case of arguments has [code to handle cases like this](https://github.com/rust-lang/rust/blob/bbebe7351fcd29af1eb9a35e315369b15887ea09/src/librustc_mir/borrow_check/diagnostics/region_name.rs#L365) but not the others. This refactoring would allow the same code path to handle this. * It might be appropriate to add another variant of `RegionNameHighlight` to say something like `lifetime '1 appears in the opaque type impl std::future::Future`. These are quite a few changes so I thought I would make sure the refactoring is OK before I start making changes that rely on it. :)
…r=estebank Refactor `region_name`: add `RegionNameHighlight` This PR does not change any diagnostics itself, rather it enables further code changes, but I would like to get approval for the refactoring first before making use of it. In `rustc_mir::borrow_check::diagnostics::region_name`, there is code that allows for, when giving a synthesized name like `'1` to an anonymous lifetime, pointing at e.g. the exact '`&`' that introduces the lifetime. This PR decouples that code from the specific case of arguments, adding a new enum `RegionNameHighlight`, enabling future changes to use it in other places. This allows: * We could change the other `AnonRegionFrom*` variants to use `RegionNameHighlight` to precisely point at where lifetimes are introduced in other locations when they have type annotations, e.g. a closure return `|...| -> &i32`. * Because of how async functions are lowered this affects async functions as well, see rust-lang#74072 * for rust-lang#74597, we could add a second, optional `RegionNameHighlight` to the `AnonRegionFromArgument` variant that highlights a lifetime in the return type of a function when, due to elision, this is the same as the argument lifetime. * in rust-lang#74497 (comment) I noticed that a diagnostic was trying to introduce a lifetime `'2` in the opaque type `impl std::future::Future`. The code for the case of arguments has [code to handle cases like this](https://github.com/rust-lang/rust/blob/bbebe7351fcd29af1eb9a35e315369b15887ea09/src/librustc_mir/borrow_check/diagnostics/region_name.rs#L365) but not the others. This refactoring would allow the same code path to handle this. * It might be appropriate to add another variant of `RegionNameHighlight` to say something like `lifetime '1 appears in the opaque type impl std::future::Future`. These are quite a few changes so I thought I would make sure the refactoring is OK before I start making changes that rely on it. :)
…mulacrum Improve lifetime name annotations for closures & async functions * Don't refer to async functions as "generators" in error output * Where possible, emit annotations pointing exactly at the `&` in the return type of closures (when they have explicit return types) and async functions, like we do for arguments. Addresses rust-lang#74072, but I wouldn't call that *closed* until annotations are identical for async and non-async functions. * Emit a better annotation when the lifetime doesn't appear in the full name type, which currently happens for opaque types like `impl Future`. Addresses rust-lang#74497, but further improves could probably be made (why *doesn't* it appear in the type as `impl Future + '1`?) This is included in the same PR because the changes to `give_name_if_anonymous_region_appears_in_output` would introduce ICE otherwise (it would return `None` in cases where it didn't previously, which then gets `unwrap`ped)
If the Future which returned by f(&32) awaited out of &32's lifetime, I think so too. This code will work:
|
One of the things I find surprising here is when the error messages talk about |
@WinLinux1028 - That's interesting that the Also, is the |
Yeah, it looks like @WinLinux1028's latest example works without |
Triage: the current output is the same as for sync
|
@rustbot label +AsyncAwait-polish |
This should point at the bounds in pub async fn foo<'a, F, T>(f: F) -> bool
where
F: Fn(&'a u8) -> T,
T: Future<Output = bool> + 'a, |
Current output of the original test case:
|
I will try to resurrect this with a deep dive into it @rustbot claim |
Okay, after spending some time on it, I've come up with a summary. This is the complete example that includes the error, and also explains why this code will not work. use std::future::Future;
pub async fn bar() {
foo(|x| baz(x)).await;
}
pub async fn baz(x: &u8) -> bool {
if *x == 1 {
false
} else {
true
}
}
/// From Nico
///
/// This code is invalid. The closure is given a &u8, which is only valid
/// during the closure execution. But baz(x) captures x and embeds it
/// into a future that is returned -- this future, when awaited, will
/// (or could) attempt to use x, even though the closure itself has
/// returned, and hence x is no longer valid.
///
/// The error message here is obviously not great. We have specialized
/// errors for these kinds of cases in closures normally, I'm not sure
/// why it didn't trigger in this case.
pub async fn foo<F, T>(f: F) -> bool
where
F: Fn(&u8) -> T,
T: Future<Output = bool>,
{
f(&32).await
}
fn main() {} But it seems that, upon further reflection, the point made by @WinLinux1028 and analyzed by @eholk is correct. I wasn't able to find a counterexample where this code doesn't work. However, I do need to say that the solution proposed by @estebank is more attractive to me because it exactly describes what the callback does (since I don't like using 'static lifetimes). Here is the revised code: use std::future::Future;
pub async fn bar() {
foo(|x| baz(x)).await;
}
pub async fn baz(x: &u8) -> bool {
if *x == 1 {
false
} else {
true
}
}
pub async fn foo<'a, F, T>(f: F) -> bool
where
F: Fn(&'a u8) -> T,
T: Future<Output = bool>,
{
f(&32).await
}
fn main() {} Furthermore, both synchronous and asynchronous code produce the same error. Therefore, I believe that this issue should be addressed for both types of code, rather than just the asynchronous function. As a result, I intend to propose this matter for discussion at the upcoming async triage meeting, in order to receive feedback on the solution suggested by @eholk for inclusion in the compiler. I did not check in deep, but this looks like another instance #102201 @rustbot label -AsyncAwait-Triaged +I-async-nominated |
We revisited this in @rust-lang/wg-async triage today. We think the behavior of the compiler is correct, but there's room to improve the error message. We'll spend some more time iterating on what an improved error message would look like. |
Here's the minimal reproduction of this issue: fn foo<F: Fn(&()) -> T, T>(f: F) {
f(&());
}
// error: lifetime may not live long enough
fn test() {
foo(|x: &()| x);
// - - ^ returning this value requires that `'1` must outlive `'2`
// | |
// | return type of closure is &'2 ()
// let's call the lifetime of this reference `'1`
} (Thanks to @compiler-errors.) |
This restricts |
Code:
produces following error:
Is this error correct? If yes, why?
following also doesn't work:
error (still not clear):
meta:
The text was updated successfully, but these errors were encountered: