[Clang] Fix concept checking for VLA types with sugared constraint substitution

[zyn0217]

Our concept-evaluation-on-normalized-constraint patch relies on sugared constraint substitution to properly work. However, it turns out that the sugar also includes non-dependent VLA types, leading to unnecessary/incorrect non-dependent Decl transform.

We avoid this by skipping substitution into non-dependent SubstTTP types.

This also removes a leftover debugging artifact in TransformTemplateArguments from that large patch.

Fixes #102353

This is a regression on trunk so there's no release note.

[llvmbot]

@llvm/pr-subscribers-clang

Author: Younan Zhang (zyn0217)

Changes

Our concept-evaluation-on-normalized-constraint patch relies on sugared constraint substitution to properly work. However, it turns out that the sugar also includes non-dependent VLA types, leading to unnecessary/incorrect non-dependent Decl transform.

We avoid this by skipping substitution into non-dependent SubstTTP types.

This also removes a leftover debugging artifact in TransformTemplateArguments from that large patch.

Fixes #102353

This is a regression on trunk so there's no release note.

---

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

2 Files Affected:

• (modified) clang/lib/Sema/TreeTransform.h (+5-5)
• (modified) clang/test/SemaTemplate/concepts.cpp (+34)

diff --git a/clang/lib/Sema/TreeTransform.h b/clang/lib/Sema/TreeTransform.h
index 940324bbc5e4d..bd21f35178b3b 100644
--- a/clang/lib/Sema/TreeTransform.h
+++ b/clang/lib/Sema/TreeTransform.h
@@ -5107,12 +5107,9 @@ bool TreeTransform<Derived>::TransformTemplateArguments(
                                                 TemplateArgument::pack_iterator>
         PackLocIterator;
 
-      TemplateArgumentListInfo *PackOutput = &Outputs;
-      TemplateArgumentListInfo New;
-
       if (TransformTemplateArguments(
               PackLocIterator(*this, In.getArgument().pack_begin()),
-              PackLocIterator(*this, In.getArgument().pack_end()), *PackOutput,
+              PackLocIterator(*this, In.getArgument().pack_end()), Outputs,
               Uneval))
         return true;
 
@@ -7220,7 +7217,10 @@ QualType TreeTransform<Derived>::TransformSubstTemplateTypeParmType(
   // that needs to be transformed. This only tends to occur with default
   // template arguments of template template parameters.
   TemporaryBase Rebase(*this, TL.getNameLoc(), DeclarationName());
-  QualType Replacement = getDerived().TransformType(T->getReplacementType());
+  QualType Replacement =
+      T->getReplacementType()->isDependentType()
+          ? getDerived().TransformType(T->getReplacementType())
+          : T->getReplacementType();
   if (Replacement.isNull())
     return QualType();
 
diff --git a/clang/test/SemaTemplate/concepts.cpp b/clang/test/SemaTemplate/concepts.cpp
index 3fbe7c0ac650f..d68b8fa432453 100644
--- a/clang/test/SemaTemplate/concepts.cpp
+++ b/clang/test/SemaTemplate/concepts.cpp
@@ -1505,3 +1505,37 @@ namespace GH162770 {
   template<typename... Ts> auto comma = (..., Ts());
   auto b = comma<check<e{}>>;
 } // namespace GH162770
+
+namespace GH102353 {
+
+template <typename _Tp, typename>
+concept same_as = true;
+
+template <typename _Lhs>
+concept assignable_from = requires {
+  { 0 } -> same_as<_Lhs>;
+};
+
+template <typename _Iter>
+struct span {
+  _Iter iter;
+};
+
+template <assignable_from _Iter>
+span(_Iter) -> span<_Iter>;
+
+template <class T, T value>
+void doHQScale() {
+  int srcWidth = value; // expected-note 2{{declared here}}
+  // We shouldn't crash with VLA types, which would appear when
+  // substituting constraint expressions, as they are part of the sugars now.
+  int edgeBuf_storage[srcWidth]; // expected-warning 2{{variable length arrays}} \
+                                 // expected-note 2{{read of non-const variable 'srcWidth'}}
+  span s(edgeBuf_storage);
+}
+
+void foo() {
+  doHQScale<int, 42>(); // expected-note {{requested here}}
+}
+
+}

[mizvekov]

I don't think this is the way to go, it would be only adding another hack, but in this case for a very minor benefit.

Better to revert the original patch.

The assert this hits is wrong, but this needs #107942 in order to be able to improve that assertion.

[mizvekov]

We need to transform types even if they are only instantiation dependent.