Skip to content

Commit e979dc2

Browse files
authored
Merge pull request #59880 from apple/wip-prevent-async-overloads-dist
[Distributed] Prevent overloading on async in distributed methods, for wire compat
2 parents be3249d + ae23892 commit e979dc2

File tree

4 files changed

+200
-3
lines changed

4 files changed

+200
-3
lines changed

include/swift/AST/DiagnosticsSema.def

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4642,6 +4642,10 @@ ERROR(distributed_actor_isolated_non_self_reference,none,
46424642
ERROR(distributed_actor_needs_explicit_distributed_import,none,
46434643
"'Distributed' module not imported, required for 'distributed actor'",
46444644
())
4645+
ERROR(distributed_func_cannot_overload_on_async_only,none,
4646+
"ambiguous distributed func declaration %0, cannot overload distributed methods on effect only", (DeclName))
4647+
NOTE(distributed_func_other_ambiguous_overload_here,none,
4648+
"ambiguous distributed func %0 declared here", (DeclName))
46454649
ERROR(actor_isolated_inout_state,none,
46464650
"actor-isolated %0 %1 cannot be passed 'inout' to"
46474651
"%select{| implicitly}2 'async' function call",

lib/Sema/TypeCheckDistributed.cpp

Lines changed: 56 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -678,10 +678,21 @@ void TypeChecker::checkDistributedActor(SourceFile *SF, NominalTypeDecl *nominal
678678
// If applicable, this will create the default 'init(transport:)' initializer
679679
(void)nominal->getDefaultInitializer();
680680

681+
// We check decls for ambiguity more strictly than normal nominal types,
682+
// because we want to record distributed accessors the same if function they
683+
// point at (in a remote process) is async or not, as that has no effect on
684+
// a caller from a different process, so we want to make the remoteCall target
685+
// identifiers, less fragile against such refactorings.
686+
//
687+
// To achieve this, we ban overloads on just "effects" of functions,
688+
// which are useful in local settings, but really should not be relied
689+
// on as differenciators in remote calls - the call will always be "async"
690+
// since it will go through a thunk, and then be asynchronously transferred
691+
// to the called process.
692+
llvm::SmallDenseSet<DeclName, 2> diagnosedAmbiguity;
681693

682694
for (auto member : nominal->getMembers()) {
683-
// A distributed computed property needs to have a thunk for
684-
// its getter accessor.
695+
// --- Ensure 'distributed func' all thunks
685696
if (auto *var = dyn_cast<VarDecl>(member)) {
686697
if (!var->isDistributed())
687698
continue;
@@ -692,7 +703,7 @@ void TypeChecker::checkDistributedActor(SourceFile *SF, NominalTypeDecl *nominal
692703
continue;
693704
}
694705

695-
// --- Ensure all thunks
706+
// --- Ensure 'distributed func' all thunks
696707
if (auto func = dyn_cast<AbstractFunctionDecl>(member)) {
697708
if (!func->isDistributed())
698709
continue;
@@ -705,6 +716,48 @@ void TypeChecker::checkDistributedActor(SourceFile *SF, NominalTypeDecl *nominal
705716
nominal->getName());
706717
return;
707718
}
719+
720+
// Check there's no async/no-async overloads, since those are more
721+
// fragile in distribution than we'd want distributed calls to be.
722+
// A remote call is always 'async throws', and we can always record
723+
// an async throws "accessor" (see AccessibleFunction.cpp) as such.
724+
// This means, if we allowed async/no-async overloads of functions,
725+
// we'd have to store the precise "it was not throwing" information,
726+
// but we'll _never_ make use of such because all remote calls are
727+
// necessarily going to async to the actor in the recipient process,
728+
// and for the remote caller, they are always as-if-async.
729+
//
730+
// By banning such overloads, which may be useful in local APIs,
731+
// but too fragile in distributed APIs, we allow a remote 'v2' version
732+
// of an implementation to add or remove `async` to their implementation
733+
// without breaking calls which were made on previous 'v1' versions of
734+
// the same interface; Callers are never broken this way, and rollouts
735+
// are simpler.
736+
//
737+
// The restriction on overloads is not a problem for distributed calls,
738+
// as we don't have a vast swab of APIs which must compatibly get async
739+
// versions, as that is what the async overloading aimed to address.
740+
//
741+
// Note also, that overloading on throws is already illegal anyway.
742+
if (!diagnosedAmbiguity.contains(func->getName())) {
743+
auto candidates = nominal->lookupDirect(func->getName());
744+
if (candidates.size() > 1) {
745+
auto firstDecl = dyn_cast<AbstractFunctionDecl>(candidates.back());
746+
for (auto decl : candidates) {
747+
if (decl == firstDecl) {
748+
decl->diagnose(
749+
diag::distributed_func_cannot_overload_on_async_only,
750+
decl->getName());
751+
} else {
752+
decl->diagnose(
753+
diag::distributed_func_other_ambiguous_overload_here,
754+
decl->getName());
755+
}
756+
}
757+
758+
diagnosedAmbiguity.insert(func->getName());
759+
}
760+
}
708761
}
709762

710763
if (auto thunk = func->getDistributedThunk()) {
Lines changed: 100 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,100 @@
1+
// RUN: %empty-directory(%t)
2+
// RUN: %target-swift-frontend-emit-module -emit-module-path %t/FakeDistributedActorSystems.swiftmodule -module-name FakeDistributedActorSystems -disable-availability-checking %S/../Inputs/FakeDistributedActorSystems.swift
3+
// RUN: %target-build-swift -module-name main -Xfrontend -disable-availability-checking -j2 -parse-as-library -I %t %s %S/../Inputs/FakeDistributedActorSystems.swift -o %t/a.out
4+
// RUN: %target-run %t/a.out | %FileCheck %s --color --dump-input=always
5+
6+
// REQUIRES: executable_test
7+
// REQUIRES: concurrency
8+
// REQUIRES: distributed
9+
10+
// rdar://76038845
11+
// UNSUPPORTED: use_os_stdlib
12+
// UNSUPPORTED: back_deployment_runtime
13+
14+
// FIXME(distributed): Distributed actors currently have some issues on windows, isRemote always returns false. rdar://82593574
15+
// UNSUPPORTED: OS=windows-msvc
16+
17+
import Distributed
18+
import FakeDistributedActorSystems
19+
20+
typealias DefaultDistributedActorSystem = FakeRoundtripActorSystem
21+
22+
distributed actor Greeter {
23+
distributed func usedToHaveAsyncBytNotAnymore()
24+
/*previously had async, and remote called invoked such async method */ -> String {
25+
return #function
26+
}
27+
}
28+
29+
distributed actor Helloer {
30+
distributed func usedToBeSyncButNowIsAsync()
31+
/* The remote caller thinks this method was not async, but actually it is */
32+
async -> String {
33+
return #function
34+
}
35+
}
36+
37+
func test_usedToBeAsync_but_remoteImplIsNotAnymore() async throws {
38+
let system = DefaultDistributedActorSystem()
39+
40+
let local = Greeter(actorSystem: system)
41+
let remote = try Greeter.resolve(id: local.id, using: system)
42+
43+
var invocation = FakeInvocationEncoder()
44+
// let reply = try await remote.usedToHaveAsyncBytNotAnymore()
45+
let reply: String = try await system.remoteCall(
46+
on: remote,
47+
// Important: notice the mangling has a YaK here, since we mangled
48+
// based on the distributed thunk, which is 'async throws' (YaK).
49+
//
50+
// This test is about ensuring, that even if a remote recipient process,
51+
// changed their implementation from async to not async, we're still able
52+
// to invoke them. From the perspective of the remote caller it truly does
53+
// not matter how the recipient implements this call: is it async or not,
54+
// and we should not tie ability to invoke a method to it's asyncness.
55+
//
56+
// Note also that a remote call is always async and throws as well,
57+
// so mangling based on the 'async throws' thunk does not introduce
58+
// unexpected effects in remote calls.
59+
//
60+
// Design limitation by choice: this means we cannot
61+
target: RemoteCallTarget("$s4main7GreeterC28usedToHaveAsyncBytNotAnymoreSSyYaKFTE"),
62+
invocation: &invocation,
63+
throwing: Never.self,
64+
returning: String.self
65+
)
66+
67+
// CHECK: >> remoteCall: on:main.Greeter, target:main.Greeter.usedToHaveAsyncBytNotAnymore(), invocation:FakeInvocationEncoder(genericSubs: [], arguments: [], returnType: nil, errorType: nil), throwing:Swift.Never, returning:Swift.String
68+
// CHECK: << onReturn: usedToHaveAsyncBytNotAnymore()
69+
70+
print("reply: \(reply)") // CHECK: reply: usedToHaveAsyncBytNotAnymore()
71+
}
72+
73+
func test_usedToBeSync_but_remoteImplIsAsyncNow() async throws {
74+
let system = DefaultDistributedActorSystem()
75+
76+
let local = Helloer(actorSystem: system)
77+
let remote = try Helloer.resolve(id: local.id, using: system)
78+
79+
var invocation = FakeInvocationEncoder()
80+
// let reply: String = try await remote.usedToBeSyncButNowIsAsync()
81+
let reply: String = try await system.remoteCall(
82+
on: remote,
83+
target: RemoteCallTarget("$s4main7HelloerC25usedToBeSyncButNowIsAsyncSSyYaKFTE"),
84+
invocation: &invocation,
85+
throwing: Never.self,
86+
returning: String.self
87+
)
88+
89+
// CHECK: >> remoteCall: on:main.Helloer, target:main.Helloer.usedToBeSyncButNowIsAsync(), invocation:FakeInvocationEncoder(genericSubs: [], arguments: [], returnType: nil, errorType: nil), throwing:Swift.Never, returning:Swift.String
90+
// CHECK: << onReturn: usedToBeSyncButNowIsAsync()
91+
92+
print("reply: \(reply)") // CHECK: reply: usedToBeSyncButNowIsAsync()
93+
}
94+
95+
@main struct Main {
96+
static func main() async {
97+
try! await test_usedToBeAsync_but_remoteImplIsNotAnymore()
98+
try! await test_usedToBeSync_but_remoteImplIsAsyncNow()
99+
}
100+
}
Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,40 @@
1+
// RUN: %empty-directory(%t)
2+
// RUN: %target-swift-frontend-emit-module -emit-module-path %t/FakeDistributedActorSystems.swiftmodule -module-name FakeDistributedActorSystems -disable-availability-checking %S/Inputs/FakeDistributedActorSystems.swift
3+
// RUN: %target-swift-frontend -typecheck -verify -disable-availability-checking -I %t 2>&1 %s
4+
// REQUIRES: concurrency
5+
// REQUIRES: distributed
6+
7+
import Distributed
8+
import FakeDistributedActorSystems
9+
10+
typealias DefaultDistributedActorSystem = FakeActorSystem
11+
12+
// ==== ------------------------------------------------------------------------
13+
14+
distributed actor Overloader {
15+
16+
func overloaded() {}
17+
func overloaded() async {}
18+
19+
distributed func overloadedDistA() {} // expected-note{{ambiguous distributed func 'overloadedDistA()' declared here}}
20+
distributed func overloadedDistA() async {} // expected-error{{ambiguous distributed func declaration 'overloadedDistA()', cannot overload distributed methods on effect only}}
21+
22+
distributed func overloadedDistT() throws {} // expected-note{{ambiguous distributed func 'overloadedDistT()' declared here}}
23+
distributed func overloadedDistT() async throws {} // expected-error{{ambiguous distributed func declaration 'overloadedDistT()', cannot overload distributed methods on effect only}}
24+
25+
// Throws overloads are not legal anyway, but let's check for them here too:
26+
distributed func overloadedDistThrows() {}
27+
// expected-note@-1{{ambiguous distributed func 'overloadedDistThrows()' declared here}}
28+
// expected-note@-2{{'overloadedDistThrows()' previously declared here}}
29+
distributed func overloadedDistThrows() throws {}
30+
// expected-error@-1{{ambiguous distributed func declaration 'overloadedDistThrows()', cannot overload distributed methods on effect only}}
31+
// expected-error@-2{{invalid redeclaration of 'overloadedDistThrows()'}}
32+
33+
distributed func overloadedDistAsync() async {}
34+
// expected-note@-1{{ambiguous distributed func 'overloadedDistAsync()' declared here}}
35+
// expected-note@-2{{'overloadedDistAsync()' previously declared here}}
36+
distributed func overloadedDistAsync() async throws {}
37+
// expected-error@-1{{ambiguous distributed func declaration 'overloadedDistAsync()', cannot overload distributed methods on effect only}}
38+
// expected-error@-2{{invalid redeclaration of 'overloadedDistAsync()'}}
39+
}
40+

0 commit comments

Comments
 (0)