[PATCH] D82739: Improve heuristic resolution of dependent types in TargetFinder

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

In D82739#2138260 , @nridge wrote:

> In D82739#2133155 , @hokein wrote:
>
> > looks like we use this heuristic for go-to-def, I think we might want to 
> > use it in more places, e.g.  libindex (for xrefs), sema code completion (so 
> > that `this->a.^` can suggest something).
>
>
> Yeah, this is a great point. I would definitely like to reuse this heuristic 
> resolution for other features like code completion (including in 
> https://github.com/clangd/clangd/issues/443 where I hope to build on it to 
> improve the original STR which involves completion).
>
> I will explore relocating this code to a place where things like code 
> completion can reuse it, in a follow-up patch.


thanks, this sounds a good plan.




Comment at: clang-tools-extra/clangd/FindTarget.cpp:196
   switch (E->getStmtClass()) {
   case Stmt::CXXDependentScopeMemberExprClass: {
 const auto *ME = llvm::cast(E);

I'm doubting whether we will miss some other exprs, since we are using it to 
find the decls for the base expr of `CXXDependentScopeMemberExpr`. 

could you try the following testcase (also add it to the unittest)?

```
struct A {
  void foo() {}
};
struct B {
  A getA();
};

template  struct C {
  C c;

  void bar() { this->c.getA().foo(); }
};
```



Comment at: clang-tools-extra/clangd/FindTarget.cpp:198
+// analyzing it.
+if (E && BT->getKind() == BuiltinType::Dependent) {
+  T = resolveDependentExprToType(E);

nridge wrote:
> hokein wrote:
> > I think this is the key point of the fix?
> > 
> > It would be nice if you could find a way to split it into two (one for 
> > refactoring, one for the fix). The diff of this patch contains large chunk 
> > of refactoring changes which make the fix less obvious.
> Yeah, sorry about that. I've split the patch and cleaned it up further (to 
> avoid unnecessary reordering of functions) to make the diff neater.
it looks better now, thanks!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D82739/new/

https://reviews.llvm.org/D82739



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D82739: Improve heuristic resolution of dependent types in TargetFinder

[Nathan Ridge via Phabricator via cfe-commits]

nridge updated this revision to Diff 276322.
nridge added a comment.

Improve patch split


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D82739/new/

https://reviews.llvm.org/D82739

Files:
  clang-tools-extra/clangd/FindTarget.cpp
  clang-tools-extra/clangd/unittests/FindTargetTests.cpp

Index: clang-tools-extra/clangd/unittests/FindTargetTests.cpp
===
--- clang-tools-extra/clangd/unittests/FindTargetTests.cpp
+++ clang-tools-extra/clangd/unittests/FindTargetTests.cpp
@@ -548,6 +548,23 @@
   EXPECT_DECLS("UnresolvedMemberExpr", "void func(int *)", "void func(char *)");
 }
 
+TEST_F(TargetDeclTest, DependentExprs) {
+  Flags = {"-fno-delayed-template-parsing"};
+
+  // Heuristic resolution of method of dependent field
+  Code = R"cpp(
+struct A { void foo() {} };
+template 
+struct B {
+  A a;
+  void bar() {
+this->a.[[foo]]();
+  }
+};
+  )cpp";
+  EXPECT_DECLS("CXXDependentScopeMemberExpr", "void foo()");
+}
+
 TEST_F(TargetDeclTest, ObjC) {
   Flags = {"-xobjective-c"};
   Code = R"cpp(
Index: clang-tools-extra/clangd/FindTarget.cpp
===
--- clang-tools-extra/clangd/FindTarget.cpp
+++ clang-tools-extra/clangd/FindTarget.cpp
@@ -58,10 +58,18 @@
   return S;
 }
 
+// Forward declaration, needed as this function is mutually recursive
+// with some of the other helpers below.
+std::vector resolveDependentExprToDecls(const Expr *E);
+
 // Helper function for getMembersReferencedViaDependentName()
