[clang][AST] RecursiveASTVisitor should visit owned TagDecl of friend type.

[balazske]

Summary

A FriendDecl node can have a friend record type that owns a RecordDecl
object. This object is different than the one got from TypeSourceInfo
object of the FriendDecl. When building a ParentMapContext this owned
tag decaration has to be encountered to have the parent set for it.

[balazske]

I really do not know why parent of the node for the owned TagDecl node is the FriendDecl and not a TypeLoc node, but it is working.
The code struct A { friend struct Fr; }; caused no problems either (no duplicate visit of struct Fr).

[davrec]

Once the FIXME is removed this looks good, but I was involved in this so better if @sammccall can give the thumbs up at least to the RecursiveASTVisitor code.

clang/include/clang/AST/RecursiveASTVisitor.h

1564–1565

I don't think anything is necessary here, because we should never see an OwnedTagDecl here.
In the FriendDecl case the ElaboratedType only has an OwnedTagDecl when struct Fr has not yet been declared before the friend decl.
In the documentation of FriendTemplateDecl on the other hand these examples are given:

/// template \<typename T> class A {
///   friend class MyVector<T>; // not a friend template
///   template \<typename U> friend class B; // not a friend template
///   template \<typename U> friend class Foo<T>::Nested; // friend template
/// };

So, a FriendTemplateDecl is only created when referencing a nested class template member. But that *must* have been declared before the friend decl, or we will get an error:

template<typename T> struct B;
template<typename T> struct A { template<typename U> friend struct B<T>::Fr; }; //ERROR: no member named 'Fr' in B<T>

So the OwnedTagDecl should always be nullptr here/the issue should never arise in this case.

[davrec]

In D131685#3716354, @balazske wrote:

I really do not know why parent of the node for the owned TagDecl node is the FriendDecl and not a TypeLoc node, but it is working.
The code struct A { friend struct Fr; }; caused no problems either (no duplicate visit of struct Fr).

This is because the TraverseDecl(OwnedTagDecl) call is performed in the TraverseFriendDecl call, after the TraverseTypeLoc call has already returned -- and I assume the ParentMap just looks uses the stack of traversal calls to generate the parents.
That said, this does raise the question of whether we should instead be changing DEF_TRAVERSE_TYPE(Elaborated) to *always* traverse its owned tag decl; that way the parent would be the TypeLoc node. However in more usual cases an ET's owned tag decl is added to the parent DeclContext; e.g. for struct B {int i} data;, B and data are added as separate declarations in the parent context (which is annoying -- so probably the Type node really *should* always be the parent of that decl, if we were writing the AST from scratch! -- but it is what it is). So if traversed all owned tag decls, we would get some duplicate visitations.

[sammccall]

This looks like a reasonable representation of such record decls to me.

Changing the AST to nest them under typelocs instead is indeed a bigger project (and it's hard to say whether it's a good idea without a lot of digging).

clang/include/clang/AST/RecursiveASTVisitor.h

1564–1565

That sounds plausible to me, FriendTemplateDecl is extremely obscure and possibly badly designed.
I'd probably err on the side of not adding a FIXME unless we're sure something needs to be done, as I doubt anyone will ever remove it.

clang/unittests/AST/ASTContextParentMapTest.cpp

145

I'm confused why this is conditional: isn't this the main thing that we're testing? Why do we want the test to silently pass if the AST structure changes so that ownedTagDecl is null? It's hard to tell from reading the code if anything is being tested at all.

[davrec]

clang/unittests/AST/ASTContextParentMapTest.cpp

145

Good point, agree we should expect FrATagDecl to be nonnull. And we should check that FrBTagDecl is null, or at a minimum that FrBTagDecl != FrATagDecl, because if they are the same decl we are traversing it twice. (This could happen if someone decides to try to reuse the ElaboratedType of A's friend decl in B's, to save memory or whatever.)

[balazske]

• Removed FIXME comment
• Updated test: check AST exactly

