[clangd] Use resolveTypeToRecordDecl() to resolve the type of a base specifier during heuristic resolution

[nridge]

Summary

The code for resolving the type of a base specifier was inside
CXXRecordDecl::lookupDependentName(), so this patch reimplements
lookupDependentName() in HeuristicResolver.

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

[nridge]

As this patch involves copying a bit of code from CXXRecordDecl::lookupDepenentName() to HeuristicResolver, I wanted to mention a couple of alternatives I considered:

1. Copy things in the other direction, i.e. implement some of the necessary parts of resolveTypeToRecordDecl() in the helpers of lookupDependentName() instead. I decided against this because HeuristicResolver is an area of the code that's actively being improved; instead of having to make multiple updates to lookupDependentName() over time, it seems cleaner to just break HeuristicResolver's dependency on that function.
2. Wait to make this change until after HeuristicResolver has been upstreamed and unified with lookupDependentName(). I do plan to work on this upstreaming, but as the process will involve several steps (e.g. writing new tests, getting approval from relevant upstream maintainers), I would prefer not to block this change on the resolution of that process.

[hokein]

In D153248#4431780, @nridge wrote:

As this patch involves copying a bit of code from CXXRecordDecl::lookupDepenentName() to HeuristicResolver, I wanted to mention a couple of alternatives I considered:

1. Copy things in the other direction, i.e. implement some of the necessary parts of resolveTypeToRecordDecl() in the helpers of lookupDependentName() instead. I decided against this because HeuristicResolver is an area of the code that's actively being improved; instead of having to make multiple updates to lookupDependentName() over time, it seems cleaner to just break HeuristicResolver's dependency on that function.
2. Wait to make this change until after HeuristicResolver has been upstreamed and unified with lookupDependentName(). I do plan to work on this upstreaming, but as the process will involve several steps (e.g. writing new tests, getting approval from relevant upstream maintainers), I would prefer not to block this change on the resolution of that process.

Thanks for all thoughtful considerations.
It is a bit sad that we duplicate the implementation in the HeuristicResolver, but given that

• this part of code is not large
• we need it in the Resolver anyway when upstreaming to clang

I think it is probably OK to do that.

The other question might worth thinking, are these cases critical to fix now? I'm not sure this->find() is a common case (find(); already works today).

[nridge]

In D153248#4432108, @hokein wrote:

The other question might worth thinking, are these cases critical to fix now? I'm not sure this->find() is a common case (find(); already works today).

I may have simplified the test case a bit too much; a more representative case is one where the base class is a template, and the derived class passes its template parameter to the base:

template <typename T>
struct Waldo {
  void find();
};

template <typename T>
using Wally = Waldo<T>;

template <typename T>
struct S : Wally<T> {
  void Foo() { this->find(); }
};

In a case like, the base class is a "dependent base", and the this-> is required (without it the code doesn't compile, because the compiler only attempts the first phase of the name lookup on find, and that gives no results).

In any case, the code was reduced from a bug report affecting a real codebase.

[hokein]

OK, fair enough. Thanks.

clang-tools-extra/clangd/HeuristicResolver.h

 
 #include "clang/AST/Decl.h"
 #include <vector>
 
 namespace clang {
 
 class ASTContext;
 class CallExpr;
+class CXXBasePath;
 class CXXDependentScopeMemberExpr;
 class DeclarationName;
 class DependentScopeDeclRefExpr;
 class NamedDecl;
 class Type;
 class UnresolvedUsingValueDecl;
 
 namespace clangd {
private:
   // `E`.
   const Type *resolveExprToType(const Expr *E) const;
   std::vector<const NamedDecl *> resolveExprToDecls(const Expr *E) const;
 
   // Helper function for HeuristicResolver::resolveDependentMember()
   // 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) const;
+
+  // This is a reimplementation of CXXRecordDecl::lookupDependentName()
+  // so that the implementation can call into other HeuristicResolver helpers.
+  // FIXME: Once HeuristicResolver is upstreamed to the clang libraries
+  // (https://github.com/clangd/clangd/discussions/1662),
+  // CXXRecordDecl::lookupDepenedentName() can be removed, and its call sites
+  // can be modified to benefit from the more comprehensive heuristics offered
+  // by HeuristicResolver instead.
+  std::vector<const NamedDecl *> lookupDependentName(
+      CXXRecordDecl *RD, DeclarationName Name,
+      llvm::function_ref<bool(const NamedDecl *ND)> Filter) const;
+  bool findOrdinaryMemberInDependentClasses(const CXXBaseSpecifier *Specifier,
+                                            CXXBasePath &Path,
+                                            DeclarationName Name) const;
 };
 
 } // namespace clangd
 } // namespace clang
 
 #endif

