Skip to content

Commit 8159d20

Browse files
hborlahborla
committed
[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. (cherry picked from commit 85b66d1)
1 parent 5ecfee7 commit 8159d20

File tree

8 files changed

+119
-50
lines changed

8 files changed

+119
-50
lines changed

‎include/swift/AST/DiagnosticsSema.def

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3127,6 +3127,9 @@ WARNING(witness_deprecated,none,
31273127
"protocol %1%select{|: %2}2",
31283128
(const ValueDecl *, Identifier, StringRef))
31293129

3130+
WARNING(unavailable_conformance,none,
3131+
"conformance of %0 to protocol %1 is already unavailable",
3132+
(Type, Identifier))
31303133
ERROR(redundant_conformance,none,
31313134
"redundant conformance of %0 to protocol %1", (Type, Identifier))
31323135
ERROR(redundant_conformance_conditional,none,

‎lib/AST/ConformanceLookupTable.cpp

Lines changed: 18 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -596,47 +596,31 @@ ConformanceLookupTable::Ordering ConformanceLookupTable::compareConformances(
596596
}
597597
}
598598

599-
// If only one of the conformances is unconditionally available on the
600-
// current deployment target, pick that one.
601-
//
602-
// FIXME: Conformance lookup should really depend on source location for
603-
// this to be 100% correct.
604-
// FIXME: When a class and an extension with the same availability declare the
605-
// same conformance, this silently takes the class and drops the extension.
606-
if (lhs->getDeclContext()->isAlwaysAvailableConformanceContext() !=
607-
rhs->getDeclContext()->isAlwaysAvailableConformanceContext()) {
608-
return (lhs->getDeclContext()->isAlwaysAvailableConformanceContext()
609-
? Ordering::Before
610-
: Ordering::After);
599+
// Unavailable Sendable conformances cannot be replaced by available ones.
600+
if (!lhs->getProtocol()->isMarkerProtocol()) {
601+
// If only one of the conformances is unconditionally available on the
602+
// current deployment target, pick that one.
603+
//
604+
// FIXME: Conformance lookup should really depend on source location for
605+
// this to be 100% correct.
606+
// FIXME: When a class and an extension with the same availability declare the
607+
// same conformance, this silently takes the class and drops the extension.
608+
if (lhs->getDeclContext()->isAlwaysAvailableConformanceContext() !=
609+
rhs->getDeclContext()->isAlwaysAvailableConformanceContext()) {
610+
return (lhs->getDeclContext()->isAlwaysAvailableConformanceContext()
611+
? Ordering::Before
612+
: Ordering::After);
613+
}
611614
}
612615

613616
// If one entry is fixed and the other is not, we have our answer.
614617
if (lhs->isFixed() != rhs->isFixed()) {
615-
auto isReplaceableOrMarker = [](ConformanceEntry *entry) -> bool {
616-
ConformanceEntryKind kind = entry->getRankingKind();
617-
if (isReplaceable(kind))
618-
return true;
619-
620-
// Allow replacement of an explicit conformance to a marker protocol.
621-
// (This permits redundant explicit declarations of `Sendable`.)
622-
//
623-
// FIXME: We need to warn on attempts to make an unavailable Sendable
624-
// conformance available, which does not work.
625-
//
626-
// We probably also want to warn if there is an existing, explicit
627-
// conformance, so clients are prompted to remove retroactive unchecked
628-
// Sendable conformances when the proper Sendable conformance is added
629-
// in the original module.
630-
return (kind == ConformanceEntryKind::Explicit
631-
&& entry->getProtocol()->isMarkerProtocol());
632-
};
633-
634618
// If the non-fixed conformance is not replaceable, we have a failure to
635619
// diagnose.
636620
// FIXME: We should probably diagnose if they have different constraints.
637-
diagnoseSuperseded = (lhs->isFixed() && !isReplaceableOrMarker(rhs)) ||
638-
(rhs->isFixed() && !isReplaceableOrMarker(lhs));
639-
621+
diagnoseSuperseded = (lhs->isFixed() && !isReplaceable(rhs->getRankingKind())) ||
622+
(rhs->isFixed() && !isReplaceable(lhs->getRankingKind()));
623+
640624
return lhs->isFixed() ? Ordering::Before : Ordering::After;
641625
}
642626

‎lib/Sema/TypeCheckConcurrency.cpp

Lines changed: 21 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6393,8 +6393,16 @@ ProtocolConformance *swift::deriveImplicitSendableConformance(
63936393

63946394
// Local function to form the implicit conformance.
63956395
auto formConformance = [&](const DeclAttribute *attrMakingUnavailable)
6396-
-> NormalProtocolConformance * {
6396+
-> ProtocolConformance * {
63976397
DeclContext *conformanceDC = nominal;
6398+
6399+
// FIXME: @_nonSendable should be a builtin extension macro. This behavior
6400+
// of explanding the unavailable conformance during implicit Sendable
6401+
// derivation means that clients can unknowingly ignore unavailable Sendable
6402+
// Sendable conformances from the original module added via @_nonSendable
6403+
// because they are not expanded if an explicit conformance is found via
6404+
// conformance lookup. So, if a retroactive, unchecked Sendable conformance
6405+
// is written, no redundant conformance warning is emitted.
63986406
if (attrMakingUnavailable) {
63996407
// Conformance availability is currently tied to the declaring extension.
64006408
// FIXME: This is a hack--we should give conformances real availability.
@@ -6422,6 +6430,18 @@ ProtocolConformance *swift::deriveImplicitSendableConformance(
64226430
file->getOrCreateSynthesizedFile().addTopLevelDecl(extension);
64236431

64246432
conformanceDC = extension;
6433+
6434+
// Let the conformance lookup table register the conformance
6435+
// from the extension. Otherwise, we'll end up with redundant
6436+
// conformances between the explicit conformance from the extension
6437+
// and the conformance synthesized below.
6438+
SmallVector<ProtocolConformance *, 2> conformances;
6439+
nominal->lookupConformance(proto, conformances);
6440+
for (auto conformance : conformances) {
6441+
if (conformance->getDeclContext() == conformanceDC) {
6442+
return conformance;
6443+
}
6444+
}
64256445
}
64266446

64276447
auto conformance = ctx.getNormalConformance(

‎lib/Sema/TypeCheckProtocol.cpp

Lines changed: 48 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -6263,20 +6263,56 @@ void TypeChecker::checkConformancesInContext(IterableDeclContext *idc) {
62636263
// protocol, just warn; we'll pick up the original conformance.
62646264
auto existingModule = diag.ExistingDC->getParentModule();
62656265
auto extendedNominal = diag.ExistingDC->getSelfNominalTypeDecl();
6266-
if (existingModule != dc->getParentModule() &&
6267-
(existingModule->getName() ==
6268-
extendedNominal->getParentModule()->getName() ||
6266+
auto definingModule = extendedNominal->getParentModule()->getName();
6267+
bool conformanceInOrigModule =
6268+
(existingModule->getName() == definingModule ||
62696269
existingModule == diag.Protocol->getParentModule() ||
6270-
existingModule->getName().is("CoreGraphics"))) {
6270+
existingModule->getName().is("CoreGraphics"));
6271+
6272+
// Redundant Sendable conformances are always warnings.
6273+
auto knownProtocol = diag.Protocol->getKnownProtocolKind();
6274+
bool isSendable = knownProtocol == KnownProtocolKind::Sendable;
6275+
// Try to find an inherited Sendable conformance if there is one.
6276+
if (isSendable && !SendableConformance) {
6277+
SmallVector<ProtocolConformance *, 2> conformances;
6278+
nominal->lookupConformance(diag.Protocol, conformances);
6279+
for (auto conformance : conformances) {
6280+
if (isa<InheritedProtocolConformance>(conformance))
6281+
SendableConformance = conformance;
6282+
}
6283+
}
6284+
6285+
if ((existingModule != dc->getParentModule() && conformanceInOrigModule) ||
6286+
isSendable) {
62716287
// Warn about the conformance.
6272-
auto diagID = differentlyConditional
6273-
? diag::redundant_conformance_adhoc_conditional
6274-
: diag::redundant_conformance_adhoc;
6275-
Context.Diags.diagnose(diag.Loc, diagID, dc->getDeclaredInterfaceType(),
6276-
diag.Protocol->getName(),
6277-
existingModule->getName() ==
6278-
extendedNominal->getParentModule()->getName(),
6279-
existingModule->getName());
6288+
if (isSendable && SendableConformance &&
6289+
isa<InheritedProtocolConformance>(SendableConformance)) {
6290+
// Allow re-stated unchecked conformances to Sendable in subclasses
6291+
// as long as the inherited conformance isn't unavailable.
6292+
auto *conformance = SendableConformance->getRootConformance();
6293+
auto *decl = conformance->getDeclContext()->getAsDecl();
6294+
if (!AvailableAttr::isUnavailable(decl)) {
6295+
continue;
6296+
}
6297+
6298+
Context.Diags.diagnose(diag.Loc, diag::unavailable_conformance,
6299+
nominal->getDeclaredInterfaceType(),
6300+
diag.Protocol->getName());
6301+
} else if (existingModule == dc->getParentModule()) {
6302+
Context.Diags.diagnose(diag.Loc, diag::redundant_conformance,
6303+
nominal->getDeclaredInterfaceType(),
6304+
diag.Protocol->getName())
6305+
.limitBehavior(DiagnosticBehavior::Warning);
6306+
} else {
6307+
auto diagID = differentlyConditional
6308+
? diag::redundant_conformance_adhoc_conditional
6309+
: diag::redundant_conformance_adhoc;
6310+
Context.Diags.diagnose(diag.Loc, diagID, dc->getDeclaredInterfaceType(),
6311+
diag.Protocol->getName(),
6312+
existingModule->getName() ==
6313+
extendedNominal->getParentModule()->getName(),
6314+
existingModule->getName());
6315+
}
62806316

62816317
// Complain about any declarations in this extension whose names match
62826318
// a requirement in that protocol.

‎test/Concurrency/Inputs/SendableConformances.swift

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
2+
public class NonSendableClass {}
3+
4+
@available(*, unavailable)
5+
extension NonSendableClass: @unchecked Sendable {}
6+
7+
public struct SendableStruct: Sendable {}

‎test/Concurrency/redundant_sendable_conformance.swift

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,19 @@
1+
// RUN: %empty-directory(%t)
2+
// RUN: %target-swift-frontend -emit-module -emit-module-path %t/SendableConformances.swiftmodule -module-name SendableConformances %S/Inputs/SendableConformances.swift
3+
4+
// RUN: %target-swift-frontend -typecheck %s -verify -swift-version 6 -I %t
5+
6+
// REQUIRES: concurrency
7+
8+
import SendableConformances
9+
10+
typealias RequireSendable<T: Sendable> = T
11+
12+
extension NonSendableClass: @retroactive @unchecked Sendable {}
13+
// expected-warning@-1 {{conformance of 'NonSendableClass' to protocol 'Sendable' was already stated in the type's module 'SendableConformances'}}
14+
15+
typealias CheckNonSendableClass = RequireSendable<NonSendableClass>
16+
// expected-error@-1 {{conformance of 'NonSendableClass' to 'Sendable' is unavailable}}
17+
18+
extension SendableStruct: @retroactive @unchecked Sendable {}
19+
// expected-warning@-1 {{conformance of 'SendableStruct' to protocol 'Sendable' was already stated in the type's module 'SendableConformances'}}

‎test/Concurrency/sendable_checking.swift

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -113,8 +113,8 @@ struct WrapClass<T: NSClass> {
113113

114114
extension WrapClass: Sendable where T: Sendable { }
115115

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

120120
@available(SwiftStdlib 5.1, *)

‎test/Concurrency/sendable_conformance_checking.swift

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -180,7 +180,7 @@ extension SendableExtSub: @unchecked Sendable {}
180180

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

185185
@available(SwiftStdlib 5.1, *)
186186
actor MyActor {

0 commit comments

Comments
 (0)