Author: Sam McCall Date: 2023-07-21T23:10:33+02:00 New Revision: d9d9a2cb2f0db7a92eb7d5ef0c619fb41aa5c8a8
URL: https://github.com/llvm/llvm-project/commit/d9d9a2cb2f0db7a92eb7d5ef0c619fb41aa5c8a8 DIFF: https://github.com/llvm/llvm-project/commit/d9d9a2cb2f0db7a92eb7d5ef0c619fb41aa5c8a8.diff LOG: [clangd] Use index for go-to-type This ensures it finds the definition even if not visible. Differential Revision: https://reviews.llvm.org/D155898 Added: Modified: clang-tools-extra/clangd/ClangdServer.cpp clang-tools-extra/clangd/XRefs.cpp clang-tools-extra/clangd/XRefs.h clang-tools-extra/clangd/unittests/XRefsTests.cpp Removed: ################################################################################ diff --git a/clang-tools-extra/clangd/ClangdServer.cpp b/clang-tools-extra/clangd/ClangdServer.cpp index 29390196a6d977..d44d1e272b9b77 100644 --- a/clang-tools-extra/clangd/ClangdServer.cpp +++ b/clang-tools-extra/clangd/ClangdServer.cpp @@ -956,12 +956,12 @@ void ClangdServer::foldingRanges(llvm::StringRef File, void ClangdServer::findType(llvm::StringRef File, Position Pos, Callback<std::vector<LocatedSymbol>> CB) { - auto Action = - [Pos, CB = std::move(CB)](llvm::Expected<InputsAndAST> InpAST) mutable { - if (!InpAST) - return CB(InpAST.takeError()); - CB(clangd::findType(InpAST->AST, Pos)); - }; + auto Action = [Pos, CB = std::move(CB), + this](llvm::Expected<InputsAndAST> InpAST) mutable { + if (!InpAST) + return CB(InpAST.takeError()); + CB(clangd::findType(InpAST->AST, Pos, Index)); + }; WorkScheduler->runWithAST("FindType", File, std::move(Action)); } diff --git a/clang-tools-extra/clangd/XRefs.cpp b/clang-tools-extra/clangd/XRefs.cpp index 5853870db77918..da1a803daebb0d 100644 --- a/clang-tools-extra/clangd/XRefs.cpp +++ b/clang-tools-extra/clangd/XRefs.cpp @@ -341,6 +341,50 @@ std::vector<LocatedSymbol> findImplementors(llvm::DenseSet<SymbolID> IDs, return Results; } +// Given LocatedSymbol results derived from the AST, query the index to obtain +// definitions and preferred declarations. +void enhanceLocatedSymbolsFromIndex(llvm::MutableArrayRef<LocatedSymbol> Result, + const SymbolIndex *Index, + llvm::StringRef MainFilePath) { + LookupRequest QueryRequest; + llvm::DenseMap<SymbolID, unsigned> ResultIndex; + for (unsigned I = 0; I < Result.size(); ++I) { + if (auto ID = Result[I].ID) { + ResultIndex.try_emplace(ID, I); + QueryRequest.IDs.insert(ID); + } + } + if (!Index || QueryRequest.IDs.empty()) + return; + std::string Scratch; + Index->lookup(QueryRequest, [&](const Symbol &Sym) { + auto &R = Result[ResultIndex.lookup(Sym.ID)]; + + if (R.Definition) { // from AST + // Special case: if the AST yielded a definition, then it may not be + // the right *declaration*. Prefer the one from the index. + if (auto Loc = toLSPLocation(Sym.CanonicalDeclaration, MainFilePath)) + R.PreferredDeclaration = *Loc; + + // We might still prefer the definition from the index, e.g. for + // generated symbols. + if (auto Loc = toLSPLocation( + getPreferredLocation(*R.Definition, Sym.Definition, Scratch), + MainFilePath)) + R.Definition = *Loc; + } else { + R.Definition = toLSPLocation(Sym.Definition, MainFilePath); + + // Use merge logic to choose AST or index declaration. + if (auto Loc = toLSPLocation( + getPreferredLocation(R.PreferredDeclaration, + Sym.CanonicalDeclaration, Scratch), + MainFilePath)) + R.PreferredDeclaration = *Loc; + } + }); +} + // 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. @@ -352,8 +396,6 @@ locateASTReferent(SourceLocation CurLoc, const syntax::Token *TouchedIdentifier, const SourceManager &SM = AST.getSourceManager(); // Results follow the order of Symbols.Decls. std::vector<LocatedSymbol> Result; - // Keep track of SymbolID -> index mapping, to fill in index data later. - llvm::DenseMap<SymbolID, size_t> ResultIndex; static constexpr trace::Metric LocateASTReferentMetric( "locate_ast_referent", trace::Metric::Counter, "case"); @@ -371,10 +413,6 @@ locateASTReferent(SourceLocation CurLoc, const syntax::Token *TouchedIdentifier, if (const NamedDecl *Def = getDefinition(D)) Result.back().Definition = makeLocation( AST.getASTContext(), nameLocation(*Def, SM), MainFilePath); - - // Record SymbolID for index lookup later. - if (auto ID = getSymbolID(D)) - ResultIndex[ID] = Result.size() - 1; }; // Emit all symbol locations (declaration or definition) from AST. @@ -448,40 +486,7 @@ locateASTReferent(SourceLocation CurLoc, const syntax::Token *TouchedIdentifier, // Otherwise the target declaration is the right one. AddResultDecl(D); } - - // Now query the index for all Symbol IDs we found in the AST. - if (Index && !ResultIndex.empty()) { - LookupRequest QueryRequest; - for (auto It : ResultIndex) - QueryRequest.IDs.insert(It.first); - std::string Scratch; - Index->lookup(QueryRequest, [&](const Symbol &Sym) { - auto &R = Result[ResultIndex.lookup(Sym.ID)]; - - if (R.Definition) { // from AST - // Special case: if the AST yielded a definition, then it may not be - // the right *declaration*. Prefer the one from the index. - if (auto Loc = toLSPLocation(Sym.CanonicalDeclaration, MainFilePath)) - R.PreferredDeclaration = *Loc; - - // We might still prefer the definition from the index, e.g. for - // generated symbols. - if (auto Loc = toLSPLocation( - getPreferredLocation(*R.Definition, Sym.Definition, Scratch), - MainFilePath)) - R.Definition = *Loc; - } else { - R.Definition = toLSPLocation(Sym.Definition, MainFilePath); - - // Use merge logic to choose AST or index declaration. - if (auto Loc = toLSPLocation( - getPreferredLocation(R.PreferredDeclaration, - Sym.CanonicalDeclaration, Scratch), - MainFilePath)) - R.PreferredDeclaration = *Loc; - } - }); - } + enhanceLocatedSymbolsFromIndex(Result, Index, MainFilePath); auto Overrides = findImplementors(VirtualMethods, RelationKind::OverriddenBy, Index, MainFilePath); @@ -490,7 +495,8 @@ locateASTReferent(SourceLocation CurLoc, const syntax::Token *TouchedIdentifier, } std::vector<LocatedSymbol> locateSymbolForType(const ParsedAST &AST, - const QualType &Type) { + const QualType &Type, + const SymbolIndex *Index) { const auto &SM = AST.getSourceManager(); auto MainFilePath = AST.tuPath(); @@ -520,6 +526,7 @@ std::vector<LocatedSymbol> locateSymbolForType(const ParsedAST &AST, Results.back().Definition = makeLocation(ASTContext, nameLocation(*Def, SM), MainFilePath); } + enhanceLocatedSymbolsFromIndex(Results, Index, MainFilePath); return Results; } @@ -784,7 +791,7 @@ std::vector<LocatedSymbol> locateSymbolAt(ParsedAST &AST, Position Pos, // go-to-definition on auto should find the definition of the deduced // type, if possible if (auto Deduced = getDeducedType(AST.getASTContext(), Tok.location())) { - auto LocSym = locateSymbolForType(AST, *Deduced); + auto LocSym = locateSymbolForType(AST, *Deduced, Index); if (!LocSym.empty()) return LocSym; } @@ -2053,13 +2060,13 @@ static void unwrapFindType( // Convenience overload, to allow calling this without the out-parameter static llvm::SmallVector<QualType> unwrapFindType( QualType T, const HeuristicResolver* H) { - llvm::SmallVector<QualType> Result; - unwrapFindType(T, H, Result); - return Result; + llvm::SmallVector<QualType> Result; + unwrapFindType(T, H, Result); + return Result; } - -std::vector<LocatedSymbol> findType(ParsedAST &AST, Position Pos) { +std::vector<LocatedSymbol> findType(ParsedAST &AST, Position Pos, + const SymbolIndex *Index) { const SourceManager &SM = AST.getSourceManager(); auto Offset = positionToOffset(SM.getBufferData(SM.getMainFileID()), Pos); std::vector<LocatedSymbol> Result; @@ -2070,7 +2077,7 @@ std::vector<LocatedSymbol> findType(ParsedAST &AST, Position Pos) { } // The general scheme is: position -> AST node -> type -> declaration. auto SymbolsFromNode = - [&AST](const SelectionTree::Node *N) -> std::vector<LocatedSymbol> { + [&](const SelectionTree::Node *N) -> std::vector<LocatedSymbol> { std::vector<LocatedSymbol> LocatedSymbols; // NOTE: unwrapFindType might return duplicates for something like @@ -2078,7 +2085,8 @@ std::vector<LocatedSymbol> findType(ParsedAST &AST, Position Pos) { // information about the type you may have not known before // (since unique_ptr<unique_ptr<T>> != unique_ptr<T>). for (const QualType& Type : unwrapFindType(typeForNode(N), AST.getHeuristicResolver())) - llvm::copy(locateSymbolForType(AST, Type), std::back_inserter(LocatedSymbols)); + llvm::copy(locateSymbolForType(AST, Type, Index), + std::back_inserter(LocatedSymbols)); return LocatedSymbols; }; diff --git a/clang-tools-extra/clangd/XRefs.h b/clang-tools-extra/clangd/XRefs.h index d8a1dc9181f2f6..df91dd15303c18 100644 --- a/clang-tools-extra/clangd/XRefs.h +++ b/clang-tools-extra/clangd/XRefs.h @@ -107,7 +107,8 @@ std::vector<LocatedSymbol> findImplementations(ParsedAST &AST, Position Pos, /// /// For example, given `b^ar()` wher bar return Foo, this function returns the /// definition of class Foo. -std::vector<LocatedSymbol> findType(ParsedAST &AST, Position Pos); +std::vector<LocatedSymbol> findType(ParsedAST &AST, Position Pos, + const SymbolIndex *Index); /// Returns references of the symbol at a specified \p Pos. /// \p Limit limits the number of results returned (0 means no limit). diff --git a/clang-tools-extra/clangd/unittests/XRefsTests.cpp b/clang-tools-extra/clangd/unittests/XRefsTests.cpp index ac19bcce7ea914..05cf891e71db40 100644 --- a/clang-tools-extra/clangd/unittests/XRefsTests.cpp +++ b/clang-tools-extra/clangd/unittests/XRefsTests.cpp @@ -33,12 +33,10 @@ namespace clangd { namespace { using ::testing::AllOf; -using ::testing::Contains; using ::testing::ElementsAre; using ::testing::Eq; using ::testing::IsEmpty; using ::testing::Matcher; -using ::testing::Not; using ::testing::UnorderedElementsAre; using ::testing::UnorderedElementsAreArray; using ::testing::UnorderedPointwise; @@ -1879,7 +1877,7 @@ TEST(FindType, All) { ASSERT_GT(A.points().size(), 0u) << Case; for (auto Pos : A.points()) - EXPECT_THAT(findType(AST, Pos), + EXPECT_THAT(findType(AST, Pos, nullptr), ElementsAre( sym("Target", HeaderA.range("Target"), HeaderA.range("Target")))) << Case; @@ -1892,7 +1890,7 @@ TEST(FindType, All) { TU.Code = A.code().str(); ParsedAST AST = TU.build(); - EXPECT_THAT(findType(AST, A.point()), + EXPECT_THAT(findType(AST, A.point(), nullptr), UnorderedElementsAre( sym("Target", HeaderA.range("Target"), HeaderA.range("Target")), sym("smart_ptr", HeaderA.range("smart_ptr"), HeaderA.range("smart_ptr")) @@ -1901,6 +1899,39 @@ TEST(FindType, All) { } } +TEST(FindType, Definition) { + Annotations A(R"cpp( + class $decl[[X]]; + X *^x; + class $def[[X]] {}; + )cpp"); + auto TU = TestTU::withCode(A.code().str()); + ParsedAST AST = TU.build(); + + EXPECT_THAT(findType(AST, A.point(), nullptr), + ElementsAre(sym("X", A.range("decl"), A.range("def")))); +} + +TEST(FindType, Index) { + Annotations Def(R"cpp( + // This definition is only available through the index. + class [[X]] {}; + )cpp"); + TestTU DefTU = TestTU::withHeaderCode(Def.code()); + DefTU.HeaderFilename = "def.h"; + auto DefIdx = DefTU.index(); + + Annotations A(R"cpp( + class [[X]]; + X *^x; + )cpp"); + auto TU = TestTU::withCode(A.code().str()); + ParsedAST AST = TU.build(); + + EXPECT_THAT(findType(AST, A.point(), DefIdx.get()), + ElementsAre(sym("X", A.range(), Def.range()))); +} + void checkFindRefs(llvm::StringRef Test, bool UseIndex = false) { Annotations T(Test); auto TU = TestTU::withCode(T.code()); _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
