Fix ICE from #33364 by selectively skipping confirmation pass. #34573
Conversation
|
(rust_highfive has picked a reviewer for you, use r? to override) |
pnkfelix
added
the
beta-nominated
|
Oh I need to add the regression test too. |
…work in trans. The main idea here is that if there are no unification problems to solve, then trans does not need to do any confirmation of the impls that it selects for a trait, nor the associated items it projects for a trait. (In principle the above mentioned ICE should instead be solved in the long term via lazy normalization. But for now this is a simpler solution and a good idea in any case.)
|
Thanks @pnkfelix ! cc @rust-lang/compiler This regression fix needs to land before monday/tuesday to get into 1.10. |
brson
added
the
T-compiler
| if selcx.projection_mode().is_any() { | ||
| // Issue #33364: `ProjectionMode::Any` means trans, during | ||
| // which there is no (good) reason for confirmation to fail | ||
| // (because type check has already admitted the source code). |
There was a problem hiding this comment.
That's not necessarily true if anything does probing (such as checking if a vtable needs to contain a specific method, based on where clauses of that method).
There was a problem hiding this comment.
@eddyb Wait so are you saying it is not actually sound for me to skip the confirms via the logic I have put here?
Or ... are you "merely" saying that the comment as written here is misleading?
There was a problem hiding this comment.
I'm not sure in which situations this may backfire, but for example, vtable building in trans probes for obligations on methods to decide whether to include them. There are also secondary probes related to specialization, during selection.
|
#33364 is a regression from 1.8 to 1.9 - it already hit stable. |
arielb1
removed
the
beta-nominated
|
De-beta-nominating since not a relevant regression. |
| @@ -1247,6 +1247,34 @@ fn confirm_callable_candidate<'cx, 'gcx, 'tcx>( | |||
| fn_sig, | |||
| flag); | |||
|
|
|||
| if selcx.projection_mode().is_any() { | |||
There was a problem hiding this comment.
Hmm. This is somehow hackier than I had imagined. What I had hoped was that we would attach a (generic) filter function to the FulfillmentContext -- when not in trans, this filter function would accept all work, but within trans, it could be more selective.
EDIT: Not that it wouldn't necessarily always be more selective.
There was a problem hiding this comment.
I actually told @pnkfelix about ProjectionMode::Any, he had some finer-grained flag initially.
I'm not sure I understand what situations this hack can produce invalid results in.
Remember that trans doesn't just look for things that are guaranteed to succeed, but also has to be able to correctly tell if something would succeed (for various reasons pointed out in a comment above).
There was a problem hiding this comment.
Remember that trans doesn't just look for things that are guaranteed to succeed, but also has to be able to correctly tell if something would succeed (for various reasons pointed out in a comment above).
Sometimes yes. It depends on context. This is why I was imagining a filter that the context can provide. =)
Certainly the majority of cases though trans is doing a lot more work than it ought to, just re-resolving things that it knows will pass (and which sometimes yield ICEs when they stumble over other bugs, as in this case).
There was a problem hiding this comment.
I'd like to try to make more progress on this, but I also want to make sure I understand the conversation here.
In particular, @nikomatsakis, when you wrote above that you expected a filter to be attached to the FulfillmentContext, did you mean that, or did you mean SelectionContext?
(I ask because I can at least see how a SelectionContext would connect up to the code as written, but attaching it to the FulfillmentContext seems like ... its leaving out details about how that filter would actually prevent this particular confirmation while not messing things up elsewhere. I have no intuition right now as to where the filter fundamentally belongs)
There was a problem hiding this comment.
(for those following along this thread, niko left a comment below saying "I meant FulfillmentContext, but I may be incorrect.")
|
triage: beta-nominated We'll probably want to backport this to ensure the ICE fix gets out soon, even though it's ICE'ing on stable as well. (so long as the compiler team approves of the backport, of course) |
|
@pnkfelix well, I meant |
|
OK, so, I dug a bit more into the approach that I had been advocating for @pnkfelix. I still think that adding a predicate to the |
|
☔ The latest upstream changes (presumably #35605) made this pull request unmergeable. Please resolve the merge conflicts. |
|
Sorry I've been slow here. I still feel uncomfortable with trying to decide the right fix. |
|
@pnkfelix and I talked the other day. We want to take a shot at pursuing a somewhat more principled fix here, but this kind of hack is still a possibility. =) Going to close this PR in any case for now. |
Fix #33364: during trans, confirmation should never fail (because type check has already admitted the source code).
So skip confirmation in that context, unless there is some reason (i.e. unresolved unification variables, or a type undering a Binder for region variables) during projection of an associated item) that we still need to run it.