[clang][Sema] Fix a CTAD regression after 42239d2e9

[zyn0217]

The most recent declaration of a template as a friend can introduce a different template parameter depth compared to what we anticipate from a CTAD guide.

Fixes #86769

[llvmbot]

@llvm/pr-subscribers-clang

Author: Younan Zhang (zyn0217)

Changes

The most recent declaration of a template as a friend can introduce a different template parameter depth compared to what we anticipate from a CTAD guide.

Fixes #86769

---

Full diff: https://github.com/llvm/llvm-project/pull/86914.diff

4 Files Affected:

• (modified) clang/docs/ReleaseNotes.rst (+3)
• (modified) clang/lib/Sema/SemaTemplate.cpp (+13-1)
• (modified) clang/test/SemaTemplate/concepts-friends.cpp (+24)
• (modified) clang/test/SemaTemplate/ctad.cpp (+1-1)

diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 7fbe2fec6ca065..ed0996bd3d30c0 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -345,6 +345,9 @@ Bug Fixes in This Version
 - Fixes an assertion failure on invalid code when trying to define member
   functions in lambdas.
 
+- Fixed a regression in CTAD that a friend declaration that befriends itself may cause
+  incorrect constraint substitution. (#GH86769).
+
 Bug Fixes to Compiler Builtins
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
diff --git a/clang/lib/Sema/SemaTemplate.cpp b/clang/lib/Sema/SemaTemplate.cpp
index 005529a53270c3..14fabe36eee794 100644
--- a/clang/lib/Sema/SemaTemplate.cpp
+++ b/clang/lib/Sema/SemaTemplate.cpp
@@ -1836,7 +1836,19 @@ static TemplateParameterList *GetTemplateParameterList(TemplateDecl *TD) {
   // Make sure we get the template parameter list from the most
   // recent declaration, since that is the only one that is guaranteed to
   // have all the default template argument information.
-  return cast<TemplateDecl>(TD->getMostRecentDecl())->getTemplateParameters();
+  Decl *ND = TD->getMostRecentDecl();
+  // Skip past friend Decls because they are not supposed to contain default
+  // template arguments. Moreover, these declarations may introduce template
+  // parameters living in different template depths than the corresponding
+  // template parameters in TD, causing unmatched constraint substitution.
+  //
+  // C++23 N4950 [temp.param]p12
+  // A default template argument shall not be specified in a friend class
+  // template declaration.
+  while (ND->getFriendObjectKind() != Decl::FriendObjectKind::FOK_None &&
+         ND->getPreviousDecl())
+    ND = ND->getPreviousDecl();
+  return cast<TemplateDecl>(ND)->getTemplateParameters();
 }
 
 DeclResult Sema::CheckClassTemplate(
diff --git a/clang/test/SemaTemplate/concepts-friends.cpp b/clang/test/SemaTemplate/concepts-friends.cpp
index 255b0858917fb6..31840309f1ec86 100644
--- a/clang/test/SemaTemplate/concepts-friends.cpp
+++ b/clang/test/SemaTemplate/concepts-friends.cpp
@@ -478,3 +478,27 @@ template <Concept> class Foo {
 };
 
 } // namespace FriendOfFriend
+
+namespace GH86769 {
+
+template <typename T>
+concept X = true;
+
+template <X T> struct Y {
+  Y(T) {}
+  template <X U> friend struct Y;
+  template <X U> friend struct Y;
+  template <X U> friend struct Y;
+};
+
+template <class T>
+struct Z {
+  // FIXME: This is ill-formed per N4950 [temp.param]p12.
+  template <X U = void> friend struct Y;
+};
+
+template struct Y<int>;
+template struct Z<int>;
+Y y(1);
+
+}
diff --git a/clang/test/SemaTemplate/ctad.cpp b/clang/test/SemaTemplate/ctad.cpp
index 388ed7d4cced18..ec144d4f44ba8c 100644
--- a/clang/test/SemaTemplate/ctad.cpp
+++ b/clang/test/SemaTemplate/ctad.cpp
@@ -53,4 +53,4 @@ X x;
 template<class T, class B> struct Y { Y(T); };
 template<class T, class B=void> struct Y ;
 Y y(1);
-};
+}

[hokein]

thanks, this looks good from my side.

[hokein]

nit: this is not a new thing in C++23, I also find it in the C++20 spec N4860. I'd remove the C++23 N4950.

And I think this is also a FIXME, clang should diagnose this error case, maybe add the FIXME in the comment.

I think this function is not only used for class templates, but also for function templates. For function templates, there is an exception per the standard (temp.param 12): If a friend function template declaration specifies a default template-argument, that declaration shall be a definition and shall be the only declaration of the function template in the translation unit.

[Sirraide]

I think this function is not only used for class templates, but also for function templates. For function templates, there is an exception per the standard (temp.param 12): If a friend function template declaration specifies a default template-argument, that declaration shall be a definition and shall be the only declaration of the function template in the translation unit.

In that case it’d make sense to add a test for a friend function template as well if there isn’t already one to make sure this change doesn’t break that.

[zyn0217]

nit: this is not a new thing in C++23, I also find it in the C++20 spec N4860. I'd remove the C++23 N4950.

I referenced that number because I'm concerned the paragraphs/wordings could vary across standards. :)
(Maybe I should use an earlier standard file number where this line first appears...?)

[Sirraide]

I referenced that number because I'm concerned the paragraphs/wordings could vary across standards. :) (Maybe I should use an earlier standard file number where this line first appears...?)

So apparently, the paragraphs do change sometimes, so if you want to cite a specific paragraph, then yeah, I’d include the standard version, but maybe cite the C++20 then in this case if it was added in C++20.

[zyn0217]

In that case it’d make sense to add a test for a friend function template as well if there isn’t already one to make sure this change doesn’t break that.

I think the change is probably fine for friend function templates with default template parameters: they're unique across one TU, and we have ensured the next declaration we visit is non-null. So in such cases, I think ND would be equivalent to "the most recent Decl".

but maybe cite the C++20 then in this case if it was added in C++20.

Finally, I decided to use N3337 (aka C++11) since I couldn't find any related words prior to that draft, although gcc and clang don't allow such default arguments even in 98/03 mode.

[Sirraide]

At least I can’t find a mention of ‘N3337’ anywhere else in the codebase

[zyn0217]

Isn’t ‘N3337’ just the last draft of C++11? Just ‘C++11’ should be enough then?

https://github.com/timsong-cpp/cppwp says N3337 is the one with editorial fixes. So, I think it is.

[Sirraide]

I’d usually not include e.g. ‘N3337’ then unless it’s something that was changed specifically in that version of the standard as opposed to a previous version. If it’s the most up-to-date version, then just e.g. ‘C++11’ in this case would have been enough imo.

[zyn0217]

At least I can’t find a mention of ‘N3337’ anywhere else in the codebase

Interestingly, I found people tend not to spell out the standard file number throughout the codebase - seemingly, we usually speak c++(version) [paragraph]/X.Y.

(I will respect such a convention next time; thanks for reminding me!)

[Sirraide]

I found people tend not to spell out the standard file number throughout the codebase

Probably beacuse most people don’t have all of them memorised (at least I don’t), whereas e.g. ‘C++11’ makes it pretty clear what we’re actually talking about.

[zyn0217]

(Since there haven't been any changes besides comments after the very first commit, which has passed the CI, so I'm landing it anyway.)

[Sirraide]

@zyn0217 You missed my last comment on the discussion above there