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

support elision in impl headers #49251

Merged
merged 8 commits into from
Mar 24, 2018

Conversation

nikomatsakis
Copy link
Contributor

@nikomatsakis nikomatsakis commented Mar 21, 2018

You can now do things like:

impl MyTrait<'_> for &u32 { ... }

Each '_ or elided lifetime is a fresh parameter. '_ and elision are still not permitted in associated type values. (Plausibly we could support that if there is a single input lifetime.) The original lifetime elision RFC was a bit unclear on this point: as documented here, I think this is the correct interpretation, both because it fits existing impls and it's most analogous to the behavior in fns.

We do not support elision with deprecated forms:

impl MyTrait for std::cell::Ref<u32> { } // ERROR

Builds on the in-band lifetime stuff.

r? @cramertj

Fixes #15872

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 21, 2018
@nikomatsakis
Copy link
Contributor Author

Oh, still running tests locally.

@nikomatsakis nikomatsakis force-pushed the issue-15872-elision-impl-header branch 4 times, most recently from 1873a9f to b149319 Compare March 22, 2018 00:04
@nikomatsakis
Copy link
Contributor Author

Looks good!

@bors
Copy link
Contributor

bors commented Mar 22, 2018

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

@nikomatsakis nikomatsakis force-pushed the issue-15872-elision-impl-header branch from b149319 to 94468da Compare March 22, 2018 20:55
lt_defs: &[LifetimeDef],
f: F
) -> T where F: FnOnce(&mut LoweringContext) -> T
lt_defs: impl Iterator<Item = &'l LifetimeDef>,
Copy link
Member

Choose a reason for hiding this comment

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

Huh-- this is interesting. Seems like this would probably trigger #44752, but it's an error to omit the lifetime. We should probably be able to make argument-position impl Trait elided lifetimes "just work", but it's backcompat to fix, so I'm not super concerned about it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We should probably be able to make argument-position impl Trait elided lifetimes "just work", but it's backcompat to fix, so I'm not super concerned about it.

Yeah, that was curious, but I came to the same conclusion. I think clearly they should work as a fresh lifetime (but I feel the same about '_ in where clauses).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should probably file an issue about it.

Copy link
Member

Choose a reason for hiding this comment

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

Filed #49287

@cramertj
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Mar 22, 2018

📌 Commit 94468da has been approved by cramertj

@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 Mar 22, 2018
@kennytm
Copy link
Member

kennytm commented Mar 24, 2018

@bors p=1

@bors
Copy link
Contributor

bors commented Mar 24, 2018

⌛ Testing commit 94468da with merge b4aa80d...

bors added a commit that referenced this pull request Mar 24, 2018
…r=cramertj

support elision in impl headers

You can now do things like:

```
impl MyTrait<'_> for &u32 { ... }
```

Each `'_` or elided lifetime is a fresh parameter. `'_` and elision are still not permitted in associated type values. (Plausibly we could support that if there is a single input lifetime.) The original lifetime elision RFC was a bit unclear on this point: [as documented here, I think this is the correct interpretation, both because it fits existing impls and it's most analogous to the behavior in fns](#15872 (comment)).

We do not support elision with deprecated forms:

```
impl MyTrait for std::cell::Ref<u32> { } // ERROR
```

Builds on the in-band lifetime stuff.

r? @cramertj

Fixes #15872
@bors
Copy link
Contributor

bors commented Mar 24, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: cramertj
Pushing b4aa80d to master...

@bors bors merged commit 94468da into rust-lang:master Mar 24, 2018
This was referenced Mar 24, 2018
bors added a commit that referenced this pull request Aug 6, 2018
…atsakis

Extract impl_header_lifetime_elision out of in_band_lifetimes

This way we can experiment with `impl Debug for &MyType` separately from `impl Debug for &'a MyType`.

I can't say I know what the code in here is doing, so please let me know if there's a better way 🙂

I marked this as enabled in 2018 so that edition code continues to work without another flag.

Actual feature PR #49251; Tracking Issue #15872; In-band lifetimes tracking issue #44524.

cc @aturon, per discussion on discord earlier
cc @cramertj & @nikomatsakis, who actually wrote these features
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.

None yet

5 participants