clang/include/clang/AST/RecursiveASTVisitor.h

 
 DEF_TRAVERSE_DECL(FileScopeAsmDecl,
                   { TRY_TO(TraverseStmt(D->getAsmString())); })
 
 DEF_TRAVERSE_DECL(ImportDecl, {})
 
 DEF_TRAVERSE_DECL(FriendDecl, {
   // Friend is either decl or a type.
-  if (D->getFriendType())
+  if (D->getFriendType()) {
     TRY_TO(TraverseTypeLoc(D->getFriendType()->getTypeLoc()));
-  else
+    // Traverse any CXXRecordDecl owned by this type, since
+    // it will not be in the parent context:
+    if (auto *ET = D->getFriendType()->getType()->getAs<ElaboratedType>())
+      TRY_TO(TraverseDecl(ET->getOwnedTagDecl()));
+  } else {
     TRY_TO(TraverseDecl(D->getFriendDecl()));
+  }
 })
 
 DEF_TRAVERSE_DECL(FriendTemplateDecl, {
   if (D->getFriendType())
     TRY_TO(TraverseTypeLoc(D->getFriendType()->getTypeLoc()));
   else
     TRY_TO(TraverseDecl(D->getFriendDecl()));
   for (unsigned I = 0, E = D->getNumTemplateParameters(); I < E; ++I) {
     TemplateParameterList *TPL = D->getTemplateParameterList(I);
     for (TemplateParameterList::iterator ITPL = TPL->begin(), ETPL = TPL->end();
          ITPL != ETPL; ++ITPL) {
       TRY_TO(TraverseDecl(*ITPL));
     }

clang/unittests/AST/ASTContextParentMapTest.cpp

EXPECT_TRUE(LambdaVerifier.match(
       "auto x = []{int y;};",
       varDecl(hasName("y"), hasAncestor(functionDecl(
                                 hasOverloadedOperatorName("()"),
                                 hasParent(cxxRecordDecl(
                                     isImplicit(), hasParent(lambdaExpr())))))),
       Lang_CXX11));
 }
 
+TEST(GetParents, FriendTypeLoc) {
+  auto AST = tooling::buildASTFromCode("struct A { friend struct Fr; };"
+                                       "struct B { friend struct Fr; };"
+                                       "struct Fr;");
+  auto &Ctx = AST->getASTContext();
+  auto &TU = *Ctx.getTranslationUnitDecl();
+  auto &A = *TU.lookup(&Ctx.Idents.get("A")).front();
+  auto &B = *TU.lookup(&Ctx.Idents.get("B")).front();
+  auto &FrA = *cast<FriendDecl>(*++(cast<CXXRecordDecl>(A).decls_begin()));
+  auto &FrB = *cast<FriendDecl>(*++(cast<CXXRecordDecl>(B).decls_begin()));
+  TypeLoc FrALoc = FrA.getFriendType()->getTypeLoc();
+  TypeLoc FrBLoc = FrB.getFriendType()->getTypeLoc();
+  TagDecl *FrATagDecl =
+      FrALoc.getTypePtr()->getAs<ElaboratedType>()->getOwnedTagDecl();
+  TagDecl *FrBTagDecl =
+      FrBLoc.getTypePtr()->getAs<ElaboratedType>()->getOwnedTagDecl();
+
+  EXPECT_THAT(Ctx.getParents(A), ElementsAre(DynTypedNode::create(TU)));
+  EXPECT_THAT(Ctx.getParents(B), ElementsAre(DynTypedNode::create(TU)));
+  EXPECT_THAT(Ctx.getParents(FrA), ElementsAre(DynTypedNode::create(A)));
+  EXPECT_THAT(Ctx.getParents(FrB), ElementsAre(DynTypedNode::create(B)));
+  EXPECT_THAT(Ctx.getParents(FrALoc), ElementsAre(DynTypedNode::create(FrA)));
+  EXPECT_THAT(Ctx.getParents(FrBLoc), ElementsAre(DynTypedNode::create(FrB)));
+  EXPECT_TRUE(FrATagDecl);
+  EXPECT_FALSE(FrBTagDecl);
+  EXPECT_THAT(Ctx.getParents(*FrATagDecl),
+              ElementsAre(DynTypedNode::create(FrA)));
+}
+
 } // end namespace ast_matchers
 } // end namespace clang