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

Further tweak lifetime errors involving dyn Trait and impl Trait in return position #72804

Merged
merged 12 commits into from
Jun 19, 2020

Conversation

estebank
Copy link
Contributor

  • Suggest substituting 'static lifetime in impl/dyn Trait + 'static instead of Trait + 'static + '_
  • When 'static is explicit, also suggest constraining argument with it
  • Reduce verbosity of suggestion message and mention lifetime in label
  • Tweak output for overlapping required/captured spans
  • Give these errors an error code

Follow up to #72543.

r? @nikomatsakis

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 31, 2020
@estebank
Copy link
Contributor Author

Public feedback given a small number of people in the community in replies to https://twitter.com/ekuber/status/1266565884379725824

Comment on lines 1 to 12
error[E0758]: cannot infer an appropriate lifetime
--> $DIR/issue-16922.rs:4:14
|
LL | fn foo<T: Any>(value: &T) -> Box<dyn Any> {
| -- data with this lifetime...
| -- this data with an anonymous lifetime `'_`...
LL | Box::new(value) as Box<dyn Any>
| ---------^^^^^-
| | |
| | ...and is captured here
| ...is required to be `'static` by this...
| ^^^^^ ...is captured here requiring it to live as long as `'static`
|
help: to permit non-static references in a `dyn Trait` value, you can add an explicit bound for the anonymous lifetime #1 defined on the function body at 3:1
help: to permit non-static references in a trait object value, you can add an explicit bound for an anonymous lifetime `'_`
|
LL | fn foo<T: Any>(value: &T) -> Box<dyn Any + '_> {
| ^^^^
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The suggestion here isn't entirely correct. We should be suggesting fn foo<'a, T: Any + 'a>(value: &'a T) -> Box<dyn Any + 'a> instead.

@estebank
Copy link
Contributor Author

estebank commented Jun 1, 2020

When adding documentation for the new error code, I noticed that we don't address the implicit 'static lifetime in impl Trait and dyn Trait anywhere in our documentation, like we do do for type parameters. Should I open a ticket for the book to explicitly address this and explain how to get deal with it?

@estebank estebank force-pushed the opaque-missing-lts-in-fn-2 branch from d961861 to 27cad49 Compare June 1, 2020 17:45
LL | fn elided4(x: &i32) -> Box<dyn Debug + 'static> { Box::new(x) }
| ---- ^ ...is captured here requiring it to live as long as `'static`
| |
| this data with an anonymous lifetime `'_`...
Copy link
Member

Choose a reason for hiding this comment

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

When we have sentences like this, can we make sure that the first fragment is always above the second? I think users will probably read left-to-right over top-to-bottom, but it wouldn't hurt to make it unambiguous.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is what this code would look like, with more realistic formatting:

Screen Shot 2020-06-01 at 4 17 31 PM

Copy link
Member

Choose a reason for hiding this comment

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

I see: it's not so much of a problem in general, then. Would it be difficult to alter the behaviour with regards to which labels are pushed down? If we can improve the situation even for more uncommon cases, it seems worth doing, unless it's significant work.

LL | fn foo<'a>(x: &i32) -> impl Copy + 'a { x }
| ---- ^ lifetime `'a` required
| |
| help: add explicit lifetime `'a` to the type of `x`: `&'a i32`
Copy link
Member

Choose a reason for hiding this comment

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

I think something as simple as:

Suggested change
| help: add explicit lifetime `'a` to the type of `x`: `&'a i32`
| help: add an explicit lifetime: `&'a i32`

would do; it's easier to read.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The explicit lifetime already exists, so saying an would imply the creation of a new lifetime (that's how it'd read to me). Removing the lifetime name and the description of x's type removes some jargon, but I think there's a case to be made about this being a good moment to teach that jargon.

Copy link
Member

Choose a reason for hiding this comment

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

That sounds reasonable. What do you think about altering slightly to help: add the explicit lifetime […]? That reads slightly better to me, but is a little longer.

@varkor
Copy link
Member

varkor commented Jun 1, 2020

As always, these diagnostics improvements look great! I just have a few nit-picky comments :)

Copy link
Contributor

@nikomatsakis nikomatsakis left a comment

Choose a reason for hiding this comment

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

Initial comments based on reading the diffs -- I do think this PR is better than status quo, so feel free to push back on me some :)

