-
Notifications
You must be signed in to change notification settings - Fork 13k
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
Conversation
s: self.scope | ||
}; | ||
self.with(scope, |_old_scope, this| { | ||
let scope = Scope::Binder { lifetimes, next_early_index, s: this.scope }; |
There was a problem hiding this comment.
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)
There was a problem hiding this 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.
src/librustc/hir/lowering.rs
Outdated
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; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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).
src/librustc/hir/lowering.rs
Outdated
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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same.
There was a problem hiding this comment.
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.
54cab78
to
6c3cab9
Compare
☔ The latest upstream changes (presumably #46657) made this pull request unmergeable. Please resolve the merge conflicts. |
6c3cab9
to
be7270f
Compare
x: &T | ||
) -> impl Into<&impl Fn(&u32) -> &u32> { x } | ||
|
||
// FIXME(cramertj) Currently ICEing: |
There was a problem hiding this comment.
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.
be7270f
to
018c403
Compare
@bors r+ |
📌 Commit 018c403 has been approved by |
⌛ Testing commit 018c403 with merge 40bd2cbc492d3a009333301e70d27870c46b910f... |
@bors retry — Prioritizing rollup (no PRs will pass before #46694 is merged or travis-ci/travis-ci#8891 is fixed). |
@bors rollup- 😑 |
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
☀️ Test successful - status-appveyor, status-travis |
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