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

Implement impl Trait lifetime elision #46616

Merged
merged 1 commit into from
Dec 13, 2017

Conversation

cramertj
Copy link
Member

Fixes #43396.

There's one weird ICE in the interaction with argument-position impl Trait. I'm still debugging it-- I've left a test for it commented out with a FIXME.

Also included a FIXME to ensure that impl Trait traits are caught under the lint in #45992.

r? @nikomatsakis

s: self.scope
};
self.with(scope, |_old_scope, this| {
let scope = Scope::Binder { lifetimes, next_early_index, s: this.scope };
Copy link
Member Author

@cramertj cramertj Dec 10, 2017

Choose a reason for hiding this comment

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

There's a bit of duplicated code here, but I couldn't figure out a good way to factor it out-- it seems too little code for its own function, but a closure would have a signature like for<'a, 'tcx> Fn(&mut LifetimeContext<'a, 'tcx>, &'tcx Generics) and I don't believe there's a way to write that ATM (though I could be wrong)

@kennytm kennytm added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 10, 2017
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.

I'm not sure what's the ICE, will investigate.

if parameters.parenthesized && self.collect_elided_lifetimes {
self.collect_elided_lifetimes = false;
hir::intravisit::walk_path_parameters(self, span, parameters);
self.collect_elided_lifetimes = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we have to save and restore the flag; what about Fn(dyn Fn(&u32)) or something?

Copy link
Member Author

Choose a reason for hiding this comment

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

This should be equivalent to collecting and restoring it-- note that this if branch is only entered if collect_elided_lifetimes starts out as true, so the logic is: "if true, set to false, run thing then set to true, else, run thing (equivalent to set to false, run thing, set to false)". I'm happy to change it if you think it's too confusing (or if I'm mistaken and my logic is actually incorrect).

if let (&hir::Ty_::TyBareFn(_), true) = (&t.node, self.collect_elided_lifetimes) {
self.collect_elided_lifetimes = false;
hir::intravisit::walk_ty(self, t);
self.collect_elided_lifetimes = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

Same.

Copy link
Member Author

Choose a reason for hiding this comment

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

As above-- the collect_elided_lifetimes in the if check should prevent us from needing to split it out, but I think it's unnecessarily "clever", so I'll change it.

@bors
Copy link
Contributor

bors commented Dec 12, 2017

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

x: &T
) -> impl Into<&impl Fn(&u32) -> &u32> { x }

// FIXME(cramertj) Currently ICEing:
Copy link
Member Author

Choose a reason for hiding this comment

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

This ICE is #46685, and is unrelated to this PR.

@nikomatsakis
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Dec 12, 2017

📌 Commit 018c403 has been approved by nikomatsakis

@bors
Copy link
Contributor

bors commented Dec 12, 2017

⌛ Testing commit 018c403 with merge 40bd2cbc492d3a009333301e70d27870c46b910f...

@kennytm
Copy link
Member

kennytm commented Dec 12, 2017

@bors retry — Prioritizing rollup (no PRs will pass before #46694 is merged or travis-ci/travis-ci#8891 is fixed).

@kennytm
Copy link
Member

kennytm commented Dec 12, 2017

@bors rollup- 😑

@kennytm kennytm 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 Dec 12, 2017
@bors
Copy link
Contributor

bors commented Dec 13, 2017

⌛ Testing commit 018c403 with merge dcf3db4...

bors added a commit that referenced this pull request Dec 13, 2017
Implement impl Trait lifetime elision

Fixes #43396.

There's one weird ICE in the interaction with argument-position `impl Trait`. I'm still debugging it-- I've left a test for it commented out with a FIXME.

Also included a FIXME to ensure that `impl Trait` traits are caught under the lint in #45992.

r? @nikomatsakis
@bors
Copy link
Contributor

bors commented Dec 13, 2017

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

@bors bors merged commit 018c403 into rust-lang:master Dec 13, 2017
@cramertj cramertj deleted the impl-trait-elision branch December 13, 2017 18:00
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.

4 participants