[Evaluator] Enforce consistent results for cyclic requests

Record when we encounter a request cycle, and enforce that the outer
step of the cycle also returns the default value. This fixes a couple
of crashers where we were ending up with conflicting values depending
on whether the request was queried from within the cycle or from
outside it.

Author: hamishknight

include/swift/AST/Evaluator.h

@@ -164,6 +164,9 @@ class Evaluator {
   /// is treated as a stack and is used to detect cycles.
   llvm::SetVector<ActiveRequest> activeRequests;
 
+  /// A set of active requests that have been diagnosed for a cycle.
+  llvm::DenseSet<ActiveRequest> diagnosedActiveCycles;
+
   /// A cache that stores the results of requests.
   evaluator::RequestCache cache;
 
@@ -344,7 +347,17 @@ class Evaluator {
 
     recorder.beginRequest<Request>();
 
-    auto result = getRequestFunction<Request>()(request, *this);
+    auto result = [&]() -> typename Request::OutputType {
+      auto reqResult = getRequestFunction<Request>()(request, *this);
+
+      // If we diagnosed a cycle for this request, we want to only use the
+      // default value to ensure we return a consistent result.
+      if (!diagnosedActiveCycles.empty() &&
+          diagnosedActiveCycles.erase(activeReq)) {
+        return defaultValueFn();
+      }
+      return reqResult;
+    }();
 
     recorder.endRequest<Request>(request);
 

lib/AST/Decl.cpp

@@ -2690,10 +2690,16 @@ bool PatternBindingDecl::hasStorage() const {
 
 const PatternBindingEntry *
 PatternBindingDecl::getCheckedPatternBindingEntry(unsigned i) const {
-  return evaluateOrDefault(
+  auto result = evaluateOrDefault(
       getASTContext().evaluator,
       PatternBindingEntryRequest{const_cast<PatternBindingDecl *>(this), i},
       nullptr);
+
+  // If we ran into a cycle, we can still return the entry.
+  if (!result)
+    return &getPatternList()[i];
+
+  return result;
 }
 
 void PatternBindingDecl::setPattern(unsigned i, Pattern *P,
@@ -7382,8 +7388,7 @@ bool ProtocolDecl::existentialConformsToSelf() const {
 }
 
 bool ProtocolDecl::hasSelfOrAssociatedTypeRequirements() const {
-  // Because we will have considered all the protocols in a cyclic hierarchy by
-  // the time the cycle is hit.
+  // Be conservative and avoid diagnosing if we hit a cycle.
   const bool resultForCycle = false;
 
   return evaluateOrDefault(getASTContext().evaluator,

lib/AST/Evaluator.cpp

@@ -155,8 +155,12 @@ void Evaluator::diagnoseCycle(const ActiveRequest &request) {
   request.diagnoseCycle(cycleDiags.getDiags());
 
   for (const auto &step : llvm::reverse(activeRequests)) {
-    if (step == request)
+    if (step == request) {
+      // Note that we diagnosed a cycle for the outermost step to ensure it
+      // also returns a cyclic result.
+      diagnosedActiveCycles.insert(step);
       return;
+    }
     step.noteCycleStep(cycleDiags.getDiags());
   }
 

test/Index/circular.swift

@@ -28,17 +28,17 @@ class Cycle2_C: Cycle2_B {}
 // CHECK:   RelBase | class/Swift | Cycle2_C | {{[^ ]*}}
 
 class TestCase1: XCTestCase {}
-// CHECK: [[@LINE-1]]:7 | class(test)/Swift | TestCase1 | {{[^ ]*}} | Def | rel: 0
+// CHECK: [[@LINE-1]]:7 | class/Swift | TestCase1 | {{[^ ]*}} | Def | rel: 0
 // CHECK: [[@LINE-2]]:18 | class/Swift | XCTestCase | {{[^ ]*}} | Ref,RelBase | rel: 1
-// CHECK:   RelBase | class(test)/Swift | TestCase1 | {{[^ ]*}}
+// CHECK:   RelBase | class/Swift | TestCase1 | {{[^ ]*}}
 class XCTestCase: TestCase1 {}
 // CHECK: [[@LINE-1]]:7 | class/Swift | XCTestCase | {{[^ ]*}} | Def | rel: 0
-// CHECK: [[@LINE-2]]:19 | class(test)/Swift | TestCase1 | {{[^ ]*}} | Ref,RelBase | rel: 1
+// CHECK: [[@LINE-2]]:19 | class/Swift | TestCase1 | {{[^ ]*}} | Ref,RelBase | rel: 1
 // CHECK:   RelBase | class/Swift | XCTestCase | {{[^ ]*}}
 class TestCase2: TestCase1 {}
-// CHECK: [[@LINE-1]]:7 | class(test)/Swift | TestCase2 | {{[^ ]*}} | Def | rel: 0
-// CHECK: [[@LINE-2]]:18 | class(test)/Swift | TestCase1 | {{[^ ]*}} | Ref,RelBase | rel: 1
-// CHECK:   RelBase | class(test)/Swift | TestCase2 | {{[^ ]*}}
+// CHECK: [[@LINE-1]]:7 | class/Swift | TestCase2 | {{[^ ]*}} | Def | rel: 0
+// CHECK: [[@LINE-2]]:18 | class/Swift | TestCase1 | {{[^ ]*}} | Ref,RelBase | rel: 1
+// CHECK:   RelBase | class/Swift | TestCase2 | {{[^ ]*}}
 
 protocol SelfCycleP: SelfCycleP {}
 // CHECK: [[@LINE-1]]:10 | protocol/Swift | SelfCycleP | {{[^ ]*}} | Def | rel: 0

test/decl/circularity.swift

@@ -98,7 +98,7 @@ protocol P1 {
 }
 
 class C4 {
-  required init(x: Int) {}
+  required init(x: Int) {} // expected-note {{'required' initializer is declared in superclass here}}
 }
 
 class D4 : C4, P1 { // expected-note@:7 {{through reference here}}
@@ -107,7 +107,7 @@ class D4 : C4, P1 { // expected-note@:7 {{through reference here}}
     // expected-note@-2:17 {{through reference here}}
     super.init(x: x)
   }
-}
+} // expected-error {{'required' initializer 'init(x:)' must be provided by subclass of 'C4'}}
 
 // https://github.com/apple/swift/issues/54662
 // N.B. This used to compile in 5.1.

test/type/explicit_existential_cyclic_protocol.swift

@@ -3,9 +3,8 @@
 
 // REQUIRES: swift_feature_ExistentialAny
 
-// 'HasSelfOrAssociatedTypeRequirementsRequest' should evaluate to false in
-// the event of a cycle because we will have considered all the protocols in a
-// cyclic hierarchy by the time the cycle is hit.
+// 'HasSelfOrAssociatedTypeRequirementsRequest' evaluates to false in the event
+// of a cycle.
 do {
   do {
     protocol P1 : P2 {}
@@ -30,6 +29,6 @@ do {
     // expected-explicit-any-error@-2 1 {{protocol 'P2' refines itself}}
 
     let _: P2
-    // expected-warning@-1 {{use of protocol 'P2' as a type must be written 'any P2'}}
+    // expected-explicit-any-warning@-1 {{use of protocol 'P2' as a type must be written 'any P2'}}
   }
 }

validation-test/compiler_crashers_2/aa9ebb248dabc27b.swift renamed to validation-test/compiler_crashers_2_fixed/aa9ebb248dabc27b.swift

@@ -1,4 +1,4 @@
 // {"kind":"typecheck","signature":"swift::ConformanceChecker::resolveWitnessViaDerivation(swift::ValueDecl*)","signatureAssert":"Assertion failed: (Conformance->getWitnessUncached(requirement).getDecl() == match.Witness && \"Deduced different witnesses?\"), function recordWitness"}
-// RUN: not --crash %target-swift-frontend -typecheck %s
+// RUN: not %target-swift-frontend -typecheck %s
 class a : Codable {
   b = a(

validation-test/compiler_crashers_2/dcd4ff250d0bde99.swift renamed to validation-test/compiler_crashers_2_fixed/dcd4ff250d0bde99.swift

@@ -1,5 +1,5 @@
 // {"kind":"typecheck","signature":"swift::ValueDecl::isObjC() const","signatureAssert":"Assertion failed: (!LazySemanticInfo.isObjCComputed || LazySemanticInfo.isObjC == value), function setIsObjC"}
-// RUN: not --crash %target-swift-frontend -typecheck %s
+// RUN: not %target-swift-frontend -typecheck %s
 // REQUIRES: objc_interop
 protocol a
   @objc protocol b : c

validation-test/compiler_crashers_2/dfe03ade76b34fdb.swift renamed to validation-test/compiler_crashers_2_fixed/dfe03ade76b34fdb.swift

@@ -1,4 +1,4 @@
 // {"kind":"typecheck","signature":"swift::ASTContext::getSpecializedConformance(swift::Type, swift::NormalProtocolConformance*, swift::SubstitutionMap)","signatureAssert":"Assertion failed: (substitutions.getGenericSignature().getCanonicalSignature() == generic->getGenericSignature().getCanonicalSignature()), function getSpecializedConformance"}
-// RUN: not --crash %target-swift-frontend -typecheck %s
+// RUN: not %target-swift-frontend -typecheck %s
 protocol a{ < } protocol b { associatedtype c : a where d == Self }
 class e<f> : a where f : b, f.c == e