Fix type inference related to upvars in closures

[sanxiyn]

Fix#20558.

[rust-highfive]

r? @Aatch

(rust_highfive has picked a reviewer for you, use r? to override)

[Aatch]

Do you mind say where they're checked? "Elsewhere" tends to be kind of frustrating when looking at these comments.

[sanxiyn]

They are checked in numerous places... @nikomatsakis, do you have better ideas how to word this?

[nikomatsakis]

I'm not sure what to make of this patch. Some thoughts:

1. @sanxiyn is correct I think that this is harmless. More specifically, upvars are always (atm) local variables, and local variables must always (atm) have sized types.
2. That said, I'm not entirely sure what error is being prevented. That is, I think that the upvar types are known, just known to be an integer literal type, but we consider integer literal types to be sized, so that doesn't seem right.
3. I thought at first that the problem was that the requirement to fulfill all obligations comes before i32 fallback. If that's true, more general support for fallback (which I'll probably be implementing soon) would also address the problem, I guess, but at the same time I can't see that this change does harm.

@sanxiyn have you traced out just where the error is cropping up that you are preventing?

[sanxiyn]

As far as I understand, 3 is correct. The error comes from checking IntVar: Sized before i32 fallback.

[nikomatsakis]

OK, so, I started digging into the problem. I think it is a bug in the caching algorithm. I am double-checking, but I think the problem is that when we ask to get the list of upvar types for a closure, in some cases we do not yet know the result. This is because we haven't run all the inference we are going to need to figure everything out. This yields an ambiguous result -- which is generally the correct thing to do, and should not (typically) be harmful. However, in this case, what happens is that the ambiguous result gets cached -- again, not (typically) harmful, because normally the cache key is careful to note what parts needed more inference, so when more inference has been done, that cache entry no longer matches. (Example: Vec<_> : Foo would be a cache key; later, after inference has resolved it to Vec<i32> : Foo, the cache entry no longer matches.) What makes it bad is that it has been made ambiguous because the upvars were not available, but that is not part of the "cache key", which is just "closure : Foo"). I think the fix is prob just not to cache these results for now, since I don't see any obvious way to get that info into the cache key.

[nikomatsakis]

That said, this patch may still be a good optimization. But I'd prefer to land the full fix first. I'll prepare a PR now.

[nikomatsakis]

taking liberty of re-assigning responsibility for this patch to me

[sanxiyn]

This is waiting on #21523.

[nikomatsakis]

So the crucial underlying problem should be fixed now. Now the question is whether this patch on its own makes sense as a kind of optimization.

[sanxiyn]

I have updated a comment as suggested.

[nikomatsakis]

@bors r+ 4ef7551 rollup