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

Lifetime error points to wrong reference in async fn return #75785

Closed
SNCPlay42 opened this issue Aug 21, 2020 · 4 comments · Fixed by #78164
Closed

Lifetime error points to wrong reference in async fn return #75785

SNCPlay42 opened this issue Aug 21, 2020 · 4 comments · Fixed by #78164
Assignees
Labels
A-async-await Area: Async & Await A-diagnostics Area: Messages for errors, warnings, and lints A-lifetimes Area: lifetime related AsyncAwait-Triaged Async-await issues that have been triaged during a working group meeting. C-bug Category: This is a bug. D-incorrect A diagnostic that is giving misleading or incorrect information T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@SNCPlay42
Copy link
Contributor

SNCPlay42 commented Aug 21, 2020

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

Output (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 |     (&32, y)
  |     -------- returning this value requires that `*x` is borrowed for `'1`
5 | }
  | - return type of generator is (&'1 i32, &i32)

The '1 lifetime name should be given to the second reference in the tuple.

@rustbot modify labels: C-bug A-async-await A-diagnostics A-lifetimes T-compiler

@rustbot rustbot added A-async-await Area: Async & Await A-diagnostics Area: Messages for errors, warnings, and lints A-lifetimes Area: lifetime related C-bug Category: This is a bug. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Aug 21, 2020
@SNCPlay42
Copy link
Contributor Author

SNCPlay42 commented Aug 21, 2020

On stable the output is different:

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

Per bisect-rustc, the commit that changed the output is f781bab - almost definitely #73806. @Aaron1011 is there something that can be done to improve this case?

Bisect output

searched nightlies: from nightly-2020-06-01 to nightly-2020-07-15
regressed nightly: nightly-2020-07-02
searched commits: from 16957bd to f781bab
regressed commit: f781bab

bisected with cargo-bisect-rustc v0.5.2

Host triple: x86_64-unknown-linux-gnu
Reproduce with:

cargo bisect-rustc --regress success --script ./test.sh --start 2020-06-01 --end 2020-07-15 --preserve 

@Aaron1011
Copy link
Member

Aaron1011 commented Aug 21, 2020

Here's the output with async removed:

pub fn async_fn(x: &mut i32) -> (&i32, &i32) {
    let y = &*x;
    *x += 1;
    (&32, y)
}
error[E0506]: cannot assign to `*x` because it is borrowed
 --> src/lib.rs:3:5
  |
1 | pub fn async_fn(x: &mut i32) -> (&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 |     (&32, y)
  |     -------- returning this value requires that `*x` is borrowed for `'1`

It looks like we're missing the "let's call the lifetime of this reference" note in the async case.

@SNCPlay42
Copy link
Contributor Author

SNCPlay42 commented Aug 21, 2020

It looks like we're missing the "let's call the lifetime of this reference" note in the async case.

The current equivalent of that annotation is the note at the bottom: (see #74072)

- return type of generator is (&'1 i32, &i32)

I actually ran into this while testing a potential fix for that issue - it would make the output

LL | pub async fn async_fn(x: &mut i32) -> (&i32, &i32) {
   |                                        - let's call the lifetime of this reference `'1`
LL |     let y = &*x;
   |             --- borrow of `*x` occurs here
LL |     *x += 1;
   |     ^^^^^^^ assignment to borrowed `*x` occurs here
LL |     (&32, y)
   |     -------- returning this value requires that `*x` is borrowed for `'1`

Which still points at the wrong reference.

@Aaron1011 Aaron1011 self-assigned this Aug 21, 2020
@tmandry tmandry added the D-incorrect A diagnostic that is giving misleading or incorrect information label Aug 25, 2020
@tmandry tmandry added this to On deck in wg-async work via automation Aug 25, 2020
@tmandry tmandry moved this from On deck to Claimed in wg-async work Aug 25, 2020
@tmandry tmandry added the AsyncAwait-Triaged Async-await issues that have been triaged during a working group meeting. label Sep 1, 2020
@tmandry
Copy link
Contributor

tmandry commented Oct 20, 2020

@Aaron1011 Are you still planning to work on this?

Aaron1011 added a commit to Aaron1011/rust that referenced this issue Oct 20, 2020
Fixes rust-lang#75785

When displaying a MIR borrowcheck error, we may need to find an upper
bound for a region, which gives us a region to point to in the error
message. However, a region might outlive multiple distinct universal
regions, in which case the only upper bound is 'static

To try to display a meaningful error message, we compute an
'approximate' upper bound by picking one of the universal regions.
Currently, we pick the region with the lowest index - however, this
caused us to produce a suboptimal error message in issue rust-lang#75785

This PR `approx_universal_upper_bound` to prefer regions with an
`external_name`. This causes us to prefer regions from function
arguments/upvars, which seems to lead to a nicer error message in some
cases.
Aaron1011 added a commit to Aaron1011/rust that referenced this issue Oct 20, 2020
Fixes rust-lang#75785

When displaying a MIR borrowcheck error, we may need to find an upper
bound for a region, which gives us a region to point to in the error
message. However, a region might outlive multiple distinct universal
regions, in which case the only upper bound is 'static

To try to display a meaningful error message, we compute an
'approximate' upper bound by picking one of the universal regions.
Currently, we pick the region with the lowest index - however, this
caused us to produce a suboptimal error message in issue rust-lang#75785

This PR `approx_universal_upper_bound` to prefer regions with an
`external_name`. This causes us to prefer regions from function
arguments/upvars, which seems to lead to a nicer error message in some
cases.
@tmandry tmandry moved this from Claimed to In progress in wg-async work Dec 3, 2020
bors added a commit to rust-lang-ci/rust that referenced this issue Dec 17, 2020
…andry

Prefer regions with an `external_name` in `approx_universal_upper_bound`

Fixes rust-lang#75785

When displaying a MIR borrowcheck error, we may need to find an upper
bound for a region, which gives us a region to point to in the error
message. However, a region might outlive multiple distinct universal
regions, in which case the only upper bound is 'static

To try to display a meaningful error message, we compute an
'approximate' upper bound by picking one of the universal regions.
Currently, we pick the region with the lowest index - however, this
caused us to produce a suboptimal error message in issue rust-lang#75785

This PR `approx_universal_upper_bound` to prefer regions with an
`external_name`. This causes us to prefer regions from function
arguments/upvars, which seems to lead to a nicer error message in some
cases.
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Dec 17, 2020
…tmandry

Prefer regions with an `external_name` in `approx_universal_upper_bound`

Fixes rust-lang#75785

When displaying a MIR borrowcheck error, we may need to find an upper
bound for a region, which gives us a region to point to in the error
message. However, a region might outlive multiple distinct universal
regions, in which case the only upper bound is 'static

To try to display a meaningful error message, we compute an
'approximate' upper bound by picking one of the universal regions.
Currently, we pick the region with the lowest index - however, this
caused us to produce a suboptimal error message in issue rust-lang#75785

This PR `approx_universal_upper_bound` to prefer regions with an
`external_name`. This causes us to prefer regions from function
arguments/upvars, which seems to lead to a nicer error message in some
cases.
@bors bors closed this as completed in 419d3ae Dec 18, 2020
wg-async work automation moved this from In progress to Done Dec 18, 2020
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-Triaged Async-await issues that have been triaged during a working group meeting. C-bug Category: This is a bug. D-incorrect A diagnostic that is giving misleading or incorrect information T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
Development

Successfully merging a pull request may close this issue.

4 participants