Merge pull request #36391 from kavon/actor-keypaths

swift-ci · web-flow · commit f1218fcc01a4 · 2021-03-11T21:31:13.000-08:00
diff --git a/include/swift/AST/DiagnosticsSema.def b/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",
diff --git a/include/swift/AST/Expr.h b/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:
diff --git a/lib/Sema/TypeCheckConcurrency.cpp b/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) {
diff --git a/test/Concurrency/Runtime/actor_keypaths.swift b/test/Concurrency/Runtime/actor_keypaths.swift
@@ -0,0 +1,77 @@
+// RUN: %target-run-simple-swift(-Xfrontend -enable-experimental-concurrency %import-libdispatch)
+
+// REQUIRES: executable_test
+// REQUIRES: concurrency
+// REQUIRES: libdispatch
+
+actor Page {
+    let initialNumWords : Int
+
+    @actorIndependent(unsafe)
+    var numWords : Int
+
+    init(_ words : Int) {
+        numWords = words
+        initialNumWords = words
+    }
+}
+
+actor Book {
+    let pages : [Page]
+
+    init(_ numPages : Int) {
+        var stack : [Page] = []
+        for i in 0 ..< numPages {
+            stack.append(Page(i))
+        }
+        pages = stack
+    }
+
+    @actorIndependent
+    subscript(_ page : Int) -> Page {
+        return pages[page]
+    }
+}
+
+func bookHasChanged(_ b : Book,
+                    currentGetter : KeyPath<Page, Int>,
+                    initialGetter : (Page) -> Int) -> Bool {
+    let numPages = b[keyPath: \.pages.count]
+
+    for i in 0 ..< numPages {
+        let pageGetter = \Book.[i]
+        let currentWords = pageGetter.appending(path: currentGetter)
+
+        if (b[keyPath: currentWords] != initialGetter(b[keyPath: pageGetter])) {
+            return true
+        }
+    }
+
+    return false
+}
+
+func enumeratePageKeys(from : Int, to : Int) -> [KeyPath<Book, Page>] {
+    var keys : [KeyPath<Book, Page>] = []
+    for i in from ..< to {
+        keys.append(\Book.[i])
+    }
+    return keys
+}
+
+func erasePages(_ book : Book, badPages: [KeyPath<Book, Page>]) {
+    for page in badPages {
+        book[keyPath: page].numWords = 0
+    }
+}
+
+let book = Book(100)
+
+if bookHasChanged(book, currentGetter: \Page.numWords, initialGetter: \Page.initialNumWords) {
+    fatalError("book should not be changed")
+}
+
+erasePages(book, badPages: enumeratePageKeys(from: 0, to: 100))
+
+guard bookHasChanged(book, currentGetter: \Page.numWords, initialGetter: \Page.initialNumWords) else {
+    fatalError("book should be wiped!")
+}
diff --git a/test/Concurrency/actor_isolation_objc.swift b/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}}
diff --git a/test/Concurrency/actor_keypath_isolation.swift b/test/Concurrency/actor_keypath_isolation.swift
@@ -0,0 +1,90 @@
+// RUN: %target-typecheck-verify-swift -enable-experimental-concurrency
+// REQUIRES: concurrency
+
+class Box {
+    let size : Int = 0
+}
+
+actor Door {
+    let immutable : Int = 0
+    let letBox : Box? = nil
+    let letDict : [Int : Box] = [:]
+    let immutableNeighbor : Door? = nil
+
+
+    var mutableNeighbor : Door? = nil
+    var varDict : [Int : Box] = [:]
+    var mutable : Int = 0
+    var varBox : Box = Box()
+    var getOnlyInt : Int {
+        get { 0 }
+    }
+    
+    @actorIndependent(unsafe) var unsafeIndependent : Int = 0
+
+    @MainActor var globActor_mutable : Int = 0
+    @MainActor let globActor_immutable : Int = 0
+
+    @MainActor(unsafe) var unsafeGlobActor_mutable : Int = 0
+    @MainActor(unsafe) let unsafeGlobActor_immutable : Int = 0
+
+    subscript(byIndex: Int) -> Int { 0 }
+
+    @MainActor subscript(byName: String) -> Int { 0 }
+
+    @actorIndependent subscript(byIEEE754: Double) -> Int { 0 }
+}
+
+func attemptAccess<T, V>(_ t : T, _ f : (T) -> V) -> V {
+    return f(t)
+}
+
+func tryKeyPathsMisc(d : Door) {
+    // as a func
+    _ = attemptAccess(d, \Door.mutable) // expected-error {{cannot form key path to actor-isolated property 'mutable'}}
+    _ = attemptAccess(d, \Door.immutable)
+    _ = attemptAccess(d, \Door.immutableNeighbor?.immutableNeighbor)
+
+    // in combination with other key paths
+
+    _ = (\Door.letBox).appending(path:  // expected-warning {{cannot form key path that accesses non-concurrent-value type 'Box?'}}
+                                       \Box?.?.size)
+
+    _ = (\Door.varBox).appending(path:  // expected-error {{cannot form key path to actor-isolated property 'varBox'}}
+                                       \Box.size)
+
+}
+
+func tryKeyPathsFromAsync() async {
+    _ = \Door.unsafeGlobActor_immutable
+    _ = \Door.unsafeGlobActor_mutable // expected-error{{cannot form key path to actor-isolated property 'unsafeGlobActor_mutable'}}
+}
+
+func tryNonConcurrentValue() {
+    _ = \Door.letDict[0] // expected-warning {{cannot form key path that accesses non-concurrent-value type '[Int : Box]'}}
+    _ = \Door.varDict[0] // expected-error {{cannot form key path to actor-isolated property 'varDict'}}
+    _ = \Door.letBox!.size // expected-warning {{cannot form key path that accesses non-concurrent-value type 'Box?'}}
+}
+
+func tryKeypaths() {
+    _ = \Door.unsafeGlobActor_immutable
+    _ = \Door.unsafeGlobActor_mutable
+
+    _ = \Door.immutable
+    _ = \Door.unsafeIndependent
+    _ = \Door.globActor_immutable
+    _ = \Door.[4.2]
+    _ = \Door.immutableNeighbor?.immutableNeighbor?.immutableNeighbor
+
+    _ = \Door.varBox // expected-error{{cannot form key path to actor-isolated property 'varBox'}}
+    _ = \Door.mutable  // expected-error{{cannot form key path to actor-isolated property 'mutable'}}
+    _ = \Door.getOnlyInt  // expected-error{{cannot form key path to actor-isolated property 'getOnlyInt'}}
+    _ = \Door.mutableNeighbor?.mutableNeighbor?.mutableNeighbor // expected-error 3 {{cannot form key path to actor-isolated property 'mutableNeighbor'}}
+
+    let _ : PartialKeyPath<Door> = \.mutable // expected-error{{cannot form key path to actor-isolated property 'mutable'}}
+    let _ : AnyKeyPath = \Door.mutable  // expected-error{{cannot form key path to actor-isolated property 'mutable'}}
+
+    _ = \Door.globActor_mutable // expected-error{{cannot form key path to actor-isolated property 'globActor_mutable'}}
+    _ = \Door.[0] // expected-error{{cannot form key path to actor-isolated subscript 'subscript(_:)'}}
+    _ = \Door.["hello"] // expected-error{{cannot form key path to actor-isolated subscript 'subscript(_:)'}}
+}
