[Concurrency] Ban passing actor state via inout

Passing actor statte to async functions via inout parameters violates
automicity. This patch bans passing actor state via inout parameter to
async functions.

This removes the warning when passing a sub-property of a property of an
actor class and replaces it with an error message since passing actor
state inout is not allowed.

Author: etcwilde

include/swift/AST/DiagnosticsSema.def

@@ -4206,6 +4206,12 @@ ERROR(actor_isolated_self_independent_context,none,
         "actor-isolated %0 %1 can not be referenced from an "
         "'@actorIndependent' context",
         (DescriptiveDeclKind, DeclName))
+ERROR(actor_isolated_inout_state,none,
+      "actor-isolated %0 %1 cannot be passed 'inout' to asynchronous function",
+      (DescriptiveDeclKind, DeclName))
+ERROR(actor_isolated_mutating_func,none,
+      "cannot call mutating async function %0 on actor-isolated %1 %2",
+      (DeclName, DescriptiveDeclKind, DeclName))
 ERROR(actor_isolated_global_actor_context,none,
       "actor-isolated %0 %1 can not be referenced from context of global "
       "actor %2",

lib/Sema/TypeCheckConcurrency.cpp

@@ -604,6 +604,24 @@ findMemberReference(Expr *expr) {
   return None;
 }
 
+/// Return true if the callee of an ApplyExpr is async
+///
+/// Note that this must be called after the implicitlyAsync flag has been set,
+/// or implicitly async calls will not return the correct value.
+static bool isAsyncCall(const ApplyExpr *call) {
+  if (call->implicitlyAsync())
+    return true;
+
+  // Effectively the same as doing a
+  // `cast_or_null<FunctionType>(call->getFn()->getType())`, check the
+  // result of that and then checking `isAsync` if it's defined.
+  Type funcTypeType = call->getFn()->getType();
+  if (!funcTypeType)
+    return false;
+  FunctionType *funcType = funcTypeType->castTo<FunctionType>();
+  return funcType->isAsync();
+}
+
 namespace {
   /// Check for adherence to the actor isolation rules, emitting errors
   /// when actor-isolated declarations are used in an unsafe manner.
@@ -672,6 +690,11 @@ namespace {
         return { true, expr };
       }
 
+      if (auto inout = dyn_cast<InOutExpr>(expr)) {
+        if (!applyStack.empty())
+          diagnoseInOutArg(applyStack.back(), inout, false);
+      }
+
       if (auto lookup = dyn_cast<LookupExpr>(expr)) {
         checkMemberReference(lookup->getBase(), lookup->getMember(),
                              lookup->getLoc());
@@ -713,11 +736,22 @@ namespace {
         Expr *fn = call->getFn()->getValueProvidingExpr();
         if (auto memberRef = findMemberReference(fn)) {
           checkMemberReference(
-              call->getArg(), memberRef->first, memberRef->second, 
+              call->getArg(), memberRef->first, memberRef->second,
               /*isEscapingPartialApply=*/false, /*maybeImplicitAsync=*/true);
 
           call->getArg()->walk(*this);
 
+          if (applyStack.size() >= 2) {
+            ApplyExpr *outerCall = applyStack[applyStack.size() - 2];
+            if (isAsyncCall(outerCall)) {
+              // This call is a partial application within an async call.
+              // If the partial application take a value inout, it is bad.
+              if (InOutExpr *inoutArg = dyn_cast<InOutExpr>(
+                      call->getArg()->getSemanticsProvidingExpr()))
+                diagnoseInOutArg(outerCall, inoutArg, true);
+            }
+          }
+
           // manual clean-up since normal traversal is skipped
           assert(applyStack.back() == dyn_cast<ApplyExpr>(expr));
           applyStack.pop_back();
@@ -853,6 +887,58 @@ namespace {
       return true;
     }
 
+    /// Diagnose an inout argument passed into an async call
+    ///
+    /// \returns true if we diagnosed the entity, \c false otherwise.
+    bool diagnoseInOutArg(const ApplyExpr *call, const InOutExpr *arg,
+                          bool isPartialApply) {
+      // check that the call is actually async
+      if (!isAsyncCall(call))
+        return false;
+
+      Expr *subArg = arg->getSubExpr();
+      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 {
+            ctx.Diags.diagnose(
+                subArg->getLoc(), diag::actor_isolated_inout_state,
+                memberDecl->getDescriptiveKind(), memberDecl->getName());
+            return true;
+          }
+        }
+        }
+      }
+      return false;
+    }
+
     /// Get the actor isolation of the innermost relevant context.
     ActorIsolation getInnermostIsolatedContext(const DeclContext *constDC) {
       // Retrieve the actor isolation for a declaration somewhere in our
@@ -1138,7 +1224,9 @@ namespace {
         llvm_unreachable("Locals cannot be referenced with member syntax");
 
       case ActorIsolationRestriction::Unsafe:
-        return diagnoseReferenceToUnsafe(member, memberLoc);
+        // This case is hit when passing actor state inout to functions in some
+        // cases. The error is emitted by diagnoseInOutArg.
+        return false;
       }
       llvm_unreachable("unhandled actor isolation kind!");
     }

test/Concurrency/actor_inout_isolation.swift

@@ -0,0 +1,110 @@
+// RUN: %target-typecheck-verify-swift -enable-experimental-concurrency
+// REQUIRES: concurrency
+
+// Verify that we don't allow actor-isolated state to be passed via inout
+// Check:
+//  - can't pass it into a normal async function
+//  - can't pass it into a first-class async function as a value
+//  - can't pass it into another actor method
+//  - can't pass it into a curried/partially applied function
+//  - can't pass it inout to a function that doesn't directly touch it
+//  - can't pass it into a function that was passed into the calling method
+//  - can't call async mutating function on actor isolated state
+
+struct Point {
+  var x: Int
+  var y: Int
+
+  mutating func setComponents(x: inout Int, y: inout Int) async {
+    defer { (x, y) = (self.x, self.y) }
+    (self.x, self.y) = (x, y)
+  }
+}
+
+actor class TestActor {
+  var position = Point(x: 0, y: 0)
+  var nextPosition = Point(x: 0, y: 1)
+  var value1: Int = 0
+  var value2: Int = 1
+}
+
+func modifyAsynchronously(_ foo: inout Int) async { foo += 1 }
+let modifyAsyncValue = modifyAsynchronously
+
+// external function call
+extension TestActor {
+
+  // Can't pass actor-isolated primitive into a function
+  func inoutAsyncFunctionCall() async {
+    // expected-error@+1{{actor-isolated property 'value1' cannot be passed 'inout' to asynchronous function}}
+    await modifyAsynchronously(&value1)
+  }
+
+  func inoutAsyncClosureCall() async {
+    // expected-error@+1{{actor-isolated property 'value1' cannot be passed 'inout' to asynchronous function}}
+    await { (_ foo: inout Int) async in foo += 1 }(&value1)
+  }
+
+  // Can't pass actor-isolated primitive into first-class function value
+  func inoutAsyncValueCall() async {
+    // expected-error@+1{{actor-isolated property 'value1' cannot be passed 'inout' to asynchronous function}}
+    await modifyAsyncValue(&value1)
+  }
+
+  // Can't pass property of actor-isolated state inout to async function
+  func inoutPropertyStateValueCall() async {
+    // expected-error@+1{{actor-isolated property 'position' cannot be passed 'inout' to asynchronous function}}
+    await modifyAsynchronously(&position.x)
+  }
+}
+
+// internal method call
+extension TestActor {
+  func modifyByValue(_ other: inout Int) async {
+    other += value1
+  }
+
+  func passStateIntoMethod() async {
+    // expected-error@+1{{actor-isolated property 'value1' cannot be passed 'inout' to asynchronous function}}
+    await modifyByValue(&value1)
+  }
+}
+
+// external class method call
+class NonAsyncClass {
+  func modifyOtherAsync(_ other : inout Int) async {
+    // ...
+  }
+
+  func modifyOtherNotAsync(_ other: inout Int) {
+    // ...
+  }
+}
+
+// Calling external class/struct async function
+extension TestActor {
+  // Can't pass state into async method of another class
+
+  func passStateIntoDifferentClassMethod() async {
+    let other = NonAsyncClass()
+    let otherCurry = other.modifyOtherAsync
+    // expected-error@+1{{actor-isolated property 'value2' cannot be passed 'inout' to asynchronous function}}
+    await other.modifyOtherAsync(&value2)
+    // expected-error@+1{{actor-isolated property 'value1' cannot be passed 'inout' to asynchronous function}}
+    await otherCurry(&value1)
+    other.modifyOtherNotAsync(&value2) // This is okay since it's not async!
+
+  }
+
+  func callMutatingFunctionOnStruct() async {
+    // expected-error@+3:20{{cannot call mutating async function 'setComponents(x:y:)' on actor-isolated property 'position'}}
+    // expected-error@+2:51{{actor-isolated property 'nextPosition' cannot be passed 'inout' to asynchronous function}}
+    // expected-error@+1:71{{actor-isolated property 'nextPosition' cannot be passed 'inout' to asynchronous function}}
+    await position.setComponents(x: &nextPosition.x, y: &nextPosition.y)
+
+    // expected-error@+3:20{{cannot call mutating async function 'setComponents(x:y:)' on actor-isolated property 'position'}}
+    // expected-error@+2:38{{actor-isolated property 'value1' cannot be passed 'inout' to asynchronous function}}
+    // expected-error@+1:50{{actor-isolated property 'value2' cannot be passed 'inout' to asynchronous function}}
+    await position.setComponents(x: &value1, y: &value2)
+  }
+}