Merge pull request #3747 from CodaFi/circumnavigating-a-graph-in-eighty-days

[Sema] Shepherd @manavgabhawala's changes to circular protocol diagnostics

Author: CodaFi

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

@@ -222,6 +222,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];
@@ -237,43 +238,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));
 }
 

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