Merge pull request #15094 from huonw/conditional-requirement-order

[AST] Ensure requirements are correctly identified as conditional.

Author: huonw

include/swift/AST/GenericSignatureBuilder.h

@@ -816,6 +816,25 @@ class GenericSignatureBuilder {
   /// don't have associated types.
   Type getCanonicalTypeParameter(Type type);
 
+  /// For each requirement in \c sig, create a new signature without it and see
+  /// if the requirement is satisfied in it.
+  ///
+  /// Specifically, for each Requirement \c req in \c sig, create a new
+  /// signature incorporating all of those in \c baseSig (if not null) and the
+  /// other requirements from \c sig, and if \c req is satisfied in this new
+  /// signature add it to \c redundant, otherwise add it to \c nonRedundant.
+  ///
+  /// If \c includeRedundantRequirements is true, all requirements from \c sig
+  /// (other than \c req) are added; if it is false, requirements already found
+  /// to be redundant are also not added (i.e. the contents of \c redundant at
+  /// that point in time).
+  static void
+  dropAndCompareEachRequirement(ASTContext &context, GenericSignature *baseSig,
+                                GenericSignature *sig,
+                                bool includeRedundantRequirements,
+                                SmallVectorImpl<Requirement> &redundant,
+                                SmallVectorImpl<Requirement> &nonRedundant);
+
   /// Verify the correctness of the given generic signature.
   ///
   /// This routine will test that the given generic signature is both minimal

lib/AST/GenericSignature.cpp

@@ -23,10 +23,17 @@
 #include "swift/AST/PrettyStackTrace.h"
 #include "swift/AST/Types.h"
 #include "swift/Basic/STLExtras.h"
+#include "llvm/ADT/Statistic.h"
 #include <functional>
 
 using namespace swift;
 
+#define DEBUG_TYPE "Generic signature"
+STATISTIC(NumRedundantRequirements,
+          "# of redundant requirements found in signature differencing");
+STATISTIC(NumNonRedundantRequirements,
+          "# of non-redundant requirements found in signature differencing");
+
 void ConformanceAccessPath::print(raw_ostream &out) const {
   interleave(begin(), end(),
              [&](const Entry &entry) {
@@ -770,6 +777,7 @@ bool GenericSignature::isRequirementSatisfied(Requirement requirement) {
 
 SmallVector<Requirement, 4> GenericSignature::requirementsNotSatisfiedBy(
                                                  GenericSignature *otherSig) {
+  auto &ctxt = getASTContext();
   SmallVector<Requirement, 4> result;
 
   // If the signatures are the same, all requirements are satisfied.
@@ -783,10 +791,52 @@ SmallVector<Requirement, 4> GenericSignature::requirementsNotSatisfiedBy(
   }
 
   // Find the requirements that aren't satisfied.
-  for (const auto &req : getRequirements()) {
-    if (!otherSig->isRequirementSatisfied(req))
-      result.push_back(req);
-  }
+  //
+  // This is unfortunately quadratic in the size of getRequirements(), but some
+  // arrangements of signatures result in different canonicalizations that mean
+  // a requirement in `this` is satisfied by `otherSig`, but not in
+  // isolation. Specifically, consider:
+  //
+  // otherSig == <T, U where U: P1>
+  // this == <T, U where T: P1, T == U>`
+  //
+  // `T: P1` is implied by `U: P1` and `T == U` together, but just checking T:
+  // P1 in `otherSig` won't find this, because it misses the T == U
+  // connection. We don't currently know of a great way to handle this in
+  // general, and instead have to do a brute-force search.
+
+  SmallVector<Requirement, 4> redundant;
+  GenericSignatureBuilder::dropAndCompareEachRequirement(
+      ctxt, otherSig, this,
+      /*includeRedundantRequirements=*/false, redundant, result);
+
+  NumNonRedundantRequirements += result.size();
+  NumRedundantRequirements += redundant.size();
+
+#ifndef NDEBUG
+  // `otherSig + result` should be `this`, so let's check it.
+  GenericSignatureBuilder builder(ctxt);
+
+  // otherSig may have fewer generic parameters, so we can't use
+  // addGenericSignature directly.
+
+  auto source =
+      GenericSignatureBuilder::FloatingRequirementSource::forAbstract();
+  for (auto gp : getGenericParams())
+    builder.addGenericParameter(gp);
+  for (auto req : otherSig->getRequirements())
+    builder.addRequirement(req, source, nullptr);
+  for (auto req : result)
+    builder.addRequirement(req, source, nullptr);
+
+  auto newSig = std::move(builder).computeGenericSignature(
+      SourceLoc(),
+      /*allowConcreteGenericParams=*/true,
+      /*allowBuilderToMove=*/true);
+
+  assert(newSig->getCanonicalSignature() == getCanonicalSignature() &&
+         "signature differencing removed too many requirements");
+#endif
 
   return result;
 }

lib/AST/GenericSignatureBuilder.cpp

@@ -7438,32 +7438,58 @@ GenericSignature *GenericSignatureBuilder::computeRequirementSignature(
 
 #pragma mark Generic signature verification
 
-void GenericSignatureBuilder::verifyGenericSignature(ASTContext &context,
-                                                     GenericSignature *sig) {
-  llvm::errs() << "Validating generic signature: ";
-  sig->print(llvm::errs());
-  llvm::errs() << "\n";
+void GenericSignatureBuilder::dropAndCompareEachRequirement(
+    ASTContext &context, GenericSignature *baseSig, GenericSignature *sig,
+    bool includeRedundantRequirements, SmallVectorImpl<Requirement> &redundant,
+    SmallVectorImpl<Requirement> &nonRedundant) {
+#ifndef NDEBUG
+  // `baseSig`'s params should be a subset of `sig`, meaning <A where ...>, <A,
+  // B where ...> (respectively) is okay, but the reverse is not.
+  if (baseSig) {
+    auto canSigParams = sig->getCanonicalSignature()->getGenericParams();
+    for (auto baseParam :
+         baseSig->getCanonicalSignature()->getGenericParams()) {
+      assert(llvm::is_contained(canSigParams, baseParam) &&
+             "baseSig params aren't a subset of sig");
+    }
+  }
+#endif
 
-  // Try removing each requirement in turn.
   auto genericParams = sig->getGenericParams();
   auto requirements = sig->getRequirements();
   for (unsigned victimIndex : indices(requirements)) {
-    PrettyStackTraceGenericSignature debugStack("verifying", sig, victimIndex);
+    PrettyStackTraceGenericSignature debugStack("dropping & comparing", sig,
+                                                victimIndex);
 
     // Form a new generic signature builder.
     GenericSignatureBuilder builder(context);
 
-    // Add the generic parameters.
+    // Add the generic parameters, and specifically those from the main
+    // signature, since it may have more
     for (auto gp : genericParams)
       builder.addGenericParameter(gp);
 
-    // Add the requirements *except* the victim.
+    // Add the appropriate requirements
     auto source = FloatingRequirementSource::forAbstract();
-    for (unsigned i : indices(requirements)) {
-      if (i != victimIndex)
-        builder.addRequirement(requirements[i], source, nullptr);
+    // First, the universal requirements from the base
+    if (baseSig) {
+      for (auto req : baseSig->getRequirements())
+        builder.addRequirement(req, source, nullptr);
     }
 
+    // Next, the requirements from the interesting signature.  Sometimes we have
+    // to add all of the leading ones, and others times only those that we've
+    // already decided aren't redundant:
+    auto leadingReqs = includeRedundantRequirements
+                           ? requirements.slice(0, victimIndex)
+                           : nonRedundant;
+    // We definitely have to add all of the remaining trailing ones:
+    auto trailingReqs = requirements.slice(victimIndex + 1);
+
+    for (auto reqs : {leadingReqs, trailingReqs})
+      for (auto req : reqs)
+        builder.addRequirement(req, source, nullptr);
+
     // Finalize the generic signature. If there were any errors, we formed
     // an invalid signature, so just continue.
     if (builder.Impl->HadAnyError) continue;
@@ -7475,23 +7501,40 @@ void GenericSignatureBuilder::verifyGenericSignature(ASTContext &context,
                                       /*allowConcreteGenericParams=*/true,
                                       /*allowBuilderToMove=*/true);
 
-    // If the removed requirement is satisfied by the new generic signature,
-    // it is redundant. Complain.
-    if (newSig->isRequirementSatisfied(requirements[victimIndex])) {
-      SmallString<32> reqString;
-      {
-        llvm::raw_svector_ostream out(reqString);
-        requirements[victimIndex].print(out, PrintOptions());
-      }
-      context.Diags.diagnose(SourceLoc(), diag::generic_signature_not_minimal,
-                             reqString, sig->getAsString());
-    }
+    auto req = requirements[victimIndex];
+    if (newSig->isRequirementSatisfied(req))
+      redundant.push_back(req);
+    else
+      nonRedundant.push_back(req);
 
     // Canonicalize the signature to check that it is canonical.
     (void)newSig->getCanonicalSignature();
   }
 }
 
+void GenericSignatureBuilder::verifyGenericSignature(ASTContext &context,
+                                                     GenericSignature *sig) {
+  llvm::errs() << "Validating generic signature: ";
+  sig->print(llvm::errs());
+  llvm::errs() << "\n";
+
+  SmallVector<Requirement, 4> redundant;
+  SmallVector<Requirement, 4> _nonRedundant;
+  dropAndCompareEachRequirement(context, /*baseSig=*/nullptr, sig,
+                                /*includeRedundantRequirements=*/true,
+                                redundant, _nonRedundant);
+  for (auto req : redundant) {
+    // Oops, the signature wasn't minimal: complain.
+    SmallString<32> reqString;
+    {
+      llvm::raw_svector_ostream out(reqString);
+      req.print(out, PrintOptions());
+    }
+    context.Diags.diagnose(SourceLoc(), diag::generic_signature_not_minimal,
+                           reqString, sig->getAsString());
+  }
+}
+
 void GenericSignatureBuilder::verifyGenericSignaturesInModule(
                                                         ModuleDecl *module) {
   LoadedFile *loadedFile = nullptr;

test/Generics/conditional_conformances.swift

@@ -307,11 +307,7 @@ extension TwoDisjointConformances: P2 where T == String {}
 // expected-note@-1{{'TwoDisjointConformances<T>' declares conformance to protocol 'P2' here}}
 
 
-// FIXME: these cases should be equivalent (and both with the same output as the
-// first), but the second one choses T as the representative of the
-// equivalence class containing both T and U in the extension's generic
-// signature, meaning the stored conditional requirement is T: P1, which isn't
-// true in the original type's generic signature.
+// rdar://problem/34944318
 struct RedundancyOrderDependenceGood<T: P1, U> {}
 // CHECK-LABEL: ExtensionDecl line={{.*}} base=RedundancyOrderDependenceGood<T, T>
 // CHECK-NEXT: (normal_conformance type=RedundancyOrderDependenceGood<T, U> protocol=P2
@@ -320,7 +316,6 @@ extension RedundancyOrderDependenceGood: P2 where U: P1, T == U {}
 struct RedundancyOrderDependenceBad<T, U: P1> {}
 // CHECK-LABEL: ExtensionDecl line={{.*}} base=RedundancyOrderDependenceBad<T, T>
 // CHECK-NEXT: (normal_conformance type=RedundancyOrderDependenceBad<T, U> protocol=P2
-// CHECK-NEXT:   conforms_to: T P1
 // CHECK-NEXT:   same_type: T U)
 extension RedundancyOrderDependenceBad: P2 where T: P1, T == U {}