Added `Follow` parameter to llvm::vfs::FileSystem::status()

[alexfh]

Summary

Currently, status() calls always resolve symlinks. This patch adds a parameter
to control this behavior.

[sammccall]

How strong is the need for this?
This adds complexity to a widely implemented and used interface, and the combination of virtual + default parameters can be at least a little confusing.

[alexfh]

In D97288#2581908, @sammccall wrote:

How strong is the need for this?
This adds complexity to a widely implemented and used interface, and the combination of virtual + default parameters can be at least a little confusing.

That's a fair question. Indeed, this is needed quire rarely, and I just removed what would be a use case for from our internal code. Even Status::isSymlink is almost exclusively used in tests. The main motivation for the patch is to make the FileSystem::status() API more aligned with sys::fs::status(), and allow Status::isSymlink to fulfil its purpose for Status instances coming from FileSystem::status(). Also a way to avoid confusion in the future. Definitely not a practical need right now.

[dexonsmith]

If we do add this, I'd suggest a separate function rather than modifying the current one. I also think implementing it should be optional. Something like:

virtual ErrorOr<vfs::Status> status(const Twine &Path) = 0;
virtual ErrorOr<vfs::Status> status(const Twine &Path, bool FollowSymlinks) {
  // By default ignore the request not to follow symlinks.
  (void)FollowSymlinks;
  return status(Path);
}

or, maybe better / more clear with a distinct name:

virtual ErrorOr<vfs::Status> statusNoFollow(const Twine &Path) {
  // By default ignore the request not to follow symlinks.
  return status(Path);
}

Note that there's also a hole in FileSystem::dir_begin which also always follows symlinks; perhaps the parallel features could be implemented (or not) at the same time.

That said, I agree it'd be nice to have a use case for this before expanding the API.

clang-tools-extra/clangd/FS.cpp

openFileForRead(const llvm::Twine &Path) override {
       // preamble build, but it should be worth it as preamble can be reused
       // many times (e.g. code completion) and the repeated status call is
       // likely to be cached in the underlying file system anyway.
       if (auto S = File->get()->status())
         StatCache.update(getUnderlyingFS(), std::move(*S));
       return File;
     }
 
-    llvm::ErrorOr<llvm::vfs::Status> status(const llvm::Twine &Path) override {
-      auto S = getUnderlyingFS().status(Path);
+    llvm::ErrorOr<llvm::vfs::Status> status(const llvm::Twine &Path,
+                                            bool Follow = true) override {
+      auto S = getUnderlyingFS().status(Path, Follow);
       if (S)
         StatCache.update(getUnderlyingFS(), *S);
       return S;
     }
 
   private:
     PreambleFileStatusCache &StatCache;
   };
   return llvm::IntrusiveRefCntPtr<CollectFS>(
       new CollectFS(std::move(FS), *this));
 }
 
 llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem>
 PreambleFileStatusCache::getConsumingFS(
     llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> FS) const {
   class CacheVFS : public llvm::vfs::ProxyFileSystem {
   public:
     CacheVFS(llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> FS,
              const PreambleFileStatusCache &StatCache)
         : ProxyFileSystem(std::move(FS)), StatCache(StatCache) {}
 
-    llvm::ErrorOr<llvm::vfs::Status> status(const llvm::Twine &Path) override {
+    llvm::ErrorOr<llvm::vfs::Status> status(const llvm::Twine &Path,
+                                            bool Follow = true) override {
       if (auto S = StatCache.lookup(Path.str()))
         return *S;
-      return getUnderlyingFS().status(Path);
+      return getUnderlyingFS().status(Path, Follow);
     }
 
   private:
     const PreambleFileStatusCache &StatCache;
   };
   return llvm::IntrusiveRefCntPtr<CacheVFS>(new CacheVFS(std::move(FS), *this));
 }
 
 Path removeDots(PathRef File) {
   llvm::SmallString<128> CanonPath(File);
   llvm::sys::path::remove_dots(CanonPath, /*remove_dot_dot=*/true);
   return CanonPath.str().str();
 }
 
 } // namespace clangd
 } // namespace clang

