Index: clangd/ClangdLSPServer.h =================================================================== --- clangd/ClangdLSPServer.h +++ clangd/ClangdLSPServer.h @@ -79,6 +79,8 @@ Callback); void onGoToDefinition(const TextDocumentPositionParams &, Callback>); + void onGoToDeclaration(const TextDocumentPositionParams &, + Callback>); void onReference(const ReferenceParams &, Callback>); void onSwitchSourceHeader(const TextDocumentIdentifier &, Callback); Index: clangd/ClangdLSPServer.cpp =================================================================== --- clangd/ClangdLSPServer.cpp +++ clangd/ClangdLSPServer.cpp @@ -329,6 +329,7 @@ llvm::json::Object{ {"triggerCharacters", {"(", ","}}, }}, + {"declarationProvider", true}, {"definitionProvider", true}, {"documentHighlightProvider", true}, {"hoverProvider", true}, @@ -656,8 +657,37 @@ void ClangdLSPServer::onGoToDefinition(const TextDocumentPositionParams &Params, Callback> Reply) { - Server->findDefinitions(Params.textDocument.uri.file(), Params.position, - std::move(Reply)); + Server->locateSymbolAt( + Params.textDocument.uri.file(), Params.position, + Bind( + [&](decltype(Reply) Reply, + llvm::Expected> Symbols) { + if (!Symbols) + return Reply(Symbols.takeError()); + std::vector Defs; + for (const auto &S : *Symbols) + Defs.push_back(S.Definition.getValueOr(S.PreferredDeclaration)); + Reply(std::move(Defs)); + }, + std::move(Reply))); +} + +void ClangdLSPServer::onGoToDeclaration( + const TextDocumentPositionParams &Params, + Callback> Reply) { + Server->locateSymbolAt( + Params.textDocument.uri.file(), Params.position, + Bind( + [&](decltype(Reply) Reply, + llvm::Expected> Symbols) { + if (!Symbols) + return Reply(Symbols.takeError()); + std::vector Decls; + for (const auto &S : *Symbols) + Decls.push_back(S.PreferredDeclaration); + Reply(std::move(Decls)); + }, + std::move(Reply))); } void ClangdLSPServer::onSwitchSourceHeader(const TextDocumentIdentifier &Params, @@ -743,6 +773,7 @@ MsgHandler->bind("textDocument/completion", &ClangdLSPServer::onCompletion); MsgHandler->bind("textDocument/signatureHelp", &ClangdLSPServer::onSignatureHelp); MsgHandler->bind("textDocument/definition", &ClangdLSPServer::onGoToDefinition); + MsgHandler->bind("textDocument/declaration", &ClangdLSPServer::onGoToDeclaration); MsgHandler->bind("textDocument/references", &ClangdLSPServer::onReference); MsgHandler->bind("textDocument/switchSourceHeader", &ClangdLSPServer::onSwitchSourceHeader); MsgHandler->bind("textDocument/rename", &ClangdLSPServer::onRename); Index: clangd/ClangdServer.h =================================================================== --- clangd/ClangdServer.h +++ clangd/ClangdServer.h @@ -18,6 +18,7 @@ #include "GlobalCompilationDatabase.h" #include "Protocol.h" #include "TUScheduler.h" +#include "XRefs.h" #include "index/Background.h" #include "index/FileIndex.h" #include "index/Index.h" @@ -164,8 +165,8 @@ void signatureHelp(PathRef File, Position Pos, Callback CB); /// Get definition of symbol at a specified \p Line and \p Column in \p File. - void findDefinitions(PathRef File, Position Pos, - Callback> CB); + void locateSymbolAt(PathRef File, Position Pos, + Callback> CB); /// Helper function that returns a path to the corresponding source file when /// given a header file and vice versa. Index: clangd/ClangdServer.cpp =================================================================== --- clangd/ClangdServer.cpp +++ clangd/ClangdServer.cpp @@ -12,7 +12,6 @@ #include "Headers.h" #include "SourceCode.h" #include "Trace.h" -#include "XRefs.h" #include "index/FileIndex.h" #include "index/Merge.h" #include "clang/Format/Format.h" @@ -338,13 +337,13 @@ WorkScheduler.runWithAST("DumpAST", File, Bind(Action, std::move(Callback))); } -void ClangdServer::findDefinitions(PathRef File, Position Pos, - Callback> CB) { - auto Action = [Pos, this](Callback> CB, +void ClangdServer::locateSymbolAt(PathRef File, Position Pos, + Callback> CB) { + auto Action = [Pos, this](decltype(CB) CB, llvm::Expected InpAST) { if (!InpAST) return CB(InpAST.takeError()); - CB(clangd::findDefinitions(InpAST->AST, Pos, Index)); + CB(clangd::locateSymbolAt(InpAST->AST, Pos, Index)); }; WorkScheduler.runWithAST("Definitions", File, Bind(Action, std::move(CB))); Index: clangd/XRefs.h =================================================================== --- clangd/XRefs.h +++ clangd/XRefs.h @@ -22,9 +22,24 @@ namespace clang { namespace clangd { +// Describes where a symbol is declared and defined (as far as clangd knows). +// There are three cases: +// - a declaration only, no definition is known (e.g. only header seen) +// - a declaration and a distinct definition (e.g. function declared in header) +// - a declaration and an equal definition (e.g. inline function, or class) +struct LocatedSymbol { + // The (unqualified) name of the symbol. + std::string Name; + // The canonical or best declaration: where most users find its interface. + Location PreferredDeclaration; + // Where the symbol is defined, if known. May equal PreferredDeclaration. + llvm::Optional Definition; +}; +llvm::raw_ostream &operator<<(llvm::raw_ostream &, const LocatedSymbol &); /// Get definition of symbol at a specified \p Pos. -std::vector findDefinitions(ParsedAST &AST, Position Pos, - const SymbolIndex *Index = nullptr); +/// Multiple locations may be returned, corresponding to distinct symbols. +std::vector locateSymbolAt(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: clangd/XRefs.cpp =================================================================== --- clangd/XRefs.cpp +++ clangd/XRefs.cpp @@ -21,18 +21,24 @@ namespace clangd { namespace { -// Get the definition from a given declaration `D`. -// Return nullptr if no definition is found, or the declaration type of `D` is -// not supported. +// Returns the single definition of the entity declared by D, if visible. +// In particular: +// - for non-redeclarable kinds (e.g. local vars), return D +// - for kinds that allow multiple definitions (e.g. namespaces), return nullptr const Decl *getDefinition(const Decl *D) { assert(D); + // Decl has one definition that we can find. if (const auto *TD = dyn_cast(D)) return TD->getDefinition(); - else if (const auto *VD = dyn_cast(D)) + if (const auto *VD = dyn_cast(D)) return VD->getDefinition(); - else if (const auto *FD = dyn_cast(D)) + if (const auto *FD = dyn_cast(D)) return FD->getDefinition(); - return nullptr; + // Only a single declaration is allowed. + if (isa(D)) // except cases above + return D; + // Multiple definitions are allowed. + return nullptr; // except cases above } void logIfOverflow(const SymbolLocation &Loc) { @@ -240,8 +246,8 @@ } // namespace -std::vector findDefinitions(ParsedAST &AST, Position Pos, - const SymbolIndex *Index) { +std::vector locateSymbolAt(ParsedAST &AST, Position Pos, + const SymbolIndex *Index) { const auto &SM = AST.getASTContext().getSourceManager(); auto MainFilePath = getCanonicalPath(SM.getFileEntryForID(SM.getMainFileID()), SM); @@ -250,114 +256,84 @@ return {}; } - std::vector Result; - // Handle goto definition for #include. + // Treat #included files as symbols, to enable go-to-definition on them. for (auto &Inc : AST.getIncludeStructure().MainFileIncludes) { - if (!Inc.Resolved.empty() && Inc.R.start.line == Pos.line) - Result.push_back( - Location{URIForFile::canonicalize(Inc.Resolved, *MainFilePath), {}}); + if (!Inc.Resolved.empty() && Inc.R.start.line == Pos.line) { + LocatedSymbol File; + File.Name = llvm::sys::path::filename(Inc.Resolved); + File.PreferredDeclaration = { + URIForFile::canonicalize(Inc.Resolved, *MainFilePath), Range{}}; + File.Definition = File.PreferredDeclaration; + // We're not going to find any further symbols on #include lines. + return {std::move(File)}; + } } - if (!Result.empty()) - return Result; - // Identified symbols at a specific position. SourceLocation SourceLocationBeg = 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, *MainFilePath); - if (L) - Result.push_back(*L); + // Macros are simple: there's no declaration/definition distinction. + // As a consequence, there's no need to look them up in the index either. + std::vector Result; + for (auto M : Symbols.Macros) { + if (auto Loc = + makeLocation(AST, M.Info->getDefinitionLoc(), *MainFilePath)) { + LocatedSymbol Macro; + Macro.Name = M.Name; + Macro.PreferredDeclaration = *Loc; + Macro.Definition = Loc; + Result.push_back(std::move(Macro)); + } } - // 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: - // - // - We use the location from AST and index (if available) to provide the - // final results. When there are duplicate results, we prefer AST over - // index because AST is more up-to-date. - // - // - For each symbol, we will return a location of the canonical declaration - // (e.g. function declaration in header), and a location of definition if - // they are available. - // - // So the work flow: - // - // 1. Identify the symbols being search for by traversing the AST. - // 2. Populate one of the locations with the AST location. - // 3. Use the AST information to query the index, and populate the index - // location (if available). - // 4. Return all populated locations for all symbols, definition first ( - // which we think is the users wants most often). - struct CandidateLocation { - llvm::Optional Def; - llvm::Optional Decl; - }; - // We respect the order in Symbols.Decls. - llvm::SmallVector ResultCandidates; - llvm::DenseMap CandidatesIndex; + // Decls are more complicated. + // The AST contains at least a declaration, maybe a definition. + // These are up-to-date, and so generally preferred over index results. + // We perform a single batch index lookup to find additional definitions. + + // Results follow the order of Symbols.Decls. + // Keep track of SymbolID -> index mapping, to fill in index data later. + llvm::DenseMap ResultIndex; // Emit all symbol locations (declaration or definition) from AST. for (const DeclInfo &DI : Symbols.Decls) { const Decl *D = DI.D; - // Fake key for symbols don't have USR (no SymbolID). - // 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. + auto Loc = makeLocation(AST, findNameLoc(D), *MainFilePath); + if (!Loc) + continue; + + Result.emplace_back(); + if (auto *ND = dyn_cast(D)) + Result.back().Name = printName(AST.getASTContext(), *ND); + Result.back().PreferredDeclaration = *Loc; + // DeclInfo.D is always a definition if possible, so this check works. + if (getDefinition(D) == D) + Result.back().Definition = *Loc; + + // Record SymbolID for index lookup later. SymbolID Key(""); 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, *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; + ResultIndex[*ID] = Result.size() - 1; } - if (Index) { + // Now query the index for all Symbol IDs we found in the AST. + if (Index && !ResultIndex.empty()) { LookupRequest QueryRequest; - // Build request for index query, using SymbolID. - for (auto It : CandidatesIndex) + for (auto It : ResultIndex) QueryRequest.IDs.insert(It.first); - std::string TUPath; - const FileEntry *FE = SM.getFileEntryForID(SM.getMainFileID()); - if (auto Path = getCanonicalPath(FE, SM)) - TUPath = *Path; - // Query the index and populate the empty slot. - 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, TUPath); - if (!Value.Decl) - Value.Decl = toLSPLocation(Sym.CanonicalDeclaration, TUPath); - }); - } + Index->lookup(QueryRequest, [&](const Symbol &Sym) { + auto &R = Result[ResultIndex.lookup(Sym.ID)]; + + // Special case: if the AST yielded a definition, then it may not be + // the right *declaration*. Prefer the one from the index. + if (R.Definition) // from AST + if (auto Loc = toLSPLocation(Sym.CanonicalDeclaration, *MainFilePath)) + R.PreferredDeclaration = *Loc; - // Populate the results, definition first. - for (const auto &Candidate : ResultCandidates) { - if (Candidate.Def) - Result.push_back(*Candidate.Def); - if (Candidate.Decl && - Candidate.Decl != Candidate.Def) // Decl and Def might be the same - Result.push_back(*Candidate.Decl); + if (!R.Definition) + R.Definition = toLSPLocation(Sym.Definition, *MainFilePath); + }); } return Result; @@ -804,5 +780,12 @@ return Results; } +llvm::raw_ostream &operator<<(llvm::raw_ostream &OS, const LocatedSymbol &S) { + OS << S.Name << ": " << S.PreferredDeclaration; + if (S.Definition) + OS << " def=" << *S.Definition; + return OS; +} + } // namespace clangd } // namespace clang Index: test/clangd/initialize-params.test =================================================================== --- test/clangd/initialize-params.test +++ test/clangd/initialize-params.test @@ -14,6 +14,7 @@ # CHECK-NEXT: ":" # CHECK-NEXT: ] # CHECK-NEXT: }, +# CHECK-NEXT: "declarationProvider": true, # CHECK-NEXT: "definitionProvider": true, # CHECK-NEXT: "documentFormattingProvider": true, # CHECK-NEXT: "documentHighlightProvider": true, Index: unittests/clangd/ClangdTests.cpp =================================================================== --- unittests/clangd/ClangdTests.cpp +++ unittests/clangd/ClangdTests.cpp @@ -43,8 +43,9 @@ using ::testing::Pair; using ::testing::UnorderedElementsAre; -MATCHER_P2(FileRange, File, Range, "") { - return Location{URIForFile::canonicalize(File, testRoot()), Range} == arg; +MATCHER_P2(DeclAt, File, Range, "") { + return arg.PreferredDeclaration == + Location{URIForFile::canonicalize(File, testRoot()), Range}; } bool diagsContainErrors(const std::vector &Diagnostics) { @@ -458,10 +459,9 @@ UnorderedElementsAre(Pair(FooCpp, false), Pair(BarCpp, true), Pair(BazCpp, false))); - auto Locations = runFindDefinitions(Server, FooCpp, FooSource.point()); + auto Locations = runLocateSymbolAt(Server, FooCpp, FooSource.point()); EXPECT_TRUE(bool(Locations)); - EXPECT_THAT(*Locations, - ElementsAre(FileRange(FooCpp, FooSource.range("one")))); + EXPECT_THAT(*Locations, ElementsAre(DeclAt(FooCpp, FooSource.range("one")))); // Undefine MACRO, close baz.cpp. CDB.ExtraClangFlags.clear(); @@ -474,10 +474,9 @@ EXPECT_THAT(DiagConsumer.filesWithDiags(), UnorderedElementsAre(Pair(FooCpp, false), Pair(BarCpp, false))); - Locations = runFindDefinitions(Server, FooCpp, FooSource.point()); + Locations = runLocateSymbolAt(Server, FooCpp, FooSource.point()); EXPECT_TRUE(bool(Locations)); - EXPECT_THAT(*Locations, - ElementsAre(FileRange(FooCpp, FooSource.range("two")))); + EXPECT_THAT(*Locations, ElementsAre(DeclAt(FooCpp, FooSource.range("two")))); } TEST_F(ClangdVFSTest, MemoryUsage) { @@ -532,7 +531,7 @@ runAddDocument(Server, FooCpp, "int main() {}"); EXPECT_EQ(runDumpAST(Server, FooCpp), ""); - EXPECT_ERROR(runFindDefinitions(Server, FooCpp, Position())); + EXPECT_ERROR(runLocateSymbolAt(Server, FooCpp, Position())); EXPECT_ERROR(runFindDocumentHighlights(Server, FooCpp, Position())); EXPECT_ERROR(runRename(Server, FooCpp, Position(), "new_name")); // FIXME: codeComplete and signatureHelp should also return errors when they @@ -717,7 +716,7 @@ clangd::CodeCompleteOptions())); }; - auto FindDefinitionsRequest = [&]() { + auto LocateSymbolRequest = [&]() { unsigned FileIndex = FileIndexDist(RandGen); // Make sure we don't violate the ClangdServer's contract. if (ReqStats[FileIndex].FileIsRemoved) @@ -727,13 +726,13 @@ Pos.line = LineDist(RandGen); Pos.character = ColumnDist(RandGen); - ASSERT_TRUE(!!runFindDefinitions(Server, FilePaths[FileIndex], Pos)); + ASSERT_TRUE(!!runLocateSymbolAt(Server, FilePaths[FileIndex], Pos)); }; std::vector> AsyncRequests = { AddDocumentRequest, ForceReparseRequest, RemoveDocumentRequest}; std::vector> BlockingRequests = { - CodeCompletionRequest, FindDefinitionsRequest}; + CodeCompletionRequest, LocateSymbolRequest}; // Bash requests to ClangdServer in a loop. std::uniform_int_distribution AsyncRequestIndexDist( Index: unittests/clangd/SyncAPI.h =================================================================== --- unittests/clangd/SyncAPI.h +++ unittests/clangd/SyncAPI.h @@ -32,8 +32,8 @@ llvm::Expected runSignatureHelp(ClangdServer &Server, PathRef File, Position Pos); -llvm::Expected> -runFindDefinitions(ClangdServer &Server, PathRef File, Position Pos); +llvm::Expected> +runLocateSymbolAt(ClangdServer &Server, PathRef File, Position Pos); llvm::Expected> runFindDocumentHighlights(ClangdServer &Server, PathRef File, Position Pos); Index: unittests/clangd/SyncAPI.cpp =================================================================== --- unittests/clangd/SyncAPI.cpp +++ unittests/clangd/SyncAPI.cpp @@ -84,10 +84,10 @@ return std::move(*Result); } -llvm::Expected> -runFindDefinitions(ClangdServer &Server, PathRef File, Position Pos) { - llvm::Optional>> Result; - Server.findDefinitions(File, Pos, capture(Result)); +llvm::Expected> +runLocateSymbolAt(ClangdServer &Server, PathRef File, Position Pos) { + llvm::Optional>> Result; + Server.locateSymbolAt(File, Pos, capture(Result)); return std::move(*Result); } Index: unittests/clangd/TUSchedulerTests.cpp =================================================================== --- unittests/clangd/TUSchedulerTests.cpp +++ unittests/clangd/TUSchedulerTests.cpp @@ -685,10 +685,10 @@ // We schedule the following tasks in the queue: // [Update] [GoToDefinition] Server.addDocument(testPath("foo.cpp"), Code.code(), WantDiagnostics::Yes); - Server.findDefinitions(testPath("foo.cpp"), Code.point(), - [](Expected> Result) { - ASSERT_TRUE((bool)Result); - }); + Server.locateSymbolAt(testPath("foo.cpp"), Code.point(), + [](Expected> Result) { + ASSERT_TRUE((bool)Result); + }); ASSERT_TRUE(Server.blockUntilIdleForTest()); Index: unittests/clangd/XRefsTests.cpp =================================================================== --- unittests/clangd/XRefsTests.cpp +++ unittests/clangd/XRefsTests.cpp @@ -17,6 +17,7 @@ #include "index/SymbolCollector.h" #include "clang/Index/IndexingAction.h" #include "llvm/Support/Path.h" +#include "llvm/Support/ScopedPrinter.h" #include "gmock/gmock.h" #include "gtest/gtest.h" @@ -25,7 +26,6 @@ namespace { using testing::ElementsAre; -using testing::Field; using testing::IsEmpty; using testing::Matcher; using testing::UnorderedElementsAreArray; @@ -95,9 +95,35 @@ } } +MATCHER_P3(Sym, Name, Decl, DefOrNone, "") { + llvm::Optional Def = DefOrNone; + if (Name != arg.Name) { + *result_listener << "Name is " << arg.Name; + return false; + } + if (Decl != arg.PreferredDeclaration.range) { + *result_listener << "Declaration is " + << llvm::to_string(arg.PreferredDeclaration); + return false; + } + if (Def && !arg.Definition) { + *result_listener << "Has no definition"; + return false; + } + if (Def && arg.Definition->range != *Def) { + *result_listener << "Definition is " << llvm::to_string(arg.Definition); + return false; + } + return true; +} +testing::Matcher Sym(std::string Name, Range Decl) { + return Sym(Name, Decl, llvm::None); +} +MATCHER_P(Sym, Name, "") { return arg.Name == Name; } + MATCHER_P(RangeIs, R, "") { return arg.range == R; } -TEST(GoToDefinition, WithIndex) { +TEST(LocateSymbol, WithIndex) { Annotations SymbolHeader(R"cpp( class $forward[[Forward]]; class $foo[[Foo]] {}; @@ -115,9 +141,9 @@ TU.Code = SymbolCpp.code(); TU.HeaderCode = SymbolHeader.code(); auto Index = TU.index(); - auto runFindDefinitionsWithIndex = [&Index](const Annotations &Main) { + auto LocateWithIndex = [&Index](const Annotations &Main) { auto AST = TestTU::withCode(Main.code()).build(); - return clangd::findDefinitions(AST, Main.point(), Index.get()); + return clangd::locateSymbolAt(AST, Main.point(), Index.get()); }; Annotations Test(R"cpp(// only declaration in AST. @@ -126,9 +152,8 @@ ^f1(); } )cpp"); - EXPECT_THAT(runFindDefinitionsWithIndex(Test), - testing::ElementsAreArray( - {RangeIs(SymbolCpp.range("f1")), RangeIs(Test.range())})); + EXPECT_THAT(LocateWithIndex(Test), + ElementsAre(Sym("f1", Test.range(), SymbolCpp.range("f1")))); Test = Annotations(R"cpp(// definition in AST. void [[f1]]() {} @@ -136,30 +161,30 @@ ^f1(); } )cpp"); - EXPECT_THAT(runFindDefinitionsWithIndex(Test), - testing::ElementsAreArray( - {RangeIs(Test.range()), RangeIs(SymbolHeader.range("f1"))})); + EXPECT_THAT(LocateWithIndex(Test), + ElementsAre(Sym("f1", SymbolHeader.range("f1"), Test.range()))); 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(LocateWithIndex(Test), + ElementsAre(Sym("Foo", Test.range(), SymbolHeader.range("foo")))); Test = Annotations(R"cpp(// defintion in AST. class [[Forward]] {}; F^orward create(); )cpp"); - EXPECT_THAT(runFindDefinitionsWithIndex(Test), - testing::ElementsAreArray({ - RangeIs(Test.range()), - RangeIs(SymbolHeader.range("forward")), - })); + EXPECT_THAT( + LocateWithIndex(Test), + ElementsAre(Sym("Forward", SymbolHeader.range("forward"), Test.range()))); } -TEST(GoToDefinition, All) { +TEST(LocateSymbol, All) { + // Ranges in tests: + // $decl is the declaration location (if absent, no symbol is located) + // $def is the definition location (if absent, symbol has no definition) + // unnamed range becomes both $decl and $def. const char *Tests[] = { R"cpp(// Local variable int main() { @@ -186,7 +211,7 @@ )cpp", R"cpp(// Function declaration via call - int [[foo]](int); + int $decl[[foo]](int); int main() { return ^foo(42); } @@ -222,7 +247,7 @@ )cpp", R"cpp(// Method call - struct Foo { int [[x]](); }; + struct Foo { int $decl[[x]](); }; int main() { Foo bar; bar.^x(); @@ -230,7 +255,7 @@ )cpp", R"cpp(// Typedef - typedef int [[Foo]]; + typedef int $decl[[Foo]]; int main() { ^Foo bar; } @@ -243,7 +268,7 @@ )cpp", */ R"cpp(// Namespace - namespace [[ns]] { + namespace $decl[[ns]] { struct Foo { static void bar(); } } // namespace ns int main() { ^ns::Foo::bar(); } @@ -304,30 +329,45 @@ }; for (const char *Test : Tests) { Annotations T(Test); + llvm::Optional WantDecl; + llvm::Optional WantDef; + if (!T.ranges().empty()) + WantDecl = WantDef = T.range(); + if (!T.ranges("decl").empty()) + WantDecl = T.range("decl"); + if (!T.ranges("def").empty()) + WantDecl = T.range("def"); + 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 Results = locateSymbolAt(AST, T.point()); + + if (!WantDecl) { + EXPECT_THAT(Results, IsEmpty()) << Test; + } else { + ASSERT_THAT(Results, ::testing::SizeIs(1)) << Test; + EXPECT_EQ(Results[0].PreferredDeclaration.range, *WantDecl) << Test; + llvm::Optional GotDef; + if (Results[0].Definition) + GotDef = Results[0].Definition->range; + EXPECT_EQ(WantDef, GotDef) << Test; + } } } -TEST(GoToDefinition, Rank) { +TEST(LocateSymbol, Ambiguous) { auto T = Annotations(R"cpp( - struct $foo1[[Foo]] { - $foo2[[Foo]](); - $foo3[[Foo]](Foo&&); - $foo4[[Foo]](const char*); + struct Foo { + Foo(); + Foo(Foo&&); + Foo(const char*); }; - Foo $f[[f]](); + Foo f(); - void $g[[g]](Foo foo); + void g(Foo foo); void call() { - const char* $str[[str]] = "123"; + const char* str = "123"; Foo a = $1^str; Foo b = Foo($2^str); Foo c = $3^f(); @@ -336,26 +376,20 @@ } )cpp"); auto AST = TestTU::withCode(T.code()).build(); - EXPECT_THAT(findDefinitions(AST, T.point("1")), - ElementsAre(RangeIs(T.range("str")), RangeIs(T.range("foo4")))); - EXPECT_THAT(findDefinitions(AST, T.point("2")), - ElementsAre(RangeIs(T.range("str")))); - EXPECT_THAT(findDefinitions(AST, T.point("3")), - ElementsAre(RangeIs(T.range("f")), RangeIs(T.range("foo3")))); - EXPECT_THAT(findDefinitions(AST, T.point("4")), - ElementsAre(RangeIs(T.range("g")))); - EXPECT_THAT(findDefinitions(AST, T.point("5")), - ElementsAre(RangeIs(T.range("f")), RangeIs(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")))); + // Ordered assertions are deliberate: we expect a predictable order. + EXPECT_THAT(locateSymbolAt(AST, T.point("1")), + ElementsAre(Sym("str"), Sym("Foo"))); + EXPECT_THAT(locateSymbolAt(AST, T.point("2")), ElementsAre(Sym("str"))); + EXPECT_THAT(locateSymbolAt(AST, T.point("3")), + ElementsAre(Sym("f"), Sym("Foo"))); + EXPECT_THAT(locateSymbolAt(AST, T.point("4")), ElementsAre(Sym("g"))); + EXPECT_THAT(locateSymbolAt(AST, T.point("5")), + ElementsAre(Sym("f"), Sym("Foo"))); + EXPECT_THAT(locateSymbolAt(AST, T.point("6")), + ElementsAre(Sym("str"), Sym("Foo"), Sym("Foo"))); } -TEST(GoToDefinition, RelPathsInCompileCommand) { +TEST(LocateSymbol, RelPathsInCompileCommand) { // The source is in "/clangd-test/src". // We build in "/clangd-test/build". @@ -397,24 +431,23 @@ // Go to a definition in main source file. auto Locations = - runFindDefinitions(Server, FooCpp, SourceAnnotations.point("p1")); + runLocateSymbolAt(Server, FooCpp, SourceAnnotations.point("p1")); EXPECT_TRUE(bool(Locations)) << "findDefinitions returned an error"; - EXPECT_THAT(*Locations, - ElementsAre(FileRange(FooCpp, SourceAnnotations.range()))); + EXPECT_THAT(*Locations, ElementsAre(Sym("foo", SourceAnnotations.range()))); // Go to a definition in header_in_preamble.h. - Locations = runFindDefinitions(Server, FooCpp, SourceAnnotations.point("p2")); + Locations = runLocateSymbolAt(Server, FooCpp, SourceAnnotations.point("p2")); EXPECT_TRUE(bool(Locations)) << "findDefinitions returned an error"; - EXPECT_THAT(*Locations, - ElementsAre(FileRange(HeaderInPreambleH, - HeaderInPreambleAnnotations.range()))); + EXPECT_THAT( + *Locations, + ElementsAre(Sym("bar_preamble", HeaderInPreambleAnnotations.range()))); // Go to a definition in header_not_in_preamble.h. - Locations = runFindDefinitions(Server, FooCpp, SourceAnnotations.point("p3")); + Locations = runLocateSymbolAt(Server, FooCpp, SourceAnnotations.point("p3")); EXPECT_TRUE(bool(Locations)) << "findDefinitions returned an error"; EXPECT_THAT(*Locations, - ElementsAre(FileRange(HeaderNotInPreambleH, - HeaderNotInPreambleAnnotations.range()))); + ElementsAre(Sym("bar_not_preamble", + HeaderNotInPreambleAnnotations.range()))); } TEST(Hover, All) { @@ -1061,46 +1094,39 @@ 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(FileRange(FooH, HeaderAnnotations.range()))); + auto Locations = runLocateSymbolAt(Server, FooCpp, SourceAnnotations.point()); + ASSERT_TRUE(bool(Locations)) << "locateSymbolAt returned an error"; + EXPECT_THAT(*Locations, ElementsAre(Sym("foo.h", 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(FileRange(FooH, HeaderAnnotations.range()))); + Locations = runLocateSymbolAt(Server, FooCpp, SourceAnnotations.point("2")); + ASSERT_TRUE(bool(Locations)) << "locateSymbolAt returned an error"; + EXPECT_THAT(*Locations, ElementsAre(Sym("foo.h", HeaderAnnotations.range()))); - Locations = runFindDefinitions(Server, FooCpp, SourceAnnotations.point("3")); - ASSERT_TRUE(bool(Locations)) << "findDefinitions returned an error"; - EXPECT_THAT(*Locations, - ElementsAre(FileRange(FooH, HeaderAnnotations.range()))); + Locations = runLocateSymbolAt(Server, FooCpp, SourceAnnotations.point("3")); + ASSERT_TRUE(bool(Locations)) << "locateSymbolAt returned an error"; + EXPECT_THAT(*Locations, ElementsAre(Sym("foo.h", 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(FileRange(FooH, HeaderAnnotations.range()))); + Locations = runLocateSymbolAt(Server, FooCpp, SourceAnnotations.point("6")); + ASSERT_TRUE(bool(Locations)) << "locateSymbolAt returned an error"; + EXPECT_THAT(*Locations, ElementsAre(Sym("foo.h", 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"; + Locations = runLocateSymbolAt(Server, FooCpp, SourceAnnotations.point("4")); + ASSERT_TRUE(bool(Locations)) << "locateSymbolAt 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(FileRange(FooH, HeaderAnnotations.range()))); + Locations = runLocateSymbolAt(Server, FooCpp, SourceAnnotations.point("5")); + ASSERT_TRUE(bool(Locations)) << "locateSymbolAt returned an error"; + EXPECT_THAT(*Locations, ElementsAre(Sym("foo.h", HeaderAnnotations.range()))); - Locations = runFindDefinitions(Server, FooCpp, SourceAnnotations.point("7")); - ASSERT_TRUE(bool(Locations)) << "findDefinitions returned an error"; - EXPECT_THAT(*Locations, - ElementsAre(FileRange(FooH, HeaderAnnotations.range()))); + Locations = runLocateSymbolAt(Server, FooCpp, SourceAnnotations.point("7")); + ASSERT_TRUE(bool(Locations)) << "locateSymbolAt returned an error"; + EXPECT_THAT(*Locations, ElementsAre(Sym("foo.h", HeaderAnnotations.range()))); } -TEST(GoToDefinition, WithPreamble) { +TEST(LocateSymbol, WithPreamble) { // Test stragety: AST should always use the latest preamble instead of last // good preamble. MockFSProvider FS; @@ -1120,18 +1146,18 @@ FS.Files[FooH] = FooHeader.code(); runAddDocument(Server, FooCpp, FooWithHeader.code()); - // GoToDefinition goes to a #include file: the result comes from the preamble. + // LocateSymbol goes to a #include file: the result comes from the preamble. EXPECT_THAT( - cantFail(runFindDefinitions(Server, FooCpp, FooWithHeader.point())), - ElementsAre(FileRange(FooH, FooHeader.range()))); + cantFail(runLocateSymbolAt(Server, FooCpp, FooWithHeader.point())), + ElementsAre(Sym("foo.h", 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(FileRange(FooCpp, FooWithoutHeader.range()))); + cantFail(runLocateSymbolAt(Server, FooCpp, FooWithoutHeader.point())), + ElementsAre(Sym("foo", FooWithoutHeader.range()))); // Reset test environment. runAddDocument(Server, FooCpp, FooWithHeader.code()); @@ -1139,8 +1165,8 @@ Server.addDocument(FooCpp, FooWithoutHeader.code(), WantDiagnostics::Yes); // Use the AST being built in above request. EXPECT_THAT( - cantFail(runFindDefinitions(Server, FooCpp, FooWithoutHeader.point())), - ElementsAre(FileRange(FooCpp, FooWithoutHeader.range()))); + cantFail(runLocateSymbolAt(Server, FooCpp, FooWithoutHeader.point())), + ElementsAre(Sym("foo", FooWithoutHeader.range()))); } TEST(FindReferences, WithinAST) {