Sema: Fix recursion into associated conformances when checking for rethrows

The logic for recursively expanding protocol requirements was broken.
Instead, let's do the recursive visitation when evaluating a
conformance to a @rethrows protocol.

Author: slavapestov

include/swift/AST/Effects.h

@@ -30,27 +30,25 @@ namespace swift {
 class ValueDecl;
 
 class ProtocolRethrowsRequirementList {
-public:
-  typedef std::pair<Type, ValueDecl *> Entry;
-
+  using ThrowingRequirements = ArrayRef<AbstractFunctionDecl *>;
+  using ThrowingConformances = ArrayRef<std::pair<Type, ProtocolDecl *>>;
 private:
-  ArrayRef<Entry> entries;
+  ThrowingRequirements requirements;
+  ThrowingConformances conformances;
 
 public:
-  ProtocolRethrowsRequirementList(ArrayRef<Entry> entries) : entries(entries) {}
-  ProtocolRethrowsRequirementList() : entries() {}
-
-  typedef const Entry *const_iterator;
-  typedef const_iterator iterator;
-
-  const_iterator begin() const { return entries.begin(); }
-  const_iterator end() const { return entries.end(); }
-
-  size_t size() const { return entries.size(); }
-
-  void print(raw_ostream &OS) const;
-
-  SWIFT_DEBUG_DUMP;
+  ProtocolRethrowsRequirementList(ThrowingRequirements requirements,
+                                  ThrowingConformances conformances)
+    : requirements(requirements), conformances(conformances) {}
+  ProtocolRethrowsRequirementList() {}
+
+  ThrowingRequirements getRequirements() const {
+    return requirements;
+  }
+
+  ThrowingConformances getConformances() const {
+    return conformances;
+  }
 };
 
 void simple_display(llvm::raw_ostream &out, const ProtocolRethrowsRequirementList reqs);

include/swift/AST/TypeCheckRequests.h

@@ -25,6 +25,7 @@
 #include "swift/AST/Type.h"
 #include "swift/AST/Evaluator.h"
 #include "swift/AST/Pattern.h"
+#include "swift/AST/ProtocolConformance.h"
 #include "swift/AST/SimpleRequest.h"
 #include "swift/AST/SourceFile.h"
 #include "swift/AST/TypeResolutionStage.h"
@@ -331,9 +332,9 @@ class ProtocolRethrowsRequirementsRequest :
   bool isCached() const { return true; }
 };
 