clang-tools-extra/clangd/HeuristicResolver.cpp

 //===--- HeuristicResolver.cpp ---------------------------*- C++-*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "HeuristicResolver.h"
 #include "clang/AST/ASTContext.h"
+#include "clang/AST/CXXInheritance.h"
 #include "clang/AST/DeclTemplate.h"
 #include "clang/AST/ExprCXX.h"
 #include "clang/AST/Type.h"
 
 namespace clang {
 namespace clangd {
 
 // Convenience lambdas for use as the 'Filter' parameter of
return resolveDeclsToType(
         Ctx);
   }
   default:
     break;
   }
   return nullptr;
 }
 
+namespace {
+
+bool isOrdinaryMember(const NamedDecl *ND) {
+  return ND->isInIdentifierNamespace(Decl::IDNS_Ordinary | Decl::IDNS_Tag |
+                                     Decl::IDNS_Member);
+}
+
+bool findOrdinaryMember(const CXXRecordDecl *RD, CXXBasePath &Path,
+                        DeclarationName Name) {
+  Path.Decls = RD->lookup(Name).begin();
+  for (DeclContext::lookup_iterator I = Path.Decls, E = I.end(); I != E; ++I)
+    if (isOrdinaryMember(*I))
+      return true;
+
+  return false;
+}
+
+} // namespace
+
+bool HeuristicResolver::findOrdinaryMemberInDependentClasses(
+    const CXXBaseSpecifier *Specifier, CXXBasePath &Path,
+    DeclarationName Name) const {
+  CXXRecordDecl *RD =
+      resolveTypeToRecordDecl(Specifier->getType().getTypePtr());
+  if (!RD)
+    return false;
+  return findOrdinaryMember(RD, Path, Name);
+}
+
+std::vector<const NamedDecl *> HeuristicResolver::lookupDependentName(
+    CXXRecordDecl *RD, DeclarationName Name,
+    llvm::function_ref<bool(const NamedDecl *ND)> Filter) const {
+  std::vector<const NamedDecl *> Results;
+
+  // Lookup in the class.
+  bool AnyOrdinaryMembers = false;
+  for (const NamedDecl *ND : RD->lookup(Name)) {
+    if (isOrdinaryMember(ND))
+      AnyOrdinaryMembers = true;
+    if (Filter(ND))
+      Results.push_back(ND);
+  }
+  if (AnyOrdinaryMembers)
+    return Results;
+
+  // Perform lookup into our base classes.
+  CXXBasePaths Paths;
+  Paths.setOrigin(RD);
+  if (!RD->lookupInBases(
+          [&](const CXXBaseSpecifier *Specifier, CXXBasePath &Path) {
+            return findOrdinaryMemberInDependentClasses(Specifier, Path, Name);
+          },
+          Paths, /*LookupInDependent=*/true))
+    return Results;
+  for (DeclContext::lookup_iterator I = Paths.front().Decls, E = I.end();
+       I != E; ++I) {
+    if (isOrdinaryMember(*I) && Filter(*I))
+      Results.push_back(*I);
+  }
+  return Results;
+}
+
 std::vector<const NamedDecl *> HeuristicResolver::resolveDependentMember(
     const Type *T, DeclarationName Name,
     llvm::function_ref<bool(const NamedDecl *ND)> Filter) const {
   if (!T)
     return {};
   if (auto *ET = T->getAs<EnumType>()) {
     auto Result = ET->getDecl()->lookup(Name);
     return {Result.begin(), Result.end()};
   }
   if (auto *RD = resolveTypeToRecordDecl(T)) {
     if (!RD->hasDefinition())
       return {};
     RD = RD->getDefinition();
-    return RD->lookupDependentName(Name, Filter);
+    return lookupDependentName(RD, Name, Filter);
   }
   return {};
 }
 
 } // namespace clangd
 } // namespace clang

clang-tools-extra/clangd/unittests/FindTargetTests.cpp

Code = R"cpp(
           using Type = Waldo<T>;
         };
         template <typename T>
         void foo(typename MetaWaldo<T>::Type w) {
           w.[[find]]();
         }
   )cpp";
   EXPECT_DECLS("CXXDependentScopeMemberExpr", "void find()");
+
+  Code = R"cpp(
+        struct Waldo {
+          void find();
+        };
+        template <typename T>
+        using Wally = Waldo;
+        template <typename>
+        struct S : Wally<int> {
+          void Foo() { this->[[find]](); }
+        };
+  )cpp";
+  EXPECT_DECLS("CXXDependentScopeMemberExpr", "void find()");
 }
 
 TEST_F(TargetDeclTest, DependentTypes) {
   // Heuristic resolution of dependent type name
   Code = R"cpp(
         template <typename>
         struct A { struct B {}; };