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

Point at correct argument when async fn output type lifetime disagrees with signature #92183

Merged
merged 3 commits into from
Jan 20, 2022

Conversation

tmandry
Copy link
Member

@tmandry tmandry commented Dec 22, 2021

Fixes most of #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

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

desugars to

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 #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 fns 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

r? @estebank

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Dec 22, 2021
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 22, 2021
@rust-log-analyzer

This comment has been minimized.

@tmandry
Copy link
Member Author

tmandry commented Jan 14, 2022

@bors r=estebank

@bors
Copy link
Contributor

bors commented Jan 14, 2022

📌 Commit 50ac0a3 has been approved by estebank

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 14, 2022
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request 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
Copy link
Member

matthiaskrgr commented Jan 15, 2022

Failed on windows in a rollup: #92925 (comment)
@bors r-

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jan 15, 2022
@bors
Copy link
Contributor

bors commented Jan 17, 2022

☔ The latest upstream changes (presumably #90986) made this pull request unmergeable. Please resolve the merge conflicts.

@rust-log-analyzer

This comment has been minimized.

@tmandry
Copy link
Member Author

tmandry commented Jan 19, 2022

@bors r=estebank

@bors
Copy link
Contributor

bors commented Jan 19, 2022

📌 Commit 698631e has been approved by estebank

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 19, 2022
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 20, 2022
…askrgr

Rollup of 13 pull requests

Successful merges:

 - rust-lang#89747 (Add MaybeUninit::(slice_)as_bytes(_mut))
 - rust-lang#89764 (Fix variant index / discriminant confusion in uninhabited enum branching)
 - rust-lang#91606 (Stabilize `-Z print-link-args` as `--print link-args`)
 - rust-lang#91694 (rustdoc: decouple stability and const-stability)
 - rust-lang#92183 (Point at correct argument when async fn output type lifetime disagrees with signature)
 - rust-lang#92582 (improve `_` constants in item signature handling)
 - rust-lang#92680 (intra-doc: Use the impl's assoc item where possible)
 - rust-lang#92704 (Change lint message to be stronger for &T -> &mut T transmute)
 - rust-lang#92861 (Rustdoc mobile: put out-of-band info on its own line)
 - rust-lang#92992 (Help optimize out backtraces when disabled)
 - rust-lang#93038 (Fix star handling in block doc comments)
 - rust-lang#93108 (:arrow_up: rust-analyzer)
 - rust-lang#93112 (Fix CVE-2022-21658)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 413f490 into rust-lang:master Jan 20, 2022
@rustbot rustbot added this to the 1.60.0 milestone Jan 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants