RequirementMachine: Fixes some bugs with superclass requirements #38743
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
slavapestov
force-pushed
the
requirement-machine-superclass-fixes
branch
from
91e813d to
c6010aa
Compare
…types of superclass bounds
If you have
protocol P {
associatedtype T
}
class C<T> : P {}
Then substituting the type parameter T.T from the generic signature
<T where T : P> into the generic signature <T, U where T : C<U>>
is the identity operation, and also returns T.T, because subst()
isn't smart enough to replace T.T with U.
So getCanonicalTypeInContext() has to do the concrete conformance
lookup here, just like it does for the analogous situation where
you have a concrete type requirement.
This could be solved in a more principled way by better book-keeping
elsewhere, but the GSB supported the old behavior, so we can
simulate it easily enough in the RequirementMachine also.
… and superclass bounds
The property map stores the concrete type or superclass bound for all
terms whose suffix is equal to some key, so eg if you have
protocol P {
associatedtype T where T == U?
associatedtype U
}
Then you have this rewrite system
[P].T => [P:T]
[P].U => [P:U]
[P:T].[concrete: Optional<τ_0_0> with <[P:U]>] => [P:T]
Which creates this property map
[P:T] => { [concrete: Optional<τ_0_0> with <[P:U]>] }
Now if I start with a generic signature like
<T, U where U : P>
This produces the following rewrite system:
τ_0_1.[U] => τ_0_1
τ_0_1.T => τ_0_1.[P:T]
τ_0_1.U => τ_0_1.[P:U]
Consider the computation of the canonical type of τ_0_1.T. This term
reduces to τ_0_1.[P:T]. The suffix [P:T] has a concrete type symbol in
the property map, [concrete: Optional<τ_0_0> with <[P:U]>].
However it would not be correct to canonicalize τ_0_1.[P:T] to
Optional<τ_0_0>.subst { τ_0_0 => getTypeForTerm([P:T]) }; this
produces the type Optional<τ_0_0.T>, and not Optional<τ_0_1.T> as
expected.
The reason is that the substitution τ_0_0 => getTypeForTerm([P:T])
is "relative" to the protocol Self type of P, since domain([P:T]) = {P}.
Indeed, the right solution here is to note that τ_0_1.[P:T] has a suffix
equal to the key of the property map entry, [P:T]. If we strip off the
suffix, we get τ_0_1. If we then prepend τ_0_1 to the substitution term,
we get the term τ_0_1.[P:U], whose canonical type is τ_0_1.U.
Now, applying the substitution τ_0_0 => τ_0_1.U to Optional<τ_0_0>
produces the desired result, Optional<τ_0_1.U>.
Note that this is the same "prepend a prefix to each substitution of
a concrete type symbol" operation that is performed in checkForOverlap()
and PropertyBag::copyPropertiesFrom(), however we can simplify things
slightly by open-coding it instead of calling the utility method
prependPrefixToConcreteSubstitutions(), since the latter creates a
new symbol, which we don't actually need.
slavapestov
force-pushed
the
requirement-machine-superclass-fixes
branch
from
c6010aa to
64c5d65
Compare
…-C reference counting I'm about to fix the same bug in the RequirementMachine, and to avoid spurious cross-checking failures in -requirement-machine=verify mode, just fix this in the GSB as well.
… uses Objective-C reference counting Also, introduce the layout requirement implied by a superclass requirement when lowering requirements, instead of when building the property map. Right now this is functionally equivalent, but if we ever start to test properties by checking for joinability of T with T.[p] instead of looking at the property map entry of T, this new formulation is the right one.
…ismatchTypeParameters()
I'm going to use this to unify superclass types as well.
Just as with concrete types, if we find that the same suffix has two different superclass symbols in the property map, we need to introduce same-type requirements between their generic parameters. The added wrinkle is that the classes might be different; in this case, one must be a superclass of the other, and we repeatedly substitute the generic arguments until we get the generic arguments of the superclass before we unify.
slavapestov
force-pushed
the
requirement-machine-superclass-fixes
branch
from
64c5d65 to
4f2aa5f
Compare
|
@swift-ci Please smoke test |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
While testing the RequirementMachine on the source compatibility suite and some other projects, I finally found an example where we need to unify superclass requirements and pick the most derived class out of the two. This was on my todo list for a while, and it turned out to be pretty straightforward to implement by refactoring the existing code for doing this with concrete type requirements.
The new implementation is more advanced than what the GSB did, because it also unifies type constructor arguments in the case where the superclasses are generic as well. For example,
The RequirementMachine will derive that
T.X : Derivednow just like the GSB, but furthermore,T.Y == Int, which the GSB was unable to prove.This PR has a couple of more minor fixes for other issues found via testing as well.