Merge pull request #69596 from ktoso/pick-wip-distributed-overloads

ktoso · web-flow · commit 805d5c5834c2 · 2023-11-10T07:20:20.000+09:00
🍒[5.10][Distributed] Allow overloads with different types; mangling can handle it
diff --git a/include/swift/AST/Decl.h b/include/swift/AST/Decl.h
@@ -273,6 +273,9 @@ struct OverloadSignature {
   /// Whether this is an async function.
   unsigned IsAsyncFunction : 1;
 
+  /// Whether this is an distributed function.
+  unsigned IsDistributed : 1;
+
   /// Whether this is a enum element.
   unsigned IsEnumElement : 1;
 
@@ -298,8 +301,8 @@ struct OverloadSignature {
   OverloadSignature()
       : UnaryOperator(UnaryOperatorKind::None), IsInstanceMember(false),
         IsVariable(false), IsFunction(false), IsAsyncFunction(false),
-        InProtocolExtension(false), InExtensionOfGenericType(false),
-        HasOpaqueReturnType(false) { }
+        IsDistributed(false), InProtocolExtension(false),
+        InExtensionOfGenericType(false), HasOpaqueReturnType(false) { }
 };
 
 /// Determine whether two overload signatures conflict.
diff --git a/include/swift/AST/DiagnosticsSema.def b/include/swift/AST/DiagnosticsSema.def
@@ -5210,8 +5210,8 @@ ERROR(conflicting_default_argument_isolation,none,
 ERROR(distributed_actor_needs_explicit_distributed_import,none,
       "'Distributed' module not imported, required for 'distributed actor'",
       ())
-ERROR(distributed_func_cannot_overload_on_async_only,none,
-      "ambiguous distributed func declaration %0, cannot overload distributed methods on effect only", (DeclName))
+NOTE(distributed_func_cannot_overload_on_async_only,none,
+     "%0 previously declared here, cannot overload distributed methods on effect only", (const ValueDecl *))
 NOTE(distributed_func_other_ambiguous_overload_here,none,
       "ambiguous distributed func %0 declared here", (DeclName))
 ERROR(actor_isolated_inout_state,none,
diff --git a/lib/AST/Decl.cpp b/lib/AST/Decl.cpp
@@ -3073,9 +3073,35 @@ bool swift::conflicting(const OverloadSignature& sig1,
   if (sig1.IsInstanceMember != sig2.IsInstanceMember)
     return false;
 
-  // If one is an async function and the other is not, they can't conflict.
-  if (sig1.IsAsyncFunction != sig2.IsAsyncFunction)
-    return false;
+  // For distributed decls, check there's no async/no-async overloads,
+  // since those are more fragile in distribution than we'd want distributed calls to be.
+  //
+  // A remote call is always 'async throws', and we can always record
+  // an async throws "accessor" (see AccessibleFunction.cpp) as such.
+  // This means, if we allowed async/no-async overloads of functions,
+  // we'd have to store the precise "it was not throwing" information,
+  // but we'll _never_ make use of such because all remote calls are
+  // necessarily going to async to the actor in the recipient process,
+  // and for the remote caller, they are always as-if-async.
+  //
+  // By banning such overloads, which may be useful in local APIs,
+  // but too fragile in distributed APIs, we allow a remote 'v2' version
+  // of an implementation to add or remove `async` to their implementation
+  // without breaking calls which were made on previous 'v1' versions of
+  // the same interface; Callers are never broken this way, and rollouts
+  // are simpler.
+  //
+  // The restriction on overloads is not a problem for distributed calls,
+  // as we don't have a vast swab of APIs which must compatibly get async
+  // versions, as that is what the async overloading aimed to address.
+  //
+  // Note also, that overloading on throws is already illegal anyway.
+  if (!sig1.IsDistributed && !sig2.IsDistributed) {
+    // For non-distributed functions,
+    // if one is an async function and the other is not, they don't conflict.
+    if (sig1.IsAsyncFunction != sig2.IsAsyncFunction)
+      return false;
+  } // else, if any of the methods was distributed, continue checking
 
   // If one is a macro and the other is not, they can't conflict.
   if (sig1.IsMacro != sig2.IsMacro)
@@ -3328,6 +3354,8 @@ OverloadSignature ValueDecl::getOverloadSignature() const {
     signature.IsFunction = true;
     if (func->hasAsync())
       signature.IsAsyncFunction = true;
+    if (func->isDistributed())
+      signature.IsDistributed = true;
   }
 
   if (auto *extension = dyn_cast<ExtensionDecl>(getDeclContext()))
diff --git a/lib/Sema/TypeCheckDeclPrimary.cpp b/lib/Sema/TypeCheckDeclPrimary.cpp
@@ -861,6 +861,16 @@ CheckRedeclarationRequest::evaluate(Evaluator &eval, ValueDecl *current,
         wouldBeSwift5Redeclaration = false;
       }
 
+      // Distributed declarations cannot be overloaded on async-ness only,
+      // because it'd cause problems with the always async distributed thunks.
+      // Provide an extra diagnostic if this is the case we're facing.
+      bool diagnoseDistributedAsyncOverload = false;
+      if (auto func = dyn_cast<AbstractFunctionDecl>(other)) {
+        diagnoseDistributedAsyncOverload = func->isDistributed();
+      } else  if (auto var = dyn_cast<VarDecl>(other)) {
+        diagnoseDistributedAsyncOverload = var->isDistributed();
+      }
+
       // If this isn't a redeclaration in the current version of Swift, but
       // would be in Swift 5 mode, emit a warning instead of an error.
       if (wouldBeSwift5Redeclaration) {
@@ -967,7 +977,13 @@ CheckRedeclarationRequest::evaluate(Evaluator &eval, ValueDecl *current,
         } else {
           ctx.Diags.diagnoseWithNotes(
             current->diagnose(diag::invalid_redecl, current), [&]() {
-            other->diagnose(diag::invalid_redecl_prev, other);
+
+            // Add a specialized note about the 'other' overload
+            if (diagnoseDistributedAsyncOverload) {
+              other->diagnose(diag::distributed_func_cannot_overload_on_async_only, other);
+            } else {
+              other->diagnose(diag::invalid_redecl_prev, other);
+            }
           });
 
           current->setInvalid();
diff --git a/lib/Sema/TypeCheckDistributed.cpp b/lib/Sema/TypeCheckDistributed.cpp
@@ -705,19 +705,6 @@ void TypeChecker::checkDistributedActor(SourceFile *SF, NominalTypeDecl *nominal
   // If applicable, this will create the default 'init(transport:)' initializer
   (void)nominal->getDefaultInitializer();
 
-  // We check decls for ambiguity more strictly than normal nominal types,
-  // because we want to record distributed accessors the same if function they
-  // point at (in a remote process) is async or not, as that has no effect on
-  // a caller from a different process, so we want to make the remoteCall target
-  // identifiers, less fragile against such refactorings.
-  //
-  // To achieve this, we ban overloads on just "effects" of functions,
-  // which are useful in local settings, but really should not be relied
-  // on as differenciators in remote calls - the call will always be "async"
-  // since it will go through a thunk, and then be asynchronously transferred
-  // to the called process.
-  llvm::SmallDenseSet<DeclName, 2> diagnosedAmbiguity;
-
   for (auto member : nominal->getMembers()) {
     // --- Ensure 'distributed func' all thunks
     if (auto *var = dyn_cast<VarDecl>(member)) {
@@ -743,48 +730,6 @@ void TypeChecker::checkDistributedActor(SourceFile *SF, NominalTypeDecl *nominal
               nominal->getName());
           return;
         }
-
-        // Check there's no async/no-async overloads, since those are more
-        // fragile in distribution than we'd want distributed calls to be.
-        // A remote call is always 'async throws', and we can always record
-        // an async throws "accessor" (see AccessibleFunction.cpp) as such.
-        // This means, if we allowed async/no-async overloads of functions,
-        // we'd have to store the precise "it was not throwing" information,
-        // but we'll _never_ make use of such because all remote calls are
-        // necessarily going to async to the actor in the recipient process,
-        // and for the remote caller, they are always as-if-async.
-        //
-        // By banning such overloads, which may be useful in local APIs,
-        // but too fragile in distributed APIs, we allow a remote 'v2' version
-        // of an implementation to add or remove `async` to their implementation
-        // without breaking calls which were made on previous 'v1' versions of
-        // the same interface; Callers are never broken this way, and rollouts
-        // are simpler.
-        //
-        // The restriction on overloads is not a problem for distributed calls,
-        // as we don't have a vast swab of APIs which must compatibly get async
-        // versions, as that is what the async overloading aimed to address.
-        //
-        // Note also, that overloading on throws is already illegal anyway.
-        if (!diagnosedAmbiguity.contains(func->getName())) {
-          auto candidates = nominal->lookupDirect(func->getName());
-          if (candidates.size() > 1) {
-            auto firstDecl = dyn_cast<AbstractFunctionDecl>(candidates.back());
-            for (auto decl : candidates) {
-              if (decl == firstDecl) {
-                decl->diagnose(
-                    diag::distributed_func_cannot_overload_on_async_only,
-                    decl->getName());
-              } else {
-                decl->diagnose(
-                    diag::distributed_func_other_ambiguous_overload_here,
-                    decl->getName());
-              }
-            }
-
-            diagnosedAmbiguity.insert(func->getName());
-          }
-        }
       }
 
       if (auto thunk = func->getDistributedThunk()) {
diff --git a/test/Distributed/Runtime/distributed_actor_func_calls_remoteCall_overloads.swift b/test/Distributed/Runtime/distributed_actor_func_calls_remoteCall_overloads.swift
@@ -0,0 +1,61 @@
+// 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 -module-name main  -Xfrontend -disable-availability-checking -j2 -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 | %FileCheck %s --color
+
+// REQUIRES: executable_test
+// REQUIRES: concurrency
+// REQUIRES: distributed
+
+// rdar://76038845
+// UNSUPPORTED: use_os_stdlib
+// UNSUPPORTED: back_deployment_runtime
+
+// FIXME(distributed): Distributed actors currently have some issues on windows, isRemote always returns false. rdar://82593574
+// UNSUPPORTED: OS=windows-msvc
+
+import Distributed
+import FakeDistributedActorSystems
+
+typealias DefaultDistributedActorSystem = FakeRoundtripActorSystem
+
+distributed actor Greeter {
+  distributed func callMe(_ name: String) -> String {
+    return "\(name)"
+  }
+  distributed func callMe(_ number: Int) -> String {
+    return "\(number)"
+  }
+}
+
+struct SomeError: Error, Sendable, Codable {}
+
+func test() async throws {
+  let system = DefaultDistributedActorSystem()
+
+  let local = Greeter(actorSystem: system)
+  let ref = try Greeter.resolve(id: local.id, using: system)
+
+  do {
+    let echo = try await ref.callMe("hello")
+    precondition(echo == "hello")
+    // CHECK: >> remoteCall: on:main.Greeter, target:main.Greeter.callMe(_:), invocation:FakeInvocationEncoder(genericSubs: [], arguments: ["hello"], returnType: Optional(Swift.String), errorType: nil), throwing:Swift.Never, returning:Swift.String
+
+    let echo2 = try await ref.callMe(42)
+    precondition(echo2 == "42")
+    // CHECK: >> remoteCall: on:main.Greeter, target:main.Greeter.callMe(_:), invocation:FakeInvocationEncoder(genericSubs: [], arguments: [42], returnType: Optional(Swift.String), errorType: nil), throwing:Swift.Never, returning:Swift.String
+
+    print("did not throw")
+    // CHECK: did not throw
+  } catch {
+    print("error: \(error)")
+    // CHECK-NOT: error:
+  }
+}
+
+@main struct Main {
+  static func main() async {
+    try! await test()
+  }
+}
diff --git a/test/Distributed/distributed_func_overloads.swift b/test/Distributed/distributed_func_overloads.swift
@@ -16,25 +16,39 @@ distributed actor Overloader {
   func overloaded() {}
   func overloaded() async {}
 
-  distributed func overloadedDistA() {} // expected-note{{ambiguous distributed func 'overloadedDistA()' declared here}}
-  distributed func overloadedDistA() async {} // expected-error{{ambiguous distributed func declaration 'overloadedDistA()', cannot overload distributed methods on effect only}}
+  distributed func overloadedDistA() {}
+  // expected-note@-1{{'overloadedDistA()' previously declared here, cannot overload distributed methods on effect only}}
+  distributed func overloadedDistA() async {}
+  // expected-error@-1{{invalid redeclaration of 'overloadedDistA()'}}
 
-  distributed func overloadedDistT() throws {} // expected-note{{ambiguous distributed func 'overloadedDistT()' declared here}}
-  distributed func overloadedDistT() async throws {} // expected-error{{ambiguous distributed func declaration 'overloadedDistT()', cannot overload distributed methods on effect only}}
+  distributed func overloadedDistT() throws {}
+  // expected-note@-1{{'overloadedDistT()' previously declared here, cannot overload distributed methods on effect only}}
+  distributed func overloadedDistT() async throws {}
+  // expected-error@-1{{invalid redeclaration of 'overloadedDistT()'}}
+
+  distributed func overloadedDistST(string: String) throws {}
+  // expected-note@-1{{'overloadedDistST(string:)' previously declared here, cannot overload distributed methods on effect only}}
+  distributed func overloadedDistST(string: String) async throws {}
+  // expected-error@-1{{invalid redeclaration of 'overloadedDistST(string:)'}}
 
   // Throws overloads are not legal anyway, but let's check for them here too:
   distributed func overloadedDistThrows() {}
-  // expected-note@-1{{ambiguous distributed func 'overloadedDistThrows()' declared here}}
-  // expected-note@-2{{'overloadedDistThrows()' previously declared here}}
+  // expected-note@-1{{'overloadedDistThrows()' previously declared here}}
   distributed func overloadedDistThrows() throws {}
-  // expected-error@-1{{ambiguous distributed func declaration 'overloadedDistThrows()', cannot overload distributed methods on effect only}}
-  // expected-error@-2{{invalid redeclaration of 'overloadedDistThrows()'}}
+  // expected-error@-1{{invalid redeclaration of 'overloadedDistThrows()'}}
 
   distributed func overloadedDistAsync() async {}
-  // expected-note@-1{{ambiguous distributed func 'overloadedDistAsync()' declared here}}
-  // expected-note@-2{{'overloadedDistAsync()' previously declared here}}
+  // expected-note@-1{{'overloadedDistAsync()' previously declared here}}
   distributed func overloadedDistAsync() async throws {}
-  // expected-error@-1{{ambiguous distributed func declaration 'overloadedDistAsync()', cannot overload distributed methods on effect only}}
-  // expected-error@-2{{invalid redeclaration of 'overloadedDistAsync()'}}
+  // expected-error@-1{{invalid redeclaration of 'overloadedDistAsync()'}}
+
+  // overloads differing by parameter type are allowed,
+  // since the mangled identifier includes full type information:
+  distributed func overloadedDistParams(param: String) async {} // ok
+  distributed func overloadedDistParams(param: Int) async {} // ok
+
+  distributed func overloadedDistParams() async {} // also ok
+
+  distributed func overloadedDistParams<A: Sendable & Codable>(param: A) async {} // ok
 }
 