-// which takes a dependent type `T` and heuristically
+// which takes a possibly-dependent type `T` and heuristically
 // resolves it to a CXXRecordDecl in which we can try name lookup.
 CXXRecordDecl *resolveTypeToRecordDecl(const Type *T) {
+  if (auto *RT = T->getAs()) {
+return dyn_cast(RT->getDecl());
+  }
+
   if (auto *ICNT = T->getAs()) {
 T = ICNT->getInjectedSpecializationType().getTypePtrOrNull();
   }
@@ -75,6 +83,19 @@
   return TD->getTemplatedDecl();
 }
 
+// Try to heuristically resolve the type of a dependent expression `E`.
+const Type *resolveDependentExprToType(const Expr *E) {
+  std::vector Decls = resolveDependentExprToDecls(E);
+  if (Decls.size() != 1) // Names an overload set -- just bail.
+return nullptr;
+  if (const auto *TD = dyn_cast(Decls[0])) {
+return TD->getTypeForDecl();
+  } else if (const auto *VD = dyn_cast(Decls[0])) {
+return VD->getType().getTypePtrOrNull();
+  }
+  return nullptr;
+}
+
 // Given a dependent type and a member name, heuristically resolve the
 // name to one or more declarations.
 // The current heuristic is simply to look up the name in the primary
@@ -88,12 +109,27 @@
 // an ASTContext, because an ASTContext may be needed to obtain the
 // name (e.g. if it's an operator name), but the caller may not have
 // access to an ASTContext.
