Merge pull request swiftlang#35099 from etcwilde/ewilde/disallow-passing-global-actor-state-inout

[Concurrency] Disallow passing global actor state inout

Author: etcwilde

lib/Sema/TypeCheckConcurrency.cpp

@@ -904,45 +904,58 @@ namespace {
         return false;
 
       Expr *subArg = arg->getSubExpr();
+      ValueDecl *valueDecl = nullptr;
       if (LookupExpr *baseArg = dyn_cast<LookupExpr>(subArg)) {
         while (LookupExpr *nextLayer = dyn_cast<LookupExpr>(baseArg->getBase()))
           baseArg = nextLayer;
         // subArg: the actual property being passed inout
         // baseArg: the property in the actor who's property is being passed
         // inout
 
-        auto memberDecl = baseArg->getMember().getDecl();
-        auto isolation = ActorIsolationRestriction::forDeclaration(memberDecl);
-        switch (isolation) {
-        case ActorIsolationRestriction::Unrestricted:
-        case ActorIsolationRestriction::LocalCapture:
-        case ActorIsolationRestriction::Unsafe:
-        case ActorIsolationRestriction::GlobalActor: // TODO: handle global
-                                                     // actors
-          break;
-        case ActorIsolationRestriction::ActorSelf: {
-          if (isPartialApply) {
-            // The partially applied InoutArg is a property of actor. This can
-            // really only happen when the property is a struct with a mutating
-            // async method.
-            if (auto partialApply = dyn_cast<ApplyExpr>(call->getFn())) {
-              ValueDecl *fnDecl =
-                  cast<DeclRefExpr>(partialApply->getFn())->getDecl();
-              ctx.Diags.diagnose(
-                  call->getLoc(), diag::actor_isolated_mutating_func,
-                  fnDecl->getName(), memberDecl->getDescriptiveKind(),
-                  memberDecl->getName());
-              return true;
-            }
-          } else {
+        valueDecl = baseArg->getMember().getDecl();
+      } else if (DeclRefExpr *declExpr = dyn_cast<DeclRefExpr>(subArg)) {
+        valueDecl = declExpr->getDecl();
+      } else {
+        llvm_unreachable("Inout argument is neither a lookup nor decl.");
+      }
+      assert(valueDecl != nullptr && "valueDecl was never set!");
+      auto isolation = ActorIsolationRestriction::forDeclaration(valueDecl);
+      switch (isolation) {
+      case ActorIsolationRestriction::Unrestricted:
+      case ActorIsolationRestriction::LocalCapture:
+      case ActorIsolationRestriction::Unsafe:
+        break;
+      case ActorIsolationRestriction::GlobalActor: {
+        ctx.Diags.diagnose(call->getLoc(),
+                           diag::actor_isolated_inout_state,
+                           valueDecl->getDescriptiveKind(),
+                           valueDecl->getName(),
+                           call->implicitlyAsync());
+        valueDecl->diagnose(diag::kind_declared_here,
+                            valueDecl->getDescriptiveKind());
+        return true;
+      }
+      case ActorIsolationRestriction::ActorSelf: {
+        if (isPartialApply) {
+          // The partially applied InoutArg is a property of actor. This can
+          // really only happen when the property is a struct with a mutating
+          // async method.
+          if (auto partialApply = dyn_cast<ApplyExpr>(call->getFn())) {
+            ValueDecl *fnDecl =
+                cast<DeclRefExpr>(partialApply->getFn())->getDecl();
             ctx.Diags.diagnose(
-                subArg->getLoc(), diag::actor_isolated_inout_state,
-                memberDecl->getDescriptiveKind(), memberDecl->getName(),
-                call->implicitlyAsync());
+                call->getLoc(), diag::actor_isolated_mutating_func,
+                fnDecl->getName(), valueDecl->getDescriptiveKind(),
+                valueDecl->getName());
             return true;
           }
+        } else {
+          ctx.Diags.diagnose(
+              subArg->getLoc(), diag::actor_isolated_inout_state,
+              valueDecl->getDescriptiveKind(), valueDecl->getName(), call->implicitlyAsync());
+          return true;
         }
-        }
+      }
       }
       return false;
     }

test/Concurrency/actor_inout_isolation.swift

@@ -131,3 +131,31 @@ extension TestActor {
     await other.modify(&value2)
   }
 }
+
+// Verify global actor protection
+
+@globalActor
+struct MyGlobalActor {
+  static let shared = TestActor()
+}
+
+@MyGlobalActor var number: Int = 0
+// expected-note@-1{{var declared here}}
+// expected-note@-2{{var declared here}}
+// expected-note@-3{{mutable state is only available within the actor instance}}
+
+// expected-error@+2{{actor-isolated var 'number' cannot be passed 'inout' to 'async' function call}}
+// expected-error@+1{{var 'number' isolated to global actor 'MyGlobalActor' can not be referenced from this context}}
+let _ = Task.runDetached { await { (_ foo: inout Int) async in foo += 1 }(&number) }
+
+// attempt to pass global state owned by the global actor to another async function
+// expected-error@+1{{actor-isolated var 'number' cannot be passed 'inout' to 'async' function call}}
+@MyGlobalActor func sneaky() async { await modifyAsynchronously(&number) }
+
+// It's okay to pass actor state inout to synchronous functions!
+
+func globalSyncFunction(_ foo: inout Int) { }
+@MyGlobalActor func globalActorSyncFunction(_ foo: inout Int) { }
+@MyGlobalActor func globalActorAsyncOkay() async { globalActorSyncFunction(&number) }
+@MyGlobalActor func globalActorAsyncOkay2() async { globalSyncFunction(&number) }
+@MyGlobalActor func globalActorSyncOkay() { globalSyncFunction(&number) }