clangd: Extend index references with constructor calls through forwarding #169742
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Constructor calls hidden behind make_unique are currently not found, but very useful, this test expects to find them in the main index.
timon-ul
changed the title
Member
|
@llvm/pr-subscribers-clang-tools-extra @llvm/pr-subscribers-clangd Author: None (timon-ul) ChangesFirst step for clangd/clangd#716 (comment) , the missing part is finding the same information directly in the AST. Not sure about the location of the new visitor and the new references collection. Also I used Full diff: https://github.com/llvm/llvm-project/pull/169742.diff 5 Files Affected:
diff --git a/clang-tools-extra/clangd/AST.cpp b/clang-tools-extra/clangd/AST.cpp
index 0dcff2eae05e7..4b73bdba8aaa9 100644
--- a/clang-tools-extra/clangd/AST.cpp
+++ b/clang-tools-extra/clangd/AST.cpp
@@ -1040,5 +1040,26 @@ bool isExpandedFromParameterPack(const ParmVarDecl *D) {
return getUnderlyingPackType(D) != nullptr;
}
+bool isLikelyForwardingFunction(FunctionTemplateDecl *FT) {
+ const auto *FD = FT->getTemplatedDecl();
+ const auto NumParams = FD->getNumParams();
+ // Check whether its last parameter is a parameter pack...
+ if (NumParams > 0) {
+ const auto *LastParam = FD->getParamDecl(NumParams - 1);
+ if (const auto *PET = dyn_cast<PackExpansionType>(LastParam->getType())) {
+ // ... of the type T&&... or T...
+ const auto BaseType = PET->getPattern().getNonReferenceType();
+ if (const auto *TTPT =
+ dyn_cast<TemplateTypeParmType>(BaseType.getTypePtr())) {
+ // ... whose template parameter comes from the function directly
+ if (FT->getTemplateParameters()->getDepth() == TTPT->getDepth()) {
+ return true;
+ }
+ }
+ }
+ }
+ return false;
+}
+
} // namespace clangd
} // namespace clang
diff --git a/clang-tools-extra/clangd/AST.h b/clang-tools-extra/clangd/AST.h
index 2b83595e5b8e9..af45ae2d9022d 100644
--- a/clang-tools-extra/clangd/AST.h
+++ b/clang-tools-extra/clangd/AST.h
@@ -253,6 +253,10 @@ resolveForwardingParameters(const FunctionDecl *D, unsigned MaxDepth = 10);
/// reference to one (e.g. `Args&...` or `Args&&...`).
bool isExpandedFromParameterPack(const ParmVarDecl *D);
+/// Heuristic that checks if FT is forwarding a parameter pack to another
+/// function. (e.g. `make_unique`).
+bool isLikelyForwardingFunction(FunctionTemplateDecl *FT);
+
} // namespace clangd
} // namespace clang
diff --git a/clang-tools-extra/clangd/Preamble.cpp b/clang-tools-extra/clangd/Preamble.cpp
index 8af9e4649218d..09aaf3290b585 100644
--- a/clang-tools-extra/clangd/Preamble.cpp
+++ b/clang-tools-extra/clangd/Preamble.cpp
@@ -7,6 +7,7 @@
//===----------------------------------------------------------------------===//
#include "Preamble.h"
+#include "AST.h"
#include "CollectMacros.h"
#include "Compiler.h"
#include "Config.h"
@@ -166,27 +167,6 @@ class CppFilePreambleCallbacks : public PreambleCallbacks {
collectPragmaMarksCallback(*SourceMgr, Marks));
}
- static bool isLikelyForwardingFunction(FunctionTemplateDecl *FT) {
- const auto *FD = FT->getTemplatedDecl();
- const auto NumParams = FD->getNumParams();
- // Check whether its last parameter is a parameter pack...
- if (NumParams > 0) {
- const auto *LastParam = FD->getParamDecl(NumParams - 1);
- if (const auto *PET = dyn_cast<PackExpansionType>(LastParam->getType())) {
- // ... of the type T&&... or T...
- const auto BaseType = PET->getPattern().getNonReferenceType();
- if (const auto *TTPT =
- dyn_cast<TemplateTypeParmType>(BaseType.getTypePtr())) {
- // ... whose template parameter comes from the function directly
- if (FT->getTemplateParameters()->getDepth() == TTPT->getDepth()) {
- return true;
- }
- }
- }
- }
- return false;
- }
-
bool shouldSkipFunctionBody(Decl *D) override {
// Usually we don't need to look inside the bodies of header functions
// to understand the program. However when forwarding function like
diff --git a/clang-tools-extra/clangd/index/SymbolCollector.cpp b/clang-tools-extra/clangd/index/SymbolCollector.cpp
index 39c479b5f4d5b..11afd16e5f99d 100644
--- a/clang-tools-extra/clangd/index/SymbolCollector.cpp
+++ b/clang-tools-extra/clangd/index/SymbolCollector.cpp
@@ -29,6 +29,7 @@
#include "clang/AST/DeclTemplate.h"
#include "clang/AST/DeclarationName.h"
#include "clang/AST/Expr.h"
+#include "clang/AST/RecursiveASTVisitor.h"
#include "clang/Basic/FileEntry.h"
#include "clang/Basic/LangOptions.h"
#include "clang/Basic/SourceLocation.h"
@@ -576,6 +577,22 @@ SymbolCollector::getRefContainer(const Decl *Enclosing,
return Enclosing;
}
+class ForwardingToConstructorVisitor
+ : public RecursiveASTVisitor<ForwardingToConstructorVisitor> {
+public:
+ ForwardingToConstructorVisitor() {}
+
+ bool VisitCXXConstructExpr(CXXConstructExpr *E) {
+ if (auto *Callee = E->getConstructor()) {
+ Constructors.push_back(Callee);
+ }
+ return true;
+ }
+
+ // Output of this visitor
+ std::vector<CXXConstructorDecl *> Constructors{};
+};
+
// Always return true to continue indexing.
bool SymbolCollector::handleDeclOccurrence(
const Decl *D, index::SymbolRoleSet Roles,
@@ -669,6 +686,27 @@ bool SymbolCollector::handleDeclOccurrence(
addRef(ID, SymbolRef{FileLoc, FID, Roles, index::getSymbolInfo(ND).Kind,
getRefContainer(ASTNode.Parent, Opts),
isSpelled(FileLoc, *ND)});
+ // Also collect indirect constructor calls like `make_unique`
+ if (auto *FD = llvm::dyn_cast<clang::FunctionDecl>(D)) {
+ if (auto *FT = FD->getDescribedFunctionTemplate();
+ FT && isLikelyForwardingFunction(FT)) {
+ ForwardingToConstructorVisitor Visitor{};
+ for (auto *Specialized : FT->specializations()) {
+ Visitor.TraverseStmt(Specialized->getBody());
+ }
+ auto FileLoc = SM.getFileLoc(Loc);
+ auto FID = SM.getFileID(FileLoc);
+ if (Opts.RefsInHeaders || FID == SM.getMainFileID()) {
+ for (auto *Constructor : Visitor.Constructors) {
+ addRef(getSymbolIDCached(Constructor),
+ SymbolRef{FileLoc, FID, Roles,
+ index::getSymbolInfo(ND).Kind,
+ getRefContainer(ASTNode.Parent, Opts),
+ isSpelled(FileLoc, *ND)});
+ }
+ }
+ }
+ }
}
}
// Don't continue indexing if this is a mere reference.
diff --git a/clang-tools-extra/clangd/unittests/XRefsTests.cpp b/clang-tools-extra/clangd/unittests/XRefsTests.cpp
index 7ed08d7cce3d3..51e29f5f1af43 100644
--- a/clang-tools-extra/clangd/unittests/XRefsTests.cpp
+++ b/clang-tools-extra/clangd/unittests/XRefsTests.cpp
@@ -13,6 +13,7 @@
#include "SyncAPI.h"
#include "TestFS.h"
#include "TestTU.h"
+#include "TestWorkspace.h"
#include "XRefs.h"
#include "index/MemIndex.h"
#include "clang/AST/Decl.h"
@@ -2713,6 +2714,36 @@ TEST(FindReferences, NoQueryForLocalSymbols) {
}
}
+TEST(FindReferences, ForwardingInIndex) {
+ Annotations Header(R"cpp(
+ namespace std {
+ template <class T> T &&forward(T &t);
+ template <class T, class... Args> T *make_unique(Args &&...args) {
+ return new T(std::forward<Args>(args)...);
+ }
+ }
+ struct Test {
+ [[T^est]](){}
+ };
+ )cpp");
+ Annotations Main(R"cpp(
+ #include "header.hpp"
+ int main() {
+ auto a = std::[[make_unique]]<Test>();
+ }
+ )cpp");
+ TestWorkspace TW;
+ TW.addMainFile("header.hpp", Header.code());
+ TW.addMainFile("main.cpp", Main.code());
+ auto AST = TW.openFile("header.hpp").value();
+ auto Index = TW.index();
+
+ EXPECT_THAT(findReferences(AST, Header.point(), 0, Index.get(),
+ /*AddContext*/ true)
+ .References,
+ ElementsAre(rangeIs(Header.range()), rangeIs(Main.range())));
+}
+
TEST(GetNonLocalDeclRefs, All) {
struct Case {
llvm::StringRef AnnotatedCode;
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
Contributor
Author
|
Interesting side effect I noticed when applying this to a project: the |
Contributor
Author
|
Laying in bed yesterday I realized that yeah I need to fix that, basically all instantiations of such a template, even if they have the wrong type parameter will be returned now, which is definitely not intended behaviour, the code is missing a way to say "only record a reference for this specialization" and not "for all specializations". |
idea is to directly at the instantiation index the template for constructor calls. For some reason the function declaration never is an instantiation though.
🐧 Linux x64 Test Results
Failed Tests(click on a test name to see its output) Clangd Unit TestsClangd Unit Tests._/ClangdTests/FindReferences/ForwardingInIndexIf these failures are unrelated to your changes (for example tests are broken or flaky at HEAD), please open an issue at https://github.com/llvm/llvm-project/issues and add the |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
First step for clangd/clangd#716 (comment) , the missing part is finding the same information directly in the AST.
Not sure about the location of the new visitor and the new references collection. Also I used
std::vectorwhich I see never used so if I should have used a different data structure please tell me.