+// Optionally, in cases where the type represents the type of a
+// dependent expression, the expression `E` in question can be
+// provided, which allows us to provide richer heuristics by
+// introspecting the expression and trying to reason about its type.
 std::vector getMembersReferencedViaDependentName(
-const Type *T,
+Expr *E, const Type *T,
 llvm::function_ref NameFactory,
 bool IsNonstaticMember) {
   if (!T)
 return {};
+  if (auto *BT = T->getAs()) {
+// If T is the type of a dependent expression, it's just represented
+// as BultinType::Dependent which gives us no information. If the
+// caller provides the expression `E`, we can get further by
+// analyzing it.
+if (E && BT->getKind() == BuiltinType::Dependent) {
+  T = resolveDependentExprToType(E);
+  if (!T)
+return {};
+}
+  }
   if (auto *ET = T->getAs()) {
 auto Result =
 ET->getDecl()->lookup(NameFactory(ET->getDecl()->getASTContext()));
@@ -128,7 +164,7 @@
   // Look up operator-> in the primary template. If we find one, it's probably a
   // smart pointer type.
   auto ArrowOps = getMembersReferencedViaDependentName(
-  T,
+  nullptr, T,
   [](ASTContext ) {
 return Ctx.DeclarationNames.getCXXOperatorName(OO_Arrow);
   },
@@ -163,14 +199,15 @@
 if (ME->isArrow()) {
   BaseType = getPointeeType(BaseType);
 }
+Expr *Base = ME->isImplicitAccess() ? nullptr : ME->getBase();
 return getMembersReferencedViaDependentName(
-BaseType, [ME](ASTContext &) { return ME->getMember(); },
+Base, BaseType, [ME](ASTContext &) { return ME->getMember(); },
 /*IsNonstaticMember=*/true);
   }
   case Stmt::DependentScopeDeclRefExprClass: {
 const auto *RE = llvm::cast(E);
 return getMembersReferencedViaDependentName(
-RE->getQualifier()->getAsType(),
+nullptr, RE->getQualifier()->getAsType(),
 [RE](ASTContext &) { return RE->getDeclName(); },
   

[PATCH] D82739: Improve heuristic resolution of dependent types in TargetFinder

[Nathan Ridge via Phabricator via cfe-commits]

nridge added a comment.

Split out the refactoring into D83371  which 
this depends on now.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D82739/new/

https://reviews.llvm.org/D82739



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D82739: Improve heuristic resolution of dependent types in TargetFinder

[Nathan Ridge via Phabricator via cfe-commits]

nridge updated this revision to Diff 276320.
nridge added a comment.

Split patch


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D82739/new/

https://reviews.llvm.org/D82739

Files:
  clang-tools-extra/clangd/FindTarget.cpp
  clang-tools-extra/clangd/unittests/FindTargetTests.cpp

Index: clang-tools-extra/clangd/unittests/FindTargetTests.cpp
===
--- clang-tools-extra/clangd/unittests/FindTargetTests.cpp
+++ clang-tools-extra/clangd/unittests/FindTargetTests.cpp
@@ -548,6 +548,23 @@
   EXPECT_DECLS("UnresolvedMemberExpr", "void func(int *)", "void func(char *)");
 }
 
+TEST_F(TargetDeclTest, DependentExprs) {
+  Flags = {"-fno-delayed-template-parsing"};
+
+  // Heuristic resolution of method of dependent field
+  Code = R"cpp(
+struct A { void foo() {} };
+template 
+struct B {
+  A a;
+  void bar() {
+this->a.[[foo]]();
+  }
+};
+  )cpp";
+  EXPECT_DECLS("CXXDependentScopeMemberExpr", "void foo()");
+}
+
 TEST_F(TargetDeclTest, ObjC) {
   Flags = {"-xobjective-c"};
   Code = R"cpp(
Index: clang-tools-extra/clangd/FindTarget.cpp
===
--- clang-tools-extra/clangd/FindTarget.cpp
+++ clang-tools-extra/clangd/FindTarget.cpp
@@ -58,6 +58,10 @@
   return S;
 }
 
+// Forward declaration, needed as this function is mutually recursive
+// with some of the other helpers below.
+std::vector resolveDependentExprToDecls(const Expr *E);
+
 // Helper function for getMembersReferencedViaDependentName()
 // which takes a possibly-dependent type `T` and heuristically
 // resolves it to a CXXRecordDecl in which we can try name lookup.
@@ -79,6 +83,19 @@
   return TD->getTemplatedDecl();
 }
 
+// Try to heuristically resolve the type of a dependent expression `E`.
+const Type *resolveDependentExprToType(const Expr *E) {
+  std::vector Decls = resolveDependentExprToDecls(E);
+  if (Decls.size() != 1) // Names an overload set -- just bail.
+return nullptr;
+  if (const auto *TD = dyn_cast(Decls[0])) {
+return TD->getTypeForDecl();
+  } else if (const auto *VD = dyn_cast(Decls[0])) {
+return VD->getType().getTypePtrOrNull();
+  }
+  return nullptr;
+}
+
 // Given a dependent type and a member name, heuristically resolve the
 // name to one or more declarations.
 // The current heuristic is simply to look up the name in the primary
@@ -92,12 +109,27 @@
 // an ASTContext, because an ASTContext may be needed to obtain the
 // name (e.g. if it's an operator name), but the caller may not have
 // access to an ASTContext.
+// Optionally, in cases where the type represents the type of a
+// dependent expression, the expression `E` in question can be
+// provided, which allows us to provide richer heuristics by
+// introspecting the expression and trying to reason about its type.
 std::vector getMembersReferencedViaDependentName(
-const Type *T,
+Expr *E, const Type *T,
 llvm::function_ref NameFactory,
 bool IsNonstaticMember) {
   if (!T)
 return {};
+  if (auto *BT = T->getAs()) {
+// If T is the type of a dependent expression, it's just represented
+// as BultinType::Dependent which gives us no information. If the
+// caller provides the expression `E`, we can get further by
+// analyzing it.
+if (E && BT->getKind() == BuiltinType::Dependent) {
+  T = resolveDependentExprToType(E);
+  if (!T)
+return {};
+}
+  }
   if (auto *ET = T->getAs()) {
 auto Result =
 ET->getDecl()->lookup(NameFactory(ET->getDecl()->getASTContext()));
@@ -132,7 +164,7 @@
   // Look up operator-> in the primary template. If we find one, it's probably a
   // smart pointer type.
   auto ArrowOps = getMembersReferencedViaDependentName(
-  T,
+  nullptr, T,
   [](ASTContext ) {
 return Ctx.DeclarationNames.getCXXOperatorName(OO_Arrow);
   },
@@ -167,14 +199,15 @@
 if (ME->isArrow()) {
   BaseType = getPointeeType(BaseType);
 }
+Expr *Base = ME->isImplicitAccess() ? nullptr : ME->getBase();
 return getMembersReferencedViaDependentName(
-BaseType, [ME](ASTContext &) { return ME->getMember(); },
+Base, BaseType, [ME](ASTContext &) { return ME->getMember(); },
 /*IsNonstaticMember=*/true);
   }
   case Stmt::DependentScopeDeclRefExprClass: {
 const auto *RE = llvm::cast(E);
 return getMembersReferencedViaDependentName(
-RE->getQualifier()->getAsType(),
+nullptr, RE->getQualifier()->getAsType(),
 [RE](ASTContext &) { return RE->getDeclName(); },
 /*IsNonstaticMember=*/false);
   }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D82739: Improve heuristic resolution of dependent types in TargetFinder

[Nathan Ridge via Phabricator via cfe-commits]

nridge marked an inline comment as done.
nridge added a comment.

In D82739#2133155 , @hokein wrote:

> looks like we use this heuristic for go-to-def, I think we might want to use 
> it in more places, e.g.  libindex (for xrefs), sema code completion (so that 
> `this->a.^` can suggest something).


Yeah, this is a great point. I would definitely like to reuse this heuristic 
resolution for other features like code completion (including in 
https://github.com/clangd/clangd/issues/443 where I hope to build on it to 
improve the original STR which involves completion).

I will explore relocating this code to a place where things like code 
completion can reuse it, in a follow-up patch.




Comment at: clang-tools-extra/clangd/FindTarget.cpp:198
+// analyzing it.
+if (E && BT->getKind() == BuiltinType::Dependent) {
+  T = resolveDependentExprToType(E);

hokein wrote:
> I think this is the key point of the fix?
> 
> It would be nice if you could find a way to split it into two (one for 
> refactoring, one for the fix). The diff of this patch contains large chunk of 
> refactoring changes which make the fix less obvious.
Yeah, sorry about that. I've split the patch and cleaned it up further (to 
avoid unnecessary reordering of functions) to make the diff neater.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D82739/new/

https://reviews.llvm.org/D82739



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D82739: Improve heuristic resolution of dependent types in TargetFinder

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

looks like we use this heuristic for go-to-def, I think we might want to use it 
in more places, e.g.  libindex (for xrefs), sema code completion (so that 
`this->a.^` can suggest something).




Comment at: clang-tools-extra/clangd/FindTarget.cpp:198
+// analyzing it.
+if (E && BT->getKind() == BuiltinType::Dependent) {
+  T = resolveDependentExprToType(E);

I think this is the key point of the fix?

It would be nice if you could find a way to split it into two (one for 
refactoring, one for the fix). The diff of this patch contains large chunk of 
refactoring changes which make the fix less obvious.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D82739/new/

https://reviews.llvm.org/D82739



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D82739: Improve heuristic resolution of dependent types in TargetFinder

[Nathan Ridge via Phabricator via cfe-commits]

nridge added reviewers: sammccall, kadircet, hokein.
nridge added a comment.

Adding some reviewers.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D82739/new/

https://reviews.llvm.org/D82739



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D82739: Improve heuristic resolution of dependent types in TargetFinder

[Nathan Ridge via Phabricator via cfe-commits]

nridge created this revision.
Herald added subscribers: cfe-commits, usaxena95, kadircet, arphaman, jkorous, 
ilya-biryukov.
Herald added a project: clang.

Fixes https://github.com/clangd/clangd/issues/441


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D82739

Files:
  clang-tools-extra/clangd/FindTarget.cpp
  clang-tools-extra/clangd/unittests/FindTargetTests.cpp

Index: clang-tools-extra/clangd/unittests/FindTargetTests.cpp
===
--- clang-tools-extra/clangd/unittests/FindTargetTests.cpp
+++ clang-tools-extra/clangd/unittests/FindTargetTests.cpp
@@ -548,6 +548,23 @@
   EXPECT_DECLS("UnresolvedMemberExpr", "void func(int *)", "void func(char *)");
 }
 
+TEST_F(TargetDeclTest, DependentExprs) {
+  Flags = {"-fno-delayed-template-parsing"};
+
+  // Heuristic resolution of method of dependent field
+  Code = R"cpp(
+struct A { void foo() {} };
+template 
+struct B {
+  A a;
+  void bar() {
+this->a.[[foo]]();
+  }
+};
+  )cpp";
+  EXPECT_DECLS("CXXDependentScopeMemberExpr", "void foo()");
+}
+
 TEST_F(TargetDeclTest, ObjC) {
   Flags = {"-xobjective-c"};
   Code = R"cpp(
Index: clang-tools-extra/clangd/FindTarget.cpp
===
--- clang-tools-extra/clangd/FindTarget.cpp
+++ clang-tools-extra/clangd/FindTarget.cpp
@@ -58,50 +58,12 @@
   return S;
 }
 
-// Given a dependent type and a member name, heuristically resolve the
-// name to one or more declarations.
-// The current heuristic is simply to look up the name in the primary
-// template. This is a heuristic because the template could potentially
-// have specializations that declare different members.
-// Multiple declarations could be returned if the name is overloaded
-// (e.g. an overloaded method in the primary template).
-// This heuristic will give the desired answer in many cases, e.g.
-// for a call to vector::size().
-// The name to look up is provided in the form of a factory that takes
-// an ASTContext, because an ASTContext may be needed to obtain the
-// name (e.g. if it's an operator name), but the caller may not have
-// access to an ASTContext.
+// Forward declaration, needed as this function is mutually recursive
+// with getPointeeType() and resolveDependentExprToDecls().
 std::vector getMembersReferencedViaDependentName(
-const Type *T,
+const Expr *E, const Type *T,
 llvm::function_ref NameFactory,
-bool IsNonstaticMember) {
-  if (!T)
-return {};
-  if (auto *ET = T->getAs()) {
-auto Result =
-ET->getDecl()->lookup(NameFactory(ET->getDecl()->getASTContext()));
-return {Result.begin(), Result.end()};
-  }
-  if (auto *ICNT = T->getAs()) {
-T = ICNT->getInjectedSpecializationType().getTypePtrOrNull();
-  }
-  auto *TST = T->getAs();
-  if (!TST)
-return {};
-  const ClassTemplateDecl *TD = dyn_cast_or_null(
-  TST->getTemplateName().getAsTemplateDecl());
-  if (!TD)
-return {};
-  CXXRecordDecl *RD = TD->getTemplatedDecl();
-  if (!RD->hasDefinition())
-return {};
-  RD = RD->getDefinition();
-  DeclarationName Name = NameFactory(RD->getASTContext());
-  return RD->lookupDependentName(Name, [=](const NamedDecl *D) {
-return IsNonstaticMember ? D->isCXXInstanceMember()
- : !D->isCXXInstanceMember();
-  });
-}
+bool IsNonstaticMember);
 
 // Given the type T of a dependent expression that appears of the LHS of a "->",
 // heuristically find a corresponding pointee type in whose scope we could look
@@ -119,7 +81,7 @@
   // Look up operator-> in the primary template. If we find one, it's probably a
   // smart pointer type.
   auto ArrowOps = getMembersReferencedViaDependentName(
-  T,
+  nullptr, T,
   [](ASTContext ) {
 return Ctx.DeclarationNames.getCXXOperatorName(OO_Arrow);
   },
@@ -144,6 +106,119 @@
   return FirstArg.getAsType().getTypePtrOrNull();
 }
 
+// Try to heuristically resolve a dependent expression `E` to one
+// or more declarations that it likely references.
+std::vector resolveDependentExprToDecls(const Expr *E) {
+  switch (E->getStmtClass()) {
+  case Stmt::CXXDependentScopeMemberExprClass: {
+const auto *ME = llvm::cast(E);
+const Type *BaseType = ME->getBaseType().getTypePtrOrNull();
+if (ME->isArrow()) {
+  BaseType = getPointeeType(BaseType);
+}
+Expr *Base = ME->isImplicitAccess() ? nullptr : ME->getBase();
+return getMembersReferencedViaDependentName(
+Base, BaseType, [ME](ASTContext &) { return ME->getMember(); },
+/*IsNonstaticMember=*/true);
+  }
+  case Stmt::DependentScopeDeclRefExprClass: {
+const auto *RE = llvm::cast(E);
+return getMembersReferencedViaDependentName(
+nullptr, RE->getQualifier()->getAsType(),
+[RE](ASTContext &) { return RE->getDeclName(); },
+/*IsNonstaticMember=*/false);
+  }
+  default: