GSB: Move signature verification from CanGenericSignature::getCanonical() to computeGenericSignature()

slavapestov · slavapestov · commit 91ebe4485ca4 · 2021-03-11T21:07:33.000-05:00
Doing this when computing a canonical signature didn't really
make sense because canonical signatures are not canonicalized
any more strongly _with respect to the builder_; they just
canonicalize their requirement types.

Instead, let's do these checks after creating the signature in
computeGenericSignature().

The old behavior had another undesirable property; since the
canonicalization was done by registerGenericSignatureBuilder(),
we would always build a new GSB from scratch for every
signature we compute.

The new location also means we do these checks for protocol
requirement signatures as well. This flags an existing fixed
crasher where we still emit bogus same-type requirements in
the requirement signature, so I moved this test back into
an unfixed state.
diff --git a/include/swift/AST/GenericSignature.h b/include/swift/AST/GenericSignature.h
@@ -175,7 +175,7 @@ class CanGenericSignature : public GenericSignature {
   /// requirements, first canonicalizing the types.
   static CanGenericSignature
   getCanonical(TypeArrayView<GenericTypeParamType> params,
-               ArrayRef<Requirement> requirements, bool skipValidation = false);
+               ArrayRef<Requirement> requirements);
 
 public:
   CanGenericSignature(std::nullptr_t) : GenericSignature(nullptr) {}
@@ -342,6 +342,8 @@ class alignas(1 << TypeAlignInBits) GenericSignatureImpl final
   ///
   /// The type parameters must be known to not be concrete within the context.
   bool areSameTypeParameterInContext(Type type1, Type type2) const;
+  bool areSameTypeParameterInContext(Type type1, Type type2,
+                                     GenericSignatureBuilder &builder) const;
 
   /// Determine if \c sig can prove \c requirement, meaning that it can deduce
   /// T: Foo or T == U (etc.) with the information it knows. This includes
@@ -360,12 +362,10 @@ class alignas(1 << TypeAlignInBits) GenericSignatureImpl final
   /// Return the canonical version of the given type under this generic
   /// signature.
   CanType getCanonicalTypeInContext(Type type) const;
-  bool isCanonicalTypeInContext(Type type) const;
-
-  /// Return the canonical version of the given type under this generic
-  /// signature.
   CanType getCanonicalTypeInContext(Type type,
                                     GenericSignatureBuilder &builder) const;
+
+  bool isCanonicalTypeInContext(Type type) const;
   bool isCanonicalTypeInContext(Type type,
                                 GenericSignatureBuilder &builder) const;
 
diff --git a/lib/AST/GenericSignature.cpp b/lib/AST/GenericSignature.cpp
@@ -176,23 +176,9 @@ bool GenericSignatureImpl::isCanonical() const {
   return getCanonicalSignature().getPointer() == this;
 }
 
