Allow generating SkyKeys for directory ancestors of files in order to…

… better process non-existent files. Also include FILE and DIRECTORY_LISTING nodes as well as FILE_STATE and DIRECTORY_LISTING_STATE.

In particular, for any given file and every ancestor directory which doesn't currently exist, include a FILE and FILE_STATE marker for the directory and include DIRECTORY_LISTING and DIRECTORY_LISTING_STATE for the existing directories that will potentially contain the new subdirectories.

PiperOrigin-RevId: 426420133

src/main/java/com/google/devtools/build/lib/query2/BUILD

@@ -87,6 +87,8 @@ java_library(
         "//src/main/java/com/google/devtools/build/lib/skyframe:configured_target_key",
         "//src/main/java/com/google/devtools/build/lib/skyframe:containing_package_lookup_function",
         "//src/main/java/com/google/devtools/build/lib/skyframe:detailed_exceptions",
+        "//src/main/java/com/google/devtools/build/lib/skyframe:directory_listing_state_value",
+        "//src/main/java/com/google/devtools/build/lib/skyframe:directory_listing_value",
         "//src/main/java/com/google/devtools/build/lib/skyframe:graph_backed_recursive_package_provider",
         "//src/main/java/com/google/devtools/build/lib/skyframe:ignored_package_prefixes_value",
         "//src/main/java/com/google/devtools/build/lib/skyframe:package_identifier_batching_callback",

src/main/java/com/google/devtools/build/lib/query2/RBuildFilesVisitor.java

@@ -15,13 +15,15 @@
 
 import static com.google.common.collect.ImmutableSet.toImmutableSet;
 
+import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Preconditions;
 import com.google.common.collect.ArrayListMultimap;
 import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Iterables;
 import com.google.common.collect.ListMultimap;
 import com.google.common.collect.Multimap;
 import com.google.devtools.build.lib.actions.FileStateValue;
+import com.google.devtools.build.lib.actions.FileValue;
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.cmdline.LabelConstants;
 import com.google.devtools.build.lib.cmdline.PackageIdentifier;
@@ -34,6 +36,8 @@
 import com.google.devtools.build.lib.query2.engine.Uniquifier;
 import com.google.devtools.build.lib.rules.repository.WorkspaceFileHelper;
 import com.google.devtools.build.lib.skyframe.ContainingPackageLookupFunction;
+import com.google.devtools.build.lib.skyframe.DirectoryListingStateValue;
+import com.google.devtools.build.lib.skyframe.DirectoryListingValue;
 import com.google.devtools.build.lib.skyframe.PackageLookupValue;
 import com.google.devtools.build.lib.skyframe.PackageValue;
 import com.google.devtools.build.lib.skyframe.SkyFunctions;
@@ -46,14 +50,20 @@
 import com.google.devtools.build.skyframe.WalkableGraph;
 import java.util.Collection;
 import java.util.HashSet;
+import java.util.List;
 import java.util.Map;
 import java.util.Set;
+import java.util.stream.Collectors;
 import javax.annotation.Nullable;
 
-/** A helper class that computes 'rbuildfiles(<blah>)' via BFS. */
+/**
+ * A helper class that visits packages of affected files by BFS and is represented by the
+ * queryfunction 'rbuildfiles'. Other query functions may also use functionality provided by this
+ * visitor.
+ */
 public class RBuildFilesVisitor extends ParallelQueryVisitor<SkyKey, PackageIdentifier, Target> {
 
-  // Each target in the full output of 'rbuildfiles' corresponds to BUILD file InputFile of a
+  // Each target in the full output of this visitor corresponds to BUILD file InputFile of a
   // unique package. So the processResultsBatchSize we choose to pass to the ParallelVisitor ctor
   // influences how many packages each leaf task doing processPartialResults will have to
   // deal with at once. A value of 100 was chosen experimentally.
@@ -160,7 +170,15 @@ protected Iterable<SkyKey> noteAndReturnUniqueVisitationKeys(
   public void visitFileIdentifiersAndWaitForCompletion(
       WalkableGraph graph, Iterable<PathFragment> fileKeys)
       throws QueryException, InterruptedException {
-    visitAndWaitForCompletion(getSkyKeysForFileFragments(graph, fileKeys));
+    visitAndWaitForCompletion(
+        getSkyKeysForFileFragments(graph, fileKeys, /*includeAncestorKeys=*/ false));
+  }
+
+  public void visitFileAndDirectoryKeysAndWaitForCompletion(
+      WalkableGraph graph, Iterable<PathFragment> fileKeys)
+      throws QueryException, InterruptedException {
+    visitAndWaitForCompletion(
+        getSkyKeysForFileFragments(graph, fileKeys, /*includeAncestorKeys=*/ true));
   }
 
   /**
@@ -179,9 +197,18 @@ public void visitFileIdentifiersAndWaitForCompletion(
    *
    * <p>Note also that we assume there'll be a PackageLookupValue node for some ancestor directory
    * of every file to which a symlink could possibly point otherwise the file will not be included.
+   *
+   * <p>If includeAncestorKeys is true, we will include a directory listing state of the first
+   * ancestor directory that exists and file states for non-existent ancestors.
    */
-  private static Collection<SkyKey> getSkyKeysForFileFragments(
-      WalkableGraph graph, Iterable<PathFragment> pathFragments) throws InterruptedException {
+  @VisibleForTesting
+  static Set<SkyKey> getSkyKeysForFileFragments(
+      WalkableGraph graph, Iterable<PathFragment> pathFragments, boolean includeAncestorKeys)
+      throws InterruptedException {
+    if (Iterables.isEmpty(pathFragments)) {
+      return ImmutableSet.of();
+    }
+
     Set<SkyKey> result = new HashSet<>();
     ListMultimap<PathFragment, PathFragment> currentAncestorToOriginalPath =
         ArrayListMultimap.create();
@@ -193,6 +220,9 @@ private static Collection<SkyKey> getSkyKeysForFileFragments(
       }
     }
 
+    // Used to find directories that have been added in the diff.
+    Set<RootedPath> pathsToCheckForNewlyAddedDirectories = new HashSet<>();
+
     // We look at each ancestor directory of every file, and use the currentAncestorToOriginalPath
     // map to avoid doing unnecessary work with common ancestors. If we don't find a package
     // with the first level of ancestors, we go up a level, until we find the first package
@@ -209,19 +239,38 @@ private static Collection<SkyKey> getSkyKeysForFileFragments(
           SkyKey packageLookupKey = entry.getKey();
           PathFragment currentFragment =
               ((PackageIdentifier) packageLookupKey.argument()).getPackageFragment();
-          Collection<PathFragment> originalFiles =
-              currentAncestorToOriginalPath.get(currentFragment);
+          List<PathFragment> originalFiles = currentAncestorToOriginalPath.get(currentFragment);
           Preconditions.checkState(!originalFiles.isEmpty(), entry);
           for (PathFragment fileName : originalFiles) {
-            result.add(
-                FileStateValue.key(
-                    RootedPath.toRootedPath(packageLookupValue.getRoot(), fileName)));
+            RootedPath rootedPath = RootedPath.toRootedPath(packageLookupValue.getRoot(), fileName);
+            result.add(FileStateValue.key(rootedPath));
+            // Include the File key too in case the FileState is never considered due to a
+            // missing parent directory.
+            result.add(FileValue.key(rootedPath));
+
+            if (includeAncestorKeys) {
+              RootedPath parentPath = rootedPath.getParentDirectory();
+              result.add(DirectoryListingStateValue.key(parentPath));
+              // Include the DirectoryListing key too in case the DirectoryListingState is never
+              // considered due to a missing parent directory.
+              result.add(DirectoryListingValue.key(parentPath));
+              for (PathFragment pathToCheckIfNewlyAdded = fileName;
+                  pathToCheckIfNewlyAdded.getPathString().length()
+                      > currentFragment.getPathString().length();
+                  pathToCheckIfNewlyAdded = pathToCheckIfNewlyAdded.getParentDirectory()) {
+                pathsToCheckForNewlyAddedDirectories.add(
+                    RootedPath.toRootedPath(packageLookupValue.getRoot(), pathToCheckIfNewlyAdded));
+              }
+            }
           }
           currentAncestorToOriginalPath.removeAll(currentFragment);
         }
       }
       currentAncestorToOriginalPath = goUpOneDirectory(currentAncestorToOriginalPath);
     }
+    if (includeAncestorKeys) {
+      includeAncestorKeysInResult(graph, result, pathsToCheckForNewlyAddedDirectories);
+    }
     return result;
   }
 
@@ -252,6 +301,45 @@ private static void checkWorkspaceFile(Set<SkyKey> result, PathFragment file) {
     }
   }
 
