[Clang][Sema] Use correct TemplateName when transforming TemplateSpecializationType

[jcsxky]

Consider the following testcase:

namespace PR12884_original {
  template <typename T> struct A {
    struct B { ##1
      template <typename U> struct X {};
      typedef int arg;
    };
    struct C {
      typedef B::X<typename B::arg> x; 
    };
  };

  template <> struct A<int>::B { ##2
    template <int N> struct X {};
    static const int arg = 0;
  };

  A<int>::C::x a;
}

It will crash when compiling with clang(assertions trunk). The reason is that we lookup X(B::X) in ##1 when instantiating typedef B::X<typename B::arg> x; during instantiating A<int>::C::x. This is incorrect because we should lookup X in ##2 when we see the declaration A<int>::C::x a;. Since clang parse A<T>::B<T> to an ElaboratedType(typename is not required while compiling with -std=c++20) while typename A<T>::B<T> turns to be a DependentTemplateSpecializationType, we should rebuild the TemplateName with transformed Qualifier(whose type is NestedNameSpecifier) to make sure the lookup context is correct.
This patch also attempts to fix #91677 which crashes with the same reason.

[llvmbot]

@llvm/pr-subscribers-clang

Author: Qizhi Hu (jcsxky)

Changes

Consider the following testcase:

namespace PR12884_original {
  template <typename T> struct A {
    struct B { ##<!-- -->1
      template &lt;typename U&gt; struct X {};
      typedef int arg;
    };
    struct C {
      typedef B::X&lt;typename B::arg&gt; x; 
    };
  };

  template &lt;&gt; struct A&lt;int&gt;::B { ##<!-- -->2
    template &lt;int N&gt; struct X {};
    static const int arg = 0;
  };

  A&lt;int&gt;::C::x a;
}

It will crash when compiling with clang(assertions trunk). The reason is that we lookup X(B::X) in ##<!-- -->1 when instantiating typedef B::X<typename B::arg> x; during instantiating A<int>::C::x. This is incorrect because we should lookup X in ##<!-- -->2 when we see the declaration A<int>::C::x a;. Since clang parse A<T>::B<T> to an ElaboratedType(typename is not required while compiling with -std=c++20) while typename A<T>::B<T> turns to be a DependentTemplateSpecializationType, we should rebuild the TemplateName with transformed Qualifier(whose type is NestedNameSpecifier) to make sure the lookup context is correct.
This patch also attempts to fix #91677 which crashes with the same reason.

---

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

3 Files Affected:

• (modified) clang/lib/Sema/TreeTransform.h (+24-3)
• (added) clang/test/SemaCXX/PR91677.cpp (+31)
• (modified) clang/test/SemaTemplate/typename-specifier-3.cpp (+4-3)

diff --git a/clang/lib/Sema/TreeTransform.h b/clang/lib/Sema/TreeTransform.h
index dee335b526991..6ef2eec09ec02 100644
--- a/clang/lib/Sema/TreeTransform.h
+++ b/clang/lib/Sema/TreeTransform.h
@@ -7216,9 +7216,30 @@ TreeTransform<Derived>::TransformElaboratedType(TypeLocBuilder &TLB,
       return QualType();
   }
 
-  QualType NamedT = getDerived().TransformType(TLB, TL.getNamedTypeLoc());
-  if (NamedT.isNull())
-    return QualType();
+  QualType NamedT;
+  if (SemaRef.getLangOpts().CPlusPlus20 && QualifierLoc &&
+      isa<TemplateSpecializationType>(TL.getNamedTypeLoc().getType())) {
+    TemplateSpecializationTypeLoc SpecTL =
+        TL.getNamedTypeLoc().castAs<TemplateSpecializationTypeLoc>();
+    const TemplateSpecializationType *TST =
+        SpecTL.getType()->castAs<TemplateSpecializationType>();
+    CXXScopeSpec SS;
+    SS.Adopt(QualifierLoc);
+    if (TemplateDecl *TD = TST->getTemplateName().getAsTemplateDecl()) {
+      TemplateName InstName = getDerived().RebuildTemplateName(
+          SS, TL.getTemplateKeywordLoc(), *TD->getIdentifier(),
+          TL.getNamedTypeLoc().getBeginLoc(), /*ObjectType=*/QualType(),
+          /*FirstQualifierInScope=*/nullptr, /*AllowInjectedClassName=*/false);
+      if (InstName.isNull())
+        return QualType();
+      NamedT = TransformTemplateSpecializationType(TLB, SpecTL, InstName);
+    }
+  }
+  if (NamedT.isNull()) {
+    NamedT = getDerived().TransformType(TLB, TL.getNamedTypeLoc());
+    if (NamedT.isNull())
+      return QualType();
+  }
 
   // C++0x [dcl.type.elab]p2:
   //   If the identifier resolves to a typedef-name or the simple-template-id
diff --git a/clang/test/SemaCXX/PR91677.cpp b/clang/test/SemaCXX/PR91677.cpp
new file mode 100644
index 0000000000000..ef2999f959506
--- /dev/null
+++ b/clang/test/SemaCXX/PR91677.cpp
@@ -0,0 +1,31 @@
+// 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>();
+}
+
+namespace N {
+  template <typename T> struct A {
+    struct B {
+      template <typename U> struct X {};
+      typedef int arg;
+    };
+    struct C {
+      typedef B::template X<B::arg> x;
+    };
+  };
+
+  template <> struct A<int>::B {
+    template <int N> struct X {};
+    static const int arg = 0;
+  };
+}
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 {

[zyn0217]

A drive-by comment: The test PR12884_original is still crashing on trunk but it slips through our testing probably because we somehow stop after errors above - if we move the whole namespace to the top, then we'd immediately run into an assert saying Unable to find instantiation of declaration!.

[zyn0217]

Did you mean getDerived().TransformTemplateSpecializationType()?

[zyn0217]

if (QualType QT = TL.getNamedTypeLoc().getType(); ... && isa<...>(QT)) {
  auto SpecTL = ...;
  auto *TST = QT.getTypePtr();
}

Probably looks better.

[mizvekov]

The problem here is the loss of the qualification on the TemplateNames

This patch fixes the problem, without taking any workarounds: #93433

It also doesn't cause any change in diagnostics in clang/test/SemaTemplate/typename-specifier-3.cpp.

[jcsxky]

The problem here is the loss of the qualification on the TemplateNames

This patch fixes the problem, without taking any workarounds: #93433

It also doesn't cause any change in diagnostics in clang/test/SemaTemplate/typename-specifier-3.cpp.

I think we should report this message since B::arg in A<int> is a NTTP.

[mizvekov]

The problem here is the loss of the qualification on the TemplateNames
This patch fixes the problem, without taking any workarounds: #93433
It also doesn't cause any change in diagnostics in clang/test/SemaTemplate/typename-specifier-3.cpp.

I think we should report this message since B::arg in A<int> is a NTTP.

Yeah, I am not saying that is wrong, but this needs more investigation.

I am not looking at this bug specifically: I just noticed my patch would have an effect here, and after testing it sure did, but I haven't looked closely.

Maybe I used the term 'fixes' incorrectly, as in my patch makes it not crash, but it's strange that type sugar would have a semantic effect here, that is not supposed to happen.

Again, thinking in first principles, without looking at this example too specifically, my intuition says that what might be happening here is that we transformed a DependentTemplateSpecializationType into a TemplateSpecializationType too early, before resolving everything that needs to be resolved related to it's NestedNameSpecifier.

[mizvekov]

So After we have formed a TemplateSpecializationType, we are done with the NNS. We keep it around in an ElaboratedType for type sugar only, it should not be needed to compile the program correctly anymore.

So I think this solution violates one important aspect of our AST design.

If we still need the NNS, we have to hold-off on building the TST until we don't.

[jcsxky]

So After we have formed a TemplateSpecializationType, we are done with the NNS. We keep it around in an ElaboratedType for type sugar only, it should not be needed to compile the program correctly anymore.

So I think this solution violates one important aspect of our AST design.

If we still need the NNS, we have to hold-off on building the TST until we don't.

Ah, I see. Thanks for your explanation on the motivation of AST design!