|
help: to permit non-static references in an `impl Trait` value, you can add an explicit bound for the anonymous lifetime #1 defined on the method body at 6:5
help: to permit non-static references in an `impl Trait` value, you can add an explicit bound for an anonymous lifetime `'_`
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like it'd be good if we could take this opportunity to explain lifetime elision a bit better. How hard would it be to say

help: to declare that you capture borrowed data from self, you can add an explicit '_ bound

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Current output:

Screen Shot 2020-06-02 at 4 06 58 PM

LL | Box::new(move || { *x })
| ^^^^^^^^^^^^^^ ...is captured here requiring it to live as long as `'static`
|
help: consider changing the trait object's explicit `'static` bound to an anonymous lifetime `'_`
Copy link
Contributor

Choose a reason for hiding this comment

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

another example. It feels like it's not just "an anonymous lifetime", it's the lifetime from the argument x.

help: consider changing the trait object's explicit 'static bound to '_, to declare that you capture data from the argument x

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Current output:

Screen Shot 2020-06-02 at 4 06 32 PM

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice!

@estebank
Copy link
Contributor Author

estebank commented Jun 1, 2020

Please check the new wording.

@rust-highfive

This comment has been minimized.

@estebank estebank force-pushed the opaque-missing-lts-in-fn-2 branch 2 times, most recently from 20f60d1 to b9be6e1 Compare June 3, 2020 18:17
@bors

This comment has been minimized.

Copy link
Contributor

@nikomatsakis nikomatsakis left a comment

Choose a reason for hiding this comment

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

r=me post rebase

|
help: to permit non-static references in a `dyn Trait` value, you can add an explicit bound for the anonymous lifetime #1 defined on the function body at 17:1
help: to declare that the trait object captures data from argument `v`, you can add an explicit `'_` lifetime bound
Copy link
Contributor

Choose a reason for hiding this comment

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

this is really good

@estebank estebank force-pushed the opaque-missing-lts-in-fn-2 branch from fb79b6d to 56b1ca4 Compare June 5, 2020 21:35
@estebank
Copy link
Contributor Author

estebank commented Jun 5, 2020

@bors r=nikomatsakis

@bors
Copy link
Contributor

bors commented Jun 5, 2020

📌 Commit 56b1ca4f60df0ead090759b60d14200f731a08a1 has been approved by nikomatsakis

@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 Jun 5, 2020
@bors
Copy link
Contributor

bors commented Jun 9, 2020