-#ifndef NDEBUG
-/// Determine the canonical ordering of requirements.
-static unsigned getRequirementKindOrder(RequirementKind kind) {
-  switch (kind) {
-  case RequirementKind::Conformance: return 2;
-  case RequirementKind::Superclass: return 0;
-  case RequirementKind::SameType: return 3;
-  case RequirementKind::Layout: return 1;
-  }
-  llvm_unreachable("unhandled kind");
-}
-#endif
-
 CanGenericSignature
 CanGenericSignature::getCanonical(TypeArrayView<GenericTypeParamType> params,
-                                  ArrayRef<Requirement> requirements,
-                                  bool skipValidation) {
+                                  ArrayRef<Requirement> requirements) {
   // Canonicalize the parameters and requirements.
   SmallVector<GenericTypeParamType*, 8> canonicalParams;
   canonicalParams.reserve(params.size());
@@ -205,116 +191,9 @@ CanGenericSignature::getCanonical(TypeArrayView<GenericTypeParamType> params,
   for (auto &reqt : requirements)
     canonicalRequirements.push_back(reqt.getCanonical());
 
-  (void)skipValidation;
   auto canSig = get(canonicalParams, canonicalRequirements,
                     /*isKnownCanonical=*/true);
 
-#ifndef NDEBUG
-  if (skipValidation)
-    return CanGenericSignature(canSig);
-
-  PrettyStackTraceGenericSignature debugStack("canonicalizing", canSig);
-
-  // Check that the signature is canonical.
-  for (unsigned idx : indices(canonicalRequirements)) {
-    debugStack.setRequirement(idx);
-
-    const auto &reqt = canonicalRequirements[idx];
-
-    // Left-hand side must be canonical in its context.
-    // Check canonicalization of requirement itself.
-    switch (reqt.getKind()) {
-    case RequirementKind::Superclass:
-      assert(canSig->isCanonicalTypeInContext(reqt.getFirstType()) &&
-             "Left-hand side is not canonical");
-      assert(canSig->isCanonicalTypeInContext(reqt.getSecondType()) &&
-             "Superclass type isn't canonical in its own context");
-      break;
-
-    case RequirementKind::Layout:
-      assert(canSig->isCanonicalTypeInContext(reqt.getFirstType()) &&
-             "Left-hand side is not canonical");
-      break;
-
-    case RequirementKind::SameType: {
-      auto isCanonicalAnchor = [&](Type type) {
-        if (auto *dmt = type->getAs<DependentMemberType>())
-          return canSig->isCanonicalTypeInContext(dmt->getBase());
-        return type->is<GenericTypeParamType>();
-      };
-
-      auto firstType = reqt.getFirstType();
-      auto secondType = reqt.getSecondType();
-      assert(isCanonicalAnchor(firstType));
-
-      if (reqt.getSecondType()->isTypeParameter()) {
-        assert(isCanonicalAnchor(secondType));
-        assert(compareDependentTypes(firstType, secondType) < 0 &&
-               "Out-of-order type parameters in same-type constraint");
-      } else {
-        assert(canSig->isCanonicalTypeInContext(secondType) &&
-               "Concrete same-type isn't canonical in its own context");
-      }
-      break;
-    }
-
-    case RequirementKind::Conformance:
-      assert(reqt.getFirstType()->isTypeParameter() &&
-             "Left-hand side must be a type parameter");
-      assert(isa<ProtocolType>(reqt.getSecondType().getPointer()) &&
-             "Right-hand side of conformance isn't a protocol type");
-      break;
-    }
-
-    // From here on, we're only interested in requirements beyond the first.
-    if (idx == 0) continue;
-
-    // Make sure that the left-hand sides are in nondecreasing order.
-    const auto &prevReqt = canonicalRequirements[idx-1];
-    int compareLHS =
-      compareDependentTypes(prevReqt.getFirstType(), reqt.getFirstType());
-    assert(compareLHS <= 0 && "Out-of-order left-hand sides");
-
-    // If we have two same-type requirements where the left-hand sides differ
-    // but fall into the same equivalence class, we can check the form.
-    if (compareLHS < 0 && reqt.getKind() == RequirementKind::SameType &&
-        prevReqt.getKind() == RequirementKind::SameType &&
-        canSig->areSameTypeParameterInContext(prevReqt.getFirstType(),
-                                              reqt.getFirstType())) {
-      // If it's a it's a type parameter, make sure the equivalence class is
-      // wired together sanely.
-      if (prevReqt.getSecondType()->isTypeParameter()) {
-        assert(prevReqt.getSecondType()->isEqual(reqt.getFirstType()) &&
-               "same-type constraints within an equiv. class are out-of-order");
-      } else {
-        // Otherwise, the concrete types must match up.
-        assert(prevReqt.getSecondType()->isEqual(reqt.getSecondType()) &&
-               "inconsistent concrete same-type constraints in equiv. class");
-      }
-    }
-
-    // From here on, we only care about cases where the previous and current
-    // requirements have the same left-hand side.
-    if (compareLHS != 0) continue;
-
-    // Check ordering of requirement kinds.
-    assert((getRequirementKindOrder(prevReqt.getKind()) <=
-            getRequirementKindOrder(reqt.getKind())) &&
-           "Requirements for a given kind are out-of-order");
-
-    // From here on, we only care about the same requirement kind.
-    if (prevReqt.getKind() != reqt.getKind()) continue;
-
-    assert(reqt.getKind() == RequirementKind::Conformance &&
-           "Only conformance requirements can have multiples");
-
-    auto prevProto = prevReqt.getProtocolDecl();
-    auto proto = reqt.getProtocolDecl();
-    assert(TypeDecl::compare(prevProto, proto) < 0 &&
-           "Out-of-order conformance requirements");
-  }
-#endif
-
   return CanGenericSignature(canSig);
 }
 
@@ -519,7 +398,19 @@ bool GenericSignatureImpl::areSameTypeParameterInContext(Type type1,
   if (type1.getPointer() == type2.getPointer())
     return true;
 
-  auto &builder = *getGenericSignatureBuilder();
+  return areSameTypeParameterInContext(type1, type2,
+                                       *getGenericSignatureBuilder());
+}
+
+bool GenericSignatureImpl::areSameTypeParameterInContext(Type type1,
+                                                         Type type2,
+                                                         GenericSignatureBuilder &builder) const {
+  assert(type1->isTypeParameter());
+  assert(type2->isTypeParameter());
+
+  if (type1.getPointer() == type2.getPointer())
+    return true;
+
   auto equivClass1 =
     builder.resolveEquivalenceClass(
                              type1,
diff --git a/lib/AST/GenericSignatureBuilder.cpp b/lib/AST/GenericSignatureBuilder.cpp
@@ -7948,6 +7948,129 @@ void GenericSignatureBuilder::addGenericSignature(GenericSignature sig) {
     addRequirement(reqt, FloatingRequirementSource::forAbstract(), nullptr);
 }
 
+#ifndef NDEBUG
+
+/// Determine the canonical ordering of requirements.
+static unsigned getRequirementKindOrder(RequirementKind kind) {
+  switch (kind) {
+  case RequirementKind::Conformance: return 2;
+  case RequirementKind::Superclass: return 0;
+  case RequirementKind::SameType: return 3;
+  case RequirementKind::Layout: return 1;
+  }
+  llvm_unreachable("unhandled kind");
+}
+
+static void checkGenericSignature(CanGenericSignature canSig,
+                                  GenericSignatureBuilder &builder) {
+  PrettyStackTraceGenericSignature debugStack("checking", canSig);
+
+  auto canonicalRequirements = canSig->getRequirements();
+
+  // Check that the signature is canonical.
+  for (unsigned idx : indices(canonicalRequirements)) {
+    debugStack.setRequirement(idx);
+
+    const auto &reqt = canonicalRequirements[idx];
+
+    // Left-hand side must be canonical in its context.
+    // Check canonicalization of requirement itself.
+    switch (reqt.getKind()) {
+    case RequirementKind::Superclass:
+      assert(canSig->isCanonicalTypeInContext(reqt.getFirstType(), builder) &&
+             "Left-hand side is not canonical");
+      assert(canSig->isCanonicalTypeInContext(reqt.getSecondType(), builder) &&
+             "Superclass type isn't canonical in its own context");
+      break;
+
+    case RequirementKind::Layout:
+      assert(canSig->isCanonicalTypeInContext(reqt.getFirstType(), builder) &&
+             "Left-hand side is not canonical");
+      break;
+
+    case RequirementKind::SameType: {
+      auto isCanonicalAnchor = [&](Type type) {
+        if (auto *dmt = type->getAs<DependentMemberType>())
+          return canSig->isCanonicalTypeInContext(dmt->getBase(), builder);
+        return type->is<GenericTypeParamType>();
+      };
+
+      auto firstType = reqt.getFirstType();
+      auto secondType = reqt.getSecondType();
+      assert(isCanonicalAnchor(firstType));
+
+      if (reqt.getSecondType()->isTypeParameter()) {
+        assert(isCanonicalAnchor(secondType));
+        assert(compareDependentTypes(firstType, secondType) < 0 &&
+               "Out-of-order type parameters in same-type constraint");
+      } else {
+        assert(canSig->isCanonicalTypeInContext(secondType) &&
+               "Concrete same-type isn't canonical in its own context");
+      }
+      break;
+    }
+
+    case RequirementKind::Conformance:
+      assert(canSig->isCanonicalTypeInContext(reqt.getFirstType(), builder) &&
+             "Left-hand side is not canonical");
+      assert(reqt.getFirstType()->isTypeParameter() &&
+             "Left-hand side must be a type parameter");
+      assert(isa<ProtocolType>(reqt.getSecondType().getPointer()) &&
+             "Right-hand side of conformance isn't a protocol type");
+      break;
+    }
+
+    // From here on, we're only interested in requirements beyond the first.
+    if (idx == 0) continue;
+
+    // Make sure that the left-hand sides are in nondecreasing order.
+    const auto &prevReqt = canonicalRequirements[idx-1];
+    int compareLHS =
+      compareDependentTypes(prevReqt.getFirstType(), reqt.getFirstType());
+    assert(compareLHS <= 0 && "Out-of-order left-hand sides");
+
+    // If we have two same-type requirements where the left-hand sides differ
+    // but fall into the same equivalence class, we can check the form.
+    if (compareLHS < 0 && reqt.getKind() == RequirementKind::SameType &&
+        prevReqt.getKind() == RequirementKind::SameType &&
+        canSig->areSameTypeParameterInContext(prevReqt.getFirstType(),
+                                              reqt.getFirstType(),
+                                              builder)) {
+      // If it's a it's a type parameter, make sure the equivalence class is
+      // wired together sanely.
+      if (prevReqt.getSecondType()->isTypeParameter()) {
+        assert(prevReqt.getSecondType()->isEqual(reqt.getFirstType()) &&
+               "same-type constraints within an equiv. class are out-of-order");
+      } else {
+        // Otherwise, the concrete types must match up.
+        assert(prevReqt.getSecondType()->isEqual(reqt.getSecondType()) &&
+               "inconsistent concrete same-type constraints in equiv. class");
+      }
+    }
+
+    // From here on, we only care about cases where the previous and current
+    // requirements have the same left-hand side.
+    if (compareLHS != 0) continue;
+
+    // Check ordering of requirement kinds.
+    assert((getRequirementKindOrder(prevReqt.getKind()) <=
+            getRequirementKindOrder(reqt.getKind())) &&
+           "Requirements for a given kind are out-of-order");
+
+    // From here on, we only care about the same requirement kind.
+    if (prevReqt.getKind() != reqt.getKind()) continue;
+
+    assert(reqt.getKind() == RequirementKind::Conformance &&
+           "Only conformance requirements can have multiples");
+
+    auto prevProto = prevReqt.getProtocolDecl();
+    auto proto = reqt.getProtocolDecl();
+    assert(TypeDecl::compare(prevProto, proto) < 0 &&
+           "Out-of-order conformance requirements");
+  }
+}
+#endif
+
 GenericSignature GenericSignatureBuilder::computeGenericSignature(
                                           bool allowConcreteGenericParams,
                                           bool allowBuilderToMove) && {
@@ -7961,6 +8084,12 @@ GenericSignature GenericSignatureBuilder::computeGenericSignature(
   // Form the generic signature.
   auto sig = GenericSignature::get(getGenericParams(), requirements);
 
+#ifndef NDEBUG
+  if (!Impl->HadAnyError) {
+    checkGenericSignature(sig.getCanonicalSignature(), *this);
+  }
+#endif
+
   // When we can, move this generic signature builder to make it the canonical
   // builder, rather than constructing a new generic signature builder that
   // will produce the same thing.
diff --git a/lib/Sema/TypeCheckDeclPrimary.cpp b/lib/Sema/TypeCheckDeclPrimary.cpp
@@ -2452,13 +2452,10 @@ class DeclChecker : public DeclVisitor<DeclChecker> {
       requirementsSig->print(llvm::errs());
       llvm::errs() << "\n";
 
-      // Note: One cannot canonicalize a requirement signature, because
-      // requirement signatures are necessarily missing requirements.
       llvm::errs() << "Canonical requirement signature: ";
       auto canRequirementSig =
         CanGenericSignature::getCanonical(requirementsSig->getGenericParams(),
-                                          requirementsSig->getRequirements(),
-                                          /*skipValidation=*/true);
+                                          requirementsSig->getRequirements());
       canRequirementSig->print(llvm::errs());
       llvm::errs() << "\n";
     }
diff --git a/validation-test/compiler_crashers_2/sr8968.swift b/validation-test/compiler_crashers_2/sr8968.swift
diff --git a/validation-test/compiler_crashers_2_fixed/sr8968.swift b/validation-test/compiler_crashers_2_fixed/sr8968.swift
