[SemaCXX] Properly scope ArgumentPackSubstitutionIndex when expanding base types

[colavitam]

Summary

Resolve the crash in issue #53609. The ArgumentPackSubstitutionIndex
from expanding base types containing parameter packs should not persist
when we check the resulting types. Checking the types may lead to
template substitution in the base type, where the index will no longer
be valid.

Fixes #53609

[colavitam]

Fix comments for new test case.

[colavitam]

Ping

[colavitam]

Ping on this

[RKSimon]

clang/lib/Sema/SemaTemplateInstantiate.cpp

2536

I'm not familiar with this code at all - but we've gone from having a local shadow BaseTypeLoc variable to reusing the BaseTypeLoc declared at line#2510 - is that what we actually want?

[colavitam]

Yes, in my opinion the original shadow was accidental—you can see that every usage of BaseTypeLoc is immediately preceded by an initialization, though sometimes in a more nested scope. When initialized in the parameter pack loop, we skip to the next iteration of the outer loop instead of accessing it in the non-parameter pack path.

[colavitam]

Ping

[colavitam]

Ping. Is there anyone familiar with the template instantiation code I should add as a reviewer?

[colavitam]

Ping.

[aaron.ballman]

Adding Erich as he's been staring at the template instantiation code recently and may have thoughts/opinions. To me, this seems like the correct approach though.

clang/lib/Sema/SemaTemplateInstantiate.cpp

2536

I don't think it matters. After this for loop terminates, we continue the outer loop. That said, this does make the code harder to read, we may want to use a local instead of reusing the outer variable like this.

clang/test/SemaCXX/template-base-class-pack-expansion.cpp

2

Can we test more than just not crashing? (e.g., can we test that the expansion happened properly?) Perhaps an AST dump test, at a minimum?

[erichkeane]

I think this patch makes sense to me. I Do like the ideas of making sure these instantiate properly though.

clang/lib/Sema/SemaTemplateInstantiate.cpp

if (Base.isPackExpansion()) {
                                           NumExpansions)) {
         Invalid = true;
         continue;
       }
 
       // If we should expand this pack expansion now, do so.
       if (ShouldExpand) {
         for (unsigned I = 0; I != *NumExpansions; ++I) {
+          {
             Sema::ArgumentPackSubstitutionIndexRAII SubstIndex(*this, I);
 
-          TypeSourceInfo *BaseTypeLoc = SubstType(Base.getTypeSourceInfo(),
-                                                  TemplateArgs,
-                                              Base.getSourceRange().getBegin(),
-                                                  DeclarationName());
+            BaseTypeLoc =
+                SubstType(Base.getTypeSourceInfo(), TemplateArgs,
+                          Base.getSourceRange().getBegin(), DeclarationName());
+          }
           if (!BaseTypeLoc) {
             Invalid = true;
             continue;
           }
 
           if (CXXBaseSpecifier *InstantiatedBase
                 = CheckBaseSpecifier(Instantiation,
                                      Base.getSourceRange(),

clang/test/SemaCXX/template-base-class-pack-expansion.cpp

+// RUN: %clang_cc1 -fsyntax-only -verify -std=c++17 %s
+// Don't crash (#53609).
+
+// expected-no-diagnostics
+
+template <class, int> struct a;
+
+template <class, class...> struct b;
+template <class x, class... y, y... z>
+struct b<x, a<y, z>...> {};
+
+template <class... x> struct c: b<x>...  {};
+
+c<int> d;