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.

Sign up for GitHub

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

Jump to bottom

Improve error message for lifetime error with dyn Trait #67378

Closed
wants to merge 1 commit into from
Closed

Improve error message for lifetime error with dyn Trait #67378

wants to merge 1 commit into from

Conversation

ohadravid
Copy link
Contributor

@ohadravid ohadravid commented Dec 17, 2019 •
edited
Loading

This is an attempt to improve the errors in cases like #54779.
Because SomeType<dyn OtherTrait> and impl SomeTrait for dyn OtherTrait are implicitly dyn OtherTrait+ 'static, it can be quite confusing as to why things don't work. (I originally stumbled upon this because of this question - playground link).

This PR tries to handle those two cases, by:

  1. Pointing at the requiring implementation.
  2. Suggesting the dyn Trait + '_, like impl Trait does.

There seems to be a lot of places to handle these errors, and I can't really say I fully understand the sub/sup/origin region stuff, but I got it to mostly work and wanted to get some feedback on what should I do next (and if! I'm not sure if this is the best idea/approach).
Also there are almost no existing examples in the test suite which seems to cover this usecase.

Most noticeably, I think that if you already specify + 'static or some other lifetime, the suggestion will be both useless and incorrect, and if you have Box<&dyn Trait> you'll get Box<&dyn Trait + '_> which isn't really correct (should be &(dyn Trait + '_)).

r? @estebank

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 17, 2019
estebank
estebank reviewed Dec 18, 2019
View reviewed changes
src/librustc/infer/error_reporting/mod.rs
"you can add an explicit constraint to the implementation so that it applies \
to types with less than `'static` lifetime",
),
snippet.replace(dyn_trait_name, &format!("{} + '_", &dyn_trait_name)),
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't have time to do an in-depth review right now, but we should try to come up with a different way of doing this. Fiddling with the textual representation of the code is a recipe for pain later on. It produces output that isn't always correct (like the problem with the missing parens and some incorrect suggestions I noticed below), and is brittle and affected by whitespace. I'll take another look later to see if I can have more actionable feedback on what to do instead.

@bors
Copy link
Contributor

bors commented Dec 22, 2019

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

@Dylan-DPC-zz Dylan-DPC-zz 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 Dec 23, 2019
@joelpalmer
Copy link

Ping from Triage: Any updates @ohadravid or @estebank?

@ohadravid
Copy link
Contributor Author

@joelpalmer I'll close this and try something more minimal in a different PR

@ohadravid ohadravid closed this Jan 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@estebank estebank estebank left review comments

Assignees

@estebank estebank

Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@ohadravid @bors @joelpalmer @estebank @rust-highfive @Dylan-DPC-zz