Merge pull request #40502 from slavapestov/rqm-diagnose-confluence-violation

slavapestov · web-flow · commit 2755f74eeb55 · 2021-12-10T13:51:49.000-05:00
RequirementMachine: Diagnose non-confluent rewrite systems instead of asserting
diff --git a/include/swift/AST/DiagnosticsSema.def b/include/swift/AST/DiagnosticsSema.def
@@ -2533,6 +2533,11 @@ WARNING(associated_type_override_typealias,none,
         "associated type %0 is redundant with type %0 declared in inherited "
         "%1 %2", (Identifier, DescriptiveDeclKind, Type))
 
+ERROR(requirement_machine_completion_failed,none,
+      "cannot build rewrite system for %select{generic signature|protocol}0; "
+      "%select{step|depth}1 limit exceeded",
+      (unsigned, unsigned))
+
 ERROR(associated_type_objc,none,
       "associated type %0 cannot be declared inside '@objc' protocol %1",
       (Identifier, Identifier))
diff --git a/lib/AST/RequirementMachine/KnuthBendix.cpp b/lib/AST/RequirementMachine/KnuthBendix.cpp
@@ -528,11 +528,6 @@ RewriteSystem::computeConfluentCompletion(unsigned maxIterations,
       auto to = lhs.getLHS().end();
       while (from < to) {
         Trie.findAll(from, to, [&](unsigned j) {
-          // We don't have to consider the same pair of rules more than once,
-          // since those critical pairs were already resolved.
-          if (!CheckedOverlaps.insert(std::make_pair(i, j)).second)
-            return;
-
           const auto &rhs = getRule(j);
           if (rhs.isSimplified())
             return;
@@ -554,6 +549,11 @@ RewriteSystem::computeConfluentCompletion(unsigned maxIterations,
               return;
           }
 
+          // We don't have to consider the same pair of rules more than once,
+          // since those critical pairs were already resolved.
+          if (!CheckedOverlaps.insert(std::make_pair(i, j)).second)
+            return;
+
           // Try to repair the confluence violation by adding a new rule.
           if (computeCriticalPair(from, lhs, rhs,
                                   resolvedCriticalPairs,
diff --git a/lib/AST/RequirementMachine/RequirementLowering.cpp b/lib/AST/RequirementMachine/RequirementLowering.cpp
@@ -342,20 +342,18 @@ void swift::rewriting::realizeRequirement(
     ModuleDecl *moduleForInference,
     SmallVectorImpl<StructuralRequirement> &result) {
   auto firstType = req.getFirstType();
-  if (moduleForInference) {
-    auto firstLoc = (reqRepr ? reqRepr->getFirstTypeRepr()->getStartLoc()
-                             : SourceLoc());
-    inferRequirements(firstType, firstLoc, moduleForInference, result);
-  }
-
   auto loc = (reqRepr ? reqRepr->getSeparatorLoc() : SourceLoc());
 
   switch (req.getKind()) {
   case RequirementKind::Superclass:
   case RequirementKind::Conformance: {
     auto secondType = req.getSecondType();
     if (moduleForInference) {
-      auto secondLoc = (reqRepr ? reqRepr->getSecondTypeRepr()->getStartLoc()
+      auto firstLoc = (reqRepr ? reqRepr->getSubjectRepr()->getStartLoc()
+                               : SourceLoc());
+      inferRequirements(firstType, firstLoc, moduleForInference, result);
+
+      auto secondLoc = (reqRepr ? reqRepr->getConstraintRepr()->getStartLoc()
                                 : SourceLoc());
       inferRequirements(secondType, secondLoc, moduleForInference, result);
     }
@@ -365,6 +363,12 @@ void swift::rewriting::realizeRequirement(
   }
 
   case RequirementKind::Layout: {
+    if (moduleForInference) {
+      auto firstLoc = (reqRepr ? reqRepr->getSubjectRepr()->getStartLoc()
+                               : SourceLoc());
+      inferRequirements(firstType, firstLoc, moduleForInference, result);
+    }
+
     SmallVector<Requirement, 2> reqs;
     desugarLayoutRequirement(firstType, req.getLayoutConstraint(), reqs);
 
@@ -377,6 +381,10 @@ void swift::rewriting::realizeRequirement(
   case RequirementKind::SameType: {
     auto secondType = req.getSecondType();
     if (moduleForInference) {
+      auto firstLoc = (reqRepr ? reqRepr->getFirstTypeRepr()->getStartLoc()
+                               : SourceLoc());
+      inferRequirements(firstType, firstLoc, moduleForInference, result);
+
       auto secondLoc = (reqRepr ? reqRepr->getSecondTypeRepr()->getStartLoc()
                                 : SourceLoc());
       inferRequirements(secondType, secondLoc, moduleForInference, result);
diff --git a/lib/AST/RequirementMachine/RequirementMachine.cpp b/lib/AST/RequirementMachine/RequirementMachine.cpp
@@ -35,12 +35,32 @@ RequirementMachine::RequirementMachine(RewriteContext &ctx)
 
 RequirementMachine::~RequirementMachine() {}
 
+static void checkCompletionResult(const RequirementMachine &machine,
+                                  CompletionResult result) {
+  switch (result) {
+  case CompletionResult::Success:
+    break;
+
+  case CompletionResult::MaxIterations:
+    llvm::errs() << "Rewrite system exceeds maximum completion step count\n";
+    machine.dump(llvm::errs());
+    abort();
+
+  case CompletionResult::MaxDepth:
+    llvm::errs() << "Rewrite system exceeds maximum completion depth\n";
+    machine.dump(llvm::errs());
+    abort();
+  }
+}
+
 /// Build a requirement machine for the requirements of a generic signature.
 ///
 /// This must only be called exactly once, before any other operations are
 /// performed on this requirement machine.
 ///
 /// Used by ASTContext::getOrCreateRequirementMachine().
+///
+/// Asserts if completion fails within the configured number of steps.
 void RequirementMachine::initWithGenericSignature(CanGenericSignature sig) {
   Sig = sig;
   Params.append(sig.getGenericParams().begin(),
@@ -64,7 +84,8 @@ void RequirementMachine::initWithGenericSignature(CanGenericSignature sig) {
                     std::move(builder.PermanentRules),
                     std::move(builder.RequirementRules));
 
-  computeCompletion(RewriteSystem::DisallowInvalidRequirements);
+  auto result = computeCompletion(RewriteSystem::DisallowInvalidRequirements);
+  checkCompletionResult(*this, result);
 
   if (Dump) {
     llvm::dbgs() << "}\n";
@@ -79,7 +100,10 @@ void RequirementMachine::initWithGenericSignature(CanGenericSignature sig) {
 /// performed on this requirement machine.
 ///
 /// Used by RequirementSignatureRequest.
-void RequirementMachine::initWithProtocols(ArrayRef<const ProtocolDecl *> protos) {
+///
+/// Returns failure if completion fails within the configured number of steps.
+CompletionResult
+RequirementMachine::initWithProtocols(ArrayRef<const ProtocolDecl *> protos) {
   Protos = protos;
 
   FrontendStatsTracer tracer(Stats, "build-rewrite-system");
@@ -100,12 +124,13 @@ void RequirementMachine::initWithProtocols(ArrayRef<const ProtocolDecl *> protos
                     std::move(builder.PermanentRules),
                     std::move(builder.RequirementRules));
 
-  // FIXME: Only if the protocols were written in source, though.
-  computeCompletion(RewriteSystem::AllowInvalidRequirements);
+  auto result = computeCompletion(RewriteSystem::AllowInvalidRequirements);
 
   if (Dump) {
     llvm::dbgs() << "}\n";
   }
+
+  return result;
 }
 
 /// Build a requirement machine from a set of generic parameters and
@@ -115,6 +140,8 @@ void RequirementMachine::initWithProtocols(ArrayRef<const ProtocolDecl *> protos
 /// performed on this requirement machine.
 ///
 /// Used by AbstractGenericSignatureRequest.
+///
+/// Asserts if completion fails within the configured number of steps.
 void RequirementMachine::initWithAbstractRequirements(
     ArrayRef<GenericTypeParamType *> genericParams,
     ArrayRef<Requirement> requirements) {
@@ -139,7 +166,8 @@ void RequirementMachine::initWithAbstractRequirements(
                     std::move(builder.PermanentRules),
                     std::move(builder.RequirementRules));
 
-  computeCompletion(RewriteSystem::AllowInvalidRequirements);
+  auto result = computeCompletion(RewriteSystem::AllowInvalidRequirements);
+  checkCompletionResult(*this, result);
 
   if (Dump) {
     llvm::dbgs() << "}\n";
@@ -153,7 +181,10 @@ void RequirementMachine::initWithAbstractRequirements(
 /// performed on this requirement machine.
 ///
 /// Used by InferredGenericSignatureRequest.
-void RequirementMachine::initWithWrittenRequirements(
+///
+/// Returns failure if completion fails within the configured number of steps.
+CompletionResult
+RequirementMachine::initWithWrittenRequirements(
     ArrayRef<GenericTypeParamType *> genericParams,
     ArrayRef<StructuralRequirement> requirements) {
   Params.append(genericParams.begin(), genericParams.end());
@@ -177,17 +208,20 @@ void RequirementMachine::initWithWrittenRequirements(
                     std::move(builder.PermanentRules),
                     std::move(builder.RequirementRules));
 
-  computeCompletion(RewriteSystem::AllowInvalidRequirements);
+  auto result = computeCompletion(RewriteSystem::AllowInvalidRequirements);
 
   if (Dump) {
     llvm::dbgs() << "}\n";
   }
+
+  return result;
 }
 
 /// Attempt to obtain a confluent rewrite system by iterating the Knuth-Bendix
 /// completion procedure together with property map construction until fixed
 /// point.
-void RequirementMachine::computeCompletion(RewriteSystem::ValidityPolicy policy) {
+CompletionResult
+RequirementMachine::computeCompletion(RewriteSystem::ValidityPolicy policy) {
   while (true) {
     // First, run the Knuth-Bendix algorithm to resolve overlapping rules.
     auto result = System.computeConfluentCompletion(
@@ -200,26 +234,8 @@ void RequirementMachine::computeCompletion(RewriteSystem::ValidityPolicy policy)
     }
 
     // Check for failure.
-    auto checkCompletionResult = [&]() {
-      switch (result.first) {
-      case CompletionResult::Success:
-        break;
-
-      case CompletionResult::MaxIterations:
-        llvm::errs() << "Generic signature " << Sig
-                     << " exceeds maximum completion step count\n";
-        System.dump(llvm::errs());
-        abort();
-
-      case CompletionResult::MaxDepth:
-        llvm::errs() << "Generic signature " << Sig
-                     << " exceeds maximum completion depth\n";
-        System.dump(llvm::errs());
-        abort();
-      }
-    };
-
-    checkCompletionResult();
+    if (result.first != CompletionResult::Success)
+      return result.first;
 
     // Check invariants.
     System.verifyRewriteRules(policy);
@@ -236,7 +252,9 @@ void RequirementMachine::computeCompletion(RewriteSystem::ValidityPolicy policy)
         .NumRequirementMachineUnifiedConcreteTerms += result.second;
     }
 
-    checkCompletionResult();
+    // Check for failure.
+    if (result.first != CompletionResult::Success)
+      return result.first;
 
     // If buildPropertyMap() added new rules, we run another round of
     // Knuth-Bendix, and build the property map again.
@@ -250,6 +268,8 @@ void RequirementMachine::computeCompletion(RewriteSystem::ValidityPolicy policy)
 
   assert(!Complete);
   Complete = true;
+
+  return CompletionResult::Success;
 }
 
 bool RequirementMachine::isComplete() const {
diff --git a/lib/AST/RequirementMachine/RequirementMachine.h b/lib/AST/RequirementMachine/RequirementMachine.h
@@ -36,6 +36,7 @@ class InferredGenericSignatureRequestRQM;
 class LayoutConstraint;
 class ProtocolDecl;
 class Requirement;
+class RequirementSignatureRequestRQM;
 class Type;
 class UnifiedStatsReporter;
 
@@ -47,6 +48,7 @@ class RewriteContext;
 class RequirementMachine final {
   friend class swift::ASTContext;
   friend class swift::rewriting::RewriteContext;
+  friend class swift::RequirementSignatureRequestRQM;
   friend class swift::AbstractGenericSignatureRequestRQM;
   friend class swift::InferredGenericSignatureRequestRQM;
 
@@ -84,17 +86,17 @@ class RequirementMachine final {
   RequirementMachine &operator=(RequirementMachine &&) = delete;
 
   void initWithGenericSignature(CanGenericSignature sig);
-  void initWithProtocols(ArrayRef<const ProtocolDecl *> protos);
+  CompletionResult initWithProtocols(ArrayRef<const ProtocolDecl *> protos);
   void initWithAbstractRequirements(
       ArrayRef<GenericTypeParamType *> genericParams,
       ArrayRef<Requirement> requirements);
-  void initWithWrittenRequirements(
+  CompletionResult initWithWrittenRequirements(
       ArrayRef<GenericTypeParamType *> genericParams,
       ArrayRef<StructuralRequirement> requirements);
 
   bool isComplete() const;
 
-  void computeCompletion(RewriteSystem::ValidityPolicy policy);
+  CompletionResult computeCompletion(RewriteSystem::ValidityPolicy policy);
 
   MutableTerm getLongestValidPrefix(const MutableTerm &term) const;
 
diff --git a/lib/AST/RequirementMachine/RequirementMachineRequests.cpp b/lib/AST/RequirementMachine/RequirementMachineRequests.cpp
@@ -21,6 +21,7 @@
 #include "RequirementMachine.h"
 #include "swift/AST/ASTContext.h"
 #include "swift/AST/Decl.h"
+#include "swift/AST/DiagnosticsSema.h"
 #include "swift/AST/GenericSignature.h"
 #include "swift/AST/LazyResolver.h"
 #include "swift/AST/Requirement.h"
@@ -241,16 +242,41 @@ RequirementSignatureRequestRQM::evaluate(Evaluator &evaluator,
 
   // We build requirement signatures for all protocols in a strongly connected
   // component at the same time.
-  auto *machine = ctx.getRewriteContext().getRequirementMachine(proto);
-  auto requirements = machine->computeMinimalProtocolRequirements();
+  auto component = ctx.getRewriteContext().getProtocolComponent(proto);
+
+  // Heap-allocate the requirement machine to save stack space.
+  std::unique_ptr<RequirementMachine> machine(new RequirementMachine(
+      ctx.getRewriteContext()));
+
+  auto status = machine->initWithProtocols(component);
+  if (status != CompletionResult::Success) {
+    // All we can do at this point is diagnose and give each protocol an empty
+    // requirement signature.
+    for (const auto *otherProto : component) {
+      ctx.Diags.diagnose(otherProto->getLoc(),
+                         diag::requirement_machine_completion_failed,
+                         /*protocol=*/1,
+                         status == CompletionResult::MaxIterations ? 0 : 1);
+
+      if (otherProto != proto) {
+        ctx.evaluator.cacheOutput(
+          RequirementSignatureRequestRQM{const_cast<ProtocolDecl *>(otherProto)},
+          ArrayRef<Requirement>());
+      }
+    }
+
+    return ArrayRef<Requirement>();
+  }
+
+  auto minimalRequirements = machine->computeMinimalProtocolRequirements();
 
   bool debug = machine->getDebugOptions().contains(DebugFlags::Minimization);
 
   // The requirement signature for the actual protocol that the result
   // was kicked off with.
   ArrayRef<Requirement> result;
 
-  for (const auto &pair : requirements) {
+  for (const auto &pair : minimalRequirements) {
     auto *otherProto = pair.first;
     const auto &reqs = pair.second;
 
@@ -393,7 +419,10 @@ InferredGenericSignatureRequestRQM::evaluate(
     return false;
   };
 
+  SourceLoc loc;
   if (genericParamList) {
+    loc = genericParamList->getLAngleLoc();
+
     // Extensions never have a parent signature.
     assert(genericParamList->getOuterParameters() == nullptr || !parentSig);
 
@@ -433,6 +462,9 @@ InferredGenericSignatureRequestRQM::evaluate(
   }
 
   if (whereClause) {
+    if (loc.isInvalid())
+      loc = whereClause.getLoc();
+
     std::move(whereClause).visitRequirements(
         TypeResolutionStage::Structural,
         visitRequirement);
@@ -457,7 +489,16 @@ InferredGenericSignatureRequestRQM::evaluate(
   std::unique_ptr<RequirementMachine> machine(new RequirementMachine(
       ctx.getRewriteContext()));
 
-  machine->initWithWrittenRequirements(genericParams, requirements);
+  auto status = machine->initWithWrittenRequirements(genericParams, requirements);
+  if (status != CompletionResult::Success) {
+    ctx.Diags.diagnose(loc,
+                       diag::requirement_machine_completion_failed,
+                       /*protocol=*/0,
+                       status == CompletionResult::MaxIterations ? 0 : 1);
+
+    auto result = GenericSignature::get(genericParams, {});
+    return GenericSignatureWithError(result, /*hadError=*/true);
+  }
 
   auto minimalRequirements =
     machine->computeMinimalGenericSignatureRequirements();
diff --git a/lib/AST/RequirementMachine/RewriteContext.cpp b/lib/AST/RequirementMachine/RewriteContext.cpp
diff --git a/lib/AST/RequirementMachine/RewriteContext.h b/lib/AST/RequirementMachine/RewriteContext.h
diff --git a/test/Generics/non_confluent.swift b/test/Generics/non_confluent.swift
