[clang][Sema] Avoid non-empty unexpanded pack assertion for FunctionParmPackExpr #69224

zyn0217 · 2023-10-16T16:04:35Z

We have FunctionParmPackExpr that serves as the unexpanded expression but from which the visitor collects none, which may lead to assertion failure during the template instantiation.

llvmbot · 2023-10-16T16:05:51Z

@llvm/pr-subscribers-clang

Author: None (zyn0217)

Changes

…ctUnexpandedParameterPacksVisitor

Closes #61460.

We have FunctionParmPackExpr that serves as the unexpanded expression but from which the visitor collects none, which may lead to assertion failure during the template instantiation.

Actually, we don't need to assert this condition since we would do nothing in the subsequent DiagnoseUnexpandedParameterPacks call even if unsatisfied.

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

3 Files Affected:

(modified) clang/docs/ReleaseNotes.rst (+2)
(modified) clang/lib/Sema/SemaTemplateVariadic.cpp (+6-12)
(added) clang/test/SemaCXX/pr61460.cpp (+8)

diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 9782c123f4c9372..0c70ddf0e9a8b1e 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -397,6 +397,8 @@ Bug Fixes in This Version
   operator in C. No longer issuing a confusing diagnostic along the lines of
   "incompatible operand types ('foo' and 'foo')" with extensions such as matrix
   types. Fixes (`#69008 <https://github.com/llvm/llvm-project/issues/69008>`_)
+- Fixed an issue that a benign assertion might hit when instantiating a pack expansion
+  inside a lambda. (`#61460 <https://github.com/llvm/llvm-project/issues/61460>`_)
 
 Bug Fixes to Compiler Builtins
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
diff --git a/clang/lib/Sema/SemaTemplateVariadic.cpp b/clang/lib/Sema/SemaTemplateVariadic.cpp
index dfcc78dafdc4c31..b8912b2ad8a372d 100644
--- a/clang/lib/Sema/SemaTemplateVariadic.cpp
+++ b/clang/lib/Sema/SemaTemplateVariadic.cpp
@@ -388,9 +388,8 @@ bool Sema::DiagnoseUnexpandedParameterPack(SourceLocation Loc,
     return false;
 
   SmallVector<UnexpandedParameterPack, 2> Unexpanded;
-  CollectUnexpandedParameterPacksVisitor(Unexpanded).TraverseTypeLoc(
-                                                              T->getTypeLoc());
-  assert(!Unexpanded.empty() && "Unable to find unexpanded parameter packs");
+  CollectUnexpandedParameterPacksVisitor(Unexpanded)
+      .TraverseTypeLoc(T->getTypeLoc());
   return DiagnoseUnexpandedParameterPacks(Loc, UPPC, Unexpanded);
 }
 
@@ -404,7 +403,6 @@ bool Sema::DiagnoseUnexpandedParameterPack(Expr *E,
 
   SmallVector<UnexpandedParameterPack, 2> Unexpanded;
   CollectUnexpandedParameterPacksVisitor(Unexpanded).TraverseStmt(E);
-  assert(!Unexpanded.empty() && "Unable to find unexpanded parameter packs");
   return DiagnoseUnexpandedParameterPacks(E->getBeginLoc(), UPPC, Unexpanded);
 }
 
@@ -442,8 +440,7 @@ bool Sema::DiagnoseUnexpandedParameterPack(const CXXScopeSpec &SS,
 
   SmallVector<UnexpandedParameterPack, 2> Unexpanded;
   CollectUnexpandedParameterPacksVisitor(Unexpanded)
-    .TraverseNestedNameSpecifier(SS.getScopeRep());
-  assert(!Unexpanded.empty() && "Unable to find unexpanded parameter packs");
+      .TraverseNestedNameSpecifier(SS.getScopeRep());
   return DiagnoseUnexpandedParameterPacks(SS.getRange().getBegin(),
                                           UPPC, Unexpanded);
 }
@@ -479,8 +476,7 @@ bool Sema::DiagnoseUnexpandedParameterPack(const DeclarationNameInfo &NameInfo,
 
   SmallVector<UnexpandedParameterPack, 2> Unexpanded;
   CollectUnexpandedParameterPacksVisitor(Unexpanded)
-    .TraverseType(NameInfo.getName().getCXXNameType());
-  assert(!Unexpanded.empty() && "Unable to find unexpanded parameter packs");
+      .TraverseType(NameInfo.getName().getCXXNameType());
   return DiagnoseUnexpandedParameterPacks(NameInfo.getLoc(), UPPC, Unexpanded);
 }
 
@@ -493,8 +489,7 @@ bool Sema::DiagnoseUnexpandedParameterPack(SourceLocation Loc,
 
   SmallVector<UnexpandedParameterPack, 2> Unexpanded;
   CollectUnexpandedParameterPacksVisitor(Unexpanded)
-    .TraverseTemplateName(Template);
-  assert(!Unexpanded.empty() && "Unable to find unexpanded parameter packs");
+      .TraverseTemplateName(Template);
   return DiagnoseUnexpandedParameterPacks(Loc, UPPC, Unexpanded);
 }
 
