[clangd] Canonicalize file path in URIForFile.

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.

Reviewers: sammccall

Reviewed By: sammccall

Subscribers: ilya-biryukov, MaskRay, jkorous, arphaman, kadircet, cfe-commits

Differential Revision: https://reviews.llvm.org/D54845

llvm-svn: 347739

clang-tools-extra/clangd/ClangdLSPServer.cpp

@@ -775,7 +775,7 @@ std::vector<Fix> ClangdLSPServer::getFixes(StringRef File,
 
 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) {

clang-tools-extra/clangd/FindSymbols.cpp

@@ -142,7 +142,9 @@ getWorkspaceSymbols(StringRef Query, int Limit, const SymbolIndex *const Index,
       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();

clang-tools-extra/clangd/Protocol.cpp

@@ -30,29 +30,44 @@ 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);
+    auto Parsed = URI::parse(*S);
-    if (!U) {
+    if (!Parsed) {
-      elog("Failed to parse URI {0}: {1}", *S, U.takeError());
+      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);
+    // "file" and "test" schemes do not require hint path.
-    if (!Path) {
+    auto U = URIForFile::fromURI(*Parsed, /*HintPath=*/"");
-      log("{0}", Path.takeError());
+    if (!U) {
+      elog("{0}", U.takeError());
       return false;
     }
-    R = URIForFile(*Path);
+    R = std::move(*U);
     return true;
   }
   return false;

clang-tools-extra/clangd/Protocol.h

@@ -67,9 +67,25 @@ public:
   }
 };
 
+// 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; }
@@ -90,6 +106,8 @@ struct URIForFile {
   }
 
 private:
+  explicit URIForFile(std::string &&File) : File(std::move(File)) {}
+
   std::string File;
 };
 

clang-tools-extra/clangd/URI.cpp

@@ -31,10 +31,10 @@ inline Error make_string_error(const Twine &Message) {
 
 /// \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("/"))
@@ -57,17 +57,14 @@ public:
     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)
@@ -200,9 +197,6 @@ URI URI::create(StringRef 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.
@@ -218,7 +212,7 @@ URI URI::create(StringRef 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);
@@ -231,6 +225,25 @@ Expected<std::string> URI::resolve(const URI &Uri, StringRef HintPath) {
   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)

clang-tools-extra/clangd/URI.h

@@ -64,6 +64,13 @@ public:
   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".
   ///

clang-tools-extra/clangd/XRefs.cpp

@@ -43,25 +43,26 @@ void logIfOverflow(const SymbolLocation &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);
+  auto U = URIForFile::fromURI(*Uri, TUPath);
-  if (!Path) {
+  if (!U) {
-    log("Could not resolve URI: {0}", Loc.FileURI);
+    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();
@@ -224,7 +225,8 @@ Range getTokenRange(ParsedAST &AST, SourceLocation 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)
@@ -235,7 +237,7 @@ Optional<Location> makeLocation(ParsedAST &AST, SourceLocation TokLoc) {
     return None;
   }
   Location L;
-  L.uri = URIForFile(*FilePath);
+  L.uri = URIForFile::canonicalize(*FilePath, TUPath);
   L.range = getTokenRange(AST, TokLoc);
   return L;
 }
@@ -244,25 +246,31 @@ Optional<Location> makeLocation(ParsedAST &AST, SourceLocation TokLoc) {
 
 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);
   }
@@ -312,7 +320,7 @@ std::vector<Location> findDefinitions(ParsedAST &AST, Position Pos,
     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;
@@ -328,22 +336,22 @@ std::vector<Location> findDefinitions(ParsedAST &AST, Position Pos,
     // 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());
+        SM.getFileEntryForID(SM.getMainFileID());
-    if (auto Path = getRealPath(FE, SourceMgr))
+    if (auto Path = getRealPath(FE, SM))
-      HintPath = *Path;
+      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);
     });
   }
 
