Remove SimpleRequest::breakCycle

This patch removes the need for Request objects to provide a default
cycle-breaking value, instead opting to return llvm::Expected so clients
must handle a cycle failure explicitly.

Currently, all clients do the 'default' behavior, but this opens the
possibility for future requests to handle failures explicitly.

docs/RequestEvaluator.md

@@ -65,7 +65,6 @@ The request-evaluator is relatively new to the Swift compiler, having been intro
 * Cycle diagnostics are far too complicated and produce very poor results. Consider replacing the current `diagnoseCycle`/`noteCycleStep` scheme with a single method that produces summary information (e.g., a short summary string + source location information) and provides richer diagnostics from that string.
 * The `isCached()` check to determine whether a specific instance of a request is worth caching may be at the wrong level, because one generally has to duplicate effort (or worse, code!) to make the decision in `isCached()`. Consider whether the `evaluator()` function could return something special to say "produce this value without caching" vs. the normal "produce this value with caching".
 * Try to eliminate more boilerplate from subclasses of [`SimpleRequest`](https://github.com/apple/swift/blob/master/include/swift/AST/SimpleRequest.h). We are going to have a *lot* of requests.
-* Make requests return LLVM's [`Expected`](https://github.com/llvm-mirror/llvm/blob/master/include/llvm/Support/Error.h), so the evaluator can report evaluation errors such as "cycle detected" to clients of the evaluator. This should eliminate the `breakCycle()` method from requests.
 * Each request supports a simple printing operation (via `simple_display`): implement a parsing scheme so we can take the output of `simple_display` and parse it into a request. Build a command-line testing interface so we can parse a source file and make a specific request (e.g., `SuperclassTypeRequest("Foo")`) to see the results.
 * Port more mutable-state AST queries over to requests. This often requires a lot of refactoring!
 * Port higher-level queries (e.g., those that come from SourceKit) over to the request-evaluator, so we can see the dependencies of a given SourceKit request for testing and performance tuning.

include/swift/AST/AccessRequests.h

@@ -42,11 +42,11 @@ private:
   friend class SimpleRequest;
 
   // Evaluation.
-  AccessLevel evaluate(Evaluator &evaluator, ValueDecl *decl) const;
+  llvm::Expected<AccessLevel> evaluate(Evaluator &evaluator,
+                                       ValueDecl *decl) const;
 
 public:
   // Cycle handling
-  AccessLevel breakCycle() const { return AccessLevel::Private; }
   void diagnoseCycle(DiagnosticEngine &diags) const;
   void noteCycleStep(DiagnosticEngine &diags) const;
 
@@ -71,11 +71,11 @@ private:
   friend class SimpleRequest;
 
   // Evaluation.
-  AccessLevel evaluate(Evaluator &evaluator, AbstractStorageDecl *decl) const;
+  llvm::Expected<AccessLevel>
+  evaluate(Evaluator &evaluator, AbstractStorageDecl *decl) const;
 
 public:
   // Cycle handling
-  AccessLevel breakCycle() const { return AccessLevel::Private; }
   void diagnoseCycle(DiagnosticEngine &diags) const;
   void noteCycleStep(DiagnosticEngine &diags) const;
 
@@ -98,14 +98,11 @@ private:
   friend class SimpleRequest;
 
   // Evaluation.
-  DefaultAndMax
+  llvm::Expected<DefaultAndMax>
   evaluate(Evaluator &evaluator, ExtensionDecl *decl) const;
 
 public:
   // Cycle handling
-  DefaultAndMax
-  breakCycle() const { return std::make_pair(AccessLevel::Private,
-                                             AccessLevel::Private); }
   void diagnoseCycle(DiagnosticEngine &diags) const;
   void noteCycleStep(DiagnosticEngine &diags) const;
 

include/swift/AST/Evaluator.h

@@ -25,6 +25,7 @@
 #include "llvm/ADT/DenseMap.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/SetVector.h"
+#include "llvm/Support/Error.h"
 #include "llvm/Support/PrettyStackTrace.h"
 #include <string>
 #include <tuple>
@@ -52,7 +53,7 @@ using AbstractRequestFunction = void(void);
 /// Form the specific request function for the given request type.
 template<typename Request>
 using RequestFunction =
-    typename Request::OutputType(const Request &, Evaluator &);
+  llvm::Expected<typename Request::OutputType>(const Request &, Evaluator &);
 
 /// Pretty stack trace handler for an arbitrary request.
 template<typename Request>
@@ -69,6 +70,49 @@ public:
   }
 };
 
+/// An llvm::ErrorInfo container for a request in which a cycle was detected
+/// and diagnosed.
+template <typename Request>
+struct CyclicalRequestError : 
+  public llvm::ErrorInfo<CyclicalRequestError<Request>> {
+public:
+  static char ID;
+  const Request &request;
+
+  CyclicalRequestError(const Request &request): request(request) {}
+
+  virtual void log(llvm::raw_ostream &out) const {
+    out << "Cycle detected:\n";
+    simple_display(out, request);
+    out << "\n";
+  }
+
+  virtual std::error_code convertToErrorCode() const {
+    // This is essentially unused, but is a temporary requirement for
+    // llvm::ErrorInfo subclasses.
+    llvm_unreachable("shouldn't get std::error_code from CyclicalRequestError");
+  }
+};
+
+template <typename Request>
+char CyclicalRequestError<Request>::ID = '\0';
+
+/// Evaluates a given request or returns a default value if a cycle is detected.
+template <typename Request>
+typename Request::OutputType
+evaluateOrDefault(
+  Evaluator &eval, Request req, typename Request::OutputType def) {
+  auto result = eval(req);
+  if (auto err = result.takeError()) {
+    llvm::handleAllErrors(std::move(err),
+      [](const CyclicalRequestError<Request> &E) {
+        // cycle detected
+      });
+    return def;
+  }
+  return *result;
+}
+
 /// Report that a request of the given kind is being evaluated, so it
 /// can be recorded by the stats reporter.
 template<typename Request>
@@ -101,7 +145,6 @@ void reportEvaluatedRequest(UnifiedStatsReporter &stats,
 ///
 ///       void diagnoseCycle(DiagnosticEngine &diags) const;
 ///       void noteCycleStep(DiagnosticEngine &diags) const;
-///       OutputType breakCycle() const;
 ///   - Caching policy:
 ///
 ///     static const bool isEverCached;
@@ -211,10 +254,13 @@ public:
   /// Evaluate the given request and produce its result,
   /// consulting/populating the cache as required.
   template<typename Request>
-  typename Request::OutputType operator()(const Request &request) {
+  llvm::Expected<typename Request::OutputType>
+  operator()(const Request &request) {
     // Check for a cycle.
-    if (checkDependency(AnyRequest(request)))
-      return request.breakCycle();
+    if (checkDependency(AnyRequest(request))) {
+      return llvm::Error(
+        llvm::make_unique<CyclicalRequestError<Request>>(request));
+    }
 
     // Make sure we remove this from the set of active requests once we're
     // done.
@@ -232,9 +278,10 @@ public:
   /// Use this to describe cases where there are multiple (known)
   /// requests that all need to be satisfied.
   template<typename ...Requests>
-  std::tuple<typename Requests::OutputType...>
+  std::tuple<llvm::Expected<typename Requests::OutputType>...>
   operator()(const Requests &...requests) {
-    return std::tuple<typename Requests::OutputType...>((*this)(requests)...);
+    return std::tuple<llvm::Expected<typename Requests::OutputType>...>(
+      (*this)(requests)...);
   }
 
   /// Clear the cache stored within this evaluator.
@@ -260,7 +307,8 @@ private:
   /// be cached.
   template<typename Request,
            typename std::enable_if<Request::isEverCached>::type * = nullptr>
-  typename Request::OutputType getResult(const Request &request) {
+  llvm::Expected<typename Request::OutputType>
+  getResult(const Request &request) {
     // The request can be cached, but check a predicate to determine
     // whether this particular instance is cached. This allows more
     // fine-grained control over which instances get cache.
@@ -274,13 +322,15 @@ private:
   /// will never be cached.
   template<typename Request,
            typename std::enable_if<!Request::isEverCached>::type * = nullptr>
-  typename Request::OutputType getResult(const Request &request) {
+  llvm::Expected<typename Request::OutputType>
+  getResult(const Request &request) {
     return getResultUncached(request);
   }
 
   /// Produce the result of the request without caching.
   template<typename Request>
-  typename Request::OutputType getResultUncached(const Request &request) {
+  llvm::Expected<typename Request::OutputType>
+  getResultUncached(const Request &request) {
     // Clear out the dependencies on this request; we're going to recompute
     // them now anyway.
     dependencies[AnyRequest(request)].clear();
@@ -298,7 +348,8 @@ private:
   /// and detect recursion.
   template<typename Request,
            typename std::enable_if<Request::hasExternalCache>::type * = nullptr>
-  typename Request::OutputType getResultCached(const Request &request) {
+  llvm::Expected<typename Request::OutputType>
+  getResultCached(const Request &request) {
     // If there is a cached result, return it.
     if (auto cached = request.getCachedResult())
       return *cached;
@@ -306,8 +357,11 @@ private:
     // Compute the result.
     auto result = getResultUncached(request);
 
-    // Cache the result.
-    request.cacheResult(result);
+    // Cache the result if applicable.
+    if (!result)
+      return result;
+
+    request.cacheResult(*result);
 
     // Return it.
     return result;
@@ -319,6 +373,9 @@ private:
       typename Request,
       typename std::enable_if<!Request::hasExternalCache>::type * = nullptr>
   typename Request::OutputType getResultCached(const Request &request) {
+  llvm::Expected<typename Request::OutputType>
+  getResultCached(const Request &request) {
+    AnyRequest anyRequest{request};
     // If we already have an entry for this request in the cache, return it.
     auto known = cache.find_as(request);
     if (known != cache.end()) {
@@ -327,9 +384,11 @@ private:
 
     // Compute the result.
     auto result = getResultUncached(request);
+    if (!result)
+      return result;
 
     // Cache the result.
-    cache.insert({AnyRequest(request), result});
+    cache.insert({AnyRequest(request), *result});
     return result;
   }
 

include/swift/AST/NameLookupRequests.h

@@ -81,7 +81,6 @@ public:
   bool isCached() const { return true; }
 
   // Cycle handling
-  DirectlyReferencedTypeDecls breakCycle() const { return { }; }
   void diagnoseCycle(DiagnosticEngine &diags) const;
   void noteCycleStep(DiagnosticEngine &diags) const;
 };
@@ -127,7 +126,6 @@ public:
   bool isCached() const { return true; }
 
   // Cycle handling
-  DirectlyReferencedTypeDecls breakCycle() const { return { }; }
   void diagnoseCycle(DiagnosticEngine &diags) const;
   void noteCycleStep(DiagnosticEngine &diags) const;
 };
@@ -145,14 +143,14 @@ private:
   friend class SimpleRequest;
 
   // Evaluation.
-  ClassDecl *evaluate(Evaluator &evaluator, NominalTypeDecl *subject) const;
+  llvm::Expected<ClassDecl *>
+  evaluate(Evaluator &evaluator, NominalTypeDecl *subject) const;
 
 public:
   // Caching
   bool isCached() const { return true; }
 
   // Cycle handling
-  ClassDecl *breakCycle() const { return nullptr; }
   void diagnoseCycle(DiagnosticEngine &diags) const;
   void noteCycleStep(DiagnosticEngine &diags) const;
 };
@@ -170,7 +168,8 @@ private:
   friend class SimpleRequest;
 
   // Evaluation.
-  NominalTypeDecl *evaluate(Evaluator &evaluator, ExtensionDecl *ext) const;
+  llvm::Expected<NominalTypeDecl *>
+  evaluate(Evaluator &evaluator, ExtensionDecl *ext) const;
 
 public:
   // Separate caching.
@@ -179,7 +178,6 @@ public:
   void cacheResult(NominalTypeDecl *value) const;
 
   // Cycle handling
-  NominalTypeDecl *breakCycle() const { return nullptr; }
   void diagnoseCycle(DiagnosticEngine &diags) const;
   void noteCycleStep(DiagnosticEngine &diags) const;
 };

include/swift/AST/SimpleRequest.h

@@ -54,15 +54,9 @@ enum class CacheKind {
 ///
 /// The \c Derived class needs to implement several operations. The most
 /// important one takes an evaluator and the input values, then computes the
-/// final result:
+/// final result, optionally bubbling up errors from recursive evaulations:
 /// \code
-///   Output evaluate(Evaluator &evaluator, Inputs...) const;
-/// \endcode
-///
-/// The \c Derived class will also need to implement an operation to break a
-/// cycle if one is found, i.e.,
-/// \code
-///   OutputType breakCycle() const;
+///   llvm::Expected<Output> evaluate(Evaluator &evaluator, Inputs...) const;
 /// \endcode
 ///
 /// Cycle diagnostics can be handled in one of two ways. Either the \c Derived
@@ -103,8 +97,8 @@ class SimpleRequest {
   }
 
   template<size_t ...Indices>
-  Output callDerived(Evaluator &evaluator,
-                     llvm::index_sequence<Indices...>) const {
+  llvm::Expected<Output>
+  callDerived(Evaluator &evaluator, llvm::index_sequence<Indices...>) const {
     static_assert(sizeof...(Indices) > 0, "Subclass must define evaluate()");
     return asDerived().evaluate(evaluator, std::get<Indices>(storage)...);
   }
@@ -131,8 +125,8 @@ public:
     : storage(inputs...) { }
 
   /// Request evaluation function that will be registered with the evaluator.
-  static OutputType evaluateRequest(const Derived &request,
-                                    Evaluator &evaluator) {
+  static llvm::Expected<OutputType>
+  evaluateRequest(const Derived &request, Evaluator &evaluator) {
     return request.callDerived(evaluator,
                                llvm::index_sequence_for<Inputs...>());
   }

include/swift/AST/TypeCheckRequests.h

@@ -52,13 +52,13 @@ private:
   friend class SimpleRequest;
 
   // Evaluation.
-  Type evaluate(Evaluator &evaluator,
+  llvm::Expected<Type>
+  evaluate(Evaluator &evaluator,
            llvm::PointerUnion<TypeDecl *, ExtensionDecl *> decl,
            unsigned index) const;
 
 public:
   // Cycle handling
-  Type breakCycle() const { return Type(); }
   void diagnoseCycle(DiagnosticEngine &diags) const;
   void noteCycleStep(DiagnosticEngine &diags) const;
 
@@ -81,11 +81,11 @@ private:
   friend class SimpleRequest;
 
   // Evaluation.
-  Type evaluate(Evaluator &evaluator, NominalTypeDecl *classDecl) const;
+  llvm::Expected<Type>
+  evaluate(Evaluator &evaluator, NominalTypeDecl *classDecl) const;
 
 public:
   // Cycle handling
-  Type breakCycle() const { return Type(); }
   void diagnoseCycle(DiagnosticEngine &diags) const;
   void noteCycleStep(DiagnosticEngine &diags) const;
 
@@ -108,11 +108,11 @@ private:
   friend class SimpleRequest;
 
   // Evaluation.
-  Type evaluate(Evaluator &evaluator, EnumDecl *enumDecl) const;
+  llvm::Expected<Type>
+  evaluate(Evaluator &evaluator, EnumDecl *enumDecl) const;
 
 public:
   // Cycle handling
-  Type breakCycle() const { return Type(); }
   void diagnoseCycle(DiagnosticEngine &diags) const;
   void noteCycleStep(DiagnosticEngine &diags) const;
 
@@ -136,12 +136,11 @@ private:
   friend class SimpleRequest;
 
   // Evaluation.
-  llvm::TinyPtrVector<ValueDecl *> evaluate(Evaluator &evaluator,
-                                            ValueDecl *decl) const;
+  llvm::Expected<llvm::TinyPtrVector<ValueDecl *>>
+  evaluate(Evaluator &evaluator, ValueDecl *decl) const;
 
 public:
   // Cycle handling
-  llvm::TinyPtrVector<ValueDecl *> breakCycle() const { return { }; }
   void diagnoseCycle(DiagnosticEngine &diags) const;
   void noteCycleStep(DiagnosticEngine &diags) const;
 
@@ -164,11 +163,10 @@ private:
   friend class SimpleRequest;
 
   // Evaluation.
-  bool evaluate(Evaluator &evaluator, ValueDecl *decl) const;
+  llvm::Expected<bool> evaluate(Evaluator &evaluator, ValueDecl *decl) const;
 
 public:
   // Cycle handling
-  bool breakCycle() const;
   void diagnoseCycle(DiagnosticEngine &diags) const;
   void noteCycleStep(DiagnosticEngine &diags) const;
 
@@ -191,11 +189,10 @@ private:
   friend class SimpleRequest;
 
   // Evaluation.
-  bool evaluate(Evaluator &evaluator, ValueDecl *decl) const;
+  llvm::Expected<bool> evaluate(Evaluator &evaluator, ValueDecl *decl) const;
 
 public:
   // Cycle handling
-  bool breakCycle() const;
   void diagnoseCycle(DiagnosticEngine &diags) const;
   void noteCycleStep(DiagnosticEngine &diags) const;
 

lib/AST/AccessRequests.cpp

@@ -36,8 +36,8 @@ namespace swift {
 //----------------------------------------------------------------------------//
 // AccessLevel computation
 //----------------------------------------------------------------------------//
-AccessLevel AccessLevelRequest::evaluate(Evaluator &evaluator,
-                                         ValueDecl *D) const {
+llvm::Expected<AccessLevel>
+AccessLevelRequest::evaluate(Evaluator &evaluator, ValueDecl *D) const {
   assert(!D->hasAccess());
 
   // Check if the decl has an explicit access control attribute.
@@ -153,7 +153,8 @@ void AccessLevelRequest::cacheResult(AccessLevel value) const {
 // the cycle of computation associated with formal accesses, we give it its own
 // request.
 
-AccessLevel SetterAccessLevelRequest::evaluate(Evaluator &evaluator,
+llvm::Expected<AccessLevel>
+SetterAccessLevelRequest::evaluate(Evaluator &evaluator,
                                    AbstractStorageDecl *ASD) const {
   assert(!ASD->Accessors.getInt().hasValue());
   if (auto *AA = ASD->getAttrs().getAttribute<SetterAccessAttr>())
@@ -196,7 +197,7 @@ void SetterAccessLevelRequest::cacheResult(AccessLevel value) const {
 // DefaultAccessLevel computation
 //----------------------------------------------------------------------------//
 
-std::pair<AccessLevel, AccessLevel>
+llvm::Expected<std::pair<AccessLevel, AccessLevel>>
 DefaultAndMaxAccessLevelRequest::evaluate(Evaluator &evaluator,
                                           ExtensionDecl *ED) const {
   assert(!ED->hasDefaultAccessLevel());

lib/AST/Decl.cpp

@@ -984,14 +984,14 @@ ExtensionDecl::takeConformanceLoaderSlow() {
 
 NominalTypeDecl *ExtensionDecl::getExtendedNominal() const {
   ASTContext &ctx = getASTContext();
-  return ctx.evaluator(
-      ExtendedNominalRequest{const_cast<ExtensionDecl *>(this)});;
+  return evaluateOrDefault(ctx.evaluator,
+    ExtendedNominalRequest{const_cast<ExtensionDecl *>(this)}, nullptr);
 }
 
 Type ExtensionDecl::getInheritedType(unsigned index) const {
   ASTContext &ctx = getASTContext();
-  return ctx.evaluator(InheritedTypeRequest{const_cast<ExtensionDecl *>(this),
-                                            index});
+  return evaluateOrDefault(ctx.evaluator,
+    InheritedTypeRequest{const_cast<ExtensionDecl *>(this), index}, Type());
 }
 
 bool ExtensionDecl::isConstrainedExtension() const {
@@ -1017,14 +1017,16 @@ bool ExtensionDecl::isEquivalentToExtendedContext() const {
 
 AccessLevel ExtensionDecl::getDefaultAccessLevel() const {
   ASTContext &ctx = getASTContext();
-  return ctx.evaluator(
-    DefaultAndMaxAccessLevelRequest{const_cast<ExtensionDecl *>(this)}).first;
+  return evaluateOrDefault(ctx.evaluator,
+    DefaultAndMaxAccessLevelRequest{const_cast<ExtensionDecl *>(this)},
+    {AccessLevel::Private, AccessLevel::Private}).first;
 }
 
 AccessLevel ExtensionDecl::getMaxAccessLevel() const {
   ASTContext &ctx = getASTContext();
-  return ctx.evaluator(
-    DefaultAndMaxAccessLevelRequest{const_cast<ExtensionDecl *>(this)}).second;
+  return evaluateOrDefault(ctx.evaluator,
+    DefaultAndMaxAccessLevelRequest{const_cast<ExtensionDecl *>(this)},
+    {AccessLevel::Private, AccessLevel::Private}).second;
 }
 
 PatternBindingDecl::PatternBindingDecl(SourceLoc StaticLoc,
@@ -2087,7 +2089,8 @@ CanType ValueDecl::getOverloadSignatureType() const {
 
 llvm::TinyPtrVector<ValueDecl *> ValueDecl::getOverriddenDecls() const {
   ASTContext &ctx = getASTContext();
-  return ctx.evaluator(OverriddenDeclsRequest{const_cast<ValueDecl *>(this)});
+  return evaluateOrDefault(ctx.evaluator,
+    OverriddenDeclsRequest{const_cast<ValueDecl *>(this)}, {});
 }
 
 void ValueDecl::setOverriddenDecls(ArrayRef<ValueDecl *> overridden) {
@@ -2098,7 +2101,9 @@ void ValueDecl::setOverriddenDecls(ArrayRef<ValueDecl *> overridden) {
 
 bool ValueDecl::isObjC() const {
   ASTContext &ctx = getASTContext();
-  return ctx.evaluator(IsObjCRequest{const_cast<ValueDecl *>(this)});
+  return evaluateOrDefault(ctx.evaluator,
+    IsObjCRequest{const_cast<ValueDecl *>(this)},
+    getAttrs().hasAttribute<ObjCAttr>());
 }
 
 void ValueDecl::setIsObjC(bool value) {
@@ -2115,7 +2120,9 @@ void ValueDecl::setIsObjC(bool value) {
 
 bool ValueDecl::isDynamic() const {
   ASTContext &ctx = getASTContext();
-  return ctx.evaluator(IsDynamicRequest{const_cast<ValueDecl *>(this)});
+  return evaluateOrDefault(ctx.evaluator,
+    IsDynamicRequest{const_cast<ValueDecl *>(this)},
+    getAttrs().hasAttribute<DynamicAttr>());
 }
 
 void ValueDecl::setIsDynamic(bool value) {
@@ -2440,7 +2447,8 @@ AccessLevel ValueDecl::getEffectiveAccess() const {
 
 AccessLevel ValueDecl::getFormalAccess() const {
   ASTContext &ctx = getASTContext();
-  return ctx.evaluator(AccessLevelRequest{const_cast<ValueDecl *>(this)});
+  return evaluateOrDefault(ctx.evaluator,
+    AccessLevelRequest{const_cast<ValueDecl *>(this)}, AccessLevel::Private);
 }
 
 bool ValueDecl::hasOpenAccess(const DeclContext *useDC) const {
@@ -2662,8 +2670,8 @@ void ValueDecl::copyFormalAccessFrom(const ValueDecl *source,
 
 Type TypeDecl::getInheritedType(unsigned index) const {
   ASTContext &ctx = getASTContext();
-  return ctx.evaluator(InheritedTypeRequest{const_cast<TypeDecl *>(this),
-                                            index});
+  return evaluateOrDefault(ctx.evaluator,
+    InheritedTypeRequest{const_cast<TypeDecl *>(this), index}, Type());
 }
 
 Type TypeDecl::getDeclaredInterfaceType() const {
@@ -3166,7 +3174,8 @@ EnumDecl::EnumDecl(SourceLoc EnumLoc,
 
 Type EnumDecl::getRawType() const {
   ASTContext &ctx = getASTContext();
-  return ctx.evaluator(EnumRawTypeRequest{const_cast<EnumDecl *>(this)});
+  return evaluateOrDefault(ctx.evaluator,
+    EnumRawTypeRequest{const_cast<EnumDecl *>(this)}, Type());
 }
 
 StructDecl::StructDecl(SourceLoc StructLoc, Identifier Name, SourceLoc NameLoc,
@@ -3615,12 +3624,14 @@ ProtocolDecl::getAssociatedTypeMembers() const {
 
 Type ProtocolDecl::getSuperclass() const {
   ASTContext &ctx = getASTContext();
-  return ctx.evaluator(SuperclassTypeRequest{const_cast<ProtocolDecl *>(this)});
+  return evaluateOrDefault(ctx.evaluator,
+    SuperclassTypeRequest{const_cast<ProtocolDecl *>(this)}, Type());
 }
 
 ClassDecl *ProtocolDecl::getSuperclassDecl() const {
   ASTContext &ctx = getASTContext();
-  return ctx.evaluator(SuperclassDeclRequest{const_cast<ProtocolDecl *>(this)});
+  return evaluateOrDefault(ctx.evaluator,
+    SuperclassDeclRequest{const_cast<ProtocolDecl *>(this)}, nullptr);
 }
 
 void ProtocolDecl::setSuperclass(Type superclass) {
@@ -4228,8 +4239,9 @@ bool AbstractStorageDecl::AccessorRecord::registerAccessor(AccessorDecl *decl,
 AccessLevel
 AbstractStorageDecl::getSetterFormalAccess() const {
   ASTContext &ctx = getASTContext();
-  return ctx.evaluator(
-        SetterAccessLevelRequest{const_cast<AbstractStorageDecl *>(this)});
+  return evaluateOrDefault(ctx.evaluator,
+        SetterAccessLevelRequest{const_cast<AbstractStorageDecl *>(this)},
+        AccessLevel::Private);
 }
 
 void AbstractStorageDecl::setComputedSetter(AccessorDecl *setter) {
@@ -6078,12 +6090,14 @@ Type TypeBase::getSwiftNewtypeUnderlyingType() {
 
 Type ClassDecl::getSuperclass() const {
   ASTContext &ctx = getASTContext();
-  return ctx.evaluator(SuperclassTypeRequest{const_cast<ClassDecl *>(this)});
+  return evaluateOrDefault(ctx.evaluator,
+    SuperclassTypeRequest{const_cast<ClassDecl *>(this)}, Type());
 }
 
 ClassDecl *ClassDecl::getSuperclassDecl() const {
   ASTContext &ctx = getASTContext();
-  return ctx.evaluator(SuperclassDeclRequest{const_cast<ClassDecl *>(this)});
+  return evaluateOrDefault(ctx.evaluator,
+    SuperclassDeclRequest{const_cast<ClassDecl *>(this)}, nullptr);
 }
 
 void ClassDecl::setSuperclass(Type superclass) {

lib/AST/NameLookup.cpp

@@ -2168,8 +2168,9 @@ resolveTypeDeclsToNominal(Evaluator &evaluator,
       if (!typealiases.insert(typealias).second)
         continue;
 
-      auto underlyingTypeReferences
-        = evaluator(UnderlyingTypeDeclsReferencedRequest{typealias});
+      auto underlyingTypeReferences = evaluateOrDefault(evaluator,
+        UnderlyingTypeDeclsReferencedRequest{typealias}, {});
+
       auto underlyingNominalReferences
         = resolveTypeDeclsToNominal(evaluator, ctx, underlyingTypeReferences,
                                     modulesFound, anyObject, typealiases);
@@ -2443,12 +2444,13 @@ DirectlyReferencedTypeDecls UnderlyingTypeDeclsReferencedRequest::evaluate(
 }
 
 /// Evaluate a superclass declaration request.
-ClassDecl *SuperclassDeclRequest::evaluate(Evaluator &evaluator,
+llvm::Expected<ClassDecl *>
+SuperclassDeclRequest::evaluate(Evaluator &evaluator,
                                 NominalTypeDecl *subject) const {
   for (unsigned i : indices(subject->getInherited())) {
     // Find the inherited declarations referenced at this position.
-    auto inheritedTypes =
-      evaluator(InheritedDeclsReferencedRequest{subject, i});
+    auto inheritedTypes = evaluateOrDefault(evaluator,
+      InheritedDeclsReferencedRequest{subject, i}, {});
 
     // Resolve those type declarations to nominal type declarations.
     SmallVector<ModuleDecl *, 2> modulesFound;
@@ -2467,7 +2469,8 @@ ClassDecl *SuperclassDeclRequest::evaluate(Evaluator &evaluator,
   return nullptr;
 }
 
-NominalTypeDecl *ExtendedNominalRequest::evaluate(Evaluator &evaluator,
+llvm::Expected<NominalTypeDecl *>
+ExtendedNominalRequest::evaluate(Evaluator &evaluator,
                                  ExtensionDecl *ext) const {
   DirectlyReferencedTypeDecls referenced;
   ASTContext &ctx = ext->getASTContext();
@@ -2504,7 +2507,8 @@ void swift::getDirectlyInheritedNominalTypeDecls(
                              : extDecl->getASTContext();
 
   // Find inherited declarations.
-  auto referenced = ctx.evaluator(InheritedDeclsReferencedRequest{decl, i});
+  auto referenced = evaluateOrDefault(ctx.evaluator,
+    InheritedDeclsReferencedRequest{decl, i}, {});
 
   // Resolve those type declarations to nominal type declarations.
   SmallVector<ModuleDecl *, 2> modulesFound;

lib/AST/TypeCheckRequests.cpp

@@ -168,11 +168,6 @@ void OverriddenDeclsRequest::noteCycleStep(DiagnosticEngine &diags) const {
 // isObjC computation.
 //----------------------------------------------------------------------------//
 
-bool IsObjCRequest::breakCycle() const {
-  auto decl = std::get<0>(getStorage());
-  return decl->getAttrs().hasAttribute<ObjCAttr>();
-}
-
 void IsObjCRequest::diagnoseCycle(DiagnosticEngine &diags) const {
   // FIXME: Improve this diagnostic.
   auto decl = std::get<0>(getStorage());
@@ -202,11 +197,6 @@ void IsObjCRequest::cacheResult(bool value) const {
 // isDynamic computation.
 //----------------------------------------------------------------------------//
 
-bool IsDynamicRequest::breakCycle() const {
-  auto decl = std::get<0>(getStorage());
-  return decl->getAttrs().hasAttribute<DynamicAttr>();
-}
-
 void IsDynamicRequest::diagnoseCycle(DiagnosticEngine &diags) const {
   // FIXME: Improve this diagnostic.
   auto decl = std::get<0>(getStorage());

lib/Sema/TypeCheckDecl.cpp

@@ -1203,7 +1203,8 @@ static bool makeDynamic(ValueDecl *decl) {
   return true;
 }
 
-bool IsDynamicRequest::evaluate(Evaluator &evaluator, ValueDecl *decl) const {
+llvm::Expected<bool>
+IsDynamicRequest::evaluate(Evaluator &evaluator, ValueDecl *decl) const {
   // If we can't infer dynamic here, don't.
   if (!DeclAttribute::canAttributeAppearOnDecl(DAK_Dynamic, decl))
     return false;
@@ -1218,11 +1219,18 @@ bool IsDynamicRequest::evaluate(Evaluator &evaluator, ValueDecl *decl) const {
   if (auto accessor = dyn_cast<AccessorDecl>(decl)) {
     switch (accessor->getAccessorKind()) {
     case AccessorKind::Get:
-    case AccessorKind::Set:
-      if (evaluator(IsDynamicRequest{accessor->getStorage()}))
+    case AccessorKind::Set: {
+      auto isDynamicResult = evaluator(
+        IsDynamicRequest{accessor->getStorage()});
+
+      if (!isDynamicResult)
+        return isDynamicResult;
+
+      if (*isDynamicResult)
         return makeDynamic(decl);
 
       return false;
+    }
 
 #define OBJC_ACCESSOR(ID, KEYWORD)
 #define ACCESSOR(ID) \
@@ -1263,7 +1271,9 @@ bool IsDynamicRequest::evaluate(Evaluator &evaluator, ValueDecl *decl) const {
   // Don't do this if the declaration is not exposed to Objective-C; that's
   // currently the (only) manner in which one can make an override of a
   // dynamic declaration non-dynamic.
-  for (auto overridden : evaluator(OverriddenDeclsRequest{decl})) {
+  auto overriddenDecls = evaluateOrDefault(evaluator,
+    OverriddenDeclsRequest{decl}, {});
+  for (auto overridden : overriddenDecls) {
     if (overridden->isDynamic())
       return makeDynamic(decl);
 

lib/Sema/TypeCheckDeclObjC.cpp

@@ -1259,7 +1259,8 @@ static void markAsObjC(ValueDecl *D, ObjCReason reason,
                        Optional<ForeignErrorConvention> errorConvention);
 
 
-bool IsObjCRequest::evaluate(Evaluator &evaluator, ValueDecl *VD) const {
+llvm::Expected<bool>
+IsObjCRequest::evaluate(Evaluator &evaluator, ValueDecl *VD) const {
   auto dc = VD->getDeclContext();
   Optional<ObjCReason> isObjC;
   if (dc->getAsClassOrClassExtensionContext() && !isa<TypeDecl>(VD)) {

lib/Sema/TypeCheckDeclOverride.cpp

@@ -1624,7 +1624,7 @@ computeOverriddenAssociatedTypes(AssociatedTypeDecl *assocType) {
   return overriddenAssocTypes;
 }
 
-llvm::TinyPtrVector<ValueDecl *>
+llvm::Expected<llvm::TinyPtrVector<ValueDecl *>>
 OverriddenDeclsRequest::evaluate(Evaluator &evaluator, ValueDecl *decl) const {
   // For an associated type, compute the (minimized) set of overridden
   // declarations.
@@ -1632,13 +1632,16 @@ OverriddenDeclsRequest::evaluate(Evaluator &evaluator, ValueDecl *decl) const {
     return computeOverriddenAssociatedTypes(assocType);
   }
 
+  // Value to return in error cases
+  auto noResults = llvm::TinyPtrVector<ValueDecl *>();
+
   // Only members of classes can override other declarations.
   if (!decl->getDeclContext()->getAsClassOrClassExtensionContext())
-    return { };
+    return noResults;
 
   // Types that aren't associated types cannot be overridden.
   if (isa<TypeDecl>(decl))
-    return { };
+    return noResults;
 
   // Accessors determine their overrides based on their abstract storage
   // declarations.
@@ -1647,13 +1650,13 @@ OverriddenDeclsRequest::evaluate(Evaluator &evaluator, ValueDecl *decl) const {
 
     // Find the overidden storage declaration. If there isn't one, we're done.
     auto baseASD = overridingASD->getOverriddenDecl();
-    if (!baseASD) return { };
+    if (!baseASD) return noResults;
 
     auto kind = accessor->getAccessorKind();
 
     // Find the base accessor; if there isn't one, we're done.
     auto baseAccessor = baseASD->getAccessor(kind);
-    if (!baseAccessor) return { };
+    if (!baseAccessor) return noResults;
 
     switch (kind) {
       case AccessorKind::Get:
@@ -1664,7 +1667,7 @@ OverriddenDeclsRequest::evaluate(Evaluator &evaluator, ValueDecl *decl) const {
         // forced static dispatch materializeForSet is not itself an
         // override.
         if (baseAccessor->hasForcedStaticDispatch())
-          return { };
+          return noResults;
 
         LLVM_FALLTHROUGH;
 
@@ -1672,7 +1675,7 @@ OverriddenDeclsRequest::evaluate(Evaluator &evaluator, ValueDecl *decl) const {
         // For setter accessors, we need the base's setter to be
         // accessible from the overriding context, or it's not an override.
         if (!baseASD->isSetterAccessibleFrom(overridingASD->getDeclContext()))
-          return { };
+          return noResults;
         break;
 
       case AccessorKind::Address:
@@ -1681,7 +1684,7 @@ OverriddenDeclsRequest::evaluate(Evaluator &evaluator, ValueDecl *decl) const {
       case AccessorKind::WillSet:
       case AccessorKind::Read:
       case AccessorKind::Modify:
-        return { };
+        return noResults;
     }
 
     // We are overriding the base accessor.
@@ -1691,7 +1694,7 @@ OverriddenDeclsRequest::evaluate(Evaluator &evaluator, ValueDecl *decl) const {
     if (matcher.checkOverride(baseAccessor,
                               OverrideCheckingAttempt::PerfectMatch)) {
       invalidateOverrideAttribute(decl);
-      return { };
+      return noResults;
     }
 
     return llvm::TinyPtrVector<ValueDecl *>{baseAccessor};
@@ -1701,17 +1704,17 @@ OverriddenDeclsRequest::evaluate(Evaluator &evaluator, ValueDecl *decl) const {
   // modifier can override declarations.
   if (!isa<ConstructorDecl>(decl) &&
       !decl->getAttrs().hasAttribute<OverrideAttr>())
-    return { };
+    return noResults;
 
   // Try to match potential overridden declarations.
   OverrideMatcher matcher(decl);
   if (!matcher) {
-    return { };
+    return noResults;
   }
 
   auto matches = matcher.match(OverrideCheckingAttempt::PerfectMatch);
   if (matches.empty()) {
-    return { };
+    return noResults;
   }
 
   // If we have more than one potential match, diagnose the ambiguity and
@@ -1720,14 +1723,14 @@ OverriddenDeclsRequest::evaluate(Evaluator &evaluator, ValueDecl *decl) const {
     diagnoseGeneralOverrideFailure(decl, matches,
                                    OverrideCheckingAttempt::PerfectMatch);
     invalidateOverrideAttribute(decl);
-    return { };
+    return noResults;
   }
 
   // Check the correctness of the override.
   if (matcher.checkOverride(matches.front().Decl,
                             OverrideCheckingAttempt::PerfectMatch)) {
     invalidateOverrideAttribute(decl);
-    return { };
+    return noResults;
   }
 
   return llvm::TinyPtrVector<ValueDecl *>{matches.front().Decl};

lib/Sema/TypeCheckRequestFunctions.cpp

@@ -20,9 +20,9 @@
 
 using namespace swift;
 
-Type InheritedTypeRequest::evaluate(
-                        Evaluator &evaluator,
-                        llvm::PointerUnion<TypeDecl *, ExtensionDecl *> decl,
+llvm::Expected<Type>
+InheritedTypeRequest::evaluate(
+  Evaluator &evaluator, llvm::PointerUnion<TypeDecl *, ExtensionDecl *> decl,
   unsigned index) const {
   // Figure out how to resolve types.
   TypeResolutionOptions options = None;
@@ -62,12 +62,24 @@ Type InheritedTypeRequest::evaluate(
   return inheritedType ? inheritedType : ErrorType::get(tc.Context);
 }
 
-Type SuperclassTypeRequest::evaluate(Evaluator &evaluator,
+llvm::Expected<Type>
+SuperclassTypeRequest::evaluate(Evaluator &evaluator,
                                 NominalTypeDecl *nominalDecl) const {
   assert(isa<ClassDecl>(nominalDecl) || isa<ProtocolDecl>(nominalDecl));
 
   for (unsigned int idx : indices(nominalDecl->getInherited())) {
-    Type inheritedType = evaluator(InheritedTypeRequest{nominalDecl, idx});
+    auto result = evaluator(InheritedTypeRequest{nominalDecl, idx});
+
+    if (auto err = result.takeError()) {
+      // FIXME: Should this just return once a cycle is detected?
+      llvm::handleAllErrors(std::move(err),
+        [](const CyclicalRequestError<InheritedTypeRequest> &E) {
+          /* cycle detected */
+        });
+      continue;
+    }
+
+    Type inheritedType = *result;
     if (!inheritedType) continue;
 
     // If we found a class, return it.
@@ -96,10 +108,20 @@ Type SuperclassTypeRequest::evaluate(Evaluator &evaluator,
   return Type();
 }
 
-Type EnumRawTypeRequest::evaluate(Evaluator &evaluator,
-                                  EnumDecl *enumDecl) const {
+llvm::Expected<Type>
+EnumRawTypeRequest::evaluate(Evaluator &evaluator, EnumDecl *enumDecl) const {
   for (unsigned int idx : indices(enumDecl->getInherited())) {
-    Type inheritedType = evaluator(InheritedTypeRequest{enumDecl, idx});
+    auto inheritedTypeResult = evaluator(InheritedTypeRequest{enumDecl, idx});
+    
+    if (auto err = inheritedTypeResult.takeError()) {
+      llvm::handleAllErrors(std::move(err),
+        [](const CyclicalRequestError<InheritedTypeRequest> &E) {
+          // cycle detected
+        });
+      continue;
+    }
+
+    auto &inheritedType = *inheritedTypeResult;
     if (!inheritedType) continue;
 
     // Skip existential types.

unittests/AST/ArithmeticEvaluator.cpp

@@ -88,7 +88,8 @@ struct EvaluationRule
 {
   using SimpleRequest<Derived, Caching, double, ArithmeticExpr *>::SimpleRequest;
 
-  double evaluate(Evaluator &evaluator, ArithmeticExpr *expr) const {
+  llvm::Expected<double>
+  evaluate(Evaluator &evaluator, ArithmeticExpr *expr) const {
     switch (expr->kind) {
     case ArithmeticExpr::Kind::Literal:
       return static_cast<Literal *>(expr)->value;
@@ -97,33 +98,28 @@ struct EvaluationRule
       auto binary = static_cast<Binary *>(expr);
 
       // Evaluate the left- and right-hand sides.
-      double lhsValue, rhsValue;
-      std::tie(lhsValue, rhsValue) =
-        evaluator(Derived{binary->lhs}, Derived{binary->rhs});
+      auto lhsValue = evaluator(Derived{binary->lhs});
+      if (!lhsValue)
+        return lhsValue;
+      auto rhsValue = evaluator(Derived{binary->rhs});
+      if (!rhsValue)
+        return rhsValue;
+
       switch (binary->operatorKind) {
       case Binary::OperatorKind::Sum:
-        return lhsValue + rhsValue;
+        return *lhsValue + *rhsValue;
 
       case Binary::OperatorKind::Product:
-        return lhsValue * rhsValue;
+        return *lhsValue * *rhsValue;
       }
     }
     }
   }
 
-  static bool brokeCycle;
-  double breakCycle() const {
-    brokeCycle = true;
-    return NAN;
-  }
-
   void diagnoseCycle(DiagnosticEngine &diags) const { }
   void noteCycleStep(DiagnosticEngine &diags) const { }
 };
 
-template<typename Derived, CacheKind Caching>
-bool EvaluationRule<Derived, Caching>::brokeCycle = false;
-
 struct InternallyCachedEvaluationRule :
 EvaluationRule<InternallyCachedEvaluationRule, CacheKind::Cached>
 {
@@ -197,6 +193,13 @@ static AbstractRequestFunction *arithmeticRequestFunctions[] = {
 #undef SWIFT_TYPEID
 };
 
+/// Helper to short-circuit errors to NaN.
+template<typename Request>
+static double evalOrNaN(Evaluator &evaluator, const Request &request) {
+  return evaluateOrDefault(evaluator, request, NAN);
+}
+
+
 TEST(ArithmeticEvaluator, Simple) {
   // (3.14159 + 2.71828) * 42
   ArithmeticExpr *pi = new Literal(3.14159);
@@ -213,27 +216,27 @@ TEST(ArithmeticEvaluator, Simple) {
                                      arithmeticRequestFunctions);
 
   const double expectedResult = (3.14159 + 2.71828) * 42.0;
-  EXPECT_EQ(evaluator(InternallyCachedEvaluationRule(product)),
+  EXPECT_EQ(evalOrNaN(evaluator, InternallyCachedEvaluationRule(product)),
             expectedResult);
 
   // Cached response.
-  EXPECT_EQ(evaluator(InternallyCachedEvaluationRule(product)),
+  EXPECT_EQ(evalOrNaN(evaluator, InternallyCachedEvaluationRule(product)),
             expectedResult);
 
   // Uncached
   evaluator.clearCache();
-  EXPECT_EQ(evaluator(UncachedEvaluationRule(product)),
+  EXPECT_EQ(evalOrNaN(evaluator, UncachedEvaluationRule(product)),
             expectedResult);
-  EXPECT_EQ(evaluator(UncachedEvaluationRule(product)),
+  EXPECT_EQ(evalOrNaN(evaluator, UncachedEvaluationRule(product)),
             expectedResult);
 
   // External cached query.
   evaluator.clearCache();
-  EXPECT_EQ(evaluator(ExternallyCachedEvaluationRule(product)),
+  EXPECT_EQ(evalOrNaN(evaluator, ExternallyCachedEvaluationRule(product)),
             expectedResult);
   EXPECT_EQ(*sum->cachedValue, 3.14159 + 2.71828);
   EXPECT_EQ(*product->cachedValue, expectedResult);
-  EXPECT_EQ(evaluator(ExternallyCachedEvaluationRule(product)),
+  EXPECT_EQ(evalOrNaN(evaluator, ExternallyCachedEvaluationRule(product)),
             expectedResult);
   EXPECT_EQ(*sum->cachedValue, 3.14159 + 2.71828);
   EXPECT_EQ(*product->cachedValue, expectedResult);
@@ -241,7 +244,13 @@ TEST(ArithmeticEvaluator, Simple) {
   // Dependency printing.
 
   // Cache some values first, so they show up in the result.
-  (void)evaluator(InternallyCachedEvaluationRule(product));
+  auto cachedResult = evaluator(InternallyCachedEvaluationRule(product));
+  if (auto err = cachedResult.takeError()) {
+    llvm::handleAllErrors(std::move(err),
+      [](const CyclicalRequestError<InternallyCachedEvaluationRule> &E) {
+        llvm_unreachable("no explicit cycles were requested");
+      });
+  }
 
   std::string productDependencies;
   {
@@ -335,36 +344,15 @@ TEST(ArithmeticEvaluator, Cycle) {
                                      arithmeticRequestFunctions);
 
   // Evaluate when there is a cycle.
-  UncachedEvaluationRule::brokeCycle = false;
-  EXPECT_TRUE(std::isnan(evaluator(UncachedEvaluationRule(product))));
-  EXPECT_TRUE(UncachedEvaluationRule::brokeCycle);
-
-  // Cycle-breaking result is cached.
-  EXPECT_TRUE(std::isnan(evaluator(UncachedEvaluationRule(product))));
-  UncachedEvaluationRule::brokeCycle = false;
-  EXPECT_FALSE(UncachedEvaluationRule::brokeCycle);
-
-  // Evaluate when there is a cycle.
-  evaluator.clearCache();
-  InternallyCachedEvaluationRule::brokeCycle = false;
-  EXPECT_TRUE(std::isnan(evaluator(InternallyCachedEvaluationRule(product))));
-  EXPECT_TRUE(InternallyCachedEvaluationRule::brokeCycle);
-
-  // Cycle-breaking result is cached.
-  InternallyCachedEvaluationRule::brokeCycle = false;
-  EXPECT_TRUE(std::isnan(evaluator(InternallyCachedEvaluationRule(product))));
-  EXPECT_FALSE(InternallyCachedEvaluationRule::brokeCycle);
-
-  // Evaluate when there is a cycle.
-  evaluator.clearCache();
-  ExternallyCachedEvaluationRule::brokeCycle = false;
-  EXPECT_TRUE(std::isnan(evaluator(ExternallyCachedEvaluationRule(product))));
-  EXPECT_TRUE(ExternallyCachedEvaluationRule::brokeCycle);
-
-  // Cycle-breaking result is cached.
-  ExternallyCachedEvaluationRule::brokeCycle = false;
-  EXPECT_TRUE(std::isnan(evaluator(ExternallyCachedEvaluationRule(product))));
-  EXPECT_FALSE(ExternallyCachedEvaluationRule::brokeCycle);
+  bool cycleDetected = false;
+  auto result = evaluator(UncachedEvaluationRule(sum));
+  if (auto err = result.takeError()) {
+    llvm::handleAllErrors(std::move(err),
+      [&](const CyclicalRequestError<UncachedEvaluationRule> &E) {
+        cycleDetected = true;
+      });
+  }
+  EXPECT_TRUE(cycleDetected);
 
   // Dependency printing.
   std::string productDependencies;
@@ -374,12 +362,8 @@ TEST(ArithmeticEvaluator, Cycle) {
   }
 
   EXPECT_EQ(productDependencies,
-    " `--InternallyCachedEvaluationRule(Binary: product)\n"
-    "     `--InternallyCachedEvaluationRule(Binary: sum)\n"
-    "     |   `--InternallyCachedEvaluationRule(Literal: 3.141590e+00)\n"
-    "     |   `--InternallyCachedEvaluationRule(Binary: product) "
-      "(cyclic dependency)\n"
-    "     `--InternallyCachedEvaluationRule(Literal: 4.200000e+01)\n");
+    " `--InternallyCachedEvaluationRule(Binary: product) "
+      "(dependency not evaluated)\n");
 
   // Cleanup
   delete pi;