[clang] Fix specialization of non-templated member classes of class templates

[Fznamznon]

Summary

Explicit specialization doesn't increase depth of template parameters,
so need to be careful when gathering template parameters for
instantiation.
For the case:

template<typename T>
struct X {
  struct impl;
};

template <>
struct X<int>::impl {
    template<int ct>
    int f() { return ct; };
};

instantiation of f used to crash because type template parameter
int of explicit specialization was taken into account, but non-type
template parameter ct had zero depth and index so wrong parameter
ended up inside of a wrong handler.

Fixes https://github.com/llvm/llvm-project/issues/61159

[shafik]

Thank you for the fix! Can you explain why the fix fixes the bug? It is not a lot of change but I am not familiar enough to see what the underlying issue actually is.

[Fznamznon]

Can you explain why the fix fixes the bug?

Sure, let me try. To be honest, I'm not very familiar with the code either, so I'll describe how I came to the fix. I'll be using added test clang/test/SemaTemplate/gh61159.cpp as example of failing code. The issue happens when instantiating X<int>::impl::f.
So, the original assertion reported by https://github.com/llvm/llvm-project/issues/61159 happens in a function Sema::BuildExpressionFromIntegralTemplateArgument, because it expects non-type integral argument, but receives type argument:

clang/lib/Sema/SemaTemplate.cpp
7955        Sema::BuildExpressionFromIntegralTemplateArgument(const TemplateArgument &Arg,
7956                                                          SourceLocation Loc) {       
7957          assert(Arg.getKind() == TemplateArgument::Integral &&                       
7958                 "Operation is only valid for integral template arguments");

(gdb) p Arg.dump()
int  // that is type argument `int` from outer specialization of struct X.

So, let's see where the argument comes from, for this I went two frames of back upper, to function TemplateInstantiator::TransformTemplateParmRefExpr:

clang/lib/Sema/SemaTemplateInstantiate.cpp
1789          TemplateArgument Arg = TemplateArgs(NTTP->getDepth(), NTTP->getPosition());

NTTP is a NonTypeTemplateParmDecl that represents template parameter ct of function f
(gdb) p NTTP->dump()
NonTypeTemplateParmDecl 0x14b76380 <t.cpp:8:14, col:18> col:18 referenced 'int' depth 0 index 0 ct
TemplateArgs is a MultiLevelTemplateArgumentList - data structure containing arguments for current instantiation 
(gdb) p TemplateArgs.dump()
NumRetainedOuterLevels: 0
0: <int>
1: <17>
We see it has two arguments - "int" and "17", we should be processing "17" but instead end up with "int". Both have same indexes since there is only one argument possible, so they should have different depth. For "17" we have zero depth.
(gdb) p NTTP->getDepth()
$8 = 0

So, after that I went to the place where NonTypeTemplateParmDecl is created to see why it has depth "0" instead of "1".

(gdb) b NonTypeTemplateParmDecl::Create
(gdb) up several times to see where Depth parameters come from

and it is here:
clang/lib/Parse/ParseTemplate.cpp

130             if (ParseTemplateParameters(TemplateParamScopes,                    
131                                         CurTemplateDepthTracker.getDepth(),     
132                                         TemplateParams, LAngleLoc, RAngleLoc)) {
133               // Skip until the semi-colon or a '}'.                            
134               SkipUntil(tok::r_brace, StopAtSemi | StopBeforeMatch);            
135               TryConsumeToken(tok::semi);                                       
136               return nullptr;                                                   
137             }                                                                                                                                    

CurTemplateDepthTracker.getDepth() is increased only if template parameter list of the declaration being parsed is not empty, i.e. when declaration we have is not an explicit specialization:

139             ExprResult OptionalRequiresClauseConstraintER;
140             if (!TemplateParams.empty()) {                
141               isSpecialization = false;                   
142               ++CurTemplateDepthTracker;                  


But this is exactly what we have! So we end up having zero depth for our NTTP.

I tried increasing depth unconditionally but it broke handful of tests and seems zero depth for cases like this is on purpose so that is not the solution. So I figured similar test case which doesn't crash:

template <>
struct X<int> {
  struct impl {
        template<int ct>
        int f() { return ct; };
 };
};
}

And went through the clang code that was failing for the original example:

7954        ExprResult                                                                     
7955        Sema::BuildExpressionFromIntegralTemplateArgument(const TemplateArgument &Arg, 
7956                                                          SourceLocation Loc) {        
7957          assert(Arg.getKind() == TemplateArgument::Integral &&                 

(gdb) p Arg.dump()
17 // correct!

Let's go see corresponding NTTP and MultiLevelTemplateArgumentList :

clang/lib/Sema/SemaTemplateInstantiate.cpp
1789          TemplateArgument Arg = TemplateArgs(NTTP->getDepth(), NTTP->getPosition());

(gdb) p NTTP->dump()
NonTypeTemplateParmDecl 0x14b761b8 <t1.cpp:8:11, col:15> col:15 referenced 'int' depth 0 index 0 ct // all same

(gdb) p TemplateArgs.dump()
NumRetainedOuterLevels: 0
0: <17>

TemplateArgs doesn't have "int" paramter at all so all is correct.

So, after that I went to see where TemplateArgs is formed. For this I went several stack frames and came to

clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
5075            MultiLevelTemplateArgumentList TemplateArgs = getTemplateInstantiationArgs(
5076                Function, /*Final=*/false, nullptr, false, PatternDecl);

So, what getTemplateInstantiationArgs does, it goes from the declaration we're instantiating X<int>::impl::f to the outer contexts gathering required template arguments, it does so in a loop:

334           while (!CurDecl->isFileContextDecl()) {                                              
 ...                     
343             } else if (const auto *ClassTemplSpec =                                            
344                            dyn_cast<ClassTemplateSpecializationDecl>(CurDecl)) {               
345               R = HandleClassTemplateSpec(ClassTemplSpec, Result,                              
346                                           SkipForSpecialization);                              
347             } else if (const auto *Function = dyn_cast<FunctionDecl>(CurDecl)) {               
348               R = HandleFunction(Function, Result, Pattern, RelativeToPrimary,                 
349                                  ForConstraintInstantiation);                                  
350             } else if (const auto *Rec = dyn_cast<CXXRecordDecl>(CurDecl)) {                   
351               R = HandleRecordDecl(Rec, Result, Context, ForConstraintInstantiation);          
352             } else if (const auto *CSD =                                                       
353                            dyn_cast<ImplicitConceptSpecializationDecl>(CurDecl)) {             
354               R = HandleImplicitConceptSpecializationDecl(CSD, Result);                        
355             } else if (const auto *FTD = dyn_cast<FunctionTemplateDecl>(CurDecl)) {            
356               R = HandleFunctionTemplateDecl(FTD, Result);

So there is a bunch of handlers for each type of declaration met, for non-failing example we go first through HandleFunctionTemplateDecl for f itself and add "17" to the resulting argument list:

(gdb) p Result.dump()
NumRetainedOuterLevels: 0
0: <17>

then on next step CurDecl is impl:

(gdb) p CurDecl->dump()
CXXRecordDecl 0x14b76468 <t1.cpp:9:3, line:12:3> line:9:10 referenced struct impl definition

We go to HandleRecordDecl and add nothing since there is no template arguments.
Next CurDecl is X:

(gdb) p CurDecl->dump()
ClassTemplateSpecializationDecl 0x14b761c8 <t1.cpp:7:1, line:13:1> line:8:8 struct X definition

We go to HandleClassTemplateSpec and add no arguments to the resulting list because what we have is explicit specialization of X:

SemaTemplateInstantiate.cpp

145         // Add template arguments from a class template instantiation.                  
146         Response                                                                        
147         HandleClassTemplateSpec(const ClassTemplateSpecializationDecl *ClassTemplSpec,  
148                                 MultiLevelTemplateArgumentList &Result,                 
149                                 bool SkipForSpecialization) {                           
150           if (!ClassTemplSpec->isClassScopeExplicitSpecialization()) {                  
151             // We're done when we hit an explicit specialization.                       
152             if (ClassTemplSpec->getSpecializationKind() == TSK_ExplicitSpecialization &&
153                 !isa<ClassTemplatePartialSpecializationDecl>(ClassTemplSpec))            // THIS IS TRUE
154               return Response::Done();                                                                           // AND WE EXIT HERE WITHOUT ADDING "INT"

(gdb) p ClassTemplSpec->getSpecializationKind()
$8 = clang::TSK_ExplicitSpecialization

Ok, how the failing case is different? We end up in HandleClassTemplateSpec in the same way, but the difference is when you do

template <>
struct X<int>::impl {

instead of

template <>
struct X<int> {
  struct impl {

specialization of X<int> becomes implicit:

(gdb) p ClassTemplSpec->getSpecializationKind()
$3 = clang::TSK_ImplicitInstantiation

And we don't exit, so adding "int" to resulting template argument list:

clang/lib/Sema/SemaTemplateInstantiate.cpp
156             if (!SkipForSpecialization)                                         
157               Result.addOuterTemplateArguments(                                 
158                   const_cast<ClassTemplateSpecializationDecl *>(ClassTemplSpec),
159                   ClassTemplSpec->getTemplateInstantiationArgs().asArray(),     
160                   /*Final=*/false);             
                               
(gdb) p Result.dump()
NumRetainedOuterLevels: 0
0: <int>
1: <17>

And I think, we shouldn't be adding "int" to the resulting list like we do in non-failing case. I don't think it brings any value to the instantiation, it just breaks the system. So I figured, we probably should exit somehow. And I noticed there is CXXRecordDecl::getMemberSpecializationInfo for the case of a non-template class member of a class template which we have in failing example. And what I understood from looking at it, MemberSpecializationInfo returned by it normally has MSInfo->getTemplateSpecializationKind() = TSK_ExplicitSpecialization if the outer template class was explicitly specialized somehow. So I inserted an early exit when we process impl struct, we don't process X for the failing case and the bug becomes fixed.

[shafik]

Your explanation makes sense, I wish we had some documentation someplace that covered how it is expected to work with some examples with expected results. Maybe we should think about tests that might exercise this code a bit more.

The definition of MultiLevelTemplateArgumentList they have a basic example but that seems to be it.

[Fznamznon]

Rebase, extend the test a bit

[Fznamznon]

Maybe we should think about tests that might exercise this code a bit more.

I tried adding more checks, but the patch fixes a specific case, so I ran out of ideas pretty quickly.
Do you have some examples in mind?
Please note that in order to test new change there should be an instantiation of nested non-template class inside of a templated class.

[aaron.ballman]

LGTM assuming precommit CI finds no surprises. I'm struggling to think of other test cases that would match the changes; if @shafik has tests in mind, we can add them (post-commit if necessary).

[shafik]

LGTM

clang/docs/ReleaseNotes.rst

- Correcly diagnose jumps into statement expressions.
   (`#63682 <https://github.com/llvm/llvm-project/issues/63682>`_)
 - Invalidate BlockDecl with implicit return type, in case any of the return
   value exprs is invalid. Propagating the error info up by replacing BlockExpr
   with a RecoveryExpr. This fixes:
   (`#63863 <https://github.com/llvm/llvm-project/issues/63863>_`)
 - Invalidate BlockDecl with invalid ParmVarDecl. Remove redundant dump of
   BlockDecl's ParmVarDecl
   (`#64005 <https://github.com/llvm/llvm-project/issues/64005>_`)
+- Fix crash on nested templated class with template function call.
+  (`#61159 <https://github.com/llvm/llvm-project/issues/61159>_`)
 
 Bug Fixes to Compiler Builtins
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
 Bug Fixes to Attribute Support
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 - Fixed a bug where attribute annotations on type specifiers (enums, classes,
   structs, unions, and scoped enums) were not properly ignored, resulting in

clang/lib/Sema/SemaTemplateInstantiate.cpp

if (ClassTemplateDecl *ClassTemplate = Rec->getDescribedClassTemplate()) {
     if (ClassTemplate->isMemberSpecialization())
       return Response::Done();
     if (ForConstraintInstantiation)
       Result.addOuterTemplateArguments(const_cast<CXXRecordDecl *>(Rec),
                                        ClassTemplate->getInjectedTemplateArgs(),
                                        /*Final=*/false);
   }
 
+  if (const MemberSpecializationInfo *MSInfo =
+          Rec->getMemberSpecializationInfo())
+    if (MSInfo->getTemplateSpecializationKind() == TSK_ExplicitSpecialization)
+      return Response::Done();
+
   bool IsFriend = Rec->getFriendObjectKind() ||
                   (Rec->getDescribedClassTemplate() &&
                    Rec->getDescribedClassTemplate()->getFriendObjectKind());
   if (ForConstraintInstantiation && IsFriend &&
       Rec->getNonTransparentDeclContext()->isFileContext()) {
     return Response::ChangeDecl(Rec->getLexicalDeclContext());
   }
 

clang/test/SemaTemplate/gh61159.cpp

+// RUN: %clang_cc1 -fsyntax-only -verify -std=c++20 %s
+// expected-no-diagnostics
+
+namespace GH61159 {
+template <typename T> struct X {
+  struct I;
+};
+
+template <> struct X<int>::I {
+  template <int ct> constexpr int f() { return ct; };
+
+  int data = 3;
+};
+
+template <typename T> struct X<T>::I {
+  template <T ct> constexpr T f() { return ct + 1; };
+  T data = 7;
+};
+
+static_assert(X<int>::I{}.f<17>() == 17);
+static_assert(X<int>::I{}.data == 3);
+static_assert(X<short>::I{}.data == 7);
+static_assert(X<short>::I{}.f<18>() == 19);
+
+template <typename T> struct Y {
+  struct I;
+};
+
+template <> struct Y<int> {
+  struct I {
+    template <int ct> constexpr int f() { return ct; };
+    int data = 3;
+  };
+};
+
+static_assert(Y<int>::I{}.f<17>() == 17);
+static_assert(Y<int>::I{}.data == 3);
+
+} // namespace GH61159