Handle more Expr types in inout actor diags

We were handling weren't handling all of the expression types that can
show up as part of a parameter. As a result, the compiler was crashing
on correct code.

This patch introduces handling for far more expression types and the
appropriate tests for each.
Added handling includes:
 - Gentle optional unwrap
 - Forceful optional unwrap
 - Identity expressions (await, parens, etc)
 - Implicit conversion exprs

Author: etcwilde

lib/Sema/TypeCheckConcurrency.cpp

@@ -637,6 +637,42 @@ static bool isAsyncCall(const ApplyExpr *call) {
   return funcType->isAsync();
 }
 
+/// Extract the correct expression for diagnosing actor state being passed inout
+///
+/// This unwraps most expression types, working toward an offending DeclRefExpr
+/// or to the LookupExpr before the DeclRef.
+///
+/// The return will either be a LookupExpr or a DeclRefExpr.
+/// If part of the sub-expression isn't interesting, returns nullptr
+static const Expr *getInoutTopExpr(const InOutExpr *arg) {
+  const Expr * currentExpr = arg->getSubExpr();
+  while (currentExpr) {
+    if (const InOutExpr *inout = dyn_cast<InOutExpr>(currentExpr)) {
+      return nullptr; // The AST walker will hit this again
+    } else if (const LookupExpr *baseArg = dyn_cast<LookupExpr>(currentExpr)) {
+      if (isa<DeclRefExpr>(baseArg->getBase()))
+        return baseArg;
+      currentExpr = baseArg->getBase();
+    } else if (const DeclRefExpr *declRef = dyn_cast<DeclRefExpr>(currentExpr))
+      return declRef;
+    else if (const BindOptionalExpr *bindingOp =
+                 dyn_cast<BindOptionalExpr>(currentExpr))
+      currentExpr = bindingOp->getSubExpr();
+    else if (const ForceValueExpr *forceOp =
+                 dyn_cast<ForceValueExpr>(currentExpr))
+      currentExpr = forceOp->getSubExpr();
+    else if (const ImplicitConversionExpr *convExpr =
+                 dyn_cast<ImplicitConversionExpr>(currentExpr))
+      currentExpr = convExpr->getSubExpr();
+    else if (const IdentityExpr *identExpr =
+                 dyn_cast<IdentityExpr>(currentExpr))
+      currentExpr = identExpr->getSubExpr();
+    else
+      return currentExpr;
+  }
+  llvm_unreachable("The current expression is a nullptr!");
+}
+
 /// Determine whether we should diagnose data races within the current context.
 ///
 /// By default, we do this only in code that makes use of concurrency
@@ -958,29 +994,20 @@ 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();
+      const Expr *subArg = getInoutTopExpr(arg);
+      if (subArg == nullptr) // Subexpression isn't interesting
+        return false;
       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)) {
+      if (const 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!");
+      else if (const LookupExpr * member = dyn_cast<LookupExpr>(subArg))
+        valueDecl = member->getMember().getDecl();
+      else
+        llvm_unreachable("Unhandled expression type");
       auto isolation = ActorIsolationRestriction::forDeclaration(valueDecl);
       switch (isolation) {
       case ActorIsolationRestriction::Unrestricted:

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
@@ -98,8 +110,8 @@ extension TestActor {
 
   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 'async' function call}}
-    // expected-error@+1:71{{actor-isolated property 'nextPosition' cannot be passed 'inout' to 'async' function call}}
+    // expected-error@+2:38{{actor-isolated property 'nextPosition' cannot be passed 'inout' to 'async' function call}}
+    // expected-error@+1:58{{actor-isolated property 'nextPosition' cannot be passed 'inout' to 'async' function call}}
     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'}}
@@ -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()
+  }
+}