Make coherence more tolerant of error types.

[nikomatsakis]

This is an alternative to #29954 for fixing #29857 that seems to me to be more inline with the general strategy around TyError. It also includes the fix for #30589 -- in fact, just the minimal change of making ty_is_local tolerate TyError avoids the ICE, but you get a lot of duplicate error reports, so in the case where the impl's trait reference already includes TyError, we just ignore the impl altogether.

cc @arielb1@sanxiyn

Fixes#29857.
Fixes#30589.

[rust-highfive]

r? @Aatch

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

[nikomatsakis]

@arielb1 in case it wasn't clear, this is what I meant -- thoughts on which approach is overall better?

Incidentally, I do agree that refactoring projection to be explicitly fallible is probably a good idea (versus the current "return an obligation that is known to result in an error").

[arielb1]

I feel that the handling of ErrorCandidate there is somewhat scary.

[nikomatsakis]

@arielb1

I feel that the handling of ErrorCandidate there is somewhat scary.

Do you mean specifically the way the projection code injects a TyError and an "obligation that should yield an error"? If so, I agree -- TyError ought to only be used when an error has been reported, and it violates that rule.

[nikomatsakis]

Added two new tests for other scenarios. I am torn here, in that I do feel this approach is how TyError is basically supposed to work -- essentially, treat error things as correct to avoid downstream errors -- but I do have some mild concern that it may cause coherence to accept programs it should have rejected, and I'm just not clever enough to come up with this programs.

[bors]

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

[nikomatsakis]

@arielb1 so take a look at that last commit. I changed the project error strategy to use the projection as the type if an error occurs -- I initially thought that this should be no more accepting than before, since the associated obligation should still fail just as before. But, in fact, this change makes the original input pass coherence, whereas before it failed, so that wasn't quite right. The thing is, I think it is correct for that input to pass actually. Anyway, I have to run, but I was curious to get your take on this approach -- I was hoping it would be one we can both agree on for fixing this bug. :)

[nikomatsakis]

Hmm, that branch seems to fail compile-fail. No time to look at why that is just ntke though.

[nikomatsakis]

Just NOW

[arielb1]

The code is an rpass in my branch (and my simplified example).

I think that improving project_to_error is best if it can be done without causing other issues.

[nikomatsakis]

Well, I see, the problems are just (as might be expected) that we get some derived errors we ought not to be seeing (because we are no longer returning TyError here). Hmm. I had one other thought along these lines, but yes I think the test should be rpass, which means the "fix" I had before I no longer like :)

[arielb1]

@nikomatsakis

If this is so simple, I'll r+ if you fix the errors.

I also prefer that you use this simplified testcase:

