AST parent of class in a friend declaration

Should have the CXXRecordDecl for struct B have a parent in ParentMap in this case?

struct A { friend struct B; };

The Decl object for struct B exists but is not lookupable in the AST and has no parent.

The question is related to bug [clang-tidy] cert-dcl58-cpp crashes with friend declaration · Issue #56902 · llvm/llvm-project · GitHub . ASTMatcher library expects that every non-root node has a parent.

IIUC ParentMap is for statements only.

There seem to be only declarations in the example.

`-**CXXRecordDecl** 0x557a12b2f830 <<source>:1:1, col:29> col:8 struct **A** definition
  `-**FriendDecl** 0x557a12b2faf8 <col:12, col:26> col:19 'struct B':'B'

Yeah decls usually have other ways to traverse to parent, such as getDeclContext() which would jump from B to A in this case.

I also don’t see why there’s no way to jump to the definition of B in this case, doesn’t getFriendType() or getFriendDecl() get you there?

So I have to change the question: It is ParentMapContext, not ParentMap. An empty list is returned if called with the Decl object of struct B. The declaration is reachable with getFriendDecl (or in the type in getFriendType) but it is not in any lookup table of a DeclContext (probably this is why it is not in ParentMapContext). If this is a normal case there is some problem in ASTMatchFinder code:

    if (Parents.empty()) {
      // Nodes may have no parents if:
      //  a) the node is the TranslationUnitDecl
      //  b) we have a limited traversal scope that excludes the parent edges
      //  c) there is a bug in the AST, and the node is not reachable

I’d expect that either the lexical declaration context (stuct A) or the semantical decl context (the TranslationUnitDecl) is in the parent map. If this is not the case, then I think we should lift this up into a github issue and develop a possible fix for that in the ParentMapContext.

My investigation shows that in code struct A { friend struct B; }; the struct A (as a DeclContext) does not contain struct B, and the TU itself does not contain it. This is probably not a bug.
It looks like that a RecursiveASTVisitor does not visit struct B (declaration) at all in this case. It is a “type” friend, not a declaration, the visitor visits only the TypeLoc in this case. This is probably a bug. The parent map do not contain the relationship if it is not visited by the AST visitor. But if the friend declaration itself is not a real declaration of struct B this can be correct. Then the ASTMatchFinder must be changed to not expect always a parent or not reach struct B at all here. (The problem may be that the canonical declaration can be the one in the friend even if another one exists at a later place.)

I thought that friend class B; introduces class B type in the enclosing scope and expected the same as @martong.

But it looks like it’s a bit more nuanced:

A name first declared in a friend declaration within a class or class template X becomes a member of the innermost enclosing namespace of X , but is not visible for lookup (except argument-dependent lookup that considers X ) unless a matching declaration at namespace scope is provided […].

This sounds like the declaration “is and is not” in the enclosing namespace at the same time.

If anywhere the hypothetical Decl of B would possibly be in DeclContext represented by TranslationUnitDecl (which would be its semantical scope). But it looks like we actually represent it in the AST by the FriendDecl in class A (the lexical scope) and don’t create the “invisible” Decl at all.

I don’t know what exactly is the interface contract of RecursiveASTVisitor but can imagine this might be correct - after all if the Decl we’d expect is not in the AST then there’s nothing to visit.

The last time I tried to understand what “canonical declaration” means in clang I got the impression that this concept is defined rather vaguely and might even just mean “the first Decl we’ve parsed”. Having FriendDecl as the canonical declaration in presence of another “normal” declaration feels a bit surprising but might be correct.

@balazske Would you be able to get the parent via this?

FriendDecl::getFriendType() -> TypeSourceInfo::getType() -> QualType:: getTypePtr() -> Type:: getAsCXXRecordDecl()

RecursiveASTVisitor has different virtual member functions that can be overridden by the derived visitor; this way the strategy of the visitation can be altered. Notably, there is shouldWalkTypesOfTypeLocs() that might just solve the issue we face here. Look at this hunk at RecursiveASTVisitor.h:

#define DEF_TRAVERSE_TYPELOC(TYPE, CODE)                                       \
  template <typename Derived>                                                  \
  bool RecursiveASTVisitor<Derived>::Traverse##TYPE##Loc(TYPE##Loc TL) {       \
    if (!getDerived().shouldTraversePostOrder()) {                             \
      TRY_TO(WalkUpFrom##TYPE##Loc(TL));                                       \
      if (getDerived().shouldWalkTypesOfTypeLocs())                            \
        TRY_TO(WalkUpFrom##TYPE(const_cast<TYPE *>(TL.getTypePtr())));         \

And this:

  // Friend is either decl or a type.
  if (D->getFriendType())

@balazske, could you please try to override shouldWalkTypesOfTypeLocs in your custom visitor?

I think what is needed is to add a conditional traversal to the FriendDecl traversal. RecursiveASTVisitor is very anal about traversing/visiting each node exactly once, so we definitely need to traverse that decl somewhere since it was not added to the parent context, but we don’t want to traverse if we have already done so elsewhere. The key case to consider:

struct A { friend struct B; };
struct C { friend struct B; };

Those two friend types refer to the same CXXRecordDecl pointer; we want to avoid traversing it twice.

Fortunately ElaboratedType has a getOwnedTagDecl() method which should help us out, since only one of those seems to own that pointer. So I think this might do it:

  // Friend is either decl or a type.
  if (D->getFriendType()) {
    // Traverse any CXXRecordDecl owned by this type, since
    // it will not be in the parent context:
    if (auto ET = TSI->getType()->getAs<ElaboratedType>())
  } else

This fix seems working, I created a patch: