Perform The Dependency Unioning Step During Replay

In order for private dependencies to be completely correct, it must perform the name lookup unioning step when a cached request is replayed - not just when lookups are first performed. In order to reduce the overhead of this union operation, it is not necessary to walk the entire active request stack, just walk to the nearest cached request in the stack and union into that. When it is popped, its replay step will itself union into the next cached request.

To see why, consider a request graph:

A* -> B -> C*
         |
          -> D*

where A, C, and D are cached.

If a caller were to force C and D, then force A independenty, today we would *only* replay the names looked up by C and D the first time A was evaluated. That is, subsequent evaluations of A do not replay the correct set of names. If we were to perform the union step during replay as well, requests that force A would also see C and D’s lookups.

Without this, callers that force requests like the DeclChecker have to be wary of the way they force the interface type request so other files see the right name sets.

rdar://64008262

Author: CodaFi

include/swift/AST/Evaluator.h

@@ -439,7 +439,7 @@ class Evaluator {
                                   !Request::isDependencySink>::type * = nullptr>
   void reportEvaluatedResult(const Request &r,
                              const typename Request::OutputType &o) {
-    recorder.replay(ActiveRequest(r));
+    recorder.replay(activeRequests, ActiveRequest(r));
   }
 
   // Report the result of evaluating a request that is a dependency sink.

include/swift/AST/EvaluatorDependencies.h

@@ -267,10 +267,16 @@ struct DependencyRecorder {
   void realize(const DependencyCollector::Reference &ref);
 
 public:
-  void replay(const swift::ActiveRequest &req);
+  void replay(const llvm::SetVector<swift::ActiveRequest> &stack,
+              const swift::ActiveRequest &req);
   void record(const llvm::SetVector<swift::ActiveRequest> &stack,
               llvm::function_ref<void(DependencyCollector &)> rec);
 
+private:
+  void
+  unionNearestCachedRequest(ArrayRef<swift::ActiveRequest> stack,
+                            const DependencyCollector::ReferenceSet &scratch);
+
 public:
   using ReferenceEnumerator =
       llvm::function_ref<void(const DependencyCollector::Reference &)>;

lib/AST/Evaluator.cpp

@@ -446,23 +446,12 @@ void evaluator::DependencyRecorder::record(
     return;
   }
 
-  assert(mode != Mode::StatusQuo);
-  for (const auto &request : stack) {
-    if (!request.isCached()) {
-      continue;
-    }
-
-    auto entry = requestReferences.find_as(request);
-    if (entry == requestReferences.end()) {
-      requestReferences.insert({AnyRequest(request), collector.scratch});
-      continue;
-    }
-
-    entry->second.insert(collector.scratch.begin(), collector.scratch.end());
-  }
+  return unionNearestCachedRequest(stack.getArrayRef(), collector.scratch);
 }
 
-void evaluator::DependencyRecorder::replay(const swift::ActiveRequest &req) {
+void evaluator::DependencyRecorder::replay(
+    const llvm::SetVector<swift::ActiveRequest> &stack,
+    const swift::ActiveRequest &req) {
   assert(!isRecording && "Probably not a good idea to allow nested recording");
 
   auto *source = getActiveDependencySourceOrNull();
@@ -482,6 +471,51 @@ void evaluator::DependencyRecorder::replay(const swift::ActiveRequest &req) {
   for (const auto &ref : entry->second) {
     realize(ref);
   }
+
+  // N.B. This is a particularly subtle detail of the replay unioning step. The
+  // evaluator does not push cached requests onto the active request stack,
+  // so it is possible (and, in fact, quite likely) we'll wind up with an
+  // empty request stack. The remaining troublesome case is when we have a
+  // cached request being run through the uncached path - take the
+  // InterfaceTypeRequest, which involves many component requests, most of which
+  // are themselves cached. In such a case, the active stack will look like
+  //
+  // -> TypeCheckSourceFileRequest
+  // -> ...
+  // -> InterfaceTypeRequest
+  // -> ...
+  // -> UnderlyingTypeRequest
+  //
+  // We want the UnderlyingTypeRequest to union its names into the
+  // InterfaceTypeRequest, and if we were to just start searching the active
+  // stack backwards for a cached request we would find...
+  // the UnderlyingTypeRequest! So, we'll just drop it from consideration.
+  //
+  // We do *not* have to consider this during the recording step because none
+  // of the name lookup requests (or any dependency sinks in general) are
+  // cached. Should this change in the future, we will need to sink this logic
+  // into the union step itself.
+  const size_t d = (!stack.empty() && req == stack.back()) ? 1 : 0;
+  return unionNearestCachedRequest(stack.getArrayRef().drop_back(d),
+                                   entry->second);
+}
+
+void evaluator::DependencyRecorder::unionNearestCachedRequest(
+    ArrayRef<swift::ActiveRequest> stack,
+    const DependencyCollector::ReferenceSet &scratch) {
+  assert(mode != Mode::StatusQuo);
+  auto nearest = std::find_if(stack.rbegin(), stack.rend(),
+                              [](const auto &req){ return req.isCached(); });
+  if (nearest == stack.rend()) {
+    return;
+  }
+
+  auto entry = requestReferences.find_as(*nearest);
+  if (entry == requestReferences.end()) {
+    requestReferences.insert({AnyRequest(*nearest), scratch});
+  } else {
+    entry->second.insert(scratch.begin(), scratch.end());
+  }
 }
 
 using namespace swift;

test/Incremental/Verifier/multi-file/Inputs/UsesInner.swift

@@ -2,7 +2,7 @@
 // rely on the interface hash changing to cause this file to be rebuilt, which
 // will *not* work with type fingerprints enabled.
 // See rdar://63984581
-// fixme-expected-provides{{Inner}}
+// fixme-provides{{Inner}}
 
 // expected-provides{{defaultFoo}}
 // expected-provides{{blah}}