[clangd] Get rid of dots and dotsdots within GlobalCompilationDatabase

[kadircet]

Summary

These bugs has surfaced during the refactoring D64712.
There are some test cases that passes paths with ".." in them to OverlayCDB and DirectoryBasedCDB, which were not failing before because (tested) users of these functions were handling the canonicalization.

It is also possible to have an LSP client sending over compile commands over the wire without canonicalizing them.
As for the second bug, it is easier to trigger for example a json comp db with an entry ("file": "/abs/path/build/../a.cc") will trigger that.

I am not sure if there are build systems generating this kind of output, but it looks valid from a comp db perspective, whereas it doesn't comply with clangd's assumption.

[sammccall]

clang-tools-extra/clangd/GlobalCompilationDatabase.cpp

77

This patch lacks a bit of context (why are we changing this behavior).
Based on offline discussion, it's not clear to me why this particular change to getFallbackCommand is necessary or desirable. (Compared to others, which seem like fairly obvious bugs, even if the reason to fix them isn't obvious)

144

nit: can you pull out a local (or FS.h) RemoveDots helper that accepts StringRef and returns std::string?

Most of the usages in this file could be inlined and I think that would read better. Not concerned about performance I think. (But returning SmallString is OK if you are)

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

307

Is this actually important/desirable?

[kadircet]

• Add removeDots helper to FS.h
• Revert changes in getFallbackCommands.
• Add comments for the reasoning behind removeDots calls.

clang-tools-extra/clangd/GlobalCompilationDatabase.cpp

77

updating the summary for reasoning

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

307

yea agreed this is not necessary.

[sammccall]

As discussed, the long-term fix might be to avoid ever encountering these paths (filtering everything we get over LSP, CDBs etc).
Maybe add a fixme on removeDots?

[kadircet]

• Address comments

clang-tools-extra/clangd/GlobalCompilationDatabase.cpp

 #include "Logger.h"
 #include "Path.h"
 #include "clang/Frontend/CompilerInvocation.h"
 #include "clang/Tooling/ArgumentsAdjusters.h"
 #include "clang/Tooling/CompilationDatabase.h"
 #include "llvm/ADT/None.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/STLExtras.h"
