[clangd] Use workspace root path as hint path for resolving URIs in workspace/symbol

[ioeric]

Summary

Some URI schemes require a hint path to be provided, and workspace root
path seems to be a good fit.

[sammccall]

clangd/ClangdServer.cpp

119

why validate it?

clangd/FindSymbols.h

30

Change to HintPath? There's no reason to couple this code to the fact that it's the workspace root in clangdserver.

30

you sometimes pass "" here, so we should document the semantics of that

unittests/clangd/TestFS.cpp

70

is a path relative to testRoot()

98

why this change?

[ioeric]

• Address review comments

clangd/ClangdServer.cpp

119

This is the current behavior, except llvm::sys::fs::is_directory only works for real file system.

unittests/clangd/TestFS.cpp

98

The consume_front above would not consume the second slash in /clangd-test/x.h which results in "/x.h" in the body, but that doesn't work with testPath() which requires a relative path. That's why I added a requirement that the body should be relative path.

[sammccall]

unittests/clangd/TestFS.cpp

98

can we switch back to that path being represented by unittest:/x.h rather than unittest:x.h?
That looks more like the URI schemes we use in practice.

[ioeric]

• Require '/' in front of unittest: body

clangd/ClangdServer.cpp

if (FileIdx && Opts.StaticIndex) {
     Index = FileIdx.get();
   else if (Opts.StaticIndex)
     Index = Opts.StaticIndex;
   else
     Index = nullptr;
 }
 
 void ClangdServer::setRootPath(PathRef RootPath) {
-  std::string NewRootPath = llvm::sys::path::convert_to_slash(
-      RootPath, llvm::sys::path::Style::posix);
-  if (llvm::sys::fs::is_directory(NewRootPath))
-    this->RootPath = NewRootPath;
+  auto FS = FSProvider.getFileSystem();
+  auto Status = FS->status(RootPath);
+  if (!Status)
+    log("Failed to get status for RootPath " + RootPath + ": " +
+        Status.getError().message());
+  else if (Status->isDirectory())
+    this->RootPath = RootPath;
+  else
+    log("The provided RootPath " + RootPath + " is not a directory.");
 }
 
 void ClangdServer::addDocument(PathRef File, StringRef Contents,
                                WantDiagnostics WantDiags) {
   DocVersion Version = ++InternalVersion[File];
   ParseInputs Inputs = {getCompileCommand(File), FSProvider.getFileSystem(),
                         Contents.str()};
 

 
 void ClangdServer::onFileEvent(const DidChangeWatchedFilesParams &Params) {
   // FIXME: Do nothing for now. This will be used for indexing and potentially
   // invalidating other caches.
 }
 
 void ClangdServer::workspaceSymbols(
     StringRef Query, int Limit, Callback<std::vector<SymbolInformation>> CB) {
-  CB(clangd::getWorkspaceSymbols(Query, Limit, Index));
+  CB(clangd::getWorkspaceSymbols(Query, Limit, Index,
+                                 RootPath ? *RootPath : ""));
 }
 
 std::vector<std::pair<Path, std::size_t>>
 ClangdServer::getUsedBytesPerFile() const {
   return WorkScheduler.getUsedBytesPerFile();
 }
 
 LLVM_NODISCARD bool
 ClangdServer::blockUntilIdleForTest(llvm::Optional<double> TimeoutSeconds) {
   return WorkScheduler.blockUntilIdle(timeoutSeconds(TimeoutSeconds));
 }

clangd/FindSymbols.h

 
 /// Searches for the symbols matching \p Query. The syntax of \p Query can be
 /// the non-qualified name or fully qualified of a symbol. For example, "vector"
 /// will match the symbol std::vector and "std::vector" would also match it.
 /// Direct children of scopes (namepaces, etc) can be listed with a trailing
 /// "::". For example, "std::" will list all children of the std namespace and
 /// "::" alone will list all children of the global namespace.
 /// \p Limit limits the number of results returned (0 means no limit).
+/// \p HintPath This is used when resolving URIs. If empty, URI resolution can
+/// fail if a hint path is required for the scheme of a specific URI.
 llvm::Expected<std::vector<SymbolInformation>>
 getWorkspaceSymbols(llvm::StringRef Query, int Limit,
-                    const SymbolIndex *const Index);
+                    const SymbolIndex *const Index, llvm::StringRef HintPath);
 
 } // namespace clangd
 } // namespace clang
 
 #endif

clangd/FindSymbols.cpp

if (L.first != R.first)
       return L.first > R.first;
     return L.second.name < R.second.name; // Earlier name is better.
   }
 };
 
 } // namespace
 
 llvm::Expected<std::vector<SymbolInformation>>
