[Concurrency][Distributed] Handle more cases of isolation, protocols, better errors (swiftlang#39895)

* [Distributed] Handle more cases of isolation, protocols, better errors/notes

* [Concurrency] Actor sync funcs can conform to async protocol reqs

* [Distributed] Offer fixit to add DistributedActor to type when dist func is encountered

* [Distributed] Witness protocol reqs in dist actor protocol extensions

* try to address availability issues

Author: ktoso

include/swift/AST/DiagnosticsSema.def

@@ -4351,6 +4351,10 @@ NOTE(note_add_async_to_function,none,
 NOTE(note_add_nonisolated_to_decl,none,
      "add 'nonisolated' to %0 to make this %1 not isolated to the actor",
      (DeclName, DescriptiveDeclKind))
+NOTE(note_add_async_and_throws_to_decl,none,
+     "mark the protocol requirement %0 '%select{|async|throws|async throws}1' "
+     "in order witness it with 'distributed' function declared in distributed actor %2",
+     (DeclName, unsigned, DeclName))
 NOTE(note_add_distributed_to_decl,none,
      "add 'distributed' to %0 to make this %1 witness the protocol requirement",
      (DeclName, DescriptiveDeclKind))
@@ -4484,11 +4488,11 @@ NOTE(actor_isolated_sync_func,none,
      "calls to %0 %1 from outside of its actor context are "
      "implicitly asynchronous",
      (DescriptiveDeclKind, DeclName))
-NOTE(distributed_actor_isolated_method_note,none,
-     "only 'distributed' functions can be called from outside the distributed actor", // TODO(distributed): improve error message
-     ())
+NOTE(note_distributed_actor_isolated_method,none,
+     "distributed actor-isolated %0 %1 declared here",
+     (DescriptiveDeclKind, DeclName))
 ERROR(distributed_actor_isolated_method,none,
-      "only 'distributed' functions can be called from outside the distributed actor", // TODO(distributed): improve error message to be more like 'non-distributed' ... defined here
+      "only 'distributed' functions can be called on a potentially remote distributed actor",
       ())
 ERROR(distributed_actor_func_param_not_codable,none,
       "distributed function parameter '%0' of type %1 does not conform to 'Codable'",
@@ -4497,7 +4501,7 @@ ERROR(distributed_actor_func_result_not_codable,none,
       "distributed function result type %0 does not conform to 'Codable'",
       (Type))
 ERROR(distributed_actor_remote_func_implemented_manually,none,
-      "Distributed function's %0 remote counterpart %1 cannot not be implemented manually.",
+      "distributed function's %0 remote counterpart %1 cannot not be implemented manually.",
       (Identifier, Identifier))
 ERROR(nonisolated_distributed_actor_storage,none,
       "'nonisolated' can not be applied to distributed actor stored properties",
@@ -4523,6 +4527,9 @@ WARNING(shared_mutable_state_access,none,
 ERROR(actor_isolated_witness,none,
      "actor-isolated %0 %1 cannot be used to satisfy a protocol requirement",
      (DescriptiveDeclKind, DeclName))
+ERROR(distributed_actor_isolated_witness,none,
+     "distributed actor-isolated %0 %1 cannot be used to satisfy a protocol requirement",
+     (DescriptiveDeclKind, DeclName))
 ERROR(global_actor_isolated_requirement,none,
       "%0 %1 must be isolated to the global actor %2 to satisfy corresponding "
       "requirement from protocol %3",

lib/Sema/TypeCheckAttr.cpp

@@ -17,6 +17,7 @@
 #include "MiscDiagnostics.h"
 #include "TypeCheckAvailability.h"
 #include "TypeCheckConcurrency.h"
+#include "TypeCheckDistributed.h"
 #include "TypeCheckObjC.h"
 #include "TypeCheckType.h"
 #include "TypeChecker.h"
@@ -5453,9 +5454,10 @@ void AttributeChecker::visitDistributedActorAttr(DistributedActorAttr *attr) {
       return;
     } else if (auto protoDecl = dc->getSelfProtocolDecl()){
       if (!protoDecl->inheritsFromDistributedActor()) {
-        // TODO: could suggest adding `: DistributedActor` to the protocol as well
-        diagnoseAndRemoveAttr(
+        auto diag = diagnoseAndRemoveAttr(
             attr, diag::distributed_actor_func_not_in_distributed_actor);
+        diagnoseDistributedFunctionInNonDistributedActorProtocol(
+            protoDecl, diag);
         return;
       }
     } else if (dc->getSelfStructDecl() || dc->getSelfEnumDecl()) {

lib/Sema/TypeCheckConcurrency.cpp

@@ -1604,7 +1604,9 @@ namespace {
           decl->diagnose(diag::distributed_actor_isolated_property);
         } else {
           // it's a function or subscript
-          decl->diagnose(diag::distributed_actor_isolated_method_note);
+          decl->diagnose(diag::note_distributed_actor_isolated_method,
+                         decl->getDescriptiveKind(),
+                         decl->getName());
         }
       } else if (auto func = dyn_cast<AbstractFunctionDecl>(decl)) {
         func->diagnose(diag::actor_isolated_sync_func,

lib/Sema/TypeCheckDistributed.cpp

@@ -53,6 +53,24 @@ DistributedModuleIsAvailableRequest::evaluate(Evaluator &evaluator,
 
 // ==== ------------------------------------------------------------------------
 
+/// Add Fix-It text for the given protocol type to inherit DistributedActor.
+void swift::diagnoseDistributedFunctionInNonDistributedActorProtocol(
+    const ProtocolDecl *proto, InFlightDiagnostic &diag) {
+  if (proto->getInherited().empty()) {
+    SourceLoc fixItLoc = proto->getBraces().Start;
+    diag.fixItInsert(fixItLoc, ": DistributedActor");
+  } else {
+    // Similar to how Sendable FitIts do this, we insert at the end of
+    // the inherited types.
+    ASTContext &ctx = proto->getASTContext();
+    SourceLoc fixItLoc = proto->getInherited().back().getSourceRange().End;
+    fixItLoc = Lexer::getLocForEndOfToken(ctx.SourceMgr, fixItLoc);
+    diag.fixItInsert(fixItLoc, ", DistributedActor");
+  }
+}
+
+// ==== ------------------------------------------------------------------------
+
 bool IsDistributedActorRequest::evaluate(
     Evaluator &evaluator, NominalTypeDecl *nominal) const {
   // Protocols are actors if they inherit from `DistributedActor`.
@@ -63,7 +81,8 @@ bool IsDistributedActorRequest::evaluate(
             protocol->inheritsFrom(distributedActorProtocol));
   }
 
-  // Class declarations are 'distributed actors' if they are declared with 'distributed actor'
+  // Class declarations are 'distributed actors' if they are declared with
+  // 'distributed actor'
   auto classDecl = dyn_cast<ClassDecl>(nominal);
   if(!classDecl)
     return false;

lib/Sema/TypeCheckDistributed.h

@@ -48,6 +48,10 @@ bool checkDistributedFunction(FuncDecl *decl, bool diagnose);
 /// Determine the distributed actor transport type for the given actor.
 Type getDistributedActorTransportType(NominalTypeDecl *actor);
 
+/// Diagnose a distributed func declaration in a not-distributed actor protocol.
+void diagnoseDistributedFunctionInNonDistributedActorProtocol(
+  const ProtocolDecl *proto, InFlightDiagnostic &diag);
+
 }
 
 

lib/Sema/TypeCheckProtocol.cpp

@@ -2829,21 +2829,105 @@ bool ConformanceChecker::checkActorIsolation(
 
     // An actor-isolated witness can only conform to an actor-isolated
     // requirement.
-    if (requirementIsolation == ActorIsolation::ActorInstance)
+    if (requirementIsolation == ActorIsolation::ActorInstance) {
       return false;
+    }
 
     auto witnessFunc = dyn_cast<AbstractFunctionDecl>(witness);
     auto requirementFunc = dyn_cast<AbstractFunctionDecl>(requirement);
+    auto nominal = dyn_cast<NominalTypeDecl>(witness->getDeclContext());
+    auto witnessClass = dyn_cast<ClassDecl>(witness->getDeclContext());
+    if (auto extension = dyn_cast<ExtensionDecl>(witness->getDeclContext())) {
+      // We can witness a distributed function in an extension, as long as
+      // that extension itself is on a DistributedActor type (including
+      // protocols that inherit from DistributedActor, even if the protocol
+      // requirement was not expressed in terms of distributed actors).
+      nominal = extension->getExtendedNominal();
+    }
 
     /// Distributed actors can witness protocol requirements either with
     /// nonisolated or distributed members.
-    auto witnessClass = dyn_cast<ClassDecl>(witness->getDeclContext());
-    if (witnessClass && witnessClass->isDistributedActor()) {
-      // Maybe we're dealing with a 'distributed func' which is witness to
-      // a distributed function requirement, this is ok.
-      if (requirementFunc && requirementFunc->isDistributed() &&
-          witnessFunc && witnessFunc->isDistributed()) {
+    if (nominal && nominal->isDistributedActor()) {
+      // A distributed actor may conform to an 'async throws' function
+      // requirement with a distributed function, because those are always
+      // cross-actor.
+      //
+      // If the distributed function is well-formed (passed checks) then it can
+      // witness this requirement. I.e. since checks to the distributed function
+      // passed, it can be called through this protocol.
+      if (witnessFunc && witnessFunc->isDistributed()) {
+        // If the requirement was also a 'distributed func' we most definitely
+        // can witness it with our 'distributed func' witness.
+        if (requirementFunc && requirementFunc->isDistributed()) {
           return false;
+        }
+        if (requirementFunc->hasAsync() && requirementFunc->hasThrows()) {
+          return false;
+        }
+
+        if (requirementFunc) {
+          // The witness was distributed, but the requirement func was not
+          // 'async throws', so we can suggest adding those to the protocol
+          int suggestAddingModifiers = 0;
+          if (!requirementFunc->hasAsync()) suggestAddingModifiers += 1;
+          if (!requirementFunc->hasThrows()) suggestAddingModifiers += 2;
+          requirementFunc->diagnose(diag::note_add_async_and_throws_to_decl,
+                                    witness->getName(),
+                                    suggestAddingModifiers,
+                                    witnessClass ? witnessClass->getName() :
+                                                   nominal->getName());
+          // TODO(distributed): fixit inserts for the async/throws
+        }
+      }
+
+      // The witness definitely is distributed actor isolated,
+      // so diagnose this as an error; next we'll provide helpful notes
+      witness->diagnose(diag::distributed_actor_isolated_witness,
+                        witness->getDescriptiveKind(), witness->getName())
+          .fixItInsert(witness->getAttributeInsertionLoc(true),
+                       "distributed ");
+
+      // witness is not 'distributed', if the requirement was though,
+      // then we definitely must mark the witness distributed as well.
+      if (requirementFunc->isDistributed()) {
+        witness->diagnose(diag::note_add_distributed_to_decl,
+                          witness->getName(),
+                          witness->getDescriptiveKind())
+            .fixItInsert(witness->getAttributeInsertionLoc(true),
+                         "distributed ");
+        requirement->diagnose(diag::note_distributed_requirement_defined_here,
+                              requirement->getName());
+      } else if (requirementFunc->hasAsync() && requirementFunc->hasThrows()) {
+        assert(!requirementFunc->isDistributed());
+        // If the requirement is 'async throws' we can add 'distributed' to the
+        // distributed actor function to witness the requirement.
+        witness->diagnose(diag::note_add_distributed_to_decl,
+                          witness->getName(), witness->getDescriptiveKind())
+            .fixItInsert(witness->getAttributeInsertionLoc(true),
+                         "distributed ");
+      }
+
+      if (!requirementFunc->hasAsync() && !requirementFunc->isDistributed()) {
+        /// The requirement is synchronous, so we can only conform to it using
+        /// 'nonisolated'...
+        witness->diagnose(diag::note_add_nonisolated_to_decl,
+                          witness->getName(), witness->getDescriptiveKind())
+            .fixItInsert(witness->getAttributeInsertionLoc(true),
+                         "nonisolated ");
+
+        /// ... or by suggesting to make it 'async'
+      }
+
+      return true;
+    }
+
+    // A synchronous actor function can witness an asynchronous protocol
+    // requirement, since calls "through" the protocol are always cross-actor,
+    // in which case the function becomes implicitly async.
+    if (witnessClass && witnessClass->isActor()) {
+      if (requirementFunc && requirementFunc->hasAsync() &&
+          (requirementFunc->hasThrows() == witnessFunc->hasThrows())) {
+        return false;
       }
     }
 
@@ -2856,9 +2940,8 @@ bool ConformanceChecker::checkActorIsolation(
         if (requirementFunc && requirementFunc->isDistributed()) {
           // a distributed protocol requirement can be witnessed with a
           // distributed function:
-          witness
-              ->diagnose(diag::note_add_distributed_to_decl,
-                         witness->getName(), witness->getDescriptiveKind())
+          witness->diagnose(diag::note_add_distributed_to_decl,
+                            witness->getName(), witness->getDescriptiveKind())
               .fixItInsert(witness->getAttributeInsertionLoc(true),
                            "distributed ");
           requirement
@@ -2878,10 +2961,72 @@ bool ConformanceChecker::checkActorIsolation(
     return true;
   }
 
-  case ActorIsolationRestriction::CrossActorSelf:
-    return diagnoseNonSendableTypesInReference(
+  case ActorIsolationRestriction::CrossActorSelf: {
+    if (diagnoseNonSendableTypesInReference(
         witness, DC->getParentModule(), witness->getLoc(),
-        ConcurrentReferenceKind::CrossActor);
+        ConcurrentReferenceKind::CrossActor)) {
+      return true;
+    }
+
+    auto witnessFunc = dyn_cast<AbstractFunctionDecl>(witness);
+    auto requirementFunc = dyn_cast<AbstractFunctionDecl>(requirement);
+    auto nominal = dyn_cast<NominalTypeDecl>(witness->getDeclContext());
+    auto witnessClass = dyn_cast<ClassDecl>(witness->getDeclContext());
+    if (auto extension = dyn_cast<ExtensionDecl>(witness->getDeclContext())) {
+      // We can witness a distributed function in an extension, as long as
+      // that extension itself is on a DistributedActor type (including
+      // protocols that inherit from DistributedActor, even if the protocol
+      // requirement was not expressed in terms of distributed actors).
+      nominal = extension->getExtendedNominal();
+      witnessClass = extension->getSelfClassDecl();
+    }
+
+    if (nominal && nominal->isDistributedActor()) {
+      // A distributed actor may conform to an 'async throws' function
+      // requirement with a distributed function, because those are always
+      // cross-actor.
+      //
+      // If the distributed function is well-formed (passed checks) then it can
+      // witness this requirement. I.e. since checks to the distributed function
+      // passed, it can be called through this protocol.
+      if (witnessFunc && witnessFunc->isDistributed()) {
+        // If the requirement was also a 'distributed func' we most definitely
+        // can witness it with our 'distributed func' witness.
+        if (requirementFunc && requirementFunc->isDistributed()) {
+          return false;
+        }
+        if (requirementFunc->hasAsync() && requirementFunc->hasThrows()) {
+          return false;
+        }
+
+        if (requirementFunc) {
+          // The witness was distributed, but the requirement func was not
+          // 'async throws', so we can suggest adding those to the protocol
+          int suggestAddingModifiers = 0;
+          if (!requirementFunc->hasAsync()) suggestAddingModifiers += 1;
+          if (!requirementFunc->hasThrows()) suggestAddingModifiers += 2;
+          requirementFunc->diagnose(diag::note_add_async_and_throws_to_decl,
+                                    witness->getName(),
+                                    suggestAddingModifiers,
+                                    witnessClass ? witnessClass->getName() :
+                                                   nominal->getName());
+          // TODO(distributed): fixit inserts for the async/throws
+        } // TODO(distributed): handle computed properties as well?
+      }
+
+      if (witnessFunc) {
+        witness
+            ->diagnose(diag::distributed_actor_isolated_witness,
+                       witness->getDescriptiveKind(), witness->getName())
+            .fixItInsert(witness->getAttributeInsertionLoc(true),
+                         "distributed ");
+      }
+
+      return true;
+    }
+
+    return false;
+  }
 
   case ActorIsolationRestriction::GlobalActorUnsafe:
     witnessIsUnsafe = true;

test/Concurrency/actor_isolation.swift

@@ -890,6 +890,17 @@ func testCrossActorProtocol<T: P>(t: T) async {
   ASPD.sd()
 }
 
+@available(SwiftStdlib 5.5, *)
+protocol Server {
+  func send<Message: Codable>(message: Message) async throws -> String
+}
+
+@available(SwiftStdlib 5.5, *)
+actor MyServer : Server {
+  // okay, asynchronously accessed from clients of the protocol
+  func send<Message: Codable>(message: Message) throws -> String { "" }
+}
+
 // ----------------------------------------------------------------------
 // @_inheritActorContext
 // ----------------------------------------------------------------------