[Concurrency] Diagnose mutating accesses to locals from concurrent code.

DougGregor · DougGregor · commit a554ad632b24 · 2021-01-29T14:27:24.000-08:00
Replace the existing warning about any access to a local variable from
concurrently-executing code with a more tailored error:
concurrently-executing code may read a mutable varable, but cannot
modify it. This is safe so long as we either always do by-value
captures in concurrent closures or we ensure that no mutation of that
variable can occur after the point of capture.

We'll follow up with one of those. For now... be careful out there.

Since we're promoting this to an error, narrow it down to concurrent
closures and local functions, dropping the assumption that escaping
closures "may execute concurrently."
diff --git a/include/swift/AST/AnyFunctionRef.h b/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();
diff --git a/include/swift/AST/DiagnosticsSema.def b/include/swift/AST/DiagnosticsSema.def
@@ -4253,9 +4253,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",
@@ -4272,6 +4271,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,
diff --git a/lib/Sema/TypeCheckConcurrency.cpp b/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;
     }
 
@@ -1215,7 +1260,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;
 
@@ -1233,22 +1279,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);
@@ -1535,6 +1596,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);
@@ -1548,10 +1613,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;
     }
 
diff --git a/test/Concurrency/actor_isolation.swift b/test/Concurrency/actor_isolation.swift
@@ -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,11 +324,12 @@ 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)
 }
@@ -335,15 +339,14 @@ func testGlobalRestrictions(actor: MyActor) async {
 // ----------------------------------------------------------------------
 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 +360,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)
diff --git a/test/Concurrency/async_let_isolation.swift b/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
diff --git a/test/attr/attr_concurrent.swift b/test/attr/attr_concurrent.swift
@@ -53,3 +53,43 @@ func closures() {
   acceptsConcurrent(closure1) // expected-error{{converting non-concurrent function value to '@concurrent (Int) -> Int' may introduce data races}}
 }
 
+// Mutation of captured locals from within @concurrent functions.
+extension Int {
+  mutating func makeNegative() {
+    self = -self
+  }
+
+  func printMe() {
+    print(self)
+  }
+}
+
+func mutationOfLocal() {
+  var localInt = 17
+  acceptsConcurrent { i in
+    // Non-mutating accesses are okay
+    print(localInt + 17)
+    localInt.printMe()
+
+    // Mutations of locally-defined variables are fine.
+    var localResult = localInt + 1
+    print(localResult)
+
+    _ = {
+      localResult = localResult + 1
+    }()
+
+    // Mutations of captured variables executing concurrently are bad.
+    localInt = 17 // expected-error{{mutation of captured var 'localInt' in concurrently-executing code}}
+    localInt += 1 // expected-error{{mutation of captured var 'localInt' in concurrently-executing code}}
+    localInt.makeNegative() // expected-error{{mutation of captured var 'localInt' in concurrently-executing code}}
+
+    _ = {
+      localInt = localInt + 12 // expected-error{{mutation of captured var 'localInt' in concurrently-executing code}}
+    }()
+
+    return i + localInt
+  }
+
+  localInt = 20
+}
