-
Notifications
You must be signed in to change notification settings - Fork 15.4k
[clang][HeuristicResolver] Default argument heuristic for template parameters #131074
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[clang][HeuristicResolver] Default argument heuristic for template parameters #131074
Conversation
32ca27b to
aeeb8f2
Compare
|
@llvm/pr-subscribers-clang-tools-extra @llvm/pr-subscribers-clang Author: Nathan Ridge (HighCommander4) ChangesFixes clangd/clangd#1056 Full diff: https://github.com/llvm/llvm-project/pull/131074.diff 3 Files Affected:
diff --git a/clang-tools-extra/clangd/unittests/XRefsTests.cpp b/clang-tools-extra/clangd/unittests/XRefsTests.cpp
index e12d7691c58fb..693e965e78a96 100644
--- a/clang-tools-extra/clangd/unittests/XRefsTests.cpp
+++ b/clang-tools-extra/clangd/unittests/XRefsTests.cpp
@@ -1091,7 +1091,7 @@ TEST(LocateSymbol, All) {
)objc",
R"cpp(
struct PointerIntPairInfo {
- static void *getPointer(void *Value);
+ static void *$decl[[getPointer]](void *Value);
};
template <typename Info = PointerIntPairInfo> struct PointerIntPair {
diff --git a/clang/lib/Sema/HeuristicResolver.cpp b/clang/lib/Sema/HeuristicResolver.cpp
index d377379c627db..feda9696b8e05 100644
--- a/clang/lib/Sema/HeuristicResolver.cpp
+++ b/clang/lib/Sema/HeuristicResolver.cpp
@@ -11,6 +11,7 @@
#include "clang/AST/CXXInheritance.h"
#include "clang/AST/DeclTemplate.h"
#include "clang/AST/ExprCXX.h"
+#include "clang/AST/TemplateBase.h"
#include "clang/AST/Type.h"
namespace clang {
@@ -125,6 +126,20 @@ TagDecl *HeuristicResolverImpl::resolveTypeToTagDecl(QualType QT) {
if (!T)
return nullptr;
+ // If T is the type of a template parameter, we can't get a useful TagDecl
+ // out of it. However, if the template parameter has a default argument,
+ // as a heuristic we can replace T with the default argument type.
+ if (const auto *TTPT = dyn_cast<TemplateTypeParmType>(T)) {
+ if (const auto *TTPD = TTPT->getDecl()) {
+ if (TTPD->hasDefaultArgument()) {
+ const auto &DefaultArg = TTPD->getDefaultArgument().getArgument();
+ if (DefaultArg.getKind() == TemplateArgument::Type) {
+ T = DefaultArg.getAsType().getTypePtrOrNull();
+ }
+ }
+ }
+ }
+
// Unwrap type sugar such as type aliases.
T = T->getCanonicalTypeInternal().getTypePtr();
diff --git a/clang/unittests/Sema/HeuristicResolverTest.cpp b/clang/unittests/Sema/HeuristicResolverTest.cpp
index c7cfe7917c532..5e36108172702 100644
--- a/clang/unittests/Sema/HeuristicResolverTest.cpp
+++ b/clang/unittests/Sema/HeuristicResolverTest.cpp
@@ -410,6 +410,23 @@ TEST(HeuristicResolver, MemberExpr_HangIssue126536) {
cxxDependentScopeMemberExpr(hasMemberName("foo")).bind("input"));
}
+TEST(HeuristicResolver, MemberExpr_DefaultTemplateArgument) {
+ std::string Code = R"cpp(
+ struct Default {
+ void foo();
+ };
+ template <typename T = Default>
+ void bar(T t) {
+ t.foo();
+ }
+ )cpp";
+ // Test resolution of "foo" in "t.foo()".
+ expectResolution(
+ Code, &HeuristicResolver::resolveMemberExpr,
+ cxxDependentScopeMemberExpr(hasMemberName("foo")).bind("input"),
+ cxxMethodDecl(hasName("foo")).bind("output"));
+}
+
TEST(HeuristicResolver, DeclRefExpr_StaticMethod) {
std::string Code = R"cpp(
template <typename T>
|
|
@llvm/pr-subscribers-clangd Author: Nathan Ridge (HighCommander4) ChangesFixes clangd/clangd#1056 Full diff: https://github.com/llvm/llvm-project/pull/131074.diff 3 Files Affected:
diff --git a/clang-tools-extra/clangd/unittests/XRefsTests.cpp b/clang-tools-extra/clangd/unittests/XRefsTests.cpp
index e12d7691c58fb..693e965e78a96 100644
--- a/clang-tools-extra/clangd/unittests/XRefsTests.cpp
+++ b/clang-tools-extra/clangd/unittests/XRefsTests.cpp
@@ -1091,7 +1091,7 @@ TEST(LocateSymbol, All) {
)objc",
R"cpp(
struct PointerIntPairInfo {
- static void *getPointer(void *Value);
+ static void *$decl[[getPointer]](void *Value);
};
template <typename Info = PointerIntPairInfo> struct PointerIntPair {
diff --git a/clang/lib/Sema/HeuristicResolver.cpp b/clang/lib/Sema/HeuristicResolver.cpp
index d377379c627db..feda9696b8e05 100644
--- a/clang/lib/Sema/HeuristicResolver.cpp
+++ b/clang/lib/Sema/HeuristicResolver.cpp
@@ -11,6 +11,7 @@
#include "clang/AST/CXXInheritance.h"
#include "clang/AST/DeclTemplate.h"
#include "clang/AST/ExprCXX.h"
+#include "clang/AST/TemplateBase.h"
#include "clang/AST/Type.h"
namespace clang {
@@ -125,6 +126,20 @@ TagDecl *HeuristicResolverImpl::resolveTypeToTagDecl(QualType QT) {
if (!T)
return nullptr;
+ // If T is the type of a template parameter, we can't get a useful TagDecl
+ // out of it. However, if the template parameter has a default argument,
+ // as a heuristic we can replace T with the default argument type.
+ if (const auto *TTPT = dyn_cast<TemplateTypeParmType>(T)) {
+ if (const auto *TTPD = TTPT->getDecl()) {
+ if (TTPD->hasDefaultArgument()) {
+ const auto &DefaultArg = TTPD->getDefaultArgument().getArgument();
+ if (DefaultArg.getKind() == TemplateArgument::Type) {
+ T = DefaultArg.getAsType().getTypePtrOrNull();
+ }
+ }
+ }
+ }
+
// Unwrap type sugar such as type aliases.
T = T->getCanonicalTypeInternal().getTypePtr();
diff --git a/clang/unittests/Sema/HeuristicResolverTest.cpp b/clang/unittests/Sema/HeuristicResolverTest.cpp
index c7cfe7917c532..5e36108172702 100644
--- a/clang/unittests/Sema/HeuristicResolverTest.cpp
+++ b/clang/unittests/Sema/HeuristicResolverTest.cpp
@@ -410,6 +410,23 @@ TEST(HeuristicResolver, MemberExpr_HangIssue126536) {
cxxDependentScopeMemberExpr(hasMemberName("foo")).bind("input"));
}
+TEST(HeuristicResolver, MemberExpr_DefaultTemplateArgument) {
+ std::string Code = R"cpp(
+ struct Default {
+ void foo();
+ };
+ template <typename T = Default>
+ void bar(T t) {
+ t.foo();
+ }
+ )cpp";
+ // Test resolution of "foo" in "t.foo()".
+ expectResolution(
+ Code, &HeuristicResolver::resolveMemberExpr,
+ cxxDependentScopeMemberExpr(hasMemberName("foo")).bind("input"),
+ cxxMethodDecl(hasName("foo")).bind("output"));
+}
+
TEST(HeuristicResolver, DeclRefExpr_StaticMethod) {
std::string Code = R"cpp(
template <typename T>
|
clang/lib/Sema/HeuristicResolver.cpp
Outdated
| if (TTPD->hasDefaultArgument()) { | ||
| const auto &DefaultArg = TTPD->getDefaultArgument().getArgument(); | ||
| if (DefaultArg.getKind() == TemplateArgument::Type) { | ||
| T = DefaultArg.getAsType().getTypePtrOrNull(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the code doesn't look safe -- getTypePtrOrNull could return a nullptr, then we have a null-reference on Line144. Move this new code block before line 126?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch -- fixed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we traverse the default arguments recursively? I'm thinking a case like
template <typename W = Waldo, typename T = W>
void bar(T t) {
t.foo(); // Can we resolve it to Waldo::foo()?
}Also there are some usages for template template parameters, e.g.
template <class E, class A, template <class, class> class V = std::vector>
void foo(V<E, A>) {
V.push_back(42); // Would be great to resolve it to std::vector<>::push_back()
}However it depends on the implementation complexity - there's no necessity to handle these in this patch
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the added testcases!
The recursive case made me realize that application of this heuristic is probably better done in simplifyType. I updated the patch and added the testcase.
For the template template parameter case, I agree that it would be nice to make this work as well, but I'd rather leave that to a follow-up patch.
aeeb8f2 to
6f29edf
Compare
zyn0217
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for missing this. LGTM assuming @hokein is happy too.
clang/lib/Sema/HeuristicResolver.cpp
Outdated
| if (TTPD->hasDefaultArgument()) { | ||
| const auto &DefaultArg = TTPD->getDefaultArgument().getArgument(); | ||
| if (DefaultArg.getKind() == TemplateArgument::Type) { | ||
| T = DefaultArg.getAsType().getTypePtrOrNull(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we traverse the default arguments recursively? I'm thinking a case like
template <typename W = Waldo, typename T = W>
void bar(T t) {
t.foo(); // Can we resolve it to Waldo::foo()?
}Also there are some usages for template template parameters, e.g.
template <class E, class A, template <class, class> class V = std::vector>
void foo(V<E, A>) {
V.push_back(42); // Would be great to resolve it to std::vector<>::push_back()
}However it depends on the implementation complexity - there's no necessity to handle these in this patch
6f29edf to
9530a21
Compare
9530a21 to
556926d
Compare
zyn0217
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks
|
@HighCommander4 did you merge it into a wrong branch? |
Oops. I forgot that the patch was stacked on top of another patch for #130468. I will resubmit and reland. |
|
Resubmitted at #132465 |
…eDeclRefExpr as well (#132576) This is a follow-up to #131074. After moving the default argument heuristic to `simplifyType` in that patch, the heuristic no longer applied to the `DependentScopeDeclRefExpr` case, because that wasn't using `simplifyType`. This patch fixes that, with an added testcase.
…c in resolveDeclRefExpr as well (#132576) This is a follow-up to llvm/llvm-project#131074. After moving the default argument heuristic to `simplifyType` in that patch, the heuristic no longer applied to the `DependentScopeDeclRefExpr` case, because that wasn't using `simplifyType`. This patch fixes that, with an added testcase.
Fixes clangd/clangd#1056