-
Notifications
You must be signed in to change notification settings - Fork 12.4k
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
Tracking issue for lifetime elision for impl headers (feature impl_header_lifetime_elision) #15872
Comments
triage: P-low |
See here for a summary of the current status. |
Should this be P-low since @brson notes it as a critical part of the implementation? |
@jonathandturner I think there's some confusion -- @brson was referring to error messages, which I believe at this point are largely as specified (but would be worth checking). This issue, OTOH, is about adding an additional point of elision. I'm nominating this for lang team discussion -- the RFC for this feature is quite old, and at this point I think we should revisit whether we even want to do it. |
We discussed in the @rust-lang/lang meeting today and decided that there is no reason to expire an RFC just because it's been a while. Basically, the motivation of the RFC still seems fairly relevant. That said, we would expect that if these changes were implemented, they would go through a stabilization period, and so if people felt like they were now bad, we could address that then. It's worth noting here for the record a few random thoughts of my own. First, I feel like elision has been wonderful except for one case: |
I might be interested in working on this. Can someone outline some steps to get going? What's the perceived complexity factor here? (I have contributed small things before, but no actual compiler work.) |
@djc You might want to start by taking a look at #39265, which stabilized I worry that It still wouldn't be that hard, as |
@nikomatsakis that would be quite welcome. I did look at lifetime_resolvers and typeck::collect a little, but a strategy to get started did not jump out at me. One thing I was wondering is if I could somehow print HIR before and after the pass that should the elision, to give me some way to debug. |
@djc the HIR is not mutated, extra information (associated by |
@nikomatsakis ping, would you still have a chance to draw up more detailed instructions? |
@djc I haven't. :( I'll do some poking about now and see how easy I think it would be. |
I have been looking into how to implicit impl elision, and I wanted to clarify one part of the RFC. The text says this (emphasis mine):
In general, though, there can be more than one type in an impl header, and the text doesn't seem to clearly address this case. (Neither do the examples.) This is how I would interpret the rules:
One question is whether to extend the "self is privileged" rule to impls. I'd be inclined to say no, even though I think it might make sense, in the absence of data, simply because it's the conservative choice. However, there is an even more conservative choice, which is to require explicit lifetimes everywhere but Therefore: impl Eq<&str> for &str // becomes:
impl<'a, 'b> Eq<&'a str> for &'b str
trait Foo<'x, T> { ... }
impl Foo<&str> for &str // error: output position `'x` has multiple possibilities
impl Foo<u8> for &str // becomes:
impl<'a> Foo<'a, u8> for &'a str
impl Foo<&str> for u8 // becomes:
impl<'a> Foo<'a, &'a str> for u8 |
Another question: in the "Elision 2.0 RFC" draft that @solson and I have been (slowly) working on, we are planning to deprecate "implicit" lifetimes (e.g., |
OK @djc, so I did some research into how to implement this. As you can see from my previous comment, there are some vaguaries in the RFC text that I think we should clear up, but it doesn't really affect the implementation very much. The good news is that the heroic @eddyb has done some nice refactoring here that I think makes this job much nicer than it once was. A quick outline of how the various bits in the compiler work that are relevant here. First off, let's take two example impls to show what I'm talking about: impl Foo for &str { .. }
impl Foo for Ref<u8> { .. } // Ref is defined as `struct Ref<'a, T>` OK, so, the general flow of the compiler is like this:
So, to make impl elision work, we are going to have to modify a little bit of each of these places. In particular, we are going to be inserted new "automatic" lifetime parameters, so those will have to appear in the OK, let's dive into a bit more detail. Let's walk through how elided lifetimes work today. In the AST, elided lifetimes are simply not present. So if you check out the enum variant for TyRptr(Lifetime, MutTy), Similarly, although you can't see this in their definition, paths like This is handy because every OK, let's take a break and walk through one of our examples now. We'll look at the first impl, the one for impl<'a> Foo for &'a str { .. } What happens to this impl as we proceed through the passes? In the AST, the type of Later on, during collect, we have to create the generics for the impl. This will read the fields from the HIR, iterate over each lifetime in there, and create a When we convert the OK, that's all I have time for for now. I haven't actually gotten into how to DO the change, but hopefully this will help you get acquanted with how the relevant code works now and maybe gives you an inkling how we will update it. |
If you treat it like argument elision in Most of the code out there shouldn't mind, i.e. it should assume it can't know what |
@nikomatsakis thanks a lot for the detailed explanation. I'll be diving in soon to see if I can make sense of it all. |
@djc I'll try to write up the next few steps soon -- but I figured I'd given you enough to chew on to start. =) |
One more point that I forgot to add. I thought that the elision rules as originally written would require you to sometimes use disjoint names just to force disjointness, which is weird. e.g. you might need to write this: impl<'a, 'b> PartialEq<&'a Foo> for &'b Foo { } to permit two references with distinct lifetimes to be used. |
To add to niko's comment: ...and we want to lint on single-use lifetimes like that. |
The final comment period, with a disposition to merge, as per the review above, is now complete. |
What's the current status here? @rfcbot says we're good to go. It seems this is already stable as part of edition 2018. What's left to do? |
@steveklabnik this needs to be stabilized by someone and then I think the reference and docs need to be updated. |
What does that stabilization effect, given that it’s already stable in 2018? For 2015?
… On Sep 20, 2018, at 2:09 PM, Mazdak Farrokhzad ***@***.***> wrote:
@steveklabnik this needs to be stabilized by someone and then I think the reference and docs need to be updated.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
@steveklabnik yes, there's no reason AFAIK that this should not be stable in 2015 because there is no breakage. |
Great, just trying to understand exactly what's up. |
@steveklabnik you and me both ;) |
I have a PR up for the issue I found with this (#54458); should be good for a stabilization PR. Edit: Started one at #54778 |
Use impl_header_lifetime_elision in libcore The feature is approved for stabilization, so let's use it to remove about 300 `'a`s. Tracking issue for the feature: rust-lang#15872
Use impl_header_lifetime_elision in libcore The feature is approved for stabilization, so let's use it to remove about 300 `'a`s. Tracking issue for the feature: rust-lang#15872
Stabilize impl_header_lifetime_elision in 2015 ~~This is currently blocked on #54902; it should be good after that~~ It's already stable in 2018; this finishes the stabilization. FCP completed (#15872 (comment)), proposal (#15872 (comment)). Tracking issue: #15872 Usage examples (from libcore): #54687
🎉 #54778 stabilized this for 1.31 🎉 |
The lifetime elision RFC included rules for eliding lifetimes in
impl
headers, but these rules were not included in the implementation.Mentoring instructions can be found here.
The text was updated successfully, but these errors were encountered: