-
Notifications
You must be signed in to change notification settings - Fork 10.6k
[Compile Time Constant Extraction] Extract result builder annotations #63091
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
[Compile Time Constant Extraction] Extract result builder annotations #63091
Conversation
@swift-ci test |
lib/ConstExtract/ConstExtract.cpp
Outdated
for (auto Member : Decl->getMembers()) { | ||
if (auto *VD = dyn_cast<swift::VarDecl>(Member)) { | ||
if (auto *attr = VD->getAttachedResultBuilder()) { | ||
if (VD->getName() == VarDecl->getName()) { |
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.
@artemcm - Is checking equality by identifier the right thing to do here? It properly filtered in my testing.
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.
Yeah, those are uniqued by the ASTContext, so this should be safe. 👍🏼
lib/ConstExtract/ConstExtract.cpp
Outdated
JSON.attribute("type", | ||
toFullyQualifiedTypeNameString(attr->getType())); | ||
writeFileInformation(JSON, VD->getASTContext(), attr); | ||
}); |
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 likely want to break out of this loop when we see a result builder.
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.
Should be sufficient to break once we've seen a var decl with the name to the one we're looking for, result builder or no result builder, 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 think so.
// CHECK-NEXT: "file": "{{.*}}test{{/|\\\\}}ConstExtraction{{/|\\\\}}ExtractResultBuilders.swift", | ||
// CHECK-NEXT: "line": 24 |
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.
For inferred conformances, this is where the protocol is defined. Do we maybe not need file/line information since we have the var file/line above?
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.
My intuition is that we don't need file/line for the attribute at all, those are largely mostly relevant for the property decl itself. But perhaps I'm missing some use-case?
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.
Agreed. Removing.
bf3f0b6
to
d4c5ca6
Compare
@swift-ci test |
lib/ConstExtract/ConstExtract.cpp
Outdated
for (auto Member : Decl->getMembers()) { | ||
if (auto *VD = dyn_cast<swift::VarDecl>(Member)) { | ||
if (auto *attr = VD->getAttachedResultBuilder()) { | ||
if (VD->getName() == VarDecl->getName()) { |
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.
Nit: getAttachedResultBuilder
fires off a request, so I think checking if the VD->getName()
is the one we're looking for first could save some unnecessary work for all the other properties.
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.
Makes sense to me. I'll change that order and add the early return.
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.
A couple of small comments, but otherwise this is looking good!
d4c5ca6
to
6bccd9f
Compare
@swift-ci test |
This PR extracts result builder type information for variables. This checks for both implicit and explicit (ie. protocol conformance) result builder annotations.