[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 ↗

(On Diff #151749)

why validate it?

clangd/FindSymbols.h

30 ↗

(On Diff #151749)

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

30 ↗

(On Diff #151749)

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

unittests/clangd/TestFS.cpp

70 ↗

(On Diff #151749)

is a path relative to testRoot()

93 ↗

(On Diff #151749)

why this change?

[ioeric]

• Address review comments

clangd/ClangdServer.cpp

119 ↗

(On Diff #151749)

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

unittests/clangd/TestFS.cpp

93 ↗

(On Diff #151749)

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

93 ↗

(On Diff #151749)

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

clang-tools-extra/trunk/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));
 }

clang-tools-extra/trunk/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

clang-tools-extra/trunk/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));

clang-tools-extra/trunk/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;
 

clang-tools-extra/trunk/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;

clang-tools-extra/trunk/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);