[AST] Traverse the class type loc inside the member pointer type loc.

[hokein]

Summary

We are missing this currently.

This would fix https://github.com/clangd/clangd/issues/216.

[ilya-biryukov]

clang/include/clang/AST/RecursiveASTVisitor.h

1164–1165

Remove this FIXME

1168–1170

Can this actually happen in practice?
Did we try doing only TRY_TO(TraverseTypeLoc(TSI->getTypeLoc())) and seeing whether this fails?

The code in MemberPointerTypeLoc suggests this can happen, but would be interesting to find examples when it happens.

[hokein]

address review comments.

[ilya-biryukov]

clang/include/clang/AST/RecursiveASTVisitor.h

1168–1170

Our tests probably don't catch the case where it's null, right?
Could we add one? (Temporarily removing the check for null should be enough to trigger a crash here)

[hokein]

clang/include/clang/AST/RecursiveASTVisitor.h

1168–1170

I tried to add one for this, but failed to figure out a case that would trigger this code path (the existing tests are all passed if we remove this if guard check.)

any thoughts?

[hokein]

use the assertion.

[hokein]

clang/include/clang/AST/RecursiveASTVisitor.h

1168–1170

per offline discussion, we don't find out such an example (all tests are passed), it seems that we may never encounter this cases. Added an assertion that would allow us to catch it.

[merge_guards_bot]

Build result: pass - 60507 tests passed, 0 failed and 726 were skipped.

Log files: console-log.txt, CMakeCache.txt

[ilya-biryukov]

LGTM

[saugustine]

clang/include/clang/AST/RecursiveASTVisitor.h

1168–1170

This can happen when traversing a TypeNodeLoc, and the assertion will fail. I'm reverting this change for now.

[hokein]

clang/include/clang/AST/RecursiveASTVisitor.h

1168–1170

This can happen when traversing a TypeNodeLoc, and the assertion will fail. I'm reverting this change for now.

Thanks for reverting it, could you share more details? would be nice if you could point us an example that triggers the assertion, I believe it was found when you did the llvm buildcop.

[rnk]

We also hit this, and I have a repro here, but it depends on the Chromium Blink GC plugin:
https://bugs.chromium.org/p/chromium/issues/detail?id=1031274#c4
I think you just need an RAV client that walks over some particular AST node to trigger the assert. I assume there is nothing special about the Blink GC plugin here.

Thanks again Sterling for the revert, more bots are green this morning as a result.

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

)cpp",
       $Class[[G]] $Variable[[L]];
       void $Function[[f]]() {
         $Class[[Foo]]<123> $LocalVariable[[F]];
         $Class[[GP]]<&$Variable[[L]]> $LocalVariable[[LL]];
         $Class[[GR]]<$Variable[[L]]> $LocalVariable[[LLL]];
       }
     )cpp",
       R"cpp(
