[Sema] Avoid an undesired pack expansion while transforming PackIndexingType

[zyn0217]

A pack indexing type can appear in a larger pack expansion, e.g Pack...[pack_of_indexes]... so we need to temporarily disable substitution of pack elements.

Besides, this patch also fixes an assertion failure in PackIndexingExpr::classify: dependent PackIndexingExprs are always LValues and thus we don't need to consider their IndexExprs.

Fixes #88925

[llvmbot]

@llvm/pr-subscribers-clang

Author: Younan Zhang (zyn0217)

Changes

Fixes #88925

---

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

2 Files Affected:

• (modified) clang/lib/Sema/TreeTransform.h (+3)
• (modified) clang/test/SemaCXX/cxx2c-pack-indexing.cpp (+19)

diff --git a/clang/lib/Sema/TreeTransform.h b/clang/lib/Sema/TreeTransform.h
index 9404be5a46f3f7..abc4a16c004a9f 100644
--- a/clang/lib/Sema/TreeTransform.h
+++ b/clang/lib/Sema/TreeTransform.h
@@ -6649,6 +6649,9 @@ TreeTransform<Derived>::TransformPackIndexingType(TypeLocBuilder &TLB,
     }
   }
 
+  // We may be doing this in the context of expanding the Pattern. Forget that
+  // because it has been handled above.
+  Sema::ArgumentPackSubstitutionIndexRAII SubstIndex(getSema(), -1);
   QualType Result = getDerived().TransformType(TLB, TL.getPatternLoc());
 
   QualType Out = getDerived().RebuildPackIndexingType(
diff --git a/clang/test/SemaCXX/cxx2c-pack-indexing.cpp b/clang/test/SemaCXX/cxx2c-pack-indexing.cpp
index 606715e6aacffd..2fd0dbfed294a5 100644
--- a/clang/test/SemaCXX/cxx2c-pack-indexing.cpp
+++ b/clang/test/SemaCXX/cxx2c-pack-indexing.cpp
@@ -160,3 +160,22 @@ namespace GH88929 {
     using E = P...[0]; // expected-error {{unknown type name 'P'}} \
                        // expected-error {{expected ';' after alias declaration}}
 }
+
+namespace GH88925 {
+template <typename...> struct S {};
+
+template <int...> struct sequence {};
+
+template <typename... Args, int... indices> auto f(sequence<indices...>) {
+  return S<Args...[indices]...>(); // #use
+}
+
+void g() {
+  static_assert(__is_same(decltype(f<int>(sequence<0, 0>())), S<int, int>));
+  static_assert(__is_same(decltype(f<int, long>(sequence<0, 0>())), S<int, int>));
+  static_assert(__is_same(decltype(f<int, long>(sequence<0, 1>())), S<int, long>));
+  f<int, long>(sequence<3>());
+  // expected-error@#use {{invalid index 3 for pack 'Args' of size 2}}}
+  // expected-note-re@-2 {{function template specialization '{{.*}}' requested here}}
+}
+}

[cor3ntin]

Thanks for working on that!
I suspect there is the same problem for pack indexing expressions, could you try to add a test?

Thanks!

[cor3ntin]

I suspect there is the same problem for expressions

template <auto...> struct S2 {};
template <auto... Args, int... indices> auto f(sequence<indices...>) {
  return S2<Args...[indices]...>(); // #use
}

[zyn0217]

Oops, yeah, good catch! Honestly, I have tried one similar example (with a fold expression) but had no luck:

template <auto... Args, int... indices> auto f(sequence<indices...>) {
  return (Args...[indices] + ...);
}

Let me see what's happening here.

[zyn0217]

template <auto...> struct S2 {};
template <auto... Args, int... indices> auto f(sequence<indices...>) {
 return S2<Args...[indices]...>(); // #use
}

OK, so the problem here is not the same: we end up classifying the PackIndexingExpr's category while deducing against the NTTP of S2. This happens before the instantiation of f, and therefore Args...[indices]... is still dependent.

I guess we can perceive dependent PackIndexingExprs as LValues in such cases, like what we did for UnresolvedLookupExpr. Additionally, P2662 now limits them to id-expressions only, and hence we can probably always treat them as LValues?

[cor3ntin]

I guess we can perceive dependent PackIndexingExprs as LValues in such cases, like what we did for UnresolvedLookupExpr. Additionally, P2662 now limits them to id-expressions only, and hence we can probably always treat them as LValues?

Yes, they are always LValues for now. So we might want to do that indeed

[zyn0217]

I realized those expressions could be PRValues after an instantiation because in PackIndexingExpr we have:

if (!isInstantiationDependent())
  setValueKind(getSelectedExpr()->getValueKind());

So, I'm going to make it always an LValue iff it's dependent.

[zyn0217]

@cor3ntin Mind looking at it again? Thanks so much!

[cor3ntin]

LGTM, thanks!

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.