From 7356fe8c8a1607358c98bfb22c20a2b413563ec1 Mon Sep 17 00:00:00 2001 From: Holly Borla Date: Wed, 10 Jul 2024 11:58:17 -0700 Subject: [PATCH 1/5] [ConformanceLookup] Don't allow skipping inherited unavailable conformances in favor of explicit available ones. The type checker does not support the notion of multiple protocol conformances; there can only be one conformance, and if that conformance is unavailable, you cannot specify your own available conformance. This is important for Sendable checking; if a framework specifies that a type is explicitly not Sendable with an unavailable Sendable conformance, clients cannot ignore Sendable violations involving that type. If a superclass wants to allow subclasses to add a Sendable conformance, it should not declare an unavailable Sendable conformance. --- lib/AST/ConformanceLookupTable.cpp | 18 ++++++++---------- test/Concurrency/sendable_checking.swift | 7 +++++-- 2 files changed, 13 insertions(+), 12 deletions(-) diff --git a/lib/AST/ConformanceLookupTable.cpp b/lib/AST/ConformanceLookupTable.cpp index 59f693365aa01..531b459931a9a 100644 --- a/lib/AST/ConformanceLookupTable.cpp +++ b/lib/AST/ConformanceLookupTable.cpp @@ -259,14 +259,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(dc)) { - if (AvailableAttr::isUnavailable(ext)) - return; - } - } - // Don't add redundant conformances here. This is merely an // optimization; resolveConformances() would zap the duplicates // anyway. @@ -628,6 +620,14 @@ ConformanceLookupTable::Ordering ConformanceLookupTable::compareConformances( // Allow replacement of an explicit conformance to a marker protocol. // (This permits redundant explicit declarations of `Sendable`.) + // + // FIXME: We need to warn on attempts to make an unavailable Sendable + // conformance available, which does not work. + // + // We probably also want to warn if there is an existing, explicit + // conformance, so clients are prompted to remove retroactive unchecked + // Sendable conformances when the proper Sendable conformance is added + // in the original module. return (kind == ConformanceEntryKind::Explicit && entry->getProtocol()->isMarkerProtocol()); }; @@ -879,8 +879,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())); diff --git a/test/Concurrency/sendable_checking.swift b/test/Concurrency/sendable_checking.swift index aa2fefde2f7e1..2e3703a230308 100644 --- a/test/Concurrency/sendable_checking.swift +++ b/test/Concurrency/sendable_checking.swift @@ -102,7 +102,10 @@ 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 { var t: T @@ -116,7 +119,7 @@ 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}} } From 85b66d1dc28244969183ec5cf49dbea263a4f5e2 Mon Sep 17 00:00:00 2001 From: Holly Borla Date: Thu, 11 Jul 2024 20:28:09 -0700 Subject: [PATCH 2/5] [ConformanceLookup] Always prefer unavailable Sendable conformances from the defining module, and diagnose redundant Sendable conformances. We still allow re-stating inherited unchecked Sendable conformances in subclasses because inherited Sendable conformances are surprising when they opt out of static checking. Otherwise, warning on redundant Sendable conformances nudges programmers toward cleaning up unnecessary retroactive Sendable conformances over time as libraries incrementally add the conformances directly. --- include/swift/AST/DiagnosticsSema.def | 3 + lib/AST/ConformanceLookupTable.cpp | 52 ++++++---------- lib/Sema/TypeCheckConcurrency.cpp | 22 ++++++- lib/Sema/TypeCheckProtocol.cpp | 60 +++++++++++++++---- .../Inputs/SendableConformances.swift | 7 +++ .../redundant_sendable_conformance.swift | 19 ++++++ test/Concurrency/sendable_checking.swift | 4 +- .../sendable_conformance_checking.swift | 2 +- 8 files changed, 119 insertions(+), 50 deletions(-) create mode 100644 test/Concurrency/Inputs/SendableConformances.swift create mode 100644 test/Concurrency/redundant_sendable_conformance.swift diff --git a/include/swift/AST/DiagnosticsSema.def b/include/swift/AST/DiagnosticsSema.def index e953aacace25e..a748db027daa3 100644 --- a/include/swift/AST/DiagnosticsSema.def +++ b/include/swift/AST/DiagnosticsSema.def @@ -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, diff --git a/lib/AST/ConformanceLookupTable.cpp b/lib/AST/ConformanceLookupTable.cpp index 531b459931a9a..b449a5a0782f9 100644 --- a/lib/AST/ConformanceLookupTable.cpp +++ b/lib/AST/ConformanceLookupTable.cpp @@ -597,47 +597,31 @@ ConformanceLookupTable::Ordering ConformanceLookupTable::compareConformances( } } - // If only one of the conformances is unconditionally available on the - // current deployment target, pick that one. - // - // FIXME: Conformance lookup should really depend on source location for - // this to be 100% correct. - // FIXME: When a class and an extension with the same availability declare the - // same conformance, this silently takes the class and drops the extension. - if (lhs->getDeclContext()->isAlwaysAvailableConformanceContext() != - rhs->getDeclContext()->isAlwaysAvailableConformanceContext()) { - return (lhs->getDeclContext()->isAlwaysAvailableConformanceContext() - ? Ordering::Before - : Ordering::After); + // Unavailable Sendable conformances cannot be replaced by available ones. + if (!lhs->getProtocol()->isMarkerProtocol()) { + // If only one of the conformances is unconditionally available on the + // current deployment target, pick that one. + // + // FIXME: Conformance lookup should really depend on source location for + // this to be 100% correct. + // FIXME: When a class and an extension with the same availability declare the + // same conformance, this silently takes the class and drops the extension. + if (lhs->getDeclContext()->isAlwaysAvailableConformanceContext() != + rhs->getDeclContext()->isAlwaysAvailableConformanceContext()) { + 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`.) - // - // FIXME: We need to warn on attempts to make an unavailable Sendable - // conformance available, which does not work. - // - // We probably also want to warn if there is an existing, explicit - // conformance, so clients are prompted to remove retroactive unchecked - // Sendable conformances when the proper Sendable conformance is added - // in the original module. - 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; } diff --git a/lib/Sema/TypeCheckConcurrency.cpp b/lib/Sema/TypeCheckConcurrency.cpp index be7fc3a350663..f8ed85ff0b698 100644 --- a/lib/Sema/TypeCheckConcurrency.cpp +++ b/lib/Sema/TypeCheckConcurrency.cpp @@ -6179,8 +6179,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. @@ -6208,6 +6216,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 conformances; + nominal->lookupConformance(proto, conformances); + for (auto conformance : conformances) { + if (conformance->getDeclContext() == conformanceDC) { + return conformance; + } + } } auto conformance = ctx.getNormalConformance( diff --git a/lib/Sema/TypeCheckProtocol.cpp b/lib/Sema/TypeCheckProtocol.cpp index b98587b0cde12..c11a3823e2af6 100644 --- a/lib/Sema/TypeCheckProtocol.cpp +++ b/lib/Sema/TypeCheckProtocol.cpp @@ -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 conformances; + nominal->lookupConformance(diag.Protocol, conformances); + for (auto conformance : conformances) { + if (isa(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(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. diff --git a/test/Concurrency/Inputs/SendableConformances.swift b/test/Concurrency/Inputs/SendableConformances.swift new file mode 100644 index 0000000000000..4333ba0bb884e --- /dev/null +++ b/test/Concurrency/Inputs/SendableConformances.swift @@ -0,0 +1,7 @@ + +public class NonSendableClass {} + +@available(*, unavailable) +extension NonSendableClass: @unchecked Sendable {} + +public struct SendableStruct: Sendable {} diff --git a/test/Concurrency/redundant_sendable_conformance.swift b/test/Concurrency/redundant_sendable_conformance.swift new file mode 100644 index 0000000000000..421dfa6dc065f --- /dev/null +++ b/test/Concurrency/redundant_sendable_conformance.swift @@ -0,0 +1,19 @@ +// 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 + +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 +// expected-error@-1 {{conformance of 'NonSendableClass' to 'Sendable' is unavailable}} + +extension SendableStruct: @retroactive @unchecked Sendable {} +// expected-warning@-1 {{conformance of 'SendableStruct' to protocol 'Sendable' was already stated in the type's module 'SendableConformances'}} diff --git a/test/Concurrency/sendable_checking.swift b/test/Concurrency/sendable_checking.swift index 2e3703a230308..9a51673d76679 100644 --- a/test/Concurrency/sendable_checking.swift +++ b/test/Concurrency/sendable_checking.swift @@ -113,8 +113,8 @@ struct WrapClass { 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, *) diff --git a/test/Concurrency/sendable_conformance_checking.swift b/test/Concurrency/sendable_conformance_checking.swift index 2a7624d09163b..a28c03ab85ad3 100644 --- a/test/Concurrency/sendable_conformance_checking.swift +++ b/test/Concurrency/sendable_conformance_checking.swift @@ -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 { From b1397703a5ab3f1569079dd85e51eb6d67b3a5cf Mon Sep 17 00:00:00 2001 From: Holly Borla Date: Thu, 11 Jul 2024 22:57:48 -0700 Subject: [PATCH 3/5] [ConformanceLookup] Just kidding, the compiler needs to prefer available Sendable conformances for source compatibility. If conformance lookup always prefers the conformance from the defining module, libraries introducing unavailable Sendable conformances can break source in clients that declare retroactive Sendable conformances. Instead, still prefer the available conformance, and always diagnose the client conformance as redundant (as a warning). Then, when the retroactive conformance is removed, the errors will surface, so the programmer has to take explicit action to experience the source break. --- lib/AST/ConformanceLookupTable.cpp | 52 +++++++++++++------ .../Inputs/SendableConformances.swift | 2 + .../redundant_sendable_conformance.swift | 7 ++- 3 files changed, 43 insertions(+), 18 deletions(-) diff --git a/lib/AST/ConformanceLookupTable.cpp b/lib/AST/ConformanceLookupTable.cpp index b449a5a0782f9..85639161717f1 100644 --- a/lib/AST/ConformanceLookupTable.cpp +++ b/lib/AST/ConformanceLookupTable.cpp @@ -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); } } @@ -597,21 +606,22 @@ ConformanceLookupTable::Ordering ConformanceLookupTable::compareConformances( } } - // Unavailable Sendable conformances cannot be replaced by available ones. - if (!lhs->getProtocol()->isMarkerProtocol()) { - // If only one of the conformances is unconditionally available on the - // current deployment target, pick that one. - // - // FIXME: Conformance lookup should really depend on source location for - // this to be 100% correct. - // FIXME: When a class and an extension with the same availability declare the - // same conformance, this silently takes the class and drops the extension. - if (lhs->getDeclContext()->isAlwaysAvailableConformanceContext() != - rhs->getDeclContext()->isAlwaysAvailableConformanceContext()) { - return (lhs->getDeclContext()->isAlwaysAvailableConformanceContext() - ? Ordering::Before - : Ordering::After); - } + // If only one of the conformances is unconditionally available on the + // current deployment target, pick that one. + // + // FIXME: Conformance lookup should really depend on source location for + // this to be 100% correct. + // FIXME: When a class and an extension with the same availability declare the + // 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. @@ -1127,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(), diff --git a/test/Concurrency/Inputs/SendableConformances.swift b/test/Concurrency/Inputs/SendableConformances.swift index 4333ba0bb884e..33a4b3995158e 100644 --- a/test/Concurrency/Inputs/SendableConformances.swift +++ b/test/Concurrency/Inputs/SendableConformances.swift @@ -5,3 +5,5 @@ public class NonSendableClass {} extension NonSendableClass: @unchecked Sendable {} public struct SendableStruct: Sendable {} + +public struct AnotherSendableStruct: Sendable {} diff --git a/test/Concurrency/redundant_sendable_conformance.swift b/test/Concurrency/redundant_sendable_conformance.swift index 421dfa6dc065f..956636e695ec4 100644 --- a/test/Concurrency/redundant_sendable_conformance.swift +++ b/test/Concurrency/redundant_sendable_conformance.swift @@ -13,7 +13,12 @@ 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 -// expected-error@-1 {{conformance of 'NonSendableClass' to 'Sendable' is unavailable}} 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 From e2ddc6c6c2a8089861764afe8999a107296200a9 Mon Sep 17 00:00:00 2001 From: Holly Borla Date: Fri, 12 Jul 2024 14:58:49 -0700 Subject: [PATCH 4/5] [ProtocolConformance] Compute the correct un-availability for inherited conformances. --- lib/AST/ProtocolConformanceRef.cpp | 3 ++- lib/Sema/TypeCheckConcurrency.cpp | 3 --- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/lib/AST/ProtocolConformanceRef.cpp b/lib/AST/ProtocolConformanceRef.cpp index 425522bc9b036..06f05ef44f3a7 100644 --- a/lib/AST/ProtocolConformanceRef.cpp +++ b/lib/AST/ProtocolConformanceRef.cpp @@ -288,7 +288,8 @@ bool ProtocolConformanceRef::hasUnavailableConformance() const { // Check whether this conformance is on an unavailable extension. auto concrete = getConcrete(); - auto ext = dyn_cast(concrete->getDeclContext()); + auto *dc = concrete->getRootConformance()->getDeclContext(); + auto ext = dyn_cast(dc); if (ext && AvailableAttr::isUnavailable(ext)) return true; diff --git a/lib/Sema/TypeCheckConcurrency.cpp b/lib/Sema/TypeCheckConcurrency.cpp index f8ed85ff0b698..19bce6083bf81 100644 --- a/lib/Sema/TypeCheckConcurrency.cpp +++ b/lib/Sema/TypeCheckConcurrency.cpp @@ -6249,9 +6249,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(); From 18b747c1815f5c82026b6c6e3a84a4b103fb74c5 Mon Sep 17 00:00:00 2001 From: Holly Borla Date: Fri, 12 Jul 2024 21:49:35 -0700 Subject: [PATCH 5/5] [Concurrency] Diagnose captures of `self` in a task created in deinit. This is done by diagnosing captures of `self` in escaping `sending` or `@Sendable` closures inside a deinit, which almost certainly means `self` will outlive deinit at runtime, which is a fatal error. This is a common mistake to make when creating isolated tasks inside nonisolated deinits to workaround the lack of synchrnous isolated deinits in Swift 6. --- include/swift/AST/DiagnosticsSema.def | 3 ++ lib/Sema/TypeCheckConcurrency.cpp | 20 +++++++++++- test/Concurrency/self_escapes_deinit.swift | 37 ++++++++++++++++++++++ 3 files changed, 59 insertions(+), 1 deletion(-) create mode 100644 test/Concurrency/self_escapes_deinit.swift diff --git a/include/swift/AST/DiagnosticsSema.def b/include/swift/AST/DiagnosticsSema.def index a748db027daa3..8a4a83c5fec4b 100644 --- a/include/swift/AST/DiagnosticsSema.def +++ b/include/swift/AST/DiagnosticsSema.def @@ -5560,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)) diff --git a/lib/Sema/TypeCheckConcurrency.cpp b/lib/Sema/TypeCheckConcurrency.cpp index 19bce6083bf81..15519cf62086b 100644 --- a/lib/Sema/TypeCheckConcurrency.cpp +++ b/lib/Sema/TypeCheckConcurrency.cpp @@ -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(closure); + auto *dc = getDeclContext(); + if (explicitClosure && isa(dc) && + !explicitClosure->getType()->isNoEscape() && + (explicitClosure->isPassedToSendingParameter() || + explicitClosure->isSendable())) { + auto var = dyn_cast_or_null(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(); @@ -2606,7 +2625,6 @@ namespace { if (type->hasError()) continue; - auto *closure = localFunc.getAbstractClosureExpr(); if (closure && closure->isImplicit()) { auto *patternBindingDecl = getTopPatternBindingDecl(); if (patternBindingDecl && patternBindingDecl->isAsyncLet()) { diff --git a/test/Concurrency/self_escapes_deinit.swift b/test/Concurrency/self_escapes_deinit.swift new file mode 100644 index 0000000000000..352256dfdb861 --- /dev/null +++ b/test/Concurrency/self_escapes_deinit.swift @@ -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 + } + } +}