pubtraitFoo{typeOutput:'static;}pubtraitBar<P>{}pubtraitBaz<P>{}impl<T:'static,W:Foo<Output=T>>Baz<*mutT>forW{}impl<P,T:Baz<P>>Bar<P>forT{}impl<T>Bar<*mutT>for[T;1]{}fnmain(){}

[arielb1]

The reordering fix also seems to resolve the ICE, but I like yours better.

[nikomatsakis]

@arielb1 So i've been thinking about this all morning and digging through logs. I think I have now come to the conclusion that indeed the only non-scary fix is to stop having project return a TyError. As we have seen previously, that idea is basically flawed. It is frustrating that this leads to downstream errors being reported though -- but I haven't yet settled on a good way to fix it. Let me just try and dump my mental state here "for the record". Sorry that I am writing some things you probably already know (but you're not the real audience, I am;).

First off, the general strategy with TyError is that something went wrong before so we should generally suppress error reporting and make optimistic assumptions from this point on (as we both agree, the code in project is slightly dubious in this respect, as the error has not, in fact, yet occurred, though we believe it will occur). This seems like a good strategy.

Now, the code in select for ErrorCandidate tries to follow this strategy by saying that evaluating any trait reference that references [type error], such as Qux<[type error]>: Bar, should be treated as successful. Under normal trait selection, this is good, because success is optimistic.

However, what coherence is interested in is actually a different predicate. Using RFC 1144 syntax, coherence is really evaluating Qux<[type error]>: !Bar, but right now the code is a bit hacky in this respect, and it piggybacks on the same code paths, and just interprets Err differently. So now the code for ErrorCandidate is actually not doing the optimistic thing by reporting success: it's causing coherence to ultimately generate an error for the end-user.

There are in fact other situations where this crops up. The current technique for handling them is checking the intercrate flag. The name is sort of not good, but the general effect is to cause us to report ambiguity instead of error in a number of cases, precisely because Err is now the success path, so we want to err (no pun intended) on the side of ambiguity or success.

So now, your original patch made me uneasy because ErrorCandidate was being treated as "success" or "failure" based on whether or not we were evaluating or selecting. That still seems incorrect to me; those two functions should be kept in sync. But my first attempt to fix missed the mark: it addressed the ICE, but it left the logic that treats error candidate trait references as resolving, which thus made coherence do the wrong thing.

At some point I was considering that we could just make TyError trait resolutions ALWAYS be treated as error. After all, in the case of coherence, this is the "optimistic" thing to do, and in the normal case, error messages that involve TyError will be suppressed anyway, so it's ok to take the pessimistic path. However, I've decided that I'm deeply scared of this path (reasons below). In fact, I'm getting pretty scared about that project code altogether.

Scary thought the first. PR #30533 introduces some logic like the following: if we see an error evaluating obligation X occurring in tree Y, we can ignore all further pending obligations in tree Y, because we know that tree Y will never succeed. However, that would be incorrect in the face of the TyError treatment above, because it may be that the actual error that is supposed to be reported is one of those later obligations, and this TyError was issued in advance, knowing that this error would be reported later.

Scary thought the second. The whole logic relies on the idea that we will report an error when we later encounter the obligation that failed to resolve. But in this case (and I think we saw this before), the projection itself is part of that trait reference. That is, we have a constraint like <Qux<$0> as Bar>::Output = $0, and we wind up unifying $0 as [type error] and then adding the obligation that Qux<$0> as Bar must hold. But of course that winds up being Qux<[Type Error]> as Bar must hold. And then the trait resolution machinery sees the [Type Error] and assumes it can take the optimistic pathways and suppress error reporting. This last scary thought shows that this whole idea is just fundamentally flawed and to make it work would require some very fragile footwork around [Type Error].

So now the question becomes: can we suppress the derived error reporting in some useful way? Initially I had the thought that we could resolve to the projection but record the projection in the infcx as being erroneous. So we would, in effect, unify $0 with <Qux<$0> as Bar>::Output, but then record that this is an erroneous projection. That way, later on, we could suppress reports like "expected <Qux<$0> as Bar>::Output but got ()". But scary thought the second makes me feel nervous about that too.

Therefore, right now, I am leaning towards not suppressing the derived errors. They are sort of comprehensible anyhow. In the future, I would like to move towards lazy normalization (enabled by #30533 in any case), which would give us a chance to overhaul the normalization and project machinery and (I think) make this case easier to handle. In particular, we can probably just propagate this error up and avoid unifying $0 with anything. I think the best solution is to punt until that time.

[nikomatsakis]

@arielb1 I feel like scary thought the second is something we discovered already and I just forgot about. Is that true? Do you remember?

[nikomatsakis]

Oh, well, obviously that is true, given that the comment that I myself removed talks about it. Well, yeah, it's not good.

[nikomatsakis]

Hmm, I just got to thinking. In the cyclic case, is it possible that unifying will still leave us in an exposed position? Surely it's better to unify with T::Output than [type error], but only marginally so -- it violates other invariants, such as that projection types, post normalization, are always valid. And maybe it turns out that the user has some kind of impl that winds up being, in a twisted way, applicable to Qux<<Qux as Bar>::Output>: Bar, even though it sort of shouldn't be? (not sure how that could happen)

[nikomatsakis]

OK, I found a strategy I like. I am now substituting a fresh inference variable. This seems to avoid all the hazards I was concerned about. I've got a PR that works great except that issue-4972.rs in compile-fail ICEs. That test case seems kind of bogus, I'm digging into a bit more now.

[arielb1]

@nikomatsakis

Yes, I was definitely aware of scary thought the second (the reason it is not an issue in practice is subtle - the check for ErrorCandidate does not resolve type variables, so you get the [type error]without type variables) - it is basically the issue I hate normalize_to_error.

I feel that normalizing to the projection is fine - after all, this is what we often do when the projection contains type parameters, so the cascading error messages are not that unusual. Maybe we should just split the normalize-to-type and normalize-and-unify paths somehow, and either normalize to a projection or not unify the resulting predicate.

The ICE in #4972 slightly scares me - we should investigate it.

[arielb1]

Of course, if unifying types causes more impls to be available, this can cause errors in other ways.

[nikomatsakis]

r? @arielb1

[nikomatsakis]

This latest version now passes make check. I did the following:

• revamp project to use a fresh inference variable in error cases instead of TyError (and generate the projection we expect to yield an error) -- this restores the invariant that TyError always means an error has been reported, to my knowledge;
• make various paths more tolerant of [error] because it crops up hither and thither for whatever reason;
• remove ErrorCandidate in favor of just being ambiguous, because it works just as well, since we suppress error reports about ambiguity if TyError is involved.

I guess the last point might be silly, it does mean that we'll keep this obligation hanging around for longer.

[arielb1]

Die!

[arielb1]

@bors r+

[bors]

📌 Commit b0f6a47 has been approved by arielb1

[pnkfelix]

@bors p=1

(upping priority for regression fixing or beta-nominated PR's)

[bors]

⌛ Testing commit b0f6a47 with merge 19d8f46...

[bors]

💔 Test failed - auto-linux-64-nopt-t

[pnkfelix]

@bors retry

[bors]

⌛ Testing commit b0f6a47 with merge d01ed8a...

[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-x-android-t, auto-linux-cross-opt, auto-linux-musl-64-opt, auto-mac-32-opt, auto-mac-64-nopt-t, auto-mac-64-opt, auto-win-gnu-32-nopt-t, auto-win-gnu-32-opt, auto-win-gnu-64-nopt-t, auto-win-gnu-64-opt, auto-win-msvc-32-opt, auto-win-msvc-64-opt

[nikomatsakis]

Accepting for beta since this addresses a regression.