Merge pull request swiftlang#36171 from slavapestov/fix-rethrows-conformance-hole

Sema: Fix conformance checking for 'rethrows' protocol requirements

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/TypeCheckEffects.cpp

@@ -239,69 +239,51 @@ class AbstractFunction {
     ParamDecl *TheParameter;
     Expr *TheExpr;
   };
-  unsigned TheKind : 2;
-  unsigned ParamCount : 2;
+  Kind TheKind;
   PolymorphicEffectKind RethrowingKind;
   SubstitutionMap Substitutions;
 
 public:
   explicit AbstractFunction(Kind kind, Expr *fn)
     : TheKind(kind),
-      ParamCount(1),
       RethrowingKind(PolymorphicEffectKind::None) {
     TheExpr = fn;
   }
 
   explicit AbstractFunction(AbstractFunctionDecl *fn, SubstitutionMap subs)
     : TheKind(Kind::Function),
-      ParamCount(fn->getNumCurryLevels()),
       RethrowingKind(fn->getPolymorphicEffectKind(EffectKind::Throws)),
       Substitutions(subs) {
     TheFunction = fn;
   }
 
   explicit AbstractFunction(AbstractClosureExpr *closure)
     : TheKind(Kind::Closure),
-      ParamCount(1),
       RethrowingKind(PolymorphicEffectKind::None) {
     TheClosure = closure;
   }
 
   explicit AbstractFunction(ParamDecl *parameter)
     : TheKind(Kind::Parameter),
-      ParamCount(1),
       RethrowingKind(PolymorphicEffectKind::None) {
     TheParameter = parameter;
   }
 
-  Kind getKind() const { return Kind(TheKind); }
-
-  /// Whether the function is marked 'rethrows'.
-  bool isBodyRethrows() const {
-    switch (RethrowingKind) {
-    case PolymorphicEffectKind::None:
-    case PolymorphicEffectKind::Always:
-      return false;
-
-    case PolymorphicEffectKind::ByClosure:
-    case PolymorphicEffectKind::ByConformance:
-    case PolymorphicEffectKind::Invalid:
-      return true;
-    }
-  }
+  Kind getKind() const { return TheKind; }
 
   PolymorphicEffectKind getRethrowingKind() const {
     return RethrowingKind;
   }
 
