Index: ClangdLSPServer.h =================================================================== --- ClangdLSPServer.h +++ ClangdLSPServer.h @@ -74,7 +74,7 @@ void onSignatureHelp(const TextDocumentPositionParams &, Callback); void onGoToDefinition(const TextDocumentPositionParams &, - Callback>); + Callback>); void onReference(const ReferenceParams &, Callback>); void onSwitchSourceHeader(const TextDocumentIdentifier &, Callback); Index: ClangdLSPServer.cpp =================================================================== --- ClangdLSPServer.cpp +++ ClangdLSPServer.cpp @@ -626,8 +626,9 @@ std::move(Reply)); } -void ClangdLSPServer::onGoToDefinition(const TextDocumentPositionParams &Params, - Callback> Reply) { +void ClangdLSPServer::onGoToDefinition( + const TextDocumentPositionParams &Params, + Callback> Reply) { Server->findDefinitions(Params.textDocument.uri.file(), Params.position, std::move(Reply)); } Index: ClangdServer.h =================================================================== --- ClangdServer.h +++ ClangdServer.h @@ -150,7 +150,7 @@ /// Get definition of symbol at a specified \p Line and \p Column in \p File. void findDefinitions(PathRef File, Position Pos, - Callback> CB); + Callback> CB); /// Helper function that returns a path to the corresponding source file when /// given a header file and vice versa. Index: ClangdServer.cpp =================================================================== --- ClangdServer.cpp +++ ClangdServer.cpp @@ -331,8 +331,8 @@ } void ClangdServer::findDefinitions(PathRef File, Position Pos, - Callback> CB) { - auto Action = [Pos, this](Callback> CB, + Callback> CB) { + auto Action = [Pos, this](Callback> CB, Expected InpAST) { if (!InpAST) return CB(InpAST.takeError()); Index: Protocol.h =================================================================== --- Protocol.h +++ Protocol.h @@ -175,6 +175,26 @@ llvm::json::Value toJSON(const Location &); llvm::raw_ostream &operator<<(llvm::raw_ostream &, const Location &); +struct DefinitionData { + Location DefOrDeclLocation; + llvm::SmallString<128> USR; + + friend bool operator==(const DefinitionData &LHS, const DefinitionData &RHS) { + return LHS.DefOrDeclLocation == RHS.DefOrDeclLocation && LHS.USR == RHS.USR; + } + + friend bool operator!=(const DefinitionData &LHS, const DefinitionData &RHS) { + return !(LHS == RHS); + } + + friend bool operator<(const DefinitionData &LHS, const DefinitionData &RHS) { + return std::tie(LHS.DefOrDeclLocation, LHS.USR) < + std::tie(RHS.DefOrDeclLocation, RHS.USR); + } +}; +llvm::json::Value toJSON(const DefinitionData &); +llvm::raw_ostream &operator<<(llvm::raw_ostream &, const DefinitionData &); + struct TextEdit { /// The range of the text document to be manipulated. To insert /// text into a document create a range where start === end. Index: Protocol.cpp =================================================================== --- Protocol.cpp +++ Protocol.cpp @@ -113,6 +113,19 @@ return OS << L.range << '@' << L.uri; } +json::Value toJSON(const DefinitionData &P) { + return json::Object{ + {"uri", P.DefOrDeclLocation.uri}, + {"range", P.DefOrDeclLocation.range}, + {"usr", P.USR}, + }; +} + +raw_ostream &operator<<(raw_ostream &OS, const DefinitionData &L) { + return OS << L.USR << '@' << L.DefOrDeclLocation.uri << ":" + << L.DefOrDeclLocation.range; +} + bool fromJSON(const json::Value &Params, TextDocumentItem &R) { json::ObjectMapper O(Params); return O && O.map("uri", R.uri) && O.map("languageId", R.languageId) && Index: XRefs.h =================================================================== --- XRefs.h +++ XRefs.h @@ -24,8 +24,8 @@ namespace clangd { /// Get definition of symbol at a specified \p Pos. -std::vector findDefinitions(ParsedAST &AST, Position Pos, - const SymbolIndex *Index = nullptr); +std::vector findDefinitions(ParsedAST &AST, Position Pos, + const SymbolIndex *Index = nullptr); /// Returns highlights for all usages of a symbol at \p Pos. std::vector findDocumentHighlights(ParsedAST &AST, Index: XRefs.cpp =================================================================== --- XRefs.cpp +++ XRefs.cpp @@ -242,15 +242,16 @@ } // namespace -std::vector findDefinitions(ParsedAST &AST, Position Pos, - const SymbolIndex *Index) { +std::vector findDefinitions(ParsedAST &AST, Position Pos, + const SymbolIndex *Index) { const SourceManager &SourceMgr = AST.getASTContext().getSourceManager(); - std::vector Result; + 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(DefinitionData{Location{URIForFile{Inc.Resolved}, {}}, + SmallString<128>{}}); } if (!Result.empty()) return Result; @@ -264,7 +265,7 @@ auto Loc = Item.Info->getDefinitionLoc(); auto L = makeLocation(AST, Loc); if (L) - Result.push_back(*L); + Result.push_back(DefinitionData{*L, SmallString<128>{}}); } // Declaration and definition are different terms in C-family languages, and @@ -287,12 +288,13 @@ // location (if available). // 4. Return all populated locations for all symbols, definition first ( // which we think is the users wants most often). - struct CandidateLocation { + struct CandidateInfo { Optional Def; Optional Decl; + SmallString<128> USR; }; // We respect the order in Symbols.Decls. - SmallVector ResultCandidates; + SmallVector ResultCandidates; DenseMap CandidatesIndex; // Emit all symbol locations (declaration or definition) from AST. @@ -302,13 +304,17 @@ // Ideally, there should be a USR for each identified symbols. Symbols // without USR are rare and unimportant cases, we use the a fake holder to // minimize the invasiveness of these cases. + SmallString<128> USR; SymbolID Key(""); - if (auto ID = getSymbolID(D)) - Key = *ID; + if (!index::generateUSRForDecl(D, USR)) + Key = SymbolID(USR); auto R = CandidatesIndex.try_emplace(Key, ResultCandidates.size()); - if (R.second) // new entry + if (R.second) { // new entry ResultCandidates.emplace_back(); + ResultCandidates[R.first->second].USR = USR; + } + auto &Candidate = ResultCandidates[R.first->second]; auto Loc = findNameLoc(D); @@ -350,10 +356,10 @@ // Populate the results, definition first. for (const auto &Candidate : ResultCandidates) { if (Candidate.Def) - Result.push_back(*Candidate.Def); + Result.push_back(DefinitionData{*Candidate.Def, Candidate.USR}); if (Candidate.Decl && Candidate.Decl != Candidate.Def) // Decl and Def might be the same - Result.push_back(*Candidate.Decl); + Result.push_back(DefinitionData{*Candidate.Decl, Candidate.USR}); } return Result; Index: clangd/ClangdTests.cpp =================================================================== --- clangd/ClangdTests.cpp +++ clangd/ClangdTests.cpp @@ -44,6 +44,8 @@ namespace { +MATCHER_P(LocationIs, R, "") { return arg.DefOrDeclLocation == R; } + bool diagsContainErrors(const std::vector &Diagnostics) { for (auto D : Diagnostics) { if (D.Severity == DiagnosticsEngine::Error || @@ -455,10 +457,10 @@ 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")})); + auto DefData = runFindDefinitions(Server, FooCpp, FooSource.point()); + EXPECT_TRUE(bool(DefData)); + EXPECT_THAT(*DefData, ElementsAre(LocationIs(Location{ + URIForFile{FooCpp}, FooSource.range("one")}))); // Undefine MACRO, close baz.cpp. CDB.ExtraClangFlags.clear(); @@ -471,10 +473,10 @@ 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")})); + DefData = runFindDefinitions(Server, FooCpp, FooSource.point()); + EXPECT_TRUE(bool(DefData)); + EXPECT_THAT(*DefData, ElementsAre(LocationIs(Location{ + URIForFile{FooCpp}, FooSource.range("two")}))); } TEST_F(ClangdVFSTest, MemoryUsage) { Index: clangd/SyncAPI.h =================================================================== --- clangd/SyncAPI.h +++ clangd/SyncAPI.h @@ -33,7 +33,7 @@ llvm::Expected runSignatureHelp(ClangdServer &Server, PathRef File, Position Pos); -llvm::Expected> +llvm::Expected> runFindDefinitions(ClangdServer &Server, PathRef File, Position Pos); llvm::Expected> Index: clangd/SyncAPI.cpp =================================================================== --- clangd/SyncAPI.cpp +++ clangd/SyncAPI.cpp @@ -86,9 +86,9 @@ return std::move(*Result); } -Expected> runFindDefinitions(ClangdServer &Server, - PathRef File, Position Pos) { - Optional>> Result; +Expected> +runFindDefinitions(ClangdServer &Server, PathRef File, Position Pos) { + Optional>> Result; Server.findDefinitions(File, Pos, capture(Result)); return std::move(*Result); } Index: clangd/XRefsTests.cpp =================================================================== --- clangd/XRefsTests.cpp +++ clangd/XRefsTests.cpp @@ -94,6 +94,11 @@ } MATCHER_P(RangeIs, R, "") { return arg.range == R; } +MATCHER_P(RangeInLocationIs, R, "") { return arg.DefOrDeclLocation.range == R; } +MATCHER_P(RangeInLocationIsRH, R, "") { + return arg == R.DefOrDeclLocation.range; +} +MATCHER_P(LocationIs, R, "") { return arg.DefOrDeclLocation == R; } TEST(GoToDefinition, WithIndex) { Annotations SymbolHeader(R"cpp( @@ -124,9 +129,10 @@ ^f1(); } )cpp"); - EXPECT_THAT(runFindDefinitionsWithIndex(Test), - testing::ElementsAreArray( - {RangeIs(SymbolCpp.range("f1")), RangeIs(Test.range())})); + EXPECT_THAT( + runFindDefinitionsWithIndex(Test), + testing::ElementsAreArray({RangeInLocationIs(SymbolCpp.range("f1")), + RangeInLocationIs(Test.range())})); Test = Annotations(R"cpp(// definition in AST. void [[f1]]() {} @@ -134,17 +140,19 @@ ^f1(); } )cpp"); - EXPECT_THAT(runFindDefinitionsWithIndex(Test), - testing::ElementsAreArray( - {RangeIs(Test.range()), RangeIs(SymbolHeader.range("f1"))})); + EXPECT_THAT( + runFindDefinitionsWithIndex(Test), + testing::ElementsAreArray({RangeInLocationIs(Test.range()), + RangeInLocationIs(SymbolHeader.range("f1"))})); Test = Annotations(R"cpp(// forward declaration in AST. class [[Foo]]; F^oo* create(); )cpp"); - EXPECT_THAT(runFindDefinitionsWithIndex(Test), - testing::ElementsAreArray( - {RangeIs(SymbolHeader.range("foo")), RangeIs(Test.range())})); + EXPECT_THAT( + runFindDefinitionsWithIndex(Test), + testing::ElementsAreArray({RangeInLocationIs(SymbolHeader.range("foo")), + RangeInLocationIs(Test.range())})); Test = Annotations(R"cpp(// defintion in AST. class [[Forward]] {}; @@ -152,7 +160,8 @@ )cpp"); EXPECT_THAT(runFindDefinitionsWithIndex(Test), testing::ElementsAreArray({ - RangeIs(Test.range()), RangeIs(SymbolHeader.range("forward")), + RangeInLocationIs(Test.range()), + RangeInLocationIs(SymbolHeader.range("forward")), })); } @@ -302,12 +311,13 @@ for (const char *Test : Tests) { Annotations T(Test); auto AST = TestTU::withCode(T.code()).build(); - std::vector> ExpectedLocations; - for (const auto &R : T.ranges()) - ExpectedLocations.push_back(RangeIs(R)); - EXPECT_THAT(findDefinitions(AST, T.point()), - ElementsAreArray(ExpectedLocations)) - << Test; + + auto definitions = findDefinitions(AST, T.point()); + std::vector> Assertions; + for (const auto &D : definitions) + Assertions.push_back(RangeInLocationIsRH(D)); + + EXPECT_THAT(T.ranges(), ElementsAreArray(Assertions)) << Test; } } @@ -334,22 +344,27 @@ )cpp"); auto AST = TestTU::withCode(T.code()).build(); EXPECT_THAT(findDefinitions(AST, T.point("1")), - ElementsAre(RangeIs(T.range("str")), RangeIs(T.range("foo4")))); + ElementsAre(RangeInLocationIs(T.range("str")), + RangeInLocationIs(T.range("foo4")))); EXPECT_THAT(findDefinitions(AST, T.point("2")), - ElementsAre(RangeIs(T.range("str")))); + ElementsAre(RangeInLocationIs(T.range("str")))); EXPECT_THAT(findDefinitions(AST, T.point("3")), - ElementsAre(RangeIs(T.range("f")), RangeIs(T.range("foo3")))); + ElementsAre(RangeInLocationIs(T.range("f")), + RangeInLocationIs(T.range("foo3")))); EXPECT_THAT(findDefinitions(AST, T.point("4")), - ElementsAre(RangeIs(T.range("g")))); + ElementsAre(RangeInLocationIs(T.range("g")))); EXPECT_THAT(findDefinitions(AST, T.point("5")), - ElementsAre(RangeIs(T.range("f")), RangeIs(T.range("foo3")))); + ElementsAre(RangeInLocationIs(T.range("f")), + RangeInLocationIs(T.range("foo3")))); auto DefinitionAtPoint6 = findDefinitions(AST, T.point("6")); EXPECT_EQ(3ul, DefinitionAtPoint6.size()); - EXPECT_THAT(DefinitionAtPoint6, HasSubsequence(RangeIs(T.range("str")), - RangeIs(T.range("foo4")))); - EXPECT_THAT(DefinitionAtPoint6, HasSubsequence(RangeIs(T.range("str")), - RangeIs(T.range("foo3")))); + EXPECT_THAT(DefinitionAtPoint6, + HasSubsequence(RangeInLocationIs(T.range("str")), + RangeInLocationIs(T.range("foo4")))); + EXPECT_THAT(DefinitionAtPoint6, + HasSubsequence(RangeInLocationIs(T.range("str")), + RangeInLocationIs(T.range("foo3")))); } TEST(GoToDefinition, RelPathsInCompileCommand) { @@ -393,25 +408,25 @@ runAddDocument(Server, FooCpp, SourceAnnotations.code()); // Go to a definition in main source file. - auto Locations = + auto DefData = runFindDefinitions(Server, FooCpp, SourceAnnotations.point("p1")); - EXPECT_TRUE(bool(Locations)) << "findDefinitions returned an error"; - EXPECT_THAT(*Locations, ElementsAre(Location{URIForFile{FooCpp}, - SourceAnnotations.range()})); + EXPECT_TRUE(bool(DefData)) << "findDefinitions returned an error"; + EXPECT_THAT(*DefData, ElementsAre(LocationIs(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}, - HeaderInPreambleAnnotations.range()})); + DefData = runFindDefinitions(Server, FooCpp, SourceAnnotations.point("p2")); + EXPECT_TRUE(bool(DefData)) << "findDefinitions returned an error"; + EXPECT_THAT(*DefData, ElementsAre(LocationIs( + 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}, - HeaderNotInPreambleAnnotations.range()})); + DefData = runFindDefinitions(Server, FooCpp, SourceAnnotations.point("p3")); + EXPECT_TRUE(bool(DefData)) << "findDefinitions returned an error"; + EXPECT_THAT(*DefData, ElementsAre(LocationIs( + Location{URIForFile{HeaderNotInPreambleH}, + HeaderNotInPreambleAnnotations.range()}))); } TEST(Hover, All) { @@ -1054,25 +1069,25 @@ auto Locations = runFindDefinitions(Server, FooCpp, SourceAnnotations.point()); ASSERT_TRUE(bool(Locations)) << "findDefinitions returned an error"; - EXPECT_THAT(*Locations, - ElementsAre(Location{FooHUri, HeaderAnnotations.range()})); + EXPECT_THAT(*Locations, ElementsAre(LocationIs( + Location{FooHUri, 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()})); + EXPECT_THAT(*Locations, ElementsAre(LocationIs( + Location{FooHUri, 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()})); + EXPECT_THAT(*Locations, ElementsAre(LocationIs( + Location{FooHUri, 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()})); + EXPECT_THAT(*Locations, ElementsAre(LocationIs( + Location{FooHUri, HeaderAnnotations.range()}))); // Test a few positions that do not result in Locations. Locations = runFindDefinitions(Server, FooCpp, SourceAnnotations.point("4")); @@ -1081,13 +1096,13 @@ Locations = runFindDefinitions(Server, FooCpp, SourceAnnotations.point("5")); ASSERT_TRUE(bool(Locations)) << "findDefinitions returned an error"; - EXPECT_THAT(*Locations, - ElementsAre(Location{FooHUri, HeaderAnnotations.range()})); + EXPECT_THAT(*Locations, ElementsAre(LocationIs( + Location{FooHUri, 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()})); + EXPECT_THAT(*Locations, ElementsAre(LocationIs( + Location{FooHUri, HeaderAnnotations.range()}))); } TEST(GoToDefinition, WithPreamble) { @@ -1115,7 +1130,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(LocationIs(Location{FooHUri, FooHeader.range()}))); // Only preamble is built, and no AST is built in this request. Server.addDocument(FooCpp, FooWithoutHeader.code(), WantDiagnostics::No); @@ -1123,7 +1138,7 @@ // stale one. EXPECT_THAT( cantFail(runFindDefinitions(Server, FooCpp, FooWithoutHeader.point())), - ElementsAre(Location{FooCppUri, FooWithoutHeader.range()})); + ElementsAre(LocationIs(Location{FooCppUri, FooWithoutHeader.range()}))); // Reset test environment. runAddDocument(Server, FooCpp, FooWithHeader.code()); @@ -1132,7 +1147,7 @@ // Use the AST being built in above request. EXPECT_THAT( cantFail(runFindDefinitions(Server, FooCpp, FooWithoutHeader.point())), - ElementsAre(Location{FooCppUri, FooWithoutHeader.range()})); + ElementsAre(LocationIs(Location{FooCppUri, FooWithoutHeader.range()}))); } TEST(FindReferences, WithinAST) { Index: clangd/textdocument-didchange-fail.test =================================================================== --- clangd/textdocument-didchange-fail.test +++ clangd/textdocument-didchange-fail.test @@ -20,6 +20,7 @@ # CHECK-NEXT: } # CHECK-NEXT: }, # CHECK-NEXT: "uri": "file://{{.*}}/clangd-test/main.cpp" +# CHECK-NEXT: "usr": "c:@F@main#" # CHECK-NEXT: } # CHECK-NEXT: ] --- Index: clangd/xrefs.test =================================================================== --- clangd/xrefs.test +++ clangd/xrefs.test @@ -18,7 +18,8 @@ # CHECK-NEXT: "line": 0 # CHECK-NEXT: } # CHECK-NEXT: }, -# CHECK-NEXT: "uri": "file://{{.*}}/{{([A-Z]:/)?}}main.cpp" +# CHECK-NEXT: "uri": "file://{{.*}}/{{([A-Z]:/)?}}main.cpp", +# CHECK-NEXT: "usr": "c:@x" # CHECK-NEXT: } # CHECK-NEXT: ] ---