Merge pull request swiftlang#35660 from DougGregor/mutating-concurrent-access-errors

Author: DougGregor

include/swift/AST/AnyFunctionRef.h

@@ -181,6 +181,17 @@ class AnyFunctionRef {
     return false;
   }
 
+  /// Whether this function is @concurrent.
+  bool isConcurrent() const {
+    if (!hasType())
+      return false;
+
+    if (auto *fnType = getType()->getAs<AnyFunctionType>())
+      return fnType->isConcurrent();
+
+    return false;
+  }
+
   bool isObjC() const {
     if (auto afd = TheFunction.dyn_cast<AbstractFunctionDecl *>()) {
       return afd->isObjC();

include/swift/AST/DiagnosticsSema.def

@@ -4256,9 +4256,8 @@ ERROR(global_actor_from_nonactor_context,none,
 ERROR(actor_isolated_partial_apply,none,
         "actor-isolated %0 %1 can not be partially applied",
         (DescriptiveDeclKind, DeclName))
-WARNING(concurrent_access_local,none,
-        "local %0 %1 is unsafe to reference in code that may execute "
-        "concurrently",
+ERROR(concurrent_access_local,none,
+        "use of local %0 %1 in concurrently-executing code",
         (DescriptiveDeclKind, DeclName))
 ERROR(actor_isolated_from_concurrent_closure,none,
         "actor-isolated %0 %1 cannot be referenced from a concurrent closure",
@@ -4275,6 +4274,9 @@ ERROR(actor_isolated_from_escaping_closure,none,
 ERROR(local_function_executed_concurrently,none,
       "concurrently-executed %0 %1 must be marked as '@concurrent'",
       (DescriptiveDeclKind, DeclName))
+ERROR(concurrent_mutation_of_local_capture,none,
+      "mutation of captured %0 %1 in concurrently-executing code",
+      (DescriptiveDeclKind, DeclName))
 NOTE(concurrent_access_here,none,
      "access in concurrently-executed code here", ())
 NOTE(actor_isolated_sync_func,none,

lib/Sema/TypeCheckConcurrency.cpp

@@ -697,6 +697,12 @@ namespace {
 
     ConcurrentExecutionChecker concurrentExecutionChecker;
 
+    using MutableVarParent = llvm::PointerUnion<InOutExpr *, LoadExpr *>;
+
+    /// Mapping from mutable local variables to the parent expression, when
+    /// that parent is either a load or a inout expression.
+    llvm::SmallDenseMap<DeclRefExpr *, MutableVarParent, 4> mutableLocalVarParent;
+
     const DeclContext *getDeclContext() const {
       return contextStack.back();
     }
@@ -709,6 +715,33 @@ namespace {
           useContext, defContext);
     }
 
+    /// If the subexpression is a reference to a mutable local variable from a
+    /// different context, record its parent. We'll query this as part of
+    /// capture semantics in concurrent functions.
+    void recordMutableVarParent(MutableVarParent parent, Expr *subExpr) {
+      auto declRef = dyn_cast<DeclRefExpr>(subExpr);
+      if (!declRef)
+        return;
+
+      auto var = dyn_cast_or_null<VarDecl>(declRef->getDecl());
+      if (!var)
+        return;
+
+      // Only mutable variables matter.
+      if (!var->supportsMutation())
+        return;
+
+      // Only mutable variables outside of the current context. This is an
+      // optimization, because the parent map won't be queried in this case, and
+      // it is the most common case for variables to be referenced in their
+      // own context.
+      if (var->getDeclContext() == getDeclContext())
+        return;
+
+      assert(mutableLocalVarParent[declRef].isNull());
+      mutableLocalVarParent[declRef] = parent;
+    }
+
   public:
     ActorIsolationChecker(const DeclContext *dc) : ctx(dc->getASTContext()) {
       contextStack.push_back(dc);
@@ -777,6 +810,12 @@ namespace {
       if (auto inout = dyn_cast<InOutExpr>(expr)) {
         if (!applyStack.empty())
           diagnoseInOutArg(applyStack.back(), inout, false);
+
+        recordMutableVarParent(inout, inout->getSubExpr());
+      }
+
+      if (auto load = dyn_cast<LoadExpr>(expr)) {
+        recordMutableVarParent(load, load->getSubExpr());
       }
 
       if (auto lookup = dyn_cast<LookupExpr>(expr)) {
@@ -786,7 +825,8 @@ namespace {
       }
 
       if (auto declRef = dyn_cast<DeclRefExpr>(expr)) {
-        checkNonMemberReference(declRef->getDeclRef(), declRef->getLoc());
+        checkNonMemberReference(
+            declRef->getDeclRef(), declRef->getLoc(), declRef);
         return { true, expr };
       }
 
@@ -865,6 +905,11 @@ namespace {
         applyStack.pop_back();
       }
 
+      // Clear out the mutable local variable parent map on the way out.
+      if (auto *declRefExpr = dyn_cast<DeclRefExpr>(expr)) {
+        mutableLocalVarParent.erase(declRefExpr);
+      }
+
       return expr;
     }
 
@@ -1216,7 +1261,8 @@ namespace {
     }
 
     /// Check a reference to a local or global.
-    bool checkNonMemberReference(ConcreteDeclRef valueRef, SourceLoc loc) {
+    bool checkNonMemberReference(
+        ConcreteDeclRef valueRef, SourceLoc loc, DeclRefExpr *declRefExpr) {
       if (!valueRef)
         return false;
 
@@ -1234,22 +1280,37 @@ namespace {
             value, loc, isolation.getGlobalActor());
 
       case ActorIsolationRestriction::LocalCapture:
-        if (!shouldDiagnoseExistingDataRaces(getDeclContext()))
+        // Check whether we are in a context that will not execute concurrently
+        // with the context of 'self'. If not, it's safe.
+        if (!mayExecuteConcurrentlyWith(
+                getDeclContext(), isolation.getLocalContext()))
           return false;
 
-        // Check whether we are in a context that will not execute concurrently
-        // with the context of 'self'.
-        if (mayExecuteConcurrentlyWith(
-                getDeclContext(), isolation.getLocalContext())) {
+        // Check whether this is a local variable, in which case we can
+        // determine whether it was captured by value.
+        if (auto var = dyn_cast<VarDecl>(value)) {
+          auto parent = mutableLocalVarParent[declRefExpr];
+
+          // If we have an immediate load of this variable, the by-value
+          // capture in a concurrent function will guarantee that this is
+          // safe.
+          if (parent.dyn_cast<LoadExpr *>())
+            return false;
+
+          // Otherwise, we have concurrent mutation. Complain.
           ctx.Diags.diagnose(
-              loc, diag::concurrent_access_local,
-              value->getDescriptiveKind(), value->getName());
-          value->diagnose(
-              diag::kind_declared_here, value->getDescriptiveKind());
+              loc, diag::concurrent_mutation_of_local_capture,
+              var->getDescriptiveKind(), var->getName());
           return true;
         }
 
-        return false;
+        // Concurrent access to some other local.
+        ctx.Diags.diagnose(
+            loc, diag::concurrent_access_local,
+            value->getDescriptiveKind(), value->getName());
+        value->diagnose(
+            diag::kind_declared_here, value->getDescriptiveKind());
+        return true;
 
       case ActorIsolationRestriction::Unsafe:
         return diagnoseReferenceToUnsafeGlobal(value, loc);
@@ -1536,6 +1597,10 @@ SourceLoc ConcurrentExecutionChecker::getConcurrentReferenceLoc(
     enclosingBody = enclosingFunc->getBody();
   else if (auto enclosingClosure = dyn_cast<ClosureExpr>(enclosingDC))
     enclosingBody = enclosingClosure->getBody();
+  else if (auto enclosingTopLevelCode = dyn_cast<TopLevelCodeDecl>(enclosingDC))
+    enclosingBody = enclosingTopLevelCode->getBody();
+  else
+    return SourceLoc();
 
   assert(enclosingBody && "Cannot have a local function here");
   ConcurrentLocalRefWalker walker(*this, localFunc);
@@ -1549,10 +1614,8 @@ bool ConcurrentExecutionChecker::mayExecuteConcurrentlyWith(
   // Walk the context chain from the use to the definition.
   while (useContext != defContext) {
     // If we find a concurrent closure... it can be run concurrently.
-    // NOTE: We also classify escaping closures this way, which detects more
-    // problematic cases.
     if (auto closure = dyn_cast<AbstractClosureExpr>(useContext)) {
-      if (isEscapingClosure(closure) || isConcurrentClosure(closure))
+      if (isConcurrentClosure(closure))
         return true;
     }
 
@@ -1963,18 +2026,21 @@ void swift::checkOverrideActorIsolation(ValueDecl *value) {
 static bool shouldDiagnoseExistingDataRaces(const DeclContext *dc) {
   while (!dc->isModuleScopeContext()) {
     if (auto closure = dyn_cast<AbstractClosureExpr>(dc)) {
-      // Async closures use concurrency features.
-      if (closure->getType() && closure->isBodyAsync())
-        return true;
+      // Async and concurrent closures use concurrency features.
+      if (auto closureType = closure->getType()) {
+        if (auto fnType = closureType->getAs<AnyFunctionType>())
+          if (fnType->isAsync() || fnType->isConcurrent())
+            return true;
+      }
     } else if (auto decl = dc->getAsDecl()) {
       // If any isolation attributes are present, we're using concurrency
       // features.
       if (getIsolationFromAttributes(decl, /*shouldDiagnose=*/false))
         return true;
 
       if (auto func = dyn_cast<AbstractFunctionDecl>(decl)) {
-        // Async functions use concurrency features.
-        if (func->hasAsync())
+        // Async and concurrent functions use concurrency features.
+        if (func->hasAsync() || func->isConcurrent())
           return true;
 
         // If there is an explicit @asyncHandler, we're using concurrency

test/Concurrency/actor_isolation.swift

@@ -2,7 +2,7 @@
 // REQUIRES: concurrency
 
 let immutableGlobal: String = "hello"
-var mutableGlobal: String = "can't touch this" // expected-note 3{{var declared here}}
+var mutableGlobal: String = "can't touch this" // expected-note 5{{var declared here}}
 
 func globalFunc() { }
 func acceptClosure<T>(_: () -> T) { }
@@ -128,7 +128,7 @@ extension MyActor {
 
     // Closures.
     let localConstant = 17
-    var localVar = 17 // expected-note 4{{var declared here}}
+    var localVar = 17
 
     // Non-escaping closures are okay.
     acceptClosure {
@@ -142,31 +142,34 @@ extension MyActor {
     acceptConcurrentClosure {
       _ = self.text[0] // expected-error{{actor-isolated property 'text' cannot be referenced from a concurrent closure}}
       _ = self.synchronous() // expected-error{{actor-isolated instance method 'synchronous()' cannot be referenced from a concurrent closure}}
-      _ = localVar // expected-warning{{local var 'localVar' is unsafe to reference in code that may execute concurrently}}
+      _ = localVar // okay
+      localVar = 25 // expected-error{{mutation of captured var 'localVar' in concurrently-executing code}}
       _ = localConstant
     }
 
     // Escaping closures might run concurrently.
     acceptEscapingClosure {
       _ = self.text[0] // expected-error{{actor-isolated property 'text' cannot be referenced from an '@escaping' closure}}
       _ = self.synchronous() // expected-error{{actor-isolated instance method 'synchronous()' cannot be referenced from an '@escaping' closure}}
-      _ = localVar // expected-warning{{local var 'localVar' is unsafe to reference in code that may execute concurrently}}
+      _ = localVar // okay, don't complain about escaping
       _ = localConstant
     }
 
     // Local functions might run concurrently.
     @concurrent func localFn1() {
       _ = self.text[0] // expected-error{{actor-isolated property 'text' cannot be referenced from a concurrent function}}
       _ = self.synchronous() // expected-error{{actor-isolated instance method 'synchronous()' cannot be referenced from a concurrent function}}
-      _ = localVar // expected-warning{{local var 'localVar' is unsafe to reference in code that may execute concurrently}}
+      _ = localVar // okay
+      localVar = 25 // expected-error{{mutation of captured var 'localVar' in concurrently-executing code}}
       _ = localConstant
     }
 
     @concurrent func localFn2() {
       acceptClosure {
         _ = text[0]  // expected-error{{actor-isolated property 'text' cannot be referenced from a concurrent function}}
         _ = self.synchronous() // expected-error{{actor-isolated instance method 'synchronous()' cannot be referenced from a concurrent function}}
-        _ = localVar // expected-warning{{local var 'localVar' is unsafe to reference in code that may execute concurrently}}
+        _ = localVar // okay
+        localVar = 25 // expected-error{{mutation of captured var 'localVar' in concurrently-executing code}}
         _ = localConstant
       }
     }
@@ -321,29 +324,39 @@ func testGlobalRestrictions(actor: MyActor) async {
   // Global mutable state cannot be accessed.
   _ = mutableGlobal // expected-warning{{reference to var 'mutableGlobal' is not concurrency-safe because it involves shared mutable state}}
 
-  // Local mutable variables cannot be accessed from concurrently-executing
+  // Local mutable variables cannot be modified from concurrently-executing
   // code.
-  var i = 17 // expected-note{{var declared here}}
-  acceptEscapingClosure {
-    i = 42 // expected-warning{{local var 'i' is unsafe to reference in code that may execute concurrently}}
+  var i = 17
+  acceptConcurrentClosure {
+    _ = i
+    i = 42 // expected-error{{mutation of captured var 'i' in concurrently-executing code}}
   }
   print(i)
 }
 
+func f() {
+  acceptConcurrentClosure {
+    _ = mutableGlobal // expected-warning{{reference to var 'mutableGlobal' is not concurrency-safe because it involves shared mutable state}}
+  }
+
+  @concurrent func g() {
+    _ = mutableGlobal // expected-warning{{reference to var 'mutableGlobal' is not concurrency-safe because it involves shared mutable state}}
+  }
+}
+
 // ----------------------------------------------------------------------
 // Local function isolation restrictions
 // ----------------------------------------------------------------------
 func checkLocalFunctions() async {
   var i = 0
-  var j = 0 // expected-note{{var declared here}}
+  var j = 0
 
   func local1() {
     i = 17
   }
 
   func local2() { // expected-error{{concurrently-executed local function 'local2()' must be marked as '@concurrent'}}{{3-3=@concurrent }}
-    j = 42 // expected-warning{{local var 'j' is unsafe to reference in code that may execute concurrently}}
-    // FIXME: the above should be an error as well
+    j = 42 // expected-error{{mutation of captured var 'j' in concurrently-executing code}}
   }
 
   // Okay to call locally.
@@ -357,22 +370,22 @@ func checkLocalFunctions() async {
   }
 
   // Escaping closures can make the local function execute concurrently.
-  acceptEscapingClosure {
+  acceptConcurrentClosure {
     local2() // expected-note{{access in concurrently-executed code here}}
   }
 
   print(i)
   print(j)
 
-  var k = 17 // expected-note{{var declared here}}
+  var k = 17
   func local4() {
-    acceptEscapingClosure {
+    acceptConcurrentClosure {
       local3() // expected-note{{access in concurrently-executed code here}}
     }
   }
 
   func local3() { // expected-error{{concurrently-executed local function 'local3()' must be marked as '@concurrent'}}
-    k = 25 // expected-warning{{local var 'k' is unsafe to reference in code that may execute concurrently}}
+    k = 25 // expected-error{{mutation of captured var 'k' in concurrently-executing code}}
   }
 
   print(k)

test/Concurrency/async_let_isolation.swift

@@ -17,9 +17,9 @@ actor class MyActor {
     async let z = synchronous()
     // expected-error @-1{{actor-isolated instance method 'synchronous()' cannot be referenced from 'async let' initializer}}
 
-    var localText = text // expected-note{{var declared here}}
+    var localText = text
     async let w = localText.removeLast()
-    // expected-warning@-1{{local var 'localText' is unsafe to reference in code that may execute concurrently}}
+    // expected-error@-1{{mutation of captured var 'localText' in concurrently-executing code}}
 
     _ = await x
     _ = await y