Unify 'distributed' checking for accesses to distributed actors.

Unify the logic for checking and marking accesses as
async/throws/uses-distributed-thunk to reduce the overall amount of
code, clarify the isolated/local/potentially-remote cases, and
consistently set these options. There are tiny semantic improvements
here where, e.g., one can asynchronously use non-distributed
functions, properties, and subscripts if we're in a context where we
know the actor is not remote.

Author: DougGregor

lib/Sema/TypeCheckConcurrency.cpp

@@ -1559,23 +1559,62 @@ namespace {
       FoundAsync, // successfully marked an implicitly-async operation
       NotFound,  // fail: no valid implicitly-async operation was found
       SyncContext, // fail: a valid implicitly-async op, but in sync context
-      NotSendable  // fail: valid op and context, but not Sendable
+      NotSendable,  // fail: valid op and context, but not Sendable
+      NotDistributed, // fail: non-distributed declaration in distributed actor
     };
 
+    /// Determine whether we can access the given declaration that is
+    /// isolated to a distributed actor from a location that is potentially not
+    /// local to this process.
+    ///
+    /// \returns the (setThrows, isDistributedThunk) bits to implicitly
+    /// mark the access/call with on success, or emits an error and returns
+    /// \c None.
+    Optional<std::pair<bool, bool>>
+    checkDistributedAccess(SourceLoc declLoc, ValueDecl *decl,
+                           Expr *context) {
+      // Cannot reference properties or subscripts of distributed actors.
+      if (isPropOrSubscript(decl)) {
+        ctx.Diags.diagnose(
+            declLoc, diag::distributed_actor_isolated_non_self_reference,
+            decl->getDescriptiveKind(), decl->getName());
+        noteIsolatedActorMember(decl, context);
+        return None;
+      }
+
+      // Check that we have a distributed function.
+      auto func = dyn_cast<AbstractFunctionDecl>(decl);
+      if (!func || !func->isDistributed()) {
+        ctx.Diags.diagnose(declLoc,
+                           diag::distributed_actor_isolated_method)
+          .fixItInsert(decl->getAttributeInsertionLoc(true), "distributed ");
+
+        noteIsolatedActorMember(decl, context);
+        return None;
+      }
+
+      return std::make_pair(!func->hasThrows(), true);
+    }
+
     /// Attempts to identify and mark a valid cross-actor use of a synchronous
     /// actor-isolated member (e.g., sync function application, property access)
     AsyncMarkingResult tryMarkImplicitlyAsync(SourceLoc declLoc,
                                               ConcreteDeclRef concDeclRef,
                                               Expr* context,
-                                              ImplicitActorHopTarget target) {
+                                              ImplicitActorHopTarget target,
+                                              bool isDistributed) {
       ValueDecl *decl = concDeclRef.getDecl();
       AsyncMarkingResult result = AsyncMarkingResult::NotFound;
+      bool isAsyncCall = false;
 
       // is it an access to a property?
       if (isPropOrSubscript(decl)) {
+        // Cannot reference properties or subscripts of distributed actors.
+        if (isDistributed && !checkDistributedAccess(declLoc, decl, context))
+          return AsyncMarkingResult::NotDistributed;
+
         if (auto declRef = dyn_cast_or_null<DeclRefExpr>(context)) {
           if (usageEnv(declRef) == VarRefUseEnv::Read) {
-
             if (!isInAsynchronousContext())
               return AsyncMarkingResult::SyncContext;
 
@@ -1601,9 +1640,7 @@ namespace {
         if (!isInAsynchronousContext())
           return AsyncMarkingResult::SyncContext;
 
-        markNearestCallAsImplicitly(/*setAsync=*/target);
-        result = AsyncMarkingResult::FoundAsync;
-
+        isAsyncCall = true;
       } else if (!applyStack.empty()) {
         // Check our applyStack metadata from the traversal.
         // Our goal is to identify whether the actor reference appears
@@ -1620,13 +1657,33 @@ namespace {
               return AsyncMarkingResult::SyncContext;
 
             // then this ValueDecl appears as the called value of the ApplyExpr.
-            markNearestCallAsImplicitly(/*setAsync=*/target);
-            result = AsyncMarkingResult::FoundAsync;
+            isAsyncCall = true;
           }
         }
       }
 
+      // Set up an implicit async call.
+      if (isAsyncCall) {
+        // If we're calling to a distributed actor, make sure the function
+        // is actually 'distributed'.
+        bool setThrows = false;
+        bool usesDistributedThunk = false;
+        if (isDistributed) {
+          if (auto access = checkDistributedAccess(declLoc, decl, context))
+            std::tie(setThrows, usesDistributedThunk) = *access;
+          else
+            return AsyncMarkingResult::NotDistributed;
+        }
+
+        // Mark call as implicitly 'async', and also potentially as
+        // throwing and using a distributed thunk.
+        markNearestCallAsImplicitly(
+            /*setAsync=*/target, setThrows, usesDistributedThunk);
+        result = AsyncMarkingResult::FoundAsync;
+      }
+
       if (result == AsyncMarkingResult::FoundAsync) {
+
         // Check for non-sendable types.
         bool problemFound =
             diagnoseNonSendableTypesInReference(
@@ -1639,56 +1696,6 @@ namespace {
       return result;
     }
 
-    enum ThrowsMarkingResult {
-      FoundThrows,
-      NotFound
-    };
-
-    ThrowsMarkingResult tryMarkImplicitlyThrows(SourceLoc declLoc,
-                                                ConcreteDeclRef concDeclRef,
-                                                Expr* context) {
-      ValueDecl *decl = concDeclRef.getDecl();
-      ThrowsMarkingResult result = ThrowsMarkingResult::NotFound;
-
-      if (isa_and_nonnull<SelfApplyExpr>(context)) {
-        if (auto func = dyn_cast<AbstractFunctionDecl>(decl)) {
-          if (func->isDistributed() && !func->hasThrows()) {
-            // A distributed function is implicitly throwing if called from
-            // outside of the actor.
-            //
-            // If it already is throwing, no need to mark it implicitly so.
-            markNearestCallAsImplicitly(
-                /*setAsync=*/None, /*setThrows=*/true);
-            result = ThrowsMarkingResult::FoundThrows;
-          }
-        }
-      } else if (!applyStack.empty()) {
-        // Check our applyStack metadata from the traversal.
-        // Our goal is to identify whether the actor reference appears
-        // as the called value of the enclosing ApplyExpr. We cannot simply
-        // inspect Parent here because of expressions like (callee)()
-        // and the fact that the reference may be just an argument to an apply
-        ApplyExpr *apply = applyStack.back();
-        Expr *fn = apply->getFn()->getValueProvidingExpr();
-        if (auto memberRef = findMemberReference(fn)) {
-          auto concDecl = memberRef->first;
-          if (decl == concDecl.getDecl() && !apply->implicitlyThrows()) {
-
-            if (auto func = dyn_cast<AbstractFunctionDecl>(decl)) {
-              if (func->isDistributed() && !func->hasThrows()) {
-                // then this ValueDecl appears as the called value of the ApplyExpr.
-                markNearestCallAsImplicitly(
-                    /*setAsync=*/None, /*setThrows=*/true);
-                result = ThrowsMarkingResult::FoundThrows;
-              }
-            }
-          }
-        }
-      }
-
-      return result;
-    }
-
     /// Check actor isolation for a particular application.
     bool checkApply(ApplyExpr *apply) {
       auto fnExprType = apply->getFn()->getType();
@@ -1850,7 +1857,8 @@ namespace {
       // Call is implicitly asynchronous.
       auto result = tryMarkImplicitlyAsync(
         loc, valueRef, context,
-        ImplicitActorHopTarget::forGlobalActor(globalActor));
+        ImplicitActorHopTarget::forGlobalActor(globalActor),
+        /*FIXME if we get global distributed actors*/false);
       if (result == AsyncMarkingResult::FoundAsync)
         return false;
 
@@ -2197,39 +2205,21 @@ namespace {
         if (isolatedActor)
           return false;
 
+        // If we have a distributed actor that might be remote, check that
+        // we are referencing a properly-distributed member.
         bool performDistributedChecks =
             isolation.getActorType()->isDistributedActor() &&
-            !(isolatedActor.isActorSelf() &&
-              member->isInstanceMember() &&
-              isActorInitOrDeInitContext(getDeclContext())
-            );
-
+            !isolatedActor.isPotentiallyIsolated &&
+            !isa<ConstructorDecl>(member) &&
+            !isActorInitOrDeInitContext(getDeclContext());
         if (performDistributedChecks) {
-          if (auto func = dyn_cast<FuncDecl>(member)) {
-            if (func->isStatic()) {
-              // no additional checks for static functions
-            } else if (func->isDistributed()) {
-              tryMarkImplicitlyThrows(memberLoc, memberRef, context);
-              markNearestCallAsImplicitly(/*setAsync*/None, /*setThrows*/false,
-                                          /*setDistributedThunk*/true);
-
-            } else {
-              // neither static or distributed, apply full distributed isolation
-              ctx.Diags.diagnose(memberLoc, diag::distributed_actor_isolated_method)
-                  .fixItInsert(member->getAttributeInsertionLoc(true), "distributed ");
-              noteIsolatedActorMember(member, context);
-              return true;
-            }
-          } // end FuncDecl
-
-          if (isPropOrSubscript(member)) {
-              ctx.Diags.diagnose(
-                  memberLoc, diag::distributed_actor_isolated_non_self_reference,
-                  member->getDescriptiveKind(),
-                  member->getName());
-              noteIsolatedActorMember(member, context);
-              return true;
-          } // end VarDecl
+          if (auto access = checkDistributedAccess(memberLoc, member, context)){
+            // This is a distributed access, so mark it as throwing or
+            // using a distributed thunk as appropriate.
+            markNearestCallAsImplicitly(None, access->first, access->second);
+          } else {
+            return true;
+          }
         }
 
         return diagnoseNonSendableTypesInReference(
@@ -2261,55 +2251,27 @@ namespace {
           return true;
         }
 
-
-        // Distributed actor properties cannot be accessed cross-actor.
-        if (isolation.getActorType()->isDistributedActor()) {
-          if (auto func = dyn_cast<AbstractFunctionDecl>(member)) {
-            if (!func->isDistributed()) {
-              ctx.Diags.diagnose(memberLoc,
-                                 diag::distributed_actor_isolated_method);
-              noteIsolatedActorMember(member, context);
-              return true;
-            } else {
-              // it is some other member and since we only allow distributed
-              // funcs this definitely is distributed-actor-isolated
-              if (!func->isDistributed()) {
-                ctx.Diags.diagnose(
-                    memberLoc,
-                    diag::distributed_actor_isolated_non_self_reference,
-                    member->getDescriptiveKind(), member->getName());
-                noteIsolatedActorMember(member, context);
-                return true;
-              }
-            }
-          }
-
-          // It wasn't a distributed func, so ban the access
-          if (isPropOrSubscript(member)) {
-            ctx.Diags.diagnose(
-                memberLoc, diag::distributed_actor_isolated_non_self_reference,
-                member->getDescriptiveKind(), member->getName());
-            noteIsolatedActorMember(member, context);
-            return true;
-          }
-        }
-
         // Try implicit asynchronous access.
+        bool isDistributed = isolation.getActorType()->isDistributedActor() &&
+            !isolatedActor.isPotentiallyIsolated;
         auto implicitAsyncResult = tryMarkImplicitlyAsync(
             memberLoc, memberRef, context,
-            ImplicitActorHopTarget::forInstanceSelf());
-        if (isolation.getActorType()->isDistributedActor() &&
-            !isolatedActor.isPotentiallyIsolated) {
-          tryMarkImplicitlyThrows(memberLoc, memberRef, context);
-          markNearestCallAsImplicitly(/*setAsync*/None, /*setThrows*/false,
-                                      /*setDistributedThunk*/true);
-        }
+            ImplicitActorHopTarget::forInstanceSelf(),
+            isDistributed);
 
-        if (implicitAsyncResult == AsyncMarkingResult::FoundAsync)
-          return false; // no problems
-        else if (implicitAsyncResult == AsyncMarkingResult::NotSendable)
+        switch (implicitAsyncResult) {
+        case AsyncMarkingResult::FoundAsync:
+          return false;
+
+        case AsyncMarkingResult::NotSendable:
+        case AsyncMarkingResult::NotDistributed:
           return true;
 
+        case AsyncMarkingResult::NotFound:
+        case AsyncMarkingResult::SyncContext:
+          // Diagnose below.
+          break;
+        }
 
         // Complain about access outside of the isolation domain.
         auto useKind = static_cast<unsigned>(

test/Distributed/distributed_actor_isolation_and_tasks.swift

@@ -14,7 +14,7 @@ distributed actor Philosopher {
   let log: Logger
   // expected-note@-1{{distributed actor state is only available within the actor instance}}
   var variable = 12
-  var variable_fromDetach = 12 // expected-note{{distributed actor state is only available within the actor instance}}
+  var variable_fromDetach = 12
   let INITIALIZED: Int
   let outside: Int = 1
 
@@ -47,7 +47,9 @@ distributed actor Philosopher {
       // because we KNOW this is a local call -- and there is no transport in
       // between that will throw.
       _ = await self.dist() // notice lack of 'try' even though 'distributed func'
-      _ = self.variable_fromDetach // expected-error{{distributed actor-isolated property 'variable_fromDetach' can only be referenced inside the distributed actor}}
+      _ = self.variable_fromDetach // expected-error{{expression is 'async' but is not marked with 'await'}}
+      // expected-note@-1{{property access is 'async'}}
+      _ = await self.variable_fromDetach // okay, we know we're on the local node
     }
   }
 }
@@ -62,4 +64,4 @@ func test_outside(transport: ActorTransport) async throws {
 
 func test_outside_isolated(phil: isolated Philosopher) async throws {
   phil.log.info("works on isolated")
-}
+}