+#include "llvm/ADT/SmallString.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/Path.h"
 #include <string>
 #include <tuple>
 #include <vector>
 
 namespace clang {
 namespace clangd {
static std::string getFallbackClangPath() {
   SmallString<128> ClangPath;
   ClangPath = llvm::sys::path::parent_path(ClangdExecutable);
   llvm::sys::path::append(ClangPath, "clang");
   return ClangPath.str();
 }
 
 tooling::CompileCommand
 GlobalCompilationDatabase::getFallbackCommand(PathRef File) const {
+  llvm::SmallString<128> CanonPath(File);
+  llvm::sys::path::remove_dots(CanonPath, true);
+
   std::vector<std::string> Argv = {getFallbackClangPath()};
   // Clang treats .h files as C by default and files without extension as linker
   // input, resulting in unhelpful diagnostics.
   // Parsing as Objective C++ is friendly to more cases.
-  auto FileExtension = llvm::sys::path::extension(File);
+  auto FileExtension = llvm::sys::path::extension(CanonPath);
   if (FileExtension.empty() || FileExtension == ".h")
     Argv.push_back("-xobjective-c++-header");
-  Argv.push_back(File);
-  tooling::CompileCommand Cmd(llvm::sys::path::parent_path(File),
-                              llvm::sys::path::filename(File), std::move(Argv),
+  Argv.push_back(CanonPath.str().str());
+  tooling::CompileCommand Cmd(llvm::sys::path::parent_path(CanonPath),
+                              llvm::sys::path::filename(CanonPath),
+                              std::move(Argv),
                               /*Output=*/"");
   Cmd.Heuristic = "clangd fallback";
   return Cmd;
 }
 
 DirectoryBasedGlobalCompilationDatabase::
     DirectoryBasedGlobalCompilationDatabase(
         llvm::Optional<Path> CompileCommandsDir)
DirectoryBasedGlobalCompilationDatabase::getCDBInDirLocked(PathRef Dir) const {
   return {Result, false};
 }
 
 llvm::Optional<DirectoryBasedGlobalCompilationDatabase::CDBLookupResult>
 DirectoryBasedGlobalCompilationDatabase::lookupCDB(
     CDBLookupRequest Request) const {
   assert(llvm::sys::path::is_absolute(Request.FileName) &&
          "path must be absolute");
+  llvm::SmallString<128> CanonPath(Request.FileName);
+  llvm::sys::path::remove_dots(CanonPath, true);
 
   CDBLookupResult Result;
   bool SentBroadcast = false;
 
   {
     std::lock_guard<std::mutex> Lock(Mutex);
     if (CompileCommandsDir) {
       std::tie(Result.CDB, SentBroadcast) =
           getCDBInDirLocked(*CompileCommandsDir);
       Result.PI.SourceRoot = *CompileCommandsDir;
     } else {
       actOnAllParentDirectories(
-          Request.FileName, [this, &SentBroadcast, &Result](PathRef Path) {
+          CanonPath, [this, &SentBroadcast, &Result](PathRef Path) {
             std::tie(Result.CDB, SentBroadcast) = getCDBInDirLocked(Path);
             Result.PI.SourceRoot = Path;
             return Result.CDB != nullptr;
           });
     }
 
     if (!Result.CDB)
       return llvm::None;
for (llvm::StringRef File : AllFiles) {
         auto Res = getCDBInDirLocked(Path);
         It.first->second = Res.first != nullptr;
         return Path == Result.PI.SourceRoot;
       });
     }
   }
 
   std::vector<std::string> GovernedFiles;
+  llvm::SmallString<128> CanonPath;
   for (llvm::StringRef File : AllFiles) {
     // A file is governed by this CDB if lookup for the file would find it.
     // Independent of whether it has an entry for that file or not.
     actOnAllParentDirectories(File, [&](PathRef Path) {
       if (DirectoryHasCDB.lookup(Path)) {
-        if (Path == Result.PI.SourceRoot)
-          GovernedFiles.push_back(File);
+        if (Path == Result.PI.SourceRoot) {
+          CanonPath = File;
+          llvm::sys::path::remove_dots(CanonPath, true);
+          GovernedFiles.push_back(CanonPath.str().str());
+        }
         // Stop as soon as we hit a CDB.
         return true;
       }
       return false;
     });
   }
 
   OnCommandChanged.broadcast(std::move(GovernedFiles));
OverlayCDB::OverlayCDB(const GlobalCompilationDatabase *Base,
   if (Base)
     BaseChanged = Base->watch([this](const std::vector<std::string> Changes) {
       OnCommandChanged.broadcast(Changes);
     });
 }
 
 llvm::Optional<tooling::CompileCommand>
 OverlayCDB::getCompileCommand(PathRef File) const {
+  llvm::SmallString<128> CanonPath(File);
+  llvm::sys::path::remove_dots(CanonPath, true);
   llvm::Optional<tooling::CompileCommand> Cmd;
   {
     std::lock_guard<std::mutex> Lock(Mutex);
-    auto It = Commands.find(File);
+    auto It = Commands.find(CanonPath);
     if (It != Commands.end())
       Cmd = It->second;
   }
   if (!Cmd && Base)
     Cmd = Base->getCompileCommand(File);
   if (!Cmd)
     return llvm::None;
   adjustArguments(*Cmd, ResourceDir);
   return Cmd;
 }
 
 tooling::CompileCommand OverlayCDB::getFallbackCommand(PathRef File) const {
   auto Cmd = Base ? Base->getFallbackCommand(File)
                   : GlobalCompilationDatabase::getFallbackCommand(File);
   std::lock_guard<std::mutex> Lock(Mutex);
   Cmd.CommandLine.insert(Cmd.CommandLine.end(), FallbackFlags.begin(),
                          FallbackFlags.end());
   adjustArguments(Cmd, ResourceDir);
   return Cmd;
 }
 
 void OverlayCDB::setCompileCommand(
     PathRef File, llvm::Optional<tooling::CompileCommand> Cmd) {
+  llvm::SmallString<128> CanonPath(File);
+  llvm::sys::path::remove_dots(CanonPath, true);
   {
     std::unique_lock<std::mutex> Lock(Mutex);
     if (Cmd)
-      Commands[File] = std::move(*Cmd);
+      Commands[CanonPath] = std::move(*Cmd);
     else
-      Commands.erase(File);
+      Commands.erase(CanonPath);
   }
-  OnCommandChanged.broadcast({File});
+  OnCommandChanged.broadcast({CanonPath.str().str()});
 }
 
 llvm::Optional<ProjectInfo> OverlayCDB::getProjectInfo(PathRef File) const {
+  llvm::SmallString<128> CanonPath(File);
+  llvm::sys::path::remove_dots(CanonPath, true);
   {
     std::lock_guard<std::mutex> Lock(Mutex);
-    auto It = Commands.find(File);
+    auto It = Commands.find(CanonPath);
     if (It != Commands.end())
       return ProjectInfo{};
   }
   if (Base)
     return Base->getProjectInfo(File);
 
   return llvm::None;
 }
 } // namespace clangd
 } // namespace clang

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

 //===-- GlobalCompilationDatabaseTests.cpp ----------------------*- 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 "GlobalCompilationDatabase.h"
 
 #include "Path.h"
 #include "TestFS.h"
+#include "clang/Tooling/CompilationDatabase.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/SmallString.h"
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/FormatVariadic.h"
 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/raw_ostream.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 #include <fstream>
 #include <string>
 
 namespace clang {
 namespace clangd {
 namespace {
 using ::testing::AllOf;
 using ::testing::Contains;
 using ::testing::ElementsAre;
 using ::testing::EndsWith;
+using ::testing::HasSubstr;
 using ::testing::IsEmpty;
 using ::testing::Not;
 using ::testing::StartsWith;
 using ::testing::UnorderedElementsAre;
 
 TEST(GlobalCompilationDatabaseTest, FallbackCommand) {
   DirectoryBasedGlobalCompilationDatabase DB(None);
   auto Cmd = DB.getFallbackCommand(testPath("foo/bar.cc"));
// happens in practice though.
     DirectoryBasedGlobalCompilationDatabase DB(llvm::None);
     std::vector<std::string> DiscoveredFiles;
     auto Sub =
         DB.watch([&DiscoveredFiles](const std::vector<std::string> Changes) {
           DiscoveredFiles = Changes;
         });
 
     File = FS.Root;
-    llvm::sys::path::append(File, "a.cc");
+    llvm::sys::path::append(File, "build", "..", "a.cc");
     DB.getCompileCommand(File.str());
-    EXPECT_THAT(DiscoveredFiles, UnorderedElementsAre(EndsWith("a.cc")));
+    EXPECT_THAT(DiscoveredFiles, UnorderedElementsAre(AllOf(
+                                     EndsWith("a.cc"), Not(HasSubstr("..")))));
     DiscoveredFiles.clear();
 
     File = FS.Root;
     llvm::sys::path::append(File, "build", "gen.cc");
     DB.getCompileCommand(File.str());
     EXPECT_THAT(DiscoveredFiles, UnorderedElementsAre(EndsWith("gen.cc")));
   }
 
// With a custom compile commands dir.
 
     File = FS.Root;
     llvm::sys::path::append(File, "build", "gen.cc");
     DB.getCompileCommand(File.str());
     EXPECT_THAT(DiscoveredFiles, IsEmpty());
   }
 }
 
+TEST(GlobalCompilationDatabaseTest, NonCanonicalFilenames) {
+  OverlayCDB DB(nullptr);
+  std::vector<std::string> DiscoveredFiles;
+  auto Sub =
+      DB.watch([&DiscoveredFiles](const std::vector<std::string> Changes) {
+        DiscoveredFiles = Changes;
+      });
+
+  llvm::SmallString<128> Root(testRoot());
+  llvm::sys::path::append(Root, "build", "..", "a.cc");
+  DB.setCompileCommand(Root.str(), tooling::CompileCommand());
+  EXPECT_THAT(DiscoveredFiles, UnorderedElementsAre(testPath("a.cc")));
+  DiscoveredFiles.clear();
+
+  llvm::SmallString<128> File(testRoot());
+  llvm::sys::path::append(File, "blabla", "..", "a.cc");
+
+  EXPECT_TRUE(DB.getCompileCommand(File));
+  EXPECT_TRUE(DB.getProjectInfo(File));
+  EXPECT_EQ(DB.getFallbackCommand(File).Directory, testRoot());
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang