[WIP][clangd] Resolve the dependent type from its single instantiation. Take 1 #71279
Conversation
|
@llvm/pr-subscribers-clangd Author: Younan Zhang (zyn0217) ChangesThis is an enhancement to the HeuristicResolver, trying to extract the deduced type from the single instantiation for a template. This partially addresses the point #1 from This patch doesn't tackle CXXUnresolvedConstructExpr or similarities since I feel that is more arduous and would prefer to leave it for my future work. Full diff: https://github.com/llvm/llvm-project/pull/71279.diff 2 Files Affected:
diff --git a/clang-tools-extra/clangd/HeuristicResolver.cpp b/clang-tools-extra/clangd/HeuristicResolver.cpp
index 3c147b6b582bf0b..d3dced9b325367a 100644
--- a/clang-tools-extra/clangd/HeuristicResolver.cpp
+++ b/clang-tools-extra/clangd/HeuristicResolver.cpp
@@ -7,10 +7,14 @@
//===----------------------------------------------------------------------===//
#include "HeuristicResolver.h"
+#include "AST.h"
#include "clang/AST/ASTContext.h"
#include "clang/AST/CXXInheritance.h"
+#include "clang/AST/Decl.h"
+#include "clang/AST/DeclCXX.h"
#include "clang/AST/DeclTemplate.h"
#include "clang/AST/ExprCXX.h"
+#include "clang/AST/RecursiveASTVisitor.h"
#include "clang/AST/Type.h"
namespace clang {
@@ -46,6 +50,98 @@ const Type *resolveDeclsToType(const std::vector<const NamedDecl *> &Decls,
return nullptr;
}
+// Visitor that helps to extract deduced type from instantiated entities.
+// This merely performs the source location comparison against each Decl
+// until it finds a Decl with the same location as the
+// dependent one. Its associated type will then be extracted.
+struct InstantiatedDeclVisitor : RecursiveASTVisitor<InstantiatedDeclVisitor> {
+
+ InstantiatedDeclVisitor(NamedDecl *DependentDecl) : DependentDecl(DependentDecl) {}
+
+ bool shouldVisitTemplateInstantiations() const { return true; }
+
+ bool shouldVisitLambdaBody() const { return true; }
+
+ bool shouldVisitImplicitCode() const { return true; }
+
+ template <typename D>
+ bool onDeclVisited(D *MaybeInstantiated) {
+ if (MaybeInstantiated->getDeclContext()->isDependentContext())
+ return true;
+ auto *Dependent = dyn_cast<D>(DependentDecl);
+ if (!Dependent)
+ return true;
+ auto LHS = MaybeInstantiated->getTypeSourceInfo(),
+ RHS = Dependent->getTypeSourceInfo();
+ if (!LHS || !RHS)
+ return true;
+ if (LHS->getTypeLoc().getSourceRange() !=
+ RHS->getTypeLoc().getSourceRange())
+ return true;
+ DeducedType = MaybeInstantiated->getType();
+ return false;
+ }
+
+ bool VisitFieldDecl(FieldDecl *FD) {
+ return onDeclVisited(FD);
+ }
+
+ bool VisitVarDecl(VarDecl *VD) {
+ return onDeclVisited(VD);
+ }
+
+ NamedDecl *DependentDecl;
+ QualType DeducedType;
+};
+
+/// Attempt to resolve the dependent type from the surrounding context for which
+/// a single instantiation is available.
+const Type *
+resolveTypeFromInstantiatedTemplate(const CXXDependentScopeMemberExpr *Expr) {
+ if (Expr->isImplicitAccess())
+ return nullptr;
+
+ auto *Base = Expr->getBase();
+ NamedDecl *ND = nullptr;
+ if (auto *CXXMember = dyn_cast<MemberExpr>(Base))
+ ND = CXXMember->getMemberDecl();
+
+ if (auto *DRExpr = dyn_cast<DeclRefExpr>(Base))
+ ND = DRExpr->getFoundDecl();
+
+ // FIXME: Handle CXXUnresolvedConstructExpr. This kind of type doesn't have
+ // available Decls to be matched against. Which inhibits the current heuristic
+ // from resolving expressions such as `T().fo^o()`, where T is a
+ // single-instantiated template parameter.
+ if (!ND)
+ return nullptr;
+
+ NamedDecl *Instantiation = nullptr;
+
+ // Find out a single instantiation that we can start with. The enclosing
+ // context for the current Decl might not be a templated entity (e.g. a member
+ // function inside a class template), hence we shall walk up the decl
+ // contexts first.
+ for (auto *EnclosingContext = ND->getDeclContext(); EnclosingContext;
+ EnclosingContext = EnclosingContext->getParent()) {
+ if (auto *ND = dyn_cast<NamedDecl>(EnclosingContext)) {
+ Instantiation = getOnlyInstantiation(ND);
+ if (Instantiation)
+ break;
+ }
+ }
+
+ if (!Instantiation)
+ return nullptr;
+
+ // This will traverse down the instantiation entity, visit each Decl, and
+ // extract the deduced type for the undetermined Decl `ND`.
+ InstantiatedDeclVisitor Visitor(ND);
+ Visitor.TraverseDecl(Instantiation);
+
+ return Visitor.DeducedType.getTypePtrOrNull();
+}
+
} // namespace
// Helper function for HeuristicResolver::resolveDependentMember()
@@ -150,6 +246,11 @@ std::vector<const NamedDecl *> HeuristicResolver::resolveMemberExpr(
if (ME->isArrow()) {
BaseType = getPointeeType(BaseType);
}
+
+ if (BaseType->isDependentType())
+ if (auto *MaybeResolved = resolveTypeFromInstantiatedTemplate(ME))
+ BaseType = MaybeResolved;
+
if (!BaseType)
return {};
if (const auto *BT = BaseType->getAs<BuiltinType>()) {
diff --git a/clang-tools-extra/clangd/unittests/XRefsTests.cpp b/clang-tools-extra/clangd/unittests/XRefsTests.cpp
index f53cbf01b7992c8..ead24dec575de0d 100644
--- a/clang-tools-extra/clangd/unittests/XRefsTests.cpp
+++ b/clang-tools-extra/clangd/unittests/XRefsTests.cpp
@@ -1222,6 +1222,54 @@ TEST(LocateSymbol, TextualSmoke) {
hasID(getSymbolID(&findDecl(AST, "MyClass"))))));
}
+TEST(LocateSymbol, DeduceDependentTypeFromSingleInstantiation) {
+ Annotations T(R"cpp(
+ struct WildCat {
+ void $wild_meow[[meow]]();
+ };
+
+ struct DomesticCat {
+ void $domestic_meow[[meow]]();
+ };
+
+ template <typename Ours>
+ struct Human {
+ template <typename Others>
+ void feed(Others O) {
+ O.me$1^ow();
+ Others Child;
+ Child.me$2^ow();
+ // FIXME: Others().me^ow();
+ Ours Baby;
+ Baby.me$3^ow();
+ // struct Inner {
+ // Ours Pet;
+ // };
+ // Inner().Pet.me^ow();
+ auto Lambda = [](auto C) {
+ C.me$4^ow();
+ };
+ Lambda(Others());
+ }
+ };
+
+ void foo() {
+ Human<DomesticCat>().feed(WildCat());
+ }
+ )cpp");
+
+ auto TU = TestTU::withCode(T.code());
+ auto AST = TU.build();
+ EXPECT_THAT(locateSymbolAt(AST, T.point("1")),
+ ElementsAre(sym("meow", T.range("wild_meow"), std::nullopt)));
+ EXPECT_THAT(locateSymbolAt(AST, T.point("2")),
+ ElementsAre(sym("meow", T.range("wild_meow"), std::nullopt)));
+ EXPECT_THAT(locateSymbolAt(AST, T.point("3")),
+ ElementsAre(sym("meow", T.range("domestic_meow"), std::nullopt)));
+ EXPECT_THAT(locateSymbolAt(AST, T.point("4")),
+ ElementsAre(sym("meow", T.range("wild_meow"), std::nullopt)));
+}
+
TEST(LocateSymbol, Textual) {
const char *Tests[] = {
R"cpp(// Comment
|
| // FIXME: Handle CXXUnresolvedConstructExpr. This kind of type doesn't have | ||
| // available Decls to be matched against. Which inhibits the current heuristic | ||
| // from resolving expressions such as `T().fo^o()`, where T is a | ||
| // single-instantiated template parameter. | ||
| if (!ND) | ||
| return nullptr; |
There was a problem hiding this comment.
RFC: Do we have a neat approach to deal with such an expression? One approach that comes to mind is:
-
Extract the TemplateTypeParmDecl (TTPD) from the expression.
-
Traverse every Decls inside the enclosing DeclContext for TTPD until we find a template entity whose template parameters contain TTPD.
-
Find the instantiation for that entity. If succeeds, perform the similar steps below to extract the Stmt corresponding to the dependent expression (i.e. our interest).
-
Figure out a way to extract the type from that expression, which can be intricate, as we don't know to which Decl the dependent expression is tied.
You can test this locally with the following command:git-clang-format --diff 99eb843c8bda5d636338edfd4f67728b6573a2a9 09f0acb3bdfe64cbc09116cd318f56cef242cc7c -- clang-tools-extra/clangd/AST.cpp clang-tools-extra/clangd/AST.h clang-tools-extra/clangd/FindTarget.cpp clang-tools-extra/clangd/FindTarget.h clang-tools-extra/clangd/HeuristicResolver.cpp clang-tools-extra/clangd/HeuristicResolver.h clang-tools-extra/clangd/Hover.cpp clang-tools-extra/clangd/InlayHints.cpp clang-tools-extra/clangd/ParsedAST.cpp clang-tools-extra/clangd/ParsedAST.h clang-tools-extra/clangd/SemanticHighlighting.cpp clang-tools-extra/clangd/XRefs.cpp clang-tools-extra/clangd/refactor/Rename.cpp clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp clang-tools-extra/clangd/unittests/ASTTests.cpp clang-tools-extra/clangd/unittests/FindTargetTests.cpp clang-tools-extra/clangd/unittests/XRefsTests.cppView the diff from clang-format here.diff --git a/clang-tools-extra/clangd/AST.cpp b/clang-tools-extra/clangd/AST.cpp
index d42dbf3163..120ba7a2b9 100644
--- a/clang-tools-extra/clangd/AST.cpp
+++ b/clang-tools-extra/clangd/AST.cpp
@@ -744,8 +744,7 @@ getOnlyInstantiatedNode(const DeclContext *StartingPoint,
}
NamedDecl *InstantiatedEnclosingDecl = nullptr;
- for (auto *DC = StartingPoint; DC;
- DC = DC->getParent()) {
+ for (auto *DC = StartingPoint; DC; DC = DC->getParent()) {
auto *ND = llvm::dyn_cast<NamedDecl>(DC);
if (!ND)
continue;
@@ -836,7 +835,6 @@ getOnlyInstantiationForMemberFunction(const CXXMethodDecl *TemplatedDecl) {
return MD;
return nullptr;
}
-
};
return Visitor(TemplatedDecl).Visit(OuterTemplate);
}
diff --git a/clang-tools-extra/clangd/HeuristicResolver.cpp b/clang-tools-extra/clangd/HeuristicResolver.cpp
index 072ff4cf13..98fc83fa73 100644
--- a/clang-tools-extra/clangd/HeuristicResolver.cpp
+++ b/clang-tools-extra/clangd/HeuristicResolver.cpp
@@ -260,7 +260,8 @@ std::vector<const NamedDecl *> HeuristicResolver::resolveMemberExpr(
return {};
if (BaseType->isDependentType())
- if (auto *MaybeResolved = resolveTypeFromInstantiatedTemplate(EnclosingDecl, ME))
+ if (auto *MaybeResolved =
+ resolveTypeFromInstantiatedTemplate(EnclosingDecl, ME))
BaseType = MaybeResolved;
if (const auto *BT = BaseType->getAs<BuiltinType>()) {
diff --git a/clang-tools-extra/clangd/HeuristicResolver.h b/clang-tools-extra/clangd/HeuristicResolver.h
index 16650e503a..261dcff138 100644
--- a/clang-tools-extra/clangd/HeuristicResolver.h
+++ b/clang-tools-extra/clangd/HeuristicResolver.h
@@ -45,7 +45,8 @@ namespace clangd {
// not a specialization. More advanced heuristics may be added in the future.
class HeuristicResolver {
public:
- HeuristicResolver(const ASTContext &Ctx, const DeclContext *EnclosingDecl = nullptr)
+ HeuristicResolver(const ASTContext &Ctx,
+ const DeclContext *EnclosingDecl = nullptr)
: Ctx(Ctx), EnclosingDecl(EnclosingDecl) {}
// Try to heuristically resolve certain types of expressions, declarations, or
diff --git a/clang-tools-extra/clangd/ParsedAST.h b/clang-tools-extra/clangd/ParsedAST.h
index 1f1425a995..b2ae420e7d 100644
--- a/clang-tools-extra/clangd/ParsedAST.h
+++ b/clang-tools-extra/clangd/ParsedAST.h
@@ -24,13 +24,13 @@
#include "Compiler.h"
#include "Diagnostics.h"
#include "Headers.h"
+#include "HeuristicResolver.h"
#include "Preamble.h"
#include "clang-include-cleaner/Record.h"
#include "support/Path.h"
#include "clang/Frontend/FrontendAction.h"
#include "clang/Lex/Preprocessor.h"
#include "clang/Tooling/Syntax/Tokens.h"
-#include "HeuristicResolver.h"
#include "llvm/ADT/ArrayRef.h"
#include "llvm/ADT/StringRef.h"
#include <memory>
diff --git a/clang-tools-extra/clangd/SemanticHighlighting.cpp b/clang-tools-extra/clangd/SemanticHighlighting.cpp
index d5cd8a3c08..21b347be5f 100644
--- a/clang-tools-extra/clangd/SemanticHighlighting.cpp
+++ b/clang-tools-extra/clangd/SemanticHighlighting.cpp
@@ -419,8 +419,7 @@ public:
HighlightingsBuilder(const ParsedAST &AST, const HighlightingFilter &Filter,
const HeuristicResolver &Resolver)
: TB(AST.getTokens()), SourceMgr(AST.getSourceManager()),
- LangOpts(AST.getLangOpts()), Filter(Filter),
- Resolver(Resolver) {}
+ LangOpts(AST.getLangOpts()), Filter(Filter), Resolver(Resolver) {}
HighlightingToken &addToken(SourceLocation Loc, HighlightingKind Kind) {
auto Range = getRangeForSourceLocation(Loc);
|
| @@ -150,6 +246,11 @@ std::vector<const NamedDecl *> HeuristicResolver::resolveMemberExpr( | |||
| if (ME->isArrow()) { | |||
| BaseType = getPointeeType(BaseType); | |||
| } | |||
|
|
|||
| if (BaseType->isDependentType()) | |||
There was a problem hiding this comment.
Check BaseType before access (line 254)
There was a problem hiding this comment.
I don't know if I should assume the non-nullity for BaseType: All of the construction points appear to create expressions with valid base types. Except for ASTImporter and ASTReader, where an empty expression may be created. But would we somehow end up resolving expressions created from these two places? I suppose we might encounter these inside a preamble, but I don't think this is where the heuristic resolver would work.
Anyway, I moved this statement after the non-null check. Thanks for spotting it!
There was a problem hiding this comment.
Thanks very much for looking at this!
I'm wondering if it's possible to achieve the same results (and also solve your CXXUnresolvedConstructExpr issue) with a more targeted change that may also be simpler.
While I haven't experimented with this approach to verify that it's feasible, here's what I had in mind:
- In a suitable place where we simplify dependent types (I'm thinking around here), check if the type is a
TemplateTypeParmType. - If so, get its
TemplateTypeParmDecl, find the template whose parameter it is, and see if the template has agetOnlyInstantiation. If so, find the argument type substituted for this template parameter, and replace the template parameter with this type.
As a bonus, the "default argument heuristic" discussed here could be implemented in the same place. (Even if the template doesn't have exactly one instantiation, if the template parameter has a default argument, replace the template parameter with the default argument.)
I think this should handle the cases in this testcase without the need for any Visitor, though it's definitely possible I'm overlooking something.
Thanks for the suggestion! Admittedly, it looks neat and terse. However, I suspect it won't work with the present
I was thinking this way before getting into the details :-). Soon I realized that we had to tackle synthesized types e.g. template <template <class, int> typename C, typename D, int E>
void work() {
C<D, E> complicated_type;
// ^ Shall we perform the type substitution once again? ;)
} |
I've done some local experimentation, and what I'm seeing is that One tricky thing I found is that when starting from a template parameter type, it's important to use I also discovered some complications related to nested templates, and I have some thoughts on how to resolve them, but I'm going to suggest that we start with getting a simple case (e.g. just one level of templates, no nested templates) to work, and then tackle nested templates as a next step.
I'm not too concerned about this. The idea behind So, to give an example: template <typename T>
void work() {
std::vector<T> a;
a.pop_back();
}here, Now, applying this to your example: template <template <typename> class Container, typename T>
void work() {
Container<T> a;
a.pop_back();
}
// work() has one instantiation, with [Container = std::vector, T = bar] applying the "only instantiation" heuristic will tell us that We could potentially envision future enhancements to make use of information from the "only instantiation" to make lookup inside template specialization types more accurate than just "use the primary template" (which could then apply to the |
|
I thought some more about this. I appreciate better now the question you asked here:
My previous suggestion side-steps this by saying that we're only going to do lookups in the primary template scope of the argument for However, if in the future we want to be more accurate and e.g. handle cases where a template specialization resolves to something different than the primary template (e.g. a partial specialization), then your question becomes relevant: do we perform type substitution again with the arguments for I realized that this consideration actually applies to any other "resolution" step performed in To illustrate, let's go back to the template <typename T>
void work() {
std::vector<T> a;
a.pop_back();
}To resolve the dependent member expression The name lookup step could also be avoided in principle if we had an "only instantiation": if we find the non-dependent And this method can lead to more accurate results, e.g. if instead of So, I think we could consider two alternative approaches to this: Approach 1: What I suggested in the previous review: a limited use of the "only instantiation" in Approach 2: If there is an "only instantiation", then resolve any expression, type, etc. in the template body to the corresponding expression, type, etc. in the "only instantiation". The implementation of "Approach 2" could look similar to the current "Approach 2" can be largely independent of |
|
Thank you again for reconsidering about this, and sorry for not responding for over a week. Just now, I replicated the experimentation
and had to admit that I was wrong previously. Something must be wrong with my recollection, but I could clearly remember that I'd run into the same pitfall at the first time. (I was about to write the comment to highlight this, but soon, I gave up taking this approach since it looks more constricted than I thought. And here I'm explaining it.)
The contrived example involving template template parameter is one obstruction; Another more common example could be such. (Which is simplified from the test case): template <typename T>
struct Outer {
template <typename U>
void foo(U arg) {
arg.fo^o();
}
};
struct Widget {
void foo();
};
Outer<int>().foo(Widget());Here, we want to find the only instantiation before locating This is because when clang performs the instantiation for member templates, it creates a new llvm-project/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp Lines 2184 to 2203 in 1449b34 And what makes it worse is, as of now, we don't have any approach to retrieve the new templated llvm-project/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp Lines 4789 to 4803 in 1449b34 ("// FIXME: New needs a pointer to Tmpl", I was quite frustrated when I read it!) (This patch first walks up the
Exactly. And this is why the patch is marked as "Take 1" :)
Yeah, I'd love to extend the HeuristicResolver to make it more accurate and not just dependent on the mere name lookup. |
Oops, I replied too quickly and I should have noticed this paragraph. Does my previous comment elaborate on the same issue you've encountered? Could you kindly share your thoughts more? |
|
Thinking some more about "Approach 2", I realized there is a challenge with it: given the node we are trying to resolve (for example, a dependent member expr) as a starting point, we need to find the enclosing template decl, so that we can query it for its "only instantiation". The current patch does this by introspecting the starting node, and in a subset of cases (base expression is However, we would ideally like our "only instantiation" heuristic to work for any AST node inside a template (and so, for dependent member exprs, we would like it to work for any base expression type, not just specific ones). In principle, it could be possible to extend the introspection approach to handle all expression types, since if an expression is dependent, it will ultimately reference a template parameter (or a Decl whose type references a template parameter, etc.) However, I feel like this may be a lot of work to implement and could be error-prone. I wonder if we could do something easier: require that the caller pass in a Editor operations like go-to-def which involve taking an action at a cursor position use In the future when we want to use |
I believe so, yes.
Let's consider a nested template like this: template <typename T>
struct Outer {
template <typename U>
void inner(U u) {
u.foo();
}
}I believe our heuristic needs to require two things:
Now, suppose we have written an algorithm for "given a template Then, I think we can proceed as follows:
|
|
@HighCommander4 While at the I didn't see any other lines initializing this member. so we're always yielding |
Thanks for spotting that. Posted #74971 to fix. |
…ke 1 This is an enhancement to the HeuristicResolver, trying to extract the deduced type from the single instantiation for a template. This partially addresses the point llvm#1 from clangd/clangd#1768. This patch doesn't tackle CXXUnresolvedConstructExpr or similarities since I feel that is more arduous and would prefer to leave it for my future work.
|
Another issue I've encountered while at it is that, given the following code, void foo();
auto lambda = [] {
return ^foo();
};let I presume there's something wrong at `-LambdaExpr <col:15, line:4:1> '(lambda at line:2:15)' <-- parent 3; not a Decl thus ignored!
|-CXXRecordDecl <line:2:15> col:15 implicit class definition
| |-DefinitionData
| | |-DefaultConstructor
| | |-CopyConstructor
| | |-MoveConstructor
| | |-CopyAssignment
| | |-MoveAssignment
| | `-Destructor
| |-CXXMethodDecl <col:16, line:4:1> line:2:15 constexpr operator() 'auto () const -> void' inline
| | `-CompoundStmt <col:18, line:4:1>
| | `-ReturnStmt <line:3:3, col:14>
| | `-CallExpr <col:10, col:14> 'void'
| | `-ImplicitCastExpr <col:10> 'void (*)()' <FunctionToPointerDecay>
| | `-DeclRefExpr <col:10> 'void ()' lvalue Function 0xb76c1c8 'foo' 'void ()'
| |-CXXConversionDecl <line:2:15, line:4:1> line:2:15 implicit constexpr operator void (*)() 'auto (*() const noexcept)() -> void' inline
| |-CXXMethodDecl <col:15, line:4:1> line:2:15 implicit __invoke 'auto () -> void' static inline
| `-CXXDestructorDecl <col:15> col:15 implicit referenced constexpr ~(lambda at line:2:15) 'void () noexcept' inline default trivial
`-CompoundStmt <col:18, line:4:1> <--parent 2
`-ReturnStmt <line:3:3, col:14> <-- parent 1
`-CallExpr <col:10, col:14> 'void' <-- starting point
`-ImplicitCastExpr <col:10> 'void (*)()' <FunctionToPointerDecay>
`-DeclRefExpr <col:10> 'void ()' lvalue Function 0xb76c1c8 'foo' 'void ()'llvm-project/clang-tools-extra/clangd/Selection.cpp Lines 1107 to 1118 in 51b988e Proposed #76329 for it. |
zyn0217
changed the title
zyn0217
force-pushed
the
lambda-with-autos
branch
from
bd05ba6 to
09f0acb
Compare
This is an enhancement to the HeuristicResolver, trying to extract the deduced type from the single instantiation for a template. This partially addresses the point 1 from
clangd/clangd#1768.
This patch doesn't tackle CXXUnresolvedConstructExpr or similarities since I feel that is more arduous and would prefer to leave it for my future work.