select: treat ErrorCandidate as a real error

[arielb1]

Otherwise, projection can result in type errors that are unified
into the trait inputs. We should probably refactor the
normalize_to_error mess.

We could instead change ctp::identify_constrained_type_params to put a projection's desugar selection before the projection. Would you like that better?

Fixes #29857.

r? @nikomatsakis

[sanxiyn]

Travis failures seem real.

[arielb1]

fixed test failures.

[nikomatsakis]

Hmm, I'm not sure about this change. I feel like this will potentially create more downstream fallout. Is it possible that the right fix to #29857 is just to change ty_is_local_constructor to not panic and instead return true or false or maybe even a Result? This would be basically more inline with the usual error strategy, which is that when you see an error result, you should suppress all downstream errors.

OTOH maybe it's ok to have trait selection fail and we just suppress the output of failed trait selection (which I know we attempt to do anyway).

[arielb1]

@nikomatsakis

The problem is that ErrorCandidate short-circuits candidate selection, so you may accidentally end up with false positives.

[arielb1]

However, I feel like putting a ProjectionPredicate's parent TraitPredicate ahead of it in the ordering would also fix this issue. Would that be better?

[nikomatsakis]

@arielb1

Sorry for the delay. Maybe my head just isn't in the right space anymore, but can you elaborate on this:

The problem is that ErrorCandidate short-circuits candidate selection, so you may accidentally end up with false positives.

It still seems to me that converting an Ok(ErrorCandidate) return value to an EvaluateToErr is inconsistent with our general error-handling strategy.

[nikomatsakis]

OK, so, given that this is a stable -> beta regression, we've got to reach some decision here. I still kind of feel like this is the wrong approach, and the right approach would be to tolerate errors in the coherence code (which feels like what we normally do -- consider errors a success). But it'd be helpful @arielb1 if you could elaborate on what you meant about "false positives" and so forth.

[arielb1]

I should really be reading my e-mails more often.

The root cause of the problem here is normalize_to_error again. It means that candidate evaluation can give us some TyErrors, and if we get unlucky the TyError can mix in with the underlying selection candidate. I think that just moving the selection predicate ahead of the projection in evaluation order would fix this - I will try that later today - but I am not sure whether that is a better idea.

[nikomatsakis]

@arielb1

I should really be reading my e-mails more often.

No worries, me too. :)

The root cause of the problem here is normalize_to_error again. It means that candidate evaluation can give us some TyErrors, and if we get unlucky the TyError can mix in with the underlying selection candidate. I think that just moving the selection predicate ahead of the projection in evaluation order would fix this - I will try that later today - but I am not sure whether that is a better idea.

So I think what you are saying here is that, but for normalize_to_error, we would not be seeing TyError at all, and if you reordered things, we might restore that invariant? That seems...fine, but it seems like being prepared for TyError is also a perfectly fine thing to do.

[arielb1]

@nikomatsakis

Right. The TyError is one of the bogus ones generated by normalize_to_error. I ended up not making that experiment.

[nikomatsakis]

@arielb1 while on vacation I was pondering this, and I was wondering if perhaps ignoring errors in coherence (as I have been suggesting) could lead to incorrect code being accepted? Still, it's not obvious to me how this could happen, I'd expect that such an error would be reported during the WF phase. Anyway, it's clear we have to land SOME patch here. I am going to briefly experiment with a patch that is more accepting of type-error today, then we can make a final decision.

[arielb1]

@nikomatsakis

Ignoring errors in coherence should typically actually lead to less code being accepted, but this is still wrong.

[bors]

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

[nikomatsakis]

Closing in favor of #30676. Since @arielb1 r+'d I presume he has no objection, but ...