[clangd] Use URIs in index symbols.

[sammccall]

This looks OK so far, where is it going? It doesn't make sense to put URIs in if we only ever use file:.
I guess others will be produced by some kind of extension point on SymbolCollector. That will specify URI schemes we should try, allow you to replace the whole toFileURI, or something else?

Unfortunately there's a bunch of Uris here, where the existing code uses URI...

clangd/index/Index.h

27

nit: FileURI?
(The other style is OK too, though I personally find it harder to read. But the class is URI and we should be consistent)

clangd/index/SymbolCollector.cpp

28

this combines

or better, "resolves relative paths against \p FallbackDir"

33

also URI here, and below

201

while here, would you mind changing GetSymbolLocation -> getSymbolLocation?

clangd/index/SymbolYAML.cpp

51

more URI

[ioeric]

• Make URIScheme customizable in SymbolCollector.

[ioeric]

clangd/index/Index.h

27

I tend to use Uri for URI strings and URI/U for URI objects. But I'd be okay with either, if you prefer URI.

[ioeric]

I was thinking about leaving URI scheme customization to the postprocessing phase, but you are right, it would be better to make the URI scheme extendable in SymbolCollector.

[sammccall]

Great, this all makes sense. I think we can/should make the scheme selection a bit more robust (we shouldn't crash if we get unexpected filenames).
And... Uri or URI (I really think this is a usability issue - i had a scarring experience with a codebase that couldn't decide how to spell abbreviations...)

clangd/index/Index.h

27

I'm fine with either spelling, but not really with mixing the two.

Is there some precedent for using different capitalization depending on the variable type? That seems... non obvious to me.

clangd/index/SymbolCollector.cpp

67

this doesn't seem unreachable?

clangd/index/SymbolCollector.h

38 ↗

(On Diff #132958)

We should document the semantics/exceptions in case of failure.
Do we drop the symbol, or drop the location, or fall back to file URI?

One scheme that would be flexible and I think pretty simple:

• make this a vector of schemes that will be tried in order
• drop the location (but not symbol) if it's not possible to encode the path
• note that the vector should probably end in "file" as a fallback, and make {"file"} the default

For our monorepo we can set this to {"google", "file"} or just {"google"} if we never want to leak local paths. It seems pretty plausible that other orgs whose builds aren't totally hermetic would want several schemes here.

unittests/clangd/SymbolCollectorTests.cpp

49

while renaming this, maybe Decl is better? we'll have Def soon!

195

Can you add a test for the case when the path fails to convert to the URI scheme?

[hokein]

clangd/index/Index.h

27

I'm +1 on URI, using all capital letters for initialisms can make the name a bit easier to read. Let's be consistent using URI?

[ioeric]

clangd/index/Index.h

27

I'll change to URI. Another reason why I avoided using URI as variable names was the name conflicts with the actual URI class... I always need to come up with a prefix...

[ioeric]

• s/Uri/URI/
• Address review comments. Support multiple schemes.
• Merged with origin/master
• Merge branch 'master' of http://llvm.org/git/clang-tools-extra into uri

[sammccall]

unittests/clangd/SymbolCollectorTests.cpp

246–253

leftover logs

[ioeric]

• removed leftover logs.
• Merge branch 'master' of http://llvm.org/git/clang-tools-extra into uri

clangd/index/Index.h

 #include "llvm/ADT/StringExtras.h"
 #include <array>
 #include <string>
 
 namespace clang {
 namespace clangd {
 
 struct SymbolLocation {
-  // The absolute path of the source file where a symbol occurs.
-  llvm::StringRef FilePath;
+  // The URI of the source file where a symbol occurs.
+  llvm::StringRef FileUri;
   // The 0-based offset to the first character of the symbol from the beginning
   // of the source file.
   unsigned StartOffset;
   // The 0-based offset to the last character of the symbol from the beginning
   // of the source file.
   unsigned EndOffset;
 };
 

clangd/index/Index.cpp

if (R.second) { // New entry added to the table, copy the string.
       *R.first = V.copy(Arena);
     }
     V = *R.first;
   };
 
   // We need to copy every StringRef field onto the arena.
   Intern(S.Name);
   Intern(S.Scope);
-  Intern(S.CanonicalDeclaration.FilePath);
+  Intern(S.CanonicalDeclaration.FileUri);
 
   Intern(S.CompletionLabel);
   Intern(S.CompletionFilterText);
   Intern(S.CompletionPlainInsertText);
   Intern(S.CompletionSnippetInsertText);
 
   if (S.Detail) {
     // Copy values of StringRefs into arena.

clangd/index/Merge.cpp

 }
 
 Symbol
 mergeSymbol(const Symbol &L, const Symbol &R, Symbol::Details *Scratch) {
   assert(L.ID == R.ID);
   Symbol S = L;
   // For each optional field, fill it from R if missing in L.
   // (It might be missing in R too, but that's a no-op).
-  if (S.CanonicalDeclaration.FilePath == "")
+  if (S.CanonicalDeclaration.FileUri == "")
     S.CanonicalDeclaration = R.CanonicalDeclaration;
   if (S.CompletionLabel == "")
     S.CompletionLabel = R.CompletionLabel;
   if (S.CompletionFilterText == "")
     S.CompletionFilterText = R.CompletionFilterText;
   if (S.CompletionPlainInsertText == "")
     S.CompletionPlainInsertText = R.CompletionPlainInsertText;
   if (S.CompletionSnippetInsertText == "")

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 "../URI.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. If failed, this combine
-// relative paths with \p FallbackDir to get an absolute path.
+// Returns a file:// URI of \p Path. This makes the \p Path absolute using the
+// current working directory of the given 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,
-                             StringRef FallbackDir) {
+std::string toFileUri(const SourceManager &SM, StringRef Path,
   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
if (const DirectoryEntry *Dir = SM.getFileManager().getDirectory(
       llvm::sys::path::append(AbsoluteFilename, DirName,
                               llvm::sys::path::filename(AbsolutePath.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();
+  return URI::createFile(AbsolutePath).toString();
 }
 
 // "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 ::");
   size_t Pos = QName.rfind("::");
   if (Pos == llvm::StringRef::npos)
     return {StringRef(), QName};
   return {QName.substr(0, Pos + 2), QName.substr(Pos + 2)};

 
 // Return the symbol location of the given declaration `D`.
 //
 // For symbols defined inside macros:
 //   * use expansion location, if the symbol is formed via macro concatenation.
 //   * use spelling location, otherwise.
 SymbolLocation GetSymbolLocation(const NamedDecl *D, SourceManager &SM,
                                  StringRef FallbackDir,
-                                 std::string &FilePathStorage) {
+                                 std::string &FileUriStorage) {
   SymbolLocation Location;
 
   SourceLocation Loc = SM.getSpellingLoc(D->getLocation());
   if (D->getLocation().isMacroID()) {
     // The symbol is formed via macro concatenation, the spelling location will
     // be "<scratch space>", which is not interesting to us, use the expansion
     // location instead.
     if (llvm::StringRef(Loc.printToString(SM)).startswith("<scratch")) {
-      FilePathStorage = makeAbsolutePath(
-          SM, SM.getFilename(SM.getExpansionLoc(D->getLocation())),
+      FileUriStorage =
+          toFileUri(SM, SM.getFilename(SM.getExpansionLoc(D->getLocation())),
                     FallbackDir);
-      return {FilePathStorage,
+      return {FileUriStorage,
               SM.getFileOffset(SM.getExpansionRange(D->getLocStart()).first),
               SM.getFileOffset(SM.getExpansionRange(D->getLocEnd()).second)};
     }
   }
 
-  FilePathStorage = makeAbsolutePath(SM, SM.getFilename(Loc), FallbackDir);
-  return {FilePathStorage,
-          SM.getFileOffset(SM.getSpellingLoc(D->getLocStart())),
+  FileUriStorage = toFileUri(SM, SM.getFilename(Loc), FallbackDir);
+  return {FileUriStorage, SM.getFileOffset(SM.getSpellingLoc(D->getLocStart())),
           SM.getFileOffset(SM.getSpellingLoc(D->getLocEnd()))};
 }
 
 } // namespace
 
 SymbolCollector::SymbolCollector(Options Opts) : Opts(std::move(Opts)) {}
 
 void SymbolCollector::initialize(ASTContext &Ctx) {
if (const NamedDecl *ND = llvm::dyn_cast<NamedDecl>(D)) {
     Policy.SuppressUnwrittenScope = true;
     ND->printQualifiedName(OS, Policy);
     OS.flush();
 
     Symbol S;
     S.ID = std::move(ID);
     std::tie(S.Scope, S.Name) = splitQualifiedName(QName);
     S.SymInfo = index::getSymbolInfo(D);
-    std::string FilePath;
-    S.CanonicalDeclaration = GetSymbolLocation(ND, SM, Opts.FallbackDir, FilePath);
+    std::string Uri;
+    S.CanonicalDeclaration = GetSymbolLocation(ND, SM, Opts.FallbackDir, Uri);
 
     // Add completion info.
     assert(ASTCtx && PP.get() && "ASTContext and Preprocessor must be set.");
     CodeCompletionResult SymbolCompletion(ND, 0);
     const auto *CCS = SymbolCompletion.CreateCodeCompletionString(
         *ASTCtx, *PP, CodeCompletionContext::CCC_Name, *CompletionAllocator,
         *CompletionTUInfo,
         /*IncludeBriefComments*/ true);

clangd/index/SymbolYAML.cpp

struct NormalizedSymbolID {
 
   std::string HexString;
 };
 
 template <> struct MappingTraits<SymbolLocation> {
   static void mapping(IO &IO, SymbolLocation &Value) {
     IO.mapRequired("StartOffset", Value.StartOffset);
     IO.mapRequired("EndOffset", Value.EndOffset);
-    IO.mapRequired("FilePath", Value.FilePath);
+    IO.mapRequired("FileUri", Value.FileUri);
   }
 };
 
 template <> struct MappingTraits<SymbolInfo> {
   static void mapping(IO &io, SymbolInfo &SymInfo) {
     // FIXME: expose other fields?
     io.mapRequired("Kind", SymInfo.Kind);
     io.mapRequired("Lang", SymInfo.Lang);

unittests/clangd/IndexTests.cpp

TEST(MergeTest, MergeIndex) {
   EXPECT_THAT(match(*mergeIndex(&I, &J), Req),
               UnorderedElementsAre("ns::A", "ns::B", "ns::C"));
 }
 
 TEST(MergeTest, Merge) {
   Symbol L, R;
   L.ID = R.ID = SymbolID("hello");
   L.Name = R.Name = "Foo";                    // same in both
-  L.CanonicalDeclaration.FilePath = "left.h"; // differs
-  R.CanonicalDeclaration.FilePath = "right.h";
+  L.CanonicalDeclaration.FileUri = "file:///left.h"; // differs
+  R.CanonicalDeclaration.FileUri = "file:///right.h";
   L.CompletionPlainInsertText = "f00";        // present in left only
   R.CompletionSnippetInsertText = "f0{$1:0}"; // present in right only
   Symbol::Details DetL, DetR;
   DetL.CompletionDetail = "DetL";
   DetR.CompletionDetail = "DetR";
   DetR.Documentation = "--doc--";
   L.Detail = &DetL;
   R.Detail = &DetR;
 
   Symbol::Details Scratch;
   Symbol M = mergeSymbol(L, R, &Scratch);
   EXPECT_EQ(M.Name, "Foo");
-  EXPECT_EQ(M.CanonicalDeclaration.FilePath, "left.h");
+  EXPECT_EQ(M.CanonicalDeclaration.FileUri, "file:///left.h");
   EXPECT_EQ(M.CompletionPlainInsertText, "f00");
   EXPECT_EQ(M.CompletionSnippetInsertText, "f0{$1:0}");
   ASSERT_TRUE(M.Detail);
   EXPECT_EQ(M.Detail->CompletionDetail, "DetL");
   EXPECT_EQ(M.Detail->Documentation, "--doc--");
 }
 
 } // namespace
 } // namespace clangd
 } // namespace clang

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(CPath, P, "") { return arg.CanonicalDeclaration.FilePath == P; }
+MATCHER_P(Uri, P, "") { return arg.CanonicalDeclaration.FileUri == P; }
 MATCHER_P(LocationOffsets, Offsets, "") {
   // Offset range in SymbolLocation is [start, end] while in Clangd is [start,
   // end).
   return arg.CanonicalDeclaration.StartOffset == Offsets.first &&
       arg.CanonicalDeclaration.EndOffset == Offsets.second - 1;
 }
 
 namespace clang {
 namespace clangd {
 
 namespace {
-const char TestHeaderName[] = "symbols.h";
-const char TestFileName[] = "symbol.cc";
 class SymbolIndexActionFactory : public tooling::FrontendActionFactory {
 public:
   SymbolIndexActionFactory(SymbolCollector::Options COpts)
       : COpts(std::move(COpts)) {}
 
   clang::FrontendAction *create() override {
     index::IndexingOptions IndexOpts;
     IndexOpts.SystemSymbolFilter =
         index::IndexingOptions::SystemSymbolFilterKind::All;
     IndexOpts.IndexFunctionLocals = false;
     Collector = std::make_shared<SymbolCollector>(COpts);
     FrontendAction *Action =
         index::createIndexingAction(Collector, IndexOpts, nullptr).release();
     return Action;
   }
 
   std::shared_ptr<SymbolCollector> Collector;
   SymbolCollector::Options COpts;
 };
 
 class SymbolCollectorTest : public ::testing::Test {
 public:
+  SymbolCollectorTest()
+      : TestHeaderName(getVirtualTestFilePath("symbol.h").str()),
+        TestFileName(getVirtualTestFilePath("symbol.cc").str()) {
+    TestHeaderUri = URI::createFile(TestHeaderName).toString();
+    TestFileUri = URI::createFile(TestFileName).toString();
+  }
+
   bool runSymbolCollector(StringRef HeaderCode, StringRef MainCode) {
     llvm::IntrusiveRefCntPtr<vfs::InMemoryFileSystem> InMemoryFileSystem(
         new vfs::InMemoryFileSystem);
     llvm::IntrusiveRefCntPtr<FileManager> Files(
         new FileManager(FileSystemOptions(), InMemoryFileSystem));
 
     auto Factory = llvm::make_unique<SymbolIndexActionFactory>(CollectorOpts);
 
     tooling::ToolInvocation Invocation(
         {"symbol_collector", "-fsyntax-only", "-std=c++11", TestFileName},
         Factory->create(), Files.get(),
         std::make_shared<PCHContainerOperations>());
 
     InMemoryFileSystem->addFile(TestHeaderName, 0,
                                 llvm::MemoryBuffer::getMemBuffer(HeaderCode));
 
     std::string Content = MainCode;
     if (!HeaderCode.empty())
-      Content = "#include\"" + std::string(TestHeaderName) + "\"\n" + Content;
+      Content = (llvm::Twine("#include\"") +
+                 llvm::sys::path::filename(TestHeaderName) + "\"\n" + Content)
+                    .str();
     InMemoryFileSystem->addFile(TestFileName, 0,
                                 llvm::MemoryBuffer::getMemBuffer(Content));
     Invocation.run();
     Symbols = Factory->Collector->takeSymbols();
     return true;
   }
 
 protected:
+  std::string TestHeaderName;
+  std::string TestHeaderUri;
+  std::string TestFileName;
+  std::string TestFileUri;
   SymbolSlab Symbols;
   SymbolCollector::Options CollectorOpts;
 };
 
 TEST_F(SymbolCollectorTest, CollectSymbols) {
   CollectorOpts.IndexMainFiles = true;
   const std::string Header = R"(
     class Foo {
EXPECT_THAT(Symbols,
                    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"), CPath("symbols.h"))));
+              UnorderedElementsAre(AllOf(QName("Foo"), Uri(TestHeaderUri))));
 }
 
 TEST_F(SymbolCollectorTest, SymbolRelativeWithFallback) {
   CollectorOpts.IndexMainFiles = false;
+  TestHeaderName = "x.h";
+  TestFileName = "x.cpp";
+  TestHeaderUri =
+      URI::createFile(getVirtualTestFilePath(TestHeaderName)).toString();
   CollectorOpts.FallbackDir = getVirtualTestRoot();
   runSymbolCollector("class Foo {};", /*Main=*/"");
   EXPECT_THAT(Symbols,
-              UnorderedElementsAre(AllOf(
-                  QName("Foo"), CPath(getVirtualTestFilePath("symbols.h")))));
+              UnorderedElementsAre(AllOf(QName("Foo"), Uri(TestHeaderUri))));
 }
 
 TEST_F(SymbolCollectorTest, IncludeEnums) {
   CollectorOpts.IndexMainFiles = false;
   const std::string Header = R"(
     enum {
       Red
     };
     enum Color {
       Green
     };
     enum class Color2 {
Annotations Header(R"(
 
     #define FF2() \
       $spelling[[class Test {}]];
 
     FF2();
   )");
 
   runSymbolCollector(Header.code(), /*Main=*/"");
-  EXPECT_THAT(Symbols,
-              UnorderedElementsAre(
-                  AllOf(QName("abc_Test"),
-                        LocationOffsets(Header.offsetRange("expansion")),
-                        CPath(TestHeaderName)),
-                  AllOf(QName("Test"),
-                        LocationOffsets(Header.offsetRange("spelling")),
-                        CPath(TestHeaderName))));
+  EXPECT_THAT(
+      Symbols,
+                Uri(TestHeaderUri)),
+          AllOf(QName("Test"), LocationOffsets(Header.offsetRange("spelling")),
+                Uri(TestHeaderUri))));
 }
 
 TEST_F(SymbolCollectorTest, SymbolFormedFromMacroInMainFile) {
   CollectorOpts.IndexMainFiles = true;
 
   Annotations Main(R"(
     #define FF(name) \
       class name##_Test {};
 
     $expansion[[FF(abc)]];
 
     #define FF2() \
       $spelling[[class Test {}]];
 
     FF2();
   )");
   runSymbolCollector(/*Header=*/"", Main.code());
-  EXPECT_THAT(Symbols,
-              UnorderedElementsAre(
+  EXPECT_THAT(Symbols, UnorderedElementsAre(
                            AllOf(QName("abc_Test"),
                                  LocationOffsets(Main.offsetRange("expansion")),
-                        CPath(TestFileName)),
+                                 Uri(TestFileUri)),
                            AllOf(QName("Test"),
                                  LocationOffsets(Main.offsetRange("spelling")),
-                        CPath(TestFileName))));
+                                 Uri(TestFileUri))));
 }
 
 TEST_F(SymbolCollectorTest, IgnoreSymbolsInMainFile) {
   CollectorOpts.IndexMainFiles = false;
   const std::string Header = R"(
     class Foo {};
     void f1();
     inline void f2() {}

 Name:   'Foo1'
 Scope:   'clang::'
 SymInfo:
   Kind:            Function
   Lang:            Cpp
 CanonicalDeclaration:
   StartOffset:     0
   EndOffset:       1
-  FilePath:        /path/foo.h
+  FileUri:        file:///path/foo.h
 CompletionLabel:    'Foo1-label'
 CompletionFilterText:    'filter'
 CompletionPlainInsertText:    'plain'
 Detail:
   Documentation:    'Foo doc'
   CompletionDetail:    'int'
 ...
 )";
   const std::string YAML2 = R"(
 ---
 ID: 057557CEBF6E6B2DD437FBF60CC58F352D1DF858
 Name:   'Foo2'
 Scope:   'clang::'
 SymInfo:
   Kind:            Function
   Lang:            Cpp
 CanonicalDeclaration:
   StartOffset:     10
   EndOffset:       12
-  FilePath:        /path/foo.h
+  FileUri:        file:///path/bar.h
 CompletionLabel:    'Foo2-label'
 CompletionFilterText:    'filter'
 CompletionPlainInsertText:    'plain'
 CompletionSnippetInsertText:    'snippet'
 ...
 )";
 
   auto Symbols1 = SymbolFromYAML(YAML1);
-  EXPECT_THAT(Symbols1, UnorderedElementsAre(
-                            AllOf(QName("clang::Foo1"), Labeled("Foo1-label"),
-                                  Doc("Foo doc"), Detail("int"))));
+  EXPECT_THAT(Symbols1,
+              UnorderedElementsAre(AllOf(
+                  QName("clang::Foo1"), Labeled("Foo1-label"), Doc("Foo doc"),
+                  Detail("int"), Uri("file:///path/foo.h"))));
   auto Symbols2 = SymbolFromYAML(YAML2);
-  EXPECT_THAT(Symbols2, UnorderedElementsAre(AllOf(QName("clang::Foo2"),
-                                                   Labeled("Foo2-label"),
-                                                   Not(HasDetail()))));
+  EXPECT_THAT(Symbols2, UnorderedElementsAre(AllOf(
+                            QName("clang::Foo2"), Labeled("Foo2-label"),
+                            Not(HasDetail()), Uri("file:///path/bar.h"))));
 
   std::string ConcatenatedYAML =
       SymbolsToYAML(Symbols1) + SymbolsToYAML(Symbols2);
   auto ConcatenatedSymbols = SymbolFromYAML(ConcatenatedYAML);
   EXPECT_THAT(ConcatenatedSymbols,
               UnorderedElementsAre(QName("clang::Foo1"),
                                    QName("clang::Foo2")));
 }
 
 } // namespace
 } // namespace clangd
 } // namespace clang