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

Unhelpful error messages from borrow checker with async/await #67765

Closed
Kestrer opened this issue Dec 31, 2019 · 2 comments · Fixed by #73806
Closed

Unhelpful error messages from borrow checker with async/await #67765

Kestrer opened this issue Dec 31, 2019 · 2 comments · Fixed by #73806
Assignees
Labels
A-async-await Area: Async & Await A-diagnostics Area: Messages for errors, warnings, and lints 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-enhancement Category: An issue proposing an enhancement or a PR with one. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@Kestrer
Copy link
Contributor

Kestrer commented Dec 31, 2019

This code:

fn main() {}

async fn func<'a>() -> Result<(), &'a str> {
    let s = String::new();

    let b = &s[..];

    Err(b)?;

    Ok(())
}

Returns this error:

error[E0597]: `s` does not live long enough
  --> src/main.rs:6:14
   |
6  |     let b = &s[..];
   |              ^ borrowed value does not live long enough
...
11 | }
   | - `s` dropped here while still borrowed

Which is very unhelpful; it should at least in some way point to the line with the ?. This happens on rustc 1.40.0 (73528e3 2019-12-16).

For reference, this is the equivalent non-async error message (much more helpful):

error[E0515]: cannot return value referencing local variable `s`
 --> src/main.rs:8:11
  |
6 |     let b = &s[..];
  |              - `s` is borrowed here
7 |
8 |     Err(b)?;
  |           ^ returns a value referencing data owned by the current function
@Centril Centril added A-diagnostics Area: Messages for errors, warnings, and lints A-async-await Area: Async & Await T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Dec 31, 2019
@jonas-schievink jonas-schievink added the C-enhancement Category: An issue proposing an enhancement or a PR with one. label Jan 1, 2020
@Aaron1011
Copy link
Member

@rustbot claim

Aaron1011 added a commit to Aaron1011/rust that referenced this issue Jan 6, 2020
Fixes rust-lang#67765

When we process closures/generators, we create a new NLL inference variable
each time we encounter an early-bound region (e.g. "'a") in the substs
of the closure. These region variables are then treated as
universal regions when the perform region inference for the closure.

However, we may encounter the same region multiple times, such
as when the closure references multiple upvars that are bound
by the same early-bound lifetime. For example:

`fn foo<'a>(x: &'a u8, y: &'a u8) -> u8 { (|| *x + *y)() }`

This results in the creation of multiple 'duplicate' region variables,
which all correspond to the same early-bound region. During
type-checking of the closure, we don't really care - any constraints
involving these regions will get propagated back up to the enclosing
function, which is then responsible for checking said constraints
using the 'real' regions.

Unfortunately, this presents a problem for diagnostic code, which may
run in the context of the closure. In order to display a good error
message, we need to map arbitrary region inference variables (which may
not correspond to anything meaningful to the user) into a 'nicer' region
variable that can be displayed to the user (e.g. a universally bound
region, written by the user). To accomplish this, we repeatedly
compute an 'upper bound' of the region variable, stopping once
we hit a universally bound region, or are unable to make progress.

During the processing of a closure, we may determine that a region
variable needs to outlive mutliple universal regions. In a closure
context, some of these universal regions may actually be 'the same'
region - that is, they correspond to the same early-bound region.
If this is the case, we will end up trying to compute an upper bound
using these regions variables, which will fail (we don't know about
any relationship between them).

However, we don't actually need to find an upper bound involving these
duplicate regions - since they're all actually "the same" region, we can
just pick an arbirary region variable from a given "duplicate set" (all
region variables that correspond to a given early-bound region).

By doing so, we can generate a more precise diagnostic, since we will be
able to print a message involving a particular early-bound region (and
the variables using it), instead of falling back to a more generic error
message.
@tmandry tmandry added AsyncAwait-Triaged Async-await issues that have been triaged during a working group meeting. AsyncAwait-Polish Async-await issues that are part of the "polish" area labels Jan 7, 2020
@tmandry
Copy link
Member

