[clang] Exclude non-template classes when checking if constraint refers to containing template arguments

[antangelo]

When checking if the constraint uses any enclosing template parameters for [temp.friend]p9, if a containing record is used as argument, we assume that the constraint depends on enclosing template parameters without checking if the record is templated. The reproducer from the bug is included as a test case.

I would appreciate if someone could double check my interpretation of the standard here as I'm not the most experienced with it.

Fixes #71595

[llvmbot]

@llvm/pr-subscribers-clang

Author: None (antangelo)

Changes

When checking if the constraint uses any enclosing template parameters for [temp.friend]p9, if a containing record is used as argument, we assume that the constraint depends on enclosing template parameters without checking if the record is templated. The reproducer from the bug is included as a test case.

I would appreciate if someone could double check my interpretation of the standard here as I'm not the most experienced with it.

Fixes #71595

---

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

3 Files Affected:

• (modified) clang/docs/ReleaseNotes.rst (+3)
• (modified) clang/lib/Sema/SemaTemplate.cpp (+2)
• (added) clang/test/SemaTemplate/GH71595.cpp (+34)

diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index c7a948fd3fae5..852b908dd1594 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -651,6 +651,9 @@ Bug Fixes in This Version
 - Fixed false positive error emitted by clang when performing qualified name
   lookup and the current class instantiation has dependent bases.
   Fixes (`#13826 <https://github.com/llvm/llvm-project/issues/13826>`_)
+- Fix a ``clang-17`` regression where a templated friend with constraints is not 
+  properly applied when its parameters reference an enclosing non-template class.
+  Fixes (`#71595 <https://github.com/llvm/llvm-project/issues/71595>`_)
 
 Bug Fixes to Compiler Builtins
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
diff --git a/clang/lib/Sema/SemaTemplate.cpp b/clang/lib/Sema/SemaTemplate.cpp
index 09bbf14d39af5..aec802b289a2d 100644
--- a/clang/lib/Sema/SemaTemplate.cpp
+++ b/clang/lib/Sema/SemaTemplate.cpp
@@ -1714,6 +1714,8 @@ class ConstraintRefersToContainingTemplateChecker
   // Friend, likely because it was referred to without its template arguments.
   void CheckIfContainingRecord(const CXXRecordDecl *CheckingRD) {
     CheckingRD = CheckingRD->getMostRecentDecl();
+    if (!CheckingRD->isTemplated())
+      return;
 
     for (const DeclContext *DC = Friend->getLexicalDeclContext();
          DC && !DC->isFileContext(); DC = DC->getParent())
diff --git a/clang/test/SemaTemplate/GH71595.cpp b/clang/test/SemaTemplate/GH71595.cpp
new file mode 100644
index 0000000000000..7d34d1bf054e4
--- /dev/null
+++ b/clang/test/SemaTemplate/GH71595.cpp
@@ -0,0 +1,34 @@
+// RUN: %clang_cc1 -std=c++20 -verify %s
+
+template<class T, class U>
+concept C = true;
+
+class non_temp {
+    template<C<non_temp> T>
+    friend void f();
+
+    non_temp();
+};
+
+template<C<non_temp> T>
+void f() {
+    auto v = non_temp();
+}
+
+template<class A>
+class temp {
+    template<C<temp> T>
+    friend void g();
+
+    temp(); // expected-note {{implicitly declared private here}}
+};
+
+template<C<temp<int>> T>
+void g() {
+    auto v = temp<T>(); // expected-error {{calling a private constructor of class 'temp<int>'}}
+}
+
+void h() {
+    f<int>();
+    g<int>(); // expected-note {{in instantiation of function template specialization 'g<int>' requested here}}
+}

[erichkeane]

We should actually be erroring on this IIRC. The fact that this is not a definition should be an error. GCC says: error: friend function template with constraints that depend on outer template parameters must be a definition

[antangelo]

Do you want me to address this as part of this patch or as a followup?

[erichkeane]

That can be a followup.

[erichkeane]

So this might not be the 'right' thing here. "isTemplated" doesn't mean "this is a template", it means "this, or something that contains it is a template".

So:

template <typename T>
struct Parent {
   struct Child {
    };
};

In this case, Parent is a class template, but Child is still isTemplated.

[antangelo]

What do you think about this example, which exercises this issue in the above case? https://godbolt.org/z/Yz1WEqM7M

If I switch from isTemplated() to getDescribedTemplate() to only check CheckingRD itself, then clang will accept that code. Otherwise, as the patch is now, the example is rejected. What should be the correct behavior?

[erichkeane]

Hmm... I THINK that should be rejected, since you can't name Child without naming Parent? But I don't think the standard is explicit about that.

So perhaps isTemplated is what we want here.

I didn't think it too far through when making the comment, but I'm glad you put together a good example.