clang-tools-extra/clangd/unittests/ClangdTests.cpp

IntrusiveRefCntPtr<llvm::vfs::FileSystem> viewImpl() const override {
                          llvm::StringMap<unsigned> &CountStats)
             : ProxyFileSystem(std::move(FS)), CountStats(CountStats) {}
 
         llvm::ErrorOr<std::unique_ptr<llvm::vfs::File>>
         openFileForRead(const Twine &Path) override {
           ++CountStats[llvm::sys::path::filename(Path.str())];
           return ProxyFileSystem::openFileForRead(Path);
         }
-        llvm::ErrorOr<llvm::vfs::Status> status(const Twine &Path) override {
+        llvm::ErrorOr<llvm::vfs::Status> status(const Twine &Path,
+                                                bool Follow = true) override {
           ++CountStats[llvm::sys::path::filename(Path.str())];
-          return ProxyFileSystem::status(Path);
+          return ProxyFileSystem::status(Path, Follow);
         }
 
       private:
         llvm::StringMap<unsigned> &CountStats;
       };
 
       return IntrusiveRefCntPtr<StatRecordingVFS>(
           new StatRecordingVFS(buildTestFS(Files), CountStats));

clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h

 public:
   DependencyScanningWorkerFilesystem(
       DependencyScanningFilesystemSharedCache &SharedCache,
       IntrusiveRefCntPtr<llvm::vfs::FileSystem> FS,
       ExcludedPreprocessorDirectiveSkipMapping *PPSkipMappings)
       : ProxyFileSystem(std::move(FS)), SharedCache(SharedCache),
         PPSkipMappings(PPSkipMappings) {}
 
-  llvm::ErrorOr<llvm::vfs::Status> status(const Twine &Path) override;
+  llvm::ErrorOr<llvm::vfs::Status> status(const Twine &Path,
+                                          bool Follow = true) override;
   llvm::ErrorOr<std::unique_ptr<llvm::vfs::File>>
   openFileForRead(const Twine &Path) override;
 
   /// The set of files that should not be minimized.
   llvm::StringSet<> IgnoredFiles;
 
 private:
   void setCachedEntry(StringRef Filename, const CachedFileSystemEntry *Entry) {

clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp

DependencyScanningWorkerFilesystem::getOrCreateFileSystemEntry(
   }
 
   // Store the result in the local cache.
   setCachedEntry(Filename, Result);
   return Result;
 }
 
 llvm::ErrorOr<llvm::vfs::Status>
