Refactor diverging and numeric fallback.

[leodasvacas]

This refactoring tries to make numeric fallback easier to reason about. Instead of applying all fallbacks at an arbitrary point in the middle of inference, we apply the fallback only when necessary and only for
the variable that requires it. The only place that requires early fallback is the target of numeric casts.

The visible consequences is that some error messages that got i32 now get {integer} because we are less eager about fallback.

The bigger goal is to make it easier to integrate user fallbacks into inference, if we ever figure that out.

[rust-highfive]

r? @nikomatsakis

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

[nikomatsakis]

Hmm. I have some concerns about this approach. Let me start with the opening sentence:

Instead of applying all fallbacks at an arbitrary point in the middle of inference

The current place where we apply inference is not really arbitrary -- it's intended to be essentially the last possible point. In other words, we want to give the maximal chance for the user's program to add constraints. The long term vision for how type-check should work is that we want to make it less imperative. Let me give an example. Right now, if you do foo.bar(), then the type of foo must be known -- if it is an unresolved type variable, you will get an error. The reason for this is that we have to be able to determine how many autoderefs to insert and so forth, and we typically need a certain amount of type information to do that. Here is an example:

let value = None;
loop {
    if value.is_some() {
        // the type of `value.unwrap()` is not yet known
        value.unwrap().bar(); 
    } else {
        // this would tell us that the type of `value.unwrap()` is `char`,
        // but we never get this far
        value = Some('a');
    }
}

Currently, this code will error because, at the time of type-checking, the type of value is Option<?T>, where ?T is an unresolved type variable, and hence value.unwrap() has type ?T, so we can't figure out how to dispatch the method bar().

What I would like to eventually do in this sort of scenario is not to report an error, but rather to file a pending obligation. Basically we could defer type checking the method call to bar and proceed. Later on we can try again, if we acquire more information about ?T -- e.g., after value = Some(a). (This is in fact what we do if you avoid the . notation, and for example invoke Bar::bar(foo.unwrap()), presuming Bar is some trait.)

In contrast, this PR would make it so that foo.bar() (which invokes structurally_resolved_type) would apply fallback to the type inference variable ?T. This feels wrong to me -- that fallback might not be the desired type, which means that later on, when we get around to type-checking the correct type, we'll get a type error, but we got that type error because we were too eager to jump to conclusions.

Now, it's true that we already do a certain amount of that -- particularly around coercions, but also as a side effect of trait matching, and I think we can find ways to go forward with the "defer" notion and still incorporate those side effects, but I am wary of introducing more such cases to accommodate.

[nikomatsakis]

@leodasvacas I was thinking more about this and #46206 (and I do apologize I haven't given you any feedback there yet) -- would you be interested in maybe scheduling a time to chat about this? (e.g., via some video chat or just IRC) I'd like to kind of brainstorm a bit and see if we can bottom out the design space a bit. I'm also curious if you're interested in helping so some related but different refactorings -- for example, making type inference less 'imperative' and prone to reporting errors, as I just described above.

[leodasvacas]

Thank you for your quick and illuminating review.

I believe you wrote your comment with user fallback in mind, and how running user fallback in structurally_resolved_type is a hazard for future improvements to inference that allow us to stop calling structurally_resolved_type. As I understand it, the hazard is that if we stabilize user fallback, and later improve a certain point in inference to no longer need fallback, we have to keep the fallback for backwards compatibility, therefore doing worse than we could.

The ideal solution is to get rid of structurally_resolved_type before stabilizing user fallback. Since user fallback is my pet feature, I'd be motivated to work on anything that unblocks it, including improvements to inference that help in solving this issue. Thank you for the offer of mentorship, I'll nag you on IRC sometime so we can chat.

However, getting back to this PR, I see it as just a refactoring. The moment where we currently run integer and diverging fallback is correct, but it's not the last possible correct moment. This PR seeks to run it in the last possible moment, for each variable that needs it. In the future if we call structurally_resolved_type less often, we will naturally fallback less often.

[nikomatsakis]

However, getting back to this PR, I see it as just a refactoring. The moment where we currently run integer and diverging fallback is correct, but it's not the last possible correct moment. This PR seeks to run it in the last possible moment, for each variable that needs it. In the future if we call structurally_resolved_type less often, we will naturally fallback less often.

I don't see it quite this way. In particular, once code starts to compile (instead of erroring), we are (more) committed to a certain course of action. It may well be that, if we fixed the call to structurally_resolved_type or tweaked the order in which we invoke it, then we would not trigger fallback at all, no?

[leodasvacas]

@nikomatsakis You're right. This PR currently:

