Sema: Improved recovery from circular generic signature construction

Returning a null GenericSignature is not the right way to break a cycle,
because then callers have to be careful to handle the case of a null
GenericSignature together with a non-null GenericParamList, for example
in applyGenericArguments().

An even worse problem can occur when a GenericSignatureRequest for a
nested generic declaration requests the signature of the parent context,
which hits a cycle. In this case, we would build a signature where
the first generic parameter did not have depth 0.

This makes the requirement machine upset, so this patch implements a new
strategy to break such cycles. Instead of returning a null
GenericSignature, we build a signature with the correct generic
parameters, but no requirements. The generic parameters can be computed
just by traversing GenericParamLists, which does not trigger more
GenericSignatureRequests, so this should be safe.

Author: slavapestov

include/swift/AST/DiagnosticsSema.def

@@ -2430,6 +2430,10 @@ ERROR(requires_generic_param_made_equal_to_concrete,none,
       (Type))
 ERROR(recursive_decl_reference,none,
       "%0 %1 references itself", (DescriptiveDeclKind, DeclBaseName))
+ERROR(recursive_generic_signature,none,
+      "%0 %1 has self-referential generic requirements", (DescriptiveDeclKind, DeclBaseName))
+ERROR(recursive_generic_signature_extension,none,
+      "extension of %0 %1 has self-referential generic requirements", (DescriptiveDeclKind, DeclBaseName))
 ERROR(recursive_same_type_constraint,none,
       "same-type constraint %0 == %1 is recursive", (Type, Type))
 ERROR(recursive_superclass_constraint,none,

include/swift/AST/TypeCheckRequests.h

@@ -1491,6 +1491,8 @@ class GenericSignatureRequest :
   bool isCached() const { return true; }
   Optional<GenericSignature> getCachedResult() const;
   void cacheResult(GenericSignature value) const;
+
+  void diagnoseCycle(DiagnosticEngine &diags) const;
 };
 
 /// Compute the underlying interface type of a typealias.

lib/AST/Decl.cpp

@@ -982,10 +982,47 @@ bool GenericContext::isComputingGenericSignature() const {
                  GenericSignatureRequest{const_cast<GenericContext*>(this)});
 }
 
