averello/swift-mirror
1
0
Fork 0
mirror of https://github.com/apple/swift.git synced 2025-12-14 20:36:38 +01:00
Code Issues Packages Projects Releases Wiki Activity

Fix a bunch of bugs with the isolation of local funcs. Since we

Browse Source 
use local funcs to implement `defer`, this also fixes several
bugs with that feature, such as it breaking in nonisolated
functions when a default isolation is in effect in the source file.

Change how we compute isolation of local funcs. The rule here is
supposed to be that non-`@Sendable` local funcs are isolated the
same as their enclosing context. Unlike closure expressions, this
is unconditional: in instance-isolated functions, the isolation
does not depend on whether `self` is captured. But the computation
was wrong: it didn't translate global actor isolation between
contexts, it didn't turn parameter isolation into capture isolation,
and it fell through for several other kinds of parent isolation,
causing the compiler to try to apply default isolation instead.
I've extracted the logic from the closure expression path into a
common function and used it for both paths.

The capture computation logic was forcing a capture of the
enclosing isolation in local funcs, but only for async functions.
Presumably this was conditional because async functions need the
isolation for actor hops, but sync functions don't really need it.
However, this was causing crashes with `-enable-actor-data-race-checks`.
(I didn't investigate whether it also failed with the similar
assertion we do with preconcurrency.) For now, I've switched this
to capture the isolated instance unconditionally. If we need to
be more conservative by either only capturing when data-race checks
are enabled or disabling the checks when the isolation isn't captured,
we can look into that.

Fix a bug in capture isolation checking. We were ignoring captures
of nonisolated declarations in order to implement the rule that
permits `nonisolated(unsafe)` variables to be captured in
non-sendable closures. This check needs to only apply to variables!
The isolation of a local func has nothing to do with its sendability
as a capture.

That fix exposed a problem where we were being unnecessarily
restrictive with generic local func declarations because we didn't
consider them to have sendable type. This was true even if the
genericity was purely from being declared in a generic context,
but it doesn't matter, they ought to be sendable regardless.

Finally, fix a handful of bugs where global actor types were not
remapped properly in SILGen.
This commit is contained in:
John McCall
2025-06-27 19:47:44 -04:00
parent 2eee30dfbe
commit 3439e0caab
10 changed files with 217 additions and 114 deletions
Download Patch File Download Diff File Expand all files Collapse all files

24
include/swift/AST/ActorIsolation.h
View File