@@ -722,7 +730,7 @@ std::vector<Location> findReferences(ParsedAST &AST, Position Pos,
   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));
   }
 
@@ -742,7 +750,7 @@ std::vector<Location> findReferences(ParsedAST &AST, Position Pos,
   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));

clang-tools-extra/unittests/clangd/ClangdTests.cpp

@@ -34,6 +34,8 @@ using namespace llvm;
 namespace clang {
 namespace clangd {
 
+namespace {
+
 using ::testing::ElementsAre;
 using ::testing::Eq;
 using ::testing::Field;
@@ -42,7 +44,9 @@ 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) {
@@ -457,8 +461,8 @@ int hello;
 
   auto Locations = runFindDefinitions(Server, FooCpp, FooSource.point());
   EXPECT_TRUE(bool(Locations));
-  EXPECT_THAT(*Locations, ElementsAre(Location{URIForFile{FooCpp},
+  EXPECT_THAT(*Locations,
-                                               FooSource.range("one")}));
+              ElementsAre(FileRange(FooCpp, FooSource.range("one"))));
 
   // Undefine MACRO, close baz.cpp.
   CDB.ExtraClangFlags.clear();
@@ -473,8 +477,8 @@ int hello;
 
   Locations = runFindDefinitions(Server, FooCpp, FooSource.point());
   EXPECT_TRUE(bool(Locations));
-  EXPECT_THAT(*Locations, ElementsAre(Location{URIForFile{FooCpp},
+  EXPECT_THAT(*Locations, ElementsAre(FileRange(FooCpp,
-                                               FooSource.range("two")}));
+                                                FooSource.range("two"))));
 }
 
 TEST_F(ClangdVFSTest, MemoryUsage) {

clang-tools-extra/unittests/clangd/ClangdUnitTests.cpp

@@ -258,9 +258,10 @@ main.cpp:2:3: error: something terrible happened)");
   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) {

clang-tools-extra/unittests/clangd/TestFS.cpp

@@ -93,7 +93,10 @@ public:
 
   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 '/'",

clang-tools-extra/unittests/clangd/URITests.cpp

@@ -137,6 +137,28 @@ TEST(URITest, Resolve) {
             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");

clang-tools-extra/unittests/clangd/XRefsTests.cpp

@@ -37,6 +37,10 @@ class IgnoreDiagnostics : public DiagnosticsConsumer {
                           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> &>
@@ -396,22 +400,22 @@ int [[bar_not_preamble]];
   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,
-                                               SourceAnnotations.range()}));
+              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},
+              ElementsAre(FileRange(HeaderInPreambleH,
-                                   HeaderInPreambleAnnotations.range()}));
+                                    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(FileRange(HeaderNotInPreambleH,
-                                   HeaderNotInPreambleAnnotations.range()}));
+                                    HeaderNotInPreambleAnnotations.range())));
 }
 
 TEST(Hover, All) {
@@ -1047,7 +1051,6 @@ TEST(GoToInclude, All) {
   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;
@@ -1063,24 +1066,24 @@ TEST(GoToInclude, All) {
       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"));
@@ -1090,12 +1093,12 @@ TEST(GoToInclude, All) {
   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) {
@@ -1107,7 +1110,6 @@ TEST(GoToDefinition, WithPreamble) {
   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");
@@ -1115,7 +1117,6 @@ TEST(GoToDefinition, WithPreamble) {
   FS.Files[FooCpp] = FooWithHeader.code();
 
   auto FooH = testPath("foo.h");
-  auto FooHUri = URIForFile{FooH};
   Annotations FooHeader(R"cpp([[]])cpp");
   FS.Files[FooH] = FooHeader.code();
 
@@ -1123,7 +1124,7 @@ TEST(GoToDefinition, WithPreamble) {
   // 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);
@@ -1131,7 +1132,7 @@ TEST(GoToDefinition, WithPreamble) {
   // 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());
@@ -1140,7 +1141,7 @@ TEST(GoToDefinition, WithPreamble) {
   // 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) {