Merge pull request swiftlang#41648 from slavapestov/rqm-concrete-contraction-fixes-and-more

RequirementMachine: Fix some problems in concrete contraction and a couple of other issues

Author: slavapestov

lib/AST/RequirementMachine/ConcreteContraction.cpp

@@ -157,8 +157,10 @@ namespace {
 class ConcreteContraction {
   bool Debug;
 
-  llvm::SmallDenseMap<GenericParamKey, Type> ConcreteTypes;
-  llvm::SmallDenseMap<GenericParamKey, Type> Superclasses;
+  llvm::SmallDenseMap<GenericParamKey,
+                      llvm::SmallDenseSet<Type, 1>> ConcreteTypes;
+  llvm::SmallDenseMap<GenericParamKey,
+                      llvm::SmallDenseSet<Type, 1>> Superclasses;
   llvm::SmallDenseMap<GenericParamKey,
                       llvm::SmallVector<ProtocolDecl *, 1>> Conformances;
 
@@ -229,32 +231,44 @@ Optional<Type> ConcreteContraction::substTypeParameter(
   if (!substBaseType)
     return None;
 
-  auto *decl = (*substBaseType)->getAnyNominal();
-  if (decl == nullptr) {
-    llvm::dbgs() << "@@@ Not a nominal type: " << *substBaseType << "\n";
-    return None;
-  }
-
-  auto *module = decl->getParentModule();
-
   // A resolved DependentMemberType stores an associated type declaration.
   //
   // Handle this by looking up the corresponding type witness in the base
   // type's conformance to the associated type's protocol.
   if (auto *assocType = memberType->getAssocType()) {
-    auto conformance = module->lookupConformance(
-        *substBaseType, assocType->getProtocol());
+    auto *proto = assocType->getProtocol();
+    auto *module = proto->getParentModule();
+
+    auto conformance = ((*substBaseType)->isTypeParameter()
+                        ? ProtocolConformanceRef(proto)
+                        : module->lookupConformance(*substBaseType, proto));
 
     // The base type doesn't conform, in which case the requirement remains
     // unsubstituted.
-    if (!conformance)
+    if (!conformance) {
+      if (Debug) {
+        llvm::dbgs() << "@@@ " << substBaseType << " does not conform to "
+                     << proto->getName() << "\n";
+      }
       return None;
+    }
 
     return assocType->getDeclaredInterfaceType()
                     ->castTo<DependentMemberType>()
                     ->substBaseType(module, *substBaseType);
   }
 
+  auto *decl = (*substBaseType)->getAnyNominal();
+  if (decl == nullptr) {
+    if (Debug) {
+      llvm::dbgs() << "@@@ Not a nominal type: " << *substBaseType << "\n";
+    }
+
+    return None;
+  }
+
+  auto *module = decl->getParentModule();
+
   // An unresolved DependentMemberType stores an identifier. Handle this
   // by performing a name lookup into the base type.
   auto *typeDecl = lookupConcreteNestedType(module, decl,
@@ -293,20 +307,22 @@ Type ConcreteContraction::substTypeParameter(
   Type concreteType;
   {
     auto found = ConcreteTypes.find(key);
-    if (found != ConcreteTypes.end())
-      concreteType = found->second;
+    if (found != ConcreteTypes.end() && found->second.size() == 1)
+      concreteType = *found->second.begin();
   }
 
   Type superclass;
   {
     auto found = Superclasses.find(key);
-    if (found != Superclasses.end())
-      superclass = found->second;
+    if (found != Superclasses.end() && found->second.size() == 1)
+      superclass = *found->second.begin();
   }
 
   if (!concreteType && !superclass)
     return type;
 
+  // If we have both, prefer the concrete type requirement since it is more
+  // specific.
   if (!concreteType) {
     assert(superclass);
 
@@ -431,35 +447,15 @@ bool ConcreteContraction::performConcreteContraction(
       if (constraintType->isTypeParameter())
         break;
 
-      auto entry = std::make_pair(GenericParamKey(genericParam),
-                                  constraintType);
-      bool inserted = ConcreteTypes.insert(entry).second;
-      if (!inserted) {
-        if (Debug) {
-          llvm::dbgs() << "@ Concrete contraction cannot proceed: "
-                       << "duplicate concrete type requirements\n";
-        }
-        return false;
-      }
-
+      ConcreteTypes[GenericParamKey(genericParam)].insert(constraintType);
       break;
     }
     case RequirementKind::Superclass: {
       auto constraintType = req.req.getSecondType();
       assert(!constraintType->isTypeParameter() &&
              "You forgot to call desugarRequirement()");
 
-      auto entry = std::make_pair(GenericParamKey(genericParam),
-                                  constraintType);
-      bool inserted = Superclasses.insert(entry).second;
-      if (!inserted) {
-        if (Debug) {
-          llvm::dbgs() << "@ Concrete contraction cannot proceed: "
-                       << "duplicate superclass requirements\n";
-        }
-        return false;
-      }
-
+      Superclasses[GenericParamKey(genericParam)].insert(constraintType);
       break;
     }
     case RequirementKind::Conformance: {
@@ -481,10 +477,10 @@ bool ConcreteContraction::performConcreteContraction(
   for (const auto &pair : Conformances) {
     auto subjectType = pair.first;
     auto found = Superclasses.find(subjectType);
-    if (found == Superclasses.end())
+    if (found == Superclasses.end() || found->second.size() != 1)
       continue;
 
-    auto superclassTy = found->second;
+    auto superclassTy = *found->second.begin();
 
     for (const auto *proto : pair.second) {
       if (auto otherSuperclassTy = proto->getSuperclass()) {
@@ -508,35 +504,27 @@ bool ConcreteContraction::performConcreteContraction(
   if (ConcreteTypes.empty() && Superclasses.empty())
     return false;
 
-  // If a generic parameter is subject to both a concrete type and superclass
-  // requirement, bail out because we're not smart enough to figure out what's
-  // going on.
-  for (auto pair : ConcreteTypes) {
-    auto subjectType = pair.first;
-
-    if (Superclasses.find(subjectType) != Superclasses.end()) {
-      if (Debug) {
-        llvm::dbgs() << "@ Concrete contraction cannot proceed; "
-                     << "τ_" << subjectType.Depth << "_" << subjectType.Index
-                     << " has both a concrete type and superclass requirement";
-      }
-      return false;
-    }
-  }
-
   if (Debug) {
     llvm::dbgs() << "@ Concrete types: @\n";
     for (auto pair : ConcreteTypes) {
       llvm::dbgs() << "- τ_" << pair.first.Depth
-                   << "_" << pair.first.Index << " == "
-                   << pair.second << "\n";
+                   << "_" << pair.first.Index;
+      if (pair.second.size() == 1) {
+        llvm::dbgs() << " == " << *pair.second.begin() << "\n";
+      } else {
+        llvm::dbgs() << " has duplicate concrete type requirements\n";
+      }
     }
 
     llvm::dbgs() << "@ Superclasses: @\n";
     for (auto pair : Superclasses) {
       llvm::dbgs() << "- τ_" << pair.first.Depth
-                   << "_" << pair.first.Index << " : "
-                   << pair.second << "\n";
+                   << "_" << pair.first.Index;
+      if (pair.second.size() == 1) {
+        llvm::dbgs() << " : " << *pair.second.begin() << "\n";
+      } else {
+        llvm::dbgs() << " has duplicate superclass requirements\n";
+      }
     }
   }
 

lib/AST/RequirementMachine/HomotopyReduction.cpp

@@ -864,18 +864,15 @@ RewriteSystem::getMinimizedGenericSignatureRules() const {
 
 /// Verify that each loop begins and ends at its basepoint.
 void RewriteSystem::verifyRewriteLoops() const {
-#ifndef NDEBUG
   for (const auto &loop : Loops) {
     loop.verify(*this);
   }
-#endif
 }
 
 /// Assert if homotopy reduction failed to eliminate a redundant conformance,
 /// since this suggests a misunderstanding on my part.
 void RewriteSystem::verifyRedundantConformances(
     const llvm::DenseSet<unsigned> &redundantConformances) const {
-#ifndef NDEBUG
   for (unsigned ruleID : redundantConformances) {
     const auto &rule = getRule(ruleID);
     assert(!rule.isPermanent() &&
@@ -893,14 +890,12 @@ void RewriteSystem::verifyRedundantConformances(
       abort();
     }
   }
-#endif
 }
 
 // Assert if homotopy reduction failed to eliminate a rewrite rule it was
 // supposed to delete.
 void RewriteSystem::verifyMinimizedRules(
     const llvm::DenseSet<unsigned> &redundantConformances) const {
-#ifndef NDEBUG
   unsigned redundantRuleCount = 0;
 
   for (unsigned ruleID : indices(Rules)) {
@@ -982,5 +977,4 @@ void RewriteSystem::verifyMinimizedRules(
       abort();
     }
   }
-#endif
 }

lib/AST/RequirementMachine/MinimalConformances.cpp

@@ -418,16 +418,25 @@ void MinimalConformances::collectConformanceRules() {
 
   // Sort the list of conformance rules in reverse order; we're going to try
   // to minimize away less canonical rules first.
-  std::sort(ConformanceRules.begin(), ConformanceRules.end(),
-            [&](unsigned lhs, unsigned rhs) -> bool {
-              const auto &lhsRule = System.getRule(lhs);
-              const auto &rhsRule = System.getRule(rhs);
-
-              if (lhsRule.isExplicit() != rhsRule.isExplicit())
-                return !lhsRule.isExplicit();
-
-              return *lhsRule.getLHS().compare(rhsRule.getLHS(), Context) > 0;
-            });
+  std::stable_sort(ConformanceRules.begin(), ConformanceRules.end(),
+                   [&](unsigned lhs, unsigned rhs) -> bool {
+                     const auto &lhsRule = System.getRule(lhs);
+                     const auto &rhsRule = System.getRule(rhs);
+
+                     if (lhsRule.isExplicit() != rhsRule.isExplicit())
+                       return !lhsRule.isExplicit();
+
+                     auto result = lhsRule.getLHS().compare(rhsRule.getLHS(), Context);
+
+                     // Concrete conformance rules are unordered if they name the
+                     // same protocol but have different types. This can come up
+                     // if we have a class inheritance relationship 'Derived : Base',
+                     // and Base conforms to a protocol:
+                     //
+                     //     T.[Base : P] => T
+                     //     T.[Derived : P] => T
+                     return (result ? *result > 0 : 0);
+                   });
 
   Context.ConformanceRulesHistogram.add(ConformanceRules.size());
 }
@@ -742,7 +751,6 @@ void MinimalConformances::dumpMinimalConformanceEquation(
 }
 
 void MinimalConformances::verifyMinimalConformanceEquations() const {
-#ifndef NDEBUG
   for (const auto &pair : ConformancePaths) {
     const auto &rule = System.getRule(pair.first);
     auto *proto = rule.getLHS().back().getProtocol();
@@ -804,7 +812,6 @@ void MinimalConformances::verifyMinimalConformanceEquations() const {
       }
     }
   }
-#endif
 }
 
 bool MinimalConformances::isDerivedViaCircularConformanceRule(
@@ -884,7 +891,6 @@ void MinimalConformances::computeMinimalConformances() {
 
 /// Check invariants.
 void MinimalConformances::verifyMinimalConformances() const {
-#ifndef NDEBUG
   for (const auto &pair : ConformancePaths) {
     unsigned ruleID = pair.first;
     const auto &rule = System.getRule(ruleID);
@@ -913,7 +919,6 @@ void MinimalConformances::verifyMinimalConformances() const {
       abort();
     }
   }
-#endif
 }
 
 void MinimalConformances::dumpMinimalConformances(

lib/AST/RequirementMachine/RequirementMachineRequests.cpp

@@ -524,6 +524,9 @@ AbstractGenericSignatureRequestRQM::evaluate(
   auto result = GenericSignature::get(genericParams, minimalRequirements);
   bool hadError = machine->hadError();
 
+  if (!hadError)
+    result.verify();
+
   return GenericSignatureWithError(result, hadError);
 }
 
@@ -669,5 +672,9 @@ InferredGenericSignatureRequestRQM::evaluate(
 
   // FIXME: Handle allowConcreteGenericParams
 
+  // Check invariants.
+  if (!hadError)
+    result.verify();
+
   return GenericSignatureWithError(result, hadError);
 }

lib/AST/RequirementMachine/RewriteLoop.cpp

@@ -203,7 +203,6 @@ void RewritePath::dumpLong(llvm::raw_ostream &out,
 }
 
 void RewriteLoop::verify(const RewriteSystem &system) const {
-#ifndef NDEBUG
   RewritePathEvaluator evaluator(Basepoint);
 
   for (const auto &step : Path) {
@@ -222,7 +221,6 @@ void RewriteLoop::verify(const RewriteSystem &system) const {
     evaluator.dump(llvm::errs());
     abort();
   }
-#endif
 }
 
 void RewriteLoop::dump(llvm::raw_ostream &out,

lib/AST/RequirementMachine/RewriteSystem.cpp

@@ -631,14 +631,12 @@ void RewriteSystem::recordRewriteLoop(MutableTerm basepoint,
 }
 
 void RewriteSystem::verifyRewriteRules(ValidityPolicy policy) const {
-#ifndef NDEBUG
-
 #define ASSERT_RULE(expr) \
   if (!(expr)) { \
     llvm::errs() << "&&& Malformed rewrite rule: " << rule << "\n"; \
     llvm::errs() << "&&& " << #expr << "\n\n"; \
     dump(llvm::errs()); \
-    assert(expr); \
+    abort(); \
   }
 
   for (const auto &rule : Rules) {
@@ -705,7 +703,6 @@ void RewriteSystem::verifyRewriteRules(ValidityPolicy policy) const {
   }
 
 #undef ASSERT_RULE
-#endif
 }
 
 void RewriteSystem::dump(llvm::raw_ostream &out) const {