Always make the "upvar types" a tuple of the actual upvar types (Parent issue#53488)

[blitzerr]

Updated by nikomatsakis with current status:

In rust-lang/rust#69968, @eddyb landed the "first step" here of introducing a tuple into the upvar types. However, in current rustc, we create that tuple "eagerly", as soon as we create the closure type:

https://github.com/rust-lang/rust/blob/1a87c49e33d87955e28fa92a8d59a17264ac6044/src/librustc_typeck/check/closure.rs#L90-L103

The next step is to move that tuple creation "late", so that it occurs during the "upvar analysis" code, so somewhere around here:

https://github.com/rust-lang/rust/blob/1a87c49e33d87955e28fa92a8d59a17264ac6044/src/librustc_typeck/check/upvar.rs#L194-L202

The problem with this is that the current accessor, ClosureSubsts::upvar_tys, assumes that the upvar types are a tuple with known arity. Thus any calls to upvar_tys that occur before the upvar analysis code runs will ICE.

Last time, we added an accessor upvar_tuple_ty that returned the tuple itself (or an unbound type variable), since it turned out that many of the calls to upvar_tys could be replaced by a call that operated on the tuple. Most notably, in the trait system, for example here:

https://github.com/rust-lang/rust/blob/1a87c49e33d87955e28fa92a8d59a17264ac6044/src/librustc_trait_selection/traits/select.rs#L2239-L2242

We changed this to yield the tuple always. The problem with that was that it was effecting the error messages, because the tuple was being introduced into the "backtrace" and we somehow never got things quiite right to remove it.

I still think that is perhaps the right approach, but there is an alternative we could try: we could get the tuple ty and check whether it's been "resolved" yet (via a call to self.infcx.shallow_resolve). If so, we can put each of its components as we do today, but if not, we can return Ambiguous, as we do here.

[blitzerr]

Main Issue
Last Issue in the chain

[blitzerr]

@nikomatsakis so we think that we should not even group upvars referenced in closure into tuples as in the https://github.com/rust-lang/rust/blob/1a87c49e33d87955e28fa92a8d59a17264ac6044/src/librustc_typeck/check/closure.rs#L91-L101

We will just use the similar logic directly in the analyze_closure()
https://github.com/rust-lang/rust/blob/1a87c49e33d87955e28fa92a8d59a17264ac6044/src/librustc_typeck/check/upvar.rs#L200-L202

Can you explain, as a motivation for the upcoming PR, why was this done this way and what does it buys us now ? I guess we discussed this but it escapes me now. Sorry for the repeated question. I am trying to tie the strings together as how this helps us achieve the over arching goal of just capturing the individual fields of an aggregate type (e.g struct) that are referenced in the closure and not the entire aggregate type.

[eddyb]

IIUC, not forcing the tuple shape early means the number of tuple entries can be inferred, and they can be field paths into the direct upvars (i.e. the ones computed from HIR by the upvars query).

[eddyb]

Although, hang on @nikomatsakis, the upvars in the type are what layouts are computed from.
Do you want every single field path to have its own borrow? Or is that only when there's a single field path for a given upvar?

[nikomatsakis]

@eddyb I don't remember to what extent we worked out an answer to that question. Obviously, the more "field paths" we are able to capture, the fewer errors you get from borrow checking (i.e., because we are borrowing more precise paths), but there is also a bit of overhead in that the number of capturing things go up.

At the moment, I am mostly interested in laying the groundwork that allows the number of upvar paths to be inferred. I think we're going to want this no matter what we do.

[eddyb]

I guess there's no harm in separately borrowing paths as long as it's opt-in behind the feature-gate, and we don't apply it to stable code (wait, that'd be bad anyway because it would have the borrowck effects, so there's no way stable code is affected size-wise at all).

[nikomatsakis]

@blitzerr To elaborate a bit on the context (let me know if this helps):

• Today, the closure type looks something like Foo<(U1, ..., Un)> where U1...Un are the types of the upvars. When we first create this closure type in rustc, we actually create a type like Foo<(_, ..., _)> -- i.e., we make a tuple with N inference variables. The idea is that we know the number of upvars that will be captured, but we don't (yet) know their types. Those final types are computed by the upvar inference, and that loop you referenced is the one that unifies those variables with the types we computed for each upvar.
• The reason that we are able to know the number of upvars is that we go through a look at the local variables that are referenced, and we can just count them (so e.g. x.y references x). We can do this without any type checking or anything, just based on name resolution.
• But when we modify things to capture paths, that gets harder. It's harder for a few reasons. First off, capturing the "unique" paths requires a bit of type inference. Something like x.y might be equivalent to (*x).y, since the compiler inserts "autoderefs" -- or it might not! We'd have to look at the types to know for sure. Furthermore, there are some limits on what we can capture: in particular, if you are moving the value into the closure, we can't move things out from types that declare a destructor, so x.y might capture x or it might capture x.y, depending (and indeed we don't yet know if we are moving or not, that is computed by the upvar inference).
• So we want to instead create a type like F<_> and then, in the upvar inference, when we have the full types and other info available, decide how many entries to make in the tuple. For now it will be the same N as before, but the important part is that we are deciding it later.

[blitzerr]

@nikomatsakis So I guess my dumb question is if we are not creating the tuple of upvars here then what shall we do here? https://github.com/rust-lang/rust/blob/1a87c49e33d87955e28fa92a8d59a17264ac6044/src/librustc_typeck/check/closure.rs#L91-L101

Go back to what it used to be here ?

[nikomatsakis]

@blitzerr not a dumb question, and yes, I think we would go back to the older behavior -- so we would just create a plain old type variable, instead of a tuple of type variables.

Then we'd have to modify the upvar code that is iterating over the upvar types:

https://github.com/rust-lang/rust/blob/1a87c49e33d87955e28fa92a8d59a17264ac6044/src/librustc_typeck/check/upvar.rs#L200-L202

so that it instead does something like

// Build a tuple (U0..Un) of the final upvar types U0..Un
// and unify the upvar tupe type in the closure with it:
let final_upvar_tuple_type = tcx.mk_tuple(final_upvar_tys);
self.demand_suptype(span, substs.upvar_tuple_ty(), final_upvar_tuple_ty);

[blitzerr]

Thanks a lot @nikomatsakis Will get on it.

[nikomatsakis]

@blitzerr let me know how it goes :)

[arora-aman]

Depends on #7

[m-hugo]

Depends on #4

so #4 depends on #4?

[arora-aman]

Depends on #4

so #4 depends on #4?

Oops. Fixed.

[arora-aman]

Depends on #4

so #4 depends on #4?

Oops. Fixed.

This issue changes where we create the tuple of capture tys where #7 changes what is being captured. The code for both issues is in very close proximity of each other but this can be done separate from #7.

[nikomatsakis]

Update:

• We are always using tuples in auto trait selection and we have a hack to fix diagnostic (when we see a tuple, check if it the parent is a closure/generator and -- if so -- ignore it)
• async-await diagnostics is also working now, similar hack

[arora-aman]

closed by rust-lang/rust#77873