Merge pull request #35325 from etcwilde/ewilde/handle-more-exprs-for-inout-checking

etcwilde · web-flow · commit 3efa18fd1c27 · 2021-01-26T19:37:15.000-08:00
[Concurrency] Handle more expr types for inout checking
diff --git a/lib/Sema/TypeCheckConcurrency.cpp b/lib/Sema/TypeCheckConcurrency.cpp
@@ -958,68 +958,69 @@ namespace {
     /// \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();
-      ValueDecl *valueDecl = nullptr;
-      if (auto binding = dyn_cast<BindOptionalExpr>(subArg))
-        subArg = binding->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
-
-        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(
-                call->getLoc(), diag::actor_isolated_mutating_func,
-                fnDecl->getName(), valueDecl->getDescriptiveKind(),
-                valueDecl->getName());
-            return true;
+      bool result = false;
+      auto checkDiagnostic = [this, call, isPartialApply,
+                              &result](ValueDecl *decl, SourceLoc argLoc) {
+        auto isolation = ActorIsolationRestriction::forDeclaration(decl);
+        switch (isolation) {
+        case ActorIsolationRestriction::Unrestricted:
+        case ActorIsolationRestriction::LocalCapture:
+        case ActorIsolationRestriction::Unsafe:
+          break;
+        case ActorIsolationRestriction::GlobalActor: {
+          ctx.Diags.diagnose(argLoc, diag::actor_isolated_inout_state,
+                             decl->getDescriptiveKind(), decl->getName(),
+                             call->implicitlyAsync());
+          decl->diagnose(diag::kind_declared_here, decl->getDescriptiveKind());
+          result = true;
+          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(), decl->getDescriptiveKind(),
+                                 decl->getName());
+              result = true;
+            }
+          } else {
+            ctx.Diags.diagnose(argLoc, diag::actor_isolated_inout_state,
+                               decl->getDescriptiveKind(), decl->getName(),
+                               call->implicitlyAsync());
+            result = true;
           }
-        } else {
-          ctx.Diags.diagnose(
-              subArg->getLoc(), diag::actor_isolated_inout_state,
-              valueDecl->getDescriptiveKind(), valueDecl->getName(), call->implicitlyAsync());
-          return true;
+          break;
         }
-      }
-      }
-      return false;
+        }
+      };
+      auto expressionWalker = [baseArg = arg->getSubExpr(),
+                               checkDiagnostic](Expr *expr) -> Expr * {
+        if (isa<InOutExpr>(expr))
+          return nullptr; // AST walker will hit this again
+        if (LookupExpr *lookup = dyn_cast<LookupExpr>(expr)) {
+          if (isa<DeclRefExpr>(lookup->getBase())) {
+            checkDiagnostic(lookup->getMember().getDecl(), baseArg->getLoc());
+            return nullptr; // Diagnosed. Don't keep walking
+          }
+        }
+        if (DeclRefExpr *declRef = dyn_cast<DeclRefExpr>(expr)) {
+          checkDiagnostic(declRef->getDecl(), baseArg->getLoc());
+          return nullptr; // Diagnosed. Don't keep walking
+        }
+        return expr;
+      };
+      arg->getSubExpr()->forEachChildExpr(expressionWalker);
+      return result;
     }
 
     /// Get the actor isolation of the innermost relevant context.
diff --git a/test/Concurrency/actor_inout_isolation.swift b/test/Concurrency/actor_inout_isolation.swift
@@ -14,6 +14,7 @@
 struct Point {
   var x: Int
   var y: Int
+  var z: Int? = nil
 
   mutating func setComponents(x: inout Int, y: inout Int) async {
     defer { (x, y) = (self.x, self.y) }
@@ -22,10 +23,12 @@ struct Point {
 }
 
 actor class TestActor {
+  // expected-note@+1{{mutable state is only available within the actor instance}}
   var position = Point(x: 0, y: 0)
   var nextPosition = Point(x: 0, y: 1)
   var value1: Int = 0
   var value2: Int = 1
+  var points: [Point] = []
 }
 
 func modifyAsynchronously(_ foo: inout Int) async { foo += 1 }
@@ -56,6 +59,15 @@ extension TestActor {
     // expected-error@+1{{actor-isolated property 'position' cannot be passed 'inout' to 'async' function call}}
     await modifyAsynchronously(&position.x)
   }
+
+  func nestedExprs() async {
+    // expected-error@+1{{actor-isolated property 'position' cannot be passed 'inout' to 'async' function call}}
+    await modifyAsynchronously(&position.z!)
+
+    // expected-error@+1{{actor-isolated property 'points' cannot be passed 'inout' to 'async' function call}}
+    await modifyAsynchronously(&points[0].z!)
+  }
+
 }
 
 // internal method call
@@ -132,6 +144,40 @@ extension TestActor {
   }
 }
 
+actor class MyActor {
+  var points: [Point] = []
+  var int: Int = 0
+  var maybeInt: Int?
+  var maybePoint: Point?
+  var myActor: TestActor = TestActor()
+
+  // Checking that various ways of unwrapping emit the right error messages at
+  // the right times and that illegal operations are caught
+  func modifyStuff() async {
+    // expected-error@+1{{actor-isolated property 'points' cannot be passed 'inout' to 'async' function call}}
+    await modifyAsynchronously(&points[0].x)
+    // expected-error@+1{{actor-isolated property 'points' cannot be passed 'inout' to 'async' function call}}
+    await modifyAsynchronously(&points[0].z!)
+    // expected-error@+1{{actor-isolated property 'int' cannot be passed 'inout' to 'async' function call}}
+    await modifyAsynchronously(&int)
+    // expected-error@+1{{actor-isolated property 'maybeInt' cannot be passed 'inout' to 'async' function call}}
+    await modifyAsynchronously(&maybeInt!)
+    // expected-error@+1{{actor-isolated property 'maybePoint' cannot be passed 'inout' to 'async' function call}}
+    await modifyAsynchronously(&maybePoint!.z!)
+    // expected-error@+1{{actor-isolated property 'int' cannot be passed 'inout' to 'async' function call}}
+    await modifyAsynchronously(&(int))
+
+    // This warning is emitted because this fails to typecheck before the
+    // async-ness is attached.
+    // expected-warning@+2{{no calls to 'async' functions occur within 'await' expression}}
+    // expected-error@+1{{cannot pass immutable value of type 'Int' as inout argument}}
+    await modifyAsynchronously(&(maybePoint?.z)!)
+    // expected-error@+2{{actor-isolated property 'position' can only be referenced inside the actor}}
+    // expected-error@+1{{actor-isolated property 'myActor' cannot be passed 'inout' to 'async' function call}}
+    await modifyAsynchronously(&myActor.position.x)
+  }
+}
+
 // Verify global actor protection
 
 @globalActor
@@ -159,3 +205,17 @@ func globalSyncFunction(_ foo: inout Int) { }
 @MyGlobalActor func globalActorAsyncOkay() async { globalActorSyncFunction(&number) }
 @MyGlobalActor func globalActorAsyncOkay2() async { globalSyncFunction(&number) }
 @MyGlobalActor func globalActorSyncOkay() { globalSyncFunction(&number) }
+
+// Gently unwrap things that are fine
+struct Cat {
+  mutating func meow() async { }
+}
+
+struct Dog {
+  var cat: Cat?
+
+  mutating func woof() async {
+    // This used to cause the compiler to crash, but should be fine
+    await cat?.meow()
+  }
+}
