-
Notifications
You must be signed in to change notification settings - Fork 10.5k
[Executors] Ensure we treat DA older than 5.9 always as DefaultActor #64800
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
Changes from all commits
f3452b1
4173699
a6ddf44
3b4ac6c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -182,6 +182,27 @@ bool IsDefaultActorRequest::evaluate( | |
if (!classDecl->isActor()) | ||
return false; | ||
|
||
// Distributed actors were not able to have custom executors until Swift 5.9, | ||
// so in order to avoid wrongly treating a resilient distributed actor from another | ||
// module as not-default we need to handle this case explicitly. | ||
if (classDecl->isDistributedActor()) { | ||
ASTContext &ctx = classDecl->getASTContext(); | ||
auto customExecutorAvailability = | ||
ctx.getConcurrencyDistributedActorWithCustomExecutorAvailability(); | ||
|
||
auto actorAvailability = TypeChecker::overApproximateAvailabilityAtLocation( | ||
classDecl->getStartLoc(), | ||
classDecl); | ||
|
||
if (!actorAvailability.isContainedIn(customExecutorAvailability)) { | ||
// Any 'distributed actor' declared with availability lower than the | ||
// introduction of custom executors for distributed actors, must be treated as default actor, | ||
// even if it were to declared the unowned executor property, as older compilers | ||
// do not have the the logic to handle that case. | ||
return true; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is the core of the change; we ensure we treat distributed actors which "could not have been non-default" always as default. |
||
} | ||
} | ||
|
||
// If the class is resilient from the perspective of the module | ||
// module, it's not a default actor. | ||
if (classDecl->isForeign() || classDecl->isResilient(M, expansion)) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,121 @@ | ||
// RUN: %empty-directory(%t) | ||
// RUN: %target-swift-frontend-emit-module -emit-module-path %t/FakeDistributedActorSystems.swiftmodule -module-name FakeDistributedActorSystems -disable-availability-checking %S/../Inputs/FakeDistributedActorSystems.swift | ||
// RUN: %target-build-swift -Xfrontend -disable-availability-checking -parse-as-library -I %t %s %S/../Inputs/FakeDistributedActorSystems.swift -o %t/a.out | ||
// RUN: %target-codesign %t/a.out | ||
// RUN: %target-run %t/a.out | ||
|
||
// REQUIRES: executable_test | ||
// REQUIRES: concurrency | ||
// REQUIRES: distributed | ||
// REQUIRES: concurrency_runtime | ||
// UNSUPPORTED: back_deployment_runtime | ||
|
||
// UNSUPPORTED: back_deploy_concurrency | ||
// UNSUPPORTED: use_os_stdlib | ||
// UNSUPPORTED: freestanding | ||
|
||
import StdlibUnittest | ||
import Distributed | ||
import FakeDistributedActorSystems | ||
|
||
@available(SwiftStdlib 5.7, *) | ||
typealias DefaultDistributedActorSystem = LocalTestingDistributedActorSystem | ||
|
||
@available(SwiftStdlib 5.7, *) | ||
distributed actor FiveSevenActor_NothingExecutor { | ||
nonisolated var localUnownedExecutor: UnownedSerialExecutor? { | ||
print("get unowned executor") | ||
return MainActor.sharedUnownedExecutor | ||
} | ||
|
||
distributed func test(x: Int) async throws { | ||
print("executed: \(#function)") | ||
defer { | ||
print("done executed: \(#function)") | ||
} | ||
assumeOnMainActorExecutor { | ||
// ignore | ||
} | ||
} | ||
} | ||
|
||
@available(SwiftStdlib 5.9, *) | ||
distributed actor FiveNineActor_NothingExecutor { | ||
// @available(SwiftStdlib 5.9, *) // because of `localUnownedExecutor` | ||
nonisolated var localUnownedExecutor: UnownedSerialExecutor? { | ||
print("get unowned executor") | ||
return MainActor.sharedUnownedExecutor | ||
} | ||
|
||
distributed func test(x: Int) async throws { | ||
print("executed: \(#function)") | ||
defer { | ||
print("done executed: \(#function)") | ||
} | ||
assumeOnMainActorExecutor { | ||
// ignore | ||
} | ||
} | ||
} | ||
|
||
@available(SwiftStdlib 5.7, *) | ||
distributed actor FiveSevenActor_FiveNineExecutor { | ||
@available(SwiftStdlib 5.9, *) | ||
nonisolated var localUnownedExecutor: UnownedSerialExecutor? { | ||
print("get unowned executor") | ||
return MainActor.sharedUnownedExecutor | ||
} | ||
|
||
distributed func test(x: Int) async throws { | ||
print("executed: \(#function)") | ||
defer { | ||
print("done executed: \(#function)") | ||
} | ||
assumeOnMainActorExecutor { | ||
// ignore | ||
} | ||
} | ||
} | ||
|
||
@main struct Main { | ||
static func main() async { | ||
if #available(SwiftStdlib 5.9, *) { | ||
let tests = TestSuite("DistributedActorExecutorAvailability") | ||
|
||
let system = LocalTestingDistributedActorSystem() | ||
|
||
#if os(macOS) || os(iOS) || os(watchOS) || os(tvOS) | ||
tests.test("5.7 actor, no availability executor property => no custom executor") { | ||
expectCrashLater(withMessage: "Fatal error: Incorrect actor executor assumption; Expected 'MainActor' executor.") | ||
try! await FiveSevenActor_NothingExecutor(actorSystem: system).test(x: 42) | ||
} | ||
|
||
tests.test("5.9 actor, no availability executor property => custom executor") { | ||
try! await FiveNineActor_NothingExecutor(actorSystem: system).test(x: 42) | ||
} | ||
|
||
tests.test("5.7 actor, 5.9 executor property => no custom executor") { | ||
expectCrashLater(withMessage: "Fatal error: Incorrect actor executor assumption; Expected 'MainActor' executor.") | ||
try! await FiveSevenActor_FiveNineExecutor(actorSystem: system).test(x: 42) | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This case I was unsure about, but I think this way to handle it makes sense; the entire actor has to be 5.9 to be treated like being able to have a custom executor, not just the property. |
||
#else | ||
// On non-apple platforms the SDK comes with the toolchains, | ||
// so the feature works because we're executing in a 5.9 context already, | ||
// which otherwise could not have been compiled | ||
tests.test("non apple platform: 5.7 actor, no availability executor property => no custom executor") { | ||
try! await FiveSevenActor_NothingExecutor(actorSystem: system).test(x: 42) | ||
} | ||
|
||
tests.test("non apple platform: 5.9 actor, no availability executor property => custom executor") { | ||
try! await FiveNineActor_NothingExecutor(actorSystem: system).test(x: 42) | ||
} | ||
|
||
tests.test("non apple platform: 5.7 actor, 5.9 executor property => no custom executor") { | ||
try! await FiveSevenActor_FiveNineExecutor(actorSystem: system).test(x: 42) | ||
} | ||
#endif | ||
|
||
await runAllTestsAsync() | ||
} | ||
} | ||
} |
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.
Minor: run
git-clang-format
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.
Thanks, I'll follow up separately if that's fine? That'd just be a linebreak here AFAICS.
// The problem with clang-format is that it is not being applied consistently in this project, so applying it adds all kinds of noise and "this commit does not look like the rest of the file or method at all now..."
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.
Follow up is fine with me! I find the
clang-format
output is pretty consistent with the style of most of the files I have worked in in the project and it is documented in https://github.com/apple/swift/blob/main/docs/HowToGuides/FirstPullRequest.md as the primary way of enforcing the project code style. In particular, long lines stick out a lot - I think the majority of the codebase adheres to the 80 char limitUh oh!
There was an error while loading. Please reload this page.
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'll follow up about my specific changes, but the reality is that files are not well formatted:
if running clang-format on files this PR touched, out of that my changes would be...
but yeah, I'll add a git hook to git-clang-format my commits and I'll do the promised follow up now as well: #64886