+/// If we hit a cycle while building the generic signature, we can't return
+/// nullptr, since this breaks invariants elsewhere. Instead, build a dummy
+/// signature with no requirements.
+static GenericSignature getPlaceholderGenericSignature(
+    const DeclContext *DC) {
+  SmallVector<GenericParamList *, 2> gpLists;
+  DC->forEachGenericContext([&](GenericParamList *genericParams) {
+    gpLists.push_back(genericParams);
+  });
+
+  if (gpLists.empty())
+    return nullptr;
+
+  std::reverse(gpLists.begin(), gpLists.end());
+  for (unsigned i : indices(gpLists))
+    gpLists[i]->setDepth(i);
+
+  SmallVector<GenericTypeParamType *, 2> result;
+  for (auto *genericParams : gpLists) {
+    for (auto *genericParam : *genericParams) {
+      result.push_back(genericParam->getDeclaredInterfaceType()
+                                   ->castTo<GenericTypeParamType>());
+    }
+  }
+
+  return GenericSignature::get(result, {});
+}
+
 GenericSignature GenericContext::getGenericSignature() const {
-  return evaluateOrDefault(
-      getASTContext().evaluator,
-      GenericSignatureRequest{const_cast<GenericContext *>(this)}, nullptr);
+  // Don't use evaluateOrDefault() here, because getting the 'default value'
+  // is slightly expensive here so we don't want to do it eagerly.
+  auto result = getASTContext().evaluator(
+      GenericSignatureRequest{const_cast<GenericContext *>(this)});
+  if (auto err = result.takeError()) {
+    llvm::handleAllErrors(std::move(err),
+      [](const CyclicalRequestError<GenericSignatureRequest> &E) {
+        // cycle detected
+      });
+    return getPlaceholderGenericSignature(this);
+  }
+  return *result;
 }
 
 GenericEnvironment *GenericContext::getGenericEnvironment() const {

lib/AST/DeclContext.cpp

@@ -139,7 +139,7 @@ void DeclContext::forEachGenericContext(
         if (auto *gpList = genericCtx->getGenericParams())
           fn(gpList);
     }
-  } while ((dc = dc->getParent()));
+  } while ((dc = dc->getParentForLookup()));
 }
 
 unsigned DeclContext::getGenericContextDepth() const {

lib/AST/GenericSignatureBuilder.cpp

@@ -3906,11 +3906,18 @@ bool GenericSignatureBuilder::addGenericParameterRequirements(
 void GenericSignatureBuilder::addGenericParameter(GenericTypeParamType *GenericParam) {
   auto params = getGenericParams();
   (void)params;
-  assert(params.empty() ||
-         ((GenericParam->getDepth() == params.back()->getDepth() &&
-           GenericParam->getIndex() == params.back()->getIndex() + 1) ||
-          (GenericParam->getDepth() > params.back()->getDepth() &&
-           GenericParam->getIndex() == 0)));
+
+#ifndef NDEBUG
+  if (params.empty()) {
+    assert(GenericParam->getDepth() == 0);
+    assert(GenericParam->getIndex() == 0);
+  } else {
+    assert((GenericParam->getDepth() == params.back()->getDepth() &&
+            GenericParam->getIndex() == params.back()->getIndex() + 1) ||
+           (GenericParam->getDepth() > params.back()->getDepth() &&
+            GenericParam->getIndex() == 0));
+  }
+#endif
 
   // Create a potential archetype for this type parameter.
   auto PA = new (Impl->Allocator) PotentialArchetype(GenericParam);

lib/AST/TypeCheckRequests.cpp

@@ -738,6 +738,22 @@ void GenericSignatureRequest::cacheResult(GenericSignature value) const {
   GC->GenericSigAndBit.setPointerAndInt(value, true);
 }
 
+void GenericSignatureRequest::diagnoseCycle(DiagnosticEngine &diags) const {
+  auto *GC = std::get<0>(getStorage());
+  auto *D = GC->getAsDecl();
+
+  if (auto *VD = dyn_cast<ValueDecl>(D)) {
+    VD->diagnose(diag::recursive_generic_signature,
+                 VD->getDescriptiveKind(), VD->getBaseName());
+  } else {
+    auto *ED = cast<ExtensionDecl>(D);
+    auto *NTD = ED->getExtendedNominal();
+
+    ED->diagnose(diag::recursive_generic_signature_extension,
+                 NTD->getDescriptiveKind(), NTD->getName());
+  }
+}
+
 //----------------------------------------------------------------------------//
 // InferredGenericSignatureRequest computation.
 //----------------------------------------------------------------------------//

lib/Sema/TypeCheckGeneric.cpp

@@ -597,7 +597,7 @@ static Type formExtensionInterfaceType(
   if (!nominal->isGeneric() || isa<ProtocolDecl>(nominal)) {
     resultType = NominalType::get(nominal, parentType,
                                   nominal->getASTContext());
-  } else {
+  } else if (genericParams) {
     auto currentBoundType = type->getAs<BoundGenericType>();
 
     // Form the bound generic type with the type parameters provided.

lib/Sema/TypeCheckType.cpp

@@ -590,8 +590,6 @@ bool TypeChecker::checkContextualRequirements(GenericTypeDecl *decl,
     return true;
   }
 
-  auto &ctx = dc->getASTContext();
-
   SourceLoc noteLoc;
   {
     // We are interested in either a contextual where clause or
@@ -610,14 +608,6 @@ bool TypeChecker::checkContextualRequirements(GenericTypeDecl *decl,
 
   const auto subMap = parentTy->getContextSubstitutions(decl->getDeclContext());
   const auto genericSig = decl->getGenericSignature();
-  if (!genericSig) {
-    if (loc.isValid()) {
-      ctx.Diags.diagnose(loc, diag::recursive_decl_reference,
-                         decl->getDescriptiveKind(), decl->getName());
-      decl->diagnose(diag::kind_declared_here, DescriptiveDeclKind::Type);
-    }
-    return false;
-  }
 
   const auto result =
     TypeChecker::checkGenericArguments(
@@ -765,14 +755,6 @@ static Type applyGenericArguments(Type type, TypeResolution resolution,
     }  
   }
 
-  // FIXME: More principled handling of circularity.
-  if (!decl->getGenericSignature()) {
-    diags.diagnose(loc, diag::recursive_decl_reference,
-                   decl->getDescriptiveKind(), decl->getName());
-    decl->diagnose(diag::kind_declared_here, DescriptiveDeclKind::Type);
-    return ErrorType::get(ctx);
-  }
-
   // Resolve the types of the generic arguments.
   auto genericResolution =
       resolution.withOptions(adjustOptionsForGenericArgs(options));

test/Generics/generic_types.swift

@@ -228,9 +228,9 @@ var y: X5<X4, Int> // expected-error{{'X5' requires the types 'X4.AssocP' (aka '
 // Recursive generic signature validation.
 class Top {}
 class Bottom<T : Bottom<Top>> {}
-// expected-error@-1 {{generic class 'Bottom' references itself}}
-// expected-note@-2 {{type declared here}}
-// expected-error@-3 {{circular reference}}
+// expected-error@-1 {{'Bottom' requires that 'Top' inherit from 'Bottom<Top>'}}
+// expected-note@-2 {{requirement specified as 'T' : 'Bottom<Top>' [with T = Top]}}
+// expected-error@-3 {{generic class 'Bottom' has self-referential generic requirements}}
 // expected-note@-4 {{while resolving type 'Bottom<Top>'}}
 // expected-note@-5 {{through reference here}}
 

test/decl/protocol/req/recursion.swift

@@ -43,19 +43,16 @@ public protocol P {
   associatedtype T
 }
 
-public struct S<A: P> where A.T == S<A> { // expected-error {{circular reference}}
-// expected-note@-1 {{type declared here}}
-// expected-error@-2 {{generic struct 'S' references itself}}
-// expected-note@-3 {{while resolving type 'S<A>'}}
+public struct S<A: P> where A.T == S<A> {
+// expected-error@-1 {{generic struct 'S' has self-referential generic requirements}}
+// expected-note@-2 {{while resolving type 'S<A>'}}
   func f(a: A.T) {
     g(a: id(t: a)) // `a` has error type which is diagnosed as circular reference
-    // expected-error@-1 {{conflicting arguments to generic parameter 'T' ('A.T' (associated type of protocol 'P') vs. 'S<A>')}}
     _ = A.T.self
   }
 
   func g(a: S<A>) {
     f(a: id(t: a))
-    // expected-error@-1 {{conflicting arguments to generic parameter 'T' ('S<A>' vs. 'A.T' (associated type of protocol 'P'))}}
     _ = S<A>.self
   }
 
@@ -72,10 +69,9 @@ protocol PI {
   associatedtype T : I
 }
 
-struct SI<A: PI> : I where A : I, A.T == SI<A> { // expected-error {{circular reference}}
-// expected-note@-1 {{type declared here}}
-// expected-error@-2 {{generic struct 'SI' references itself}}
-// expected-note@-3 {{while resolving type 'SI<A>'}}
+struct SI<A: PI> : I where A : I, A.T == SI<A> {
+// expected-error@-1 {{generic struct 'SI' has self-referential generic requirements}}
+// expected-note@-2 {{while resolving type 'SI<A>'}}
   func ggg<T : I>(t: T.Type) -> T {
     return T()
   }