Simplify to a common type variable when picking a disjunction of bindings #18770
Conversation
|
@swift-ci Please test. |
| if (llvm::all_of( | ||
| disjunction->getNestedConstraints(), | ||
| [&](Constraint *bindingConstraint) { | ||
| if (bindingConstraint->getKind() != ConstraintKind::Bind) | ||
| return false; | ||
|
|
||
| auto *tv = | ||
| bindingConstraint->getFirstType()->getAs<TypeVariableType>(); | ||
| auto *tv = cs.simplifyType(bindingConstraint->getFirstType()) |
There was a problem hiding this comment.
Maybe a better choice in this case would be to skip disjunction completely if it already has its type variable bound to something?
There was a problem hiding this comment.
In the example you've added force unwrap disjunction is bound to @lvalue ImplicitlyUnwrapped.x: $T1 == @lvalue Int? by the time we get to selectBestBindingDisjunction.
There was a problem hiding this comment.
Well, I don't know if the case where the original first type has been merged with some other type variable is important to handle here. It at least seems plausible.
There was a problem hiding this comment.
If we have simplified type variable to something else once why do it for every choice? That disqualifies disjunction right away...
There was a problem hiding this comment.
It's not necessarily simplified to a concrete type. It is in my example, but I don't think it would have to be.
I'm just trying to stop this from crashing on actual code. If you think there's a significant improvement to the algorithm possible, I'd rather leave that to you.
Honestly, the whole structure of this function seems strange to me: it's building up a list of viable disjunctions, then doing a second pass to preferentially pick one if it satisfies some obscure extra condition, then if that doesn't work just using the first viable one. Why not check the extra condition in the first pass and fast-path out immediately if it passes rather than considering all the other disjunctions first, and just save the first viable disjunction in case we don't ever find one that passes the extra condition?
There was a problem hiding this comment.
+1, I think this could be made better. Disjunction should either have a single type variable on the or function type e.g. ($T0) -> $T1 for initializers on left-hand side of every choice, which means that all of the choices should be simplified to the same type (on left-hand side), I'm not sure why it's checked in the loop.
@rudkx could you please shed some light on this?
There was a problem hiding this comment.
I'm going to merge; if you want me to do a follow-up to restructure this function, I can do that.
There was a problem hiding this comment.
No worries, I will handle it!
|
Build failed |
|
@swift-ci Please smoke test Linux. |
|
Another flaky LLDB test? |
|
@xedin I think that was just paranoia on my part. We don't have any real verification on the shape of the constraints in the constraint solver. In some cases I've put debug-only code to verify assumptions that are being made, and in this case it didn't end up in debug-only code. It seems like we might be able to actually switch this around, too, so that it looks for conversion constraints where the RHS appears in a disjunction of binds. |
|
Alright, I think we should also make it assertion when we create a disjunction, check that all of the choices have the same left-hand side, otherwise it should be different disjunctions... |
|
Yes that certainly seems reasonable for a disjunction of bind constraints in particular. |
No description provided.