ModuleInterface: Wrap synthesized extensions in swiftinterfaces with feature guards #42276
Conversation
| [&] { printBody(); }); | ||
| } else { | ||
| printBody(); | ||
| } |
There was a problem hiding this comment.
Okay, so @slavapestov removed this logic in a recent patch because (1) he realized that it's checking based on the features used by the extended declaration and not the extension itself, so it's a bit broken, and (2) it wasn't tested in any way. If we're going to put this back, we should really fix the first part. It looks like extensions are considered to use the $GlobalActors feature if they're extending a type that's a global actor, so maybe you just need to pass in the extension here instead of nominal.
There was a problem hiding this comment.
There's no ExtensionDecl here though; these extensions are "synthesized", meaning they only exist in the imagination of the ASTPrinter.
There was a problem hiding this comment.
I tried restoring the logic removed with #42238 and that doesn't solve this bug (because it checked whether the protocol requires features, rather than the extended type). Given what Slava points out above (we don't have an ExtensionDecl) is there something better I can do here? Do I understand correctly that what I have currently is correct but theoretically insufficient for fixing some other bugs where the extension, rather than the extended type, depends on a conditionally available feature?
There was a problem hiding this comment.
Can we synthesize a real ExtensionDecl, or is that difficult in practice?
There was a problem hiding this comment.
Do I understand correctly that what I have currently is correct but theoretically insufficient for fixing some other bugs where the extension, rather than the extended type, depends on a conditionally available feature?
At the very least, I think it probably needs to consider the availability of both the protocol and the extended type.
It will also redundantly emit the extension under different conditions if the nominal type is emitted redundantly that way, which is unnecessary but I guess not actually a problem.
There was a problem hiding this comment.
Slava can speak to the request-evaluator parts better than I can.
There was a problem hiding this comment.
The request evaluator parts look fine, but I'm wondering if you can remove the printBody lambda with it's bespoke printing logic and just feed the synthesized ExtensionDecl to the ASTPrinter itself?
There was a problem hiding this comment.
I guess you'll need to attach the correct availability and SPI attributes to the synthesized extension also, but it would still be better to not duplicate the printing logic here.
There was a problem hiding this comment.
It does seem feasible to let the ASTPrinter be responsible for the printing. I am having to handle a number of small details with care, though, to ensure that the swiftinterface contents remain unchanged after taking this approach. (I don't want this under the hood change to cause a lot of churn in interfaces for no good reason since folks diff the interfaces to understand what is being added and removed from build to build).
There was a problem hiding this comment.
Alright, I completely revamped this to use the ASTPrinter to emit the extension declaration. PTAL @slavapestov
tshortli
force-pushed
the
global-actor-broken-swift-interface
branch
2 times, most recently
from
8cb9535 to
3878df8
Compare
tshortli
force-pushed
the
global-actor-broken-swift-interface
branch
3 times, most recently
from
3f3c37b to
7694fa1
Compare
…sure to guard them with required features if applicable. Not doing so can result in broken interfaces that do not typecheck because, for instance, a conformance can refer to a nominal type that is only declared when certain features are enabled. Also, fix a typo where `#elsif` was printed into interfaces instead of `#elseif`. Resolves rdar://91509673
tshortli
force-pushed
the
global-actor-broken-swift-interface
branch
from
7694fa1 to
f27005b
Compare
|
@swift-ci please test |
…aces Erroneous declaration attributes were sometimes being printed in the private swiftinterfaces of modules because the changes from swiftlang#42276 were effectively corrupting the attribute list for any decl with sythesized conformances (e.g. `Equatable`, `Hashable`). It is necessary to clone the attributes before adding them to the synthesized conformance extension decls. Resolves rdar://94009296
…aces Erroneous declaration attributes were sometimes being printed in the private swiftinterfaces of modules because the changes from swiftlang#42276 were effectively corrupting the attribute list for any decl with sythesized conformances (e.g. `Equatable`, `Hashable`). It is necessary to clone the attributes before adding them to the synthesized conformance extension decls. Resolves rdar://94009296
…aces Erroneous declaration attributes were sometimes being printed in the private swiftinterfaces of modules because the changes from swiftlang#42276 were effectively corrupting the attribute list for any decl with sythesized conformances (e.g. `Equatable`, `Hashable`). It is necessary to clone the attributes before adding them to the synthesized conformance extension decls. Resolves rdar://94009296
…aces Erroneous declaration attributes were sometimes being printed in the private swiftinterfaces of modules because the changes from swiftlang#42276 were effectively corrupting the attribute list for any decl with sythesized conformances (e.g. `Equatable`, `Hashable`). It is necessary to clone the attributes before adding them to the synthesized conformance extension decls. Resolves rdar://94009296
When printing synthesized extensions, we need to be sure to guard them with required features if applicable. Not doing so can result in broken interfaces that do not typecheck because, for instance, a conformance can refer to a nominal type that is only declared when certain features are enabled.
Also, fix a typo where
#elsifwas printed into interfaces instead of#elseif.Resolves rdar://91509673