[Clang][Sema] qualifier should be transformed

[jcsxky]

Transform qualifier of TemplateName. Attempt to fix #91677 and some other issues.

[llvmbot]

@llvm/pr-subscribers-clang

Author: Qizhi Hu (jcsxky)

Changes

---

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

6 Files Affected:

• (modified) clang/lib/Sema/SemaTemplateInstantiateDecl.cpp (+3-4)
• (modified) clang/lib/Sema/TreeTransform.h (+18)
• (modified) clang/test/SemaCXX/many-template-parameter-lists.cpp (+3-3)
• (added) clang/test/SemaTemplate/PR91677.cpp (+14)
• (modified) clang/test/SemaTemplate/instantiate-scope.cpp (+8-7)
• (modified) clang/test/SemaTemplate/typename-specifier-3.cpp (+4-3)

diff --git a/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp b/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
index 0681520764d9a..d30a9c2a2588f 100644
--- a/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
+++ b/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
@@ -60,10 +60,9 @@ static bool SubstQualifier(Sema &SemaRef, const DeclT *OldDecl, DeclT *NewDecl,
           !OldDecl->getLexicalDeclContext()->isDependentContext()) &&
          "non-friend with qualified name defined in dependent context");
   Sema::ContextRAII SavedContext(
-      SemaRef,
-      const_cast<DeclContext *>(NewDecl->getFriendObjectKind()
-                                    ? NewDecl->getLexicalDeclContext()
-                                    : OldDecl->getLexicalDeclContext()));
+      SemaRef, const_cast<DeclContext *>(NewDecl->getFriendObjectKind()
+                                             ? NewDecl->getLexicalDeclContext()
+                                             : NewDecl->getDeclContext()));
 
   NestedNameSpecifierLoc NewQualifierLoc
       = SemaRef.SubstNestedNameSpecifierLoc(OldDecl->getQualifierLoc(),
diff --git a/clang/lib/Sema/TreeTransform.h b/clang/lib/Sema/TreeTransform.h
index 3bfda09d5f80f..b4d31cbbf0c7c 100644
--- a/clang/lib/Sema/TreeTransform.h
+++ b/clang/lib/Sema/TreeTransform.h
@@ -4568,6 +4568,24 @@ TreeTransform<Derived>::TransformTemplateName(CXXScopeSpec &SS,
   if (QualifiedTemplateName *QTN = Name.getAsQualifiedTemplateName()) {
     TemplateDecl *Template = QTN->getUnderlyingTemplate().getAsTemplateDecl();
     assert(Template && "qualified template name must refer to a template");
+    if (QTN->getQualifier()) {
+      CXXScopeSpec QualifierSS;
+      QualifierSS.MakeTrivial(getSema().getASTContext(), QTN->getQualifier(),
+                              NameLoc);
+      NestedNameSpecifierLoc QualifierLoc =
+          QualifierSS.getWithLocInContext(getSema().getASTContext());
+      NestedNameSpecifierLoc TransformedQualifierLoc =
+          getDerived().TransformNestedNameSpecifierLoc(QualifierLoc);
+      if (!TransformedQualifierLoc)
+        return Name;
+      if (!getDerived().AlwaysRebuild() &&
+          QualifierLoc != TransformedQualifierLoc) {
+        SS.Adopt(TransformedQualifierLoc);
+        return getDerived().RebuildTemplateName(
+            SS, SourceLocation(), *Template->getIdentifier(), NameLoc,
+            ObjectType, FirstQualifierInScope, /*AllowInjectedClassName=*/true);
+      }
+    }
 
     TemplateDecl *TransTemplate
       = cast_or_null<TemplateDecl>(getDerived().TransformDecl(NameLoc,
diff --git a/clang/test/SemaCXX/many-template-parameter-lists.cpp b/clang/test/SemaCXX/many-template-parameter-lists.cpp
index f98005c7e6fb5..b5d954986dd4f 100644
--- a/clang/test/SemaCXX/many-template-parameter-lists.cpp
+++ b/clang/test/SemaCXX/many-template-parameter-lists.cpp
@@ -5,7 +5,7 @@
 template <class T>
 struct X {
   template <class U>
-  struct A { // expected-note {{not-yet-instantiated member is declared here}}
+  struct A {
     template <class V>
     struct B {
       template <class W>
@@ -28,9 +28,9 @@ struct X {
   template <class X>
   template <class Y>
   template <class Z>
-  friend void A<U>::template B<V>::template C<W>::template D<X>::template E<Y>::operator+=(Z); // expected-warning {{not supported}} expected-error {{no member 'A' in 'X<int>'; it has not yet been instantiated}}
+  friend void A<U>::template B<V>::template C<W>::template D<X>::template E<Y>::operator+=(Z); // expected-warning {{dependent nested name specifier 'A<U>::B<V>::C<W>::D<X>::template E<Y>::' for friend class declaration is not supported; turning off access control for 'X'}}
 };
 
 void test() {
-  X<int>::A<int>::B<int>::C<int>::D<int>::E<int>() += 1.0; // expected-note {{in instantiation of template class 'X<int>' requested here}}
+  X<int>::A<int>::B<int>::C<int>::D<int>::E<int>() += 1.0;
 }
diff --git a/clang/test/SemaTemplate/PR91677.cpp b/clang/test/SemaTemplate/PR91677.cpp
new file mode 100644
index 0000000000000..cc8db60a438ea
--- /dev/null
+++ b/clang/test/SemaTemplate/PR91677.cpp
@@ -0,0 +1,14 @@
+// RUN: %clang_cc1 -verify -std=c++20 -fsyntax-only %s
+// expected-no-diagnostics
+
+template <typename> struct t1 {
+  template <typename>
+  struct t2 {};
+};
+
+template <typename T>
+t1<T>::template t2<T> f1();
+
+void f2() {
+  f1<bool>();
+}
diff --git a/clang/test/SemaTemplate/instantiate-scope.cpp b/clang/test/SemaTemplate/instantiate-scope.cpp
index 733105674b7a4..1f0f58d6a024e 100644
--- a/clang/test/SemaTemplate/instantiate-scope.cpp
+++ b/clang/test/SemaTemplate/instantiate-scope.cpp
@@ -2,7 +2,8 @@
 
 template<typename ...T> struct X {
   void f(int);
-  void f(...);
+  void f(...); // expected-note {{member is declared here}} \
+                  expected-note {{member is declared here}}
   static int n;
 };
 
@@ -11,11 +12,11 @@ template<typename T, typename U> using A = T;
 // These definitions are OK, X<A<T, decltype(...)>...> is equivalent to X<T...>
 // so this defines the member of the primary template.
 template<typename ...T>
-void X<A<T, decltype(f(T()))>...>::f(int) {} // expected-error {{undeclared}}
-
+void X<A<T, decltype(f(T()))>...>::f(int) {} // expected-error {{explicit qualification required to use member 'f' from dependent base class}} \
+                                                expected-error {{call to non-static member function without an object argument}}
 template<typename ...T>
-int X<A<T, decltype(f(T()))>...>::n = 0; // expected-error {{undeclared}}
-
+int X<A<T, decltype(f(T()))>...>::n = 0; // expected-error {{explicit qualification required to use member 'f' from dependent base class}} \
+                                            expected-error {{call to non-static member function without an object argument}}
 struct Y {}; void f(Y);
 
 void g() {
@@ -25,6 +26,6 @@ void g() {
 
   // Error, substitution fails; this should not be treated as a SFINAE-able
   // condition, so we don't select X<void>::f(...).
-  X<void>().f(0); // expected-note {{instantiation of}}
-  X<void>::n = 1; // expected-note {{instantiation of}}
+  X<void>().f(0); // expected-note {{in instantiation of member function 'X<void>::f' requested here}}
+  X<void>::n = 1; // expected-note {{in instantiation of static data member 'X<void>::n' requested here}}
 }
diff --git a/clang/test/SemaTemplate/typename-specifier-3.cpp b/clang/test/SemaTemplate/typename-specifier-3.cpp
index 714830f0032d2..a62a1fc5ab39c 100644
--- a/clang/test/SemaTemplate/typename-specifier-3.cpp
+++ b/clang/test/SemaTemplate/typename-specifier-3.cpp
@@ -28,16 +28,17 @@ namespace PR12884_original {
       typedef int arg;
     };
     struct C {
-      typedef B::X<typename B::arg> x; // precxx17-warning{{missing 'typename' prior to dependent type name B::X; implicit 'typename' is a C++20 extension}}
+      typedef B::X<typename B::arg> x; // precxx17-warning{{missing 'typename' prior to dependent type name B::X; implicit 'typename' is a C++20 extension}} \
+                                       cxx17-error{{typename specifier refers to non-type member 'arg' in 'PR12884_original::A<int>::B'}}
     };
   };
 
   template <> struct A<int>::B {
     template <int N> struct X {};
-    static const int arg = 0;
+    static const int arg = 0; // cxx17-note{{referenced member 'arg' is declared here}}
   };
 
-  A<int>::C::x a;
+  A<int>::C::x a; // cxx17-note{{in instantiation of member class 'PR12884_original::A<int>::C' requested here}}
 }
 namespace PR12884_half_fixed {
   template <typename T> struct A {

[jcsxky]

crash on trunk assertion.

[mizvekov]

I think this is missing one detail: We now have the same qualifier in two places: The elaborated type node attached to the TST, and in the name of the TST itself.

While this is not ideal situation, I think it makes sense to just drop the TemplateName qualifier in the transform for now, so we don't keep transforming it twice, until we make the situation consistent again. Right now the NNS in the TST name is not printed, its only purpose there is to carry the template keyword information, which shouldn't be affected here.

The fact that this change somehow affects program semantics is still unexpected and unexplained.

As I said on the other PR, after we have built a TST, per current AST design, we shouldn't need the nested name specifier anymore beyond type sugar.

So either we built this TST too early, as in it should have stayed as a DependentTemplateSpecializationType longer or some such, or there is something that needs to be rectified in the AST design, but I can't imagine what.

If I had to take a guess, there is something wrong in the dependency computation for the NNS. Have you checked that?

[mizvekov]

Needs changes as discussed.

[jcsxky]

Needs changes as discussed.

Needs changes as discussed.

I am really appreciate for your guidance and I will check in the weekend.

[jcsxky]

@mizvekov After looking into the code, I think we should transform qualifier in TST. Consider the following code:

template <typename T>
t1<T>::template t2<T> f1();

void f2() {
  f1<bool>();
}

TemplateSpecializationType of t2 whose Template is a QualifiedTemplateName(t1<T>::) will be dependent if we didn't transform the qualifier since its qualifier and DeclContext are both dependent and it wouldn't be specialized in Sema::CheckTemplateIdType due to its name dependency. Transform the qualifier and rebuild the TemplateName as a DependentTemplateName is also consistent with what it is before cpp20(t2 is a DependentTemplateName) which needs a typename keyword(typename t1<T>::template t2<T>).

The fact that this change somehow affects program semantics is still unexpected and unexplained.

The case in clang/test/SemaCXX/many-template-parameter-lists.cpp is affected is because the qualifier didn't be transformed and we can't find the instantiation of the declaration(X) which will not happen after this patch. This case is also accepted by gcc. The other cases are expected or explained in the comments.

If I had to take a guess, there is something wrong in the dependency computation for the NNS. Have you checked that?

I didn't find some problems in dependency computation for the NNS.

[mizvekov]

@mizvekov After looking into the code, I think we should transform qualifier in TST. Consider the following code:

So the NNS is already dependent, that's great.

I still don't understand why you are saying we didn't or can't build a DependentTemplateSpecializationType instead.

Yes, the template name needs to stay a DependentTemplateName for that.

[jcsxky]

I still don't understand why you are saying we didn't or can't build a DependentTemplateSpecializationType instead.

I mean we can transform QualifiedTemplateName to DependentTemplateName and this is what this patch does. If we build a DependentTemplateName instead of QualifiedTemplateName earlier would affect the code in partial specialization like:

template<typename T>
struct A<T*>::B<T*>

[mizvekov]

I mean we can transform QualifiedTemplateName to DependentTemplateName and this is what this patch does.

But that goes against the natural flow of template instantiation. We can't go back from non-dependent into dependent.
We can't transform a regular TemplateName back into a DependentTemplateName, just as we can't go back to DependentTemplateSpecializationType from a TemplateSpecializationType.

That's why I find this so confusing...

If we build a DependentTemplateName instead of QualifiedTemplateName earlier would affect the code in partial specialization like:

template<typename T>
struct A<T*>::B<T*>

Can you elaborate on that? That still seems like the right path to me, you probably found another problem, that happens some times.

[jcsxky]

We can't go back from non-dependent into dependent.

Sorry, maybe I use the word 'transform' that makes you confused. Actually, what I did in TransformTemplateName is transforming a QualifiedTemplateName which is dependent due to its qualifier dependency and dependent DeclContext to a DependentTemplateName. Although the transformed DependentTemplateName is also dependent, its qualifier is independent. This make the condition

llvm-project/clang/lib/Sema/SemaTemplate.cpp

Line 4691 in e7622ab

} else if (Name.isDependent() ||

false which holds before this patch. This time the flow goes into

llvm-project/clang/lib/Sema/SemaTemplate.cpp

Lines 4741 to 4746 in e7622ab

	} else if (ClassTemplateDecl *ClassTemplate =
	dyn_cast<ClassTemplateDecl>(Template)) {
	// Find the class template specialization declaration that
	// corresponds to these arguments.
	void *InsertPos = nullptr;
	ClassTemplateSpecializationDecl *Decl =

and do the specialization.

Can you elaborate on that?

I just tried to create DependentTemplateName at

llvm-project/clang/lib/Sema/SemaTemplate.cpp

Lines 297 to 298 in e7622ab

	Template = Context.getQualifiedTemplateName(Qualifier, hasTemplateKeyword,
	Template);

a few days ago and break several cases which contains the code related to partial specialization. But I didn't dig into the code and give up that approach.