-getWorkspaceSymbols(StringRef Query, int Limit,
-                    const SymbolIndex *const Index) {
+getWorkspaceSymbols(StringRef Query, int Limit, const SymbolIndex *const Index,
+                    StringRef HintPath) {
   std::vector<SymbolInformation> Result;
   if (Query.empty() || !Index)
     return Result;
 
   auto Names = splitQualifiedName(Query);
 
   FuzzyFindRequest Req;
   Req.Query = Names.second;
 
   // FuzzyFind doesn't want leading :: qualifier
   bool IsGlobalQuery = Names.first.consume_front("::");
   // Restrict results to the scope in the query string if present (global or
   // not).
   if (IsGlobalQuery || !Names.first.empty())
     Req.Scopes = {Names.first};
   if (Limit)
     Req.MaxCandidateCount = Limit;
   TopN<ScoredSymbolInfo, ScoredSymbolGreater> Top(Req.MaxCandidateCount);
   FuzzyMatcher Filter(Req.Query);
-  Index->fuzzyFind(Req, [&Top, &Filter](const Symbol &Sym) {
+  Index->fuzzyFind(Req, [HintPath, &Top, &Filter](const Symbol &Sym) {
     // Prefer the definition over e.g. a function declaration in a header
     auto &CD = Sym.Definition ? Sym.Definition : Sym.CanonicalDeclaration;
     auto Uri = URI::parse(CD.FileURI);
     if (!Uri) {
       log(llvm::formatv(
           "Workspace symbol: Could not parse URI '{0}' for symbol '{1}'.",
           CD.FileURI, Sym.Name));
       return;
     }
-    // FIXME: Passing no HintPath here will work for "file" and "test" schemes
-    // because they don't use it but this might not work for other custom ones.
-    auto Path = URI::resolve(*Uri);
+    auto Path = URI::resolve(*Uri, HintPath);
     if (!Path) {
       log(llvm::formatv("Workspace symbol: Could not resolve path for URI "
                         "'{0}' for symbol '{1}'.",
                         (*Uri).toString(), Sym.Name.str()));
       return;
     }
     Location L;
     L.uri = URIForFile((*Path));

unittests/clangd/FindSymbolsTests.cpp

 MATCHER_P(InContainer, ContainerName, "") {
   return arg.containerName == ContainerName;
 }
 MATCHER_P(WithKind, Kind, "") { return arg.kind == Kind; }
 
 ClangdServer::Options optsForTests() {
   auto ServerOpts = ClangdServer::optsForTest();
   ServerOpts.BuildDynamicSymbolIndex = true;
+  ServerOpts.URISchemes = {"unittest", "file"};
   return ServerOpts;
 }
 
 class WorkspaceSymbolsTest : public ::testing::Test {
 public:
   WorkspaceSymbolsTest()
-      : Server(CDB, FSProvider, DiagConsumer, optsForTests()) {}
+      : Server(CDB, FSProvider, DiagConsumer, optsForTests()) {
+    // Make sure the test root directory is created.
+    FSProvider.Files[testPath("unused")] = "";
+    Server.setRootPath(testRoot());
+  }
 
 protected:
   MockFSProvider FSProvider;
   MockCompilationDatabase CDB;
   IgnoreDiagnostics DiagConsumer;
   ClangdServer Server;
   int Limit = 0;
 

unittests/clangd/TestFS.cpp

std::string testPath(PathRef File) {
 
   SmallString<32> NativeFile = File;
   sys::path::native(NativeFile);
   SmallString<32> Path;
   sys::path::append(Path, testRoot(), NativeFile);
   return Path.str();
 }
 
-/// unittest: is a scheme that refers to files relative to testRoot()
+/// unittest: is a scheme that refers to files relative to testRoot().
+/// URI body is a path relative to testRoot() e.g. unittest:///x.h for
+/// /clangd-test/x.h.
 class TestScheme : public URIScheme {
 public:
   static const char *Scheme;
 
   llvm::Expected<std::string>
   getAbsolutePath(llvm::StringRef /*Authority*/, llvm::StringRef Body,
                   llvm::StringRef HintPath) const override {
     assert(HintPath.startswith(testRoot()));
+    if (!Body.consume_front("/"))
+      return llvm::make_error<llvm::StringError>(
+          "Body of an unittest: URI must start with '/'",
+          llvm::inconvertibleErrorCode());
     llvm::SmallString<16> Path(Body.begin(), Body.end());
     llvm::sys::path::native(Path);
     return testPath(Path);
   }
 
   llvm::Expected<URI>
   uriFromAbsolutePath(llvm::StringRef AbsolutePath) const override {
     llvm::StringRef Body = AbsolutePath;
     if (!Body.consume_front(testRoot()))
       return llvm::make_error<llvm::StringError>(
           AbsolutePath + "does not start with " + testRoot(),
           llvm::inconvertibleErrorCode());
 
     return URI(Scheme, /*Authority=*/"",
                llvm::sys::path::convert_to_slash(Body));
   }
 };
 
 const char *TestScheme::Scheme = "unittest";
 
 static URISchemeRegistry::Add<TestScheme> X(TestScheme::Scheme, "Test schema");
 
 volatile int UnittestSchemeAnchorSource = 0;
 
 } // namespace clangd
 } // namespace clang

unittests/clangd/URITests.cpp

#ifdef _WIN32
   EXPECT_THAT(resolveOrDie(parseOrDie("file:///c%3a/x/y/z")), "c:\\x\\y\\z");
 #else
   EXPECT_EQ(resolveOrDie(parseOrDie("file:/a/b/c")), "/a/b/c");
   EXPECT_EQ(resolveOrDie(parseOrDie("file://auth/a/b/c")), "/a/b/c");
   EXPECT_THAT(resolveOrDie(parseOrDie("file://au%3dth/%28x%29/y/%20z")),
               "/(x)/y/ z");
   EXPECT_THAT(resolveOrDie(parseOrDie("file:///c:/x/y/z")), "c:/x/y/z");
 #endif
-  EXPECT_EQ(resolveOrDie(parseOrDie("unittest:a"), testPath("x")),
+  EXPECT_EQ(resolveOrDie(parseOrDie("unittest:///a"), testPath("x")),
             testPath("a"));
 }
 
 TEST(URITest, Platform) {
   auto Path = testPath("x");
   auto U = URI::create(Path, "file");
   EXPECT_TRUE(static_cast<bool>(U));
   EXPECT_THAT(resolveOrDie(*U), Path);