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

async fn can't handle multiple lifetimes in a slice of slices #76547

Open
asomers opened this issue Sep 9, 2020 · 10 comments
Open

async fn can't handle multiple lifetimes in a slice of slices #76547

asomers opened this issue Sep 9, 2020 · 10 comments
Labels
A-async-await Area: Async & Await A-diagnostics Area: Messages for errors, warnings, and lints A-lifetimes Area: lifetime related AsyncAwait-Polish Async-await issues that are part of the "polish" area AsyncAwait-Triaged Async-await issues that have been triaged during a working group meeting. C-bug Category: This is a bug. D-confusing Confusing diagnostic error that should be reworked E-easy Call for participation: Experience needed to fix: Easy / not much (good first issue) E-help-wanted Call for participation: Help is requested to fix this issue. E-mentor Call for participation: This issue is currently mentored.

Comments

@asomers
Copy link
Contributor

asomers commented Sep 9, 2020

I tried this code:

use futures::{
    Future,
    task::{Context, Poll}
};
use std::pin::Pin;

pub struct ListFut<'a>(&'a mut [&'a mut [u8]]);

impl<'a> Future for ListFut<'a> {
    type Output = ();

    fn poll(self: Pin<&mut Self>, _cx: &mut Context) -> Poll<Self::Output> {
        unimplemented!()
    }
}

/// The compiler complains that "this parameter and the return type are declared
/// with different lifetimes"
pub async fn readv_at(bufs: &mut [&mut [u8]]) {
    ListFut(bufs).await
}

/// The same thing, but making the lifetimes explicit
pub async fn readv_at2<'a, 'b>(bufs: &'a mut [&'b mut [u8]]) {
    ListFut(bufs).await
}

/// But the compiler accepts this method just fine, because there is only one
/// lifetime.
pub async fn readv_at1<'a>(bufs: &'a mut [&'a mut [u8]]) {
    ListFut(bufs).await
}

/// Even if we explicitly bound one lifetime by the other, the compiler still
/// can't handle it.
pub async fn readv_at3<'a, 'b: 'a>(bufs: &'a mut [&'b mut [u8]]) {
    ListFut(bufs).await
}

I expected to see this happen: No error

Instead, this happened: For every variant with two lifetimes, the compiler complains "these two types are declared with different lifetimes... but data from bufs flows into bufs here"

This looks similar to #56238 , but I see that the commit which closed that issue didn't included any test cases involving slices.

Meta

rustc --version --verbose:

rustc 1.48.0-nightly (d006f5734 2020-08-28)
rustc 1.42.0 (b8cedc004 2020-03-09)
@asomers asomers added the C-bug Category: This is a bug. label Sep 9, 2020
@Matthias247
Copy link
Contributor

Matthias247 commented Sep 10, 2020

As discussed on discord I'm not sure whether this is a bug. It works if ListFut is declared as

pub struct ListFut<'a, 'b>(&'a mut [&'b mut [u8]]);

it works. That allows the 2 lifetimes to be separate - compared the original declaration which forces them to be the same.

It could be that the compiler interprets:

pub async fn readv_at(bufs: &mut [&mut [u8]]) {
    ListFut(bufs).await
}

as

those 2 lifetimes could be different

However I'm not sure whether this makes a difference here. On the first glance it doesn't, because all those references are pure "inputs". Nothing is returned, so as long as the lifetimes are covering the function duration we should be good - independent whether they are the same or different.

However on the other hand readv_at returns a hidden impl Future, which carries the input lifetime forwards. Based on that it could matter if these are different or not.

@taiki-e
Copy link
Member

taiki-e commented Sep 10, 2020

This is because elided lifetimes are handled as different lifetimes. (in this case, handled as &'1 mut &'2 mut [u8])

It can be reproduced not only with async functions but also with normal functions:
playground