• Does no fallback in structurally_resolved_type.
• Does fallback only at the very end of type inference in select_all_obligations_or_error.
• And in the target expression of casts. For example in:

let x = 512;
x as u8;

We must infer x to i32 rather than u8, for backwards compatibility. So our current fallback isn't that correct after all, proving the point that it's kind of arbitrary and hard to reason about.

It would be prudent to crater, to see if there are other unintended effects of the current fallback that this accidentally "fixes".

[alexcrichton]

ping @nikomatsakis, this may be ready for another look?

[nikomatsakis]

I think @leodasvacas still need to schedule a chat. Sorry that's been difficult. I'm back from vacation now, but I'll be traveling a bit next week -- something like wed, thu, or fri could probably work. (@leodasvacas -- I forget, were we chatting before on gitter? e-mail?)

[carols10cents]

Marking as blocked pending a meeting with @nikomatsakis.

[leodasvacas]

We've met and @nikomatsakis is taking another look considering this is supposed make no more and no less code compile, as it should be just a refactoring. This is now S-waiting-on-review.

[nikomatsakis]

This looks nice, I agree, I didn't have time to fully grok why some of the behavior changed, though. Maybe you can walk me through it a bit? Thanks :)

[nikomatsakis]

This looks nice, I agree, I didn't have time to fully grok why some of the behavior changed, though. Maybe you can walk me through it a bit? Thanks :)

[nikomatsakis]

Nit: I'd rather see something like this

Neither if self.type_var_diverges(ty) => match fallback {
    Fallback::Full => ...
    Fallback::Whatever => ...
}
Neither => return

the reason is that this way I know what the variants are for fallback without having to check, so it's easier for me to reason about whether this is doing the right thing

[leodasvacas]

Done

[nikomatsakis]

So, clearly this isn't a pure refactoring -- at least the error messages changed. This new error doesn't appear particularly helpful -- I wonder if we should find a way to suppress it.

[leodasvacas]

The error suppression regressed here, don't know what's going on. An is_tainted_by_errors() check before emitting cast errors should fix this, not sure if it's a good fix.

[nikomatsakis]

That seems like a reasonable thing to do, to me, or at least around cases involve "unknown" things like type variables.

[nikomatsakis]

this test was renamed, but what happened to the old .stderr file?

[nikomatsakis]

can you walk me through why the error behavior changed in this example?

[leodasvacas]

i32 was replaced with {integer} because previously we did fallback before analyze_closure(), now we do fallback after analyze_closure() so any errors in closures taint the context and prevent fallback.

Don't know why the order of the two errors was swapped, or why there is a place where _ was replaced with ((),).

[leodasvacas]

I'm now checking for errors before emitting "unknown cast" errors. This fixes the test that regressed. The issue issue-26480.rs test did not need to be touched at all, I reverted it to master.

[bors]

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

[nikomatsakis]

I am pondering whether this is the right change.

I mean, the original intention of applying fallback where we did was that -- after this point -- we were not supposed to be introducing "new constraints" that might have influenced the type of the variables. Therefore, it makes sense to do fallback (which may in turn introduce new constraints, leading to some iteration).

Do you think we are introducing new constraints such that fallback is occurring too early? (Do we have an example of that?)

In general, though, I am on board for trying to restructure typeck to be as "lazy" as possible, so I am trying to decide whether this change is in fact doing that. It's not the way I had originally thought to go about things, which would be more a matter of identifying places where we invoke structurally_resolve_type or similar helpers and changing them into deferrable obligations (which would then be ultimately resolved during select_obligations_where_possible).

