[clangd] Canonicalize file path in URIForFile.

[ioeric]

Summary

File paths in URIForFile can come from index or local AST. Path from
index goes through URI transformation and the final path is resolved by URI
scheme and could be potentially different from the original path. Hence, we
should do the same transformation for all paths. We do this in URIForFile, which
now converts a path to URI and back to a canonicalized path.

[sammccall]

clangd/Protocol.h

73

We need to document the APIs and motivation, these are subtle.

74

I think it's no longer sufficiently clear what this constructor does - it should be a named factory function, and the HintPath should be required.
(If it's easy to implicitly skip canonicalization, then there's no advantage of changing URIForFile vs just asking people to canonicalize the inputs)

e.g. URIForFile URIForFile::canonicalize(StringRef AbsPath, StringRef HintPath)

74

As we're exposing HintPath in more far-flung places, its semantics are becoming less obvious.

I think we should make them more concrete, e.g. rename to TUPath and doc like

Files can be referred to by several paths (e.g. in the presence of links).
Which one we prefer may depend on where we're coming from.
TUPath is a hint, and should usually be the main entrypoint file we're processing.

98

Are there really *no* cases where the caller doesn't need canonicalization?

clangd/URI.cpp

235

this seems pretty indirect - including the call to create(), this function special-cases File 3 times.

Seems a little cleaner to me to remove "file" from the registry, and repeat the loop here. Up to you.

[ioeric]

address comments and rebase

[ioeric]

Thanks for the review! PTAL

clangd/Protocol.h

74

Sounds good. Thanks!

98

As chatted offline, there seems to be no cases where we don
t need canonicalization. We didn't do this in unit test, but we should do via matchers.

[sammccall]

Thanks!
Mostly just places that should be updated HintPath -> TUPath. Changing the whole codebase doesn't seem important, but in places that are touched...

clangd/Protocol.cpp

42

TUPath

62–65

maybe more explicit: input should be "file" or "test" scheme, which do not...

clangd/Protocol.h

87

TUPath

clangd/URI.cpp

227

TUPath

234

TUPath

clangd/URI.h

65

TUPath (and in comment)

72

TUPath

unittests/clangd/TestFS.cpp

92

TUPath

unittests/clangd/URITests.cpp

155

TUPath

[ioeric]

In D54845#1309589, @sammccall wrote:

Mostly just places that should be updated HintPath -> TUPath. Changing the whole codebase doesn't seem important, but in places that are touched...

As chatted offline, URIForFile is closer to ClangdServer, so TUPath is a good fit. But URI is more generic, and HintPath still seems to be a good fit there.

clangd/FindSymbols.cpp