-  unsigned getNumArgumentsForFullApply() const {
-    return ParamCount;
-  }
-
   Type getType() const {
     switch (getKind()) {
     case Kind::Opaque: return getOpaqueFunction()->getType();
-    case Kind::Function: return getFunction()->getInterfaceType();
+    case Kind::Function: {
+      auto *AFD = getFunction();
+      if (AFD->hasImplicitSelfDecl())
+        return AFD->getMethodInterfaceType();
+      return AFD->getInterfaceType();
+    }
     case Kind::Closure: return getClosure()->getType();
     case Kind::Parameter: return getParameter()->getType();
     }
@@ -336,23 +318,20 @@ class AbstractFunction {
   }
 
   static AbstractFunction decomposeApply(ApplyExpr *apply,
-                           SmallVectorImpl<SmallVector<Expr *, 2>> &argLists) {
-    Expr *fn;
-    do {
-      auto *argExpr = apply->getArg();
-      if (auto *tupleExpr = dyn_cast<TupleExpr>(argExpr)) {
-        auto args = tupleExpr->getElements();
-        argLists.emplace_back(args.begin(), args.end());
-      } else if (auto *parenExpr = dyn_cast<ParenExpr>(argExpr)) {
-        argLists.emplace_back();
-        argLists.back().push_back(parenExpr->getSubExpr());
-      } else {
-        argLists.emplace_back();
-        argLists.back().push_back(argExpr);
-      }
+                                         SmallVectorImpl<Expr *> &args) {
+    auto *argExpr = apply->getArg();
+    if (auto *tupleExpr = dyn_cast<TupleExpr>(argExpr)) {
+      auto elts = tupleExpr->getElements();
+      args.append(elts.begin(), elts.end());
+    } else {
+      auto *parenExpr = cast<ParenExpr>(argExpr);
+      args.push_back(parenExpr->getSubExpr());
+    }
+
+    Expr *fn = apply->getFn()->getValueProvidingExpr();
 
-      fn = apply->getFn()->getValueProvidingExpr();
-    } while ((apply = dyn_cast<ApplyExpr>(fn)));
+    if (auto *selfCall = dyn_cast<SelfApplyExpr>(fn))
+      fn = selfCall->getFn()->getValueProvidingExpr();
 
     return decomposeFunction(fn);
   }
@@ -702,6 +681,9 @@ class ApplyClassifier {
 
   /// Check to see if the given function application throws or is async.
   Classification classifyApply(ApplyExpr *E) {
+    if (isa<SelfApplyExpr>(E))
+      return Classification();
+
     // An apply expression is a potential throw site if the function throws.
     // But if the expression didn't type-check, suppress diagnostics.
     if (!E->getType() || E->getType()->hasError())
@@ -713,88 +695,66 @@ class ApplyClassifier {
     if (!fnType) return Classification::forInvalidCode();
 
     bool isAsync = fnType->isAsync() || E->implicitlyAsync();
+
     // Decompose the application.
-    SmallVector<SmallVector<Expr *, 2>, 2> argLists;
-    auto fnRef = AbstractFunction::decomposeApply(E, argLists);
+    SmallVector<Expr *, 2> args;
+    auto fnRef = AbstractFunction::decomposeApply(E, args);
 
     // If any of the arguments didn't type check, fail.
-    for (auto argList : argLists) {
-      for (auto *arg : argList) {
-        if (!arg->getType() || arg->getType()->hasError())
-          return Classification::forInvalidCode();
-      }
+    for (auto *arg : args) {
+      if (!arg->getType() || arg->getType()->hasError())
+        return Classification::forInvalidCode();
     }
 
     // If the function doesn't throw at all, we're done here.
     if (!fnType->isThrowing()) {
       return isAsync ? Classification::forAsync() : Classification();
     }
 
-    // If we're applying more arguments than the natural argument
-    // count, then this is a call to the opaque value returned from
-    // the function.
-    if (argLists.size() != fnRef.getNumArgumentsForFullApply()) {
-      // Special case: a reference to an operator within a type might be
-      // missing 'self'.
-      // FIXME: The issue here is that this is an ill-formed expression, but
-      // we don't know it from the structure of the expression.
-      if (argLists.size() == 1 && fnRef.getKind() == AbstractFunction::Function &&
-          isa<FuncDecl>(fnRef.getFunction()) &&
-          cast<FuncDecl>(fnRef.getFunction())->isOperator() &&
-          fnRef.getNumArgumentsForFullApply() == 2 &&
-          fnRef.getFunction()->getDeclContext()->isTypeContext()) {
-        // Can only happen with invalid code.
-        assert(fnRef.getFunction()->getASTContext().Diags.hadAnyError());
-        return Classification::forInvalidCode();
-      }
-
-      assert(argLists.size() > fnRef.getNumArgumentsForFullApply() &&
-             "partial application was throwing?");
-      return Classification::forThrow(PotentialThrowReason::forThrowingApply(),
-                                      isAsync);
-    }
-
-    if (fnRef.getRethrowingKind() == PolymorphicEffectKind::ByConformance) {
+    // Handle rethrowing functions.
+    switch (fnRef.getRethrowingKind()) {
+    case PolymorphicEffectKind::ByConformance: {
       auto substitutions = fnRef.getSubstitutions();
       for (auto conformanceRef : substitutions.getConformances()) {
         if (conformanceRef.hasEffect(EffectKind::Throws)) {
           return Classification::forRethrowingOnly(
             PotentialThrowReason::forRethrowsConformance(E), isAsync);
         }
       }
+
+      // 'ByConformance' is a superset of 'ByClosure', so check for
+      // closure arguments too.
+      LLVM_FALLTHROUGH;
     }
 
-    // If the function's body is 'rethrows' for the number of
-    // arguments we gave it, apply the rethrows logic.
-    if (fnRef.isBodyRethrows()) {
+    case PolymorphicEffectKind::ByClosure: {
       // We need to walk the original parameter types in parallel
       // because it only counts for 'rethrows' purposes if it lines up
       // with a throwing function parameter in the original type.
-      Type type = fnRef.getType();
-      if (!type) return Classification::forInvalidCode();
+      auto *origType = fnRef.getType()->getAs<AnyFunctionType>();
+      if (!origType)
+        return Classification::forInvalidCode();
 
       // Use the most significant result from the arguments.
       Classification result = isAsync ? Classification::forAsync() 
                                       : Classification();
-      for (auto argList : llvm::reverse(argLists)) {
-        auto fnType = type->getAs<AnyFunctionType>();
-        if (!fnType)
-          return Classification::forInvalidCode();
-
-        auto params = fnType->getParams();
-        if (params.size() != argList.size())
-          return Classification::forInvalidCode();
-
-        for (unsigned i = 0, e = params.size(); i < e; ++i) {
-          result.merge(classifyRethrowsArgument(argList[i],
-                                                params[i].getParameterType()));
-        }
 
-        type = fnType->getResult();
+      auto params = origType->getParams();
+      if (params.size() != args.size())
+        return Classification::forInvalidCode();
+
+      for (unsigned i = 0, e = params.size(); i < e; ++i) {
+        result.merge(classifyRethrowsArgument(args[i],
+                                              params[i].getParameterType()));
       }
+
       return result;
     }
 
+    default:
+      break;
+    }
+
     // Try to classify the implementation of functions that we have
     // local knowledge of.
     Classification result =

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);
       }
     }
@@ -2524,6 +2538,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) {