(Though a pre-req for doing that work is probably reworking the trait system to improve efficiency, another effort I'd like to have started yesterday.)

[nikomatsakis]

More specifically, things like closure upvar inference (and casts) were meant to operate over the "fully inferred types", basically. That said, I think there are corner cases that make this something of a fiction. Have to bring that back into cache.

[leodasvacas]

identifying places where we invoke structurally_resolve_type or similar helpers and changing them into deferrable obligations

I understand this goal, in my simplistic bovine mind the ideal type checker goes like this:

1. Introduce all lazy constraints.
2. Solve as much as possible.
3. Do fallback.
4. Solve as much as possible.

This PR tries to make it blatantly obvious in the code that no other constraints are added between steps 3 and 4.

Therefore, it makes sense to do fallback (which may in turn introduce new constraints, leading to some iteration).

I guess we haven't found any cases where the "early" fallback was actually helping inference more than the "late" fallback does.

Do you think we are introducing new constraints such that fallback is occurring too early?

I gave the following example up-thread:

let x = 512;
x as u8;

Casting would hint x to u8, but fallback forces it to i32, this is the only case I found where early fallback was needed for backwards compatibility.

I don't know if it's true that we don't introduce any new constraints in closure_analyze, but it sounds like something difficult to reason about. With this PR we wouldn't have to reason about it anymore. That fact that this PR passes CI is empirical evidence that closure_analyze isn't really relying on fallback, whether that's really true is beyond my knowledge.

[nikomatsakis]

This PR tries to make it blatantly obvious in the code that no other constraints are added between steps 3 and 4.

OK, that makes sense to me.

I guess we haven't found any cases where the "early" fallback was actually helping inference more than the "late" fallback does.

Well, consider the upvar inference in particular. This code is tasked with figuring out whether a closure ought to be FnOnce, FnMut, or Fn. It does this by looking at what sorts of things the closure does when it executes. For example, given this closure:

|| foo(x)

if the type of x is Vec<String>, that's a move of x, and hence the closure must be FnOnce. But if the type is u32, then this is a copy., and the closure can be simply Fn.

So what are we to do if the type is not yet inferred and we are later going to come and apply defaults? We can't know yet whether that is a move or a copy.

I think the only thing we could do would be to defer the decision about what traits the closure implements until after defaults have been applied.

[leodasvacas]

Based on your explanation, it seems plausible that upvar inference would like to add constraints, consider:

fn fn_closure<F: Fn()>(f: F) {}

fn main() {
    let x = None; // Something uninferred.
    fn_closure(|| std::mem::drop(x));
}

It seems reasonable that upvar inference would add a Copy bound to x or perhaps a Move bound (though I don't see how those bounds could actually help inference).

It's also possible that defaulting would prevent upvar inference from working, in fn foo<T: FnOnce() = Fn()>(_: &T) {} after defaulting T is stuck as Fn() when upvar inference could have inferred it to FnOnce().

So perhaps upvar inference isn't a special case and should be a lazy constraint, while it isn't it would use the usual mechanisms for eager type resolution such as structurally_resolve_type. Whether we want defaulting in eager type resolution is something to be decided.

[nikomatsakis]

Argh, sorry it took me a few days to respond. Just juggling a lot of reviews lately and this one takes deeper thought than most.

---

it seems plausible that upvar inference would like to add constraints, consider:

That is true, you could imagine it imposing the requirement, but it's now how it's defined today. There are actually two things involved here. First, we have a rule (which perhaps was not the wisest rule, but it's currently there) that based on the "expected type and where-clauses", we will select the closure kind -- so in this case, upvar inference actually doesn't get involved, we just pick the closure kind as Fn from the get go. (If you're interested, I'd like to try and experiment where we back off from that rule and measure the impact.)

In that case, the error is detected during borrowck, which comes after typeck and therefore can assume all types are fully known. Note that in general the requirement for things to be Copy (or else be moved) is flow dependent and is ultimately enforced on MIR, and we require all types to be built in order to build MIR.

But if you wrote the example as:

let x = || drop(x);
fn_closure(x);

then the expected type is not involved, and indeed the closure would be selected as FnOnce, because it needs to own its content. This would result in an error as well, but later on, when invoking fn_closure (since Fn would not be satisfied).

---

It's also possible that defaulting would prevent upvar inference from working

Actually, upvar inference is supposed to be independent of the demands that are placed on the closure (modulo that bit about the "expected type"). That is, we select the most permissive trait that the closure could possibly implement, and that is weighed against what it required to implement by others.

In any case, in your example, the type variable T would be inferred to the closure's unique type, so the default (of dyn Fn()) would never be used.

---

So perhaps upvar inference isn't a special case and should be a lazy constraint

Maybe, but I'm not convinced yet. =) Keep in mind that it's always something we could change later.

[bors]

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

[pietroalbini]

@nikomatsakis this PR needs a review!

[nikomatsakis]

@bors r+

[bors]

📌 Commit d49d428 has been approved by nikomatsakis

[bors]

⌛️ Testing commit d49d428 with merge 9d1980c...

[bors]

💔 Test failed - status-travis

[kennytm]

@bors retry #48192 (dist-i686-linux + dist-mips64el-linux)

Both are the victim of 20-minute libstd build.

[bors]

⌛️ Testing commit d49d428 with merge dab83c7...

[bors]

💔 Test failed - status-travis

[kennytm]

@bors retry

Spuriously canceled? I can't find any failures.

[pietroalbini]

@kennytm some builds were manually cancelled to allow jobs related to the release to run.

[bors]

⌛️ Testing commit d49d428 with merge 5570cdc...

[bors]

☀️ Test successful - status-appveyor, status-travis
Approved by: nikomatsakis
Pushing 5570cdc to master...