librustc: Copy or move upvars by value.

[pcwalton]

Closes #12831.

This breaks many closures (about 10%) that look like this:

let mut sum = 0;
f.map(|x| sum += x);

Instead, rewrite them like this:

let mut sum = 0;
{
    let sum_ptr = &mut sum;
    f.map(|x| *sum_ptr += x);
}

Or, ideally, switch to RAII- or iterator-like patterns.

[breaking-change]

r? @pnkfelix

[alexcrichton]

For affecting 10% of all closures, a 6 kloc change seems a bit excessive, are we sure that the data was accurate?

[alexcrichton]

Specifically, I was under the impression this would be a pretty small change, and this is much larger than I thought it would be. It looks like each site requiring treatment is taking a step backwards in terms of readability, and having that step backwards taken across the entire codebase is unfortunate.

[lilyball]

This seems unintentional.

[lilyball]

I agree with @alexcrichton, there's a significant readability cost to all these modified closures. This suggests to me that we should rethink the idea of having syntactic sugar for auto-generating these references.

FWIW, ignoring whitespace, the diff is +2991/-2407, which is a bit better than +3864/-3280

[pnkfelix]

Just a reminder, counting the lines from the diff is going to conflate the changes that come from the compiler hacking to remove up-var-by-reference support with the changes that are fallout from that hacking.

The former is the reason for the majority of the changes to src/librustc/middle/mem_categorization.rs, for example.

It may have been nice to try to separate out those two things into two separate commits (i.e. remove up var by-reference usage first and then remove support for it in the second commit), though I admit that doing so probably would be somewhat painful to maintain.

[pcwalton]

There are something like 5,000-6,000 closures in all of rustc, so the data is accurate.

It is possible we need capture clauses. Let's not debate this here. It is clear that whatever syntax for capture clauses we may or may not come up with will be layered on top of the current unboxed closures proposal, and therefore should require a new RFC and should not prevent this from landing.

[alexcrichton]

This PR is for an RFC that has not been accepted (rust-lang/rfcs#77), and it was agreed upon based upon the data that you collected, so I just wanted to ensure that the data is accurate. If there are inconsistencies, then we're accepting that RFC under false pretenses, which seems like something we should definitely deal with now, not later. I'm not saying we should debate about capture clauses.

It looks like your data only deals with the class of upvars being capture, but it sounded like you were surprised by a few use cases when implementing this. Some upvars were copyable, but they needed to be captured by reference (due to how they were used in the closure). Does your analysis take into account these use cases, or are they accidentally outside the originally counted 10% of closures that needed to change?

One of my worries is that this new by-val capture is double-bad where you need by-ref captures. Not only do you have to create another local for the capture, but you also have to indent the entire closure by creating a new scope to kill the borrow. This is a fixable issue, but it will seriously reduce the prevalence of closures in libraries to do what would possibly otherwise be simple tasks.

I also agree with @pnkfelix that this is difficult to review because it's all jumbled together (and github is super slow at huge diffs). Could you at least separate the rustc analysis changes, and then per-library migrations? It may also be ok to have rustc analysis changes and rustc-library migrations in the same commit (just splicing this commit up by folder in that case).

[pcwalton]

If you have objections to unboxed closures be clear about that. Otherwise vague "worries" about this PR are extremely unhelpful. Even if you want capture clauses, this is a necessary step toward them and objecting to this PR is completely unproductive.

[pcwalton]

And yes, as I stated above I have no reason to doubt my data. There are just a lot of closures in rustc.

[lilyball]

@pcwalton I really want by-value closures. And I'm strongly in favor of this PR. I don't doubt your data either. My comment was just meant to say that we need to think about capture clauses sooner rather than later, but I did not intend to imply that it should block this PR.

[alexcrichton]

I concretely asked in my last comment about whether this use case was captured in your data as a closure that needed to be modified:

let mut a = 2;
let foo = || { a = 4; };

Your data looks like it classified upvars based on their type, not how they were used. This upvar has a copyable type, and it looks like your data classifies it as a copyable upvar. Your statistics would then include this as a closure which doesn't need to be modified. How does this closure get classified in your data?

I'm still not opposed to this change, I wanted to verify that your data was accurate because this was much larger than I was expecting. If there really are 5172 unmodified closures, then this seems like it's still beneficial. I was just expressing that on the surface it appears that the data you collected may be inaccurate, and was curious if you discovered anything during this migration which would cause you to revise your results.

[pcwalton]

The statistics would have considered that a mutable borrow, so it would have counted it as one of the closures that needed to change.

[pcwalton]

I apologize for snapping at you.

[alexcrichton]

Sorry if I sounded like I was bashing this, I think that all of us are very eagerly awaiting unboxed closures!

[pnkfelix]

@pcwalton If you do not object, I was thinking of breaking up your PR into separate commits segregating the separate language changes versus fallout-from-those-changes. I figure that doing so will force me to be thorough in my review. Do you object to this?

[pnkfelix]

closing in favor of the history-edited PR #14620