tmandry commented Jan 7, 2020

Marking as focus, since improving diagnostics are important to the wg.

Manishearth added a commit to Manishearth/rust that referenced this issue Jun 29, 2020
…upper, r=estebank

Use an 'approximate' universal upper bound when reporting region errors

Fixes rust-lang#67765

When reporting errors during MIR region inference, we sometimes use
`universal_upper_bound` to obtain a named universal region that we
can display to the user. However, this is not always possible - in a
case like `fn foo<'a, 'b>() { .. }`, the only upper bound for a region
containing `'a` and `'b` is `'static`. When displaying diagnostics, it's
usually better to display *some* named region (even if there are
multiple involved) rather than fall back to a generic error involving
`'static`.

This commit adds a new `approx_universal_upper_bound` method, which
uses the lowest-numbered universal region if the only alternative is to
return `'static`.
Manishearth added a commit to Manishearth/rust that referenced this issue Jun 29, 2020
…upper, r=estebank

Use an 'approximate' universal upper bound when reporting region errors

Fixes rust-lang#67765

When reporting errors during MIR region inference, we sometimes use
`universal_upper_bound` to obtain a named universal region that we
can display to the user. However, this is not always possible - in a
case like `fn foo<'a, 'b>() { .. }`, the only upper bound for a region
containing `'a` and `'b` is `'static`. When displaying diagnostics, it's
usually better to display *some* named region (even if there are
multiple involved) rather than fall back to a generic error involving
`'static`.

This commit adds a new `approx_universal_upper_bound` method, which
uses the lowest-numbered universal region if the only alternative is to
return `'static`.
Manishearth added a commit to Manishearth/rust that referenced this issue Jun 30, 2020
…upper, r=estebank

Use an 'approximate' universal upper bound when reporting region errors

Fixes rust-lang#67765

When reporting errors during MIR region inference, we sometimes use
`universal_upper_bound` to obtain a named universal region that we
can display to the user. However, this is not always possible - in a
case like `fn foo<'a, 'b>() { .. }`, the only upper bound for a region
containing `'a` and `'b` is `'static`. When displaying diagnostics, it's
usually better to display *some* named region (even if there are
multiple involved) rather than fall back to a generic error involving
`'static`.

This commit adds a new `approx_universal_upper_bound` method, which
uses the lowest-numbered universal region if the only alternative is to
return `'static`.
Manishearth added a commit to Manishearth/rust that referenced this issue Jul 1, 2020
…upper, r=estebank

Use an 'approximate' universal upper bound when reporting region errors

Fixes rust-lang#67765

When reporting errors during MIR region inference, we sometimes use
`universal_upper_bound` to obtain a named universal region that we
can display to the user. However, this is not always possible - in a
case like `fn foo<'a, 'b>() { .. }`, the only upper bound for a region
containing `'a` and `'b` is `'static`. When displaying diagnostics, it's
usually better to display *some* named region (even if there are
multiple involved) rather than fall back to a generic error involving
`'static`.

This commit adds a new `approx_universal_upper_bound` method, which
uses the lowest-numbered universal region if the only alternative is to
return `'static`.
@bors bors closed this as completed in 517d361 Jul 1, 2020
indirect pushed a commit to indirect/rust that referenced this issue Jul 4, 2020
Fixes rust-lang#67765

When reporting errors during MIR region inference, we sometimes use
`universal_upper_bound` to obtain a named universal region that we
can display to the user. However, this is not always possible - in a
case like `fn foo<'a, 'b>() { .. }`, the only upper bound for a region
containing `'a` and `'b` is `'static`. When displaying diagnostics, it's
usually better to display *some* named region (even if there are
multiple involved) rather than fall back to a generic error involving
`'static`.

This commit adds a new `approx_universal_upper_bound` method, which
uses the lowest-numbered universal region if the only alternative is to
return `'static`.
@tmandry tmandry moved this to Done in wg-async work Dec 8, 2022
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 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-enhancement Category: An issue proposing an enhancement or a PR with one. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
Archived in project
5 participants