Make path.readdir() watch the dirents by default

- Added a new parameter `watch` to `path.readdir()` that allows one to watch for changes in entries under a given directory.
  - only names are watched; 'entry types' are not. This somewhat matches the return value of `path.readdir()`, which only contains entry names
  - same restrictions as `rctx.watch()` in terms of which paths can be watched and which cannot; similarly for `mctx`.
- Made a big-ish refactor that eliminated the two `RepoRecordedInput.File` subclasses, and pulled the difference into a separate `RepoRecordedInput.RepoCacheFriendlyPath` instead. This new path class is used by the new `RepoRecordedInput.Dirents` as well.

Work towards #20952.

Closes #21341.

PiperOrigin-RevId: 607772207
Change-Id: Ibba2b3389acd23e0a703818fec2cd58321a9b896

src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelLockFileValue.java

@@ -37,7 +37,7 @@
 @GenerateTypeAdapter
 public abstract class BazelLockFileValue implements SkyValue, Postable {
 
-  public static final int LOCK_FILE_VERSION = 5;
+  public static final int LOCK_FILE_VERSION = 6;
 
   @SerializationConstant public static final SkyKey KEY = () -> SkyFunctions.BAZEL_LOCK_FILE;
 

src/main/java/com/google/devtools/build/lib/bazel/bzlmod/GsonTypeAdapterUtil.java

@@ -459,6 +459,22 @@ public RepoRecordedInput.File read(JsonReader jsonReader) throws IOException {
         }
       };
 