+  private static void includeAncestorKeysInResult(
+      WalkableGraph graph, Set<SkyKey> result, Set<RootedPath> pathsToCheckForNewlyAddedDirectories)
+      throws InterruptedException {
+    // Do a single batch fetch of all FileState's corresponding to directories with
+    // failed package lookups.
+    Set<SkyKey> fileStateKeysToFetch =
+        pathsToCheckForNewlyAddedDirectories.stream()
+            .map(FileStateValue::key)
+            .collect(Collectors.toSet());
+    Map<SkyKey, SkyValue> fileStateValues = graph.getSuccessfulValues(fileStateKeysToFetch);
+
+    for (SkyKey fileStateKey : fileStateKeysToFetch) {
+      if (fileStateValues.containsKey(fileStateKey)) {
+        FileStateValue fsv = (FileStateValue) fileStateValues.get(fileStateKey);
+        if (!fsv.getType().exists() && !fsv.getType().isDirectory()) {
+          processFileStateKeyForMissingDirectory(result, (FileStateValue.Key) fileStateKey);
+        }
+      } else {
+        processFileStateKeyForMissingDirectory(result, (FileStateValue.Key) fileStateKey);
+      }
+    }
+  }
+
+  private static void processFileStateKeyForMissingDirectory(
+      Set<SkyKey> result, FileStateValue.Key key) {
+    RootedPath rootedPath = key.argument();
+    result.add(key);
+    result.add(FileValue.key(rootedPath));
+    // Add a DirectoryListingState node to our traversal even if the ancestor path too didn't exist
+    // prior to the diff. This will have no effect on the results if the ancestor directory was also
+    // newly created doesn't exist but has the consequence that the first ancestor path that did
+    // exist prior to the diff will be correctly marked as having a changed directory listing state.
+    RootedPath parentPath = rootedPath.getParentDirectory();
+    if (parentPath != null) {
+      result.add(DirectoryListingStateValue.key(parentPath));
+      result.add(DirectoryListingValue.key(parentPath));
+    }
+  }
+
   /**
    * Returns package lookup key for looking up the package root for which there may be a relevant
    * {@link FileStateValue} node in the graph for {@code originalFileFragment}, which is assumed to

src/main/java/com/google/devtools/build/lib/skyframe/DirectoryListingStateValue.java

@@ -64,9 +64,10 @@ public static Key key(RootedPath rootedPath) {
     return Key.create(rootedPath);
   }
 
+  /** Key type for DirectoryListingStateValue. */
   @AutoCodec.VisibleForSerialization
   @AutoCodec
