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

Suggest setting lifetime in borrowck error involving types with elided lifetimes #124682

Merged
merged 6 commits into from
May 20, 2024

Conversation

estebank
Copy link
Contributor

@estebank estebank commented May 3, 2024

error: lifetime may not live long enough
  --> $DIR/ex3-both-anon-regions-both-are-structs-2.rs:7:5
   |
LL | fn foo(mut x: Ref, y: Ref) {
   |        -----       - has type `Ref<'_, '1>`
   |        |
   |        has type `Ref<'_, '2>`
LL |     x.b = y.b;
   |     ^^^^^^^^^ assignment requires that `'1` must outlive `'2`
   |
help: consider introducing a named lifetime parameter
   |
LL | fn foo<'a>(mut x: Ref<'a, 'a>, y: Ref<'a, 'a>) {
   |       ++++           ++++++++        ++++++++

As can be seen above, it currently doesn't try to compare the ty::Ty lifetimes that diverged vs the hir::Ty to correctly suggest the following

help: consider introducing a named lifetime parameter
   |
LL | fn foo<'a>(mut x: Ref<'_, 'a>, y: Ref<'_, 'a>) {
   |       ++++           ++++++++        ++++++++

but I believe this to still be an improvement over the status quo.

Fix #40990.

@rustbot
Copy link
Collaborator

rustbot commented May 3, 2024

r? @pnkfelix

rustbot has assigned @pnkfelix.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels May 3, 2024
@estebank
Copy link
Contributor Author

estebank commented May 3, 2024

The case highlighted in #40990:

error: lifetime may not live long enough
 --> f205.rs:8:16
  |
7 |     fn resolve_symbolic_reference(&self, reference: Option<Reference>) -> Option<Reference> {
  |                                   -      --------- has type `Option<Reference<'1>>`
  |                                   |
  |                                   let's call the lifetime of this reference `'2`
8 |         return reference;
  |                ^^^^^^^^^ method was supposed to return data with lifetime `'2` but it is returning data with lifetime `'1`
  |
help: consider introducing a named lifetime parameter
  |
7 |     fn resolve_symbolic_reference<'a>(&'a self, reference: Option<Reference<'a>>) -> Option<Reference> {
  |                                  ++++  ++                                  ++++

Ideally we'd tell people to write the lifetime in the return type, instead of on the self param, but this suggestion does make the code compile.


Edit: The current output for the above case is

error: lifetime may not live long enough
 --> f205.rs:8:16
  |
7 |     fn resolve_symbolic_reference(&self, reference: Option<Reference>) -> Option<Reference> {
  |                                   -      --------- has type `Option<Reference<'1>>`
  |                                   |
  |                                   let's call the lifetime of this reference `'2`
8 |         return reference;
  |                ^^^^^^^^^ method was supposed to return data with lifetime `'2` but it is returning data with lifetime `'1`
  |
help: consider introducing a named lifetime parameter
  |
7 |     fn resolve_symbolic_reference<'a>(&self, reference: Option<Reference<'a>>) -> Option<Reference<'a>> {
  |                                  ++++                                   ++++                      ++++

At the cost of not suggesting &'a self when it is necessary, but after applying the change, we do suggest the correct constraint, so I feel this is good enough to land as-is.

@@ -8,6 +8,11 @@ LL | fn foo<'a>(x: &i32, y: &'a i32) -> &'a i32 {
LL |
LL | if x > y { x } else { y }
| ^ associated function was supposed to return data with lifetime `'a` but it is returning data with lifetime `'1`
|
help: consider introducing a named lifetime parameter and update trait if needed
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm. In a case like this, the first part of the help message isn't phrased quite right.

For me, the phrase "introduce a named lifetime parameter" is talking about adding the generic binder itself, i.e. the <'a> in foo<a'>.

But in this case, what you are doing is adding a use of a pre-existing named lifetime parameter.

Would it be possible to change the diagnostic messaging here to reflect that distinction?

LL | x
| ^ method was supposed to return data with lifetime `'1` but it is returning data with lifetime `'a`
|
help: consider introducing a named lifetime parameter and update trait if needed
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as earlier message: this is another case where I view the suggestion as adding a new use of an existing lifetime paramter, rather than introducing a new one.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated

@pnkfelix
Copy link
Member

@rustbot author

@rustbot rustbot 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-review Status: Awaiting review from the assignee but also interested parties. labels May 14, 2024
…d lifetimes

```
error: lifetime may not live long enough
  --> $DIR/ex3-both-anon-regions-both-are-structs-2.rs:7:5
   |
LL | fn foo(mut x: Ref, y: Ref) {
   |        -----       - has type `Ref<'_, '1>`
   |        |
   |        has type `Ref<'_, '2>`
LL |     x.b = y.b;
   |     ^^^^^^^^^ assignment requires that `'1` must outlive `'2`
   |
help: consider introducing a named lifetime parameter
   |
LL | fn foo<'a>(mut x: Ref<'a, 'a>, y: Ref<'a, 'a>) {
   |       ++++           ++++++++        ++++++++
```

As can be seen above, it currently doesn't try to compare the `ty::Ty` lifetimes that diverged vs the `hir::Ty` to correctly suggest the following

```
help: consider introducing a named lifetime parameter
   |
LL | fn foo<'a>(mut x: Ref<'_, 'a>, y: Ref<'_, 'a>) {
   |       ++++           ++++++++        ++++++++
```

but I believe this to still be an improvement over the status quo.

CC rust-lang#40990.
```
error: lifetime may not live long enough
 --> f205.rs:8:16
  |
7 |     fn resolve_symbolic_reference(&self, reference: Option<Reference>) -> Option<Reference> {
  |                                   -      --------- has type `Option<Reference<'1>>`
  |                                   |
  |                                   let's call the lifetime of this reference `'2`
8 |         return reference;
  |                ^^^^^^^^^ method was supposed to return data with lifetime `'2` but it is returning data with lifetime `'1`
  |
help: consider introducing a named lifetime parameter
  |
7 |     fn resolve_symbolic_reference<'a>(&'a self, reference: Option<Reference<'a>>) -> Option<Reference<'a>> {
  |                                  ++++  ++                                  ++++                      ++++
```

The correct suggestion would be

```
help: consider introducing a named lifetime parameter
  |
7 |     fn resolve_symbolic_reference<'a>(&self, reference: Option<Reference<'a>>) -> Option<Reference<'a>> {
  |                                  ++++                                   ++++                      ++++
```

but we are not doing the analysis to detect that yet. If we constrain `&'a self`, then the return type with a borrow will implicitly take its lifetime from `'a`, it is better to make it explicit in the suggestion, in case that `&self` *doesn't* need to be `'a`, but the return does.
… `run-rustfix`

```
error: lifetime may not live long enough
  --> $DIR/lt-ref-self.rs:12:9
   |
LL |     fn ref_self(&self, f: &u32) -> &u32 {
   |                 -         - let's call the lifetime of this reference `'1`
   |                 |
   |                 let's call the lifetime of this reference `'2`
LL |         f
   |         ^ method was supposed to return data with lifetime `'2` but it is returning data with lifetime `'1`
   |
help: consider introducing a named lifetime parameter and update trait if needed
   |
LL |     fn ref_self<'b>(&'b self, f: &'b u32) -> &'b u32 {
   |                ++++  ++           ++          ++
```
…ceiver

Do not suggest constraining the `&self` param, but rather the return type.
If that is wrong (because it is not sufficient), a follow up error will tell the
user to fix it. This way we lower the chances of *over* constraining, but still
get the cake of "correctly" contrained in two steps.

This is a correct suggestion:

```
error: lifetime may not live long enough
  --> $DIR/ex3-both-anon-regions-return-type-is-anon.rs:9:9
   |
LL |     fn foo<'a>(&self, x: &i32) -> &i32 {
   |                -         - let's call the lifetime of this reference `'1`
   |                |
   |                let's call the lifetime of this reference `'2`
LL |         x
   |         ^ method was supposed to return data with lifetime `'2` but it is returning data with lifetime `'1`
   |
help: consider introducing a named lifetime parameter and update trait if needed
   |
LL |     fn foo<'a>(&self, x: &'a i32) -> &'a i32 {
   |                           ++          ++
```

While this is incomplete because it should suggestino `&'a self`

```
error: lifetime may not live long enough
  --> $DIR/ex3-both-anon-regions-self-is-anon.rs:7:19
   |
LL |     fn foo<'a>(&self, x: &Foo) -> &Foo {
   |                -         - let's call the lifetime of this reference `'1`
   |                |
   |                let's call the lifetime of this reference `'2`
LL |         if true { x } else { self }
   |                   ^ method was supposed to return data with lifetime `'2` but it is returning data with lifetime `'1`
   |
help: consider introducing a named lifetime parameter and update trait if needed
   |
LL |     fn foo<'a>(&self, x: &'a Foo) -> &'a Foo {
   |                           ++          ++
```

but the follow up error is

```
error: lifetime may not live long enough
 --> tests/ui/lifetimes/lifetime-errors/ex3-both-anon-regions-self-is-anon.rs:7:30
  |
6 |     fn foo<'a>(&self, x: &'a Foo) -> &'a Foo {
  |            --  - let's call the lifetime of this reference `'1`
  |            |
  |            lifetime `'a` defined here
7 |         if true { x } else { self }
  |                              ^^^^ method was supposed to return data with lifetime `'a` but it is returning data with lifetime `'1`
  |
help: consider introducing a named lifetime parameter and update trait if needed
  |
6 |     fn foo<'a>(&'a self, x: &'a Foo) -> &'a Foo {
  |                 ++
```
@estebank estebank added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 17, 2024
@pnkfelix
Copy link
Member

@bors r+ rollup

@bors
Copy link
Contributor

bors commented May 20, 2024

📌 Commit cf5702e has been approved by pnkfelix

It is now in the queue for this repository.

@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 May 20, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request May 20, 2024
…iaskrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#124682 (Suggest setting lifetime in borrowck error involving types with elided lifetimes)
 - rust-lang#124917 (Check whether the next_node is else-less if in get_return_block)
 - rust-lang#125106 (coverage: Memoize and simplify counter expressions)
 - rust-lang#125173 (Remove `Rvalue::CheckedBinaryOp`)
 - rust-lang#125305 (add some codegen tests for issue 120493)
 - rust-lang#125314 ( Add an experimental feature gate for global registration)
 - rust-lang#125318 (Migrate `run-make/rustdoc-scrape-examples-whitespace` to `rmake.rs`)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 29c603c into rust-lang:master May 20, 2024
6 checks passed
@rustbot rustbot added this to the 1.80.0 milestone May 20, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request May 20, 2024
Rollup merge of rust-lang#124682 - estebank:issue-40990, r=pnkfelix

Suggest setting lifetime in borrowck error involving types with elided lifetimes

```
error: lifetime may not live long enough
  --> $DIR/ex3-both-anon-regions-both-are-structs-2.rs:7:5
   |
LL | fn foo(mut x: Ref, y: Ref) {
   |        -----       - has type `Ref<'_, '1>`
   |        |
   |        has type `Ref<'_, '2>`
LL |     x.b = y.b;
   |     ^^^^^^^^^ assignment requires that `'1` must outlive `'2`
   |
help: consider introducing a named lifetime parameter
   |
LL | fn foo<'a>(mut x: Ref<'a, 'a>, y: Ref<'a, 'a>) {
   |       ++++           ++++++++        ++++++++
```

As can be seen above, it currently doesn't try to compare the `ty::Ty` lifetimes that diverged vs the `hir::Ty` to correctly suggest the following

```
help: consider introducing a named lifetime parameter
   |
LL | fn foo<'a>(mut x: Ref<'_, 'a>, y: Ref<'_, 'a>) {
   |       ++++           ++++++++        ++++++++
```

but I believe this to still be an improvement over the status quo.

Fix rust-lang#40990.
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.

Handle ADTs with implicit lifetime params resulting a lifetime mismatch in fn definition
4 participants