-
Notifications
You must be signed in to change notification settings - Fork 10.5k
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
AST: Remodel MemberTypeRepr
to be recursive
#70467
AST: Remodel MemberTypeRepr
to be recursive
#70467
Conversation
Excellent! |
24ae79c
to
f326a7d
Compare
This comment was marked as duplicate.
This comment was marked as duplicate.
f326a7d
to
92c238b
Compare
This comment was marked as duplicate.
This comment was marked as duplicate.
92c238b
to
9cc9fd3
Compare
This comment was marked as duplicate.
This comment was marked as duplicate.
9cc9fd3
to
01ba7c6
Compare
@swift-ci please smoke test macOS |
@swift-ci please test source compatibility release |
@swift-ci please smoke test compiler performance |
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.
Very nice!
@@ -363,55 +373,20 @@ class DeclRefTypeRepr : public TypeRepr { | |||
/// Bar<Gen> | |||
/// \endcode | |||
class IdentTypeRepr : public DeclRefTypeRepr { |
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 wonder, would it be worth folding IdentTypeRepr + SimpleIdentTypeRepr + GenericIdentTypeRepr together into a single type at this point? Are there many clients relying on the distinction here?
Merging them would provide consistency with MemberTypeRepr where generic args don't have their own TypeRepr, and would help avoid clients attempting to handle GenericIdentTypeRepr and assuming that's sufficient to handle all generic args present in the TypeRepr, instead of using the API on DeclRefTypeRepr. As a bonus, it would also more closely match SwiftSyntax's IdentifierTypeSyntax.
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.
That’s the plan! With this PR, there are around 9 TypeKind
checks and dynamic casts benefitting from the type-level distinction; no API-level clients apart from visitors. I’m not entirely happy about having to call hasGenericArgList
on top of isa
, but merging SimpleIdentTypeRepr
and GenericIdentTypeRepr
also makes isa
faster for IdentTypeRepr
and DeclRefTypeRepr
, and it feels like it will balance out.
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.
01ba7c6
to
90754bc
Compare
@swift-ci please smoke test macOS |
@swift-ci please test compiler performance |
@swift-ci please test source compatibility debug |
@swift-ci please test source compatibility release |
@AnthonyLatsis do you mind rebasing this? We should get it landed. |
@slavapestov Gladly! |
bc4e92c
to
aa5d5c5
Compare
…epr` representation
aa5d5c5
to
8970afc
Compare
@swift-ci please test |
@swift-ci please test source compatibility release |
@slavapestov Are we ready to merge this? |
I dropped the ball, again. Sorry @AnthonyLatsis. Let's run the tests one more time. |
@swift-ci Please test |
@swift-ci Please test source compatibility |
Hakuna matata, thanks for taking a look. The UPASS is unrelated. |
A public interface that forces clients to interpret and traverse
MemberTypeRepr
nodes recursively such that every qualified type identifier is aMemberTypeRepr
pointing to its qualifier:IdentTypeRepr
nodes, and the only way to tell whether one is qualified is to look at the parent node, which is normally not a straightforward task with our AST. Knowledge of whether a type identifier is qualified is important in many of our analyses, but the absence of a general typed or other visible distinction between qualified and unqualified type identifiers is not helpful in guiding contributors to this realization.ASTWalker
clients.IdentTypeRepr
nodes are now unqualified by definition, and since each next dot-separated components is now itself aMemberTypeRepr
and a parent of the previous one (vs. all components being direct descendants of a singleMemberTypeRepr
), children may be skipped halfway through aMemberTypeRepr
..Type
and.Protocol
types asMemberTypeRepr
.