-
Notifications
You must be signed in to change notification settings - Fork 10.6k
Lazy accessor synthesis for storage in secondary files #26461
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
Lazy accessor synthesis for storage in secondary files #26461
Conversation
@swift-ci Please test |
@swift-ci Please test source compatibility |
@swift-ci Please test compiler performance |
Build failed |
Build failed |
include/swift/AST/Decl.h
Outdated
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.
"Return an accessor that's known to be part of the set..."
Technically, it's possible to distinguish between opaque accessors, which are guaranteed-to-exist ABI-stable implementations of the core access operations, and accessors that might be part of the (non-stable or frozen) ABI. For example, we might want to do funny things with didSet
in the future. Not sure if this is worth spelling out in the comment.
lib/AST/ASTVerifier.cpp
Outdated
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.
These changes don't seem right. The AST verifier is unlike most clients in that it should be doing verification of accessors if they exist for any reason, and I don't think we want to be embedding assumptions in it about whether specific accessors only exist if they're parsed.
Presumably we visit all the accessors that exist as part of the walk? (That seems right for the AST verifier even if it's not normally a good operation to provide.) Maybe these checks can be moved to when we visit the accessor.
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 can change this back. I was planning on keeping something like getAccessor() around specifically for the verifier (and maybe even friend the verifier so people don’t use it on accident).
Also, change visitOpaqueAccessors() to call getOpaqueAccessor() instead of asserting if the expected accessor does not exist.
When checking availability of referenced accessors, we may not have synthesized the accessors yet. This meant that we didn't diagnose references to internal(set) properties from inlinable contexts if the property was defined in another file.
Make sure we test checkObjCWitnessSelector() in the multi-file case. I made some changes that regressed a source compatibility project but the regression was not caught by our test suite, so make sure we have a test for this now.
We can lazily synthesize accessor witnesses from SILGen now.
Previously we would synthesize accessors for any referenced storage, as well as any storage members of classes and protocols. Now that synthesis is sufficiently lazy this is no longer needed. The only remaining work done in finalizeDecl() is adding certain implicit members to ClassDecls; no other declaration kind ends up in DeclsToFinalize anymore.
This is a false positive, since the '_read' accessor is synthesized on demand and emitted with shared linkage. See <rdar://problem/53776566>.
63f0482
to
24b20a3
Compare
@swift-ci Please smoke test |
1 similar comment
@swift-ci Please smoke test |
@swift-ci Please smoke test Linux |
Lots of counters dropped; very nice |
When the compiler is asked to produce code for 1 file out of N in the project, but it's blocked by an error in a different file, it still needs to print a diagnostic; it's unreasonable for it to just fail silently. |
We now have three methods on AbstractStorageDecl that will eventually replace
getAccessor()
:getParsedAccessor()
-- returns an accessor explicitly written in source; otherwise always returns nullptr.getOpaqueAccessor()
-- if the accessor kind is part of the ABI of the storage, or if it was parsed, returns the accessor. Returns nullptr if the accessor was not parsed and is not part of the opaque ABI set. The accessor is synthesized if necessary. It's possible for an accessor to be parsed, but not part of the ABI; an example isAccessorKind::WillSet
orAccessorKind::DidSet
. For convenience this method returns an accessor in this case also. An example of an accessor that is neither parsed or part of the ABI isAccessorKind::Read
on a computed property that only has a getter. In this case, this method will return nullptr.getSynthesizedAccessor()
-- returns an accessor, always synthesizing one if necessary. This should only be used when, eg, emitting protocol witnesses, which can sometimes require accessors not part of the storage's ABI (for example, an@objc dynamic
property does not usually have anAccessorKind::Modify
, but if it witnesses a mutable protocol requirement, we synthesize one). Note that ifgetSynthesizedAccessor()
returns an accessor not part of the opaque ABI accessor set, the generated accessor will be emitted with shared linkage.Using the above three operations, we can now trigger lazy accessor synthesis from various stages of the pipeline, eliminating some logic found in Sema's
finalizeDecl()
for forcing accessor synthesis for declarations in secondary files.We still force synthesis of accessors in primary files, and in a couple of other random places. These need a little bit more refactoring to eliminate, because we still call
getAccessor()
,getAllAccessors()
andhasAnyAccessors()
on declarations in primary files.As for
finalizeDecl()
, it's still used to add some implicit members to ClassDecls, and not much else. TheTypeChecker::DeclsToFinalize
list now only ever contains ClassDecls. This is also going away soon, in favor of a request to get the "vtable members" of a class.