Merge pull request swiftlang#39174 from slavapestov/circular-generic-signature

Sema: Improved recovery from circular generic signature construction

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()
   }