-DependencyScanningWorkerFilesystem::status(const Twine &Path) {
+DependencyScanningWorkerFilesystem::status(const Twine &Path,
+                                           bool Follow /*= true*/) {
   SmallString<256> OwnedFilename;
   StringRef Filename = Path.toStringRef(OwnedFilename);
   const llvm::ErrorOr<const CachedFileSystemEntry *> Result =
       getOrCreateFileSystemEntry(Filename);
   if (!Result)
     return Result.getError();
   return (*Result)->getStatus();
 }

clang/unittests/Driver/DistroTest.cpp

 }
 
 TEST(DistroTest, DetectWindowsAndCrossCompile) {
 
   class CountingFileSystem : public llvm::vfs::ProxyFileSystem {
   public:
     CountingFileSystem() : ProxyFileSystem(llvm::vfs::getRealFileSystem()) {}
 
-    llvm::ErrorOr<llvm::vfs::Status> status(const llvm::Twine &Path) override {
+    llvm::ErrorOr<llvm::vfs::Status> status(const llvm::Twine &Path,
+                                            bool Follow = true) override {
       ++Count;
-      return llvm::vfs::ProxyFileSystem::status(Path);
+      return llvm::vfs::ProxyFileSystem::status(Path, Follow);
     }
 
     llvm::ErrorOr<std::unique_ptr<llvm::vfs::File>>
     openFileForRead(const llvm::Twine &Path) override {
       ++Count;
       return llvm::vfs::ProxyFileSystem::openFileForRead(Path);
     }
 

llvm/include/llvm/Support/VirtualFileSystem.h

 };
 
 /// The virtual file system interface.
 class FileSystem : public llvm::ThreadSafeRefCountedBase<FileSystem> {
 public:
   virtual ~FileSystem();
 
   /// Get the status of the entry at \p Path, if one exists.
-  virtual llvm::ErrorOr<Status> status(const Twine &Path) = 0;
+  ///
+  /// @param Path Input path.
+  /// @param Follow When true, follows symlinks.  Otherwise, the symlink itself
+  ///               is statted.
+  virtual llvm::ErrorOr<Status> status(const Twine &Path,
+                                       bool Follow = true) = 0;
 
   /// Get a \p File object for the file at \p Path, if one exists.
   virtual llvm::ErrorOr<std::unique_ptr<File>>
   openFileForRead(const Twine &Path) = 0;
 
   /// This is a convenience method that opens a file, gets its content and then
   /// closes the file.
   llvm::ErrorOr<std::unique_ptr<llvm::MemoryBuffer>>
class OverlayFileSystem : public FileSystem {
   FileSystemList FSList;
 
 public:
   OverlayFileSystem(IntrusiveRefCntPtr<FileSystem> Base);
 
   /// Pushes a file system on top of the stack.
   void pushOverlay(IntrusiveRefCntPtr<FileSystem> FS);
 
-  llvm::ErrorOr<Status> status(const Twine &Path) override;
+  llvm::ErrorOr<Status> status(const Twine &Path, bool Follow = true) override;
   llvm::ErrorOr<std::unique_ptr<File>>
   openFileForRead(const Twine &Path) override;
   directory_iterator dir_begin(const Twine &Dir, std::error_code &EC) override;
   llvm::ErrorOr<std::string> getCurrentWorkingDirectory() const override;
   std::error_code setCurrentWorkingDirectory(const Twine &Path) override;
   std::error_code isLocal(const Twine &Path, bool &Result) override;
   std::error_code getRealPath(const Twine &Path,
                               SmallVectorImpl<char> &Output) const override;

 /// By default, this delegates all calls to the underlying file system. This
 /// is useful when derived file systems want to override some calls and still
 /// proxy other calls.
 class ProxyFileSystem : public FileSystem {
 public:
   explicit ProxyFileSystem(IntrusiveRefCntPtr<FileSystem> FS)
       : FS(std::move(FS)) {}
 
-  llvm::ErrorOr<Status> status(const Twine &Path) override {
-    return FS->status(Path);
+  llvm::ErrorOr<Status> status(const Twine &Path, bool Follow = true) override {
+    return FS->status(Path, Follow);
   }
   llvm::ErrorOr<std::unique_ptr<File>>
   openFileForRead(const Twine &Path) override {
     return FS->openFileForRead(Path);
   }
   directory_iterator dir_begin(const Twine &Dir, std::error_code &EC) override {
     return FS->dir_begin(Dir, EC);
   }
bool addFileNoOwn(const Twine &Path, time_t ModificationTime,
                     Optional<llvm::sys::fs::file_type> Type = None,
                     Optional<llvm::sys::fs::perms> Perms = None);
 
   std::string toString() const;
 
   /// Return true if this file system normalizes . and .. in paths.
   bool useNormalizedPaths() const { return UseNormalizedPaths; }
 
-  llvm::ErrorOr<Status> status(const Twine &Path) override;
+  llvm::ErrorOr<Status> status(const Twine &Path, bool Follow = true) override;
   llvm::ErrorOr<std::unique_ptr<File>>
   openFileForRead(const Twine &Path) override;
   directory_iterator dir_begin(const Twine &Dir, std::error_code &EC) override;
 
   llvm::ErrorOr<std::string> getCurrentWorkingDirectory() const override {
     return WorkingDirectory;
   }
   /// Canonicalizes \p Path by combining with the current working
create(std::unique_ptr<MemoryBuffer> Buffer,
          SourceMgr::DiagHandlerTy DiagHandler, StringRef YAMLFilePath,
          void *DiagContext, IntrusiveRefCntPtr<FileSystem> ExternalFS);
 
   /// Redirect each of the remapped files from first to second.
   static std::unique_ptr<RedirectingFileSystem>
   create(ArrayRef<std::pair<std::string, std::string>> RemappedFiles,
          bool UseExternalNames, FileSystem &ExternalFS);
 
-  ErrorOr<Status> status(const Twine &Path) override;
+  ErrorOr<Status> status(const Twine &Path, bool Follow = true) override;
   ErrorOr<std::unique_ptr<File>> openFileForRead(const Twine &Path) override;
 
   std::error_code getRealPath(const Twine &Path,
                               SmallVectorImpl<char> &Output) const override;
 
   llvm::ErrorOr<std::string> getCurrentWorkingDirectory() const override;
 
   std::error_code setCurrentWorkingDirectory(const Twine &Path) override;

llvm/lib/Support/FileCollector.cpp

 namespace llvm {
 
 class FileCollectorFileSystem : public vfs::FileSystem {
 public:
   explicit FileCollectorFileSystem(IntrusiveRefCntPtr<vfs::FileSystem> FS,
                                    std::shared_ptr<FileCollector> Collector)
       : FS(std::move(FS)), Collector(std::move(Collector)) {}
 
-  llvm::ErrorOr<llvm::vfs::Status> status(const Twine &Path) override {
-    auto Result = FS->status(Path);
+  llvm::ErrorOr<llvm::vfs::Status> status(const Twine &Path,
+                                          bool Follow /*= true*/) override {
+    auto Result = FS->status(Path, Follow);
     if (Result && Result->exists())
       Collector->addFile(Path);
     return Result;
   }
 
   llvm::ErrorOr<std::unique_ptr<llvm::vfs::File>>
   openFileForRead(const Twine &Path) override {
     auto Result = FS->openFileForRead(Path);

llvm/lib/Support/VirtualFileSystem.cpp

if (!LinkCWDToProcess) {
         return; // Awful, but nothing to do here.
       if (llvm::sys::fs::real_path(PWD, RealPWD))
         WD = {PWD, PWD};
       else
         WD = {PWD, RealPWD};
     }
   }
 
-  ErrorOr<Status> status(const Twine &Path) override;
+  ErrorOr<Status> status(const Twine &Path, bool Follow = true) override;
   ErrorOr<std::unique_ptr<File>> openFileForRead(const Twine &Path) override;
   directory_iterator dir_begin(const Twine &Dir, std::error_code &EC) override;
 
   llvm::ErrorOr<std::string> getCurrentWorkingDirectory() const override;
   std::error_code setCurrentWorkingDirectory(const Twine &Path) override;
   std::error_code isLocal(const Twine &Path, bool &Result) override;
   std::error_code getRealPath(const Twine &Path,
                               SmallVectorImpl<char> &Output) const override;
struct WorkingDirectory {
     // The current working directory, with links resolved. (readlink .).
     SmallString<128> Resolved;
   };
   Optional<WorkingDirectory> WD;
 };
 
 } // namespace
 
-ErrorOr<Status> RealFileSystem::status(const Twine &Path) {
+ErrorOr<Status> RealFileSystem::status(const Twine &Path,
+                                       bool Follow /*= true*/) {
   SmallString<256> Storage;
   sys::fs::file_status RealStatus;
   if (std::error_code EC =
-          sys::fs::status(adjustPath(Path, Storage), RealStatus))
+          sys::fs::status(adjustPath(Path, Storage), RealStatus, Follow))
     return EC;
   return Status::copyWithNewName(RealStatus, Path);
 }
 
 ErrorOr<std::unique_ptr<File>>
 RealFileSystem::openFileForRead(const Twine &Name) {
   SmallString<256> RealName, Storage;
   Expected<file_t> FDOrErr = sys::fs::openNativeFileForRead(

 
 void OverlayFileSystem::pushOverlay(IntrusiveRefCntPtr<FileSystem> FS) {
   FSList.push_back(FS);
   // Synchronize added file systems by duplicating the working directory from
   // the first one in the list.
   FS->setCurrentWorkingDirectory(getCurrentWorkingDirectory().get());
 }
 
-ErrorOr<Status> OverlayFileSystem::status(const Twine &Path) {
+ErrorOr<Status> OverlayFileSystem::status(const Twine &Path,
+                                          bool Follow /*= true*/) {
   // FIXME: handle symlinks that cross file systems
   for (iterator I = overlays_begin(), E = overlays_end(); I != E; ++I) {
-    ErrorOr<Status> Status = (*I)->status(Path);
+    ErrorOr<Status> Status = (*I)->status(Path, Follow);
     if (Status || Status.getError() != llvm::errc::no_such_file_or_directory)
       return Status;
   }
   return make_error_code(llvm::errc::no_such_file_or_directory);
 }
 
 ErrorOr<std::unique_ptr<File>>
 OverlayFileSystem::openFileForRead(const llvm::Twine &Path) {
bool InMemoryFileSystem::addHardLink(const Twine &FromPath,
   // FromPath must not have been added before. ToPath must have been added
   // before. Resolved ToPath must be a File.
   if (!ToNode || FromNode || !isa<detail::InMemoryFile>(*ToNode))
     return false;
   return this->addFile(FromPath, 0, nullptr, None, None, None, None,
                        cast<detail::InMemoryFile>(*ToNode));
 }
 
-llvm::ErrorOr<Status> InMemoryFileSystem::status(const Twine &Path) {
+llvm::ErrorOr<Status> InMemoryFileSystem::status(const Twine &Path,
+                                                 bool Follow /*= true*/) {
+  // FIXME: Follow is not implemented for InMemoryFileSystem.
   auto Node = lookupInMemoryNode(*this, Root.get(), Path);
   if (Node)
     return detail::getNodeStatus(*Node, Path);
   return Node.getError();
 }
 
 llvm::ErrorOr<std::unique_ptr<File>>
 InMemoryFileSystem::openFileForRead(const Twine &Path) {
if (Optional<StringRef> ExtRedirect = Result.getExternalRedirect()) {
     return getRedirectedFileStatus(Path, RE->useExternalName(UseExternalNames),
                                    *S);
   }
 
   auto *DE = cast<RedirectingFileSystem::DirectoryEntry>(Result.E);
   return Status::copyWithNewName(DE->getStatus(), Path);
 }
 
-ErrorOr<Status> RedirectingFileSystem::status(const Twine &Path_) {
+ErrorOr<Status> RedirectingFileSystem::status(const Twine &Path_,
+                                              bool Follow /*= true*/) {
   SmallString<256> Path;
   Path_.toVector(Path);
 
   if (std::error_code EC = makeCanonical(Path))
     return EC;
 
   ErrorOr<RedirectingFileSystem::LookupResult> Result = lookupPath(Path);
   if (!Result) {
     if (shouldFallBackToExternalFS(Result.getError()))
-      return ExternalFS->status(Path);
+      return ExternalFS->status(Path, Follow);
     return Result.getError();
   }
 
   ErrorOr<Status> S = status(Path, *Result);
   if (!S && shouldFallBackToExternalFS(S.getError(), Result->E))
-    S = ExternalFS->status(Path);
+    S = ExternalFS->status(Path, Follow);
   return S;
 }
 
 namespace {
 
 /// Provide a file wrapper with an overriden status.
 class FileWithFixedStatus : public File {
   std::unique_ptr<File> InnerFile;

llvm/unittests/Support/VirtualFileSystemTest.cpp

class DummyFileSystem : public vfs::FileSystem {
   static int getNextFSID() {
     static int Count = 0;
     return Count++;
   }
 
 public:
   DummyFileSystem() : FSID(getNextFSID()), FileID(0) {}
 
-  ErrorOr<vfs::Status> status(const Twine &Path) override {
+  ErrorOr<vfs::Status> status(const Twine &Path, bool Follow = true) override {
     auto I = findEntry(Path);
     if (I == FilesAndDirs.end())
       return make_error_code(llvm::errc::no_such_file_or_directory);
     return I->second;
   }
   ErrorOr<std::unique_ptr<vfs::File>>
   openFileForRead(const Twine &Path) override {
     auto S = status(Path);