Index->fuzzyFind(Req, [HintPath, &Top, &Filter](const Symbol &Sym) {
     auto Path = URI::resolve(*Uri, HintPath);
     if (!Path) {
       log("Workspace symbol: Could not resolve path for URI '{0}' for symbol "
           "'{1}'.",
           Uri->toString(), Sym.Name);
       return;
     }
     Location L;
-    L.uri = URIForFile((*Path));
+    L.uri = URIForFile(*Path, HintPath);
     Position Start, End;
     Start.line = CD.Start.line();
     Start.character = CD.Start.column();
     End.line = CD.End.line();
     End.character = CD.End.column();
     L.range = {Start, End};
     SymbolKind SK = indexSymbolKindToSymbolKind(Sym.SymInfo.Kind);
     std::string Scope = Sym.Scope;
public:
 
   void initialize(ASTContext &Ctx) override {
     // Compute the absolute path of the main file which we will use for all
     // results.
     const SourceManager &SM = AST.getSourceManager();
     const FileEntry *F = SM.getFileEntryForID(SM.getMainFileID());
     if (!F)
       return;
-    auto FilePath = getRealPath(F, SM);
-    if (FilePath)
-      MainFileUri = URIForFile(*FilePath);
+    auto MainFilePath = getRealPath(F, SM);
+    if (MainFilePath)
+      MainFileUri = URIForFile(*MainFilePath);
   }
 
   bool shouldIncludeSymbol(const NamedDecl *ND) {
     if (!ND || ND->isImplicit())
       return false;
     // Skip anonymous declarations, e.g (anonymous enum/class/struct).
     if (ND->getDeclName().isEmpty())
       return false;

clangd/Protocol.h

public:
   void log(llvm::raw_ostream &OS) const override {
     OS << int(Code) << ": " << Message;
   }
   std::error_code convertToErrorCode() const override {
     return llvm::inconvertibleErrorCode();
   }
 };
 
+// URI in "file" scheme for a file.
 struct URIForFile {
   URIForFile() = default;
-  explicit URIForFile(std::string AbsPath);
+
+  /// \p AbsPath is resolved to a canonical path corresponding to its URI.
+  URIForFile(llvm::StringRef AbsPath, llvm::StringRef HintPath = "");
+
+  static llvm::Expected<URIForFile> fromURI(const URI &U,
+                                            llvm::StringRef HintPath);
 
   /// Retrieves absolute path to the file.
   llvm::StringRef file() const { return File; }
 
   explicit operator bool() const { return !File.empty(); }
   std::string uri() const { return URI::createFile(File).toString(); }
 
   friend bool operator==(const URIForFile &LHS, const URIForFile &RHS) {
     return LHS.File == RHS.File;
   }
 
   friend bool operator!=(const URIForFile &LHS, const URIForFile &RHS) {
     return !(LHS == RHS);
   }
 
   friend bool operator<(const URIForFile &LHS, const URIForFile &RHS) {
     return LHS.File < RHS.File;
   }
 
 private:
+  explicit URIForFile(std::string &&File) : File(std::move(File)) {}
+
   std::string File;
 };
 
 /// Serialize/deserialize \p URIForFile to/from a string URI.
 llvm::json::Value toJSON(const URIForFile &U);
 bool fromJSON(const llvm::json::Value &, URIForFile &);
 
 struct TextDocumentIdentifier {

clangd/Protocol.cpp

 #include "llvm/Support/raw_ostream.h"
 
 using namespace llvm;
 namespace clang {
 namespace clangd {
 
 char LSPError::ID;
 
-URIForFile::URIForFile(std::string AbsPath) {
+URIForFile::URIForFile(StringRef AbsPath, StringRef HintPath) {
   assert(sys::path::is_absolute(AbsPath) && "the path is relative");
-  File = std::move(AbsPath);
+  auto Resolved = URI::resolvePath(AbsPath, HintPath);
+  if (!Resolved) {
+    elog("URIForFile: failed to resolve path {0} with hint path {1}: "
+         "{2}.\nUsing unresolved path.",
+         AbsPath, HintPath, Resolved.takeError());
+    File = AbsPath;
+  } else {
+    File = *Resolved;
+  }
+}
+
+Expected<URIForFile> URIForFile::fromURI(const URI &U, StringRef HintPath) {
+  auto Resolved = URI::resolve(U, HintPath);
+  if (!Resolved)
+    return Resolved.takeError();
+  return URIForFile(std::move(*Resolved));
 }
 
 bool fromJSON(const json::Value &E, URIForFile &R) {
   if (auto S = E.getAsString()) {
-    auto U = URI::parse(*S);
-    if (!U) {
-      elog("Failed to parse URI {0}: {1}", *S, U.takeError());
+    auto Parsed = URI::parse(*S);
+    if (!Parsed) {
+      elog("Failed to parse URI {0}: {1}", *S, Parsed.takeError());
       return false;
     }
-    if (U->scheme() != "file" && U->scheme() != "test") {
+    if (Parsed->scheme() != "file" && Parsed->scheme() != "test") {
       elog("Clangd only supports 'file' URI scheme for workspace files: {0}",
            *S);
       return false;
     }
-    auto Path = URI::resolve(*U);
-    if (!Path) {
-      log("{0}", Path.takeError());
+    // "file" and "test" schemes do not require hint path.
+    auto U = URIForFile::fromURI(*Parsed, /*HintPath=*/"");
+    if (!U) {
+      elog("{0}", U.takeError());
       return false;
     }
-    R = URIForFile(*Path);
+    R = std::move(*U);
     return true;
   }
   return false;
 }
 
 json::Value toJSON(const URIForFile &U) { return U.uri(); }
 
 raw_ostream &operator<<(raw_ostream &OS, const URIForFile &U) {

clangd/URI.h

public:
   static llvm::Expected<URI> parse(llvm::StringRef Uri);
 
   /// Resolves the absolute path of \p U. If there is no matching scheme, or the
   /// URI is invalid in the scheme, this returns an error.
   ///
   /// \p HintPath A related path, such as the current file or working directory,
   /// which can help disambiguate when the same file exists in many workspaces.
   static llvm::Expected<std::string> resolve(const URI &U,
                                              llvm::StringRef HintPath = "");
 
+  /// Resolves \p AbsPath into a canonical path of its URI, by converting
+  /// \p AbsPath to URI and resolving the URI to get th canonical path.
+  /// This ensures that paths with the same URI are resolved into consistent
+  /// file path.
+  static llvm::Expected<std::string> resolvePath(llvm::StringRef AbsPath,
+                                                 llvm::StringRef HintPath = "");
+
   /// Gets the preferred spelling of this file for #include, if there is one,
   /// e.g. <system_header.h>, "path/to/x.h".
   ///
   /// This allows URI schemas to provide their customized include paths.
   ///
   /// Returns an empty string if normal include-shortening based on the absolute
   /// path should be used.
   /// Fails if the URI is not valid in the schema.

clangd/URI.cpp

 
 URI URI::createFile(StringRef AbsolutePath) {
   auto U = create(AbsolutePath, "file");
   if (!U)
     llvm_unreachable(llvm::toString(U.takeError()).c_str());
   return std::move(*U);
 }
 
 Expected<std::string> URI::resolve(const URI &Uri, StringRef HintPath) {
   auto S = findSchemeByName(Uri.Scheme);
   if (!S)
     return S.takeError();
   return S->get()->getAbsolutePath(Uri.Authority, Uri.Body, HintPath);
 }
 
+Expected<std::string> URI::resolvePath(StringRef AbsPath, StringRef HintPath) {
+  auto U = create(AbsPath);
+  // "file" scheme doesn't do anything fancy; it would resolve to the same
+  // \p AbsPath.
+  return U.scheme() == "file" ? AbsPath : resolve(U, HintPath);
+}
+
 Expected<std::string> URI::includeSpelling(const URI &Uri) {
   auto S = findSchemeByName(Uri.Scheme);
   if (!S)
     return S.takeError();
   return S->get()->getIncludeSpelling(Uri);
 }
 
 } // namespace clangd
 } // namespace clang

clangd/XRefs.cpp

 // FIXME: figure out a good home for it, and share the implementation with
 // FindSymbols.
 Optional<Location> toLSPLocation(const SymbolLocation &Loc,
                                  StringRef HintPath) {
   if (!Loc)
     return None;
   auto Uri = URI::parse(Loc.FileURI);
   if (!Uri) {
-    log("Could not parse URI: {0}", Loc.FileURI);
+    elog("Could not parse URI {0}: {1}", Loc.FileURI, Uri.takeError());
     return None;
   }
-  auto Path = URI::resolve(*Uri, HintPath);
-  if (!Path) {
-    log("Could not resolve URI: {0}", Loc.FileURI);
+  auto U = URIForFile::fromURI(*Uri, HintPath);
+  if (!U) {
+    elog("Could not resolve URI {0}: {1}", Loc.FileURI, U.takeError());
     return None;
   }
+
   Location LSPLoc;
-  LSPLoc.uri = URIForFile(*Path);
+  LSPLoc.uri = std::move(*U);
   LSPLoc.range.start.line = Loc.Start.line();
   LSPLoc.range.start.character = Loc.Start.column();
   LSPLoc.range.end.line = Loc.End.line();
   LSPLoc.range.end.character = Loc.End.column();
   logIfOverflow(Loc);
   return LSPLoc;
 }
 

 Range getTokenRange(ParsedAST &AST, SourceLocation TokLoc) {
   const SourceManager &SourceMgr = AST.getASTContext().getSourceManager();
   SourceLocation LocEnd = Lexer::getLocForEndOfToken(
       TokLoc, 0, SourceMgr, AST.getASTContext().getLangOpts());
   return {sourceLocToPosition(SourceMgr, TokLoc),
           sourceLocToPosition(SourceMgr, LocEnd)};
 }
 
-Optional<Location> makeLocation(ParsedAST &AST, SourceLocation TokLoc) {
+Optional<Location> makeLocation(ParsedAST &AST, SourceLocation TokLoc,
+                                StringRef HintPath) {
   const SourceManager &SourceMgr = AST.getASTContext().getSourceManager();
   const FileEntry *F = SourceMgr.getFileEntryForID(SourceMgr.getFileID(TokLoc));
   if (!F)
     return None;
   auto FilePath = getRealPath(F, SourceMgr);
   if (!FilePath) {
     log("failed to get path!");
     return None;
   }
   Location L;
-  L.uri = URIForFile(*FilePath);
+  L.uri = URIForFile(*FilePath, HintPath);
   L.range = getTokenRange(AST, TokLoc);
   return L;
 }
 
 } // namespace
 
 std::vector<Location> findDefinitions(ParsedAST &AST, Position Pos,
                                       const SymbolIndex *Index) {
-  const SourceManager &SourceMgr = AST.getASTContext().getSourceManager();
+  const auto &SM = AST.getASTContext().getSourceManager();
+  auto MainFilePath = getRealPath(SM.getFileEntryForID(SM.getMainFileID()), SM);
+  if (!MainFilePath) {
+    elog("Failed to get a path for the main file, so no references");
+    return {};
+  }
 
   std::vector<Location> Result;
   // Handle goto definition for #include.
   for (auto &Inc : AST.getIncludeStructure().MainFileIncludes) {
     if (!Inc.Resolved.empty() && Inc.R.start.line == Pos.line)
-      Result.push_back(Location{URIForFile{Inc.Resolved}, {}});
+      Result.push_back(Location{URIForFile(Inc.Resolved, *MainFilePath), {}});
   }
   if (!Result.empty())
     return Result;
 
   // Identified symbols at a specific position.
   SourceLocation SourceLocationBeg =
-      getBeginningOfIdentifier(AST, Pos, SourceMgr.getMainFileID());
+      getBeginningOfIdentifier(AST, Pos, SM.getMainFileID());
   auto Symbols = getSymbolAtPosition(AST, SourceLocationBeg);
 
   for (auto Item : Symbols.Macros) {
     auto Loc = Item.Info->getDefinitionLoc();
-    auto L = makeLocation(AST, Loc);
+    auto L = makeLocation(AST, Loc, *MainFilePath);
     if (L)
       Result.push_back(*L);
   }
 
   // Declaration and definition are different terms in C-family languages, and
   // LSP only defines the "GoToDefinition" specification, so we try to perform
   // the "most sensible" GoTo operation:
   //
if (auto ID = getSymbolID(D))
       Key = *ID;
 
     auto R = CandidatesIndex.try_emplace(Key, ResultCandidates.size());
     if (R.second) // new entry
       ResultCandidates.emplace_back();
     auto &Candidate = ResultCandidates[R.first->second];
 
     auto Loc = findNameLoc(D);
-    auto L = makeLocation(AST, Loc);
+    auto L = makeLocation(AST, Loc, *MainFilePath);
     // The declaration in the identified symbols is a definition if possible
     // otherwise it is declaration.
     bool IsDef = getDefinition(D) == D;
     // Populate one of the slots with location for the AST.
     if (!IsDef)
       Candidate.Decl = L;
     else
       Candidate.Def = L;
   }
 
   if (Index) {
     LookupRequest QueryRequest;
     // Build request for index query, using SymbolID.
     for (auto It : CandidatesIndex)
       QueryRequest.IDs.insert(It.first);
     std::string HintPath;
     const FileEntry *FE =
-        SourceMgr.getFileEntryForID(SourceMgr.getMainFileID());
-    if (auto Path = getRealPath(FE, SourceMgr))
+        SM.getFileEntryForID(SM.getMainFileID());
+    if (auto Path = getRealPath(FE, SM))
       HintPath = *Path;
     // Query the index and populate the empty slot.
     Index->lookup(QueryRequest, [&HintPath, &ResultCandidates,
                                  &CandidatesIndex](const Symbol &Sym) {
       auto It = CandidatesIndex.find(Sym.ID);
       assert(It != CandidatesIndex.end());
       auto &Value = ResultCandidates[It->second];
 

unittests/clangd/ClangdTests.cpp

)cpp");
   ASSERT_TRUE(Server.blockUntilIdleForTest());
 
   EXPECT_THAT(DiagConsumer.filesWithDiags(),
               UnorderedElementsAre(Pair(FooCpp, false), Pair(BarCpp, true),
                                    Pair(BazCpp, false)));
 
   auto Locations = runFindDefinitions(Server, FooCpp, FooSource.point());
   EXPECT_TRUE(bool(Locations));
-  EXPECT_THAT(*Locations, ElementsAre(Location{URIForFile{FooCpp},
+  EXPECT_THAT(*Locations, ElementsAre(Location{URIForFile(FooCpp),
                                                FooSource.range("one")}));
 
   // Undefine MACRO, close baz.cpp.
   CDB.ExtraClangFlags.clear();
   DiagConsumer.clear();
   Server.removeDocument(BazCpp);
   Server.addDocument(FooCpp, FooSource.code(), WantDiagnostics::Auto);
   Server.addDocument(BarCpp, BarSource.code(), WantDiagnostics::Auto);
   ASSERT_TRUE(Server.blockUntilIdleForTest());
 
   EXPECT_THAT(DiagConsumer.filesWithDiags(),
               UnorderedElementsAre(Pair(FooCpp, false), Pair(BarCpp, false)));
 
   Locations = runFindDefinitions(Server, FooCpp, FooSource.point());
   EXPECT_TRUE(bool(Locations));
-  EXPECT_THAT(*Locations, ElementsAre(Location{URIForFile{FooCpp},
+  EXPECT_THAT(*Locations, ElementsAre(Location{URIForFile(FooCpp),
                                                FooSource.range("two")}));
 }
 
 TEST_F(ClangdVFSTest, MemoryUsage) {
   MockFSProvider FS;
   ErrorCheckingDiagConsumer DiagConsumer;
   MockCompilationDatabase CDB;
   ClangdServer Server(CDB, FS, DiagConsumer, ClangdServer::optsForTest());

unittests/clangd/ClangdUnitTests.cpp

 
 main.cpp:2:3: error: something terrible happened)");
 
   // Transform dianostics and check the results.
   std::vector<std::pair<clangd::Diagnostic, std::vector<clangd::Fix>>> LSPDiags;
   toLSPDiags(
       D,
 #ifdef _WIN32
-      URIForFile("c:\\path\\to\\foo\\bar\\main.cpp"),
+      URIForFile(StringRef("c:\\path\\to\\foo\\bar\\main.cpp")),
 #else
-      URIForFile("/path/to/foo/bar/main.cpp"),
+      URIForFile(StringRef("/path/to/foo/bar/main.cpp")),
 #endif
       ClangdDiagnosticOptions(),
       [&](clangd::Diagnostic LSPDiag, ArrayRef<clangd::Fix> Fixes) {
         LSPDiags.push_back(
             {std::move(LSPDiag),
              std::vector<clangd::Fix>(Fixes.begin(), Fixes.end())});
       });
 

unittests/clangd/TestFS.cpp

 /// 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;
 
   Expected<std::string> getAbsolutePath(StringRef /*Authority*/, StringRef Body,
                                         StringRef HintPath) const override {
-    assert(HintPath.startswith(testRoot()));
+    if (!HintPath.startswith(testRoot()))
+      return make_error<StringError>(
+          "Hint path doesn't start with test root: " + HintPath,
+          inconvertibleErrorCode());
     if (!Body.consume_front("/"))
       return make_error<StringError>(
           "Body of an unittest: URI must start with '/'",
           inconvertibleErrorCode());
     SmallString<16> Path(Body.begin(), Body.end());
     sys::path::native(Path);
     return testPath(Path);
   }

unittests/clangd/URITests.cpp

#else
   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")),
             testPath("a"));
 }
 
+std::string resolvePathOrDie(StringRef AbsPath, StringRef HintPath = "") {
+  auto Path = URI::resolvePath(AbsPath, HintPath);
+  if (!Path)
+    llvm_unreachable(toString(Path.takeError()).c_str());
+  return *Path;
+}
+
+TEST(URITest, ResolvePath) {
+  StringRef FilePath =
+#ifdef _WIN32
+      "c:\\x\\y\\z";
+#else
+      "/a/b/c";
+#endif
+  EXPECT_EQ(resolvePathOrDie(FilePath), FilePath);
+  EXPECT_EQ(resolvePathOrDie(testPath("x"), testPath("hint")), testPath("x"));
+  // HintPath is not in testRoot(); resolution fails.
+  auto Resolve = URI::resolvePath(testPath("x"), FilePath);
+  EXPECT_FALSE(Resolve);
+  llvm::consumeError(Resolve.takeError());
+}
+
 TEST(URITest, Platform) {
   auto Path = testPath("x");
   auto U = URI::create(Path, "file");
   EXPECT_TRUE(static_cast<bool>(U));
   EXPECT_THAT(resolveOrDie(*U), Path);
 }
 
 TEST(URITest, ResolveFailed) {

unittests/clangd/XRefsTests.cpp

)cpp");
   FS.Files[HeaderNotInPreambleH] = HeaderNotInPreambleAnnotations.code();
 
   runAddDocument(Server, FooCpp, SourceAnnotations.code());
 
   // Go to a definition in main source file.
   auto Locations =
       runFindDefinitions(Server, FooCpp, SourceAnnotations.point("p1"));
   EXPECT_TRUE(bool(Locations)) << "findDefinitions returned an error";
-  EXPECT_THAT(*Locations, ElementsAre(Location{URIForFile{FooCpp},
+  EXPECT_THAT(*Locations, ElementsAre(Location{URIForFile(FooCpp),
                                                SourceAnnotations.range()}));
 
   // Go to a definition in header_in_preamble.h.
   Locations = runFindDefinitions(Server, FooCpp, SourceAnnotations.point("p2"));
   EXPECT_TRUE(bool(Locations)) << "findDefinitions returned an error";
   EXPECT_THAT(*Locations,
-              ElementsAre(Location{URIForFile{HeaderInPreambleH},
+              ElementsAre(Location{URIForFile(HeaderInPreambleH),
                                    HeaderInPreambleAnnotations.range()}));
 
   // Go to a definition in header_not_in_preamble.h.
   Locations = runFindDefinitions(Server, FooCpp, SourceAnnotations.point("p3"));
   EXPECT_TRUE(bool(Locations)) << "findDefinitions returned an error";
   EXPECT_THAT(*Locations,
-              ElementsAre(Location{URIForFile{HeaderNotInPreambleH},
+              ElementsAre(Location{URIForFile(HeaderNotInPreambleH),
                                    HeaderNotInPreambleAnnotations.range()}));
 }
 
 TEST(Hover, All) {
   struct OneTest {
     StringRef Input;
     StringRef ExpectedHover;
   };
TEST(GoToInclude, All) {
   int b = a;
   // test
   int foo;
   #in$5^clude "$6^foo.h"$7^
   )cpp";
   Annotations SourceAnnotations(SourceContents);
   FS.Files[FooCpp] = SourceAnnotations.code();
   auto FooH = testPath("foo.h");
-  auto FooHUri = URIForFile{FooH};
+  URIForFile FooHUri(FooH);
 
   const char *HeaderContents = R"cpp([[]]#pragma once
                                      int a;
                                      )cpp";
   Annotations HeaderAnnotations(HeaderContents);
   FS.Files[FooH] = HeaderAnnotations.code();
 
   Server.addDocument(FooH, HeaderAnnotations.code());