diff --git a/include/swift/AST/AnyFunctionRef.h b/include/swift/AST/AnyFunctionRef.h index 72236d1b229..aa1cef7c29a 100644 --- a/include/swift/AST/AnyFunctionRef.h +++ b/include/swift/AST/AnyFunctionRef.h @@ -181,6 +181,17 @@ public: return false; } + /// Whether this function is @concurrent. + bool isConcurrent() const { + if (!hasType()) + return false; + + if (auto *fnType = getType()->getAs()) + return fnType->isConcurrent(); + + return false; + } + bool isObjC() const { if (auto afd = TheFunction.dyn_cast()) { return afd->isObjC(); diff --git a/include/swift/AST/DiagnosticsSema.def b/include/swift/AST/DiagnosticsSema.def index 3e54d452108..b02ad9aa370 100644 --- 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 index 2edcd2b1f3a..a17902e974c 100644 --- a/lib/Sema/TypeCheckConcurrency.cpp +++ b/lib/Sema/TypeCheckConcurrency.cpp @@ -697,6 +697,12 @@ namespace { ConcurrentExecutionChecker concurrentExecutionChecker; + using MutableVarParent = llvm::PointerUnion; + + /// Mapping from mutable local variables to the parent expression, when + /// that parent is either a load or a inout expression. + llvm::SmallDenseMap 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(subExpr); + if (!declRef) + return; + + auto var = dyn_cast_or_null(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(expr)) { if (!applyStack.empty()) diagnoseInOutArg(applyStack.back(), inout, false); + + recordMutableVarParent(inout, inout->getSubExpr()); + } + + if (auto load = dyn_cast(expr)) { + recordMutableVarParent(load, load->getSubExpr()); } if (auto lookup = dyn_cast(expr)) { @@ -786,7 +825,8 @@ namespace { } if (auto declRef = dyn_cast(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(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(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()) + 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(enclosingDC)) enclosingBody = enclosingClosure->getBody(); + else if (auto enclosingTopLevelCode = dyn_cast(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(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 index a8752ebae70..40329aaa0f1 100644 --- 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,7 +142,8 @@ 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 } @@ -150,7 +151,7 @@ extension MyActor { 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 } @@ -158,7 +159,8 @@ extension MyActor { @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 } @@ -166,7 +168,8 @@ extension MyActor { 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 index 252fcd5df63..435904af777 100644 --- 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 index f1b02be6465..3d8d6f2f2cf 100644 --- 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 +}