☔ The latest upstream changes (presumably #73147) made this pull request unmergeable. Please resolve the merge conflicts.

@bors bors removed the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Jun 9, 2020
@bors
Copy link
Contributor

bors commented Jun 15, 2020

📌 Commit bfe1434 has been approved by nikomatsakis

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 15, 2020
Manishearth added a commit to Manishearth/rust that referenced this pull request Jun 16, 2020
…, r=nikomatsakis

Further tweak lifetime errors involving `dyn Trait` and `impl Trait` in return position

* Suggest substituting `'static` lifetime in impl/dyn `Trait + 'static` instead of `Trait + 'static + '_`
* When `'static` is explicit, also suggest constraining argument with it
* Reduce verbosity of suggestion message and mention lifetime in label
* Tweak output for overlapping required/captured spans
* Give these errors an error code

Follow up to rust-lang#72543.

r? @nikomatsakis
Manishearth added a commit to Manishearth/rust that referenced this pull request Jun 16, 2020
…, r=nikomatsakis

Further tweak lifetime errors involving `dyn Trait` and `impl Trait` in return position

* Suggest substituting `'static` lifetime in impl/dyn `Trait + 'static` instead of `Trait + 'static + '_`
* When `'static` is explicit, also suggest constraining argument with it
* Reduce verbosity of suggestion message and mention lifetime in label
* Tweak output for overlapping required/captured spans
* Give these errors an error code

Follow up to rust-lang#72543.

r? @nikomatsakis
Manishearth added a commit to Manishearth/rust that referenced this pull request Jun 16, 2020
…, r=nikomatsakis

Further tweak lifetime errors involving `dyn Trait` and `impl Trait` in return position

* Suggest substituting `'static` lifetime in impl/dyn `Trait + 'static` instead of `Trait + 'static + '_`
* When `'static` is explicit, also suggest constraining argument with it
* Reduce verbosity of suggestion message and mention lifetime in label
* Tweak output for overlapping required/captured spans
* Give these errors an error code

Follow up to rust-lang#72543.

r? @nikomatsakis
Manishearth added a commit to Manishearth/rust that referenced this pull request Jun 16, 2020
…, r=nikomatsakis

Further tweak lifetime errors involving `dyn Trait` and `impl Trait` in return position

* Suggest substituting `'static` lifetime in impl/dyn `Trait + 'static` instead of `Trait + 'static + '_`
* When `'static` is explicit, also suggest constraining argument with it
* Reduce verbosity of suggestion message and mention lifetime in label
* Tweak output for overlapping required/captured spans
* Give these errors an error code

Follow up to rust-lang#72543.

r? @nikomatsakis
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Jun 17, 2020
…, r=nikomatsakis

Further tweak lifetime errors involving `dyn Trait` and `impl Trait` in return position

* Suggest substituting `'static` lifetime in impl/dyn `Trait + 'static` instead of `Trait + 'static + '_`
* When `'static` is explicit, also suggest constraining argument with it
* Reduce verbosity of suggestion message and mention lifetime in label
* Tweak output for overlapping required/captured spans
* Give these errors an error code

Follow up to rust-lang#72543.

r? @nikomatsakis
tmandry added a commit to tmandry/rust that referenced this pull request Jun 17, 2020
…, r=nikomatsakis

Further tweak lifetime errors involving `dyn Trait` and `impl Trait` in return position

* Suggest substituting `'static` lifetime in impl/dyn `Trait + 'static` instead of `Trait + 'static + '_`
* When `'static` is explicit, also suggest constraining argument with it
* Reduce verbosity of suggestion message and mention lifetime in label
* Tweak output for overlapping required/captured spans
* Give these errors an error code

Follow up to rust-lang#72543.

r? @nikomatsakis
Manishearth added a commit to Manishearth/rust that referenced this pull request Jun 18, 2020
…, r=nikomatsakis

Further tweak lifetime errors involving `dyn Trait` and `impl Trait` in return position

* Suggest substituting `'static` lifetime in impl/dyn `Trait + 'static` instead of `Trait + 'static + '_`
* When `'static` is explicit, also suggest constraining argument with it
* Reduce verbosity of suggestion message and mention lifetime in label
* Tweak output for overlapping required/captured spans
* Give these errors an error code

Follow up to rust-lang#72543.

r? @nikomatsakis
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 19, 2020
…arth

Rollup of 17 pull requests

Successful merges:

 - rust-lang#70551 (Make all uses of ty::Error delay a span bug)
 - rust-lang#71338 (Expand "recursive opaque type" diagnostic)
 - rust-lang#71976 (Improve diagnostics for `let x += 1`)
 - rust-lang#72279 (add raw_ref macros)
 - rust-lang#72628 (Add tests for 'impl Default for [T; N]')
 - rust-lang#72804 (Further tweak lifetime errors involving `dyn Trait` and `impl Trait` in return position)
 - rust-lang#72814 (remove visit_terminator_kind from MIR visitor)
 - rust-lang#72836 (Complete the std::time documentation to warn about the inconsistencies between OS)
 - rust-lang#72968 (Only highlight doc search results via mouseover if mouse has moved)
 - rust-lang#73034 (Export `#[inline]` fns with extern indicators)
 - rust-lang#73315 (Clean up some weird command strings)
 - rust-lang#73320 (Make new type param suggestion more targetted)
 - rust-lang#73361 (Tweak "non-primitive cast" error)
 - rust-lang#73425 (Mention functions pointers in the documentation)
 - rust-lang#73428 (Fix typo in librustc_ast docs)
 - rust-lang#73447 (Improve document for `Result::as_deref(_mut)` methods)
 - rust-lang#73476 (Added tooltip for should_panic code examples)

Failed merges:

r? @ghost
@bors bors merged commit 40fd2bd into rust-lang:master Jun 19, 2020
Manishearth added a commit to Manishearth/rust that referenced this pull request Jun 23, 2020
…, r=nikomatsakis

Account for multiple impl/dyn Trait in return type when suggesting `'_`

Make `impl` and `dyn` Trait lifetime suggestions a bit more resilient.

Follow up to rust-lang#72804.

r? @nikomatsakis
@estebank estebank deleted the opaque-missing-lts-in-fn-2 branch November 9, 2023 05:16
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants