[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

74

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

75

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)

75

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.

109

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

clangd/URI.cpp

229

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

75

Sounds good. Thanks!

109

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

45

TUPath

64–67

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

clangd/Protocol.h

88

TUPath

clangd/URI.cpp

221

TUPath

228

TUPath

clangd/URI.h

65

TUPath (and in comment)

72

TUPath

unittests/clangd/TestFS.cpp

95

TUPath

unittests/clangd/URITests.cpp

140

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/ClangdLSPServer.cpp

std::vector<Fix> ClangdLSPServer::getFixes(StringRef File,
   if (FixItsIter == DiagToFixItsMap.end())
     return {};
 
   return FixItsIter->second;
 }
 
 void ClangdLSPServer::onDiagnosticsReady(PathRef File,
                                          std::vector<Diag> Diagnostics) {
-  URIForFile URI(File);
+  auto URI = URIForFile::canonicalize(File, /*TUPath=*/File);
   std::vector<Diagnostic> LSPDiagnostics;
   DiagnosticToReplacementMap LocalFixIts; // Temporary storage
   for (auto &Diag : Diagnostics) {
     toLSPDiags(Diag, URI, DiagOpts,
                [&](clangd::Diagnostic Diag, ArrayRef<Fix> Fixes) {
                  auto &FixItsForDiagnostic = LocalFixIts[Diag];
                  llvm::copy(Fixes, std::back_inserter(FixItsForDiagnostic));
                  LSPDiagnostics.push_back(std::move(Diag));

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));
+    // Use HintPath as TUPath since there is no TU associated with this
+    // request.
+    L.uri = URIForFile::canonicalize(*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;

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);
+
+  /// Canonicalizes \p AbsPath via URI.
+  ///
+  /// 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 potentially be different from the original path.
+  /// Hence, we do the same transformation for all paths.
+  ///
+  /// 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. \p TUPath is a
+  /// hint, and should usually be the main entrypoint file we're processing.
+  static URIForFile canonicalize(llvm::StringRef AbsPath,
+                                 llvm::StringRef TUPath);
+
+  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::canonicalize(StringRef AbsPath, StringRef TUPath) {
   assert(sys::path::is_absolute(AbsPath) && "the path is relative");
-  File = std::move(AbsPath);
+  auto Resolved = URI::resolvePath(AbsPath, TUPath);
+  if (!Resolved) {
+    elog("URIForFile: failed to resolve path {0} with TU path {1}: "
+         "{2}.\nUsing unresolved path.",
+         AbsPath, TUPath, Resolved.takeError());
+    return URIForFile(AbsPath);
+  }
+  return URIForFile(std::move(*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

 namespace {
 
 inline Error make_string_error(const Twine &Message) {
   return make_error<StringError>(Message, inconvertibleErrorCode());
 }
 
 /// \brief This manages file paths in the file system. All paths in the scheme
 /// are absolute (with leading '/').
+/// Note that this scheme is hardcoded into the library and not registered in
+/// registry.
 class FileSystemScheme : public URIScheme {
 public:
-  static const char *Scheme;
-
   Expected<std::string> getAbsolutePath(StringRef /*Authority*/, StringRef Body,
                                         StringRef /*HintPath*/) const override {
     if (!Body.startswith("/"))
       return make_string_error("File scheme: expect body to be an absolute "
                                "path starting with '/': " +
                                Body);
     // For Windows paths e.g. /X:
     if (Body.size() > 2 && Body[0] == '/' && Body[2] == ':')
       Body.consume_front("/");
     SmallVector<char, 16> Path(Body.begin(), Body.end());
     sys::path::native(Path);
     return std::string(Path.begin(), Path.end());
   }
 
   Expected<URI> uriFromAbsolutePath(StringRef AbsolutePath) const override {
     using namespace llvm::sys;
 
     std::string Body;
     // For Windows paths e.g. X:
     if (AbsolutePath.size() > 1 && AbsolutePath[1] == ':')
       Body = "/";
     Body += path::convert_to_slash(AbsolutePath);
-    return URI(Scheme, /*Authority=*/"", Body);
+    return URI("file", /*Authority=*/"", Body);
   }
 };
 
-const char *FileSystemScheme::Scheme = "file";
-
-static URISchemeRegistry::Add<FileSystemScheme>
-    X(FileSystemScheme::Scheme,
-      "URI scheme for absolute paths in the file system.");
-
 Expected<std::unique_ptr<URIScheme>> findSchemeByName(StringRef Scheme) {
+  if (Scheme == "file")
+    return make_unique<FileSystemScheme>();
+
   for (auto I = URISchemeRegistry::begin(), E = URISchemeRegistry::end();
        I != E; ++I) {
     if (I->getName() != Scheme)
       continue;
     return I->instantiate();
   }
   return make_string_error("Can't find scheme: " + Scheme);
 }
Expected<URI> URI::create(StringRef AbsolutePath, StringRef Scheme) {
   return S->get()->uriFromAbsolutePath(AbsolutePath);
 }
 
 URI URI::create(StringRef AbsolutePath) {
   if (!sys::path::is_absolute(AbsolutePath))
     llvm_unreachable(
         ("Not a valid absolute path: " + AbsolutePath).str().c_str());
   for (auto &Entry : URISchemeRegistry::entries()) {
-    if (Entry.getName() == "file")
-      continue;
-
     auto URI = Entry.instantiate()->uriFromAbsolutePath(AbsolutePath);
     // For some paths, conversion to different URI schemes is impossible. These
     // should be just skipped.
     if (!URI) {
       // Ignore the error.
       consumeError(URI.takeError());
       continue;
     }
     return std::move(*URI);
   }
   // Fallback to file: scheme which should work for any paths.
   return URI::createFile(AbsolutePath);
 }
 
 URI URI::createFile(StringRef AbsolutePath) {
-  auto U = create(AbsolutePath, "file");
+  auto U = FileSystemScheme().uriFromAbsolutePath(AbsolutePath);
   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) {
+  if (!sys::path::is_absolute(AbsPath))
+    llvm_unreachable(("Not a valid absolute path: " + AbsPath).str().c_str());
+  for (auto &Entry : URISchemeRegistry::entries()) {
+    auto S =  Entry.instantiate();
+    auto U = S->uriFromAbsolutePath(AbsPath);
+    // For some paths, conversion to different URI schemes is impossible. These
+    // should be just skipped.
+    if (!U) {
+      // Ignore the error.
+      consumeError(U.takeError());
+      continue;
+    }
+    return S->getAbsolutePath(U->Authority, U->Body, HintPath);
+  }
+  // Fallback to file: scheme which doesn't do any canonicalization.
+  return AbsPath;
+}
+
 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

 }
 
 void logIfOverflow(const SymbolLocation &Loc) {
   if (Loc.Start.hasOverflow() || Loc.End.hasOverflow())
     log("Possible overflow in symbol location: {0}", Loc);
 }
 
 // Convert a SymbolLocation to LSP's Location.
-// HintPath is used to resolve the path of URI.
+// TUPath is used to resolve the path of URI.
 // FIXME: figure out a good home for it, and share the implementation with
 // FindSymbols.
 Optional<Location> toLSPLocation(const SymbolLocation &Loc,
-                                 StringRef HintPath) {
+                                 StringRef TUPath) {
   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, TUPath);
+  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 TUPath) {
   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::canonicalize(*FilePath, TUPath);
   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::canonicalize(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;
+    std::string TUPath;
     const FileEntry *FE =
-        SourceMgr.getFileEntryForID(SourceMgr.getMainFileID());
-    if (auto Path = getRealPath(FE, SourceMgr))
-      HintPath = *Path;
+        SM.getFileEntryForID(SM.getMainFileID());
+    if (auto Path = getRealPath(FE, SM))
+      TUPath = *Path;
     // Query the index and populate the empty slot.
-    Index->lookup(QueryRequest, [&HintPath, &ResultCandidates,
+    Index->lookup(QueryRequest, [&TUPath, &ResultCandidates,
                                  &CandidatesIndex](const Symbol &Sym) {
       auto It = CandidatesIndex.find(Sym.ID);
       assert(It != CandidatesIndex.end());
       auto &Value = ResultCandidates[It->second];
 
       if (!Value.Def)
-        Value.Def = toLSPLocation(Sym.Definition, HintPath);
+        Value.Def = toLSPLocation(Sym.Definition, TUPath);
       if (!Value.Decl)
-        Value.Decl = toLSPLocation(Sym.CanonicalDeclaration, HintPath);
+        Value.Decl = toLSPLocation(Sym.CanonicalDeclaration, TUPath);
     });
   }
 
   // Populate the results, definition first.
   for (const auto &Candidate : ResultCandidates) {
     if (Candidate.Def)
       Result.push_back(*Candidate.Def);
     if (Candidate.Decl &&
std::vector<Location> findReferences(ParsedAST &AST, Position Pos,
   }
 
   // We traverse the AST to find references in the main file.
   // TODO: should we handle macros, too?
   auto MainFileRefs = findRefs(TargetDecls, AST);
   for (const auto &Ref : MainFileRefs) {
     Location Result;
     Result.range = getTokenRange(AST, Ref.Loc);
-    Result.uri = URIForFile(*MainFilePath);
+    Result.uri = URIForFile::canonicalize(*MainFilePath, *MainFilePath);
     Results.push_back(std::move(Result));
   }
 
   // Now query the index for references from other files.
   if (!Index)
     return Results;
   RefsRequest Req;
   for (const Decl *D : TargetDecls) {
     // Not all symbols can be referenced from outside (e.g. function-locals).
     // TODO: we could skip TU-scoped symbols here (e.g. static functions) if
     // we know this file isn't a header. The details might be tricky.
     if (D->getParentFunctionOrMethod())
       continue;
     if (auto ID = getSymbolID(D))
       Req.IDs.insert(*ID);
   }
   if (Req.IDs.empty())
     return Results;
   Index->refs(Req, [&](const Ref &R) {
-    auto LSPLoc = toLSPLocation(R.Location, /*HintPath=*/*MainFilePath);
+    auto LSPLoc = toLSPLocation(R.Location, *MainFilePath);
     // Avoid indexed results for the main file - the AST is authoritative.
     if (LSPLoc && LSPLoc->uri.file() != *MainFilePath)
       Results.push_back(std::move(*LSPLoc));
   });
   return Results;
 }
 
 std::vector<SymbolDetails> getSymbolInfo(ParsedAST &AST, Position Pos) {

unittests/clangd/ClangdTests.cpp

 #include <string>
 #include <thread>
 #include <vector>
 
 using namespace llvm;
 namespace clang {
 namespace clangd {
 
+namespace {
+
 using ::testing::ElementsAre;
 using ::testing::Eq;
 using ::testing::Field;
 using ::testing::Gt;
 using ::testing::IsEmpty;
 using ::testing::Pair;
 using ::testing::UnorderedElementsAre;
 
-namespace {
+MATCHER_P2(FileRange, File, Range, "") {
+  return Location{URIForFile::canonicalize(File, testRoot()), Range} == arg;
+}
 
 bool diagsContainErrors(const std::vector<Diag> &Diagnostics) {
   for (auto D : Diagnostics) {
     if (D.Severity == DiagnosticsEngine::Error ||
         D.Severity == DiagnosticsEngine::Fatal)
       return true;
   }
   return false;
)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},
-                                               FooSource.range("one")}));
+  EXPECT_THAT(*Locations,
+              ElementsAre(FileRange(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},
-                                               FooSource.range("two")}));
+  EXPECT_THAT(*Locations, ElementsAre(FileRange(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::canonicalize("c:\\path\\to\\foo\\bar\\main.cpp",
+                               /*TUPath=*/""),
 #else
-      URIForFile("/path/to/foo/bar/main.cpp"),
+      URIForFile::canonicalize("/path/to/foo/bar/main.cpp", /*TUPath=*/""),
 #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

 using testing::Matcher;
 using testing::UnorderedElementsAreArray;
 
 class IgnoreDiagnostics : public DiagnosticsConsumer {
   void onDiagnosticsReady(PathRef File,
                           std::vector<Diag> Diagnostics) override {}
 };
 
+MATCHER_P2(FileRange, File, Range, "") {
+  return Location{URIForFile::canonicalize(File, testRoot()), Range} == arg;
+}
+
 // Extracts ranges from an annotated example, and constructs a matcher for a
 // highlight set. Ranges should be named $read/$write as appropriate.
 Matcher<const std::vector<DocumentHighlight> &>
 HighlightsFrom(const Annotations &Test) {
   std::vector<DocumentHighlight> Expected;
   auto Add = [&](const Range &R, DocumentHighlightKind K) {
     Expected.emplace_back();
     Expected.back().range = R;
)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},
-                                               SourceAnnotations.range()}));
+  EXPECT_THAT(*Locations,
+              ElementsAre(FileRange(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},
-                                   HeaderInPreambleAnnotations.range()}));
+              ElementsAre(FileRange(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},
-                                   HeaderNotInPreambleAnnotations.range()}));
+              ElementsAre(FileRange(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};
 
   const char *HeaderContents = R"cpp([[]]#pragma once
                                      int a;
                                      )cpp";
   Annotations HeaderAnnotations(HeaderContents);
   FS.Files[FooH] = HeaderAnnotations.code();
 
   Server.addDocument(FooH, HeaderAnnotations.code());
   Server.addDocument(FooCpp, SourceAnnotations.code());
 
   // Test include in preamble.
   auto Locations =
       runFindDefinitions(Server, FooCpp, SourceAnnotations.point());
   ASSERT_TRUE(bool(Locations)) << "findDefinitions returned an error";
   EXPECT_THAT(*Locations,
-              ElementsAre(Location{FooHUri, HeaderAnnotations.range()}));
+              ElementsAre(FileRange(FooH, HeaderAnnotations.range())));
 
   // Test include in preamble, last char.
   Locations = runFindDefinitions(Server, FooCpp, SourceAnnotations.point("2"));
   ASSERT_TRUE(bool(Locations)) << "findDefinitions returned an error";
   EXPECT_THAT(*Locations,
-              ElementsAre(Location{FooHUri, HeaderAnnotations.range()}));
+              ElementsAre(FileRange(FooH, HeaderAnnotations.range())));
 
   Locations = runFindDefinitions(Server, FooCpp, SourceAnnotations.point("3"));
   ASSERT_TRUE(bool(Locations)) << "findDefinitions returned an error";
   EXPECT_THAT(*Locations,
-              ElementsAre(Location{FooHUri, HeaderAnnotations.range()}));
+              ElementsAre(FileRange(FooH, HeaderAnnotations.range())));
 
   // Test include outside of preamble.
   Locations = runFindDefinitions(Server, FooCpp, SourceAnnotations.point("6"));
   ASSERT_TRUE(bool(Locations)) << "findDefinitions returned an error";
   EXPECT_THAT(*Locations,
-              ElementsAre(Location{FooHUri, HeaderAnnotations.range()}));
+              ElementsAre(FileRange(FooH, HeaderAnnotations.range())));
 
   // Test a few positions that do not result in Locations.
   Locations = runFindDefinitions(Server, FooCpp, SourceAnnotations.point("4"));
   ASSERT_TRUE(bool(Locations)) << "findDefinitions returned an error";
   EXPECT_THAT(*Locations, IsEmpty());
 
   Locations = runFindDefinitions(Server, FooCpp, SourceAnnotations.point("5"));
   ASSERT_TRUE(bool(Locations)) << "findDefinitions returned an error";
   EXPECT_THAT(*Locations,
-              ElementsAre(Location{FooHUri, HeaderAnnotations.range()}));
+              ElementsAre(FileRange(FooH, HeaderAnnotations.range())));
 
   Locations = runFindDefinitions(Server, FooCpp, SourceAnnotations.point("7"));
   ASSERT_TRUE(bool(Locations)) << "findDefinitions returned an error";
   EXPECT_THAT(*Locations,
-              ElementsAre(Location{FooHUri, HeaderAnnotations.range()}));
+              ElementsAre(FileRange(FooH, HeaderAnnotations.range())));
 }
 
 TEST(GoToDefinition, WithPreamble) {
   // Test stragety: AST should always use the latest preamble instead of last
   // good preamble.
   MockFSProvider FS;
   IgnoreDiagnostics DiagConsumer;
   MockCompilationDatabase CDB;
   ClangdServer Server(CDB, FS, DiagConsumer, ClangdServer::optsForTest());
 
   auto FooCpp = testPath("foo.cpp");
-  auto FooCppUri = URIForFile{FooCpp};
   // The trigger locations must be the same.
   Annotations FooWithHeader(R"cpp(#include "fo^o.h")cpp");
   Annotations FooWithoutHeader(R"cpp(double    [[fo^o]]();)cpp");
 
   FS.Files[FooCpp] = FooWithHeader.code();
 
   auto FooH = testPath("foo.h");
-  auto FooHUri = URIForFile{FooH};
   Annotations FooHeader(R"cpp([[]])cpp");
   FS.Files[FooH] = FooHeader.code();
 
   runAddDocument(Server, FooCpp, FooWithHeader.code());
   // GoToDefinition goes to a #include file: the result comes from the preamble.
   EXPECT_THAT(
       cantFail(runFindDefinitions(Server, FooCpp, FooWithHeader.point())),
-      ElementsAre(Location{FooHUri, FooHeader.range()}));
+      ElementsAre(FileRange(FooH, FooHeader.range())));
 
   // Only preamble is built, and no AST is built in this request.
   Server.addDocument(FooCpp, FooWithoutHeader.code(), WantDiagnostics::No);
   // We build AST here, and it should use the latest preamble rather than the
   // stale one.
   EXPECT_THAT(
       cantFail(runFindDefinitions(Server, FooCpp, FooWithoutHeader.point())),
-      ElementsAre(Location{FooCppUri, FooWithoutHeader.range()}));
+      ElementsAre(FileRange(FooCpp, FooWithoutHeader.range())));
 
   // Reset test environment.
   runAddDocument(Server, FooCpp, FooWithHeader.code());
   // Both preamble and AST are built in this request.
   Server.addDocument(FooCpp, FooWithoutHeader.code(), WantDiagnostics::Yes);
   // Use the AST being built in above request.
   EXPECT_THAT(
       cantFail(runFindDefinitions(Server, FooCpp, FooWithoutHeader.point())),
-      ElementsAre(Location{FooCppUri, FooWithoutHeader.range()}));
+      ElementsAre(FileRange(FooCpp, FooWithoutHeader.range())));
 }
 
 TEST(FindReferences, WithinAST) {
   const char *Tests[] = {
       R"cpp(// Local variable
         int main() {
           int [[foo]];
           [[^foo]] = 2;