pub struct ListFut<'a>(&'a mut [&'a mut [u8]]);

// error
pub fn readv_at(bufs: &mut [&mut [u8]]) {
    ListFut(bufs);
}

// error
pub fn readv_at2<'a, 'b>(bufs: &'a mut [&'b mut [u8]]) {
    ListFut(bufs);
}

// ok
pub fn readv_at1<'a>(bufs: &'a mut [&'a mut [u8]]) {
    ListFut(bufs);
}

// error
pub fn readv_at3<'a, 'b: 'a>(bufs: &'a mut [&'b mut [u8]]) {
    ListFut(bufs);
}

// ok
pub fn readv_at4<'a: 'b, 'b>(bufs: &'a mut [&'b mut [u8]]) {
    ListFut(bufs);
}

@taiki-e
Copy link
Member

taiki-e commented Sep 10, 2020

/// Even if we explicitly bound one lifetime by the other, the compiler still
/// can't handle it.
pub async fn readv_at3<'a, 'b: 'a>(bufs: &'a mut [&'b mut [u8]]) {
    ListFut(bufs).await
}

fwiw, 'b overlives than 'a, so this fails to compile, but you could compile it using a lifetime bound like 'a: 'b, 'b.

@jonas-schievink jonas-schievink added A-async-await Area: Async & Await A-lifetimes Area: lifetime related labels Sep 10, 2020
@tmandry tmandry added A-diagnostics Area: Messages for errors, warnings, and lints D-confusing Confusing diagnostic error that should be reworked labels Sep 15, 2020
@tmandry
Copy link
Contributor

tmandry commented Sep 15, 2020

Discussed in our async foundations triage meeting today.

We can solve this in two parts.. one would be to change the wording "...but data from bufs is returned here" in the async case to make it more clear that we're returning a future (or just not use the special case wording for returned at all.)
EDIT: Added E- labels for this first part.

The other is not async-related, really, but has to do with improving the error message wording and suggestion.

@tmandry tmandry added E-easy Call for participation: Experience needed to fix: Easy / not much (good first issue) E-help-wanted Call for participation: Help is requested to fix this issue. E-mentor Call for participation: This issue is currently mentored. labels Sep 15, 2020
@tmandry tmandry added the AsyncAwait-Triaged Async-await issues that have been triaged during a working group meeting. label Sep 22, 2020
JohnTitor added a commit to JohnTitor/rust that referenced this issue Nov 5, 2020
Make it more clear what an about async fn's returns when referring to what it returns

see rust-lang#76547

This is *likely* not the ONLY place that this happens to be unclear, but we can move this fn to rustc_middle or something like that and reuse it if need be, to apply it to more diagnostics

One outstanding question I have is, if the fn returns (), should I make the message more clear (what about `fn f()` vs `fn f() -> ()`, can you tell those apart in the hir?)

R? `@tmandry`

`@rustbot` modify labels +A-diagnostics +T-compiler
jonas-schievink added a commit to jonas-schievink/rust that referenced this issue Nov 10, 2020
Make it more clear what an about async fn's returns when referring to what it returns

see rust-lang#76547

This is *likely* not the ONLY place that this happens to be unclear, but we can move this fn to rustc_middle or something like that and reuse it if need be, to apply it to more diagnostics

One outstanding question I have is, if the fn returns (), should I make the message more clear (what about `fn f()` vs `fn f() -> ()`, can you tell those apart in the hir?)

R? `@tmandry`

`@rustbot` modify labels +A-diagnostics +T-compiler
@sjakobi
Copy link
Contributor

sjakobi commented Mar 26, 2021

Is there still work to do here now that #76765 has landed?

@halvko
Copy link

halvko commented Nov 6, 2021

On the current nightly:
rustc 1.58.0-nightly (0d1754e 2021-11-05)

The sync version of the problem:

