Remove implicit binder from FnSpace in VecPerParamSpace (fixes #20526)

[jseyfried]

This removes the implicit binder from FnSpace in VecPerParamSpace so that Binder<T> is the only region binder (as described in issue #20526), and refactors away enter_region_binder and exit_region_binder from TypeFolder.

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @alexcrichton (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. 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.

[alexcrichton]

r? @pnkfelix (or others on @rust-lang/compiler)

[alexcrichton]

Also, can you be sure to add a test as well? Thanks!

[jseyfried]

I didn't think a test was needed since the issue is that there is unnecessary complexity in the type folder, not that anything is incorrect. The issue is marked I-cleanup.

Removing the implicit binder around a FnSpace shouldn't change anything because the FnSpace will not contain any late-bound lifetimes bound at the implicit binder (see this comment).

[alexcrichton]

Oh sorry I should have at least read the referenced issue, carry on :)

[jseyfried]

What is the status of this pull request?
ping @pnkfelix

[jseyfried]

r? @nikomatsakis

[nikomatsakis]

I think we may be missing a test here. This branch seems good, but it feels like it was a bit too easy. Diving into the code, and re-reading my notes to myself, I suspect a missing case might be named, higher-rank lifetimes appearing in methods. I'll have to checkout the branch and play out with making a test.

[bors]

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

[nikomatsakis]

Sorry I haven't had time to dredge up an example yet of my concern :( I'll try to do that soon.

[bors]

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

[bstrie]

Thanks for keeping this up to date @jseyfried! Sorry that this review is taking so long. :( I'll ping Niko on IRC to see if he can spare a moment for this between all his MIR work.

[nikomatsakis]

Yeah, I've been putting off trying to work up that example. Always a
backlog. Still pretty sure that the required patch is a bit more
involved though.

On Mon, Mar 28, 2016 at 07:23:26PM -0700, Ben Striegel wrote:

Thanks for keeping this up to date @jseyfried! Sorry that this review is taking so long. :( I'll ping Niko on IRC to see if he can spare a moment for this between all his MIR work.

---

You are receiving this because you were assigned.
Reply to this email directly or view it on GitHub:
#29463 (comment)

[nikomatsakis]

OK, so, I finally went back to this branch. I can't quite figure out what I was worried about. I still remain a sense of unease, because I think that the "resolve_lifetimes" code assumes that there is a binder around methods -- but if it's compiling rustc, I admit it's probably working ok. I started a crater run just to be safe. If that comes back clean, we can land -- if we encounter problems, they'll show up as ICEs soon enough.

[jseyfried]

@nikomatsakis ok, sounds good.

I looked through the relevant code in more detail and I am confident that this is correct.

The resolve_lifetimes code assumes that there is a binder around the field lifetimes: HirVec<LifetimeDef> of hir::Generics when the generics are for a function or a method (like all binders at the hir level, this is implicit).

When converting these hir::Generics into ty::Generics and ty::GenericsPredicates (here and here, respectively), we only consider the early bound regions, so the hir level binder has no effect. The late bound regions are converted here only (along with the rest of the function signature), and they do end up behind an explicit binder.

[nikomatsakis]

@jseyfried it may be, but it seems odd that I went out of my way to add an extra level of binder in this case -- clearly it was necessary at some point. (And in fact I remember vaguely that there was some tricky bug that took me a while to track down, for which this was the fix.) But then again, as I said, if things are working, I guess it means we refactored that at some point.

[arielb1]

@nikomatsakis

We refactored the method handling code to be saner a while ago.

[nikomatsakis]

@bors r+

OK, I give up. Crater run was clean. :) Sorry for holding things up so long over a vague (and unsubstantiated) fear, @jseyfried.

[bors]

📌 Commit 588e0f9 has been approved by nikomatsakis

[bors]

⌛ Testing commit 588e0f9 with merge 7ded11a...

[jseyfried]

No problem!

[bors]

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-debug-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-opt-mir, auto-linux-64-opt-rustbuild, auto-linux-64-x-android-t, auto-linux-64-x-armhf, auto-linux-64-x-armsf, auto-linux-64-x-freebsd, auto-linux-64-x-netbsd, auto-linux-cross-opt, auto-linux-musl-64-opt, auto-mac-32-opt, auto-mac-64-nopt-t, auto-mac-64-opt, auto-mac-64-opt-rustbuild, auto-mac-ios-opt, auto-win-gnu-32-nopt-t, auto-win-gnu-32-opt, auto-win-gnu-32-opt-rustbuild, auto-win-gnu-64-nopt-t, auto-win-gnu-64-opt, auto-win-msvc-32-opt, auto-win-msvc-64-opt, auto-win-msvc-64-opt-mir, auto-win-msvc-64-opt-rustbuild