Sema: Fix conformance checking for 'rethrows' protocol requirements

A protocol requirement in a non-'@rethrows' protocol cannot be
witnessed by a rethrows-via-conformance witness, because there
is not enough information at a potential call site of the
protocol requirement to determine if the witness uses a
rethrows source or not.

Author: slavapestov

include/swift/AST/DiagnosticsSema.def

@@ -2232,6 +2232,9 @@ NOTE(protocol_witness_settable_conflict,none,
      "candidate is not settable, but protocol requires it", ())
 NOTE(protocol_witness_rethrows_conflict,none,
      "candidate is not 'rethrows', but protocol requires it", ())
+NOTE(protocol_witness_rethrows_by_conformance_conflict,none,
+     "candidate is 'rethrows' via a conformance, "
+     "but the protocol requirement is not from a '@rethrows' protocol", ())
 NOTE(protocol_witness_throws_conflict,none,
      "candidate throws, but protocol does not allow it", ())
 NOTE(protocol_witness_not_objc,none,

lib/Sema/TypeCheckProtocol.cpp

@@ -558,25 +558,39 @@ swift::matchWitness(
     if (!funcReq->isMutating() && funcWitness->isMutating())
       return RequirementMatch(witness, MatchKind::MutatingConflict);
 
-    // If the requirement is rethrows, the witness must either be
-    // rethrows or be non-throwing if the requirement is not by conformance
-    // else the witness can be by conformance, throwing or non throwing
-    if (reqAttrs.hasAttribute<RethrowsAttr>() &&
-        !witnessAttrs.hasAttribute<RethrowsAttr>()) {
+    // If the requirement has an explicit 'rethrows' argument, the witness
+    // must be 'rethrows', too.
+    if (reqAttrs.hasAttribute<RethrowsAttr>()) {
       auto reqRethrowingKind =
           funcReq->getPolymorphicEffectKind(EffectKind::Throws);
       auto witnessRethrowingKind =
           funcWitness->getPolymorphicEffectKind(EffectKind::Throws);
-      if (reqRethrowingKind == PolymorphicEffectKind::ByConformance) {
-        switch (witnessRethrowingKind) {
-        case PolymorphicEffectKind::ByConformance:
-        case PolymorphicEffectKind::Always:
-        case PolymorphicEffectKind::None:
+
+      assert(reqRethrowingKind != PolymorphicEffectKind::Always &&
+             reqRethrowingKind != PolymorphicEffectKind::None);
+
+      switch (witnessRethrowingKind) {
+      case PolymorphicEffectKind::None:
+      case PolymorphicEffectKind::Invalid:
+      case PolymorphicEffectKind::ByClosure:
+        break;
+
+      case PolymorphicEffectKind::ByConformance: {
+        // A by-conformance `rethrows` witness cannot witness a
+        // by-conformance `rethrows` requirement unless the protocol
+        // is @rethrows. Otherwise, we don't have enough information
+        // at the call site to assess if the conformance actually
+        // throws or not.
+        auto *proto = cast<ProtocolDecl>(req->getDeclContext());
+        if (reqRethrowingKind == PolymorphicEffectKind::ByConformance &&
+            proto->hasPolymorphicEffect(EffectKind::Throws))
           break;
-        default:
-          return RequirementMatch(witness, MatchKind::RethrowsConflict);
-        }
-      } else if (cast<AbstractFunctionDecl>(witness)->hasThrows()) {
+
+        return RequirementMatch(witness,
+                                MatchKind::RethrowsByConformanceConflict);
+      }
+
+      case PolymorphicEffectKind::Always:
         return RequirementMatch(witness, MatchKind::RethrowsConflict);
       }
     }
@@ -2525,6 +2539,13 @@ diagnoseMatch(ModuleDecl *module, NormalProtocolConformance *conformance,
     diag.fixItReplace(FD->getThrowsLoc(), getTokenText(tok::kw_rethrows));
     break;
   }
+  case MatchKind::RethrowsByConformanceConflict: {
+    auto witness = match.Witness;
+    auto diag =
+        diags.diagnose(witness,
+                       diag::protocol_witness_rethrows_by_conformance_conflict);
+    break;
+  }
   case MatchKind::NonObjC:
     diags.diagnose(match.Witness, diag::protocol_witness_not_objc);
     break;

lib/Sema/TypeCheckProtocol.h

@@ -263,9 +263,13 @@ enum class MatchKind : uint8_t {
   /// The witness did not match because of __consuming conflicts.
   ConsumingConflict,
 
-  /// The witness is not rethrows, but the requirement is.
+  /// The witness throws unconditionally, but the requirement rethrows.
   RethrowsConflict,
 
+  /// The witness rethrows via conformance, but the requirement rethrows
+  /// via closure and is not in a '@rethrows' protocol.
+  RethrowsByConformanceConflict,
+
   /// The witness is explicitly @nonobjc but the requirement is @objc.
   NonObjC,
 
@@ -498,6 +502,7 @@ struct RequirementMatch {
     case MatchKind::NonMutatingConflict:
     case MatchKind::ConsumingConflict:
     case MatchKind::RethrowsConflict:
+    case MatchKind::RethrowsByConformanceConflict:
     case MatchKind::AsyncConflict:
     case MatchKind::ThrowsConflict:
     case MatchKind::NonObjC:
@@ -530,6 +535,7 @@ struct RequirementMatch {
     case MatchKind::NonMutatingConflict:
     case MatchKind::ConsumingConflict:
     case MatchKind::RethrowsConflict:
+    case MatchKind::RethrowsByConformanceConflict:
     case MatchKind::AsyncConflict:
     case MatchKind::ThrowsConflict:
     case MatchKind::NonObjC:

test/attr/attr_rethrows_protocol.swift

@@ -139,22 +139,32 @@ func takesEmpty<T : Empty>(_: T) rethrows {}
 
 takesEmpty(EmptyWitness())
 
-// FIXME: Fix this soundness hole
+// Rethrows kinds need to line up when a 'rethrows' function witnesses a
+// 'rethrows' protocol requirement
 
-// Note: SimpleThrowsClosure is not @rethrows
+// Note: the SimpleThrowsClosure protocol is not @rethrows
 protocol SimpleThrowsClosure {
   func doIt(_: () throws -> ()) rethrows
+  // expected-note@-1 {{protocol requires function 'doIt' with type '(() throws -> ()) throws -> ()'; do you want to add a stub?}}
+
+  func doIt2<T : Empty>(_: T) rethrows
+  // expected-note@-1 {{protocol requires function 'doIt2' with type '<T> (T) throws -> ()'; do you want to add a stub?}}
 }
 
 struct ConformsToSimpleThrowsClosure<T : RethrowingProtocol> : SimpleThrowsClosure {
+// expected-error@-1 {{type 'ConformsToSimpleThrowsClosure<T>' does not conform to protocol 'SimpleThrowsClosure'}}
   let t: T
 
   // This cannot witness SimpleThrowsClosure.doIt(), because the
   // T : RethrowingProtocol conformance is a source here, but that
   // is not captured in the protocol's requirement signature.
   func doIt(_: () throws -> ()) rethrows {
+  // expected-note@-1 {{candidate is 'rethrows' via a conformance, but the protocol requirement is not from a '@rethrows' protocol}}
     try t.source()
   }
+
+  func doIt2<T : Empty>(_: T) rethrows {}
+  // expected-note@-1 {{candidate is 'rethrows' via a conformance, but the protocol requirement is not from a '@rethrows' protocol}}
 }
 
 func soundnessHole<T : SimpleThrowsClosure>(_ t: T) {