Skip to content

Commit 85b66d1

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.
1 parent 7356fe8 commit 85b66d1

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
@@ -3147,6 +3147,9 @@ WARNING(witness_deprecated,none,
31473147
"protocol %1%select{|: %2}2",
31483148
(const ValueDecl *, Identifier, StringRef))
31493149

3150+
WARNING(unavailable_conformance,none,
3151+
"conformance of %0 to protocol %1 is already unavailable",
3152+
(Type, Identifier))
31503153
ERROR(redundant_conformance,none,
31513154
"redundant conformance of %0 to protocol %1", (Type, Identifier))
31523155
ERROR(redundant_conformance_conditional,none,

‎lib/AST/ConformanceLookupTable.cpp‎

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

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

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

‎lib/Sema/TypeCheckConcurrency.cpp‎

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

61806180
// Local function to form the implicit conformance.
61816181
auto formConformance = [&](const DeclAttribute *attrMakingUnavailable)
6182-
-> NormalProtocolConformance * {
6182+
-> ProtocolConformance * {
61836183
DeclContext *conformanceDC = nominal;
6184+
6185+
// FIXME: @_nonSendable should be a builtin extension macro. This behavior
6186+
// of explanding the unavailable conformance during implicit Sendable
6187+
// derivation means that clients can unknowingly ignore unavailable Sendable
6188+
// Sendable conformances from the original module added via @_nonSendable
6189+
// because they are not expanded if an explicit conformance is found via
6190+
// conformance lookup. So, if a retroactive, unchecked Sendable conformance
6191+
// is written, no redundant conformance warning is emitted.
61846192
if (attrMakingUnavailable) {
61856193
// Conformance availability is currently tied to the declaring extension.
61866194
// FIXME: This is a hack--we should give conformances real availability.
@@ -6208,6 +6216,18 @@ ProtocolConformance *swift::deriveImplicitSendableConformance(
62086216
file->getOrCreateSynthesizedFile().addTopLevelDecl(extension);
62096217

62106218
conformanceDC = extension;
6219+
6220+
// Let the conformance lookup table register the conformance
6221+
// from the extension. Otherwise, we'll end up with redundant
6222+
// conformances between the explicit conformance from the extension
6223+
// and the conformance synthesized below.
6224+
SmallVector<ProtocolConformance *, 2> conformances;
6225+
nominal->lookupConformance(proto, conformances);
6226+
for (auto conformance : conformances) {
6227+
if (conformance->getDeclContext() == conformanceDC) {
6228+
return conformance;
6229+
}
6230+
}
62116231
}
62126232

62136233
auto conformance = ctx.getNormalConformance(

‎lib/Sema/TypeCheckProtocol.cpp‎

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

62746310
// Complain about any declarations in this extension whose names match
62756311
// 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)