+  private static final TypeAdapter<RepoRecordedInput.Dirents>
+      REPO_RECORDED_INPUT_DIRENTS_TYPE_ADAPTER =
+          new TypeAdapter<>() {
+            @Override
+            public void write(JsonWriter jsonWriter, RepoRecordedInput.Dirents value)
+                throws IOException {
+              jsonWriter.value(value.toStringInternal());
+            }
+
+            @Override
+            public RepoRecordedInput.Dirents read(JsonReader jsonReader) throws IOException {
+              return (RepoRecordedInput.Dirents)
+                  RepoRecordedInput.Dirents.PARSER.parse(jsonReader.nextString());
+            }
+          };
+
   public static Gson createLockFileGson(Path moduleFilePath, Path workspaceRoot) {
     return new GsonBuilder()
         .setPrettyPrinting()
@@ -484,6 +500,8 @@ public static Gson createLockFileGson(Path moduleFilePath, Path workspaceRoot) {
         .registerTypeAdapter(AttributeValues.class, new AttributeValuesAdapter())
         .registerTypeAdapter(byte[].class, BYTE_ARRAY_TYPE_ADAPTER)
         .registerTypeAdapter(RepoRecordedInput.File.class, REPO_RECORDED_INPUT_FILE_TYPE_ADAPTER)
+        .registerTypeAdapter(
+            RepoRecordedInput.Dirents.class, REPO_RECORDED_INPUT_DIRENTS_TYPE_ADAPTER)
         .create();
   }
 

src/main/java/com/google/devtools/build/lib/bazel/bzlmod/LockFileModuleExtension.java

@@ -43,6 +43,8 @@ public static Builder builder() {
 
   public abstract ImmutableMap<RepoRecordedInput.File, String> getRecordedFileInputs();
 
+  public abstract ImmutableMap<RepoRecordedInput.Dirents, String> getRecordedDirentsInputs();
+
   public abstract ImmutableMap<String, String> getEnvVariables();
 
   public abstract ImmutableMap<String, RepoSpec> getGeneratedRepoSpecs();
@@ -68,6 +70,9 @@ public abstract static class Builder {
     public abstract Builder setRecordedFileInputs(
         ImmutableMap<RepoRecordedInput.File, String> value);
 
+    public abstract Builder setRecordedDirentsInputs(
+        ImmutableMap<RepoRecordedInput.Dirents, String> value);
+
     public abstract Builder setEnvVariables(ImmutableMap<String, String> value);
 
     public abstract Builder setGeneratedRepoSpecs(ImmutableMap<String, RepoSpec> value);

src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionContext.java

@@ -72,7 +72,7 @@ protected ModuleExtensionContext(
         processWrapper,
         starlarkSemantics,
         remoteExecutor,
-        /* allowWatchingFilesOutsideWorkspace= */ false);
+        /* allowWatchingPathsOutsideWorkspace= */ false);
     this.extensionId = extensionId;
     this.modules = modules;
     this.rootModuleHasNonDevDependency = rootModuleHasNonDevDependency;

src/main/java/com/google/devtools/build/lib/bazel/bzlmod/SingleExtensionEvalFunction.java

@@ -218,6 +218,7 @@ public SkyValue compute(SkyKey skyKey, Environment env)
                   LockFileModuleExtension.builder()
                       .setBzlTransitiveDigest(extension.getBzlTransitiveDigest())
                       .setRecordedFileInputs(moduleExtensionResult.getRecordedFileInputs())
+                      .setRecordedDirentsInputs(moduleExtensionResult.getRecordedDirentsInputs())
                       .setEnvVariables(extension.getEnvVars())
                       .setGeneratedRepoSpecs(generatedRepoSpecs)
                       .setModuleExtensionMetadata(moduleExtensionMetadata)
@@ -289,10 +290,16 @@ private SingleExtensionEvalValue tryGettingValueFromLockFile(
                 + extensionId
                 + "' have changed");
       }
-      if (didFilesChange(env, directories, lockedExtension.getRecordedFileInputs())) {
+      if (didRecordedInputsChange(env, directories, lockedExtension.getRecordedFileInputs())) {
         diffRecorder.record(
             "One or more files the extension '" + extensionId + "' is using have changed");
       }
+      if (didRecordedInputsChange(env, directories, lockedExtension.getRecordedDirentsInputs())) {
+        diffRecorder.record(
+            "One or more directory listings watched by the extension '"
+                + extensionId
+                + "' have changed");
+      }
     } catch (DiffFoundEarlyExitException ignored) {
       // ignored
     }
@@ -390,12 +397,12 @@ private static boolean didRepoMappingsChange(
     return false;
   }
 
-  private static boolean didFilesChange(
+  private static boolean didRecordedInputsChange(
       Environment env,
       BlazeDirectories directories,
-      ImmutableMap<RepoRecordedInput.File, String> recordedFileInputs)
+      ImmutableMap<? extends RepoRecordedInput, String> recordedInputs)
       throws InterruptedException, NeedsSkyframeRestartException {
-    boolean upToDate = RepoRecordedInput.areAllValuesUpToDate(env, directories, recordedFileInputs);
+    boolean upToDate = RepoRecordedInput.areAllValuesUpToDate(env, directories, recordedInputs);
     if (env.valuesMissing()) {
       throw new NeedsSkyframeRestartException();
     }
@@ -762,6 +769,7 @@ public RunModuleExtensionResult run(
         generatedRepoSpecs.put(name, repoSpec);
       }
       return RunModuleExtensionResult.create(
+          ImmutableMap.of(),
           ImmutableMap.of(),
           generatedRepoSpecs.buildOrThrow(),
           Optional.of(ModuleExtensionMetadata.REPRODUCIBLE),
@@ -931,6 +939,7 @@ public RunModuleExtensionResult run(
       }
       return RunModuleExtensionResult.create(
           moduleContext.getRecordedFileInputs(),
+          moduleContext.getRecordedDirentsInputs(),
           threadContext.getGeneratedRepoSpecs(),
           moduleExtensionMetadata,
           repoMappingRecorder.recordedEntries());
@@ -993,6 +1002,8 @@ abstract static class RunModuleExtensionResult {
 
     abstract ImmutableMap<RepoRecordedInput.File, String> getRecordedFileInputs();
 
+    abstract ImmutableMap<RepoRecordedInput.Dirents, String> getRecordedDirentsInputs();
+
     abstract ImmutableMap<String, RepoSpec> getGeneratedRepoSpecs();
 
     abstract Optional<ModuleExtensionMetadata> getModuleExtensionMetadata();
@@ -1001,11 +1012,13 @@ abstract static class RunModuleExtensionResult {
 
     static RunModuleExtensionResult create(
         ImmutableMap<RepoRecordedInput.File, String> recordedFileInputs,
+        ImmutableMap<RepoRecordedInput.Dirents, String> recordedDirentsInputs,
         ImmutableMap<String, RepoSpec> generatedRepoSpecs,
         Optional<ModuleExtensionMetadata> moduleExtensionMetadata,
         ImmutableTable<RepositoryName, String, RepositoryName> recordedRepoMappingEntries) {
       return new AutoValue_SingleExtensionEvalFunction_RunModuleExtensionResult(
           recordedFileInputs,
+          recordedDirentsInputs,
           generatedRepoSpecs,
           moduleExtensionMetadata,
           recordedRepoMappingEntries);

src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/BUILD

@@ -42,6 +42,7 @@ java_library(
         "//src/main/java/com/google/devtools/build/lib/rules:repository/workspace_file_helper",
         "//src/main/java/com/google/devtools/build/lib/shell",
         "//src/main/java/com/google/devtools/build/lib/skyframe:action_environment_function",
+        "//src/main/java/com/google/devtools/build/lib/skyframe:directory_listing_value",
         "//src/main/java/com/google/devtools/build/lib/skyframe:ignored_package_prefixes_value",
         "//src/main/java/com/google/devtools/build/lib/skyframe:precomputed_value",
         "//src/main/java/com/google/devtools/build/lib/starlarkbuildapi/repository",

src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkBaseExternalContext.java

@@ -51,21 +51,20 @@
 import com.google.devtools.build.lib.profiler.SilentCloseable;
 import com.google.devtools.build.lib.rules.repository.NeedsSkyframeRestartException;
 import com.google.devtools.build.lib.rules.repository.RepoRecordedInput;
-import com.google.devtools.build.lib.rules.repository.RepoRecordedInput.FileInsideWorkspace;
-import com.google.devtools.build.lib.rules.repository.RepoRecordedInput.FileOutsideWorkspace;
+import com.google.devtools.build.lib.rules.repository.RepoRecordedInput.Dirents;
+import com.google.devtools.build.lib.rules.repository.RepoRecordedInput.RepoCacheFriendlyPath;
 import com.google.devtools.build.lib.rules.repository.RepositoryFunction;
 import com.google.devtools.build.lib.rules.repository.RepositoryFunction.RepositoryFunctionException;
 import com.google.devtools.build.lib.runtime.ProcessWrapper;
 import com.google.devtools.build.lib.runtime.RepositoryRemoteExecutor;
 import com.google.devtools.build.lib.runtime.RepositoryRemoteExecutor.ExecutionResult;
 import com.google.devtools.build.lib.skyframe.ActionEnvironmentFunction;
+import com.google.devtools.build.lib.skyframe.DirectoryListingValue;
 import com.google.devtools.build.lib.util.OsUtils;
-import com.google.devtools.build.lib.util.Pair;
 import com.google.devtools.build.lib.util.io.OutErr;
 import com.google.devtools.build.lib.vfs.FileSystemUtils;
 import com.google.devtools.build.lib.vfs.Path;
 import com.google.devtools.build.lib.vfs.PathFragment;
-import com.google.devtools.build.lib.vfs.Root;
 import com.google.devtools.build.lib.vfs.RootedPath;
 import com.google.devtools.build.lib.vfs.Symlinks;
 import com.google.devtools.build.skyframe.SkyFunction.Environment;
@@ -149,10 +148,11 @@ private interface AsyncTask {
   @Nullable private final ProcessWrapper processWrapper;
   protected final StarlarkSemantics starlarkSemantics;
   private final HashMap<RepoRecordedInput.File, String> recordedFileInputs = new HashMap<>();
+  private final HashMap<RepoRecordedInput.Dirents, String> recordedDirentsInputs = new HashMap<>();
   private final HashSet<String> accumulatedEnvKeys = new HashSet<>();
   private final RepositoryRemoteExecutor remoteExecutor;
   private final List<AsyncTask> asyncTasks;
-  private final boolean allowWatchingFilesOutsideWorkspace;
+  private final boolean allowWatchingPathsOutsideWorkspace;
 
   protected StarlarkBaseExternalContext(
       Path workingDirectory,
@@ -164,7 +164,7 @@ protected StarlarkBaseExternalContext(
       @Nullable ProcessWrapper processWrapper,
       StarlarkSemantics starlarkSemantics,
       @Nullable RepositoryRemoteExecutor remoteExecutor,
-      boolean allowWatchingFilesOutsideWorkspace) {
+      boolean allowWatchingPathsOutsideWorkspace) {
     this.workingDirectory = workingDirectory;
     this.directories = directories;
     this.env = env;
@@ -176,7 +176,7 @@ protected StarlarkBaseExternalContext(
     this.starlarkSemantics = starlarkSemantics;
     this.remoteExecutor = remoteExecutor;
     this.asyncTasks = new ArrayList<>();
-    this.allowWatchingFilesOutsideWorkspace = allowWatchingFilesOutsideWorkspace;
+    this.allowWatchingPathsOutsideWorkspace = allowWatchingPathsOutsideWorkspace;
   }
 
   public boolean ensureNoPendingAsyncTasks(EventHandler eventHandler, boolean forSuccessfulFetch) {
@@ -211,6 +211,10 @@ public ImmutableMap<RepoRecordedInput.File, String> getRecordedFileInputs() {
     return ImmutableMap.copyOf(recordedFileInputs);
   }
 
+  public ImmutableMap<Dirents, String> getRecordedDirentsInputs() {
+    return ImmutableMap.copyOf(recordedDirentsInputs);
+  }
+
   /** Returns set of environment variable keys encountered so far. */
   public ImmutableSet<String> getAccumulatedEnvKeys() {
     return ImmutableSet.copyOf(accumulatedEnvKeys);
@@ -1140,7 +1144,7 @@ public StarlarkPath path(Object path) throws EvalException, InterruptedException
   protected StarlarkPath getPath(String method, Object path)
       throws EvalException, InterruptedException {
     if (path instanceof String) {
-      return new StarlarkPath(workingDirectory.getRelative(path.toString()));
+      return new StarlarkPath(this, workingDirectory.getRelative(path.toString()));
     } else if (path instanceof Label) {
       return getPathFromLabel((Label) path);
     } else if (path instanceof StarlarkPath) {
@@ -1195,13 +1199,30 @@ public String readFile(Object path, String watch, StarlarkThread thread)
     }
   }
 
-  protected Pair<RepoRecordedInput.File, RootedPath> toRootedPath(Path path) {
+  /**
+   * Converts a regular {@link Path} to a {@link RepoCacheFriendlyPath} based on {@link
+   * ShouldWatch}. If the path shouldn't be watched for whatever reason, returns null. If it's
+   * illegal to watch the path in the current context, but the user still requested a watch, throws
+   * an exception.
+   */
+  @Nullable
+  protected RepoCacheFriendlyPath toRepoCacheFriendlyPath(Path path, ShouldWatch shouldWatch)
+      throws EvalException {
+    if (shouldWatch == ShouldWatch.NO) {
+      return null;
+    }
+    if (path.startsWith(workingDirectory)) {
+      // The path is under the working directory. Don't watch it, as it would cause a dependency
+      // cycle.
+      if (shouldWatch == ShouldWatch.AUTO) {
+        return null;
+      }
+      throw Starlark.errorf("attempted to watch path under working directory");
+    }
     if (path.startsWith(directories.getWorkspace())) {
       // The file is under the workspace root.
       PathFragment relPath = path.relativeTo(directories.getWorkspace());
-      return Pair.of(
-          new FileInsideWorkspace(RepositoryName.MAIN, relPath),
-          RootedPath.toRootedPath(Root.fromPath(directories.getWorkspace()), relPath));
+      return RepoCacheFriendlyPath.createInsideWorkspace(RepositoryName.MAIN, relPath);
     }
     Path outputBaseExternal =
         directories.getOutputBase().getRelative(LabelConstants.EXTERNAL_REPOSITORY_LOCATION);
@@ -1212,16 +1233,19 @@ protected Pair<RepoRecordedInput.File, RootedPath> toRootedPath(Path path) {
         String repoName = relPath.getSegment(0);
         PathFragment repoRelPath =
             relPath.relativeTo(PathFragment.createAlreadyNormalized(repoName));
-        return Pair.of(
-            new FileInsideWorkspace(RepositoryName.createUnvalidated(repoName), repoRelPath),
-            RootedPath.toRootedPath(
-                Root.fromPath(outputBaseExternal.getChild(repoName)), repoRelPath));
+        return RepoCacheFriendlyPath.createInsideWorkspace(
+            RepositoryName.createUnvalidated(repoName), repoRelPath);
       }
     }
     // The file is just under a random absolute path.
-    return Pair.of(
-        new FileOutsideWorkspace(path.asFragment()),
-        RootedPath.toRootedPath(Root.absoluteRoot(path.getFileSystem()), path));
+    if (!allowWatchingPathsOutsideWorkspace) {
+      if (shouldWatch == ShouldWatch.AUTO) {
+        return null;
+      }
+      throw Starlark.errorf(
+          "attempted to watch path outside workspace, but it's prohibited in the current context");
+    }
+    return RepoCacheFriendlyPath.createOutsideWorkspace(path.asFragment());
   }
 
   /** Whether to watch a path. See {@link #readFile} for semantics */
@@ -1247,34 +1271,47 @@ static ShouldWatch fromString(String s) throws EvalException {
 
   protected void maybeWatch(StarlarkPath starlarkPath, ShouldWatch shouldWatch)
       throws EvalException, RepositoryFunctionException, InterruptedException {
-    if (shouldWatch == ShouldWatch.NO) {
+    RepoCacheFriendlyPath repoCacheFriendlyPath =
+        toRepoCacheFriendlyPath(starlarkPath.getPath(), shouldWatch);
+    if (repoCacheFriendlyPath == null) {
       return;
     }
-    Path path = starlarkPath.getPath();
-    Pair<RepoRecordedInput.File, RootedPath> pair = toRootedPath(path);
-    if (path.startsWith(workingDirectory)) {
-      // The file is under the working directory. Don't watch it, as it would cause a dependency
-      // cycle.
-      if (shouldWatch == ShouldWatch.AUTO) {
-        return;
-      }
-      throw Starlark.errorf("attempted to watch file under working directory");
-    }
-    if (!allowWatchingFilesOutsideWorkspace && pair.first instanceof FileOutsideWorkspace) {
-      if (shouldWatch == ShouldWatch.AUTO) {
-        return;
-      }
-      throw Starlark.errorf(
-          "attempted to watch file outside workspace, but it's prohibited in the current context");
+    RootedPath rootedPath = repoCacheFriendlyPath.getRootedPath(env, directories);
+    if (rootedPath == null) {
+      throw new NeedsSkyframeRestartException();
     }
     try {
       FileValue fileValue =
-          (FileValue) env.getValueOrThrow(FileValue.key(pair.second), IOException.class);
+          (FileValue) env.getValueOrThrow(FileValue.key(rootedPath), IOException.class);
       if (fileValue == null) {
         throw new NeedsSkyframeRestartException();
       }
 
-      recordedFileInputs.put(pair.first, RepoRecordedInput.File.fileValueToMarkerValue(fileValue));
+      recordedFileInputs.put(
+          new RepoRecordedInput.File(repoCacheFriendlyPath),
+          RepoRecordedInput.File.fileValueToMarkerValue(fileValue));
+    } catch (IOException e) {
+      throw new RepositoryFunctionException(e, Transience.TRANSIENT);
+    }
+  }
+
+  protected void maybeWatchDirents(Path path, ShouldWatch shouldWatch)
+      throws EvalException, RepositoryFunctionException, InterruptedException {
+    RepoCacheFriendlyPath repoCacheFriendlyPath = toRepoCacheFriendlyPath(path, shouldWatch);
+    if (repoCacheFriendlyPath == null) {
+      return;
+    }
+    RootedPath rootedPath = repoCacheFriendlyPath.getRootedPath(env, directories);
+    if (env.valuesMissing()) {
+      throw new NeedsSkyframeRestartException();
+    }
+    if (env.getValue(DirectoryListingValue.key(rootedPath)) == null) {
+      throw new NeedsSkyframeRestartException();
+    }
+    try {
+      recordedDirentsInputs.put(
+          new RepoRecordedInput.Dirents(repoCacheFriendlyPath),
+          RepoRecordedInput.Dirents.getDirentsMarkerValue(path));
     } catch (IOException e) {
       throw new RepositoryFunctionException(e, Transience.TRANSIENT);
     }
@@ -1290,12 +1327,11 @@ protected void maybeWatch(StarlarkPath starlarkPath, ShouldWatch shouldWatch)
               + "(if the path is a file); if the path was a file but is now a directory, or vice "
               + "versa; and if the path starts or stops existing. Notably, this does <em>not</em> "
               + "include changes to any files under the directory if the path is a directory. For "
-              // TODO: add `watch_dir()`
-              + "that, use <a href=\"#watch_dir\"><code>watch_dir()</code></a> instead.<p>Note "
-              + "that attempting to watch paths inside the repo currently being fetched, or inside "
-              + "the working directory of the current module extension, will result in an error. A "
-              + "module extension attempting to watch a path outside the current Bazel workspace "
-              + "will also result in an error.",
+              + "that, use <a href=\"path.html#readdir\"><code>path.readdir()</code></a> "
+              + "instead.<p>Note that attempting to watch paths inside the repo currently being "
+              + "fetched, or inside the working directory of the current module extension, will "
+              + "result in an error. A module extension attempting to watch a path outside the "
+              + "current Bazel workspace will also result in an error.",
       parameters = {
         @Param(
             name = "path",
@@ -1658,7 +1694,7 @@ private StarlarkPath findCommandOnPath(String program) throws IOException {
         // root?).
         Path path = workingDirectory.getFileSystem().getPath(fragment).getChild(program.trim());
         if (path.exists() && path.isFile(Symlinks.FOLLOW) && path.isExecutable()) {
-          return new StarlarkPath(path);
+          return new StarlarkPath(this, path);
         }
       }
     }
@@ -1668,7 +1704,7 @@ private StarlarkPath findCommandOnPath(String program) throws IOException {
   // Resolve the label given by value into a file path.
   protected StarlarkPath getPathFromLabel(Label label) throws EvalException, InterruptedException {
     RootedPath rootedPath = RepositoryFunction.getRootedPathFromLabel(label, env);
-    StarlarkPath starlarkPath = new StarlarkPath(rootedPath.asPath());
+    StarlarkPath starlarkPath = new StarlarkPath(this, rootedPath.asPath());
     try {
       maybeWatch(starlarkPath, ShouldWatch.AUTO);
     } catch (RepositoryFunctionException e) {