Remove ad-hoc ambiguity detection code; use conflicting()

Author: ktoso

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.

include/swift/AST/DiagnosticsSema.def

@@ -5213,8 +5213,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,

lib/AST/Decl.cpp

@@ -3072,9 +3072,37 @@ 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) {
+    if (sig1.IsAsyncFunction != sig2.IsAsyncFunction)
+      return true;
+  } else {
+    // Otherwise one is an async function and the other is not, they don't conflict.
+    if (sig1.IsAsyncFunction != sig2.IsAsyncFunction)
+      return false;
+  }
 
   // If one is a macro and the other is not, they can't conflict.
   if (sig1.IsMacro != sig2.IsMacro)
@@ -3327,6 +3355,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()))

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();

lib/Sema/TypeCheckDistributed.cpp

@@ -707,7 +707,7 @@ void TypeChecker::checkDistributedActor(SourceFile *SF, NominalTypeDecl *nominal
   // 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;
+//  llvm::SmallDenseSet<DeclName, 2> diagnosedAmbiguity;
 
   for (auto member : nominal->getMembers()) {
     // --- Ensure 'distributed func' all thunks
@@ -734,63 +734,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 candidate: candidates) {
-              if (auto candidateFunc = dyn_cast<AbstractFunctionDecl>(candidate)) {
-                assert(candidateFunc->getParameters()->size() ==
-                       firstDecl->getParameters()->size());
-                bool allSame = true;
-                for (size_t i = 0; i < candidateFunc->getParameters()->size(); ++i) {
-                  auto lhs = firstDecl->getParameters()->get(i);
-                  auto rhs = candidateFunc->getParameters()->get(i);
-                  if (!lhs->getInterfaceType()->isEqual(rhs->getInterfaceType())) {
-                    allSame = false;
-                    break;
-                  }
-                }
-
-                if (candidate != firstDecl && // can't be ambiguous with itself
-                    allSame && // diagnose if ambiguous
-                    !diagnosedAmbiguity.contains(func->getName())) {
-                  candidate->diagnose(
-                      diag::distributed_func_cannot_overload_on_async_only,
-                      candidate->getName());
-                  diagnosedAmbiguity.insert(func->getName());
-                } else if (diagnosedAmbiguity.contains(func->getName())) {
-                  candidate->diagnose(
-                      diag::distributed_func_other_ambiguous_overload_here,
-                      candidate->getName());
-                }
-              }
-            }
-          }
-        }
       }
 
       if (auto thunk = func->getDistributedThunk()) {

test/Distributed/distributed_func_overloads.swift

@@ -16,34 +16,39 @@ distributed actor Overloader {
   func overloaded() {}
   func overloaded() async {}
 
-  distributed func overloadedDistA() {} // expected-error{{ambiguous distributed func declaration 'overloadedDistA()', cannot overload distributed methods on effect only}}
-  distributed func overloadedDistA() async {} // expected-note{{ambiguous distributed func 'overloadedDistA()' declared here}}
+  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-error{{ambiguous distributed func declaration 'overloadedDistT()', cannot overload distributed methods on effect only}}
-  distributed func overloadedDistT() async throws {} // expected-note{{ambiguous distributed func 'overloadedDistT()' declared here}}
+  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{{'overloadedDistThrows()' previously declared here}}
-  // expected-error@-2{{ambiguous distributed func declaration 'overloadedDistThrows()', cannot overload distributed methods on effect only}}
   distributed func overloadedDistThrows() throws {}
   // expected-error@-1{{invalid redeclaration of 'overloadedDistThrows()'}}
-  // expected-note@-2{{ambiguous distributed func 'overloadedDistThrows()' declared here}}
 
   distributed func overloadedDistAsync() async {}
   // expected-note@-1{{'overloadedDistAsync()' previously declared here}}
-  // expected-error@-2{{ambiguous distributed func declaration 'overloadedDistAsync()', cannot overload distributed methods on effect only}}
   distributed func overloadedDistAsync() async throws {}
   // expected-error@-1{{invalid redeclaration of 'overloadedDistAsync()'}}
-  // expected-note@-2{{ambiguous distributed func 'overloadedDistAsync()' declared here}}
 
   // overloads differing by parameter type are allowed,
   // since the mangled identifier includes full type information:
-  distributed func overloadedDistParams(param: String) async {}
-  distributed func overloadedDistParams(param: Int) async {}
+  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 {}
+  distributed func overloadedDistParams<A: Sendable & Codable>(param: A) async {} // ok
 }