This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by balazske on Aug 11 2022, 8:18 AM.

Details

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.

Diff Detail

Event Timeline

balazske created this revision.Aug 11 2022, 8:18 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 11 2022, 8:18 AM
balazske requested review of this revision.Aug 11 2022, 8:18 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 11 2022, 8:18 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

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).

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
1577–1578

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.

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 accepted this revision.Aug 17 2022, 9:27 AM

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
1577–1578

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.

This revision is now accepted and ready to land.Aug 17 2022, 9:27 AM
davrec added inline comments.Aug 17 2022, 4:53 PM
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 updated this revision to Diff 454408.Aug 22 2022, 1:26 AM
balazske marked 4 inline comments as done.
  • Removed FIXME comment
  • Updated test: check AST exactly