New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Generic Signature Builder] Improve the handling of typealiases in protocols. #7389
Conversation
57cdbdc
to
f88fce0
Compare
lib/AST/ArchetypeBuilder.cpp
Outdated
SubstitutionMap subMap; | ||
subMap.addSubstitution(self, concreteSelf); | ||
subMap.addConformance(self, ProtocolConformanceRef(proto)); | ||
auto typeHere = typeInProtocol.subst(subMap); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@slavapestov is going to be grouchy at building the substitution map this way. Can you just provide the trivial lookup functions that handle "self -> concreteSelf" and the construction of the abstract conformance? There's no need to build a full substitution map here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I didn't realize I could subst with a closure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even better would be to use the new SubstitutionMap::getProtocolSubstitutions() function. It handles exactly this case where you're replacing Self and you have a conformance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! I noticed that and have it in my local branch already... currently trying to make the rest of the patch work properly/clean.
lib/AST/ArchetypeBuilder.cpp
Outdated
// (e.g. protocol P { associatedtype A; typealias B = A }), so we need to | ||
// resolve that. | ||
auto concrete = pa->getConcreteType(); | ||
auto rhsPA = resolveArchetype(concrete, basePA); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The assertion above is the reason we don't need to recursively call resolveArchetypeOrTypeThroughTypeAlias
here, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes; I'll add a comment.
lib/AST/ArchetypeBuilder.cpp
Outdated
return type; | ||
|
||
pa = pa->getRepresentative(); | ||
if (!pa->getParent() || !pa->getTypeAliasDecl()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm... it occurs to me that there might be direct callers to addSameTypeRequirementBetweenArchetypes
that unknowingly end up with a typealias
for one of the archetypes. Should this logic be sunk down into that function instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the only remaining direct callers are either addSameTypeRequirement
, or are in places where equating to the type alias PA directly seems correct, specifically: potential archetype typo correction, equating any new PAs inside getNestedType
itself, and recursively to merge the PAs.
However, having it inside addSameTypeRequirementBetweenArchetypes
does seem like it might be nicer.
f88fce0
to
4927c77
Compare
I've updated this in a fairly invasive way. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is way cleaner. Looks great!
@swift-ci please smoke test |
1 similar comment
@swift-ci please smoke test |
Looks like some compiler crashers regressed due to the |
lib/AST/GenericSignatureBuilder.cpp
Outdated
PotentialArchetype *getPotentialArchetype() const { | ||
return paOrT.dyn_cast<PotentialArchetype *>(); | ||
} | ||
bool isUnresolvedTypeAlias() const { return unresolvedTypeAlias; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note to self: this needs renaming.
lib/AST/GenericSignatureBuilder.cpp
Outdated
// (e.g. protocol P { associatedtype A; typealias B = A }), so we need to | ||
// resolve that. However, the archetype should always be resolved far enough | ||
// upon creation to not be another type alias (verified by the assertion | ||
// below), and hence this function doesn't need to be recursive.. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note to self: this comment is outdated.
271798d
to
a499863
Compare
Another update, removing the reimplementation of #7563, among other things. There's an unfortunate FIXME in |
a499863
to
a82a1b2
Compare
…ments. This adds the concept of an "unresolved type" and a "resolved type", where the latter involves stripping away any typealias archetype layers from the former, to get to the "true" potential archetype or concrete type that something refers to.
…nd different types. At the moment, these are only done on demand, when the typealias is actually used somewhere, because there's currently a recursive validation loop that stops doing it more eagerly from working in many cases.
…ses in protocols. Generic typealiases are entirely broken as archetypes, but some things mostly worked with them before introducing ResolvedType, so let's restore that behaviour.
a82a1b2
to
d671a65
Compare
@swift-ci Please test |
Build failed |
Build failed |
I recommend smoke tests :) |
@swift-ci Please smoke test and merge |
@swift-ci please smoke test Linux |
Previously, the following example
would result in the T.B.X == Int requirement being dropped, because it adopted the (Protocol) RequirementSource of the T.B.X == T.B.A same-type requirement implied by the typealias declaration.