pub struct NestedList<'a>(&'a mut [&'a mut [u8]]);

pub fn readv_at(bufs: &mut [&mut [u8]]) -> NestedList {
    bufs
}

The error messages nicely guides the developer to fix their lifetimes, but in the async version no such guidance is given:

error[E0623]: lifetime mismatch
  --> src/main.rs:24:13
   |
23 | pub async fn readv_at(bufs: &mut [&mut [u8]]) {
   |                                   ---------   -
   |                                   |           |
   |                                   |           this `async fn` implicitly returns an `impl Future<Output = ()>`
   |                                   this parameter and the returned future are declared with different lifetimes...
24 |     ListFut(bufs).await
   |             ^^^^ ...but data from `bufs` is held across an await point here

For an experienced rust developer the first line is somewhat usefull (lifetime mismatch), and the specific await point could be enough of a hint to look at ListFuts documentation, but nothing hints at ListFuts lifetime requirements.

For an not so experienced rust developer I'd say the error message is less than usefull, as it hints at the need to lifetime annotate the returned future.

@tmandry
Copy link
Contributor

tmandry commented Nov 10, 2021

@rustbot label: -AsyncAwait-Triaged

Re-submitting to the triage queue since things have changed.

@rustbot rustbot removed the AsyncAwait-Triaged Async-await issues that have been triaged during a working group meeting. label Nov 10, 2021
@eholk
Copy link
Contributor

eholk commented Nov 15, 2021

@rustbot label +AsyncAwait-Polish

@rustbot rustbot added the AsyncAwait-Polish Async-await issues that are part of the "polish" area label Nov 15, 2021
@eholk eholk added this to On deck in wg-async work Nov 15, 2021
@eholk
Copy link
Contributor

eholk commented Nov 15, 2021

@rustbot label +AsyncAwait-Triaged

@rustbot rustbot added the AsyncAwait-Triaged Async-await issues that have been triaged during a working group meeting. label Nov 15, 2021
@nikomatsakis
Copy link
Contributor

nikomatsakis commented Nov 15, 2021

This is probably covered in the above comments, but just a few notes:

The problem here is that you have &mut [&mut [u8]] in the function, which is indeed two distinct lifetimes, and only one lifetime in the structure. It doesn't seem to be particularly related to async. The error message is just calibrated for the case of "parameters with simple types" (that have a single lifetime) and not able to cope with this case. Definitely a bug, but a diganostic bug.

Playground for a non-async version that gets the same error.

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Jan 15, 2022
Point at correct argument when async fn output type lifetime disagrees with signature

Fixes most of rust-lang#74256.

## Problems fixed

This PR fixes a couple of related problems in the error reporting code.

### Highlighting the wrong argument

First, the error reporting code was looking at the desugared return type of an `async fn` to decide which parameter to highlight. For example, a function like

```rust
async fn async_fn(self: &Struct, f: &u32) -> &u32
{ f }
```

desugars to

```rust
async fn async_fn<'a, 'b>(self: &'a Struct, f: &'b u32)
-> impl Future<Output = &'a u32> + 'a + 'b
{ f }
```

Since `f: &'b u32` is returned but the output type is `&'a u32`, the error would occur when checking that `'a: 'b`.

The reporting code would look to see if the "offending" lifetime `'b` was included in the return type, and because the code was looking at the desugared future type, it was included. So it defaulted to reporting that the source of the other lifetime `'a` (the `self` type) was the problem, when it was really the type of `f`. (Note that if it had chosen instead to look at `'a` first, it too would have been included in the output type, and it would have arbitrarily reported the error (correctly this time) on the type of `f`.)

Looking at the actual future type isn't useful for this reason; it captures all input lifetimes. Using the written return type for `async fn` solves this problem and results in less confusing error messages for the user.

This isn't a perfect fix, unfortunately; writing the "manually desugared" form of the above function still results in the wrong parameter being highlighted. Looking at the output type of every `impl Future` return type doesn't feel like a very principled approach, though it might work. The problem would remain for function signatures that look like the desugared one above but use different traits. There may be deeper changes required to pinpoint which part of each type is conflicting.

### Lying about await point capture causing lifetime conflicts

The second issue fixed by this PR is the unnecessary complexity in `try_report_anon_anon_conflict`. It turns out that the root cause I suggested in rust-lang#76547 (comment) wasn't really the root cause. Adding special handling to report that a variable was captured over an await point only made the error messages less correct and pointed to a problem other than the one that actually occurred.

Given the above discussion, it's easy to see why: `async fn`s capture all input lifetimes in their return type, so holding an argument across an await point should never cause a lifetime conflict! Removing the special handling simplified the code and improved the error messages (though they still aren't very good!)

## Future work

* Fix error reporting on the "desugared" form of this code
* Get the `suggest_adding_lifetime_params` suggestion firing on these examples
  * cc rust-lang#42703, I think

r? `@estebank`
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Jan 20, 2022
Point at correct argument when async fn output type lifetime disagrees with signature

Fixes most of rust-lang#74256.

## Problems fixed

This PR fixes a couple of related problems in the error reporting code.

### Highlighting the wrong argument

First, the error reporting code was looking at the desugared return type of an `async fn` to decide which parameter to highlight. For example, a function like

```rust
async fn async_fn(self: &Struct, f: &u32) -> &u32
{ f }
```

desugars to

```rust
async fn async_fn<'a, 'b>(self: &'a Struct, f: &'b u32)
-> impl Future<Output = &'a u32> + 'a + 'b
{ f }
```

Since `f: &'b u32` is returned but the output type is `&'a u32`, the error would occur when checking that `'a: 'b`.

The reporting code would look to see if the "offending" lifetime `'b` was included in the return type, and because the code was looking at the desugared future type, it was included. So it defaulted to reporting that the source of the other lifetime `'a` (the `self` type) was the problem, when it was really the type of `f`. (Note that if it had chosen instead to look at `'a` first, it too would have been included in the output type, and it would have arbitrarily reported the error (correctly this time) on the type of `f`.)

Looking at the actual future type isn't useful for this reason; it captures all input lifetimes. Using the written return type for `async fn` solves this problem and results in less confusing error messages for the user.

This isn't a perfect fix, unfortunately; writing the "manually desugared" form of the above function still results in the wrong parameter being highlighted. Looking at the output type of every `impl Future` return type doesn't feel like a very principled approach, though it might work. The problem would remain for function signatures that look like the desugared one above but use different traits. There may be deeper changes required to pinpoint which part of each type is conflicting.

### Lying about await point capture causing lifetime conflicts

The second issue fixed by this PR is the unnecessary complexity in `try_report_anon_anon_conflict`. It turns out that the root cause I suggested in rust-lang#76547 (comment) wasn't really the root cause. Adding special handling to report that a variable was captured over an await point only made the error messages less correct and pointed to a problem other than the one that actually occurred.

Given the above discussion, it's easy to see why: `async fn`s capture all input lifetimes in their return type, so holding an argument across an await point should never cause a lifetime conflict! Removing the special handling simplified the code and improved the error messages (though they still aren't very good!)

## Future work

* Fix error reporting on the "desugared" form of this code
* Get the `suggest_adding_lifetime_params` suggestion firing on these examples
  * cc rust-lang#42703, I think

r? `@estebank`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-async-await Area: Async & Await A-diagnostics Area: Messages for errors, warnings, and lints A-lifetimes Area: lifetime related AsyncAwait-Polish Async-await issues that are part of the "polish" area AsyncAwait-Triaged Async-await issues that have been triaged during a working group meeting. C-bug Category: This is a bug. D-confusing Confusing diagnostic error that should be reworked E-easy Call for participation: Experience needed to fix: Easy / not much (good first issue) E-help-wanted Call for participation: Help is requested to fix this issue. E-mentor Call for participation: This issue is currently mentored.
Projects
Development

No branches or pull requests

10 participants