@@ -506,8 +501,7 @@ bool Sema::DiagnoseUnexpandedParameterPack(TemplateArgumentLoc Arg,
 
   SmallVector<UnexpandedParameterPack, 2> Unexpanded;
   CollectUnexpandedParameterPacksVisitor(Unexpanded)
-    .TraverseTemplateArgumentLoc(Arg);
-  assert(!Unexpanded.empty() && "Unable to find unexpanded parameter packs");
+      .TraverseTemplateArgumentLoc(Arg);
   return DiagnoseUnexpandedParameterPacks(Arg.getLocation(), UPPC, Unexpanded);
 }
 
diff --git a/clang/test/SemaCXX/pr61460.cpp b/clang/test/SemaCXX/pr61460.cpp
new file mode 100644
index 000000000000000..ee9d1db4bb09486
--- /dev/null
+++ b/clang/test/SemaCXX/pr61460.cpp
@@ -0,0 +1,8 @@
+// RUN: %clang_cc1 -std=c++17 %s -fsyntax-only -verify
+// expected-no-diagnostics
+template <typename... Ts> void g(Ts... p1s) {
+  (void)[&](auto... p2s) { ([&] { p1s; p2s; }, ...); };
+}                                                       
+void f1() {
+  g();
+}

erichkeane · 2023-10-16T16:06:49Z

@cor3ntin : Lambda stuff, I think you're more familiar than I am with this. Mind taking a pass?

cor3ntin · 2023-10-16T16:51:24Z

I think the issue here is that we make the assumption that a full expression cannot contain an unexpanded pack
But that does not hold for lambda.

Maybe instead of removing the assert entirely we could check whether one of the enclosing scope is a lambda.

While I think the change would be correct, i also think the assert is useful and I'm a bit concerned that removing it entirely will create problems down the line.

so i would encouraging researching

assert((!Unexpanded.empty() || enclosedInAnOtherExpression()) && "Unable to find unexpanded parameter packs");

where enclosedInAnOtherExpression would try to check that we are in a lambda - maybe Sema::getEnclosingLambda() is sufficient - not sure though.

Can we run into similar problems with statement expressions ? (I don't think so as they don't have parameter or capture list)

clang/test/SemaCXX/pr61460.cpp

zyn0217 · 2023-10-21T12:35:55Z

Sorry for my late update - I've had a busy week ;)

I don't think so as they don't have parameter or capture list

+1. and it looks like we don't allow expanding a pack outside a statement expression, which more or less prevents the appearance of FunctionParmPackExpr.

so i would encouraging researching
assert((!Unexpanded.empty() || enclosedInAnOtherExpression()) && "Unable to find unexpanded parameter packs");
where enclosedInAnOtherExpression would try to check that we are in a lambda - maybe Sema::getEnclosingLambda() is sufficient - not sure though.

Instead of putting it inside an assertion, how about adding a check for FunctionParmPackExpr separately? I mean,

if (auto *Expr = dyn_cast<FunctionParmPackExpr>(E); Expr && getEnclosingLambda())
  return false;

to bail out only inside the Expr overload of DiagnoseUnexpandedParameterPack. (since it's the one being called anyways during template instantiation.)

cor3ntin · 2023-10-22T07:49:22Z

Instead of putting it inside an assertion, how about adding a check for FunctionParmPackExpr separately? I mean,

If that works it would be fine with me, yes

clang/lib/Sema/SemaTemplateVariadic.cpp

cor3ntin · 2023-10-25T07:40:19Z

Looks generally good to me. can you remove the whitespace only changes? thanks!

zyn0217 · 2023-10-25T07:58:03Z

Done. Sorry for leaving irrelevant changes ;)

AaronBallman · 2023-10-27T15:39:47Z

clang/lib/Sema/SemaTemplateVariadic.cpp

+  // Exception: The `CollectUnexpandedParameterPacksVisitor` collects nothing
+  // from a FunctionParmPackExpr. In the context where the collector is being
+  // used such as `collectUnexpandedParameterPacks`, this type of expression
+  // is not expected to be collected.
+  //
+  // Nonetheless, this function for diagnosis is still called anyway during
+  // template instantiation, with an expression of such a type if we're inside a
+  // lambda with unexpanded parameters.
+  //
+  // Rule out this case to prevent the assertion failure.


Suggested change

// Exception: The `CollectUnexpandedParameterPacksVisitor` collects nothing

// from a FunctionParmPackExpr. In the context where the collector is being

// used such as `collectUnexpandedParameterPacks`, this type of expression

// is not expected to be collected.

//

// Nonetheless, this function for diagnosis is still called anyway during

// template instantiation, with an expression of such a type if we're inside a

// lambda with unexpanded parameters.

//

// Rule out this case to prevent the assertion failure.

// CollectUnexpandedParameterPacksVisitor does not expect to see a FunctionParmPackExpr, but

