Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[clang][Sema] Avoid guessing unexpanded packs' size in getFullyPackExpandedSize #87768

Merged
merged 2 commits into from
Apr 10, 2024

Conversation

zyn0217
Copy link
Contributor

@zyn0217 zyn0217 commented Apr 5, 2024

There has been an optimization for SizeOfPackExprs since c5452ed, in which
we overlooked a case where the template arguments were not yet
formed into a PackExpansionType at the token annotation stage. This
led to a problem in that a template involving such expressions may
lose its nature of being dependent, causing some false-positive
diagnostics.

Fixes #84220

…pandedSize

There has been an optimization for SizeOfPackExprs since c5452ed, in which
we overlooked a case where the template arguments were not yet
formed into a PackExpansionType at the token annotation stage. This
led to a problem in that a template involving such expressions may
lose its nature of being dependent, causing some false-positive
diagnostics.

Fixes llvm#84220
@zyn0217 zyn0217 marked this pull request as ready for review April 7, 2024 15:31
@llvmbot llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Apr 7, 2024
@llvmbot
Copy link
Collaborator

llvmbot commented Apr 7, 2024

@llvm/pr-subscribers-clang

Author: Younan Zhang (zyn0217)

Changes

There has been an optimization for SizeOfPackExprs since c5452ed, in which
we overlooked a case where the template arguments were not yet
formed into a PackExpansionType at the token annotation stage. This
led to a problem in that a template involving such expressions may
lose its nature of being dependent, causing some false-positive
diagnostics.

Fixes #84220


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

3 Files Affected:

  • (modified) clang/docs/ReleaseNotes.rst (+1)
  • (modified) clang/lib/Sema/SemaTemplateVariadic.cpp (+11)
  • (modified) clang/test/SemaTemplate/alias-templates.cpp (+23)
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 28e8ddb3c41c3e..4501bf3e8fe99f 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -504,6 +504,7 @@ Bug Fixes to C++ Support
 - Fix crash when inheriting from a cv-qualified type. Fixes:
   (`#35603 <https://github.com/llvm/llvm-project/issues/35603>`_)
 - Fix a crash when the using enum declaration uses an anonymous enumeration. Fixes (#GH86790).
+- Handled an edge case in ``getFullyPackExpandedSize`` so that we now avoid a false-positive diagnostic. (#GH84220)
 
 Bug Fixes to AST Handling
 ^^^^^^^^^^^^^^^^^^^^^^^^^
diff --git a/clang/lib/Sema/SemaTemplateVariadic.cpp b/clang/lib/Sema/SemaTemplateVariadic.cpp
index 903fbfd18e779c..4909414c0c78d4 100644
--- a/clang/lib/Sema/SemaTemplateVariadic.cpp
+++ b/clang/lib/Sema/SemaTemplateVariadic.cpp
@@ -1243,6 +1243,17 @@ std::optional<unsigned> Sema::getFullyPackExpandedSize(TemplateArgument Arg) {
     // expanded this pack expansion into the enclosing pack if we could.
     if (Elem.isPackExpansion())
       return std::nullopt;
+    // Don't guess the size of unexpanded packs. The pack within a template
+    // argument may have yet to be of a PackExpansion type before we see the
+    // ellipsis in the annotation stage.
+    //
+    // This doesn't mean we would invalidate the optimization: Arg can be an
+    // unexpanded pack regardless of Elem's dependence. For instance,
+    // A TemplateArgument that contains either a SubstTemplateTypeParmPackType
+    // or SubstNonTypeTemplateParmPackExpr is always considered Unexpanded, but
+    // the underlying TemplateArgument thereof may not.
+    if (Elem.containsUnexpandedParameterPack())
+      return std::nullopt;
   }
   return Pack.pack_size();
 }
diff --git a/clang/test/SemaTemplate/alias-templates.cpp b/clang/test/SemaTemplate/alias-templates.cpp
index 8d7cc6118610a0..ab5cad72faf1b7 100644
--- a/clang/test/SemaTemplate/alias-templates.cpp
+++ b/clang/test/SemaTemplate/alias-templates.cpp
@@ -236,6 +236,29 @@ namespace PR14858 {
   void test_q(int (&a)[5]) { Q<B, B, B>().f<B, B>(&a); }
 }
 
+namespace PR84220 {
+
+template <class...> class list {};
+
+template <int> struct foo_impl {
+  template <class> using f = int;
+};
+
+template <class... xs>
+using foo = typename foo_impl<sizeof...(xs)>::template f<xs...>;
+
+// We call getFullyPackExpandedSize at the annotation stage
+// before parsing the ellipsis next to the foo<xs>. This happens before
+// a PackExpansionType is formed for foo<xs>.
+// getFullyPackExpandedSize shouldn't determine the value here. Otherwise,
+// foo_impl<sizeof...(xs)> would lose its dependency despite the template
+// arguments being unsubstituted.
+template <class... xs> using test = list<foo<xs>...>;
+
+test<int> a;
+
+}
+
 namespace redecl {
   template<typename> using A = int;
   template<typename = void> using A = int;

@zyn0217 zyn0217 merged commit a0651db into llvm:main Apr 10, 2024
4 of 5 checks passed
@zyn0217 zyn0217 deleted the GH84220 branch April 10, 2024 11:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:frontend Language frontend issues, e.g. anything involving "Sema" clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Clang's error on non-pack parameter of alias template in a dependent type
3 participants