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

Error message in async fn refers to lifetime '1 without defining it #74072

Closed
SNCPlay42 opened this issue Jul 5, 2020 · 9 comments
Closed

Error message in async fn refers to lifetime '1 without defining it #74072

SNCPlay42 opened this issue Jul 5, 2020 · 9 comments
Labels
A-async-await Area: Async & Await A-diagnostics Area: Messages for errors, warnings, and lints A-lifetimes Area: Lifetimes / regions AsyncAwait-Triaged Async-await issues that have been triaged during a working group meeting. C-bug Category: This is a bug. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@SNCPlay42
Copy link
Contributor

SNCPlay42 commented Jul 5, 2020

pub async fn f(x: &mut i32) -> &i32 {
    let y = &*x;
    *x += 1;
    y
}

Output on 1.46.0-nightly (2020-07-04 0cd7ff7) (Playground)

   Compiling playground v0.0.1 (/playground)
error[E0506]: cannot assign to `*x` because it is borrowed
 --> src/lib.rs:3:5
  |
2 |     let y = &*x;
  |             --- borrow of `*x` occurs here
3 |     *x += 1;
  |     ^^^^^^^ assignment to borrowed `*x` occurs here
4 |     y
  |     - returning this value requires that `*x` is borrowed for `'1`
5 | }
  | - return type of generator is &'1 i32

error: aborting due to previous error

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

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

Without async, a note like this is emitted, and should also be emitted with async:

1 | pub fn f(x: &mut i32) -> &i32 {
  |             - let's call the lifetime of this reference `'1`

Output on stable 1.44.1 (with async) is different:

   Compiling playground v0.0.1 (/playground)
error[E0506]: cannot assign to `*x` because it is borrowed
 --> src/lib.rs:3:5
  |
2 |     let y = &*x;
  |             --- borrow of `*x` occurs here
3 |     *x += 1;
  |     ^^^^^^^ assignment to borrowed `*x` occurs here

error: aborting due to previous error

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

To learn more, run the command again with --verbose.
@SNCPlay42
Copy link
Contributor Author

SNCPlay42 commented Jul 6, 2020

This note

5 | }
  | - return type of generator is &'1 i32

Is the one that is supposed to define '1 (it's produced by highlight_region_name), but it's not clear that it's doing so - it appears after the mention of '1, not before, uses "generator" instead of "async function", and has a poor span.

@jonas-schievink jonas-schievink added A-async-await Area: Async & Await A-diagnostics Area: Messages for errors, warnings, and lints A-lifetimes Area: Lifetimes / regions C-bug Category: This is a bug. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jul 6, 2020
@tmandry tmandry added the AsyncAwait-Triaged Async-await issues that have been triaged during a working group meeting. label Jul 7, 2020
@tmandry
Copy link
Member

tmandry commented Jul 7, 2020

Discussed in @rust-lang/wg-async-foundations. We think rewording the message to something like "let's call the return type of the async function &'1 i32" is a good way to improve this message. But further improvements will be needed to bring it up to par with the non-async version.

@nikomatsakis
Copy link
Contributor

In particular, we would really like to cite the function parameter, not the return type, which is kind of random. But I guess I'd have to dig into the code to figure out how we can make that association.

@SNCPlay42
Copy link
Contributor Author

@rustbot claim

I've got as far as

error[E0506]: cannot assign to `*x` because it is borrowed
  --> issue-74072.rs:5:5
   |
LL | pub async fn f(x: &mut i32) -> &i32 {
   |                                ---- return type of async function is &'1 i32
LL |     let y = &*x;
   |             --- borrow of `*x` occurs here
LL |     *x += 1;
   |     ^^^^^^^ assignment to borrowed `*x` occurs here
LL |     y
   |     - returning this value requires that `*x` is borrowed for `'1`

I think I can make further improvements.

@rustbot rustbot self-assigned this Jul 21, 2020
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Jul 24, 2020
…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. :)
Manishearth added a commit to Manishearth/rust that referenced this issue Jul 24, 2020
…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. :)
@jyn514
Copy link
Member

jyn514 commented Sep 1, 2020

@SNCPlay42 are you interested in following up on #74072 (comment)? I don't see a linked PR with those changes, but even if they're not perfect it would be great to have something :)

@SNCPlay42
Copy link
Contributor Author

@jyn514 sorry for the wait! PR is up

Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Nov 9, 2020
…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)
@tmandry
Copy link
Member

tmandry commented Dec 3, 2020

@SNCPlay42 Should this be closed or is there more work to do?

For now,

@rustbot release-assignment

Feel free to claim again if you plan to do more work on this.

@SNCPlay42
Copy link
Contributor Author

It looks like #78164 will fix this completely.

There's still room for improvement around closures (they still highlight the closing brace of the block, and it would be nice to use consistent "let's call this..." language in all cases) but these might be appropriate for separate issues (they also aren't specific to async closures).

@tmandry
Copy link
Member

tmandry commented Jan 28, 2021

Triage: looks fixed (probably by #78164). Output on nightly today:

error[E0506]: cannot assign to `*x` because it is borrowed
 --> src/lib.rs:3:5
  |
1 | pub async fn f(x: &mut i32) -> &i32 {
  |                   - let's call the lifetime of this reference `'1`
2 |     let y = &*x;
  |             --- borrow of `*x` occurs here
3 |     *x += 1;
  |     ^^^^^^^ assignment to borrowed `*x` occurs here
4 |     y
  |     - returning this value requires that `*x` is borrowed for `'1`

@tmandry tmandry closed this as completed Jan 28, 2021
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: Lifetimes / regions AsyncAwait-Triaged Async-await issues that have been triaged during a working group meeting. C-bug Category: This is a bug. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
Archived in project
Development

No branches or pull requests

6 participants