[clang] [clang][deps] Overload `Filesystem::exists` in `DependencyScanningFilesystem` to have it use cached `status` (PR #88152)

[Artem Chikin via cfe-commits]

https://github.com/artemcm updated https://github.com/llvm/llvm-project/pull/88152

>From 1dd15b331795daaf2ac70cbd013f56f2d7677041 Mon Sep 17 00:00:00 2001
From: Artem Chikin <achikin at apple.com>
Date: Tue, 9 Apr 2024 09:37:09 -0700
Subject: [PATCH] [clang][deps] Overload 'Filesystem::exists' in
 'DependencyScanningFilesystem' to have it use cached `status`

As-is, calls to `exists()` fallback on the implementation in 'ProxyFileSystem::exists' which explicitly calls out to the underlying `FS`, which for the 'DependencyScanningFilesystem' (overlay) is the real underlying filesystem.

Instead, directly overloading 'exists' allows us to have it rely on the cached `status` behavior used elsewhere by the 'DependencyScanningFilesystem'.
---
 .../DependencyScanningFilesystem.h            |  4 ++
 .../DependencyScanningFilesystem.cpp          | 11 ++++
 .../DependencyScanningFilesystemTest.cpp      | 53 ++++++++++++++-----
 3 files changed, 55 insertions(+), 13 deletions(-)

diff --git a/clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h b/clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h
index 8b6f149c7cb266..f7b4510d7f7beb 100644
--- a/clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h
+++ b/clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h
@@ -365,6 +365,10 @@ class DependencyScanningWorkerFilesystem
   /// false if not (i.e. this entry is not a file or its scan fails).
   bool ensureDirectiveTokensArePopulated(EntryRef Entry);
 
+  /// Check whether \p Path exists. By default checks cached result of \c
+  /// status(), and falls back on FS if unable to do so.
+  bool exists(const Twine &Path) override;
+
 private:
   /// For a filename that's not yet associated with any entry in the caches,
   /// uses the underlying filesystem to either look up the entry based in the
diff --git a/clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp b/clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
index 84185c931b552c..0cab17a3424406 100644
--- a/clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
+++ b/clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
@@ -300,6 +300,17 @@ DependencyScanningWorkerFilesystem::status(const Twine &Path) {
   return Result->getStatus();
 }
 
+bool DependencyScanningWorkerFilesystem::exists(const Twine &Path) {
+  // While some VFS overlay filesystems may implement more-efficient
+  // mechanisms for `exists` queries, `DependencyScanningWorkerFilesystem`
+  // typically wraps `RealFileSystem` which does not specialize `exists`,
+  // so it is not likely to benefit from such optimizations. Instead,
+  // it is more-valuable to have this query go through the
+  // cached-`status` code-path of the `DependencyScanningWorkerFilesystem`.
+  llvm::ErrorOr<llvm::vfs::Status> Status = status(Path);
+  return Status && Status->exists();
+}
+
 namespace {
 
 /// The VFS that is used by clang consumes the \c CachedFileSystemEntry using
diff --git a/clang/unittests/Tooling/DependencyScanning/DependencyScanningFilesystemTest.cpp b/clang/unittests/Tooling/DependencyScanning/DependencyScanningFilesystemTest.cpp
index 697b7d70ff035a..256dd980b2118f 100644
--- a/clang/unittests/Tooling/DependencyScanning/DependencyScanningFilesystemTest.cpp
+++ b/clang/unittests/Tooling/DependencyScanning/DependencyScanningFilesystemTest.cpp
@@ -8,6 +8,7 @@
 
 #include "clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h"
 #include "llvm/ADT/SmallString.h"
+#include "llvm/ADT/Twine.h"
 #include "llvm/Support/VirtualFileSystem.h"
 #include "gtest/gtest.h"
 
@@ -15,31 +16,36 @@ using namespace clang::tooling::dependencies;
 
 namespace {
 struct InstrumentingFilesystem
-    : llvm::RTTIExtends<InstrumentingFilesystem, llvm::vfs::ProxyFileSystem> {
+    : llvm::RTTIExtends<InstrumentingFilesystem, llvm::vfs::InMemoryFileSystem> {
   unsigned NumStatusCalls = 0;
   unsigned NumGetRealPathCalls = 0;
+  unsigned NumExistsCalls = 0;
 
   using llvm::RTTIExtends<InstrumentingFilesystem,
-                          llvm::vfs::ProxyFileSystem>::RTTIExtends;
+                          llvm::vfs::InMemoryFileSystem>::RTTIExtends;
 
   llvm::ErrorOr<llvm::vfs::Status> status(const llvm::Twine &Path) override {
     ++NumStatusCalls;
-    return ProxyFileSystem::status(Path);
+    return InMemoryFileSystem::status(Path);
   }
 
   std::error_code getRealPath(const llvm::Twine &Path,
                               llvm::SmallVectorImpl<char> &Output) override {
     ++NumGetRealPathCalls;
-    return ProxyFileSystem::getRealPath(Path, Output);
+    return InMemoryFileSystem::getRealPath(Path, Output);
+  }
+
+  bool exists(const llvm::Twine &Path) override {
+    ++NumExistsCalls;
+    return InMemoryFileSystem::exists(Path);
   }
 };
+
 } // namespace
 
 TEST(DependencyScanningWorkerFilesystem, CacheStatusFailures) {
-  auto InMemoryFS = llvm::makeIntrusiveRefCnt<llvm::vfs::InMemoryFileSystem>();
-
   auto InstrumentingFS =
-      llvm::makeIntrusiveRefCnt<InstrumentingFilesystem>(InMemoryFS);
+      llvm::makeIntrusiveRefCnt<InstrumentingFilesystem>();
 
   DependencyScanningFilesystemSharedCache SharedCache;
   DependencyScanningWorkerFilesystem DepFS(SharedCache, InstrumentingFS);
@@ -59,13 +65,11 @@ TEST(DependencyScanningWorkerFilesystem, CacheStatusFailures) {
 }
 
 TEST(DependencyScanningFilesystem, CacheGetRealPath) {
-  auto InMemoryFS = llvm::makeIntrusiveRefCnt<llvm::vfs::InMemoryFileSystem>();
-  InMemoryFS->setCurrentWorkingDirectory("/");
-  InMemoryFS->addFile("/foo", 0, llvm::MemoryBuffer::getMemBuffer(""));
-  InMemoryFS->addFile("/bar", 0, llvm::MemoryBuffer::getMemBuffer(""));
-
   auto InstrumentingFS =
-      llvm::makeIntrusiveRefCnt<InstrumentingFilesystem>(InMemoryFS);
+      llvm::makeIntrusiveRefCnt<InstrumentingFilesystem>();
+  InstrumentingFS->setCurrentWorkingDirectory("/");
+  InstrumentingFS->addFile("/foo", 0, llvm::MemoryBuffer::getMemBuffer(""));
+  InstrumentingFS->addFile("/bar", 0, llvm::MemoryBuffer::getMemBuffer(""));
 
   DependencyScanningFilesystemSharedCache SharedCache;
   DependencyScanningWorkerFilesystem DepFS(SharedCache, InstrumentingFS);
@@ -147,3 +151,26 @@ TEST(DependencyScanningFilesystem, RealPathAndStatusInvariants) {
     DepFS.status("/bar");
   }
 }
+
+TEST(DependencyScanningFilesystem, CacheStatOnExists) {
+  auto InMemoryInstrumentingFS =
+      llvm::makeIntrusiveRefCnt<InstrumentingFilesystem>();
+  InMemoryInstrumentingFS->setCurrentWorkingDirectory("/");
+  InMemoryInstrumentingFS->addFile("/foo", 0,
+                                   llvm::MemoryBuffer::getMemBuffer(""));
+  InMemoryInstrumentingFS->addFile("/bar", 0,
+                                   llvm::MemoryBuffer::getMemBuffer(""));
+  DependencyScanningFilesystemSharedCache SharedCache;
+  DependencyScanningWorkerFilesystem DepFS(SharedCache,
+                                           InMemoryInstrumentingFS);
+
+  DepFS.status("/foo");
+  DepFS.status("/foo");
+  DepFS.status("/bar");
+  EXPECT_EQ(InMemoryInstrumentingFS->NumStatusCalls, 2u);
+
+  DepFS.exists("/foo");
+  DepFS.exists("/bar");
+  EXPECT_EQ(InMemoryInstrumentingFS->NumStatusCalls, 2u);
+  EXPECT_EQ(InMemoryInstrumentingFS->NumExistsCalls, 0u);
+}