-class ProtocolConformanceRefClassifyAsThrowsRequest : 
-    public SimpleRequest<ProtocolConformanceRefClassifyAsThrowsRequest,
-                         bool(ProtocolConformanceRef),
+class ProtocolConformanceClassifyAsThrowsRequest : 
+    public SimpleRequest<ProtocolConformanceClassifyAsThrowsRequest,
+                         bool(ProtocolConformance *),
                          RequestFlags::Cached> {
 public:
   using SimpleRequest::SimpleRequest;
@@ -343,7 +344,7 @@ class ProtocolConformanceRefClassifyAsThrowsRequest :
 
   // Evaluation.
   bool 
-  evaluate(Evaluator &evaluator, ProtocolConformanceRef conformanceRef) const;
+  evaluate(Evaluator &evaluator, ProtocolConformance *conformance) const;
 
 public:
   // Caching.

include/swift/AST/TypeCheckerTypeIDZone.def

@@ -270,7 +270,7 @@ SWIFT_REQUEST(TypeChecker, ResolveTypeEraserTypeRequest,
 SWIFT_REQUEST(TypeChecker, ProtocolRethrowsRequirementsRequest,
               ProtocolRethrowsRequirementList(ProtocolDecl *), 
               Cached, NoLocationInfo)
-SWIFT_REQUEST(TypeChecker, ProtocolConformanceRefClassifyAsThrowsRequest,
+SWIFT_REQUEST(TypeChecker, ProtocolConformanceClassifyAsThrowsRequest,
               bool(ProtocolConformanceRef), 
               Cached, NoLocationInfo)
 SWIFT_REQUEST(TypeChecker, ResolveTypeRequest,

lib/AST/Effects.cpp

@@ -28,9 +28,16 @@ using namespace swift;
 
 void swift::simple_display(llvm::raw_ostream &out,
                            const ProtocolRethrowsRequirementList list) {
-  for (auto entry : list) {
-    simple_display(out, entry.first);
-    simple_display(out, entry.second);
+  for (auto req : list.getRequirements()) {
+    simple_display(out, req);
+    out << "\n";
+  }
+
+  for (auto conf : list.getConformances()) {
+    simple_display(out, conf.first);
+    out << " : ";
+    simple_display(out, conf.second);
+    llvm::errs() << "\n";
   }
 }
 
@@ -75,6 +82,6 @@ void swift::simple_display(llvm::raw_ostream &out,
 bool ProtocolConformanceRef::classifyAsThrows() const {
   if (!isConcrete()) { return true; }
   return evaluateOrDefault(getRequirement()->getASTContext().evaluator,
-     ProtocolConformanceRefClassifyAsThrowsRequest{ *this }, 
+     ProtocolConformanceClassifyAsThrowsRequest{getConcrete()}, 
      true);
 }

lib/Sema/TypeCheckEffects.cpp

@@ -56,44 +56,40 @@ static bool hasThrowingFunctionClosureParameter(CanType type) {
 ProtocolRethrowsRequirementList
 ProtocolRethrowsRequirementsRequest::evaluate(Evaluator &evaluator,
                                               ProtocolDecl *decl) const {
-  SmallVector<std::pair<Type, ValueDecl*>, 2> found;
-  llvm::DenseSet<ProtocolDecl*> checkedProtocols;
-
   ASTContext &ctx = decl->getASTContext();
 
   // only allow rethrowing requirements to be determined from marked protocols
-  if (!decl->getAttrs().hasAttribute<swift::AtRethrowsAttr>()) {
-    return ProtocolRethrowsRequirementList(ctx.AllocateCopy(found));
+  if (!decl->isRethrowingProtocol()) {
+    return ProtocolRethrowsRequirementList();
   }
 
+  SmallVector<AbstractFunctionDecl *, 2> requirements;
+  SmallVector<std::pair<Type, ProtocolDecl *>, 2> conformances;
+  
   // check if immediate members of protocol are 'throws'
   for (auto member : decl->getMembers()) {
     auto fnDecl = dyn_cast<AbstractFunctionDecl>(member);
     if (!fnDecl || !fnDecl->hasThrows())
       continue;
 
-    found.push_back(
-      std::pair<Type, ValueDecl*>(decl->getSelfInterfaceType(), fnDecl));
+    requirements.push_back(fnDecl);
   }
-  checkedProtocols.insert(decl);
 
   // check associated conformances of associated types or inheritance
   for (auto requirement : decl->getRequirementSignature()) {
-    if (requirement.getKind() != RequirementKind::Conformance) {
+    if (requirement.getKind() != RequirementKind::Conformance)
       continue;
-    }
+
     auto protoTy = requirement.getSecondType()->castTo<ProtocolType>();
-    auto proto = protoTy->getDecl();
-    if (checkedProtocols.count(proto) != 0) {
+    auto protoDecl = protoTy->getDecl();
+    if (!protoDecl->isRethrowingProtocol())
       continue;
-    }
-    checkedProtocols.insert(proto);
-    for (auto entry : proto->getRethrowingRequirements()) {
-      found.emplace_back(requirement.getFirstType(), entry.second);
-    }
+
+    conformances.emplace_back(requirement.getFirstType(), protoDecl);
   }
 
-  return ProtocolRethrowsRequirementList(ctx.AllocateCopy(found));
+  return ProtocolRethrowsRequirementList(ctx.AllocateCopy(requirements),
+                                         ctx.AllocateCopy(conformances));
 }
 
 FunctionRethrowingKind
@@ -135,14 +131,14 @@ FunctionRethrowingKindRequest::evaluate(Evaluator &evaluator,
   return FunctionRethrowingKind::None;
 }
 
-static bool classifyRequirement(ModuleDecl *module, 
-                                ProtocolConformance *reqConformance, 
-                                ValueDecl *requiredFn) {
-  auto declRef = reqConformance->getWitnessDeclRef(requiredFn);
+static bool classifyWitness(ModuleDecl *module, 
+                            ProtocolConformance *conformance, 
+                            AbstractFunctionDecl *req) {
+  auto declRef = conformance->getWitnessDeclRef(req);
   auto witnessDecl = cast<AbstractFunctionDecl>(declRef.getDecl());
   switch (witnessDecl->getRethrowingKind()) {
     case FunctionRethrowingKind::ByConformance: {
-      auto substitutions = reqConformance->getSubstitutions(module);
+      auto substitutions = conformance->getSubstitutions(module);
       for (auto conformanceRef : substitutions.getConformances()) {
         if (conformanceRef.classifyAsThrows()) {
           return true;
@@ -160,43 +156,42 @@ static bool classifyRequirement(ModuleDecl *module,
   return false;
 }
 
-// classify the type requirements of a given protocol type with a function
-// requirement as throws or not. This will detect if the signature of the 
-// function is throwing or not depending on associated types.
-static bool classifyTypeRequirement(ModuleDecl *module, Type protoType, 
-                                    ValueDecl *requiredFn, 
-                                    ProtocolConformance *conformance,
-                                    ProtocolDecl *requiredProtocol) {
-  auto reqProtocol = cast<ProtocolDecl>(requiredFn->getDeclContext());
-  ProtocolConformance *reqConformance;
-
-  if(protoType->isEqual(reqProtocol->getSelfInterfaceType()) && 
-     requiredProtocol == reqProtocol) {
-    reqConformance = conformance;
-  } else {
-    auto reqConformanceRef = 
-      conformance->getAssociatedConformance(protoType, reqProtocol);
-    if (!reqConformanceRef.isConcrete()) {
-      return true;
+bool
+ProtocolConformanceClassifyAsThrowsRequest::evaluate(
+  Evaluator &evaluator, ProtocolConformance *conformance) const {
+  auto *module = conformance->getDeclContext()->getParentModule();
+
+  llvm::SmallDenseSet<ProtocolConformance *, 2> visited;
+  SmallVector<ProtocolConformance *, 2> worklist;
+
+  worklist.push_back(conformance);
+
+  while (!worklist.empty()) {
+    auto *current = worklist.back();
+    worklist.pop_back();
+
+    if (!visited.insert(current).second)
+      continue;
+
+    auto protoDecl = current->getProtocol();
+
+    auto list = protoDecl->getRethrowingRequirements();
+    for (auto req : list.getRequirements()) {
+      if (classifyWitness(module, current, req))
+        return true;
     }
-    reqConformance = reqConformanceRef.getConcrete();
-  }
 
-  return classifyRequirement(module, reqConformance, requiredFn);
-}
+    for (auto pair : list.getConformances()) {
+      auto assocConf = 
+          current->getAssociatedConformance(
+              pair.first, pair.second);
+      if (!assocConf.isConcrete())
+        return true;
 
-bool
-ProtocolConformanceRefClassifyAsThrowsRequest::evaluate(
-  Evaluator &evaluator, ProtocolConformanceRef conformanceRef) const {
-  auto conformance = conformanceRef.getConcrete();
-  auto requiredProtocol = conformanceRef.getRequirement();
-  auto module = requiredProtocol->getModuleContext();
-  for (auto req : requiredProtocol->getRethrowingRequirements()) {
-    if (classifyTypeRequirement(module, req.first, req.second, 
-                                conformance, requiredProtocol)) {
-      return true;
+      worklist.push_back(assocConf.getConcrete());
     }
   }
+
   return false;
 }
 

test/attr/attr_rethrows_protocol.swift

@@ -162,4 +162,33 @@ func soundnessHole<T : SimpleThrowsClosure>(_ t: T) {
 }
 
 // This actually can throw...
-soundnessHole(ConformsToSimpleThrowsClosure(t: Throws()))
+soundnessHole(ConformsToSimpleThrowsClosure(t: Throws()))
+
+// Test deeply-nested associated conformances
+@rethrows protocol First {
+  associatedtype A : Second
+}
+
+@rethrows protocol Second {
+  associatedtype B : Third
+}
+
+@rethrows protocol Third {
+  func f() throws
+}
+
+struct FirstWitness : First {
+  typealias A = SecondWitness
+}
+
+struct SecondWitness : Second {
+  typealias B = ThirdWitness
+}
+
+struct ThirdWitness : Third {
+  func f() {}
+}
+
+func takesFirst<T : First>(_: T) rethrows {}
+
+takesFirst(FirstWitness())