[clangd] Add a fallback directory for collected symbols with relative paths.

[sammccall]

clangd/global-symbol-builder/GlobalSymbolBuilderMain.cpp

41 ↗

(On Diff #131771)

I have trouble understanding this.

"fallback dir" is maybe too vague a name for this flag - "assume-header-directory" or "header-base-dir" or something?

The description assumes some context about the problem - can we provide it?
Maybe:

The index includes the header that a symbol is defined in.
If the absolute path cannot be determined (e.g. an in-memory VFS) then
the relative path is resolved against this directory, which must be absolute.
If this flag is not given, such headers will have relative paths.

42 ↗

(On Diff #131771)

nit: i.e -> e.g

113 ↗

(On Diff #131771)

wrong flag name :-)

[ioeric]

• Addressed review comments.

[ioeric]

clangd/global-symbol-builder/GlobalSymbolBuilderMain.cpp

41 ↗

(On Diff #131771)

Thanks for the suggestion! Let's go with --assume-header-dir.

clang-tools-extra/trunk/clangd/global-symbol-builder/GlobalSymbolBuilderMain.cpp

 
 using namespace llvm;
 using namespace clang::tooling;
 using clang::clangd::SymbolSlab;
 
 namespace clang {
 namespace clangd {
 
+static llvm::cl::opt<std::string> AssumedHeaderDir(
+    "assume-header-dir",
+    llvm::cl::desc("The index includes header that a symbol is defined in. "
+                   "If the absolute path cannot be determined (e.g. an "
+                   "in-memory VFS) then the relative path is resolved against "
+                   "this directory, which must be absolute. If this flag is "
+                   "not given, such headers will have relative paths."));
+
 class SymbolIndexActionFactory : public tooling::FrontendActionFactory {
 public:
   SymbolIndexActionFactory(tooling::ExecutionContext *Ctx) : Ctx(Ctx) {}
 
   clang::FrontendAction *create() override {
     // Wraps the index action and reports collected symbols to the execution
     // context at the end of each translation unit.
     class WrappedIndexAction : public WrapperFrontendAction {
private:
       tooling::ExecutionContext *Ctx;
       std::shared_ptr<SymbolCollector> Collector;
     };
 
     index::IndexingOptions IndexOpts;
     IndexOpts.SystemSymbolFilter =
         index::IndexingOptions::SystemSymbolFilterKind::All;
     IndexOpts.IndexFunctionLocals = false;
+    auto CollectorOpts = SymbolCollector::Options();
+    CollectorOpts.FallbackDir = AssumedHeaderDir;
     return new WrappedIndexAction(
-        std::make_shared<SymbolCollector>(SymbolCollector::Options()),
-        IndexOpts, Ctx);
+        std::make_shared<SymbolCollector>(std::move(CollectorOpts)), IndexOpts,
+        Ctx);
   }
 
   tooling::ExecutionContext *Ctx;
 };
 
 } // namespace clangd
 } // namespace clang
 
 int main(int argc, const char **argv) {
   llvm::sys::PrintStackTraceOnErrorSignal(argv[0]);
 
   const char* Overview =
       "This is an **experimental** tool to generate YAML-format "
       "project-wide symbols for clangd (global code completion). It would be "
       "changed and deprecated eventually. Don't use it in production code!";
   auto Executor = clang::tooling::createExecutorFromCommandLineArgs(
       argc, argv, cl::GeneralCategory, Overview);
 
   if (!Executor) {
     llvm::errs() << llvm::toString(Executor.takeError()) << "\n";
     return 1;
   }
 
+  if (!clang::clangd::AssumedHeaderDir.empty() &&
+      !llvm::sys::path::is_absolute(clang::clangd::AssumedHeaderDir)) {
+    llvm::errs() << "--assume-header-dir must be an absolute path.\n";
+    return 1;
+  }
+
   auto Err = Executor->get()->execute(
       llvm::make_unique<clang::clangd::SymbolIndexActionFactory>(
           Executor->get()->getExecutionContext()));
   if (Err) {
     llvm::errs() << llvm::toString(std::move(Err)) << "\n";
   }
 
   // Deduplicate the result by key and keep the longest value.

clang-tools-extra/trunk/clangd/index/SymbolCollector.h

 /// index::indexTopLevelDecls to retrieve all symbols when the source file is
 /// changed.
 class SymbolCollector : public index::IndexDataConsumer {
 public:
   struct Options {
     /// Whether to collect symbols in main files (e.g. the source file
     /// corresponding to a TU).
     bool IndexMainFiles = false;
+    // When symbol paths cannot be resolved to absolute paths (e.g. files in
+    // VFS that does not have absolute path), combine the fallback directory
+    // with symbols' paths to get absolute paths. This must be an absolute path.
+    std::string FallbackDir;
   };
 
   SymbolCollector(Options Opts);
 
   void initialize(ASTContext &Ctx) override;
 
   void setPreprocessor(std::shared_ptr<Preprocessor> PP) override {
     this->PP = std::move(PP);

clang-tools-extra/trunk/clangd/index/SymbolCollector.cpp

 //===--- SymbolCollector.cpp -------------------------------------*- C++-*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "SymbolCollector.h"
 #include "../CodeCompletionStrings.h"
 #include "clang/AST/DeclCXX.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
 #include "clang/Basic/SourceManager.h"
 #include "clang/Index/IndexSymbol.h"
 #include "clang/Index/USRGeneration.h"
+#include "llvm/Support/FileSystem.h"
 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/Path.h"
 
 namespace clang {
 namespace clangd {
 
 namespace {
 // Make the Path absolute using the current working directory of the given
-// SourceManager if the Path is not an absolute path.
+// SourceManager if the Path is not an absolute path. If failed, this combine
+// relative paths with \p FallbackDir to get an absolute path.
 //
 // The Path can be a path relative to the build directory, or retrieved from
 // the SourceManager.
-std::string makeAbsolutePath(const SourceManager &SM, StringRef Path) {
+std::string makeAbsolutePath(const SourceManager &SM, StringRef Path,
+                             StringRef FallbackDir) {
   llvm::SmallString<128> AbsolutePath(Path);
   if (std::error_code EC =
           SM.getFileManager().getVirtualFileSystem()->makeAbsolute(
               AbsolutePath))
     llvm::errs() << "Warning: could not make absolute file: '" << EC.message()
                  << '\n';
+  if (llvm::sys::path::is_absolute(AbsolutePath)) {
     // Handle the symbolic link path case where the current working directory
     // (getCurrentWorkingDirectory) is a symlink./ We always want to the real
     // file path (instead of the symlink path) for the  C++ symbols.
     //
     // Consider the following example:
     //
     //   src dir: /project/src/foo.h
     //   current working directory (symlink): /tmp/build -> /project/src/
     //
     // The file path of Symbol is "/project/src/foo.h" instead of
     // "/tmp/build/foo.h"
-  const DirectoryEntry *Dir = SM.getFileManager().getDirectory(
-      llvm::sys::path::parent_path(AbsolutePath.str()));
-  if (Dir) {
+    if (const DirectoryEntry *Dir = SM.getFileManager().getDirectory(
+            llvm::sys::path::parent_path(AbsolutePath.str()))) {
       StringRef DirName = SM.getFileManager().getCanonicalName(Dir);
       SmallString<128> AbsoluteFilename;
       llvm::sys::path::append(AbsoluteFilename, DirName,
                               llvm::sys::path::filename(AbsolutePath.str()));
-    return AbsoluteFilename.str();
+      AbsolutePath = AbsoluteFilename;
+    }
+  } else if (!FallbackDir.empty()) {
+    llvm::sys::fs::make_absolute(FallbackDir, AbsolutePath);
+    llvm::sys::path::remove_dots(AbsolutePath, /*remove_dot_dot=*/true);
   }
   return AbsolutePath.str();
 }
 
 // "a::b::c", return {"a::b::", "c"}. Scope is empty if there's no qualifier.
 std::pair<llvm::StringRef, llvm::StringRef>
 splitQualifiedName(llvm::StringRef QName) {
   assert(!QName.startswith("::") && "Qualified names should not start with ::");
if (const NamedDecl *ND = llvm::dyn_cast<NamedDecl>(D)) {
     if (index::generateUSRForDecl(ND, USR))
       return true;
 
     auto ID = SymbolID(USR);
     if (Symbols.find(ID) != nullptr)
       return true;
 
     auto &SM = ND->getASTContext().getSourceManager();
-    std::string FilePath =
-        makeAbsolutePath(SM, SM.getFilename(D->getLocation()));
+    std::string FilePath = makeAbsolutePath(
+        SM, SM.getFilename(D->getLocation()), Opts.FallbackDir);
     SymbolLocation Location = {FilePath, SM.getFileOffset(D->getLocStart()),
                                SM.getFileOffset(D->getLocEnd())};
     std::string QName = ND->getQualifiedNameAsString();
 
     Symbol S;
     S.ID = std::move(ID);
     std::tie(S.Scope, S.Name) = splitQualifiedName(QName);
     S.SymInfo = index::getSymbolInfo(D);

clang-tools-extra/trunk/unittests/clangd/SymbolCollectorTests.cpp

MATCHER_P(Detail, D, "") {
   return arg.Detail && arg.Detail->CompletionDetail == D;
 }
 MATCHER_P(Doc, D, "") { return arg.Detail && arg.Detail->Documentation == D; }
 MATCHER_P(Plain, Text, "") { return arg.CompletionPlainInsertText == Text; }
 MATCHER_P(Snippet, S, "") {
   return arg.CompletionSnippetInsertText == S;
 }
 MATCHER_P(QName, Name, "") { return (arg.Scope + arg.Name).str() == Name; }
+MATCHER_P(Path, P, "") { return arg.CanonicalDeclaration.FilePath == P; }
 
 namespace clang {
 namespace clangd {
 
 namespace {
 class SymbolIndexActionFactory : public tooling::FrontendActionFactory {
 public:
   SymbolIndexActionFactory(SymbolCollector::Options COpts)
const std::string Main = R"(
     // implicit declarations (the implicit using shadow declaration) by default,
     // and there is no way to customize this behavior at the moment.
     using bar::v2;
     } // namespace foo
   )";
   runSymbolCollector(Header, Main);
   EXPECT_THAT(Symbols,
               UnorderedElementsAreArray(
-                  {QName("Foo"),
-                   QName("f1"),
-                   QName("f2"),
-                   QName("KInt"),
-                   QName("kStr"),
-                   QName("foo"),
-                   QName("foo::bar"),
-                   QName("foo::int32"),
-                   QName("foo::int32_t"),
-                   QName("foo::v1"),
-                   QName("foo::bar::v2"),
-                   QName("foo::baz")}));
+                  {QName("Foo"), QName("f1"), QName("f2"), QName("KInt"),
+                   QName("kStr"), QName("foo"), QName("foo::bar"),
+                   QName("foo::int32"), QName("foo::int32_t"), QName("foo::v1"),
+                   QName("foo::bar::v2"), QName("foo::baz")}));
+}
+
+TEST_F(SymbolCollectorTest, SymbolRelativeNoFallback) {
+  CollectorOpts.IndexMainFiles = false;
+  runSymbolCollector("class Foo {};", /*Main=*/"");
+  EXPECT_THAT(Symbols,
+              UnorderedElementsAre(AllOf(QName("Foo"), Path("symbols.h"))));
+}
+
+TEST_F(SymbolCollectorTest, SymbolRelativeWithFallback) {
+  CollectorOpts.IndexMainFiles = false;
+  CollectorOpts.FallbackDir = "/cwd";
+  runSymbolCollector("class Foo {};", /*Main=*/"");
+  EXPECT_THAT(Symbols, UnorderedElementsAre(
+                           AllOf(QName("Foo"), Path("/cwd/symbols.h"))));
 }
 
 TEST_F(SymbolCollectorTest, IncludeEnums) {
   CollectorOpts.IndexMainFiles = false;
   const std::string Header = R"(
     enum {
       Red
     };