Merge pull request swiftlang#18454 from harlanhaskins/requests-ive-had-a-few

Remove SimpleRequest::breakCycle

Author: Harlan

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 @@ class AccessLevelRequest :
   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 @@ class SetterAccessLevelRequest :
   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 @@ class DefaultAndMaxAccessLevelRequest :
   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,47 @@ class PrettyStackTraceRequest : public llvm::PrettyStackTraceEntry {
   }
 };
 
+/// 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;
+  const Evaluator &evaluator;
+
+  CyclicalRequestError(const Request &request, const Evaluator &evaluator)
+    : request(request), evaluator(evaluator) {}
+
+  virtual void log(llvm::raw_ostream &out) const;
+
+  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 +143,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 +252,13 @@ class Evaluator {
   /// 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, *this));
+    }
 
     // Make sure we remove this from the set of active requests once we're
     // done.
@@ -232,9 +276,10 @@ class Evaluator {
   /// 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 +305,8 @@ class Evaluator {
   /// 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 +320,15 @@ class Evaluator {
   /// 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,16 +346,20 @@ class Evaluator {
   /// 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;
 
     // 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;
@@ -318,7 +370,9 @@ class Evaluator {
   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) {
+    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 +381,11 @@ class Evaluator {
 
     // 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;
   }
 
@@ -371,6 +427,13 @@ class Evaluator {
     "Only meant for use in the debugger");
 };
 
+template <typename Request>
+void CyclicalRequestError<Request>::log(llvm::raw_ostream &out) const {
+  out << "Cycle detected:\n";
+  evaluator.printDependencies(request, out);
+  out << "\n";
+}
+
 } // end namespace evaluator
 
 #endif // SWIFT_AST_EVALUATOR_H

include/swift/AST/NameLookupRequests.h

@@ -81,7 +81,6 @@ class InheritedDeclsReferencedRequest :
   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 @@ class UnderlyingTypeDeclsReferencedRequest :
   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 @@ class SuperclassDeclRequest :
   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 @@ class ExtendedNominalRequest :
   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 @@ class ExtendedNominalRequest :
   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 @@ class SimpleRequest {
     : 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...>());
   }