Skip to content

downgrade actor deinit accesses to warnings in more contexts #41609

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Mar 2, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions include/swift/AST/DiagnosticsSema.def
Original file line number Diff line number Diff line change
Expand Up @@ -4500,9 +4500,10 @@ NOTE(note_distributed_actor_system_conformance_missing_adhoc_requirement,none,
ERROR(override_implicit_unowned_executor,none,
"cannot override an actor's 'unownedExecutor' property that wasn't "
"explicitly defined", ())
ERROR(actor_isolated_from_deinit,none,
"actor-isolated %0 %1 can not be referenced from a non-isolated deinit",
(DescriptiveDeclKind, DeclName))
ERROR(actor_isolated_from_decl,none,
"actor-isolated %0 %1 can not be referenced from a non-isolated "
"%select{deinit|autoclosure|closure}2",
(DescriptiveDeclKind, DeclName, unsigned))
ERROR(actor_isolated_non_self_reference,none,
"actor-isolated %0 %1 can not be "
"%select{referenced|mutated|used 'inout'}2 "
Expand Down
68 changes: 52 additions & 16 deletions lib/Sema/TypeCheckConcurrency.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2686,6 +2686,52 @@ namespace {
});
}

static bool isStoredProperty(ValueDecl const *member) {
if (auto *var = dyn_cast<VarDecl>(member))
if (var->hasStorage() && var->isInstanceMember())
return true;
return false;
}

/// an ad-hoc check specific to member isolation checking.
static bool memberAccessWasAllowedInSwift5(DeclContext const *refCxt,
ValueDecl const *member,
SourceLoc memberLoc) {
// no need for this in Swift 6+
if (refCxt->getASTContext().isSwiftVersionAtLeast(6))
return false;

// In Swift 5, we were allowing all members to be referenced from a
// deinit, nested within a wide variety of contexts.
if (auto oldFn = isActorInitOrDeInitContext(refCxt)) {
if (isa<DestructorDecl>(oldFn) && member->isInstanceMember()) {
auto &diags = refCxt->getASTContext().Diags;

// if the context in which we consider the access matches between
// old and new, and its a stored property, then skip the warning
// because it will still be allowed in Swift 6.
if (!(refCxt == oldFn && isStoredProperty(member))) {
unsigned cxtKind = 0; // deinit

// try to get a better name for this context.
if (isa<AutoClosureExpr>(refCxt)) {
cxtKind = 1;
} else if (isa<AbstractClosureExpr>(refCxt)) {
cxtKind = 2;
}

diags.diagnose(memberLoc, diag::actor_isolated_from_decl,
member->getDescriptiveKind(),
member->getName(),
cxtKind).warnUntilSwiftVersion(6);
}
return true;
}
}

return false;
}

/// To support flow-isolation, some member accesses in inits / deinits
/// must be permitted, despite the isolation of 'self' not being
/// correct in Sema.
Expand All @@ -2697,7 +2743,7 @@ namespace {
///
/// \returns true iff the member access is permitted in Sema because it will
/// be verified later by flow-isolation.
bool checkedByFlowIsolation(DeclContext const *refCxt,
static bool checkedByFlowIsolation(DeclContext const *refCxt,
ReferencedActor &baseActor,
ValueDecl const *member,
SourceLoc memberLoc) {
Expand All @@ -2716,7 +2762,7 @@ namespace {
while (true) {
fnDecl = dyn_cast_or_null<AbstractFunctionDecl>(refCxt->getAsDecl());
if (!fnDecl)
return false;
break;

// go up one level if this context is a defer.
if (auto *d = dyn_cast<FuncDecl>(fnDecl)) {
Expand All @@ -2728,25 +2774,15 @@ namespace {
break;
}

if (memberAccessWasAllowedInSwift5(refCxt, member, memberLoc))
return true; // then permit it now.

if (!usesFlowSensitiveIsolation(fnDecl))
return false;

// Stored properties are definitely OK.
if (auto *var = dyn_cast<VarDecl>(member))
if (var->hasStorage() && var->isInstanceMember())
return true;

// In Swift 5, we were allowing all members to be referenced from a
// deinit, but that will not be valid in Swift 6+, so warn about it.
if (!refCxt->getASTContext().isSwiftVersionAtLeast(6)) {
if (isa<DestructorDecl>(fnDecl) && member->isInstanceMember()) {
auto &diags = refCxt->getASTContext().Diags;
diags.diagnose(memberLoc, diag::actor_isolated_from_deinit,
member->getDescriptiveKind(),
member->getName()).warnUntilSwiftVersion(6);
if (isStoredProperty(member))
return true;
}
}

return false;
}
Expand Down
20 changes: 16 additions & 4 deletions test/Concurrency/actor_isolation.swift
Original file line number Diff line number Diff line change
Expand Up @@ -811,7 +811,7 @@ extension SomeClassInActor.ID {
// ----------------------------------------------------------------------
@available(SwiftStdlib 5.1, *)
actor SomeActorWithInits {
var mutableState: Int = 17 // expected-note 3 {{mutation of this property is only permitted within the actor}}
var mutableState: Int = 17 // expected-note 2 {{mutation of this property is only permitted within the actor}}
var otherMutableState: Int
let nonSendable: SomeClass

Expand Down Expand Up @@ -911,16 +911,16 @@ actor SomeActorWithInits {

let _ = {
defer {
isolated() // expected-error{{actor-isolated instance method 'isolated()' can not be referenced from a non-isolated context}}
mutableState += 1 // expected-error{{actor-isolated property 'mutableState' can not be mutated from a non-isolated context}}
isolated() // expected-warning{{actor-isolated instance method 'isolated()' can not be referenced from a non-isolated closure; this is an error in Swift 6}}
mutableState += 1 // expected-warning{{actor-isolated property 'mutableState' can not be referenced from a non-isolated closure; this is an error in Swift 6}}
nonisolated()
}
nonisolated()
}()
}


func isolated() { } // expected-note 8 {{calls to instance method 'isolated()' from outside of its actor context are implicitly asynchronous}}
func isolated() { } // expected-note 7 {{calls to instance method 'isolated()' from outside of its actor context are implicitly asynchronous}}
nonisolated func nonisolated() {}
}

Expand Down Expand Up @@ -1344,3 +1344,15 @@ final class MainActorInit: Sendable {

func f() {}
}

actor DunkTracker {
private var lebron: Int?
private var curry: Int?

deinit {
// expected-warning@+1 {{actor-isolated property 'curry' can not be referenced from a non-isolated autoclosure; this is an error in Swift 6}}
if lebron != nil || curry != nil {

}
}
}