Sema: Clean up BindingSet::addBinding()#87018
Sema: Clean up BindingSet::addBinding()#87018slavapestov wants to merge 12 commits intoswiftlang:mainfrom
Conversation
slavapestov
force-pushed
the
add-binding-cleanup
branch
from
ee71dcc to
a8fb38d
Compare
|
@swift-ci Please smoke test |
|
@swift-ci Please test source compatibility |
…zableConformance Instead of SK_Unavailable. This is so that test/Constraints/rdar139812024.swift will continue to pass with an upcoming change. The problem was that in this test, an unsound optimization meant that we would find a worse solution, by not considering the better solution, according to our scoring rules. The worse solution involved an unavailable conformance to Sendable, while the better one used a missing synthesized one. Note that this is only an issue in Swift 5 mode, where unavailable Sendable conformance is a warning. Both are an error in Swift 6 mode, so it doesn't matter which solution we pick there anyway.
We would sometimes replace a binding whose type contains type variables with one that does not. This is unsound in the general case, but we need to to avoid performance problems and ambiguous solutions in certain pathological cases. I found two test cases in the test suite that exercised this specific behavior. Extract them into their own file, and generalize at the test case. At the same time, tweak the hack to narrow it down a bit.
slavapestov
force-pushed
the
add-binding-cleanup
branch
from
a8fb38d to
592a7da
Compare
xedin
left a comment
xedin
left a comment
There was a problem hiding this comment.
Looks good! Please address my comment about referencedTypeVars before landing though.
| for (const auto &binding : joined) | ||
| (void)Bindings.insert(binding); | ||
| // Insert the possibly updated binding. | ||
| for (auto *adjacentVar : referencedTypeVars) |
There was a problem hiding this comment.
Don't we need to check whether subsuming binding was modified and if so re-fetch referencedTypeVars? I don't think we can safely assume that updated binding has the same referenced variables...
There was a problem hiding this comment.
It might be better to move this into for (const auto &binding : joined) loop since that's where the binding gets used.
There was a problem hiding this comment.
Yeah, I'll add a FIXME. Today, we only join bindings that don't contain type variables (see isViableForJoin()) so nothing can go wrong, and Type::join() itself is really incomplete. I wanted to refactor it and move it into Subtype.cpp at some point.
There was a problem hiding this comment.
It's fine to add a FIXME but it the problem is that if something changes in subsumesBinding and the author is not careful enough they might miss that the caller should now re-request the referenced vars...
| /// - false: discard new binding | ||
| static std::optional<bool> subsumeBinding(PotentialBinding &binding, | ||
| const PotentialBinding &existing, | ||
| bool isClosureParameterType) { |
There was a problem hiding this comment.
Could this method instead of bool return a binding? It would definitely make the call-site easier to follow and possibly less surprising to users if this didn't have side-effects. See my comment below about referencedTypeVars, that's pretty had to spot.
There was a problem hiding this comment.
Yeah, what it really returns is an optional<PotentialBinding> to possibly add, together with a bool indicating if the old binding should be removed or not. I can clean this up.
2 participants
BindingSet::addBinding() has various heuristics to try to drop either the existing binding or the new binding, and it even attempts to compute the "join" if two supertype bindings do not contain type variables.
This would make up to three passes over the existing bindings, and now it only makes one. The actual rules around which bindings subsume others are pretty subtle, and I've extracted them into a new
subsumesBinding()method. I've mostly left the rules unchanged, but it should now be easier to clean it up further and add more advanced heuristics. Also now that adding a binding has been untangled from merging it with an existing binding, it should also be easier to have this coalescing happen later or incrementally, if we decide to do so.