sammccall created this revision.
Herald added subscribers: kadircet, arphaman.
Herald added a project: All.
sammccall requested review of this revision.
Herald added subscribers: cfe-commits, wangpc, MaskRay, ilya-biryukov.
Herald added a project: clang-tools-extra.
Previously it'd ~always jump to the lexically first declaration, which was
often an unhelpful forward declaration.
- consult the index for definition and preferred declaration locations (query
code shared with go-to-definition)
- when unwrapping to LSP, prefer definition to declaration This matches
"go-to-definition", which is the most common go-to operation
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D155898
Files:
clang-tools-extra/clangd/ClangdLSPServer.cpp
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
Index: clang-tools-extra/clangd/unittests/XRefsTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/XRefsTests.cpp
+++ clang-tools-extra/clangd/unittests/XRefsTests.cpp
@@ -33,12 +33,10 @@
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 @@
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 @@
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, 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());
Index: clang-tools-extra/clangd/XRefs.h
===================================================================
--- clang-tools-extra/clangd/XRefs.h
+++ clang-tools-extra/clangd/XRefs.h
@@ -107,7 +107,8 @@
///
/// 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).
Index: clang-tools-extra/clangd/XRefs.cpp
===================================================================
--- clang-tools-extra/clangd/XRefs.cpp
+++ clang-tools-extra/clangd/XRefs.cpp
@@ -341,6 +341,46 @@
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 llvm::DenseMap<SymbolID, size_t> &ResultIndex,
+ const SymbolIndex *Index, llvm::StringRef MainFilePath) {
+ if (!Index || ResultIndex.empty())
+ return;
+ 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;
+ }
+ });
+}
+
// 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.
@@ -448,40 +488,7 @@
// 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, ResultIndex, Index, MainFilePath);
auto Overrides = findImplementors(VirtualMethods, RelationKind::OverriddenBy,
Index, MainFilePath);
@@ -490,7 +497,8 @@
}
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();
@@ -505,6 +513,8 @@
std::vector<LocatedSymbol> Results;
const auto &ASTContext = AST.getASTContext();
+ // Keep track of SymbolID -> index mapping, to fill in index data later.
+ llvm::DenseMap<SymbolID, size_t> ResultIndex;
for (const NamedDecl *D : Decls) {
D = getPreferredDecl(D);
@@ -519,7 +529,10 @@
if (const NamedDecl *Def = getDefinition(D))
Results.back().Definition =
makeLocation(ASTContext, nameLocation(*Def, SM), MainFilePath);
+ if (auto ID = getSymbolID(D))
+ ResultIndex[ID] = Results.size() - 1;
}
+ enhanceLocatedSymbolsFromIndex(Results, ResultIndex, Index, MainFilePath);
return Results;
}
@@ -784,7 +797,7 @@
// 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;
}
@@ -2056,13 +2069,13 @@
// 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;
@@ -2073,7 +2086,7 @@
}
// 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
@@ -2081,7 +2094,8 @@
// 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;
};
Index: clang-tools-extra/clangd/ClangdServer.cpp
===================================================================
--- clang-tools-extra/clangd/ClangdServer.cpp
+++ clang-tools-extra/clangd/ClangdServer.cpp
@@ -900,12 +900,12 @@
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));
}
Index: clang-tools-extra/clangd/ClangdLSPServer.cpp
===================================================================
--- clang-tools-extra/clangd/ClangdLSPServer.cpp
+++ clang-tools-extra/clangd/ClangdLSPServer.cpp
@@ -1452,7 +1452,7 @@
return Reply(Types.takeError());
std::vector<Location> Response;
for (const LocatedSymbol &Sym : *Types)
- Response.push_back(Sym.PreferredDeclaration);
+ Response.push_back(Sym.Definition.value_or(Sym.PreferredDeclaration));
return Reply(std::move(Response));
});
}
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
