[Request-evaluator] Improve debuggability

include/swift/AST/Decl.h

@@ -520,10 +520,10 @@ class alignas(1 << DeclAlignInBits) Decl {
     KnownProtocol : 8, // '8' for speed. This only needs 6.
 
     /// The number of requirements in the requirement signature.
-		NumRequirementsInSignature : 16,
+    NumRequirementsInSignature : 16,
 
-		/// Whether we are currently computing inherited protocols.
-		ComputingInheritedProtocols : 1
+    /// Whether we are currently computing inherited protocols.
+    ComputingInheritedProtocols : 1
   );
 
   SWIFT_INLINE_BITFIELD(ClassDecl, NominalTypeDecl, 1+2+1+2+1+3+1+1,

include/swift/AST/Evaluator.h

@@ -20,10 +20,12 @@
 
 #include "swift/AST/AnyRequest.h"
 #include "swift/Basic/AnyValue.h"
+#include "swift/Basic/CycleDiagnosticKind.h"
 #include "swift/Basic/Defer.h"
 #include "llvm/ADT/DenseMap.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/SetVector.h"
+#include "llvm/Support/PrettyStackTrace.h"
 #include <string>
 #include <tuple>
 #include <type_traits>
@@ -40,6 +42,21 @@ using llvm::None;
 
 class DiagnosticEngine;
 
+/// Pretty stack trace handler for an arbitrary request.
+template<typename Request>
+class PrettyStackTraceRequest : public llvm::PrettyStackTraceEntry {
+  const Request &request;
+
+public:
+  PrettyStackTraceRequest(const Request &request) : request(request) { }
+
+  void print(llvm::raw_ostream &out) const {
+    out << "While evaluating request ";
+    simple_display(out, request);
+    out << "\n";
+  }
+};
+
 /// Evaluation engine that evaluates and caches "requests", checking for cyclic
 /// dependencies along the way.
 ///
@@ -108,7 +125,7 @@ class Evaluator {
   DiagnosticEngine &diags;
 
   /// Whether to diagnose cycles or ignore them completely.
-  bool shouldDiagnoseCycles;
+  CycleDiagnosticKind shouldDiagnoseCycles;
 
   /// A vector containing all of the active evaluation requests, which
   /// is treated as a stack and is used to detect cycles.
@@ -131,7 +148,7 @@ class Evaluator {
 public:
   /// Construct a new evaluator that can emit cyclic-dependency
   /// diagnostics through the given diagnostics engine.
-  Evaluator(DiagnosticEngine &diags, bool shouldDiagnoseCycles);
+  Evaluator(DiagnosticEngine &diags, CycleDiagnosticKind shouldDiagnoseCycles);
 
   /// Evaluate the given request and produce its result,
   /// consulting/populating the cache as required.
@@ -210,6 +227,7 @@ class Evaluator {
     // them now anyway.
     dependencies[request].clear();
 
+    PrettyStackTraceRequest<Request> prettyStackTrace(request);
     return request(*this);
   }
 
@@ -270,7 +288,9 @@ class Evaluator {
   /// the other overload.
   void printDependencies(const AnyRequest &request,
                          llvm::raw_ostream &out,
-                         llvm::DenseSet<AnyRequest> &visited,
+                         llvm::DenseSet<AnyRequest> &visitedAnywhere,
+                         llvm::SmallVectorImpl<AnyRequest> &visitedAlongPath,
+                         llvm::ArrayRef<AnyRequest> highlightPath,
                          std::string &prefixStr,
                          bool lastChild) const;
 

include/swift/Basic/CycleDiagnosticKind.h

@@ -0,0 +1,34 @@
+//===--- CycleDiagnosticKind.h - Cycle Diagnostic Kind ----------*- C++ -*-===//
+//
+// This source file is part of the Swift.org open source project
+//
+// Copyright (c) 2014 - 2017 Apple Inc. and the Swift project authors
+// Licensed under Apache License v2.0 with Runtime Library Exception
+//
+// See https://swift.org/LICENSE.txt for license information
+// See https://swift.org/CONTRIBUTORS.txt for the list of Swift project authors
+//
+//===----------------------------------------------------------------------===//
+//
+// This file defines the CycleDiagnosticKind enum.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef CycleDiagnosticKind_h
+#define CycleDiagnosticKind_h
+
+namespace swift {
+
+/// How to diagnose cycles when they are encountered during evaluation.
+enum class CycleDiagnosticKind {
+  /// Don't diagnose cycles at all.
+  NoDiagnose,
+  /// Diagnose cycles as full-fledged errors.
+  FullDiagnose,
+  /// Diagnose cycles via debugging dumps.
+  DebugDiagnose,
+};
+
+} // end namespace swift
+
+#endif /* CycleDiagnosticKind_h */

include/swift/Basic/LangOptions.h

@@ -19,6 +19,7 @@
 #define SWIFT_BASIC_LANGOPTIONS_H
 
 #include "swift/Config.h"
+#include "swift/Basic/CycleDiagnosticKind.h"
 #include "swift/Basic/LLVM.h"
 #include "swift/Basic/Version.h"
 #include "clang/Basic/VersionTuple.h"
@@ -168,6 +169,10 @@ namespace swift {
     /// This is for testing purposes.
     std::string DebugForbidTypecheckPrefix;
 
+    /// \brief How to diagnose cycles encountered
+    CycleDiagnosticKind EvaluatorCycleDiagnostics =
+        CycleDiagnosticKind::NoDiagnose;
+
     /// \brief The upper bound, in bytes, of temporary data that can be
     /// allocated by the constraint solver.
     unsigned SolverMemoryThreshold = 512 * 1024 * 1024;

include/swift/Option/FrontendOptions.td

@@ -167,6 +167,9 @@ def debug_forbid_typecheck_prefix : Separate<["-"], "debug-forbid-typecheck-pref
   HelpText<"Triggers llvm fatal_error if typechecker tries to typecheck a decl "
            "with the provided prefix name">;
 
+def debug_cycles : Flag<["-"], "debug-cycles">,
+  HelpText<"Print out debug dumps when cycles are detected in evaluation">;
+
 def debug_time_compilation : Flag<["-"], "debug-time-compilation">,
   HelpText<"Prints the time taken by each compilation phase">;
 def debug_time_function_bodies : Flag<["-"], "debug-time-function-bodies">,

lib/AST/ASTContext.cpp

@@ -463,7 +463,7 @@ ASTContext::ASTContext(LangOptions &langOpts, SearchPathOptions &SearchPathOpts,
     SearchPathOpts(SearchPathOpts),
     SourceMgr(SourceMgr),
     Diags(Diags),
-    evaluator(Diags, /*shouldDiagnoseCycles=*/false),
+    evaluator(Diags, langOpts.EvaluatorCycleDiagnostics),
     TheBuiltinModule(createBuiltinModule(*this)),
     StdlibModuleName(getIdentifier(STDLIB_NAME)),
     SwiftShimsModuleName(getIdentifier(SWIFT_SHIMS_NAME)),

lib/AST/Decl.cpp

@@ -3170,7 +3170,7 @@ ProtocolDecl::ProtocolDecl(DeclContext *DC, SourceLoc ProtocolLoc,
   Bits.ProtocolDecl.NumRequirementsInSignature = 0;
   Bits.ProtocolDecl.HasMissingRequirements = false;
   Bits.ProtocolDecl.KnownProtocol = 0;
-	Bits.ProtocolDecl.ComputingInheritedProtocols = false;
+  Bits.ProtocolDecl.ComputingInheritedProtocols = false;
 }
 
 llvm::TinyPtrVector<ProtocolDecl *>
@@ -3181,10 +3181,10 @@ ProtocolDecl::getInheritedProtocols() const {
   // We shouldn't need this, but it shows up in recursive invocations.
   if (!isRequirementSignatureComputed()) {
     SmallPtrSet<ProtocolDecl *, 4> known;
-		if (Bits.ProtocolDecl.ComputingInheritedProtocols) return result;
+    if (Bits.ProtocolDecl.ComputingInheritedProtocols) return result;
 
-		auto *self = const_cast<ProtocolDecl *>(this);
-		self->Bits.ProtocolDecl.ComputingInheritedProtocols = true;
+    auto *self = const_cast<ProtocolDecl *>(this);
+    self->Bits.ProtocolDecl.ComputingInheritedProtocols = true;
     for (unsigned index : indices(getInherited())) {
       if (auto type = getInheritedType(index)) {
         // Only protocols can appear in the inheritance clause
@@ -3200,7 +3200,7 @@ ProtocolDecl::getInheritedProtocols() const {
         }
       }
     }
-		self->Bits.ProtocolDecl.ComputingInheritedProtocols = false;
+    self->Bits.ProtocolDecl.ComputingInheritedProtocols = false;
     return result;
   }
 

lib/AST/Evaluator.cpp

@@ -32,7 +32,8 @@ std::string AnyRequest::getAsString() const {
   return result;
 }
 
-Evaluator::Evaluator(DiagnosticEngine &diags, bool shouldDiagnoseCycles)
+Evaluator::Evaluator(DiagnosticEngine &diags,
+                     CycleDiagnosticKind shouldDiagnoseCycles)
   : diags(diags), shouldDiagnoseCycles(shouldDiagnoseCycles) { }
 
 bool Evaluator::checkDependency(const AnyRequest &request) {
@@ -45,8 +46,26 @@ bool Evaluator::checkDependency(const AnyRequest &request) {
     return false;
   }
 
-  if (shouldDiagnoseCycles)
+  // Diagnose cycle.
+  switch (shouldDiagnoseCycles) {
+  case CycleDiagnosticKind::NoDiagnose:
+    return true;
+
+  case CycleDiagnosticKind::DebugDiagnose: {
+    llvm::errs() << "===CYCLE DETECTED===\n";
+    llvm::DenseSet<AnyRequest> visitedAnywhere;
+    llvm::SmallVector<AnyRequest, 4> visitedAlongPath;
+    std::string prefixStr;
+    printDependencies(activeRequests.front(), llvm::errs(), visitedAnywhere,
+                      visitedAlongPath, activeRequests.getArrayRef(),
+                      prefixStr, /*lastChild=*/true);
+    return true;
+  }
+
+  case CycleDiagnosticKind::FullDiagnose:
     diagnoseCycle(request);
+    return true;
+  }
 
   return true;
 }
@@ -62,27 +81,52 @@ void Evaluator::diagnoseCycle(const AnyRequest &request) {
   llvm_unreachable("Diagnosed a cycle but it wasn't represented in the stack");
 }
 
-void Evaluator::printDependencies(const AnyRequest &request,
-                                  llvm::raw_ostream &out,
-                                  llvm::DenseSet<AnyRequest> &visited,
-                                  std::string &prefixStr,
-                                  bool lastChild) const {
+void Evaluator::printDependencies(
+                            const AnyRequest &request,
+                            llvm::raw_ostream &out,
+                            llvm::DenseSet<AnyRequest> &visitedAnywhere,
+                            llvm::SmallVectorImpl<AnyRequest> &visitedAlongPath,
+                            llvm::ArrayRef<AnyRequest> highlightPath,
+                            std::string &prefixStr,
+                            bool lastChild) const {
   out << prefixStr << " `--";
 
+  // Determine whether this node should be highlighted.
+  bool isHighlighted = false;
+  if (std::find(highlightPath.begin(), highlightPath.end(), request)
+        != highlightPath.end()) {
+    isHighlighted = true;
+    out.changeColor(llvm::buffer_ostream::Colors::GREEN);
+  }
+
   // Print this node.
   simple_display(out, request);
 
+  // Turn off the highlight.
+  if (isHighlighted) {
+    out.resetColor();
+  }
+
   // Print the cached value, if known.
   auto cachedValue = cache.find(request);
   if (cachedValue != cache.end()) {
     out << " -> ";
     PrintEscapedString(cachedValue->second.getAsString(), out);
   }
 
-  if (!visited.insert(request).second) {
-    // We've already seed this node, so we have a cyclic dependency.
-    out.changeColor(llvm::raw_ostream::RED);
-    out << " (cyclic dependency)\n";
+  if (!visitedAnywhere.insert(request).second) {
+    // We've already seed this node. Check whether it's part of a cycle.
+    if (std::find(visitedAlongPath.begin(), visitedAlongPath.end(), request)
+          != visitedAlongPath.end()) {
+      // We have a cyclic dependency.
+      out.changeColor(llvm::raw_ostream::RED);
+      out << " (cyclic dependency)\n";
+    } else {
+      // We have seen this node before, but it's not a cycle. Elide its
+      // children.
+      out << " (elided)\n";
+    }
+
     out.resetColor();
   } else if (dependencies.count(request) == 0) {
     // We have not seen this node before, so we don't know its dependencies.
@@ -91,7 +135,7 @@ void Evaluator::printDependencies(const AnyRequest &request,
     out.resetColor();
 
     // Remove from the visited set.
-    visited.erase(request);
+    visitedAnywhere.erase(request);
   } else {
     // Print children.
     out << "\n";
@@ -101,26 +145,33 @@ void Evaluator::printDependencies(const AnyRequest &request,
     prefixStr += (lastChild ? ' ' : '|');
     prefixStr += "  ";
 
+    // Note that this request is along the path.
+    visitedAlongPath.push_back(request);
+
     // Print the children.
     auto &dependsOn = dependencies.find(request)->second;
     for (unsigned i : indices(dependsOn)) {
-      printDependencies(dependsOn[i], out, visited, prefixStr,
-                        i == dependsOn.size()-1);
+      printDependencies(dependsOn[i], out, visitedAnywhere, visitedAlongPath,
+                        highlightPath, prefixStr, i == dependsOn.size()-1);
     }
 
     // Drop our changes to the prefix.
     prefixStr.erase(prefixStr.end() - 4, prefixStr.end());
 
-    // Remove from the visited set.
-    visited.erase(request);
+    // Remove from the visited set and path.
+    visitedAnywhere.erase(request);
+    assert(visitedAlongPath.back() == request);
+    visitedAlongPath.pop_back();
   }
 }
 
 void Evaluator::printDependencies(const AnyRequest &request,
                                   llvm::raw_ostream &out) const {
   std::string prefixStr;
-  llvm::DenseSet<AnyRequest> visited;
-  printDependencies(request, out, visited, prefixStr, /*lastChild=*/true);
+  llvm::DenseSet<AnyRequest> visitedAnywhere;
+  llvm::SmallVector<AnyRequest, 4> visitedAlongPath;
+  printDependencies(request, out, visitedAnywhere, visitedAlongPath, { },
+                    prefixStr, /*lastChild=*/true);
 }
 
 void Evaluator::dumpDependencies(const AnyRequest &request) const {

lib/Frontend/CompilerInvocation.cpp

@@ -253,6 +253,10 @@ static bool ParseLangArgs(LangOptions &Opts, ArgList &Args,
     Opts.DebugForbidTypecheckPrefix = A->getValue();
   }
 
+  if (Args.getLastArg(OPT_debug_cycles)) {
+    Opts.EvaluatorCycleDiagnostics = CycleDiagnosticKind::DebugDiagnose;
+  }
+
   if (const Arg *A = Args.getLastArg(OPT_solver_memory_threshold)) {
     unsigned threshold;
     if (StringRef(A->getValue()).getAsInteger(10, threshold)) {

test/decl/class/circular_inheritance.swift

@@ -1,4 +1,6 @@
 // RUN: %target-typecheck-verify-swift
+// RUN: not %target-swift-frontend -typecheck -debug-cycles %s 2> %t.cycles
+// RUN: %FileCheck %s < %t.cycles
 
 class C : B { } // expected-error{{circular class inheritance 'C' -> 'B' -> 'A' -> 'C'}}
 class B : A { } // expected-note{{class 'B' declared here}}
@@ -37,3 +39,10 @@ class Outer3
     : Outer3.Inner<Int> {
   class Inner<T> {}
 }
+
+// CHECK: ===CYCLE DETECTED===
+// CHECK-NEXT: `--{{.*}}SuperclassTypeRequest
+// CHECK-NEXT:      `--{{.*}}InheritedTypeRequest(circular_inheritance.(file).Left@
+// CHECK-NEXT:          `--{{.*}}SuperclassTypeRequest
+// CHECK-NEXT:              `--{{.*}}InheritedTypeRequest(circular_inheritance.(file).Right@
+// CHECK-NEXT:                  `--{{.*}}SuperclassTypeRequest{{.*(cyclic dependency)}}

unittests/AST/ArithmeticEvaluator.cpp

@@ -200,7 +200,7 @@ TEST(ArithmeticEvaluator, Simple) {
 
   SourceManager sourceMgr;
   DiagnosticEngine diags(sourceMgr);
-  Evaluator evaluator(diags, /*shouldDiagnoseCycles=*/true);
+  Evaluator evaluator(diags, CycleDiagnosticKind::FullDiagnose);
 
   const double expectedResult = (3.14159 + 2.71828) * 42.0;
   EXPECT_EQ(evaluator(InternallyCachedEvaluationRule(product)),
@@ -320,7 +320,7 @@ TEST(ArithmeticEvaluator, Cycle) {
 
   SourceManager sourceMgr;
   DiagnosticEngine diags(sourceMgr);
-  Evaluator evaluator(diags, /*shouldDiagnoseCycles=*/true);
+  Evaluator evaluator(diags, CycleDiagnosticKind::FullDiagnose);
 
   // Evaluate when there is a cycle.
   UncachedEvaluationRule::brokeCycle = false;