Reimplement circularly check for protocol inheritance.

First, make it actually check for cycles properly. Second, pull it
into the checking of the protocol itself, rather than keeping it as a
separate pass that happens too late to be useful. Finally, put the
unchecked/checking/checked bits into the AST to avoid having to keep a
separate DenseMap just for this purpose. Fixes <rdar://problem/14750346>.


Swift SVN r7324

include/swift/AST/Decl.h

@@ -73,6 +73,16 @@ enum class DeclKind : uint8_t {
 #include "swift/AST/DeclNodes.def"
 };
 
+/// Keeps track of stage of circularity checking for the given protocol.
+enum class CircularityCheck {
+  /// Circularity has not yet been checked.
+  Unchecked,
+  /// We're currently checking circularity.
+  Checking,
+  // Circularity has already been checked.
+  Checked
+};
+
 /// Decl - Base class for all declarations in Swift.
 class alignas(8) Decl {
   // alignas(8) because we use three tag bits on Decl*.
@@ -159,8 +169,11 @@ class alignas(8) Decl {
     /// If this is a compiler-known protocol, this will be a KnownProtocolKind
     /// value, plus one. Otherwise, it will be 0.
     unsigned KnownProtocol : 4;
+
+    /// The stage of the circularity check for this protocol.
+    unsigned Circularity : 2;
   };
-  enum { NumProtocolDeclBits = NumNominalTypeDeclBits + 8 };
+  enum { NumProtocolDeclBits = NumNominalTypeDeclBits + 10 };
   static_assert(NumProtocolDeclBits <= 32, "fits in an unsigned");
 
   class InfixOperatorDeclBitFields {
@@ -1636,6 +1649,16 @@ public:
     assert(*getKnownProtocolKind() == kind && "not enough bits");
   }
 
+  /// Retrieve the status of circularity checking for protocol inheritance.
+  CircularityCheck getCircularityCheck() const {
+    return static_cast<CircularityCheck>(ProtocolDeclBits.Circularity);
+  }
+
+  /// Record the current stage of circularity checking.
+  void setCircularityCheck(CircularityCheck circularity) {
+    ProtocolDeclBits.Circularity = static_cast<unsigned>(circularity);
+  }
+
   // Implement isa/cast/dyncast/etc.
   static bool classof(const Decl *D) {
     return D->getKind() == DeclKind::Protocol;

lib/AST/Decl.cpp

@@ -628,6 +628,8 @@ ProtocolDecl::ProtocolDecl(DeclContext *DC, SourceLoc ProtocolLoc,
   ProtocolDeclBits.ExistentialConformsToSelfValid = false;
   ProtocolDeclBits.ExistentialConformsToSelf = false;
   ProtocolDeclBits.KnownProtocol = 0;
+  ProtocolDeclBits.Circularity
+    = static_cast<unsigned>(CircularityCheck::Unchecked);
 }
 
 bool ProtocolDecl::inheritsFrom(const ProtocolDecl *Super) const {

lib/Sema/TypeCheckDecl.cpp

@@ -20,6 +20,7 @@
 #include "swift/AST/Attr.h"
 #include "swift/AST/PrettyStackTrace.h"
 #include "swift/Parse/Lexer.h"
+#include "llvm/ADT/SmallString.h"
 #include "llvm/ADT/Twine.h"
 using namespace swift;
 
@@ -77,7 +78,7 @@ static void checkInheritanceClause(TypeChecker &tc, Decl *decl) {
     if (type->checkedInheritanceClause())
       return;
 
-    // FIXME: This breaks infinite recursion, but doesn't diagnose it well.
+    // This breaks infinite recursion, which will be diagnosed separately.
     type->setCheckedInheritanceClause();
     inheritedClause = type->getInherited();
   } else {
@@ -85,7 +86,7 @@ static void checkInheritanceClause(TypeChecker &tc, Decl *decl) {
     if (ext->checkedInheritanceClause())
       return;
 
-    // FIXME: This breaks infinite recursion, but doesn't diagnose it well.
+    // This breaks infinite recursion, which will be diagnosed separately.
     ext->setCheckedInheritanceClause();
     inheritedClause = ext->getInherited();
   }
@@ -241,6 +242,56 @@ static void checkInheritanceClause(TypeChecker &tc, Decl *decl) {
   }
 }
 
+/// Check for circular inheritance among protocols, starting at the given
+/// protocol.
+static void checkProtocolCircularity(TypeChecker &tc, ProtocolDecl *proto,
+                                     SmallVectorImpl<ProtocolDecl *> &path) {
+  switch (proto->getCircularityCheck()) {
+  case CircularityCheck::Checked:
+    return;
+
+  case CircularityCheck::Checking: {
+    // We're already checking this protocol, which means we have a cycle.
+
+    // Find the beginning of the cycle within the full path.
+    auto cycleStart = path.end()-2;
+    while (*cycleStart != proto) {
+      assert(cycleStart != path.begin() && "Missing cycle start?");
+      --cycleStart;
+    }
+
+    // Form the textual path illustrating the cycle.
+    llvm::SmallString<128> pathStr;
+    for (auto i = cycleStart, iEnd = path.end(); i != iEnd; ++i) {
+      if (!pathStr.empty())
+        pathStr += " -> ";
+      pathStr += ("'" + (*i)->getName().str() + "'").str();
+    }
+    pathStr += (" -> '" + proto->getName().str() + "'").str();
+
+    // Diagnose the cycle.
+    tc.diagnose(proto->getLoc(), diag::circular_protocol_def, pathStr);
+    for (auto i = cycleStart + 1, iEnd = path.end(); i != iEnd; ++i) {
+      tc.diagnose(*i, diag::protocol_here, (*i)->getName());
+    }
+
+    proto->setInvalid();
+    break;
+  }
+
+  case CircularityCheck::Unchecked:
+    // Check the inherited protocols for circular inheritance.
+    path.push_back(proto);
+    proto->setCircularityCheck(CircularityCheck::Checking);
+    for (auto inheritedProto : tc.getDirectConformsTo(proto)) {
+      checkProtocolCircularity(tc, inheritedProto, path);
+    }
+    proto->setCircularityCheck(CircularityCheck::Checked);
+    path.pop_back();
+    break;
+  }
+}
+
 namespace {
 
 class DeclChecker : public DeclVisitor<DeclChecker> {
@@ -775,6 +826,12 @@ public:
 
     checkInheritanceClause(TC, PD);
 
+    {
+      // Check for circular inheritance within the protocol.
+      SmallVector<ProtocolDecl *, 8> path;
+      checkProtocolCircularity(TC, PD, path);
+    }
+
     // If the protocol is [objc], it may only refine other [objc] protocols.
     // FIXME: Revisit this restriction.
     if (PD->getAttrs().isObjC()) {

lib/Sema/TypeChecker.cpp

@@ -170,37 +170,6 @@ Type TypeChecker::lookupBoolType() {
   });
 }
 
-/// \brief Check for circular inheritance of protocols.
-///
-/// \param Path The circular path through the protocol inheritance hierarchy,
-/// which will be constructed (backwards) if there is in fact a circular path.
-///
-/// \returns True if there was circular inheritance, false otherwise.
-bool checkProtocolCircularity(TypeChecker &TC, ProtocolDecl *Proto,
-                              llvm::SmallPtrSet<ProtocolDecl *, 16> &Visited,
-                              llvm::SmallPtrSet<ProtocolDecl *, 16> &Local,
-                              SmallVectorImpl<ProtocolDecl *> &Path) {
-  for (auto inheritedProto : Proto->getProtocols()) {
-    if (Visited.count(inheritedProto)) {
-      // We've seen this protocol as part of another protocol search;
-      // it's not circular.
-      continue;
-    }
-
-    // Whether we've seen this protocol before in our search or visiting it
-    // detects a circularity, record it in the path and abort.
-    if (!Local.insert(inheritedProto) ||
-        checkProtocolCircularity(TC, inheritedProto, Visited, Local, Path)) {
-      Path.push_back(inheritedProto);
-      return true;
-    }
-
-
-  }
-  
-  return false;
-}
-
 static void overrideDecl(TypeChecker &TC,
                          llvm::SmallPtrSet<ValueDecl*, 16> &OverriddenDecls,
                          ValueDecl *MemberVD, ValueDecl *OverriddenDecl) {
@@ -471,44 +440,6 @@ void swift::performTypeChecking(TranslationUnit *TU, unsigned StartElem) {
     TC.typeCheckDecl(D, /*isFirstPass*/true);
   }
 
-  // Check for explicit conformance to protocols and for circularity in
-  // protocol definitions.
-  {
-    // FIXME: This check should be in TypeCheckDecl.
-    llvm::SmallPtrSet<ProtocolDecl *, 16> VisitedProtocols;
-    for (auto D : TU->Decls) {
-      if (auto Protocol = dyn_cast<ProtocolDecl>(D)) {
-        // Check for circular protocol definitions.
-        llvm::SmallPtrSet<ProtocolDecl *, 16> LocalVisited;
-        SmallVector<ProtocolDecl *, 4> Path;
-        if (VisitedProtocols.count(Protocol) == 0) {
-          LocalVisited.insert(Protocol);
-          if (checkProtocolCircularity(TC, Protocol, VisitedProtocols,
-                                       LocalVisited, Path)) {
-            llvm::SmallString<128> PathStr;
-            PathStr += "'";
-            PathStr += Protocol->getName().str();
-            PathStr += "'";
-            for (unsigned I = Path.size(); I != 0; --I) {
-              PathStr += " -> '";
-              PathStr += Path[I-1]->getName().str();
-              PathStr += "'";
-            }
-            
-            TC.diagnose(Protocol->getLoc(), diag::circular_protocol_def,
-                        PathStr);
-            for (unsigned I = Path.size(); I != 1; --I) {
-              TC.diagnose(Path[I-1], diag::protocol_here,
-                          Path[I-1]->getName());
-            }
-          }
-          
-          VisitedProtocols.insert(LocalVisited.begin(), LocalVisited.end());
-        }
-      }
-    }
-  }
-
   // Check for correct overriding in classes.  We have to do this after the
   // first pass so we have valid types, but before the second pass because
   // it affects name lookup.  The loop here is a bit complicated because we

lib/Serialization/ModuleFile.cpp

@@ -1036,6 +1036,7 @@ Decl *ModuleFile::getDecl(DeclID DID, Optional<DeclContext *> ForcedContext,
     assert(members.hasValue() && "could not read struct members");
     proto->setMembers(members.getValue(), SourceRange());
     proto->setCheckedInheritanceClause();
+    proto->setCircularityCheck(CircularityCheck::Checked);
     break;
   }