llvmorg-github-actions[bot] wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang-tools-extra Author: Yuli Fiterman (fitermay) <details> <summary>Changes</summary> ## Summary When the user invokes **Go to Definition** on a call like `std::make_unique<T>(args...)` or `std::make_shared<T>(args...)`, surface the constructor of `T` that is actually invoked inside the wrapper, alongside the wrapper itself. The constructor is added before the wrapper so LSP clients that auto-jump to the first target land on it; clients that present a menu still let the user reach the wrapper. This is the forward-direction counterpart to the find-references work in #<!-- -->169742 (clangd/clangd#<!-- -->716): the same `isLikelyForwardingFunction` + `searchConstructorsInForwardingFunction` machinery, applied to `locateASTReferent`. ## Changes - Extract `getForwardedConstructors(FD, cache)` in `AST.h`/`AST.cpp` from the duplicated bodies of `ReferenceFinder::forwardsToConstructor` (XRefs.cpp) and `SymbolCollector::findIndirectConstructors` (`index/SymbolCollector.cpp`). Both existing call sites now delegate to the shared helper — no semantic change for find-references or the offline index. - In `locateASTReferent`, when the targeted candidate is a function-template pattern whose described template is a likely forwarding function, walk the `SelectionTree` to find the enclosing `CallExpr` and resolve from the actual specialization (the relation filter strips `TemplateInstantiation` candidates, so the pattern is what we receive). The forwarded ctors are added before the default `AddResultDecl(D)`. - Tests in `XRefsTests.cpp` cover: - `make_unique` - `make_shared` - Chained forwarding (three nested wrappers) - Constructor selection driven by overload resolution inside the instantiated body - Negative case: a non-forwarding template call gains no extra target ## Limitations (inherited from the find-refs infrastructure) - The visitor only follows `CXXNewExpr` and only recurses through nested calls whose callee is itself a forwarding function template. Wrappers that go through a non-template intermediate, or that construct via something other than a `new`-expression, will not be resolved. - Recursion is capped at depth 10. - Requires `PreambleParseForwardingFunctions=true` (the default) so the wrapper bodies are kept in the preamble. ## Follow-ups (not addressed here) The same helper can be wired into: - `textDocument/declaration` - `textDocument/implementation` Happy to do these in separate PRs if reviewers prefer; left out to keep this change focused on `textDocument/definition`. ## Test plan - [x] New `LocateSymbol.ConstructorForwarding*` tests pass (5 tests). - [x] Full `ClangdTests` suite passes (1388 tests; one pre-existing unrelated crash in `ExpandDeducedTypeTest.Test` excluded). - [x] `clang-format` clean on the diff. ## AI Use - Assisted by [Google Antigravity](https://antigravity.google/) with strict human supervision, --- Full diff: https://github.com/llvm/llvm-project/pull/199480.diff 5 Files Affected: - (modified) clang-tools-extra/clangd/AST.cpp (+15) - (modified) clang-tools-extra/clangd/AST.h (+21) - (modified) clang-tools-extra/clangd/XRefs.cpp (+42-21) - (modified) clang-tools-extra/clangd/index/SymbolCollector.cpp (+3-15) - (modified) clang-tools-extra/clangd/unittests/XRefsTests.cpp (+138) ``````````diff diff --git a/clang-tools-extra/clangd/AST.cpp b/clang-tools-extra/clangd/AST.cpp index 2ebc9b49cac55..1c5ef5c10e98f 100644 --- a/clang-tools-extra/clangd/AST.cpp +++ b/clang-tools-extra/clangd/AST.cpp @@ -1115,5 +1115,20 @@ searchConstructorsInForwardingFunction(const FunctionDecl *FD) { return Result; } +llvm::ArrayRef<const CXXConstructorDecl *> +getForwardedConstructors(const FunctionDecl *FD, + ForwardingToConstructorCache &Cache) { + if (FD == nullptr || !FD->isTemplateInstantiation()) + return {}; + if (auto It = Cache.find(FD); It != Cache.end()) + return It->getSecond(); + const auto *PT = FD->getPrimaryTemplate(); + if (PT == nullptr || !isLikelyForwardingFunction(PT)) + return {}; + auto Inserted = + Cache.try_emplace(FD, searchConstructorsInForwardingFunction(FD)); + return Inserted.first->getSecond(); +} + } // namespace clangd } // namespace clang diff --git a/clang-tools-extra/clangd/AST.h b/clang-tools-extra/clangd/AST.h index 2bb4943b6de0b..e55517a7e005a 100644 --- a/clang-tools-extra/clangd/AST.h +++ b/clang-tools-extra/clangd/AST.h @@ -21,6 +21,9 @@ #include "clang/AST/TypeLoc.h" #include "clang/Basic/SourceLocation.h" #include "clang/Lex/MacroInfo.h" +#include "llvm/ADT/ArrayRef.h" +#include "llvm/ADT/DenseMap.h" +#include "llvm/ADT/SmallVector.h" #include "llvm/ADT/StringRef.h" #include <optional> #include <string> @@ -261,6 +264,24 @@ bool isLikelyForwardingFunction(const FunctionTemplateDecl *FT); SmallVector<const CXXConstructorDecl *, 1> searchConstructorsInForwardingFunction(const FunctionDecl *FD); +/// Cache mapping forwarding function instantiations (e.g. `make_unique<T>`) +/// to the constructors they ultimately call. +using ForwardingToConstructorCache = + llvm::DenseMap<const FunctionDecl *, + SmallVector<const CXXConstructorDecl *, 1>>; + +/// Returns the constructors that FD forwards to, if FD is a template +/// instantiation of a likely forwarding function (see +/// `isLikelyForwardingFunction`). Results are memoized in `Cache`. Returns +/// an empty range for non-forwarding functions; the cache is only populated +/// for true forwarding instantiations to keep its size bounded. +/// +/// The returned ArrayRef is owned by `Cache`; consume it before any other +/// call that may insert into the same cache. +llvm::ArrayRef<const CXXConstructorDecl *> +getForwardedConstructors(const FunctionDecl *FD, + ForwardingToConstructorCache &Cache); + } // namespace clangd } // namespace clang diff --git a/clang-tools-extra/clangd/XRefs.cpp b/clang-tools-extra/clangd/XRefs.cpp index 5f64f01c1ff34..6538174edbe5a 100644 --- a/clang-tools-extra/clangd/XRefs.cpp +++ b/clang-tools-extra/clangd/XRefs.cpp @@ -506,6 +506,44 @@ locateASTReferent(SourceLocation CurLoc, const syntax::Token *TouchedIdentifier, AddResultDecl(ID); } + // Special case: navigate through forwarding functions (e.g. + // `std::make_unique<T>(...)`) to the constructor of `T` that they call. + // The targeted forwarding function itself is still added below so users + // can fall back to it. The candidate `D` is the function template pattern + // (the relation filter strips instantiations), so we walk the + // SelectionTree to locate the call's specialization and resolve from it. + if (const auto *FD = dyn_cast<FunctionDecl>(D)) { + if (const auto *FT = FD->getDescribedFunctionTemplate(); + FT && isLikelyForwardingFunction(FT)) { + unsigned Offset = SM.getDecomposedSpellingLoc(CurLoc).second; + const FunctionTemplateDecl *TargetFT = FT->getCanonicalDecl(); + llvm::SmallPtrSet<const CXXConstructorDecl *, 1> Seen; + SelectionTree::createEach( + AST.getASTContext(), AST.getTokens(), Offset, Offset, + [&](SelectionTree ST) { + for (const auto *Cur = ST.commonAncestor(); Cur; + Cur = Cur->Parent) { + const auto *CE = Cur->ASTNode.get<CallExpr>(); + if (!CE) + continue; + const auto *Callee = CE->getDirectCallee(); + if (!Callee || !Callee->getPrimaryTemplate() || + Callee->getPrimaryTemplate()->getCanonicalDecl() != + TargetFT) + continue; + for (const auto *Ctor : getForwardedConstructors( + Callee, AST.ForwardingToConstructorCache)) + if (Seen.insert(Ctor).second) { + LocateASTReferentMetric.record(1, "forwarded-constructor"); + AddResultDecl(Ctor); + } + return true; + } + return false; + }); + } + } + LocateASTReferentMetric.record(1, "regular"); // Otherwise the target declaration is the right one. AddResultDecl(D); @@ -966,27 +1004,10 @@ class ReferenceFinder : public index::IndexDataConsumer { bool forwardsToConstructor(const Decl *D) { if (TargetConstructors.empty()) return false; - auto *FD = llvm::dyn_cast<clang::FunctionDecl>(D); - if (FD == nullptr || !FD->isTemplateInstantiation()) - return false; - - SmallVector<const CXXConstructorDecl *, 1> *Constructors = nullptr; - if (auto Entry = AST.ForwardingToConstructorCache.find(FD); - Entry != AST.ForwardingToConstructorCache.end()) - Constructors = &Entry->getSecond(); - if (Constructors == nullptr) { - if (auto *PT = FD->getPrimaryTemplate(); - PT == nullptr || !isLikelyForwardingFunction(PT)) - return false; - - SmallVector<const CXXConstructorDecl *, 1> FoundConstructors = - searchConstructorsInForwardingFunction(FD); - auto Iter = AST.ForwardingToConstructorCache.try_emplace( - FD, std::move(FoundConstructors)); - Constructors = &Iter.first->getSecond(); - } - for (auto *Constructor : *Constructors) - if (TargetConstructors.contains(Constructor)) + const auto *FD = llvm::dyn_cast<clang::FunctionDecl>(D); + for (const auto *Ctor : + getForwardedConstructors(FD, AST.ForwardingToConstructorCache)) + if (TargetConstructors.contains(Ctor)) return true; return false; } diff --git a/clang-tools-extra/clangd/index/SymbolCollector.cpp b/clang-tools-extra/clangd/index/SymbolCollector.cpp index bd974e4c18818..4f31a68074e5a 100644 --- a/clang-tools-extra/clangd/index/SymbolCollector.cpp +++ b/clang-tools-extra/clangd/index/SymbolCollector.cpp @@ -578,21 +578,9 @@ SymbolCollector::getRefContainer(const Decl *Enclosing, SmallVector<const CXXConstructorDecl *, 1> SymbolCollector::findIndirectConstructors(const Decl *D) { - auto *FD = llvm::dyn_cast<clang::FunctionDecl>(D); - if (FD == nullptr || !FD->isTemplateInstantiation()) - return {}; - if (auto Entry = ForwardingToConstructorCache.find(FD); - Entry != ForwardingToConstructorCache.end()) - return Entry->getSecond(); - if (auto *PT = FD->getPrimaryTemplate(); - PT == nullptr || !isLikelyForwardingFunction(PT)) - return {}; - - SmallVector<const CXXConstructorDecl *, 1> FoundConstructors = - searchConstructorsInForwardingFunction(FD); - auto Iter = ForwardingToConstructorCache.try_emplace( - FD, std::move(FoundConstructors)); - return Iter.first->getSecond(); + const auto *FD = llvm::dyn_cast<clang::FunctionDecl>(D); + auto Forwarded = getForwardedConstructors(FD, ForwardingToConstructorCache); + return {Forwarded.begin(), Forwarded.end()}; } // Always return true to continue indexing. diff --git a/clang-tools-extra/clangd/unittests/XRefsTests.cpp b/clang-tools-extra/clangd/unittests/XRefsTests.cpp index 7f31a1e97d5d7..ef72512abb7b3 100644 --- a/clang-tools-extra/clangd/unittests/XRefsTests.cpp +++ b/clang-tools-extra/clangd/unittests/XRefsTests.cpp @@ -34,6 +34,7 @@ namespace clangd { namespace { using ::testing::AllOf; +using ::testing::Contains; using ::testing::ElementsAre; using ::testing::Eq; using ::testing::IsEmpty; @@ -2939,6 +2940,143 @@ TEST(FindReferences, TemplatedConstructorForwarding) { rangeIs(Main.range("ForwardedCaller2")))); } +TEST(LocateSymbol, ConstructorForwarding) { + Annotations Code(R"cpp( + namespace std { + template <class T> T &&forward(T &t); + template <class T, class... Args> + T *$MakeUnique[[make_unique]](Args &&...args) { + return new T(std::forward<Args>(args)...); + } + } + + struct Test { + $Ctor[[Test]]() {} + }; + + int main() { + auto a = std::ma^ke_unique<Test>(); + } + )cpp"); + TestTU TU = TestTU::withCode(Code.code()); + auto AST = TU.build(); + EXPECT_THAT( + locateSymbolAt(AST, Code.point()), + UnorderedElementsAre(sym("Test", Code.range("Ctor"), Code.range("Ctor")), + sym("make_unique", Code.range("MakeUnique"), + Code.range("MakeUnique")))); +} + +TEST(LocateSymbol, ConstructorForwardingMakeShared) { + Annotations Code(R"cpp( + namespace std { + template <class T> T &&forward(T &t); + template <class T> struct shared_ptr { + shared_ptr(T *) {} + }; + template <class T, class... Args> + shared_ptr<T> $MakeShared[[make_shared]](Args &&...args) { + return shared_ptr<T>(new T(std::forward<Args>(args)...)); + } + } + + struct Test { + $Ctor[[Test]](int) {} + }; + + int main() { + auto a = std::ma^ke_shared<Test>(1); + } + )cpp"); + TestTU TU = TestTU::withCode(Code.code()); + auto AST = TU.build(); + EXPECT_THAT( + locateSymbolAt(AST, Code.point()), + UnorderedElementsAre(sym("Test", Code.range("Ctor"), Code.range("Ctor")), + sym("make_shared", Code.range("MakeShared"), + Code.range("MakeShared")))); +} + +TEST(LocateSymbol, ConstructorForwardingChained) { + Annotations Code(R"cpp( + namespace std { + template <class T> T &&forward(T &t); + template <class T, class... Args> T *make_unique(Args &&...args) { + return new T(forward<Args>(args)...); + } + template <class T, class... Args> T *make_unique2(Args &&...args) { + return make_unique<T>(forward<Args>(args)...); + } + template <class T, class... Args> + T *$MakeUnique3[[make_unique3]](Args &&...args) { + return make_unique2<T>(forward<Args>(args)...); + } + } + + struct Test { + $Ctor[[Test]]() {} + }; + + int main() { + auto a = std::ma^ke_unique3<Test>(); + } + )cpp"); + TestTU TU = TestTU::withCode(Code.code()); + auto AST = TU.build(); + EXPECT_THAT( + locateSymbolAt(AST, Code.point()), + UnorderedElementsAre(sym("Test", Code.range("Ctor"), Code.range("Ctor")), + sym("make_unique3", Code.range("MakeUnique3"), + Code.range("MakeUnique3")))); +} + +TEST(LocateSymbol, ConstructorForwardingPicksCalledOverload) { + // Overload resolution inside the instantiated body decides which + // constructor make_unique<Test>(...) actually invokes. + Annotations Code(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 { + $IntCtor[[Test]](int) {} + Test(const char *) {} + }; + + int main() { + auto a = std::ma^ke_unique<Test>(42); + } + )cpp"); + TestTU TU = TestTU::withCode(Code.code()); + auto AST = TU.build(); + EXPECT_THAT( + locateSymbolAt(AST, Code.point()), + Contains(sym("Test", Code.range("IntCtor"), Code.range("IntCtor")))); +} + +TEST(LocateSymbol, ConstructorForwardingNotTriggeredOnUnrelatedCall) { + // A regular (non-forwarding) function call should not gain extra targets. + Annotations Code(R"cpp( + template <class T> T *$Make[[make]]() { return nullptr; } + + struct Test { + Test() {} + }; + + int main() { + auto a = ma^ke<Test>(); + } + )cpp"); + TestTU TU = TestTU::withCode(Code.code()); + auto AST = TU.build(); + EXPECT_THAT(locateSymbolAt(AST, Code.point()), + ElementsAre(sym("make", Code.range("Make"), Code.range("Make")))); +} + TEST(GetNonLocalDeclRefs, All) { struct Case { llvm::StringRef AnnotatedCode; `````````` </details> https://github.com/llvm/llvm-project/pull/199480 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