@@ -154,7 +154,8 @@ public:
explicit ActorIsolation(Kind kind = Unspecified, bool isSILParsed = false)
: pointer(nullptr), kind(kind), isolatedByPreconcurrency(false),
silParsed(isSILParsed), encodedParameterIndex(0) {
assert(kind != ActorInstance);
// SIL's use of this has weaker invariants for now.
assert(kind != ActorInstance || isSILParsed);
}
static ActorIsolation forUnspecified() {
@@ -209,21 +210,14 @@ public:
static std::optional<ActorIsolation> forSILString(StringRef string) {
auto kind =
llvm::StringSwitch<std::optional<ActorIsolation::Kind>>(string)
.Case("unspecified",
std::optional<ActorIsolation>(ActorIsolation::Unspecified))
.Case("actor_instance",
std::optional<ActorIsolation>(ActorIsolation::ActorInstance))
.Case("nonisolated",
std::optional<ActorIsolation>(ActorIsolation::Nonisolated))
.Case("nonisolated_unsafe", std::optional<ActorIsolation>(
ActorIsolation::NonisolatedUnsafe))
.Case("global_actor",
std::optional<ActorIsolation>(ActorIsolation::GlobalActor))
.Case("global_actor_unsafe",
std::optional<ActorIsolation>(ActorIsolation::GlobalActor))
.Case("unspecified", ActorIsolation::Unspecified)
.Case("actor_instance", ActorIsolation::ActorInstance)
.Case("nonisolated", ActorIsolation::Nonisolated)
.Case("nonisolated_unsafe", ActorIsolation::NonisolatedUnsafe)
.Case("global_actor", ActorIsolation::GlobalActor)
.Case("global_actor_unsafe", ActorIsolation::GlobalActor)
.Case("caller_isolation_inheriting",
std::optional<ActorIsolation>(
ActorIsolation::CallerIsolationInheriting))
ActorIsolation::CallerIsolationInheriting)
.Default(std::nullopt);
if (kind == std::nullopt)
return std::nullopt;

3
lib/AST/ActorIsolation.cpp
View File

@@ -191,6 +191,9 @@ bool ActorIsolation::isEqual(const ActorIsolation &lhs,
auto *lhsActor = lhs.getActorInstance();
auto *rhsActor = rhs.getActorInstance();
if (lhsActor && rhsActor) {
if (lhsActor == rhsActor)
return true;
// FIXME: This won't work for arbitrary isolated parameter captures.
if ((lhsActor->isSelfParameter() && rhsActor->isSelfParamCapture()) ||
(lhsActor->isSelfParamCapture() && rhsActor->isSelfParameter())) {

13
lib/AST/ConformanceLookup.cpp
View File

@@ -269,8 +269,13 @@ static ProtocolConformanceRef getBuiltinTupleTypeConformance(
return ProtocolConformanceRef::forMissingOrInvalid(type, protocol);
}
// We can end up checking builtin conformances for generic function types
// when e.g. we're checking whether a captured local func declaration is
// sendable. That's fine, we can answer that question in the abstract
// without needing to generally support conformances on generic function
// types.
using EitherFunctionType =
llvm::PointerUnion<const SILFunctionType *, const FunctionType *>;
llvm::PointerUnion<const SILFunctionType *, const AnyFunctionType *>;
/// Whether the given function type conforms to Sendable.
static bool isSendableFunctionType(EitherFunctionType eitherFnTy) {
@@ -288,7 +293,7 @@ static bool isSendableFunctionType(EitherFunctionType eitherFnTy) {
representation = *converted;
} else {
auto functionType = eitherFnTy.get<const FunctionType *>();
auto functionType = eitherFnTy.get<const AnyFunctionType *>();
if (functionType->isSendable())
return true;
@@ -332,7 +337,7 @@ static bool isBitwiseCopyableFunctionType(EitherFunctionType eitherFnTy) {
if (auto silFnTy = eitherFnTy.dyn_cast<const SILFunctionType *>()) {
representation = silFnTy->getRepresentation();
} else {
auto fnTy = eitherFnTy.get<const FunctionType *>();
auto fnTy = eitherFnTy.get<const AnyFunctionType *>();
representation = convertRepresentation(fnTy->getRepresentation());
}
@@ -621,7 +626,7 @@ LookupConformanceInModuleRequest::evaluate(
}
// Function types can conform to protocols.
if (auto functionType = type->getAs<FunctionType>()) {
if (auto functionType = type->getAs<AnyFunctionType>()) {
return getBuiltinFunctionTypeConformance(type, functionType, protocol);
}

23
lib/SILGen/SILGenConcurrency.cpp
View File

@@ -195,7 +195,8 @@ void SILGenFunction::emitExpectedExecutorProlog() {
case ActorIsolation::GlobalActor:
if (F.isAsync() || wantDataRaceChecks) {
setExpectedExecutorForGlobalActor(*this, actorIsolation.getGlobalActor());
auto globalActorType = F.mapTypeIntoContext(actorIsolation.getGlobalActor());
setExpectedExecutorForGlobalActor(*this, globalActorType);
}
break;
}
@@ -226,7 +227,8 @@ void SILGenFunction::emitExpectedExecutorProlog() {
case ActorIsolation::GlobalActor:
if (wantExecutor) {
setExpectedExecutorForGlobalActor(*this, actorIsolation.getGlobalActor());
auto globalActorType = F.mapTypeIntoContext(actorIsolation.getGlobalActor());
setExpectedExecutorForGlobalActor(*this, globalActorType);
break;
}
}
@@ -573,9 +575,10 @@ SILGenFunction::emitFunctionTypeIsolation(SILLocation loc,
// Emit global actor isolation by loading .shared from the global actor,
// erasing it into `any Actor`, and injecting that into Optional.
case FunctionTypeIsolation::Kind::GlobalActor:
case FunctionTypeIsolation::Kind::GlobalActor: {
return emitGlobalActorIsolation(loc,
isolation.getGlobalActorType()->getCanonicalType());
}
// Emit @isolated(any) isolation by loading the actor reference from the
// function.
@@ -646,9 +649,11 @@ SILGenFunction::emitClosureIsolation(SILLocation loc, SILDeclRef constant,
case ActorIsolation::Erased:
llvm_unreachable("closures cannot directly have erased isolation");
case ActorIsolation::GlobalActor:
return emitGlobalActorIsolation(loc,
isolation.getGlobalActor()->getCanonicalType());
case ActorIsolation::GlobalActor: {
auto globalActorType = F.mapTypeIntoContext(isolation.getGlobalActor())
->getCanonicalType();
return emitGlobalActorIsolation(loc, globalActorType);
}
case ActorIsolation::ActorInstance: {
assert(isolation.isActorInstanceForCapture());
@@ -703,8 +708,10 @@ SILGenFunction::emitExecutor(SILLocation loc, ActorIsolation isolation,
return emitLoadActorExecutor(loc, self);
}
case ActorIsolation::GlobalActor:
return emitLoadGlobalActorExecutor(isolation.getGlobalActor());
case ActorIsolation::GlobalActor: {
auto globalActorType = F.mapTypeIntoContext(isolation.getGlobalActor());
return emitLoadGlobalActorExecutor(globalActorType);
}
}
llvm_unreachable("covered switch");
}

2
lib/Sema/TypeCheckCaptures.cpp
View File

@@ -761,7 +761,7 @@ CaptureInfo CaptureInfoRequest::evaluate(Evaluator &evaluator,
finder.checkType(type, AFD->getLoc());
}
if (AFD->isLocalCapture() && AFD->hasAsync()) {
if (AFD->isLocalCapture()) {
// If a local function inherits isolation from the enclosing context,
// make sure we capture the isolated parameter, if we haven't already.
auto actorIsolation = getActorIsolation(AFD);

169
lib/Sema/TypeCheckConcurrency.cpp
View File

@@ -3029,13 +3029,13 @@ namespace {
// If the closure won't execute concurrently with the context in
// which the declaration occurred, it's okay.
auto decl = capture.getDecl();
auto isolation = getActorIsolation(decl);
// 'nonisolated' local variables are always okay to capture in
// 'Sendable' closures because they can be accessed from anywhere.
// Note that only 'nonisolated(unsafe)' can be applied to local
// variables.
if (isolation.isNonisolated())
if (isa<VarDecl>(decl) &&
getActorIsolation(decl).isNonisolated())
continue;
auto *context = localFunc.getAsDeclContext();
@@ -4815,6 +4815,70 @@ namespace {
};
} // end anonymous namespace
/// Compute the isolation of a closure or local function from its parent
/// isolation.
///
/// Doesn't apply preconcurrency because it's generally easier for the
/// caller to do so.
static ActorIsolation
computeClosureIsolationFromParent(DeclContext *closure,
ActorIsolation parentIsolation,
bool checkIsolatedCapture) {
// We must have parent isolation determined to get here.
switch (parentIsolation) {
case ActorIsolation::CallerIsolationInheriting:
case ActorIsolation::Nonisolated:
case ActorIsolation::NonisolatedUnsafe:
case ActorIsolation::Unspecified:
return ActorIsolation::forNonisolated(
parentIsolation == ActorIsolation::NonisolatedUnsafe);
case ActorIsolation::Erased:
llvm_unreachable("context cannot have erased isolation");
case ActorIsolation::GlobalActor:
// This should already be an interface type, so we don't need to remap
// it between the contexts.
return ActorIsolation::forGlobalActor(parentIsolation.getGlobalActor());
case ActorIsolation::ActorInstance: {
// In non-@Sendable local functions, we always inherit the enclosing
// isolation, forcing a capture of it if necessary.
if (isa<FuncDecl>(closure)) {
// We should always have a VarDecl in this case, where we got the
// ActorIsolation from a context; the non-VarDecl cases are only used
// locally within isolation checking.
auto actor = parentIsolation.getActorInstance();
assert(actor);
return ActorIsolation::forActorInstanceCapture(actor);
}
if (checkIsolatedCapture) {
auto closureAsFn = AnyFunctionRef::fromFunctionDeclContext(closure);
if (auto param = closureAsFn.getCaptureInfo().getIsolatedParamCapture())
return ActorIsolation::forActorInstanceCapture(param);
auto *explicitClosure = dyn_cast<ClosureExpr>(closure);
// @_inheritActorContext(always) forces the isolation capture.
if (explicitClosure && explicitClosure->alwaysInheritsActorContext()) {
if (parentIsolation.isActorInstanceIsolated()) {
if (auto *param = parentIsolation.getActorInstance())
return ActorIsolation::forActorInstanceCapture(param);
}
return parentIsolation;
}
} else {
// If we don't have capture information during code completion, assume
// that the closure captures the `isolated` parameter from the parent
// context.
return parentIsolation;
}
return ActorIsolation::forNonisolated(/*unsafe=*/false);
}
}
}
ActorIsolation ActorIsolationChecker::determineClosureIsolation(
AbstractClosureExpr *closure) const {
bool preconcurrency = false;
@@ -4871,48 +4935,8 @@ ActorIsolation ActorIsolationChecker::determineClosureIsolation(
closure->getParent(), getClosureActorIsolation);
preconcurrency |= parentIsolation.preconcurrency();
// We must have parent isolation determined to get here.
switch (parentIsolation) {
case ActorIsolation::CallerIsolationInheriting:
case ActorIsolation::Nonisolated:
case ActorIsolation::NonisolatedUnsafe:
case ActorIsolation::Unspecified:
return ActorIsolation::forNonisolated(
parentIsolation == ActorIsolation::NonisolatedUnsafe);
case ActorIsolation::Erased:
llvm_unreachable("context cannot have erased isolation");
case ActorIsolation::GlobalActor: {
Type globalActor = closure->mapTypeIntoContext(
parentIsolation.getGlobalActor()->mapTypeOutOfContext());
return ActorIsolation::forGlobalActor(globalActor);
}
case ActorIsolation::ActorInstance: {
if (checkIsolatedCapture) {
if (auto param = closure->getCaptureInfo().getIsolatedParamCapture())
return ActorIsolation::forActorInstanceCapture(param);
auto *explicitClosure = dyn_cast<ClosureExpr>(closure);
// @_inheritActorContext(always) forces the isolation capture.
if (explicitClosure && explicitClosure->alwaysInheritsActorContext()) {
if (parentIsolation.isActorInstanceIsolated()) {
if (auto *param = parentIsolation.getActorInstance())
return ActorIsolation::forActorInstanceCapture(param);
}
return parentIsolation;
}
} else {
// If we don't have capture information during code completion, assume
// that the closure captures the `isolated` parameter from the parent
// context.
return parentIsolation;
}
return ActorIsolation::forNonisolated(/*unsafe=*/false);
}
}
return computeClosureIsolationFromParent(closure, parentIsolation,
checkIsolatedCapture);
}();
// Apply computed preconcurrency.
@@ -5109,7 +5133,8 @@ getIsolationFromAttributes(const Decl *decl, bool shouldDiagnose = true,
std::optional<ActorIsolation> result;
if (selfTypeDecl) {
if (selfTypeDecl->isAnyActor()) {
result = ActorIsolation::forActorInstanceSelf(selfTypeDecl);
result = ActorIsolation::forActorInstanceSelf(
const_cast<AbstractFunctionDecl*>(cast<AbstractFunctionDecl>(decl)));
} else {
// If the declaration is in an extension that has one of the isolation
// attributes, use that.
@@ -6227,11 +6252,18 @@ static InferredActorIsolation computeActorIsolation(Evaluator &evaluator,
return inferred;
};
// If this is an accessor, use the actor isolation of its storage
// declaration. All of the logic for FuncDecls below only applies to
// non-accessor functions.
if (auto accessor = dyn_cast<AccessorDecl>(value)) {
return getInferredActorIsolation(accessor->getStorage());
}
// If this is a local function, inherit the actor isolation from its
// context if it global or was captured.
if (auto func = dyn_cast<FuncDecl>(value)) {
if (func->isLocalCapture() && !func->isSendable()) {
auto *dc = func->getDeclContext();
auto *dc = func->getDeclContext();
if (dc->isLocalContext() && !func->isSendable()) {
llvm::PointerUnion<Decl *, AbstractClosureExpr *> inferenceSource;
if (auto *closure = dyn_cast<AbstractClosureExpr>(dc)) {
inferenceSource = closure;
@@ -6239,38 +6271,18 @@ static InferredActorIsolation computeActorIsolation(Evaluator &evaluator,
inferenceSource = dc->getAsDecl();
}
switch (auto enclosingIsolation = getActorIsolationOfContext(dc)) {
case ActorIsolation::Nonisolated:
case ActorIsolation::CallerIsolationInheriting:
case ActorIsolation::NonisolatedUnsafe:
case ActorIsolation::Unspecified:
// Do nothing.
break;
case ActorIsolation::Erased:
llvm_unreachable("context cannot have erased isolation");
case ActorIsolation::ActorInstance:
return {
inferredIsolation(enclosingIsolation),
IsolationSource(inferenceSource, IsolationSource::LexicalContext)
};
case ActorIsolation::GlobalActor:
return {
inferredIsolation(enclosingIsolation),
IsolationSource(inferenceSource, IsolationSource::LexicalContext)
};
}
auto enclosingIsolation = getActorIsolationOfContext(dc);
auto isolation =
computeClosureIsolationFromParent(func, enclosingIsolation,
/*checkIsolatedCapture*/true)
.withPreconcurrency(enclosingIsolation.preconcurrency());
return {
inferredIsolation(isolation),
IsolationSource(inferenceSource, IsolationSource::LexicalContext)
};
}
}
// If this is an accessor, use the actor isolation of its storage
// declaration.
if (auto accessor = dyn_cast<AccessorDecl>(value)) {
return getInferredActorIsolation(accessor->getStorage());
}
if (auto var = dyn_cast<VarDecl>(value)) {
auto &ctx = var->getASTContext();
if (!ctx.LangOpts.isConcurrencyModelTaskToThread() &&
@@ -6512,7 +6524,8 @@ bool HasIsolatedSelfRequest::evaluate(
}
}
if (attrIsolation) {
return attrIsolation == ActorIsolation::forActorInstanceSelf(selfTypeDecl);
return attrIsolation->getKind() == ActorIsolation::ActorInstance &&
attrIsolation->isActorInstanceForSelfParameter();
}
// If this is a variable, check for a property wrapper that alters its
@@ -7776,7 +7789,7 @@ ActorIsolation swift::getActorIsolationForReference(ValueDecl *decl,
// as needing to enter the actor.
if (auto nominal = ctor->getDeclContext()->getSelfNominalTypeDecl()) {
if (nominal->isAnyActor())
return ActorIsolation::forActorInstanceSelf(decl);
return ActorIsolation::forActorInstanceSelf(ctor);
}
// Fall through to treat initializers like any other declaration.

12
lib/Serialization/DeserializeSIL.cpp
View File

@@ -2165,8 +2165,10 @@ bool SILDeserializer::readSILInstruction(SILFunction *Fn,
std::optional<ApplyIsolationCrossing> IsolationCrossing;
if (bool(ApplyCallerIsolation) || bool(ApplyCalleeIsolation)) {
auto caller = ActorIsolation(ActorIsolation::Kind(ApplyCallerIsolation));
auto callee = ActorIsolation(ActorIsolation::Kind(ApplyCalleeIsolation));
auto caller = ActorIsolation(ActorIsolation::Kind(ApplyCallerIsolation),
/*forSIL*/ true);
auto callee = ActorIsolation(ActorIsolation::Kind(ApplyCalleeIsolation),
/*forSIL*/ true);
IsolationCrossing = {caller, callee};
}
@@ -2209,8 +2211,10 @@ bool SILDeserializer::readSILInstruction(SILFunction *Fn,
std::optional<ApplyIsolationCrossing> IsolationCrossing;
if (bool(ApplyCallerIsolation) || bool(ApplyCalleeIsolation)) {
auto caller = ActorIsolation(ActorIsolation::Kind(ApplyCallerIsolation));
auto callee = ActorIsolation(ActorIsolation::Kind(ApplyCalleeIsolation));
auto caller = ActorIsolation(ActorIsolation::Kind(ApplyCallerIsolation),
/*forSIL*/true);
auto callee = ActorIsolation(ActorIsolation::Kind(ApplyCalleeIsolation),
/*forSIL*/true);
IsolationCrossing = {caller, callee};
}

19
test/Concurrency/default_isolation.swift
View File

@@ -30,6 +30,14 @@ func test() {
}
}
// Tested below. This used to fail in default-isolation mode because
// the type-checker applied the default isolation to the implicit $defer
// function, causing it to have MainActor isolation despite the enclosing
// context being nonisolated.
nonisolated func test_defer() {
defer {}
}
//--- concurrent.swift
using nonisolated
@@ -37,7 +45,14 @@ using nonisolated
// CHECK: // S.init(value:)
// CHECK-NEXT: // Isolation: unspecified
struct S {
// CHECK: // S.value.getter
// CHECK-NEXT: // Isolation: unspecified
var value: Int
}
// CHECK: // test_defer()
// CHECK-NEXT: // Isolation: nonisolated
// CHECK: // $defer #1 () in test_defer()
// CHECK-NEXT: // Isolation: nonisolated
// CHECK: // S.value.getter
// CHECK-NEXT: // Isolation: unspecified

62
test/Concurrency/local_functions.swift Normal file
View File

@@ -0,0 +1,62 @@
// RUN: %target-swift-frontend -target %target-swift-5.1-abi-triple %s -emit-sil -o - -verify -strict-concurrency=complete -enable-actor-data-race-checks -disable-availability-checking | %FileCheck %s
// Issue #80772. This used to crash in SILGen because we gave local functions
// the isolation of their enclosing context instead of trying to convert
// parameter isolation to capture isolation.
actor TestActor {
// CHECK-LABEL: // nested #1 () in TestActor.testWithoutCapture()
// CHECK-NEXT: // Isolation: actor_instance. name: 'self'
func testWithoutCapture() {
func nested() -> String {
return "test"
}
print(nested())
}
// CHECK-LABEL: // nested #1 () in TestActor.testWithCapture()
// CHECK-NEXT: // Isolation: actor_instance. name: 'self'
// CHECK: [[SELF_EXECUTOR:%.*]] = extract_executor %0
// CHECK: [[CHECK_FN:%.*]] = function_ref @swift_task_isCurrentExecutor
// CHECK: apply [[CHECK_FN]]([[SELF_EXECUTOR]])
func testWithCapture() {
func nested() -> String {
_ = self
return "test"
}
print(nested())
}
}
@globalActor struct GenericGlobalActor<T> {
static var shared: TestActor {
// not a valid implementation
return TestActor()
}
}
struct Generic<T> {
// CHECK-LABEL: // nested #1 <A><A1>(_:) in Generic.testGenericGlobalActor()
// CHECK-NEXT: // Isolation: global_actor. type: GenericGlobalActor<T>
@GenericGlobalActor<T> func testGenericGlobalActor() {
func nested<U>(_ type: U.Type) -> String {
// CHECK: [[FN:%.*]] = function_ref @$s15local_functions18GenericGlobalActorV6sharedAA04TestE0CvgZ
// CHECK: apply [[FN]]<T>(
return "test"
}
print(nested(Int.self))
}
}
actor MyActor {
// CHECK-LABEL: // nested #1 () in MyActor.deinit
// CHECK-NEXT: // Isolation: actor_instance. name: 'self'
isolated deinit {
func nested() -> String {
return "test"
}
print(nested())
}
}

4
test/SILGen/local_function_isolation.swift
View File

@@ -71,9 +71,9 @@ actor GenericActor<K> {
// Make sure defer doesn't capture anything.
actor DeferInsideInitActor {
init(foo: ()) async throws {
// CHECK-LABEL: sil private [ossa] @$s24local_function_isolation20DeferInsideInitActorC3fooACyt_tYaKcfc6$deferL_yyF : $@convention(thin) () -> () {
// CHECK-LABEL: sil private [ossa] @$s24local_function_isolation20DeferInsideInitActorC3fooACyt_tYaKcfc6$deferL_yyF : $@convention(thin) (@sil_isolated @guaranteed DeferInsideInitActor) -> () {
defer {}
try self.init()
self.init()
}
}