[clangd] Make PreambleStatusCache handle filenames more carefully

[sammccall]

Summary

• when we hit the cache, the reported filename should be that of the cache query, not that of the cache store. This matches behaviors of common FSes, and avoids triggering difficult edge cases in FileManager when files are being moved around concurrently.
• filename comparisons (both cache queries and == mainfile checks) should fold away . and .. in paths. These can appear when relative paths occur in compile_commands.json. (gn does this).

[kadircet]

LGTM, thanks

clangd/FS.cpp

 //===--- FS.cpp - File system related utils ----------------------*- C++-*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "FS.h"
 #include "clang/Basic/LLVM.h"
 #include "llvm/ADT/None.h"
 #include "llvm/Support/Path.h"
+#include "llvm/Support/VirtualFileSystem.h"
 
 namespace clang {
 namespace clangd {
 
-PreambleFileStatusCache::PreambleFileStatusCache(llvm::StringRef MainFilePath)
-    : MainFilePath(MainFilePath) {
+PreambleFileStatusCache::PreambleFileStatusCache(llvm::StringRef MainFilePath){
   assert(llvm::sys::path::is_absolute(MainFilePath));
+  llvm::SmallString<256> MainFileCanonical(MainFilePath);
+  llvm::sys::path::remove_dots(MainFileCanonical, /*remove_dot_dot=*/true);
+  this->MainFilePath = MainFileCanonical.str();
 }
 
 void PreambleFileStatusCache::update(const llvm::vfs::FileSystem &FS,
                                      llvm::vfs::Status S) {
+  // Canonicalize path for later lookup, which is usually by absolute path.
   llvm::SmallString<32> PathStore(S.getName());
   if (FS.makeAbsolute(PathStore))
     return;
+  llvm::sys::path::remove_dots(PathStore, /*remove_dot_dot=*/true);
   // Do not cache status for the main file.
   if (PathStore == MainFilePath)
     return;
   // Stores the latest status in cache as it can change in a preamble build.
   StatCache.insert({PathStore, std::move(S)});
 }
 
 llvm::Optional<llvm::vfs::Status>
 PreambleFileStatusCache::lookup(llvm::StringRef File) const {
-  auto I = StatCache.find(File);
+  // Canonicalize to match the cached form.
+  // Lookup tends to be first by absolute path, so no need to make absolute.
+  llvm::SmallString<256> PathLookup(File);
+  llvm::sys::path::remove_dots(PathLookup, /*remove_dot_dot=*/true);
+
+  auto I = StatCache.find(PathLookup);
   if (I != StatCache.end())
-    return I->getValue();
+    // Returned Status name should always match the requested File.
+    return llvm::vfs::Status::copyWithNewName(I->getValue(), File);
   return None;
 }
 
 llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem>
 PreambleFileStatusCache::getProducingFS(
     llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> FS) {
   // This invalidates old status in cache if files are re-`open()`ed or
   // re-`stat()`ed in case file status has changed during preamble build.

clangd/unittests/FSTests.cpp

TEST(FSTests, PreambleStatusCache) {
   EXPECT_TRUE(ProduceFS->status("y"));
   EXPECT_TRUE(ProduceFS->status("main"));
 
   EXPECT_TRUE(StatCache.lookup(testPath("x")).hasValue());
   EXPECT_TRUE(StatCache.lookup(testPath("y")).hasValue());
   // Main file is not cached.
   EXPECT_FALSE(StatCache.lookup(testPath("main")).hasValue());
 
-  llvm::vfs::Status S("fake", llvm::sys::fs::UniqueID(0, 0),
+  llvm::vfs::Status S("fake", llvm::sys::fs::UniqueID(123, 456),
                       std::chrono::system_clock::now(), 0, 0, 1024,
                       llvm::sys::fs::file_type::regular_file,
                       llvm::sys::fs::all_all);
   StatCache.update(*FS, S);
   auto ConsumeFS = StatCache.getConsumingFS(FS);
   auto Cached = ConsumeFS->status(testPath("fake"));
   EXPECT_TRUE(Cached);
-  EXPECT_EQ(Cached->getName(), S.getName());
+  EXPECT_EQ(Cached->getName(), testPath("fake"));
+  EXPECT_EQ(Cached->getUniqueID(), S.getUniqueID());
+
+  // fake and temp/../fake should hit the same cache entry.
+  // However, the Status returned reflects the actual path requested.
+  auto CachedDotDot = ConsumeFS->status(testPath("temp/../fake"));
+  EXPECT_TRUE(CachedDotDot);
+  EXPECT_EQ(CachedDotDot->getName(), testPath("temp/../fake"));
+  EXPECT_EQ(CachedDotDot->getUniqueID(), S.getUniqueID());
 }
 
 } // namespace
 } // namespace clangd
 } // namespace clang