RequirementMachine: Fix subtle bug in isRecursivelyConstructingRequirementMachine()

I don't have a reduced test case. It was possible for computing the requirement
signatures of a connected component to have finished, and yet for the
ProtocolDecl::hasComputedRequirementSignature() method to return false, if
we had evaluated a RequirementSignatureRequestRQM but not the top-level
RequirementSignatureRequest.

Instead, track whether we've computed the signatures for a component directly.

I don't have a reduced test case. It would arise with associated type inference,
which uses this predicate to break nasty cycles.

Author: slavapestov

lib/AST/RequirementMachine/RequirementMachineRequests.cpp

@@ -28,6 +28,7 @@
 #include "swift/AST/RequirementSignature.h"
 #include "swift/AST/TypeCheckRequests.h"
 #include "swift/AST/TypeRepr.h"
+#include "swift/Basic/Defer.h"
 #include "swift/Basic/Statistic.h"
 #include <memory>
 #include <vector>
@@ -214,9 +215,15 @@ RequirementSignatureRequestRQM::evaluate(Evaluator &evaluator,
                                 ctx.AllocateCopy(typeAliases));
   }
 
+  auto &rewriteCtx = ctx.getRewriteContext();
+
   // We build requirement signatures for all protocols in a strongly connected
   // component at the same time.
-  auto component = ctx.getRewriteContext().getProtocolComponent(proto);
+  auto component = rewriteCtx.startComputingRequirementSignatures(proto);
+
+  SWIFT_DEFER {
+    rewriteCtx.finishComputingRequirementSignatures(proto);
+  };
 
   // Collect user-written requirements from the protocols in this connected
   // component.
@@ -234,7 +241,7 @@ RequirementSignatureRequestRQM::evaluate(Evaluator &evaluator,
   for (;;) {
     // Heap-allocate the requirement machine to save stack space.
     std::unique_ptr<RequirementMachine> machine(new RequirementMachine(
-        ctx.getRewriteContext()));
+        rewriteCtx));
 
     auto status = machine->initWithProtocolWrittenRequirements(component, protos);
     if (status.first != CompletionResult::Success) {

lib/AST/RequirementMachine/RewriteContext.cpp

@@ -288,7 +288,8 @@ RewriteContext::getProtocolComponentImpl(const ProtocolDecl *proto) {
 ///
 /// This can only be called once, to prevent multiple requirement machines
 /// for being built with the same component.
-ArrayRef<const ProtocolDecl *> RewriteContext::getProtocolComponent(
+ArrayRef<const ProtocolDecl *>
+RewriteContext::startComputingRequirementSignatures(
     const ProtocolDecl *proto) {
   auto &component = getProtocolComponentImpl(proto);
 
@@ -305,6 +306,17 @@ ArrayRef<const ProtocolDecl *> RewriteContext::getProtocolComponent(
   return component.Protos;
 }
 
+/// Mark the component as having completed, which will ensure that
+/// isRecursivelyComputingRequirementMachine() returns false.
+void RewriteContext::finishComputingRequirementSignatures(
+    const ProtocolDecl *proto) {
+  auto &component = getProtocolComponentImpl(proto);
+
+  assert(component.ComputingRequirementSignatures &&
+         "Didn't call startComputingRequirementSignatures()");
+  component.ComputedRequirementSignatures = true;
+}
+
 /// Get the list of protocols in the strongly connected component (SCC)
 /// of the protocol dependency graph containing the given protocol.
 ///
@@ -340,11 +352,12 @@ RequirementMachine *RewriteContext::getRequirementMachine(
   return newMachine;
 }
 
+/// Note: This doesn't use Evaluator::hasActiveRequest(), because in reality
+/// the active request could be for any protocol in the connected component.
+///
+/// Instead, just check a flag set in the component itself.
 bool RewriteContext::isRecursivelyConstructingRequirementMachine(
     const ProtocolDecl *proto) {
-  if (proto->isRequirementSignatureComputed())
-    return false;
-
   auto found = Protos.find(proto);
   if (found == Protos.end())
     return false;
@@ -353,7 +366,10 @@ bool RewriteContext::isRecursivelyConstructingRequirementMachine(
   if (component == Components.end())
     return false;
 
-  return component->second.ComputingRequirementSignatures;
+  // If we've started but not finished, we're in the middle of computing
+  // requirement signatures.
+  return (component->second.ComputingRequirementSignatures &&
+          !component->second.ComputedRequirementSignatures);
 }
 
 /// We print stats in the destructor, which should get executed at the end of

lib/AST/RequirementMachine/RewriteContext.h

@@ -86,10 +86,14 @@ class RewriteContext final {
     /// The members of this connected component.
     ArrayRef<const ProtocolDecl *> Protos;
 
-    /// Whether we are currently computing the requirement signatures of
+    /// Whether we have started computing the requirement signatures of
     /// the protocols in this component.
     bool ComputingRequirementSignatures = false;
 
+    /// Whether we have finished computing the requirement signatures of
+    /// the protocols in this component.
+    bool ComputedRequirementSignatures = false;
+
     /// Each connected component has a lazily-created requirement machine
     /// built from the requirement signatures of the protocols in this
     /// component.
@@ -194,7 +198,11 @@ class RewriteContext final {
   RequirementMachine *getRequirementMachine(CanGenericSignature sig);
   bool isRecursivelyConstructingRequirementMachine(CanGenericSignature sig);
 
-  ArrayRef<const ProtocolDecl *> getProtocolComponent(const ProtocolDecl *proto);
+  ArrayRef<const ProtocolDecl *>
+  startComputingRequirementSignatures(const ProtocolDecl *proto);
+
+  void finishComputingRequirementSignatures(const ProtocolDecl *proto);
+
   RequirementMachine *getRequirementMachine(const ProtocolDecl *proto);
   bool isRecursivelyConstructingRequirementMachine(const ProtocolDecl *proto);