Introduce @_backDeploy attribute #41271
Conversation
- Parse the attribute and diagnose parsing issues - Serialize the attribute
…` into a separate function and improve diagnostics.
|
@swift-ci please smoke test |
| // Ensure @_backDeploy attributes and function bodies are present after | ||
| // deserializing .swiftmodule files. | ||
| // RUN: %target-swift-frontend -emit-module -o /dev/null -merge-modules %t/Test.swiftmodule -disable-objc-attr-requires-foundation-module -emit-module-interface-path %t/TestFromModule.swiftinterface -module-name Test | ||
| // RUN: %FileCheck %s --check-prefix FROMMODULE --check-prefix CHECK < %t/TestFromModule.swiftinterface |
There was a problem hiding this comment.
Just so you know, merge-module isn't used in practice anymore (except on Windows) and we are planning on removing the merge-modules logic completely when the new driver is the default everywhere. I'm not sure what is the best alternative here, I guess when the attribute is fully implemented just building a client will be enough to test if it can be properly deserialized. Leaving this as is is fine for now.
There was a problem hiding this comment.
That's helpful clarification, thanks! I was following the example set by test/ModuleInterface/inlinable-function.swift. I can remove this step when there is a test that exercises using a back deployed decl from a library, as you say.
| // Ensure @_backDeploy attributes and function bodies are printed in | ||
| // swiftinterface files. | ||
| // RUN: %target-swift-frontend -emit-module -o %t/Test.swiftmodule -emit-module-interface-path %t/Test.swiftinterface -module-name Test %s | ||
| // RUN: %FileCheck %s --check-prefix FROMSOURCE --check-prefix CHECK < %t/Test.swiftinterface |
There was a problem hiding this comment.
I like to verify the swiftinterfaces that the compiler emits whenever possible. Just for this it might be worth invoking the driver instead of the frontend and passing -verify-emitted-module-interface.
| @@ -713,6 +713,12 @@ DECL_ATTR(exclusivity, Exclusivity, | |||
| ABIStableToAdd | ABIStableToRemove | APIStableToAdd | APIStableToRemove, | |||
| 128) | |||
|
|
|||
| DECL_ATTR(_backDeploy, BackDeploy, | |||
| OnAbstractFunction | | |||
| AllowMultipleAttributes | LongAttribute | UserInaccessible | | |||
There was a problem hiding this comment.
Do we want AllowMultipleAttributes or is one attribute listing many platforms enough?
There was a problem hiding this comment.
I chose to allow multiple attributes because currently this attribute explodes into one attribute per platform and version tuple when printed into the interface (following @_originallyDefinedIn as an archetype).
| "* as platform name has no effect in '%0' attribute", (StringRef)) | ||
|
|
||
| ERROR(attr_availability_need_platform_version,none, | ||
| "expected at least one platform version in in '%0' attribute", (StringRef)) |
|
|
||
| // FIXME(backDeploy): Computed properties should be supported | ||
| @available(macOS 12.0, *) | ||
| @_backDeploy(macOS 11.0) // expected-error {{'@_backDeploy' attribute cannot be applied to this declaration}} |
There was a problem hiding this comment.
It would be nice to explain why a bit. Sometimes we pass down the kind of decl cannot be applied to a computer property and it gives just enough context for a google search to return useful information.
There was a problem hiding this comment.
There are facilities to print a textual version of the decl kind in the diagnostics already, I can't find it at the moment.
There was a problem hiding this comment.
Good idea, I'll dig around to see if I can find that.
There was a problem hiding this comment.
Looked into this but it seems like improving the diagnostic would improve it for every attribute which would affect a ton of tests. I'll file a follow up radar to investigate doing this systemically.
|
|
||
| @available(macOS 12.0, *) | ||
| @_backDeploy(macOS 11.0) | ||
| public func backDeployedTopLevelFunc() {} |
There was a problem hiding this comment.
Great work!
Semantically, does this mean we emit the code into client when deploying to macOS 11.0 (inclusively) and before? I wonder what happens if the deployment target is somewhere between 11.0 and 12.0, say 11.4?
There was a problem hiding this comment.
Never mind, it seems 11.0 is the lower bound of backward deployment, which makes a lot of sense.
There was a problem hiding this comment.
That's right, it's intended to be a lower bound for inlinability, which is going to help us diagnose when library authors use potentially unavailable APIs from the bodies of back deployed functions.
|
@swift-ci please smoke test |
…iftinterface containing a back-deployed decl. Also leave a FIXME to stop using -merge-modules in the future and fix a diagnostic typo.
tshortli
force-pushed
the
introduce-backdeploy-attr
branch
from
a9eed36
to
2de28df
Compare
|
@swift-ci please smoke test |
|
🎉 |
Introduces the
@_backDeployattribute for use on function declarations. The purpose and desired semantics of this attribute are documented in UnderscoredAttributes.md.This PR implements parsing and serialization and upcoming PRs will implement the semantics as well as additional diagnostics. Aspects of the parsing have been designed to be reused by
@_originallyDefinedInin the future.Resolves rdar://88453905