Index: clangd/FindSymbols.cpp =================================================================== --- clangd/FindSymbols.cpp +++ clangd/FindSymbols.cpp @@ -140,7 +140,7 @@ 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(); @@ -196,9 +196,9 @@ 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) { Index: clangd/Protocol.h =================================================================== --- clangd/Protocol.h +++ clangd/Protocol.h @@ -66,9 +66,15 @@ } }; +// 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 fromURI(const URI &U, + llvm::StringRef HintPath); /// Retrieves absolute path to the file. llvm::StringRef file() const { return File; } @@ -89,6 +95,8 @@ } private: + explicit URIForFile(std::string &&File) : File(std::move(File)) {} + std::string File; }; Index: clangd/Protocol.cpp =================================================================== --- clangd/Protocol.cpp +++ clangd/Protocol.cpp @@ -27,29 +27,45 @@ 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::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; Index: clangd/URI.h =================================================================== --- clangd/URI.h +++ clangd/URI.h @@ -64,6 +64,13 @@ static llvm::Expected 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 resolvePath(llvm::StringRef AbsPath, + llvm::StringRef HintPath = ""); + /// Gets the preferred spelling of this file for #include, if there is one, /// e.g. , "path/to/x.h". /// Index: clangd/URI.cpp =================================================================== --- clangd/URI.cpp +++ clangd/URI.cpp @@ -231,6 +231,13 @@ return S->get()->getAbsolutePath(Uri.Authority, Uri.Body, HintPath); } +Expected 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 URI::includeSpelling(const URI &Uri) { auto S = findSchemeByName(Uri.Scheme); if (!S) Index: clangd/XRefs.cpp =================================================================== --- clangd/XRefs.cpp +++ clangd/XRefs.cpp @@ -52,16 +52,17 @@ 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(); @@ -224,7 +225,8 @@ sourceLocToPosition(SourceMgr, LocEnd)}; } -Optional makeLocation(ParsedAST &AST, SourceLocation TokLoc) { +Optional makeLocation(ParsedAST &AST, SourceLocation TokLoc, + StringRef HintPath) { const SourceManager &SourceMgr = AST.getASTContext().getSourceManager(); const FileEntry *F = SourceMgr.getFileEntryForID(SourceMgr.getFileID(TokLoc)); if (!F) @@ -235,7 +237,7 @@ return None; } Location L; - L.uri = URIForFile(*FilePath); + L.uri = URIForFile(*FilePath, HintPath); L.range = getTokenRange(AST, TokLoc); return L; } @@ -244,25 +246,30 @@ std::vector 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 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); } @@ -312,7 +319,7 @@ 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; @@ -330,8 +337,8 @@ 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, Index: unittests/clangd/ClangdTests.cpp =================================================================== --- unittests/clangd/ClangdTests.cpp +++ unittests/clangd/ClangdTests.cpp @@ -457,7 +457,7 @@ 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. @@ -473,7 +473,7 @@ 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")})); } Index: unittests/clangd/ClangdUnitTests.cpp =================================================================== --- unittests/clangd/ClangdUnitTests.cpp +++ unittests/clangd/ClangdUnitTests.cpp @@ -235,9 +235,9 @@ 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 Fixes) { Index: unittests/clangd/TestFS.cpp =================================================================== --- unittests/clangd/TestFS.cpp +++ unittests/clangd/TestFS.cpp @@ -90,7 +90,10 @@ Expected getAbsolutePath(StringRef /*Authority*/, StringRef Body, StringRef HintPath) const override { - assert(HintPath.startswith(testRoot())); + if (!HintPath.startswith(testRoot())) + return make_error( + "Hint path doesn't start with test root: " + HintPath, + inconvertibleErrorCode()); if (!Body.consume_front("/")) return make_error( "Body of an unittest: URI must start with '/'", Index: unittests/clangd/URITests.cpp =================================================================== --- unittests/clangd/URITests.cpp +++ unittests/clangd/URITests.cpp @@ -152,6 +152,28 @@ 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"); Index: unittests/clangd/XRefsTests.cpp =================================================================== --- unittests/clangd/XRefsTests.cpp +++ unittests/clangd/XRefsTests.cpp @@ -396,21 +396,21 @@ 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()})); } @@ -1047,7 +1047,7 @@ 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;