[Clang] Don't assume unexpanded PackExpansions' size when expanding packs #120380
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
llvmbot
added
clang
|
@llvm/pr-subscribers-clang Author: Younan Zhang (zyn0217) ChangesCheckParameterPacksForExpansion() previously assumed that template arguments don't include PackExpansion types when attempting another pack expansion (i.e. when NumExpansions is present). However, this assumption doesn't hold for type aliases, whose substitution might involve unexpanded packs. This can lead to incorrect diagnostics during substitution because the pack size is not yet determined. To address this, this patch calculates the minimum pack size (ignoring unexpanded PackExpansionTypes) and compares it to the previously expanded size. If the minimum pack size is smaller, then there's still a chance for future substitution to expand it to a correct size, so we don't diagnose it too eagerly. Fixes #61415 Full diff: https://github.com/llvm/llvm-project/pull/120380.diff 4 Files Affected:
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index befa411e882b4c..10ed4cf3c964a1 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -825,6 +825,7 @@ Bug Fixes to C++ Support
- Clang no longer rejects deleting a pointer of incomplete enumeration type. (#GH99278)
- Fixed recognition of ``std::initializer_list`` when it's surrounded with ``extern "C++"`` and exported
out of a module (which is the case e.g. in MSVC's implementation of ``std`` module). (#GH118218)
+- Fixed a pack expansion issue in checking unexpanded parameter sizes. (#GH17042)
Bug Fixes to AST Handling
^^^^^^^^^^^^^^^^^^^^^^^^^
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index 811265151fa0da..762e01dbaa857c 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -5863,10 +5863,10 @@ def err_pack_expansion_without_parameter_packs : Error<
"pack expansion does not contain any unexpanded parameter packs">;
def err_pack_expansion_length_conflict : Error<
"pack expansion contains parameter packs %0 and %1 that have different "
- "lengths (%2 vs. %3)">;
+ "lengths (%2 vs. %select{|at least }3%4))">;
def err_pack_expansion_length_conflict_multilevel : Error<
"pack expansion contains parameter pack %0 that has a different "
- "length (%1 vs. %2) from outer parameter packs">;
+ "length (%1 vs. %select{|at least }2%3) from outer parameter packs">;
def err_pack_expansion_length_conflict_partial : Error<
"pack expansion contains parameter pack %0 that has a different "
"length (at least %1 vs. %2) from outer parameter packs">;
diff --git a/clang/lib/Sema/SemaTemplateVariadic.cpp b/clang/lib/Sema/SemaTemplateVariadic.cpp
index 88a21240e1c800..c8452db6bc9014 100644
--- a/clang/lib/Sema/SemaTemplateVariadic.cpp
+++ b/clang/lib/Sema/SemaTemplateVariadic.cpp
@@ -780,7 +780,7 @@ bool Sema::CheckParameterPacksForExpansion(
}
// Determine the size of this argument pack.
- unsigned NewPackSize;
+ unsigned NewPackSize, PendingPackExpansionSize = 0;
if (IsVarDeclPack) {
// Figure out whether we're instantiating to an argument pack or not.
typedef LocalInstantiationScope::DeclArgumentPack DeclArgumentPack;
@@ -808,7 +808,25 @@ bool Sema::CheckParameterPacksForExpansion(
}
// Determine the size of the argument pack.
- NewPackSize = TemplateArgs(Depth, Index).pack_size();
+ ArrayRef<TemplateArgument> Pack =
+ TemplateArgs(Depth, Index).getPackAsArray();
+ NewPackSize = Pack.size();
+ PendingPackExpansionSize =
+ llvm::count_if(Pack, [](const TemplateArgument &TA) {
+ if (!TA.isPackExpansion())
+ return false;
+
+ if (TA.getKind() == TemplateArgument::Type)
+ return !TA.getAsType()
+ ->getAs<PackExpansionType>()
+ ->getNumExpansions();
+
+ if (TA.getKind() == TemplateArgument::Expression)
+ return !cast<PackExpansionExpr>(TA.getAsExpr())
+ ->getNumExpansions();
+
+ return !TA.getNumTemplateExpansions();
+ });
}
// C++0x [temp.arg.explicit]p9:
@@ -831,7 +849,7 @@ bool Sema::CheckParameterPacksForExpansion(
}
if (!NumExpansions) {
- // The is the first pack we've seen for which we have an argument.
+ // This is the first pack we've seen for which we have an argument.
// Record it.
NumExpansions = NewPackSize;
FirstPack.first = Name;
@@ -841,17 +859,44 @@ bool Sema::CheckParameterPacksForExpansion(
}
if (NewPackSize != *NumExpansions) {
+ // In some cases, we might be handling packs with unexpanded template
+ // arguments. For example, this can occur when substituting into a type
+ // alias declaration that uses its injected template parameters as
+ // arguments:
+ //
+ // template <class... Outer> struct S {
+ // template <class... Inner> using Alias = S<void(Outer, Inner)...>;
+ // };
+ //
+ // Consider an instantiation attempt like 'S<int>::Alias<Pack...>', where
+ // Pack comes from another template parameter. 'S<int>' is first
+ // instantiated, expanding the outer pack 'Outer' to <int>. The alias
+ // declaration is accordingly substituted, leaving the template arguments
+ // as unexpanded
+ // '<Pack...>'.
+ //
+ // Since we have no idea of the size of '<Pack...>' until its expansion,
+ // we shouldn't assume its pack size for validation. However if we are
+ // certain that there are extra arguments beyond unexpanded packs, in
+ // which case the pack size is already larger than the previous expansion,
+ // we can complain that before instantiation.
+ unsigned LeastNewPackSize = NewPackSize - PendingPackExpansionSize;
+ if (PendingPackExpansionSize && LeastNewPackSize <= *NumExpansions) {
+ ShouldExpand = false;
+ continue;
+ }
// C++0x [temp.variadic]p5:
// All of the parameter packs expanded by a pack expansion shall have
// the same number of arguments specified.
if (HaveFirstPack)
Diag(EllipsisLoc, diag::err_pack_expansion_length_conflict)
- << FirstPack.first << Name << *NumExpansions << NewPackSize
+ << FirstPack.first << Name << *NumExpansions
+ << (LeastNewPackSize != NewPackSize) << LeastNewPackSize
<< SourceRange(FirstPack.second) << SourceRange(ParmPack.second);
else
Diag(EllipsisLoc, diag::err_pack_expansion_length_conflict_multilevel)
- << Name << *NumExpansions << NewPackSize
- << SourceRange(ParmPack.second);
+ << Name << *NumExpansions << (LeastNewPackSize != NewPackSize)
+ << LeastNewPackSize << SourceRange(ParmPack.second);
return true;
}
}
diff --git a/clang/test/SemaTemplate/pack-deduction.cpp b/clang/test/SemaTemplate/pack-deduction.cpp
index 28fb127a386441..b3104609994a4e 100644
--- a/clang/test/SemaTemplate/pack-deduction.cpp
+++ b/clang/test/SemaTemplate/pack-deduction.cpp
@@ -199,3 +199,62 @@ constexpr auto baz(Int<foo<T>(T())>... x) -> int { return 1; }
static_assert(baz<Int<1>, Int<2>, Int<3>>(Int<10>(), Int<10>(), Int<10>()) == 1, "");
}
+
+namespace GH17042 {
+
+template <class... Ts> struct X {
+ template <class... Us> using Y = X<void(Ts, Us)...>; // #GH17042_Y
+};
+
+template <class... T>
+using any_pairs_list = X<int, int>::Y<T...>; // #any_pairs_list
+
+template <class... T>
+using any_pairs_list_2 = X<int, int>::Y<>;
+// expected-error@#GH17042_Y {{different length (2 vs. 0)}} \
+// expected-note@-1 {{requested here}}
+
+template <class A, class B, class... P>
+using any_pairs_list_3 = X<int, int>::Y<A, B, P...>; // #any_pairs_list_3
+
+template <class A, class B, class C, class... P>
+using any_pairs_list_4 = X<int, int>::Y<A, B, C, P...>;
+// expected-error@#GH17042_Y {{different length (2 vs. at least 3)}} \
+// expected-note@-1 {{requested here}}
+
+static_assert(__is_same(any_pairs_list<char, char>, X<void(int, char), void(int, char)>), "");
+
+static_assert(!__is_same(any_pairs_list<char, char, char>, X<void(int, char), void(int, char)>), "");
+// expected-error@#GH17042_Y {{different length (2 vs. 3)}} \
+// expected-note@#any_pairs_list {{requested here}} \
+// expected-note@-1 {{requested here}}
+
+static_assert(__is_same(any_pairs_list_3<char, char>, X<void(int, char), void(int, char)>), "");
+
+static_assert(!__is_same(any_pairs_list_3<char, char, float>, X<void(int, char), void(int, char)>), "");
+// expected-error@#GH17042_Y {{different length (2 vs. 3)}} \
+// expected-note@#any_pairs_list_3 {{requested here}} \
+// expected-note@-1 {{requested here}}
+
+namespace TemplateTemplateParameters {
+template <class... T> struct C {};
+
+template <class T, template <class> class... Args1> struct Ttp {
+ template <template <class> class... Args2>
+ using B = C<void(Args1<T>, Args2<T>)...>;
+};
+template <class> struct D {};
+
+template <template <class> class... Args>
+using Alias = Ttp<int, D, D>::B<Args...>;
+}
+
+namespace NTTP {
+template <int... Args1> struct Nttp {
+ template <int... Args2> using B = Nttp<(Args1 + Args2)...>;
+};
+
+template <int... Args> using Alias = Nttp<1, 2, 3>::B<Args...>;
+}
+
+}
|
erichkeane
approved these changes
Dec 18, 2024
hubert-reinterpretcast
added a commit
that referenced
this pull request
Jan 8, 2025
Following #120380, `err_pack_expansion_length_conflict` has one close paren too many. Remove the extra parenthesis.
hstk30-hw
pushed a commit
that referenced
this pull request
Jan 8, 2025
Following #120380, `err_pack_expansion_length_conflict` has one close paren too many. Remove the extra parenthesis.
github-actions bot
pushed a commit
to arm/arm-toolchain
that referenced
this pull request
Jan 10, 2025
Following llvm/llvm-project#120380, `err_pack_expansion_length_conflict` has one close paren too many. Remove the extra parenthesis.
This was referenced Jun 2, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
CheckParameterPacksForExpansion() previously assumed that template arguments don't include PackExpansion types when attempting another pack expansion (i.e. when NumExpansions is present). However, this assumption doesn't hold for type aliases, whose substitution might involve unexpanded packs. This can lead to incorrect diagnostics during substitution because the pack size is not yet determined.
To address this, this patch calculates the minimum pack size (ignoring unexpanded PackExpansionTypes) and compares it to the previously expanded size. If the minimum pack size is smaller, then there's still a chance for future substitution to expand it to a correct size, so we don't diagnose it too eagerly.
Fixes #61415
Fixes #32252
Fixes #17042