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

E0623 highlights wrong parameter in async fn #74256

Open
SNCPlay42 opened this issue Jul 11, 2020 · 13 comments
Open

E0623 highlights wrong parameter in async fn #74256

SNCPlay42 opened this issue Jul 11, 2020 · 13 comments
Assignees
Labels
A-async-await A-diagnostics AsyncAwait-Polish AsyncAwait-Triaged C-bug D-incorrect NLL-fixed-by-NLL T-compiler

Comments

@SNCPlay42
Copy link
Contributor

SNCPlay42 commented Jul 11, 2020

struct Struct { }

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

Output (playground)

   Compiling playground v0.0.1 (/playground)
error[E0623]: lifetime mismatch
 --> src/main.rs:5:9
  |
4 |     async fn async_fn(self: &Struct, f: &u32) -> &u32 {
  |                             -------              ----
  |                             |
  |                             this parameter and the return type are declared with different lifetimes...
5 |         f
  |         ^ ...but data from `f` is returned here

error[E0623]: lifetime mismatch
 --> src/main.rs:9:9
  |
8 |     fn sync_fn(self: &Struct, f: &u32) -> &u32 {
  |                                  ----     ----
  |                                  |
  |                                  this parameter and the return type are declared with different lifetimes...
9 |         f
  |         ^ ...but data from `f` is returned here

error: aborting due to 2 previous errors

For more information about this error, try `rustc --explain E0623`.
error: could not compile `playground`.

To learn more, run the command again with --verbose.

The error in sync_fn is correct, but the one for async_fn incorrectly claims the return type lifetime is different from self's lifetime, when they are in fact the same per the elision rules.

@jonas-schievink jonas-schievink added A-async-await A-diagnostics C-bug T-compiler labels Jul 11, 2020
@ayazhafiz
Copy link
Contributor

ayazhafiz commented Jul 12, 2020

related: #63499

@tmandry
Copy link
Contributor

tmandry commented Jul 14, 2020

Seems related to #74072. Triaging as On Deck since this is an incorrect diagnostic which can cause confusion.

@tmandry tmandry added AsyncAwait-Triaged D-incorrect labels Jul 14, 2020
@tmandry tmandry added this to On deck in wg-async work via automation Jul 14, 2020
@SNCPlay42
Copy link
Contributor Author

SNCPlay42 commented Jul 22, 2020

Another case:

async fn async_fn(x: &i32, y: &mut &i32) {
    *y = x;
}

fn sync_fn(x: &i32, y: &mut &i32) {
    *y = x;
}

Output (playground)

error[E0623]: lifetime mismatch
 --> src/main.rs:2:10
  |
1 | async fn async_fn(x: &i32, y: &mut &i32) {
  |                                    ----  -
  |                                    |
  |                                    this parameter and the return type are declared with different lifetimes...
2 |     *y = x;
  |          ^ ...but data from `x` is returned here

error[E0623]: lifetime mismatch
 --> src/main.rs:6:10
  |
5 | fn sync_fn(x: &i32, y: &mut &i32) {
  |               ----          ----
  |               |
  |               these two types are declared with different lifetimes...
6 |     *y = x;
  |          ^ ...but data from `x` flows into `y` here

The error message for async_fn refers to returning when no returning is involved!

@SNCPlay42
Copy link
Contributor Author

SNCPlay42 commented Jul 22, 2020

Output for the async functions in both cases with #![feature(nll)]: (playground)

error: lifetime may not live long enough
 --> src/main.rs:4:5
  |
3 | async fn async_fn(x: &i32, y: &mut &i32) {
  |                      -             - let's call the lifetime of this reference `'2`
  |                      |
  |                      let's call the lifetime of this reference `'1`
4 |     *y = x;
  |     ^^^^^^ assignment requires that `'1` must outlive `'2`

error: lifetime may not live long enough
  --> src/main.rs:11:9
   |
10 |     async fn async_fn(self: &Struct, f: &u32) -> &u32 {
   |                             -           - let's call the lifetime of this reference `'1`
   |                             |
   |                             let's call the lifetime of this reference `'2`
11 |         f
   |         ^ associated function was supposed to return data with lifetime `'2` but it is returning data with lifetime `'1`

Looks like this can be tagged as fixed by NLL.

@csmoe csmoe added the NLL-fixed-by-NLL label Feb 26, 2021
@eholk
Copy link
Contributor

eholk commented Sep 8, 2021

@SNCPlay42 - since this is tagged as fixed by NLL, do you think we can go ahead and close the whole issue?

@eholk eholk moved this from On deck to In progress in wg-async work Sep 8, 2021
@tmandry
Copy link
Contributor

tmandry commented Sep 9, 2021

This still reproduces for me. Adding #![feature(nll)] does fix the issue, I'm not sure what we're using that feature for nowadays.

@eholk
Copy link
Contributor

eholk commented Sep 16, 2021

@rustbot claim

eholk added a commit to eholk/rust that referenced this issue Sep 29, 2021
@eholk
Copy link
Contributor

eholk commented Sep 29, 2021

I think we can make some more significant improvements to the error message here, since it's taken me awhile to figure out what it's really trying to tell me. Since this issue was filed, the error message has changed slightly (probably because of this change. Here's the current error message:

   Compiling playground v0.0.1 (/playground)
error[E0623]: lifetime mismatch
 --> src/main.rs:5:9
  |
4 |     async fn async_fn(self: &Struct, f: &u32) -> &u32 {
  |                             -------              ----
  |                             |                    |
  |                             |                    this `async fn` implicitly returns an `impl Future<Output = &u32>`
  |                             this parameter and the returned future are declared with different lifetimes...
5 |         f
  |         ^ ...but data from `f` is held across an await point here

error[E0623]: lifetime mismatch
 --> src/main.rs:9:9
  |
8 |     fn sync_fn(self: &Struct, f: &u32) -> &u32 {
  |                                  ----     ----
  |                                  |
  |                                  this parameter and the return type are declared with different lifetimes...
9 |         f
  |         ^ ...but data from `f` is returned here

For more information about this error, try `rustc --explain E0623`.
error: could not compile `playground` due to 2 previous errors

To me the most confusing thing, in both the sync and async case, is that it's not clear how the parameters and the return values are declared with different lifetimes. I thought "they aren't declared with any lifetime, can't the compiler infer one that works?" So I tried a similar example that does not use an impl:

fn sync_fn(_: &i32, f: &u32) -> &u32 {
    f
}

For this one, the error is:

error[E0106]: missing lifetime specifier
 --> src/lib.rs:1:33
  |
1 | fn sync_fn(_: &i32, f: &u32) -> &u32 {
  |               ----     ----     ^ expected named lifetime parameter
  |
  = help: this function's return type contains a borrowed value, but the signature does not say whether it is borrowed from argument 1 or `f`
help: consider introducing a named lifetime parameter
  |
1 | fn sync_fn<'a>(_: &'a i32, f: &'a u32) -> &'a u32 {
  |           ^^^^    ^^^^^^^     ^^^^^^^     ^^^

For more information about this error, try `rustc --explain E0106`.
error: could not compile `playground` due to previous error

This one is a lot clearer to me. Basically, the compiler is saying "there are too many lifetimes here and I can't tell which ones you meant to relate to which others. Please be more explicit to help me figure it out."

In the impl case, it seems like we have a lifetime elision rule that assumes returned references are borrowed from self. I think in both the sync and async examples, it'd be nice to have the error message explicitly call out that the return value is borrowed from self.

@SNCPlay42
Copy link
Contributor Author

SNCPlay42 commented Sep 30, 2021

In the impl case, it seems like we have a lifetime elision rule that assumes returned references are borrowed from self. I think in both the sync and async examples, it'd be nice to have the error message explicitly call out that the return value is borrowed from self.

See #74597

@eholk
Copy link
Contributor

eholk commented Oct 14, 2021

I'm going to unassign myself from this for I can focus more on the generator captures issue.

@eholk eholk removed their assignment Oct 14, 2021
@eholk eholk moved this from In progress (current sprint) to On deck in wg-async work Oct 14, 2021
@tmandry
Copy link
Contributor

tmandry commented Oct 14, 2021

@rustbot claim

@eholk eholk moved this from On deck to In progress (current sprint) in wg-async work Oct 14, 2021
@eholk
Copy link
Contributor

eholk commented Oct 18, 2021

@rustbot label +AsyncAwait-Polish

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`
@tmandry
Copy link
Contributor

tmandry commented Apr 14, 2022

There is a remaining issue not fixed by #92183 which I'll attempt to elucidate here.

In try_report_anon_anon_conflict, we are reporting an error with two anonymous lifetimes. We rely on the fact that anonymous lifetimes can only be introduced in parameters, and the only other place they can appear is the return type. To check if the conflict occurs in the return type, we check to see if either lifetime appears in the return type. If it does, we highlight the return type and the parameter corresponding to the other lifetime.

This is a little broken. Just because a lifetime appears in the return type does not necessarily mean the return type had anything to do with the current error. We want to know what types the failing lifetime obligation came from, and the span of those types. For example, for each lifetime it could be

  • The return type
  • The type of the parameter that introduced the lifetime
  • An associated type embedded in either of the above (e.g., impl Future<Output = &'a i32>)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-async-await A-diagnostics AsyncAwait-Polish AsyncAwait-Triaged C-bug D-incorrect NLL-fixed-by-NLL T-compiler
Projects
wg-async work
In progress (current sprint)
Development

No branches or pull requests

7 participants