[Clang] `friend` declarations with a dependent NNS are not supported

[Sirraide]

Consider (https://godbolt.org/z/brf4183r8):

template <typename T>
struct TS {
    struct Nested {};
};

struct S {
    template <typename T>
    friend struct TS<T>::Nested;
};

This valid, if horrible, friend declaration, roughly speaking, has the effect of making TS<T>::Nested a friend of S for each specialisation of TS—whether implicit or explicit—that contains a declaration of some struct Nested. [temp.friend]p5

We do not and seemingly have never supported this feature properly (there are FIXMEs about this from 2013). When we encounter such a declaration, we print

<source>:8:26: warning: dependent nested name specifier 'TS<T>::' for friend class declaration is not
supported; turning off access control for 'S' [-Wunsupported-friend]
    8 |     friend struct TS<T>::Nested;
      |                   ~~~~~~~^

and disable access checking entirely for S (yes, that means anyone can now access any private members of S). GCC and MSVC both seem to support this feature properly, though EDG doesn’t (see the godbolt link above).

There is an AST node in Clang, FriendTemplateDecl, that seems to have been intended for this exact use case, but it is currently unused: we can serialise it, traverse it, and print it, but we only create it in exactly one place and never use it again after that; instantiating would theoretically raise an error, but I don’t think it’s currently possible to get to that code path.

If someone wants to look into working on this: from what I can tell, a good starting point would be to search for references to setUnsupportedFriend. In at least one of these places, we seem to be creating a regular FriendDecl for this, which should probably be a FriendTemplateDecl, but I haven’t looked into this too much...

[llvmbot]

@llvm/issue-subscribers-clang-frontend

Author: None (Sirraide)

Consider (https://godbolt.org/z/brf4183r8): ```c++ template <typename T> struct TS { struct Nested {}; };

struct S {
template <typename T>
friend struct TS<T>::Nested;
};

This valid, if horrible, `friend` declaration, roughly speaking, has the effect of making `TS&lt;T&gt;::Nested` a friend of `S` for each specialisation of `TS`—whether implicit or explicit—that contains a declaration of some `struct Nested`. [[temp.friend]p5](https://eel.is/c++draft/temp.friend#<!-- -->5)

We do not and seemingly have never supported this feature properly (there are FIXMEs about this from 2013). When we encounter such a declaration, we print

<source>:8:26: warning: dependent nested name specifier 'TS<T>::' for friend class declaration is not
supported; turning off access control for 'S' [-Wunsupported-friend]
8 | friend struct TS<T>::Nested;
| ~~~~~~~^

and disable access checking entirely for `S` (yes, that means *anyone* can now access any private members of `S`). GCC and MSVC both seem to support this feature properly, though EDG doesn’t (see the godbolt link above).

There is an AST node in Clang, `FriendTemplateDecl`, that seems to have been intended for this exact use case, but it is currently unused: we can serialise it, traverse it, and print it, but we only create it in exactly one place and never use it again after that; instantiating would theoretically raise an error, but I don’t think it’s currently possible to get to that code path.

If someone wants to look into working on this: from what I can tell, a good starting point would be to search for references to `setUnsupportedFriend`. In at least one of these places, we seem to be creating a regular `FriendDecl` for this, which should probably be a `FriendTemplateDecl`, but I haven’t looked into this too much...
</details>

[llvmbot]

@llvm/issue-subscribers-c-1

Author: None (Sirraide)

Consider (https://godbolt.org/z/brf4183r8): ```c++ template <typename T> struct TS { struct Nested {}; };

struct S {
template <typename T>
friend struct TS<T>::Nested;
};

This valid, if horrible, `friend` declaration, roughly speaking, has the effect of making `TS&lt;T&gt;::Nested` a friend of `S` for each specialisation of `TS`—whether implicit or explicit—that contains a declaration of some `struct Nested`. [[temp.friend]p5](https://eel.is/c++draft/temp.friend#<!-- -->5)

We do not and seemingly have never supported this feature properly (there are FIXMEs about this from 2013). When we encounter such a declaration, we print

<source>:8:26: warning: dependent nested name specifier 'TS<T>::' for friend class declaration is not
supported; turning off access control for 'S' [-Wunsupported-friend]
8 | friend struct TS<T>::Nested;
| ~~~~~~~^

and disable access checking entirely for `S` (yes, that means *anyone* can now access any private members of `S`). GCC and MSVC both seem to support this feature properly, though EDG doesn’t (see the godbolt link above).

There is an AST node in Clang, `FriendTemplateDecl`, that seems to have been intended for this exact use case, but it is currently unused: we can serialise it, traverse it, and print it, but we only create it in exactly one place and never use it again after that; instantiating would theoretically raise an error, but I don’t think it’s currently possible to get to that code path.

If someone wants to look into working on this: from what I can tell, a good starting point would be to search for references to `setUnsupportedFriend`. In at least one of these places, we seem to be creating a regular `FriendDecl` for this, which should probably be a `FriendTemplateDecl`, but I haven’t looked into this too much...
</details>

[a-tarasyuk]

@Sirraide should FriendTemplateDecl be used, or should FriendDecl be extended with template params? For example, should .friends() be split by each kind of friend, or should it be like a union?

llvm-project/clang/include/clang/AST/DeclCXX.h

Line 697 in c2717a8

friend_range friends() const;

llvm-project/clang/include/clang/AST/DeclFriend.h

Lines 201 to 206 in c2717a8

	class CXXRecordDecl::friend_iterator {
	friend class CXXRecordDecl;
	FriendDecl *Ptr;
	explicit friend_iterator(FriendDecl *Ptr) : Ptr(Ptr) {}

[Sirraide]

@Sirraide should FriendTemplateDecl be used

Afaik the entire point of that AST node, which is currently just unused, is to be used for this, but other than that I don’t know either; I haven’t checked what exactly it would take to implement this.

[a-tarasyuk]

@Sirraide Oke, thanks. I looked into it a bit, however I'm still unsure about CXXRecordDecl friends — it seems like both should be considered friends of CXXRecordDecl...

[Sirraide]

Yeah, afaik, implementing support for this is likely to entail some refactoring (I’ve heard that someone else tried to implement this before, but gave up on it because they had to change too much code, but I don’t remember who unfortunately...)

[a-tarasyuk]

I want to try to refactor this, but I’m unsure how long it’ll take :). I just wanted to ensure I’m on the right track by starting with the right base structure since the refactor depends on it. I'm thinking of using the Base class and moving the friend iterator logic into it, in case I need to handle FriendDecl and FriendTemplateDecl as separate types, or rely entirely on FriendDecl and clean up FriendTemplateDecl...

Is it okay to start a discussion here to gather other opinions (not sure who should be included)? Or is there a better alternative (discord ?) for that?

[Sirraide]

Is it okay to start a discussion here to gather other opinions (not sure who should be included)? Or is there a better alternative (discord ?) for that?

Here or on Discord works, yeah, but I feel like (some) people are more likely to respond on Discord unless you ping them directly here. As to who would know anything about what to do with this, I’m not sure. Iirc we have FIXMEs for this from over a decade ago, and I doubt whoever worked on friend declarations back then still remembers what the plan was for this...

[a-tarasyuk]

Here or on Discord works, yeah, but I feel like (some) people are more likely to respond on Discord unless you ping them directly here.

@Sirraide got it, thanks for the clear explanation!

@AaronBallman @erichkeane @cor3ntin - I’d be grateful if you could share your thoughts... Thanks!

[erichkeane]

I believe that FriendTemplateDecl should be a separate thing from FriendDecl, so combining them would be a mistake, we typically need to keep things with template params separate from the 'normal' ast nodes.

That said, there is PERHAPS potential of putting them in a common hierarchy, where FriendTemplateDecl inherits from FriendDecl (or contains one?).

[a-tarasyuk]

@erichkeane thanks. I’ll explore whether FriendTemplateDecl could inherit from FriendDecl...