-  static class Key extends AbstractSkyKey<RootedPath> {
+  public static class Key extends AbstractSkyKey<RootedPath> {
     private static final Interner<Key> interner = BlazeInterners.newWeakInterner();
 
     private Key(RootedPath arg) {

src/main/java/com/google/devtools/build/lib/skyframe/DirectoryListingValue.java

@@ -62,9 +62,10 @@ public static Key key(RootedPath directoryUnderRoot) {
     return Key.create(directoryUnderRoot);
   }
 
+  /** Key type for DirectoryListingValue. */
   @AutoCodec.VisibleForSerialization
   @AutoCodec
-  static class Key extends AbstractSkyKey<RootedPath> {
+  public static class Key extends AbstractSkyKey<RootedPath> {
     private static final Interner<Key> interner = BlazeInterners.newWeakInterner();
 
     private Key(RootedPath arg) {

src/test/java/com/google/devtools/build/lib/query2/BUILD

@@ -15,6 +15,31 @@ filegroup(
     visibility = ["//src:__subpackages__"],
 )
 
+java_test(
+    name = "RBuildFilesVisitorTest",
+    srcs = ["RBuildFilesVisitorTest.java"],
+    deps = [
+        "//src/main/java/com/google/devtools/build/lib/actions:file_metadata",
+        "//src/main/java/com/google/devtools/build/lib/clock",
+        "//src/main/java/com/google/devtools/build/lib/cmdline",
+        "//src/main/java/com/google/devtools/build/lib/packages",
+        "//src/main/java/com/google/devtools/build/lib/query2",
+        "//src/main/java/com/google/devtools/build/lib/skyframe:directory_listing_state_value",
+        "//src/main/java/com/google/devtools/build/lib/skyframe:directory_listing_value",
+        "//src/main/java/com/google/devtools/build/lib/skyframe:package_lookup_value",
+        "//src/main/java/com/google/devtools/build/lib/skyframe:sky_functions",
+        "//src/main/java/com/google/devtools/build/lib/vfs",
+        "//src/main/java/com/google/devtools/build/lib/vfs:pathfragment",
+        "//src/main/java/com/google/devtools/build/lib/vfs/inmemoryfs",
+        "//src/main/java/com/google/devtools/build/skyframe",
+        "//src/main/java/com/google/devtools/build/skyframe:skyframe-objects",
+        "//third_party:guava",
+        "//third_party:junit4",
+        "//third_party:mockito",
+        "//third_party:truth",
+    ],
+)
+
 test_suite(
     name = "AllTests",
     tests = [