implement actor key path typechecking

The rule is that you cannot form a key path to
any actor-isolated members. This avoids issues
with having to track whether a keypath does
something 'async' in the KeyPath type.

Author: kavon

include/swift/AST/DiagnosticsSema.def

@@ -4331,6 +4331,9 @@ ERROR(actor_isolated_from_async_let,none,
 ERROR(actor_isolated_from_escaping_closure,none,
         "actor-isolated %0 %1 cannot be %select{referenced|mutated|used 'inout'}2 from an '@escaping' closure",
         (DescriptiveDeclKind, DeclName, unsigned))
+ERROR(actor_isolated_keypath_component,none,
+      "cannot form key path to actor-isolated %0 %1",
+      (DescriptiveDeclKind, DeclName))
 ERROR(local_function_executed_concurrently,none,
       "concurrently-executed %0 %1 must be marked as '@concurrent'",
       (DescriptiveDeclKind, DeclName))
@@ -4377,6 +4380,9 @@ WARNING(non_concurrent_property_type,none,
 WARNING(non_concurrent_keypath_capture,none,
         "cannot form key path that captures non-concurrent-value type %0",
         (Type))
+WARNING(non_concurrent_keypath_access,none,
+        "cannot form key path that accesses non-concurrent-value type %0",
+        (Type))
 ERROR(non_concurrent_type_member,none,
       "%select{stored property %1|associated value %1}0 of "
       "'ConcurrentValue'-conforming %2 %3 has non-concurrent-value type %4",

include/swift/AST/Expr.h

@@ -5654,6 +5654,26 @@ class KeyPathExpr : public Expr {
       llvm_unreachable("unhandled kind");
     }
 
+    bool hasDeclRef() const {
+      switch (getKind()) {
+      case Kind::Property:
+      case Kind::Subscript:
+        return true;
+
+      case Kind::Invalid:
+      case Kind::UnresolvedProperty:
+      case Kind::UnresolvedSubscript:
+      case Kind::OptionalChain:
+      case Kind::OptionalWrap:
+      case Kind::OptionalForce:
+      case Kind::Identity:
+      case Kind::TupleElement:
+      case Kind::DictionaryKey:
+        return false;
+      }
+      llvm_unreachable("unhandled kind");
+    }
+
     ConcreteDeclRef getDeclRef() const {
       switch (getKind()) {
       case Kind::Property:

lib/Sema/TypeCheckConcurrency.cpp

@@ -1266,21 +1266,9 @@ namespace {
         }
       }
 
-      // Key paths require any captured values to be ConcurrentValue-conforming.
-      if (auto keyPath = dyn_cast<KeyPathExpr>(expr)) {
-        for (const auto &component : keyPath->getComponents()) {
-          auto indexExpr = component.getIndexExpr();
-          if (!indexExpr || !indexExpr->getType())
-            continue;
 
-          if (shouldDiagnoseNonConcurrentValueViolations(ctx.LangOpts) &&
-              !isConcurrentValueType(getDeclContext(), indexExpr->getType())) {
-            ctx.Diags.diagnose(
-                component.getLoc(), diag::non_concurrent_keypath_capture,
-                indexExpr->getType());
-          }
-        }
-      }
+      if (auto keyPath = dyn_cast<KeyPathExpr>(expr))
+        checkKeyPathExpr(keyPath);
 
       // The children of #selector expressions are not evaluated, so we do not
       // need to do isolation checking there. This is convenient because such
@@ -1868,6 +1856,89 @@ namespace {
       return true;
     }
 
+    ///
+    /// \return true iff a diagnostic was emitted
+    bool checkKeyPathExpr(KeyPathExpr *keyPath) {
+      bool diagnosed = false;
+
+      // returns None if it is not a 'let'-bound var decl. Otherwise,
+      // the bool indicates whether a diagnostic was emitted.
+      auto checkLetBoundVarDecl = [&](KeyPathExpr::Component const& component)
+                                                            -> Optional<bool> {
+        auto decl = component.getDeclRef().getDecl();
+        if (auto varDecl = dyn_cast<VarDecl>(decl)) {
+          if (varDecl->isLet()) {
+            auto type = component.getComponentType();
+            if (shouldDiagnoseNonConcurrentValueViolations(ctx.LangOpts)
+                && !isConcurrentValueType(getDeclContext(), type)) {
+              ctx.Diags.diagnose(
+                  component.getLoc(), diag::non_concurrent_keypath_access,
+                  type);
+              return true;
+            }
+            return false;
+          }
+        }
+        return None;
+      };
+
+      // check the components of the keypath.
+      for (const auto &component : keyPath->getComponents()) {
+        // The decl referred to by the path component cannot be within an actor.
+        if (component.hasDeclRef()) {
+          auto concDecl = component.getDeclRef();
+          auto isolation = ActorIsolationRestriction::forDeclaration(concDecl);
+
+          switch (isolation.getKind()) {
+          case ActorIsolationRestriction::Unsafe:
+          case ActorIsolationRestriction::Unrestricted:
+            break; // OK. Does not refer to an actor-isolated member.
+
+          case ActorIsolationRestriction::GlobalActorUnsafe:
+            // Only check if we're in code that's adopted concurrency features.
+            if (!shouldDiagnoseExistingDataRaces(getDeclContext()))
+              break; // do not check
+
+            LLVM_FALLTHROUGH; // otherwise, perform checking
+
+          case ActorIsolationRestriction::GlobalActor:
+          case ActorIsolationRestriction::CrossActorSelf:
+            // 'let'-bound decls with this isolation are OK, just check them.
+            if (auto wasLetBound = checkLetBoundVarDecl(component)) {
+              diagnosed = wasLetBound.getValue();
+              break;
+            }
+            LLVM_FALLTHROUGH; // otherwise, it's invalid so diagnose it.
+
+          case ActorIsolationRestriction::ActorSelf: {
+            auto decl = concDecl.getDecl();
+            ctx.Diags.diagnose(component.getLoc(),
+                               diag::actor_isolated_keypath_component,
+                               decl->getDescriptiveKind(), decl->getName());
+            diagnosed = true;
+            break;
+          }
+          }; // end switch
+        }
+
+        // Captured values in a path component must conform to ConcurrentValue.
+        // These captured values appear in Subscript, aka "index" components,
+        // such as \Type.dict[k] where k is a captured dictionary key.
+        if (auto indexExpr = component.getIndexExpr()) {
+          auto type = indexExpr->getType();
+          if (type && shouldDiagnoseNonConcurrentValueViolations(ctx.LangOpts)
+              && !isConcurrentValueType(getDeclContext(), type)) {
+            ctx.Diags.diagnose(
+                component.getLoc(), diag::non_concurrent_keypath_capture,
+                indexExpr->getType());
+            diagnosed = true;
+          }
+        }
+      }
+
+      return diagnosed;
+    }
+
     /// Check a reference to a local or global.
     bool checkNonMemberReference(
         ConcreteDeclRef valueRef, SourceLoc loc, DeclRefExpr *declRefExpr) {

test/Concurrency/actor_isolation_objc.swift

@@ -14,12 +14,22 @@ actor A {
   }
 
   func keypaths() {
+    _ = #keyPath(A.w) // expected-error {{'#keyPath' refers to non-'@objc' property 'w'}}
+
+    // expected-error@+1 {{cannot form key path to actor-isolated property 'x'}}
     _ = #keyPath(A.x) // expected-error{{argument of '#keyPath' refers to non-'@objc' property 'x'}}
+
+    // expected-error@+1 {{cannot form key path to actor-isolated property 'y'}}
     _ = #keyPath(A.y) // expected-error{{argument of '#keyPath' refers to non-'@objc' property 'y'}}
+
+    // expected-error@+1 {{cannot form key path to actor-isolated property 'computed'}}
     _ = #keyPath(A.computed) // expected-error{{argument of '#keyPath' refers to non-'@objc' property 'computed'}}
+
     _ = #keyPath(A.z)
   }
 
+  let w: Int = 0 // expected-note{{add '@objc' to expose this property to Objective-C}}
+
   var x: Int = 0 // expected-note{{add '@objc' to expose this property to Objective-C}}
 
   @objc var y: Int = 0 // expected-note{{add '@objc' to expose this property to Objective-C}}