Merge pull request #42203 from slavapestov/rqm-fixes-5.7

Cherry-pick RequirementMachine fixes to 5.7 branch

Author: slavapestov

include/swift/AST/TypeCheckRequests.h

@@ -604,7 +604,6 @@ struct WhereClauseOwner {
     return !(lhs == rhs);
   }
 
-public:
   /// Retrieve the array of requirements.
   MutableArrayRef<RequirementRepr> getRequirements() const;
 
@@ -1903,8 +1902,7 @@ class InferredGenericSignatureRequest :
 /// InferredGenericSignatureRequest.
 class InferredGenericSignatureRequestRQM :
     public SimpleRequest<InferredGenericSignatureRequestRQM,
-                         GenericSignatureWithError (ModuleDecl *,
-                                                    const GenericSignatureImpl *,
+                         GenericSignatureWithError (const GenericSignatureImpl *,
                                                     GenericParamList *,
                                                     WhereClauseOwner,
                                                     SmallVector<Requirement, 2>,
@@ -1920,7 +1918,6 @@ class InferredGenericSignatureRequestRQM :
   // Evaluation.
   GenericSignatureWithError
   evaluate(Evaluator &evaluator,
-           ModuleDecl *parentModule,
            const GenericSignatureImpl *baseSignature,
            GenericParamList *genericParams,
            WhereClauseOwner whereClause,

lib/AST/GenericSignatureBuilder.cpp

@@ -8464,7 +8464,6 @@ InferredGenericSignatureRequest::evaluate(
     return evaluateOrDefault(
         ctx.evaluator,
         InferredGenericSignatureRequestRQM{
-          parentModule,
           parentSig,
           genericParams,
           whereClause,

lib/AST/RequirementMachine/KnuthBendix.cpp

@@ -157,8 +157,66 @@ RewriteSystem::computeCriticalPair(ArrayRef<Symbol>::const_iterator from,
       return false;
     }
 
-    // Add the pair (X, TYV).
-    pairs.emplace_back(x, tyv, path);
+    // If X == TUW for some W, then the critical pair is (TUW, TYV),
+    // and we have
+    // - lhs == (TUV => TUW)
+    // - rhs == (U => Y).
+    //
+    // We explicitly apply the rewrite step (Y => U) to the beginning of the
+    // rewrite path, transforming the critical pair to (TYW, TYV).
+    //
+    // In particular, if V == W.[P] for some protocol P, then we in fact have
+    // a property rule and a same-type rule:
+    //
+    // - lhs == (TUW.[P] => TUW)
+    // - rhs == (U => Y)
+    //
+    // Without this hack, the critical pair would be:
+    //
+    // (TUW => TYW.[P])
+    //
+    // With this hack, the critical pair becomes:
+    //
+    // (TYW.[P] => TYW)
+    //
+    // This ensures that the newly-added rule is itself a property rule;
+    // otherwise, this would only be the case if addRule() reduced TUW
+    // into TYW without immediately reducing some subterm of TUW first.
+    //
+    // While completion will eventually simplify all such rules down into
+    // property rules, their existance in the first place breaks subtle
+    // invariants in the minimal conformances algorithm, which expects
+    // homotopy generators describing redundant protocol conformance rules
+    // to have a certain structure.
+    if (t.size() + rhs.getLHS().size() <= x.size() &&
+        std::equal(rhs.getLHS().begin(),
+                   rhs.getLHS().end(),
+                   x.begin() + t.size())) {
+      // We have a path from TUW to TYV. Invert to get a path from TYV to
+      // TUW.
+      path.invert();
+
+      // Compute the term W.
+      MutableTerm w(x.begin() + t.size() + rhs.getLHS().size(), x.end());
+
+      // Now add a rewrite step T.(U => Y).W to get a path from TYV to
+      // TYW.
+      path.add(RewriteStep::forRewriteRule(/*startOffset=*/t.size(),
+                                           /*endOffset=*/w.size(),
+                                           getRuleID(rhs),
+                                           /*inverse=*/false));
+
+      // Compute the term TYW.
+      MutableTerm tyw(t);
+      tyw.append(rhs.getRHS());
+      tyw.append(w);
+
+      // Add the pair (TYV, TYW).
+      pairs.emplace_back(tyv, tyw, path);
+    } else {
+      // Add the pair (X, TYV).
+      pairs.emplace_back(x, tyv, path);
+    }
   } else {
     // lhs == TU -> X, rhs == UV -> Y.
 
@@ -217,7 +275,7 @@ RewriteSystem::computeCriticalPair(ArrayRef<Symbol>::const_iterator from,
     // - lhs == (TU -> X)
     // - rhs == (UV -> UW).
     //
-    // We explicitly apply the rewrite step (TU = X) to the rewrite path,
+    // We explicitly apply the rewrite step (TU => X) to the rewrite path,
     // transforming the critical pair to (XV, XW).
     //
     // In particular, if T == X, U == [P] for some protocol P, and
@@ -229,15 +287,15 @@ RewriteSystem::computeCriticalPair(ArrayRef<Symbol>::const_iterator from,
     //
     // Without this hack, the critical pair would be:
     //
-    // (T.w.[p] => T.[P].w)
+    // (T.W.[p] => T.[P].W)
     //
     // With this hack, the critical pair becomes:
     //
-    // (T.w.[p] => T.w)
+    // (T.W.[p] => T.W)
     //
     // This ensures that the newly-added rule is itself a property rule;
-    // otherwise, this would only be the case if addRule() reduced T.[P].w
-    // into T.w without immediately reducing some subterm of T first.
+    // otherwise, this would only be the case if addRule() reduced T.[P].W
+    // into T.W without immediately reducing some subterm of T first.
     //
     // While completion will eventually simplify all such rules down into
     // property rules, their existance in the first place breaks subtle

lib/AST/RequirementMachine/RequirementLowering.cpp

@@ -160,6 +160,7 @@
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/SetVector.h"
 #include "RewriteContext.h"
+#include "NameLookup.h"
 
 using namespace swift;
 using namespace rewriting;
@@ -384,12 +385,40 @@ swift::rewriting::desugarRequirement(Requirement req, SourceLoc loc,
 // Requirement realization and inference.
 //
 
-static void realizeTypeRequirement(Type subjectType, Type constraintType,
+static void realizeTypeRequirement(DeclContext *dc,
+                                   Type subjectType, Type constraintType,
                                    SourceLoc loc,
                                    SmallVectorImpl<StructuralRequirement> &result,
                                    SmallVectorImpl<RequirementError> &errors) {
   SmallVector<Requirement, 2> reqs;
 
+  // The GenericSignatureBuilder allowed the right hand side of a
+  // conformance or superclass requirement to reference a protocol
+  // typealias whose underlying type was a protocol or class.
+  //
+  // Since protocol typealiases resolve to DependentMemberTypes in
+  // ::Structural mode, this relied on the GSB's "delayed requirements"
+  // mechanism.
+  //
+  // The RequirementMachine does not have an equivalent, and cannot really
+  // support that because we need to collect the protocols mentioned on
+  // the right hand sides of conformance requirements ahead of time.
+  //
+  // However, we can support it in simple cases where the typealias is
+  // defined in the protocol itself and is accessed as a member of 'Self'.
+  if (auto *proto = dc->getSelfProtocolDecl()) {
+    if (auto memberType = constraintType->getAs<DependentMemberType>()) {
+      if (memberType->getBase()->isEqual(proto->getSelfInterfaceType())) {
+        SmallVector<TypeDecl *, 1> result;
+        lookupConcreteNestedType(proto, memberType->getName(), result);
+        auto *typeDecl = findBestConcreteNestedType(result);
+        if (auto *aliasDecl = dyn_cast_or_null<TypeAliasDecl>(typeDecl)) {
+          constraintType = aliasDecl->getUnderlyingType();
+        }
+      }
+    }
+  }
+
   if (constraintType->isConstraintType()) {
     // Handle conformance requirements.
     desugarConformanceRequirement(subjectType, constraintType, loc, reqs, errors);
@@ -534,18 +563,20 @@ void swift::rewriting::inferRequirements(
 /// Desugar a requirement and perform requirement inference if requested
 /// to obtain zero or more structural requirements.
 void swift::rewriting::realizeRequirement(
+    DeclContext *dc,
     Requirement req, RequirementRepr *reqRepr,
-    ModuleDecl *moduleForInference,
+    bool shouldInferRequirements,
     SmallVectorImpl<StructuralRequirement> &result,
     SmallVectorImpl<RequirementError> &errors) {
   auto firstType = req.getFirstType();
   auto loc = (reqRepr ? reqRepr->getSeparatorLoc() : SourceLoc());
+  auto *moduleForInference = dc->getParentModule();
 
   switch (req.getKind()) {
   case RequirementKind::Superclass:
   case RequirementKind::Conformance: {
     auto secondType = req.getSecondType();
-    if (moduleForInference) {
+    if (shouldInferRequirements) {
       auto firstLoc = (reqRepr ? reqRepr->getSubjectRepr()->getStartLoc()
                                : SourceLoc());
       inferRequirements(firstType, firstLoc, moduleForInference, result);
@@ -555,12 +586,12 @@ void swift::rewriting::realizeRequirement(
       inferRequirements(secondType, secondLoc, moduleForInference, result);
     }
 
-    realizeTypeRequirement(firstType, secondType, loc, result, errors);
+    realizeTypeRequirement(dc, firstType, secondType, loc, result, errors);
     break;
   }
 
   case RequirementKind::Layout: {
-    if (moduleForInference) {
+    if (shouldInferRequirements) {
       auto firstLoc = (reqRepr ? reqRepr->getSubjectRepr()->getStartLoc()
                                : SourceLoc());
       inferRequirements(firstType, firstLoc, moduleForInference, result);
@@ -578,7 +609,7 @@ void swift::rewriting::realizeRequirement(
 
   case RequirementKind::SameType: {
     auto secondType = req.getSecondType();
-    if (moduleForInference) {
+    if (shouldInferRequirements) {
       auto firstLoc = (reqRepr ? reqRepr->getFirstTypeRepr()->getStartLoc()
                                : SourceLoc());
       inferRequirements(firstType, firstLoc, moduleForInference, result);
@@ -602,11 +633,13 @@ void swift::rewriting::realizeRequirement(
 /// Collect structural requirements written in the inheritance clause of an
 /// AssociatedTypeDecl or GenericTypeParamDecl.
 void swift::rewriting::realizeInheritedRequirements(
-    TypeDecl *decl, Type type, ModuleDecl *moduleForInference,
+    TypeDecl *decl, Type type, bool shouldInferRequirements,
     SmallVectorImpl<StructuralRequirement> &result,
     SmallVectorImpl<RequirementError> &errors) {
   auto &ctx = decl->getASTContext();
   auto inheritedTypes = decl->getInherited();
+  auto *dc = decl->getInnermostDeclContext();
+  auto *moduleForInference = dc->getParentModule();
 
   for (unsigned index : indices(inheritedTypes)) {
     Type inheritedType
@@ -616,41 +649,21 @@ void swift::rewriting::realizeInheritedRequirements(
                           Type());
     if (!inheritedType) continue;
 
-    // The GenericSignatureBuilder allowed an associated type's inheritance
-    // clause to reference a protocol typealias whose underlying type was a
-    // protocol or class.
-    //
-    // Since protocol typealiases resolve to DependentMemberTypes in
-    // ::Structural mode, this relied on the GSB's "delayed requirements"
-    // mechanism.
-    //
-    // The RequirementMachine does not have an equivalent, and cannot really
-    // support that because we need to collect the protocols mentioned on
-    // the right hand sides of conformance requirements ahead of time.
-    //
-    // However, we can support it in simple cases where the typealias is
-    // defined in the protocol itself and is accessed as a member of 'Self'.
-    if (auto *assocTypeDecl = dyn_cast<AssociatedTypeDecl>(decl)) {
-      if (auto memberType = inheritedType->getAs<DependentMemberType>()) {
-        if (memberType->getBase()->isEqual(
-            assocTypeDecl->getProtocol()->getSelfInterfaceType())) {
-          inheritedType
-            = evaluateOrDefault(ctx.evaluator,
-                                InheritedTypeRequest{decl, index,
-                                TypeResolutionStage::Interface},
-                                Type());
-          if (!inheritedType) continue;
-        }
-      }
+    // Ignore trivially circular protocol refinement (protocol P : P)
+    // since we diagnose that elsewhere. Adding a rule here would emit
+    // a useless redundancy warning.
+    if (auto *protoDecl = dyn_cast<ProtocolDecl>(decl)) {
+      if (inheritedType->isEqual(protoDecl->getDeclaredInterfaceType()))
+        continue;
     }
 
     auto *typeRepr = inheritedTypes[index].getTypeRepr();
     SourceLoc loc = (typeRepr ? typeRepr->getStartLoc() : SourceLoc());
-    if (moduleForInference) {
+    if (shouldInferRequirements) {
       inferRequirements(inheritedType, loc, moduleForInference, result);
     }
 
-    realizeTypeRequirement(type, inheritedType, loc, result, errors);
+    realizeTypeRequirement(dc, type, inheritedType, loc, result, errors);
   }
 }
 
@@ -823,14 +836,14 @@ StructuralRequirementsRequest::evaluate(Evaluator &evaluator,
   auto selfTy = proto->getSelfInterfaceType();
 
   realizeInheritedRequirements(proto, selfTy,
-                               /*moduleForInference=*/nullptr,
+                               /*inferRequirements=*/false,
                                result, errors);
 
   // Add requirements from the protocol's own 'where' clause.
   WhereClauseOwner(proto).visitRequirements(TypeResolutionStage::Structural,
       [&](const Requirement &req, RequirementRepr *reqRepr) {
-        realizeRequirement(req, reqRepr,
-                           /*moduleForInference=*/nullptr,
+        realizeRequirement(proto, req, reqRepr,
+                           /*inferRequirements=*/false,
                            result, errors);
         return false;
       });
@@ -855,15 +868,15 @@ StructuralRequirementsRequest::evaluate(Evaluator &evaluator,
     // Add requirements placed directly on this associated type.
     auto assocType = assocTypeDecl->getDeclaredInterfaceType();
     realizeInheritedRequirements(assocTypeDecl, assocType,
-                                 /*moduleForInference=*/nullptr,
+                                 /*inferRequirements=*/false,
                                  result, errors);
 
     // Add requirements from this associated type's where clause.
     WhereClauseOwner(assocTypeDecl).visitRequirements(
         TypeResolutionStage::Structural,
         [&](const Requirement &req, RequirementRepr *reqRepr) {
-          realizeRequirement(req, reqRepr,
-                             /*moduleForInference=*/nullptr,
+          realizeRequirement(proto, req, reqRepr,
+                             /*inferRequirements=*/false,
                              result, errors);
           return false;
         });

lib/AST/RequirementMachine/RequirementLowering.h

@@ -45,13 +45,14 @@ void desugarRequirement(Requirement req, SourceLoc loc,
 void inferRequirements(Type type, SourceLoc loc, ModuleDecl *module,
                        SmallVectorImpl<StructuralRequirement> &result);
 
-void realizeRequirement(Requirement req, RequirementRepr *reqRepr,
-                        ModuleDecl *moduleForInference,
+void realizeRequirement(DeclContext *dc,
+                        Requirement req, RequirementRepr *reqRepr,
+                        bool shouldInferRequirements,
                         SmallVectorImpl<StructuralRequirement> &result,
                         SmallVectorImpl<RequirementError> &errors);
 
 void realizeInheritedRequirements(TypeDecl *decl, Type type,
-                                  ModuleDecl *moduleForInference,
+                                  bool shouldInferRequirements,
                                   SmallVectorImpl<StructuralRequirement> &result,
                                   SmallVectorImpl<RequirementError> &errors);
 

lib/AST/RequirementMachine/RequirementMachine.cpp

@@ -485,6 +485,10 @@ RequirementMachine::computeCompletion(RewriteSystem::ValidityPolicy policy) {
 
       unsigned rulesAdded = (System.getRules().size() - ruleCount);
 
+      // If buildPropertyMap() didn't add any new rules, we are done.
+      if (rulesAdded == 0)
+        break;
+
       if (Stats) {
         Stats->getFrontendCounters()
           .NumRequirementMachineUnifiedConcreteTerms += rulesAdded;
@@ -507,10 +511,6 @@ RequirementMachine::computeCompletion(RewriteSystem::ValidityPolicy policy) {
         return std::make_pair(CompletionResult::MaxRuleCount,
                               System.getRules().size() - 1);
       }
-
-      // If buildPropertyMap() didn't add any new rules, we are done.
-      if (rulesAdded == 0)
-        break;
     }
   }