Index: clangd/XRefs.cpp =================================================================== --- clangd/XRefs.cpp +++ clangd/XRefs.cpp @@ -68,10 +68,24 @@ const MacroInfo *Info; }; +struct DeclInfo { + const Decl *D; + // Indicates the declaration is referenced by an explicit AST node. + bool IsReferencedExplicitly; + bool operator==(const DeclInfo &L) { + return std::tie(D, IsReferencedExplicitly) == + std::tie(L.D, L.IsReferencedExplicitly); + } +}; + /// Finds declarations locations that a given source location refers to. class DeclarationAndMacrosFinder : public index::IndexDataConsumer { - std::vector Decls; std::vector MacroInfos; + // The value of the map indicates whether the declaration has been referenced + // explicitly in the code. + // True means the declaration is explicitly referenced at least once; false + // otherwise + llvm::DenseMap Decls; const SourceLocation &SearchedLocation; const ASTContext * Preprocessor &PP; @@ -82,13 +96,17 @@ ASTContext &AST, Preprocessor &PP) : SearchedLocation(SearchedLocation), AST(AST), PP(PP) {} - std::vector takeDecls() { - // Don't keep the same declaration multiple times. - // This can happen when nodes in the AST are visited twice. - std::sort(Decls.begin(), Decls.end()); - auto Last = std::unique(Decls.begin(), Decls.end()); - Decls.erase(Last, Decls.end()); - return std::move(Decls); + std::vector getDecls() const { + std::vector Result; + for (auto It : Decls) + Result.push_back({It.first, It.second}); + + // Sort results. Declarations being referenced explicitly come first. + std::sort(Result.begin(), Result.end(), + [](const DeclInfo &L, const DeclInfo &R) { + return L.IsReferencedExplicitly > R.IsReferencedExplicitly; + }); + return Result; } std::vector takeMacroInfos() { @@ -106,21 +124,45 @@ return std::move(MacroInfos); } + void insertDecl(const Decl *D, bool IsExplicitReferenced) { + auto It = Decls.find(D); + if (It != Decls.end()) + It->getSecond() |= IsExplicitReferenced; + else + Decls.insert({D, IsExplicitReferenced}); + } + bool handleDeclOccurence(const Decl *D, index::SymbolRoleSet Roles, ArrayRef Relations, SourceLocation Loc, index::IndexDataConsumer::ASTNodeInfo ASTNode) override { if (Loc == SearchedLocation) { + // Check whether the E has an implicit AST node (e.g. ImplicitCastExpr). + auto hasImplicitExpr = [](const Expr *E) { + if (E && E->child_begin() != E->child_end()) { + // Use the first child is good enough for most cases -- normally the + // expression returned by handleDeclOccurence contains exactly one + // child expression. + const auto *FirstChild = *E->child_begin(); + return llvm::isa(FirstChild) || + llvm::isa(FirstChild) || + llvm::isa(FirstChild) || + llvm::isa(FirstChild); + } + return false; + }; + + bool IsExplicit = !hasImplicitExpr(ASTNode.OrigE); // Find and add definition declarations (for GoToDefinition). // We don't use parameter `D`, as Parameter `D` is the canonical // declaration, which is the first declaration of a redeclarable // declaration, and it could be a forward declaration. if (const auto *Def = getDefinition(D)) { - Decls.push_back(Def); + insertDecl(Def, IsExplicit); } else { // Couldn't find a definition, fall back to use `D`. - Decls.push_back(D); + insertDecl(D, IsExplicit); } } return true; @@ -158,7 +200,7 @@ }; struct IdentifiedSymbol { - std::vector Decls; + std::vector Decls; std::vector Macros; }; @@ -172,7 +214,7 @@ indexTopLevelDecls(AST.getASTContext(), AST.getLocalTopLevelDecls(), DeclMacrosFinder, IndexOpts); - return {DeclMacrosFinder.takeDecls(), DeclMacrosFinder.takeMacroInfos()}; + return {DeclMacrosFinder.getDecls(), DeclMacrosFinder.takeMacroInfos()}; } llvm::Optional @@ -253,10 +295,13 @@ llvm::Optional Def; llvm::Optional Decl; }; - llvm::DenseMap ResultCandidates; + // We respect the order in Symbols.Decls. + llvm::SmallVector ResultCandidates; + llvm::DenseMap CandidatesIndex; // Emit all symbol locations (declaration or definition) from AST. - for (const auto *D : Symbols.Decls) { + for (const auto &DI : Symbols.Decls) { + const auto *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 @@ -265,7 +310,11 @@ if (auto ID = getSymbolID(D)) Key = *ID; - auto &Candidate = ResultCandidates[Key]; + 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, SourceRange(Loc, Loc)); // The declaration in the identified symbols is a definition if possible @@ -281,7 +330,7 @@ if (Index) { LookupRequest QueryRequest; // Build request for index query, using SymbolID. - for (auto It : ResultCandidates) + for (auto It : CandidatesIndex) QueryRequest.IDs.insert(It.first); std::string HintPath; const FileEntry *FE = @@ -289,22 +338,21 @@ if (auto Path = getAbsoluteFilePath(FE, SourceMgr)) HintPath = *Path; // Query the index and populate the empty slot. - Index->lookup( - QueryRequest, [&HintPath, &ResultCandidates](const Symbol &Sym) { - auto It = ResultCandidates.find(Sym.ID); - assert(It != ResultCandidates.end()); - auto &Value = It->second; - - if (!Value.Def) - Value.Def = toLSPLocation(Sym.Definition, HintPath); - if (!Value.Decl) - Value.Decl = toLSPLocation(Sym.CanonicalDeclaration, HintPath); - }); + Index->lookup(QueryRequest, [&HintPath, &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); + if (!Value.Decl) + Value.Decl = toLSPLocation(Sym.CanonicalDeclaration, HintPath); + }); } // Populate the results, definition first. - for (auto It : ResultCandidates) { - const auto &Candidate = It.second; + for (const auto &Candidate : ResultCandidates) { if (Candidate.Def) Result.push_back(*Candidate.Def); if (Candidate.Decl && @@ -386,7 +434,9 @@ getBeginningOfIdentifier(AST, Pos, SourceMgr.getMainFileID()); auto Symbols = getSymbolAtPosition(AST, SourceLocationBeg); - std::vector SelectedDecls = Symbols.Decls; + std::vector SelectedDecls; + for (const auto &DI : Symbols.Decls) + SelectedDecls.push_back(DI.D); DocumentHighlightsFinder DocHighlightsFinder( llvm::errs(), AST.getASTContext(), AST.getPreprocessor(), SelectedDecls); @@ -651,7 +701,7 @@ return getHoverContents(Symbols.Macros[0].Name); if (!Symbols.Decls.empty()) - return getHoverContents(Symbols.Decls[0]); + return getHoverContents(Symbols.Decls[0].D); auto DeducedType = getDeducedType(AST, SourceLocationBeg); if (DeducedType && !DeducedType->isNull()) Index: unittests/clangd/XRefsTests.cpp =================================================================== --- unittests/clangd/XRefsTests.cpp +++ unittests/clangd/XRefsTests.cpp @@ -311,6 +311,50 @@ } } +TEST(GoToDefinition, Rank) { + auto T = Annotations(R"cpp( + struct $foo1[[Foo]] { + $foo2[[Foo]](); + $foo3[[Foo]](Foo&&); + $foo4[[Foo]](const char*); + }; + + Foo $f[[f]](); + + void $g[[g]](Foo foo); + + void call() { + const char* $str[[str]] = "123"; + Foo a = $1^str; + Foo b = Foo($2^str); + Foo c = $3^f(); + $4^g($5^f()); + g($6^str); + } + )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")))); +} + TEST(GoToDefinition, RelPathsInCompileCommand) { Annotations SourceAnnotations(R"cpp( int [[foo]];