[7.1.0] Clarify the meaning of Dirent.Type.UNKNOWN. (#21434)

Also add some tests to verify that all filesystem implementations follow
the readdir spec. (RemoteActionFileSystemTest needs its own because it
doesn't inherit from FileSystemTest. UnixFileSystemTest needs its own
because it's the only implementation supporting special files.)

PiperOrigin-RevId: 608529008
Change-Id: I73dbbe8f218779b76d8879f4d4f5dfb228835157

src/main/java/com/google/devtools/build/lib/remote/RemoteActionFileSystem.java

@@ -895,9 +895,6 @@ private Dirent maybeFollowSymlinkForDirent(
     }
     PathFragment path = dirPath.getChild(entry.getName());
     FileStatus st = statNullable(path, /* followSymlinks= */ true);
-    if (st == null) {
-      return new Dirent(entry.getName(), Dirent.Type.UNKNOWN);
-    }
     return new Dirent(entry.getName(), direntFromStat(st));
   }
 

src/main/java/com/google/devtools/build/lib/unix/NativePosixFiles.java

@@ -255,15 +255,23 @@ private static Type forChar(char c) {
 
     /** The names of the entries in a directory. */
     private final String[] names;
+
     /**
-     * An optional (nullable) array of entry types, corresponding positionally
-     * to the "names" field.  The types are:
-     *   'd': a subdirectory
-     *   'f': a regular file
-     *   's': a symlink (only returned with {@code NOFOLLOW})
-     *   '?': anything else
-     * Note that unlike libc, this implementation of readdir() follows
-     * symlinks when determining these types.
+     * An optional (nullable) array of entry types, corresponding positionally to the "names" field.
+     * The possible types are:
+     *
+     * <ul>
+     *   <li>'d': a subdirectory
+     *   <li>'f': a regular file
+     *   <li>'s': a symlink, only returned for {@link ReadTypes.NOFOLLOW}
+     *   <li>'?': anything else, including:
+     *       <ul>
+     *         <li>a special file
+     *         <li>a nonexistent symlink target
+     *         <li>an error occurred while determining the file type, for example because of a
+     *             symlink loop
+     *       </ul>
+     * </ul>
      *
      * <p>This is intentionally a byte array rather than a array of enums to save memory.
      */

src/main/java/com/google/devtools/build/lib/vfs/Dirent.java

@@ -26,7 +26,8 @@ public enum Type {
     DIRECTORY,
     // A symlink.
     SYMLINK,
-    // Not one of the above. For example, a special file.
+    // None of the above.
+    // For example, a special file, or a path that could not be resolved while following symlinks.
     UNKNOWN;
   }
 

src/main/java/com/google/devtools/build/lib/vfs/FileSystem.java

@@ -618,7 +618,7 @@ public boolean exists(PathFragment path) {
    */
   protected abstract Collection<String> getDirectoryEntries(PathFragment path) throws IOException;
 
-  protected static Dirent.Type direntFromStat(FileStatus stat) {
+  protected static Dirent.Type direntFromStat(@Nullable FileStatus stat) {
     if (stat == null) {
       return Dirent.Type.UNKNOWN;
     } else if (stat.isSpecialFile()) {

src/test/java/com/google/devtools/build/lib/remote/RemoteActionFileSystemTest.java

@@ -675,17 +675,15 @@ public void readdir_fromRemoteOutputTree() throws Exception {
   @Test
   public void readdir_fromLocalFilesystem() throws Exception {
     RemoteActionFileSystem actionFs = (RemoteActionFileSystem) createActionFileSystem();
-    writeLocalFile(actionFs, getOutputPath("dir/out1"), "contents1");
-    writeLocalFile(actionFs, getOutputPath("dir/out2"), "contents2");
-    writeLocalFile(actionFs, getOutputPath("dir/subdir/out3"), "contents3");
+    writeLocalFile(actionFs, getOutputPath("dir/file"), "contents");
+    writeLocalFile(actionFs, getOutputPath("dir/subdir/file"), "contents");
     PathFragment dirPath = getOutputPath("dir");
 
     assertReaddir(
         actionFs,
         dirPath,
         /* followSymlinks= */ true,
-        new Dirent("out1", Dirent.Type.FILE),
-        new Dirent("out2", Dirent.Type.FILE),
+        new Dirent("file", Dirent.Type.FILE),
         new Dirent("subdir", Dirent.Type.DIRECTORY));
 
     assertReaddirThrows(actionFs, getOutputPath("dir/out1"), /* followSymlinks= */ true);
@@ -792,27 +790,49 @@ public void readdir_followSymlinks_forDirectoryEntries(
     FileSystem toFs = to.getFilesystem(actionFs);
 
     PathFragment dirPath = getOutputPath("dir");
-    PathFragment linkPath = getOutputPath("dir/sym");
-    PathFragment targetPath = getOutputPath("target");
+    PathFragment fileLinkPath = getOutputPath("dir/file_sym");
+    PathFragment fileTargetPath = getOutputPath("file_target");
+    PathFragment dirLinkPath = getOutputPath("dir/dir_sym");
+    PathFragment dirTargetPath = getOutputPath("dir_target");
+    PathFragment loopingLinkPath = getOutputPath("dir/looping_sym");
+    PathFragment danglingLinkPath = getOutputPath("dir/dangling_sym");
 
     fromFs.getPath(dirPath).createDirectory();
-    fromFs.getPath(linkPath).createSymbolicLink(execRoot.getRelative(targetPath).asFragment());
-
-    assertReaddir(
-        actionFs, dirPath, /* followSymlinks= */ false, new Dirent("sym", Dirent.Type.SYMLINK));
-    assertReaddir(
-        actionFs, dirPath, /* followSymlinks= */ true, new Dirent("sym", Dirent.Type.UNKNOWN));
+    fromFs
+        .getPath(fileLinkPath)
+        .createSymbolicLink(execRoot.getRelative(fileTargetPath).asFragment());
+    fromFs
+        .getPath(dirLinkPath)
+        .createSymbolicLink(execRoot.getRelative(dirTargetPath).asFragment());
+    fromFs
+        .getPath(loopingLinkPath)
+        .createSymbolicLink(execRoot.getRelative(loopingLinkPath).asFragment());
+    fromFs.getPath(danglingLinkPath).createSymbolicLink(PathFragment.create("/does_not_exist"));
 
     if (toFs.equals(actionFs.getLocalFileSystem())) {
-      writeLocalFile(actionFs, targetPath, "content");
+      writeLocalFile(actionFs, fileTargetPath, "content");
+      actionFs.getLocalFileSystem().getPath(dirTargetPath).createDirectoryAndParents();
     } else {
-      injectRemoteFile(actionFs, targetPath, "content");
+      injectRemoteFile(actionFs, fileTargetPath, "content");
+      actionFs.getRemoteOutputTree().createDirectoryAndParents(dirTargetPath);
     }
 
     assertReaddir(
-        actionFs, dirPath, /* followSymlinks= */ false, new Dirent("sym", Dirent.Type.SYMLINK));
+        actionFs,
+        dirPath,
+        /* followSymlinks= */ false,
+        new Dirent("file_sym", Dirent.Type.SYMLINK),
+        new Dirent("dir_sym", Dirent.Type.SYMLINK),
+        new Dirent("looping_sym", Dirent.Type.SYMLINK),
+        new Dirent("dangling_sym", Dirent.Type.SYMLINK));
     assertReaddir(
-        actionFs, dirPath, /* followSymlinks= */ true, new Dirent("sym", Dirent.Type.FILE));
+        actionFs,
+        dirPath,
+        /* followSymlinks= */ true,
+        new Dirent("file_sym", Dirent.Type.FILE),
+        new Dirent("dir_sym", Dirent.Type.DIRECTORY),
+        new Dirent("looping_sym", Dirent.Type.UNKNOWN),
+        new Dirent("dangling_sym", Dirent.Type.UNKNOWN));
   }
 
   @Test
@@ -841,12 +861,10 @@ private void assertReaddir(
       boolean followSymlinks,
       Dirent... expected)
       throws Exception {
-    assertThat(actionFs.readdir(dirPath, followSymlinks))
-        .containsExactlyElementsIn(expected)
-        .inOrder();
+    assertThat(actionFs.readdir(dirPath, followSymlinks)).containsExactlyElementsIn(expected);
     assertThat(actionFs.getDirectoryEntries(dirPath))
-        .containsExactlyElementsIn(stream(expected).map(Dirent::getName).collect(toImmutableList()))
-        .inOrder();
+        .containsExactlyElementsIn(
+            stream(expected).map(Dirent::getName).collect(toImmutableList()));
   }
 
   private void assertReaddirThrows(

src/test/java/com/google/devtools/build/lib/unix/UnixFileSystemTest.java

@@ -17,6 +17,7 @@
 import static org.junit.Assert.assertThrows;
 
 import com.google.devtools.build.lib.vfs.DigestHashFunction;
+import com.google.devtools.build.lib.vfs.Dirent;
 import com.google.devtools.build.lib.vfs.FileSystem;
 import com.google.devtools.build.lib.vfs.FileSystemUtils;
 import com.google.devtools.build.lib.vfs.Path;
@@ -61,6 +62,7 @@ public void testIsSpecialFile() throws Exception {
     Path fifo = absolutize("fifo");
     FileSystemUtils.createEmptyFile(regular);
     NativePosixFiles.mkfifo(fifo.toString(), 0777);
+
     assertThat(regular.isFile()).isTrue();
     assertThat(regular.isSpecialFile()).isFalse();
     assertThat(regular.stat().isFile()).isTrue();
@@ -70,4 +72,24 @@ public void testIsSpecialFile() throws Exception {
     assertThat(fifo.stat().isFile()).isTrue();
     assertThat(fifo.stat().isSpecialFile()).isTrue();
   }
+
+  @Test
+  public void testReaddirSpecialFile() throws Exception {
+    Path dir = absolutize("readdir");
+    Path symlink = dir.getChild("symlink");
+    Path fifo = dir.getChild("fifo");
+    dir.createDirectoryAndParents();
+    symlink.createSymbolicLink(fifo.asFragment());
+    NativePosixFiles.mkfifo(fifo.toString(), 0777);
+
+    assertThat(dir.getDirectoryEntries()).containsExactly(symlink, fifo);
+
+    assertThat(dir.readdir(Symlinks.NOFOLLOW))
+        .containsExactly(
+            new Dirent("symlink", Dirent.Type.SYMLINK), new Dirent("fifo", Dirent.Type.UNKNOWN));
+
+    assertThat(dir.readdir(Symlinks.FOLLOW))
+        .containsExactly(
+            new Dirent("symlink", Dirent.Type.UNKNOWN), new Dirent("fifo", Dirent.Type.UNKNOWN));
+  }
 }

src/test/java/com/google/devtools/build/lib/vfs/FileSystemTest.java

@@ -20,6 +20,7 @@
 import static java.nio.charset.StandardCharsets.UTF_8;
 import static org.junit.Assert.assertThrows;
 import static org.junit.Assert.fail;
+import static org.junit.Assume.assumeTrue;
 
 import com.google.common.base.Preconditions;
 import com.google.common.collect.ImmutableList;
@@ -1768,29 +1769,68 @@ public void testStatNullableFailsFastOnNonExistingFiles() throws Exception {
 
   @Test
   public void testResolveSymlinks() throws Exception {
-    if (testFS.supportsSymbolicLinksNatively(xLink.asFragment())) {
-      createSymbolicLink(xLink, xFile);
-      FileSystemUtils.createEmptyFile(xFile);
-      assertThat(testFS.resolveOneLink(xLink.asFragment())).isEqualTo(xFile.asFragment());
-      assertThat(xLink.resolveSymbolicLinks()).isEqualTo(xFile);
-    }
+    assumeTrue(testFS.supportsSymbolicLinksNatively(xLink.asFragment()));
+
+    createSymbolicLink(xLink, xFile);
+    FileSystemUtils.createEmptyFile(xFile);
+    assertThat(testFS.resolveOneLink(xLink.asFragment())).isEqualTo(xFile.asFragment());
+    assertThat(xLink.resolveSymbolicLinks()).isEqualTo(xFile);
   }
 
   @Test
   public void testResolveDanglingSymlinks() throws Exception {
-    if (testFS.supportsSymbolicLinksNatively(xLink.asFragment())) {
-      createSymbolicLink(xLink, xNothing);
-      assertThat(testFS.resolveOneLink(xLink.asFragment())).isEqualTo(xNothing.asFragment());
-      assertThrows(IOException.class, () -> xLink.resolveSymbolicLinks());
-    }
+    assumeTrue(testFS.supportsSymbolicLinksNatively(xLink.asFragment()));
+
+    createSymbolicLink(xLink, xNothing);
+    assertThat(testFS.resolveOneLink(xLink.asFragment())).isEqualTo(xNothing.asFragment());
+    assertThrows(IOException.class, () -> xLink.resolveSymbolicLinks());
   }
 
   @Test
   public void testResolveNonSymlinks() throws Exception {
-    if (testFS.supportsSymbolicLinksNatively(xFile.asFragment())) {
-      assertThat(testFS.resolveOneLink(xFile.asFragment())).isNull();
-      assertThat(xFile.resolveSymbolicLinks()).isEqualTo(xFile);
-    }
+    assertThat(testFS.resolveOneLink(xFile.asFragment())).isNull();
+    assertThat(xFile.resolveSymbolicLinks()).isEqualTo(xFile);
+  }
+
+  @Test
+  public void testReaddir() throws Exception {
+    Path dir = workingDir.getChild("readdir");
+
+    assumeTrue(testFS.supportsSymbolicLinksNatively(dir.asFragment()));
+
+    dir.getChild("dir").createDirectoryAndParents();
+    FileSystemUtils.createEmptyFile(dir.getChild("file"));
+    dir.getChild("file_link").createSymbolicLink(dir.getChild("file"));
+    dir.getChild("dir_link").createSymbolicLink(dir.getChild("dir"));
+    dir.getChild("looping_link").createSymbolicLink(dir.getChild("looping_link"));
+    dir.getChild("dangling_link").createSymbolicLink(testFS.getPath("/does_not_exist"));
+
+    assertThat(dir.getDirectoryEntries())
+        .containsExactly(
+            dir.getChild("file"),
+            dir.getChild("dir"),
+            dir.getChild("file_link"),
+            dir.getChild("dir_link"),
+            dir.getChild("looping_link"),
+            dir.getChild("dangling_link"));
+
+    assertThat(dir.readdir(Symlinks.NOFOLLOW))
+        .containsExactly(
+            new Dirent("file", Dirent.Type.FILE),
+            new Dirent("dir", Dirent.Type.DIRECTORY),
+            new Dirent("file_link", Dirent.Type.SYMLINK),
+            new Dirent("dir_link", Dirent.Type.SYMLINK),
+            new Dirent("looping_link", Dirent.Type.SYMLINK),
+            new Dirent("dangling_link", Dirent.Type.SYMLINK));
+
+    assertThat(dir.readdir(Symlinks.FOLLOW))
+        .containsExactly(
+            new Dirent("file", Dirent.Type.FILE),
+            new Dirent("dir", Dirent.Type.DIRECTORY),
+            new Dirent("file_link", Dirent.Type.FILE),
+            new Dirent("dir_link", Dirent.Type.DIRECTORY),
+            new Dirent("looping_link", Dirent.Type.UNKNOWN),
+            new Dirent("dangling_link", Dirent.Type.UNKNOWN));
   }
 
   @Test