Merge pull request #75135 from hborla/unavailable-superclass-conformance

hborla · web-flow · commit e08bd9565d79 · 2024-07-13T10:15:49.000-07:00
[ConformanceLookup] Don't allow skipping inherited unavailable conformances in favor of explicit available ones.
diff --git 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
@@ -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);
   }
 }
 
@@ -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.
@@ -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;
   }
 
@@ -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()));
@@ -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(),
diff --git 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<ExtensionDecl>(concrete->getDeclContext());
+  auto *dc = concrete->getRootConformance()->getDeclContext();
+  auto ext = dyn_cast<ExtensionDecl>(dc);
   if (ext && AvailableAttr::isUnavailable(ext))
     return true;
 
diff --git a/lib/Sema/TypeCheckConcurrency.cpp b/lib/Sema/TypeCheckConcurrency.cpp
@@ -6413,8 +6413,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.
@@ -6442,6 +6450,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(
@@ -6463,9 +6483,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();
diff --git 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<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.
diff --git a/test/Concurrency/Inputs/SendableConformances.swift b/test/Concurrency/Inputs/SendableConformances.swift
@@ -0,0 +1,9 @@
+
+public class NonSendableClass {}
+
+@available(*, unavailable)
+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
@@ -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>
diff --git a/test/Concurrency/sendable_checking.swift b/test/Concurrency/sendable_checking.swift
@@ -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}}
 }
 
 
diff --git 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 {
