[CS] Adjust applied overload simplification #30716
Conversation
Currently `simplifyAppliedOverloads` depends on the order in which constraints are simplified, specifically that a lookup constraint for a function gets simplified before the applicable function constraint. This happens to work out just fine today with the order in which we re-activate constraints, but I'm planning on changing that order. This commit changes the logic such that it it's no longer affected by the order in which constraints are simplified. We'll now run it when either an applicable function constraint is added, or a new bind overload disjunction is added. This also means we no longer need to run it potentially multiple times when simplifying the applicable fn.
|
@swift-ci please test |
|
@swift-ci please test compiler performance |
There was a problem hiding this comment.
My biggest concern is that we are getting even more expensive calls to gatherConstraints involved in simplication. Maybe a better approach would be to remove disjunction constraint and associate all of the choices direct with corresponding type variable as a set of possible types. It might be a bigger refactoring but it should allow us to eliminate all prominent uses of gatherConstraints which should speed up solving.
| addDisjunctionConstraint(choices, locator, ForgetChoice); | ||
| auto *disjunction = | ||
| Constraint::createDisjunction(*this, choices, locator, ForgetChoice); | ||
| addUnsolvedConstraint(disjunction); |
There was a problem hiding this comment.
This seems weird because we add disjunction just to retire it right away, shouldn't it be check and add instead?
There was a problem hiding this comment.
@xedin Unfortunately filterDisjunction currently requires it to be in the inactive list.
@xedin Could you elaborate on what you mean by this? I definitely agree that it would be nice to remove a lot of the calls to I'd be more than happy to work on a follow-up to help remove more calls to |
|
I think re-activating all of the constraints is a bit backwards if we are trying to avoid traversing constraint graph to find constraints related to newly bound type variable (I haven't actually looked at the PR so I'm just assuming that's what it does). Is it possible to optimize constraint graph representation so equivalence classes are not as expensive to traverse?
I think we should move towards a solver where bindings are computed incrementally as new constraints are being added/removed and each type variable (or constraint system) keeps track of its current "possible types" set. For disjunctions we already allocate a new type variable which then gets bound to a particular choice, if we where to keep disjunction choices are "possible types" that would unify |
@xedin The idea behind re-activating all constraints was that if we were to fix Though having thought about it some more, I'm actually not sure why we need to visit constraints along a type variable's adjacencies, do you recall why we do this? The test suite appears to more or less pass if we just visit fixed bindings (which I believe should be quite a bit cheaper to traverse). Just to be clear, my primary motive here is correctness, we currently miscompile programs like this: func foo(_ x: Int) {
(x ?? 0) as String
}Because we don't transitively visit fixed bindings when we bind a type variable, and then drop the coercion constraint. If possible, I'd really like to get a fix for this into 5.3 :)
Hmm, I suppose we could try unifying the constraints and fixed bindings we need to visit on the representative, though I'm not sure how much of a win that would be as it would mean additional work when adding/removing constraints and rolling back a scope.
That would be great! Though for |
func foo(_ x: Int) {
(x ?? 0) as String
}This example looks like a problem with binding inference, of overload of I think the intention of that code was to avoid inferring incorrect bindings but unwrapping an optional there leads to us trying to binding two type variables together, which should actually be avoided. I think it should be possible to extend this logic to discard bindings or at least mark them as WDYT? |
|
@swift-ci please test |
|
@swift-ci please test Windows |
|
Talked with Pavel offline, we're going to merge this so the fix for |
Currently
simplifyAppliedOverloadsdepends on the order in which constraints are simplified, specifically that a lookup constraint for a function gets simplified before the applicable function constraint. This happens to work out just fine today with the order in which we re-activate constraints, but I'm planning on changing that order.This PR changes the logic such that it's no longer affected by the order in which constraints are simplified. We'll now run it when either an applicable function constraint is added, or a new bind overload disjunction is added. This also means we no longer need to run it potentially multiple times when simplifying an applicable fn.