-      template<typename $TemplateParameter[[T]], 
-        void (T::*$TemplateParameter[[method]])(int)>
+      template<typename $TemplateParameter[[T]],
+        void ($TemplateParameter[[T]]::*$TemplateParameter[[method]])(int)>
       struct $Class[[G]] {
         void $Method[[foo]](
             $TemplateParameter[[T]] *$Parameter[[O]]) {
           ($Parameter[[O]]->*$TemplateParameter[[method]])(10);
         }
       };
       struct $Class[[F]] {
         void $Method[[f]](int);

clang/include/clang/AST/RecursiveASTVisitor.h

 DEF_TRAVERSE_TYPELOC(BlockPointerType,
                      { TRY_TO(TraverseTypeLoc(TL.getPointeeLoc())); })
 
 DEF_TRAVERSE_TYPELOC(LValueReferenceType,
                      { TRY_TO(TraverseTypeLoc(TL.getPointeeLoc())); })
 
 DEF_TRAVERSE_TYPELOC(RValueReferenceType,
                      { TRY_TO(TraverseTypeLoc(TL.getPointeeLoc())); })
 
-// FIXME: location of base class?
 // We traverse this in the type case as well, but how is it not reached through
 // the pointee type?
 DEF_TRAVERSE_TYPELOC(MemberPointerType, {
-  TRY_TO(TraverseType(QualType(TL.getTypePtr()->getClass(), 0)));
+  auto *TSI = TL.getClassTInfo();
+  assert(TSI);
+  TRY_TO(TraverseTypeLoc(TSI->getTypeLoc()));
   TRY_TO(TraverseTypeLoc(TL.getPointeeLoc()));
 })
 
 DEF_TRAVERSE_TYPELOC(AdjustedType,
                      { TRY_TO(TraverseTypeLoc(TL.getOriginalLoc())); })
 
 DEF_TRAVERSE_TYPELOC(DecayedType,
                      { TRY_TO(TraverseTypeLoc(TL.getOriginalLoc())); })

clang/unittests/Tooling/CMakeLists.txt

add_clang_unittest(ToolingTests
   RecursiveASTVisitorTests/InitListExprPostOrder.cpp
   RecursiveASTVisitorTests/InitListExprPostOrderNoQueue.cpp
   RecursiveASTVisitorTests/InitListExprPreOrder.cpp
   RecursiveASTVisitorTests/InitListExprPreOrderNoQueue.cpp
   RecursiveASTVisitorTests/IntegerLiteral.cpp
   RecursiveASTVisitorTests/LambdaDefaultCapture.cpp
   RecursiveASTVisitorTests/LambdaExpr.cpp
   RecursiveASTVisitorTests/LambdaTemplateParams.cpp
+  RecursiveASTVisitorTests/MemberPointerTypeLoc.cpp
   RecursiveASTVisitorTests/NestedNameSpecifiers.cpp
   RecursiveASTVisitorTests/ParenExpr.cpp
   RecursiveASTVisitorTests/TemplateArgumentLocTraverser.cpp
   RecursiveASTVisitorTests/TraversalScope.cpp
   RecursiveASTVisitorTestDeclVisitor.cpp
   RecursiveASTVisitorTestPostOrderVisitor.cpp
   RecursiveASTVisitorTestTypeLocVisitor.cpp
   RefactoringActionRulesTest.cpp

clang/unittests/Tooling/RecursiveASTVisitorTests/MemberPointerTypeLoc.cpp

+//===- unittest/Tooling/RecursiveASTVisitorTests/MemberPointerTypeLoc.cpp -===//
+//
+// 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 "TestVisitor.h"
+
+using namespace clang;
+
+namespace {
+
+class MemberPointerTypeLocVisitor
+    : public ExpectedLocationVisitor<MemberPointerTypeLocVisitor> {
+public:
+  bool VisitTemplateTypeParmTypeLoc(TemplateTypeParmTypeLoc TL) {
+    if (!TL)
+      return true;
+    Match(TL.getDecl()->getName(), TL.getNameLoc());
+    return true;
+  }
+  bool VisitRecordTypeLoc(RecordTypeLoc RTL) {
+    if (!RTL)
+      return true;
+    Match(RTL.getDecl()->getName(), RTL.getNameLoc());
+    return true;
+  }
+};
+
+TEST(RecursiveASTVisitor, VisitTypeLocInMemberPointerTypeLoc) {
+  MemberPointerTypeLocVisitor Visitor;
+  Visitor.ExpectMatch("Bar", 4, 36);
+  Visitor.ExpectMatch("T", 7, 23);
+  EXPECT_TRUE(Visitor.runOver(R"cpp(
+     class Bar { void func(int); };
+     class Foo {
+       void bind(const char*, void(Bar::*Foo)(int)) {}
+
+       template<typename T>
+       void test(void(T::*Foo)());
+     };
+  )cpp"));
+}
+
+} // end anonymous namespace