Index: clangd/ClangdLSPServer.cpp =================================================================== --- clangd/ClangdLSPServer.cpp +++ clangd/ClangdLSPServer.cpp @@ -775,7 +775,7 @@ void ClangdLSPServer::onDiagnosticsReady(PathRef File, std::vector Diagnostics) { - URIForFile URI(File); + auto URI = URIForFile::canonicalize(File, /*TUPath=*/File); std::vector LSPDiagnostics; DiagnosticToReplacementMap LocalFixIts; // Temporary storage for (auto &Diag : Diagnostics) { Index: clangd/FindSymbols.cpp =================================================================== --- clangd/FindSymbols.cpp +++ clangd/FindSymbols.cpp @@ -142,7 +142,9 @@ 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(); Index: clangd/Protocol.h =================================================================== --- clangd/Protocol.h +++ clangd/Protocol.h @@ -67,9 +67,25 @@ } }; +// 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 fromURI(const URI &U, + llvm::StringRef HintPath); /// Retrieves absolute path to the file. llvm::StringRef file() const { return File; } @@ -90,6 +106,8 @@ } private: + explicit URIForFile(std::string &&File) : File(std::move(File)) {} + std::string File; }; Index: clangd/Protocol.cpp =================================================================== --- clangd/Protocol.cpp +++ clangd/Protocol.cpp @@ -30,29 +30,44 @@ 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::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 @@ -31,10 +31,10 @@ /// \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 getAbsolutePath(StringRef /*Authority*/, StringRef Body, StringRef /*HintPath*/) const override { if (!Body.startswith("/")) @@ -57,17 +57,14 @@ 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 - X(FileSystemScheme::Scheme, - "URI scheme for absolute paths in the file system."); - Expected> findSchemeByName(StringRef Scheme) { + if (Scheme == "file") + return make_unique(); + for (auto I = URISchemeRegistry::begin(), E = URISchemeRegistry::end(); I != E; ++I) { if (I->getName() != Scheme) @@ -200,9 +197,6 @@ 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::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 @@ return S->get()->getAbsolutePath(Uri.Authority, Uri.Body, HintPath); } +Expected 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 URI::includeSpelling(const URI &Uri) { auto S = findSchemeByName(Uri.Scheme); if (!S) Index: clangd/XRefs.cpp =================================================================== --- clangd/XRefs.cpp +++ clangd/XRefs.cpp @@ -43,25 +43,26 @@ } // 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 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(); @@ -224,7 +225,8 @@ sourceLocToPosition(SourceMgr, LocEnd)}; } -Optional makeLocation(ParsedAST &AST, SourceLocation TokLoc) { +Optional 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 @@ 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 @@ 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::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 @@ 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 @@ // 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); }); } @@ -722,7 +730,7 @@ 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 @@ 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)); Index: unittests/clangd/ClangdTests.cpp =================================================================== --- unittests/clangd/ClangdTests.cpp +++ unittests/clangd/ClangdTests.cpp @@ -34,6 +34,8 @@ namespace clang { namespace clangd { +namespace { + using ::testing::ElementsAre; using ::testing::Eq; using ::testing::Field; @@ -42,7 +44,9 @@ 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 &Diagnostics) { for (auto D : Diagnostics) { @@ -457,8 +461,8 @@ 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(); @@ -473,8 +477,8 @@ 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) { Index: unittests/clangd/ClangdUnitTests.cpp =================================================================== --- unittests/clangd/ClangdUnitTests.cpp +++ unittests/clangd/ClangdUnitTests.cpp @@ -258,9 +258,10 @@ 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 Fixes) { Index: unittests/clangd/TestFS.cpp =================================================================== --- unittests/clangd/TestFS.cpp +++ unittests/clangd/TestFS.cpp @@ -93,7 +93,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 @@ -137,6 +137,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 @@ -37,6 +37,10 @@ std::vector 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 &> @@ -396,22 +400,22 @@ 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) { @@ -1047,7 +1051,6 @@ 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 @@ 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 @@ 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 @@ 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 @@ 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 @@ // 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 @@ // 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 @@ // 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) {