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

Explain elision when it is relevant to lifetime errors #74597

Open
SNCPlay42 opened this issue Jul 21, 2020 · 0 comments
Open

Explain elision when it is relevant to lifetime errors #74597

SNCPlay42 opened this issue Jul 21, 2020 · 0 comments
Labels
A-diagnostics Area: Messages for errors, warnings, and lints A-lifetimes Area: Lifetimes / regions 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

@SNCPlay42
Copy link
Contributor

SNCPlay42 commented Jul 21, 2020

#![feature(nll)]

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

struct S;
impl S {
    fn wrong_elided_lifetime(&self, x: &i32) -> &i32 {
        x
    }
}

Gives these errors

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

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

error: aborting due to 2 previous errors

It's not made clear why the returns are expected to have lifetime '1. It would be helpful to have annotations like

3 | fn mutate_while_borrowed(x: &mut i32) -> &i32 {
  |                             -            - this elided lifetime is inferred to be `'1`
  |                             |
  |                             let's call the lifetime of this reference `'1`

Maybe link to the book section as well? For the second function, maybe adding a named lifetime (f<'a>(&self, &'a i32) -> &'a i32) could be automatically suggested, but that does change the semantics of the function to callers.

Without #![feature(nll)], the second function produces a different error, which could also use these enhancements:

error[E0623]: lifetime mismatch
  --> src/lib.rs:10:9
   |
9  |     fn wrong_elided_lifetime(&self, x: &i32) -> &i32 {
   |                                        ----     ----
   |                                        |
   |                                        this parameter and the return type are declared with different lifetimes...
10 |         x
   |         ^ ...but data from `x` is returned here

@rustbot modify labels: +C-enhancement +A-diagnostics +A-lifetimes

@rustbot claim

@rustbot rustbot added A-diagnostics Area: Messages for errors, warnings, and lints A-lifetimes Area: Lifetimes / regions C-enhancement Category: An issue proposing an enhancement or a PR with one. labels Jul 21, 2020
@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. :)
@Noratrieb Noratrieb added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Apr 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-diagnostics Area: Messages for errors, warnings, and lints A-lifetimes Area: Lifetimes / regions 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
None yet
Development

No branches or pull requests

3 participants