Skip to content

wip: Better error when violating for<'a> T: 'a #40413

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

Closed
wants to merge 1 commit into from
Closed

wip: Better error when violating for<'a> T: 'a #40413

wants to merge 1 commit into from

Conversation

tbg
Copy link
Contributor

@tbg tbg commented Mar 10, 2017

Rust currently handles certain higher-ranked trait bound such as where for<'a> T: 'a
through a 'static region, which gives an unintuitive error message in the case in which
the lifetime bound does not hold (see #27114).

This is a work in progress that gives a better result but not with code worth
checking in yet. Please see the comments starting with Q: in the diff.

This is the new message:

error[E0592]: the type `&'b i32` does not fulfill the required lifetime imposed
by a higher-ranked trait bound
 --> /Users/tschottdorf/rust/i27114/main.rs:6:5
  |
6 |     foo::<&'b i32>();
  |     ^^^^^^^^^^^^^^
  |
  = note: type must outlive the static lifetime
note: which is implied by
 --> /Users/tschottdorf/rust/i27114/main.rs:3:23
  |
3 | fn foo<T>() where for<'a> T: 'a {}
  |                       ^^

error: aborting due to previous error

Fixes the simple case in #27114, though without attempting at all to handle
more complicated HRTBs (which is also discussed in said issue).

cc @eddyb

@rust-highfive
Copy link
Contributor

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @pnkfelix (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@eddyb
Copy link
Member

eddyb commented Mar 10, 2017

r? @nikomatsakis

@tbg
Copy link
Contributor Author

tbg commented Mar 11, 2017

CI passed but for one run which hit a github timeout.

@eddyb
Copy link
Member

eddyb commented Mar 11, 2017

Only one should run but OSX fails to start (only to stop soon after) sometimes. I restarted it, should be green in a minute now.

This is a work in progress that gives the right result but not with code worth
checking in yet. Please see the comments starting with `Q:` in the diff.

```
error[E0592]: the type `&'b i32` does not fulfill the required lifetime imposed
by a higher-ranked trait bound
 --> /Users/tschottdorf/rust/i27114/main.rs:6:5
  |
6 |     foo::<&'b i32>();
  |     ^^^^^^^^^^^^^^
  |
  = note: type must outlive the static lifetime
note: which is implied by
 --> /Users/tschottdorf/rust/i27114/main.rs:3:23
  |
3 | fn foo<T>() where for<'a> T: 'a {}
  |                       ^^

error: aborting due to previous error
```

Fixes the simple case in #27114, though without attempting at all to handle
more complicated HRTBs (which is also discussed in said issue).

cc @eddyb
@nikomatsakis
Copy link
Contributor

@tschottdorf argh sorry for the delay. Not sure how I have failed to review this for so long! Will get on it.

@nikomatsakis
Copy link
Contributor

OK, sorry for the delay. So I've read the code. I agree it's kind of a messy patch, but the system it's touching is also messy. I am somewhat reluctant to make things messier given that I expect we're going to be making a lot of changes in all of this logic (in particular in the logic around higher-ranked things as well as how we propagate region bounds).

On a related note, would you be interested in working with me on some of those refactorings?

It seems like if we were going to continue with this direction, then yes I would prefer to substitute a new ObligationCause at the point where we "rewrite" a higher-ranked rule to be a restriction against 'static, rather than including the Option<Span> that kind of silently indicates that a HR clause is in use.

@tbg
Copy link
Contributor Author

tbg commented Apr 4, 2017

@nikomatsakis: I'd much prefer helping you with the refactorings over adding on top of the pile of things that will need to be touched by those refactors. I should have some time on my hands I can dedicate. Let me know what I should look at/where to go from here.
Just a heads up - I'm travelling on generally poor WiFi, so I can only communicate async for the moment.

@nikomatsakis
Copy link
Contributor

@tschottdorf very good -- I am also traveling but I would love to sync up with you, I will try to draw up some plans.

Shall I close this PR in the meantime?

@tbg
Copy link
Contributor Author

tbg commented Apr 6, 2017

@nikomatsakis sounds good! I'll do the honors of closing. Do me a favor and email me when you're ready to go - there's a chance I would miss a GitHub mention.

@tbg tbg closed this Apr 6, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants