[AST][Sema] Fixes the IterativeTypeChecker and better manages circular protocol inheritance

The IterativeTypeChecker now use loops instead of recursion to help keep the stack size low
We diagnose circular dependencies for protocols in a more efficient manner and also prevent the possibility of infinite loops

Author: manavgabhawala

lib/AST/ConformanceLookupTable.cpp

@@ -445,7 +445,7 @@ bool ConformanceLookupTable::addProtocol(NominalTypeDecl *nominal,
 
   // If this entry is synthesized or implied, scan to determine
   // whether there are any explicit better conformances that make this
-  // conformance trivially superseded (and, therefore, no worth
+  // conformance trivially superseded (and, therefore, not worth
   // recording).
   auto &conformanceEntries = Conformances[protocol];
   if (kind == ConformanceEntryKind::Implied ||
@@ -458,7 +458,9 @@ bool ConformanceLookupTable::addProtocol(NominalTypeDecl *nominal,
 
       case ConformanceEntryKind::Implied:
         // An implied conformance is better than a synthesized one.
-        if (kind == ConformanceEntryKind::Synthesized)
+        // Ignore implied circular protocol inheritance
+        if (kind == ConformanceEntryKind::Synthesized ||
+            existingEntry->getProtocol() == protocol)
           return false;
         break;
 

lib/Sema/ITCDecl.cpp

@@ -220,6 +220,7 @@ void IterativeTypeChecker::processInheritedProtocols(
   // FIXME: Technically, we only need very basic name binding.
   auto inheritedClause = protocol->getInherited();
   bool anyDependencies = false;
+  bool diagnosedCircularity = false;
   llvm::SmallSetVector<ProtocolDecl *, 4> allProtocols;
   for (unsigned i = 0, n = inheritedClause.size(); i != n; ++i) {
     TypeLoc &inherited = inheritedClause[i];
@@ -235,43 +236,32 @@ void IterativeTypeChecker::processInheritedProtocols(
     // FIXME: We'd prefer to keep what the user wrote here.
     SmallVector<ProtocolDecl *, 4> protocols;
     if (inherited.getType()->isExistentialType(protocols)) {
-      allProtocols.insert(protocols.begin(), protocols.end());
-      continue;
+      for (auto inheritedProtocol: protocols) {
+        if (inheritedProtocol == protocol ||
+            inheritedProtocol->inheritsFrom(protocol)) {
+          if (!diagnosedCircularity &&
+			  !protocol->isInheritedProtocolsValid()) {
+            diagnose(protocol,
+                     diag::circular_protocol_def, protocol->getName().str())
+                    .fixItRemove(inherited.getSourceRange());
+            diagnosedCircularity = true;
+          }
+          continue;
+        }
+        allProtocols.insert(inheritedProtocol);
+      }
     }
   }
 
   // If we enumerated any dependencies, we can't complete this request.
   if (anyDependencies)
     return;
 
-  // Check for circular inheritance.
-  // FIXME: The diagnostics here should be improved... and this should probably
-  // be handled by the normal cycle detection.
-  bool diagnosedCircularity = false;
-  for (unsigned i = 0, n = allProtocols.size(); i != n; /*in loop*/) {
-    if (allProtocols[i] == protocol ||
-        allProtocols[i]->inheritsFrom(protocol)) {
-      if (!diagnosedCircularity &&
-          !protocol->isInheritedProtocolsValid()) {
-        diagnose(protocol,
-                 diag::circular_protocol_def, protocol->getName().str());
-        diagnosedCircularity = true;
-      }
-
-      allProtocols.remove(allProtocols[i]);
-      --n;
-      continue;
-    }
-
-    ++i;
-  }
-
   // FIXME: Hack to deal with recursion elsewhere.
   // We recurse through DeclContext::getLocalProtocols() -- this should be
   // redone to use the IterativeDeclChecker also.
   if (protocol->isInheritedProtocolsValid())
     return;
-
   protocol->setInheritedProtocols(getASTContext().AllocateCopy(allProtocols));
 }
 

lib/Sema/IterativeTypeChecker.cpp

@@ -66,49 +66,43 @@ bool IterativeTypeChecker::breakCycle(TypeCheckRequest request) {
 }
 
 void IterativeTypeChecker::satisfy(TypeCheckRequest request) {
-  // If the request has already been satisfied, we're done.
-  if (isSatisfied(request)) return;
-
-  // Check for circular dependencies in our requests.
-  // FIXME: This stack operation is painfully inefficient.
-  auto existingRequest = std::find(ActiveRequests.rbegin(),
-                                   ActiveRequests.rend(),
-                                   request);
-  if (existingRequest != ActiveRequests.rend()) {
-    auto first = existingRequest.base();
-    --first;
-    diagnoseCircularReference(llvm::makeArrayRef(&*first,
-                                                 &*ActiveRequests.end()));
-    return;
-  }
-
-  // Add this request to the stack of active requests.
-  ActiveRequests.push_back(request);
-  defer { ActiveRequests.pop_back(); };
-
-  while (true) {
+  
+  auto startingSize = ActiveRequests.size();
+  
+  auto addToActiveRequests = [&](TypeCheckRequest request) {
+    // Check for circular dependencies in our requests.
+    auto existingRequest = std::find(ActiveRequests.rbegin(),
+                                     ActiveRequests.rend(),
+                                     request);
+    if (existingRequest != ActiveRequests.rend()) {
+      auto first = existingRequest.base();
+      --first;
+      diagnoseCircularReference(llvm::makeArrayRef(&*first,
+                                                   &*ActiveRequests.end()));
+      return;
+    }
+    // Add this request to the stack of active requests.
+    ActiveRequests.push_back(request);
+  };
+  
+  addToActiveRequests(request);
+  
+  while (ActiveRequests.size() != startingSize) {
+    request = ActiveRequests.back();
+    // If the request has already been satisfied, we're done.
+    if (isSatisfied(request)) {
+      ActiveRequests.pop_back();
+      continue;
+    }
+    
     // Process this requirement, enumerating dependencies if anything else needs
     // to be handled first.
-    SmallVector<TypeCheckRequest, 4> unsatisfied;
     process(request, [&](TypeCheckRequest dependency) -> bool {
       if (isSatisfied(dependency)) return false;
-
       // Record the unsatisfied dependency.
-      unsatisfied.push_back(dependency);
+      addToActiveRequests(dependency);
       return true;
     });
-
-    // If there were no unsatisfied dependencies, we're done.
-    if (unsatisfied.empty()) {
-      assert(isSatisfied(request));
-      break;
-    }
-
-    // Recurse to satisfy any unsatisfied dependencies.
-    // FIXME: Don't recurse in the iterative type checker, silly!
-    for (auto dependency : unsatisfied) {
-      satisfy(dependency);
-    }
   }
 }
 

validation-test/compiler_crashers/10659-swift-printingdiagnosticconsumer-handlediagnostic.timeout.swift renamed to validation-test/compiler_crashers_fixed/10659-swift-printingdiagnosticconsumer-handlediagnostic.timeout.swift

@@ -5,7 +5,7 @@
 // See http://swift.org/LICENSE.txt for license information
 // See http://swift.org/CONTRIBUTORS.txt for the list of Swift project authors
 
-// RUN: not --crash %target-swift-frontend %s -parse
+// RUN: not %target-swift-frontend %s -parse
 // REQUIRES: asserts
 protocol c:A
 protocol A:d