// diagnosing unexpected parameter packs may still see such an expression in a lambda body. We'll

// bail out early in this case to avoid triggering an assertion.

Slight rewording.

…armPackExpr Closes llvm#61460. We have FunctionParmPackExpr that serves as the unexpanded expression but from which the visitor collects none, which may lead to assertion failure during the template instantiation.

zyn0217 · 2023-10-31T05:32:12Z

Thank you folks for the suggestions! I've updated, PTAL. :-)

cor3ntin

LGTM, thanks

…armPackExpr (llvm#69224) Closes llvm#61460. We have FunctionParmPackExpr that serves as the unexpanded expression but from which the visitor collects none, which may lead to assertion failure during the template instantiation.

…edParameterPack This is a follow-up for llvm#69224, where the previous fix failed to handle the parentheses around the expression. Fixes llvm#86361.

`FunctionParmPackExpr`s are peculiar in that they have to be of unexpanded dependency while they don't introduce any unexpanded packs. So this patch rules them out in the non-empty pack assertion in `DiagnoseUnexpandedParameterPack()`. There was a fix #69224, but that turned out to be insufficient. I also moved the separate tests to a pre-existing file. Fixes #86361

zyn0217 requested review from cor3ntin, shafik and erichkeane October 16, 2023 16:04

llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Oct 16, 2023

zyn0217 mentioned this pull request Oct 16, 2023

Assertion failure with nested parameter packs and lambdas #61460

Closed

cor3ntin reviewed Oct 16, 2023

View reviewed changes

clang/test/SemaCXX/pr61460.cpp Outdated Show resolved Hide resolved

zyn0217 changed the title ~~[clang][Sema] Don't assert non-empty unexpanded packs following Colle…~~ [clang][Sema] Avoid non-empty unexpanded pack assertion for FunctionParmPackExpr Oct 21, 2023

zyn0217 force-pushed the issue-61460 branch from 5ecdf88 to 89e4866 Compare October 21, 2023 13:19

tbaederr reviewed Oct 23, 2023

View reviewed changes

clang/lib/Sema/SemaTemplateVariadic.cpp Outdated Show resolved Hide resolved

zyn0217 force-pushed the issue-61460 branch 2 times, most recently from 697d116 to 8cf9e71 Compare October 24, 2023 02:03

zyn0217 force-pushed the issue-61460 branch from 8cf9e71 to 880f271 Compare October 25, 2023 07:56

AaronBallman reviewed Oct 27, 2023

View reviewed changes

zyn0217 force-pushed the issue-61460 branch from 880f271 to 0d2fd4e Compare October 31, 2023 05:29

cor3ntin approved these changes Nov 11, 2023

View reviewed changes

zyn0217 merged commit 868007a into llvm:main Nov 13, 2023

zyn0217 mentioned this pull request Mar 23, 2024

[clang][Sema] Ignore the parentheses in the guard of DiagnoseUnexpandedParameterPack #86401

Closed

zyn0217 mentioned this pull request Sep 6, 2024

[Clang] Don't assert non-empty packs for FunctionParmPackExprs #107561

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[clang][Sema] Avoid non-empty unexpanded pack assertion for FunctionParmPackExpr #69224

[clang][Sema] Avoid non-empty unexpanded pack assertion for FunctionParmPackExpr #69224

Uh oh!

zyn0217 commented Oct 16, 2023 •

edited

Loading

Uh oh!

llvmbot commented Oct 16, 2023

Uh oh!

erichkeane commented Oct 16, 2023

Uh oh!

cor3ntin commented Oct 16, 2023

Uh oh!

Uh oh!

zyn0217 commented Oct 21, 2023

Uh oh!

cor3ntin commented Oct 22, 2023

Uh oh!

Uh oh!

cor3ntin commented Oct 25, 2023

Uh oh!

zyn0217 commented Oct 25, 2023

Uh oh!

AaronBallman Oct 27, 2023

Uh oh!

zyn0217 commented Oct 31, 2023

Uh oh!

cor3ntin left a comment

Uh oh!

Uh oh!

[clang][Sema] Avoid non-empty unexpanded pack assertion for FunctionParmPackExpr #69224

[clang][Sema] Avoid non-empty unexpanded pack assertion for FunctionParmPackExpr #69224

Uh oh!

Conversation

zyn0217 commented Oct 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Oct 16, 2023

Uh oh!

erichkeane commented Oct 16, 2023

Uh oh!

cor3ntin commented Oct 16, 2023

Uh oh!

Uh oh!

zyn0217 commented Oct 21, 2023

Uh oh!

cor3ntin commented Oct 22, 2023

Uh oh!

Uh oh!

cor3ntin commented Oct 25, 2023

Uh oh!

zyn0217 commented Oct 25, 2023

Uh oh!

AaronBallman Oct 27, 2023

Choose a reason for hiding this comment

Uh oh!

zyn0217 commented Oct 31, 2023

Uh oh!

cor3ntin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

zyn0217 commented Oct 16, 2023 •

edited

Loading