Skip to content

[Concurrency] Diagnose captures of self in a task created in deinit. #75224

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 5 commits into from
Jul 13, 2024
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
6 changes: 6 additions & 0 deletions include/swift/AST/DiagnosticsSema.def
Original file line number Diff line number Diff line change
Expand Up @@ -3147,6 +3147,9 @@ WARNING(witness_deprecated,none,
"protocol %1%select{|: %2}2",
(const ValueDecl *, Identifier, StringRef))

WARNING(unavailable_conformance,none,
"conformance of %0 to protocol %1 is already unavailable",
(Type, Identifier))
ERROR(redundant_conformance,none,
"redundant conformance of %0 to protocol %1", (Type, Identifier))
ERROR(redundant_conformance_conditional,none,
Expand Down Expand Up @@ -5557,6 +5560,9 @@ ERROR(non_sendable_isolated_capture,none,
ERROR(implicit_async_let_non_sendable_capture,none,
"capture of %1 with non-sendable type %0 in 'async let' binding",
(Type, DeclName))
ERROR(self_capture_deinit_task,none,
"capture of 'self' in a closure that outlives deinit",
())
ERROR(implicit_non_sendable_capture,none,
"implicit capture of %1 requires that %0 conforms to `Sendable`",
(Type, DeclName))
Expand Down
52 changes: 26 additions & 26 deletions lib/AST/ConformanceLookupTable.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -69,9 +69,18 @@ void ConformanceLookupTable::ConformanceEntry::markSupersededBy(
SupersededBy = entry;

if (diagnose) {
// If an unavailable Sendable conformance is superseded by a
// retroactive one in the client, we need to record this error
// at the client decl context.
auto *dc = getDeclContext();
if (getProtocol()->isMarkerProtocol() && isFixed() &&
!entry->isFixed()) {
dc = entry->getDeclContext();
}

// Record the problem in the conformance table. We'll
// diagnose these in semantic analysis.
table.AllSupersededDiagnostics[getDeclContext()].push_back(this);
table.AllSupersededDiagnostics[dc].push_back(this);
}
}

Expand Down Expand Up @@ -259,14 +268,6 @@ void ConformanceLookupTable::inheritConformances(ClassDecl *classDecl,
auto addInheritedConformance = [&](ConformanceEntry *entry) {
auto protocol = entry->getProtocol();

// Don't add unavailable conformances.
if (auto dc = entry->Source.getDeclContext()) {
if (auto ext = dyn_cast<ExtensionDecl>(dc)) {
if (AvailableAttr::isUnavailable(ext))
return;
}
}

// Don't add redundant conformances here. This is merely an
// optimization; resolveConformances() would zap the duplicates
// anyway.
Expand Down Expand Up @@ -614,30 +615,23 @@ ConformanceLookupTable::Ordering ConformanceLookupTable::compareConformances(
// same conformance, this silently takes the class and drops the extension.
if (lhs->getDeclContext()->isAlwaysAvailableConformanceContext() !=
rhs->getDeclContext()->isAlwaysAvailableConformanceContext()) {
// Diagnose conflicting marker protocol conformances that differ in
// un-availability.
diagnoseSuperseded = lhs->getProtocol()->isMarkerProtocol();

return (lhs->getDeclContext()->isAlwaysAvailableConformanceContext()
? Ordering::Before
: Ordering::After);
}

// If one entry is fixed and the other is not, we have our answer.
if (lhs->isFixed() != rhs->isFixed()) {
auto isReplaceableOrMarker = [](ConformanceEntry *entry) -> bool {
ConformanceEntryKind kind = entry->getRankingKind();
if (isReplaceable(kind))
return true;

// Allow replacement of an explicit conformance to a marker protocol.
// (This permits redundant explicit declarations of `Sendable`.)
return (kind == ConformanceEntryKind::Explicit
&& entry->getProtocol()->isMarkerProtocol());
};

// If the non-fixed conformance is not replaceable, we have a failure to
// diagnose.
// FIXME: We should probably diagnose if they have different constraints.
diagnoseSuperseded = (lhs->isFixed() && !isReplaceableOrMarker(rhs)) ||
(rhs->isFixed() && !isReplaceableOrMarker(lhs));
diagnoseSuperseded = (lhs->isFixed() && !isReplaceable(rhs->getRankingKind())) ||
(rhs->isFixed() && !isReplaceable(lhs->getRankingKind()));

return lhs->isFixed() ? Ordering::Before : Ordering::After;
}

Expand Down Expand Up @@ -879,8 +873,6 @@ DeclContext *ConformanceLookupTable::getConformingContext(
return nullptr;
auto inheritedConformance = ModuleDecl::lookupConformance(
superclassTy, protocol, /*allowMissing=*/false);
if (inheritedConformance.hasUnavailableConformance())
inheritedConformance = ProtocolConformanceRef::forInvalid();
if (inheritedConformance)
return superclassDecl;
} while ((superclassDecl = superclassDecl->getSuperclassDecl()));
Expand Down Expand Up @@ -1145,9 +1137,17 @@ void ConformanceLookupTable::lookupConformances(
if (diagnostics) {
auto knownDiags = AllSupersededDiagnostics.find(dc);
if (knownDiags != AllSupersededDiagnostics.end()) {
for (const auto *entry : knownDiags->second) {
for (auto *entry : knownDiags->second) {
ConformanceEntry *supersededBy = entry->getSupersededBy();

// Diagnose the client conformance as superseded.
auto *definingModule = nominal->getParentModule();
if (entry->getDeclContext()->getParentModule() == definingModule &&
supersededBy->getDeclContext()->getParentModule() != definingModule) {
supersededBy = entry;
entry = entry->getSupersededBy();
}

diagnostics->push_back({entry->getProtocol(),
entry->getDeclaredLoc(),
entry->getKind(),
Expand Down
3 changes: 2 additions & 1 deletion lib/AST/ProtocolConformanceRef.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -288,7 +288,8 @@ bool ProtocolConformanceRef::hasUnavailableConformance() const {

// Check whether this conformance is on an unavailable extension.
auto concrete = getConcrete();
auto ext = dyn_cast<ExtensionDecl>(concrete->getDeclContext());
auto *dc = concrete->getRootConformance()->getDeclContext();
auto ext = dyn_cast<ExtensionDecl>(dc);
if (ext && AvailableAttr::isUnavailable(ext))
return true;

Expand Down
45 changes: 40 additions & 5 deletions lib/Sema/TypeCheckConcurrency.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2582,6 +2582,25 @@ namespace {
if (capture.isOpaqueValue())
continue;

auto *closure = localFunc.getAbstractClosureExpr();

// Diagnose a `self` capture inside an escaping `sending`
// `@Sendable` closure in a deinit, which almost certainly
// means `self` would escape deinit at runtime.
auto *explicitClosure = dyn_cast_or_null<ClosureExpr>(closure);
auto *dc = getDeclContext();
if (explicitClosure && isa<DestructorDecl>(dc) &&
!explicitClosure->getType()->isNoEscape() &&
(explicitClosure->isPassedToSendingParameter() ||
explicitClosure->isSendable())) {
auto var = dyn_cast_or_null<VarDecl>(capture.getDecl());
if (var && var->isSelfParameter()) {
ctx.Diags.diagnose(explicitClosure->getLoc(),
diag::self_capture_deinit_task)
.warnUntilSwiftVersion(6);
}
}

// If the closure won't execute concurrently with the context in
// which the declaration occurred, it's okay.
auto decl = capture.getDecl();
Expand All @@ -2606,7 +2625,6 @@ namespace {
if (type->hasError())
continue;

auto *closure = localFunc.getAbstractClosureExpr();
if (closure && closure->isImplicit()) {
auto *patternBindingDecl = getTopPatternBindingDecl();
if (patternBindingDecl && patternBindingDecl->isAsyncLet()) {
Expand Down Expand Up @@ -6179,8 +6197,16 @@ ProtocolConformance *swift::deriveImplicitSendableConformance(

// Local function to form the implicit conformance.
auto formConformance = [&](const DeclAttribute *attrMakingUnavailable)
-> NormalProtocolConformance * {
-> ProtocolConformance * {
DeclContext *conformanceDC = nominal;

// FIXME: @_nonSendable should be a builtin extension macro. This behavior
// of explanding the unavailable conformance during implicit Sendable
// derivation means that clients can unknowingly ignore unavailable Sendable
// Sendable conformances from the original module added via @_nonSendable
// because they are not expanded if an explicit conformance is found via
// conformance lookup. So, if a retroactive, unchecked Sendable conformance
// is written, no redundant conformance warning is emitted.
if (attrMakingUnavailable) {
// Conformance availability is currently tied to the declaring extension.
// FIXME: This is a hack--we should give conformances real availability.
Expand Down Expand Up @@ -6208,6 +6234,18 @@ ProtocolConformance *swift::deriveImplicitSendableConformance(
file->getOrCreateSynthesizedFile().addTopLevelDecl(extension);

conformanceDC = extension;

// Let the conformance lookup table register the conformance
// from the extension. Otherwise, we'll end up with redundant
// conformances between the explicit conformance from the extension
// and the conformance synthesized below.
SmallVector<ProtocolConformance *, 2> conformances;
nominal->lookupConformance(proto, conformances);
for (auto conformance : conformances) {
if (conformance->getDeclContext() == conformanceDC) {
return conformance;
}
}
}

auto conformance = ctx.getNormalConformance(
Expand All @@ -6229,9 +6267,6 @@ ProtocolConformance *swift::deriveImplicitSendableConformance(
auto inheritedConformance = ModuleDecl::checkConformance(
classDecl->mapTypeIntoContext(superclass),
proto, /*allowMissing=*/false);
if (inheritedConformance.hasUnavailableConformance())
inheritedConformance = ProtocolConformanceRef::forInvalid();

if (inheritedConformance) {
inheritedConformance = inheritedConformance
.mapConformanceOutOfContext();
Expand Down
60 changes: 48 additions & 12 deletions lib/Sema/TypeCheckProtocol.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -6256,20 +6256,56 @@ void TypeChecker::checkConformancesInContext(IterableDeclContext *idc) {
// protocol, just warn; we'll pick up the original conformance.
auto existingModule = diag.ExistingDC->getParentModule();
auto extendedNominal = diag.ExistingDC->getSelfNominalTypeDecl();
if (existingModule != dc->getParentModule() &&
(existingModule->getName() ==
extendedNominal->getParentModule()->getName() ||
auto definingModule = extendedNominal->getParentModule()->getName();
bool conformanceInOrigModule =
(existingModule->getName() == definingModule ||
existingModule == diag.Protocol->getParentModule() ||
existingModule->getName().is("CoreGraphics"))) {
existingModule->getName().is("CoreGraphics"));

// Redundant Sendable conformances are always warnings.
auto knownProtocol = diag.Protocol->getKnownProtocolKind();
bool isSendable = knownProtocol == KnownProtocolKind::Sendable;
// Try to find an inherited Sendable conformance if there is one.
if (isSendable && !SendableConformance) {
SmallVector<ProtocolConformance *, 2> conformances;
nominal->lookupConformance(diag.Protocol, conformances);
for (auto conformance : conformances) {
if (isa<InheritedProtocolConformance>(conformance))
SendableConformance = conformance;
}
}

if ((existingModule != dc->getParentModule() && conformanceInOrigModule) ||
isSendable) {
// Warn about the conformance.
auto diagID = differentlyConditional
? diag::redundant_conformance_adhoc_conditional
: diag::redundant_conformance_adhoc;
Context.Diags.diagnose(diag.Loc, diagID, dc->getDeclaredInterfaceType(),
diag.Protocol->getName(),
existingModule->getName() ==
extendedNominal->getParentModule()->getName(),
existingModule->getName());
if (isSendable && SendableConformance &&
isa<InheritedProtocolConformance>(SendableConformance)) {
// Allow re-stated unchecked conformances to Sendable in subclasses
// as long as the inherited conformance isn't unavailable.
auto *conformance = SendableConformance->getRootConformance();
auto *decl = conformance->getDeclContext()->getAsDecl();
if (!AvailableAttr::isUnavailable(decl)) {
continue;
}

Context.Diags.diagnose(diag.Loc, diag::unavailable_conformance,
nominal->getDeclaredInterfaceType(),
diag.Protocol->getName());
} else if (existingModule == dc->getParentModule()) {
Context.Diags.diagnose(diag.Loc, diag::redundant_conformance,
nominal->getDeclaredInterfaceType(),
diag.Protocol->getName())
.limitBehavior(DiagnosticBehavior::Warning);
} else {
auto diagID = differentlyConditional
? diag::redundant_conformance_adhoc_conditional
: diag::redundant_conformance_adhoc;
Context.Diags.diagnose(diag.Loc, diagID, dc->getDeclaredInterfaceType(),
diag.Protocol->getName(),
existingModule->getName() ==
extendedNominal->getParentModule()->getName(),
existingModule->getName());
}

// Complain about any declarations in this extension whose names match
// a requirement in that protocol.
Expand Down
9 changes: 9 additions & 0 deletions test/Concurrency/Inputs/SendableConformances.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@

public class NonSendableClass {}

@available(*, unavailable)
extension NonSendableClass: @unchecked Sendable {}

public struct SendableStruct: Sendable {}

public struct AnotherSendableStruct: Sendable {}
24 changes: 24 additions & 0 deletions test/Concurrency/redundant_sendable_conformance.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
// RUN: %empty-directory(%t)
// RUN: %target-swift-frontend -emit-module -emit-module-path %t/SendableConformances.swiftmodule -module-name SendableConformances %S/Inputs/SendableConformances.swift

// RUN: %target-swift-frontend -typecheck %s -verify -swift-version 6 -I %t

// REQUIRES: concurrency

import SendableConformances

typealias RequireSendable<T: Sendable> = T

extension NonSendableClass: @retroactive @unchecked Sendable {}
// expected-warning@-1 {{conformance of 'NonSendableClass' to protocol 'Sendable' was already stated in the type's module 'SendableConformances'}}

typealias CheckNonSendableClass = RequireSendable<NonSendableClass>

extension SendableStruct: @retroactive @unchecked Sendable {}
// expected-warning@-1 {{conformance of 'SendableStruct' to protocol 'Sendable' was already stated in the type's module 'SendableConformances'}}

@available(*, unavailable)
extension AnotherSendableStruct: @retroactive @unchecked Sendable {}
// expected-warning@-1 {{conformance of 'AnotherSendableStruct' to protocol 'Sendable' was already stated in the type's module 'SendableConformances'}}

typealias CheckAnotherSendableStruct = RequireSendable<AnotherSendableStruct>
37 changes: 37 additions & 0 deletions test/Concurrency/self_escapes_deinit.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
// RUN: %target-typecheck-verify-swift -strict-concurrency=complete -disable-availability-checking

@MainActor
class C {
let x: Int = 0

deinit {
// expected-warning@+1 {{capture of 'self' in a closure that outlives deinit; this is an error in the Swift 6 language mode}}
Task { @MainActor in
_ = self
}

// expected-warning@+1 {{capture of 'self' in a closure that outlives deinit; this is an error in the Swift 6 language mode}}
Task {
_ = x
}
}
}

func enqueueSomewhereElse(_ closure: @escaping @Sendable () -> Void) {}

@MainActor
class C2 {
let x: Int = 0

deinit {
// expected-warning@+1 {{capture of 'self' in a closure that outlives deinit; this is an error in the Swift 6 language mode}}
enqueueSomewhereElse {
_ = self
}

// expected-warning@+1 {{capture of 'self' in a closure that outlives deinit; this is an error in the Swift 6 language mode}}
enqueueSomewhereElse {
_ = self.x
}
}
}
11 changes: 7 additions & 4 deletions test/Concurrency/sendable_checking.swift
Original file line number Diff line number Diff line change
Expand Up @@ -102,21 +102,24 @@ public actor MyActor: MyProto {
}

// Make sure the generic signature doesn't minimize away Sendable requirements.
@_nonSendable class NSClass { }
class NSClass { }

@available(*, unavailable)
extension NSClass: @unchecked Sendable {} // expected-note {{conformance of 'NSClass' to 'Sendable' has been explicitly marked unavailable here}}

struct WrapClass<T: NSClass> {
var t: T
}

extension WrapClass: Sendable where T: Sendable { }

// Make sure we don't inherit the unavailable Sendable conformance from
// our superclass.
// expected-warning@+2 {{conformance of 'SendableSubclass' to protocol 'Sendable' is already unavailable}}
// expected-note@+1 {{'SendableSubclass' inherits conformance to protocol 'Sendable' from superclass here}}
class SendableSubclass: NSClass, @unchecked Sendable { }

@available(SwiftStdlib 5.1, *)
func testSubclassing(obj: SendableSubclass) async {
acceptCV(obj) // okay!
acceptCV(obj) // expected-warning {{conformance of 'NSClass' to 'Sendable' is unavailable; this is an error in the Swift 6 language mode}}
}


Expand Down
2 changes: 1 addition & 1 deletion test/Concurrency/sendable_conformance_checking.swift
Original file line number Diff line number Diff line change
Expand Up @@ -180,7 +180,7 @@ extension SendableExtSub: @unchecked Sendable {}

// Still want to know about same-class redundancy
class MultiConformance: @unchecked Sendable {} // expected-note {{'MultiConformance' declares conformance to protocol 'Sendable' here}}
extension MultiConformance: @unchecked Sendable {} // expected-error {{redundant conformance of 'MultiConformance' to protocol 'Sendable'}}
extension MultiConformance: @unchecked Sendable {} // expected-warning {{redundant conformance of 'MultiConformance' to protocol 'Sendable'}}

@available(SwiftStdlib 5.1, *)
actor MyActor {
Expand Down