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

[zyn0217]

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

Fixes #86361.

[llvmbot]

@llvm/pr-subscribers-clang

Author: Younan Zhang (zyn0217)

Changes

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

Fixes #86361.

---

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

3 Files Affected:

• (modified) clang/docs/ReleaseNotes.rst (+2)
• (modified) clang/lib/Sema/SemaTemplateVariadic.cpp (+1-1)
• (modified) clang/test/SemaCXX/pr61460.cpp (+1)

diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 8054d90fc70f93..1bf7d7fcda2a4a 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -439,6 +439,8 @@ Bug Fixes to C++ Support
 - Fix a crash when instantiating a lambda that captures ``this`` outside of its context. Fixes (#GH85343).
 - Fix an issue where a namespace alias could be defined using a qualified name (all name components
   following the first `::` were ignored).
+- A follow-up fix was added for (#GH61460), where the fix previously overlooked the parentheses
+  around the expression. (#GH86361)
 
 Bug Fixes to AST Handling
 ^^^^^^^^^^^^^^^^^^^^^^^^^
diff --git a/clang/lib/Sema/SemaTemplateVariadic.cpp b/clang/lib/Sema/SemaTemplateVariadic.cpp
index 903fbfd18e779c..1c4a09bca4e3ff 100644
--- a/clang/lib/Sema/SemaTemplateVariadic.cpp
+++ b/clang/lib/Sema/SemaTemplateVariadic.cpp
@@ -415,7 +415,7 @@ bool Sema::DiagnoseUnexpandedParameterPack(Expr *E,
   // 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.
-  if (isa<FunctionParmPackExpr>(E) && getEnclosingLambda())
+  if (isa<FunctionParmPackExpr>(E->IgnoreParens()) && getEnclosingLambda())
     return false;
 
   SmallVector<UnexpandedParameterPack, 2> Unexpanded;
diff --git a/clang/test/SemaCXX/pr61460.cpp b/clang/test/SemaCXX/pr61460.cpp
index 471b1b39d23c2b..44a11c30b95b59 100644
--- a/clang/test/SemaCXX/pr61460.cpp
+++ b/clang/test/SemaCXX/pr61460.cpp
@@ -2,6 +2,7 @@
 
 template <typename... Ts> void g(Ts... p1s) {
   (void)[&](auto... p2s) { ([&] { p1s; p2s; }, ...); };
+  (void)[&](auto... p2s) { ([&] { (p1s); p2s; }, ...); };
 }
 
 void f1() {

[github-actions]

✅ With the latest revision this PR passed the Python code formatter.

[github-actions]

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

[dwblaikie]

LGTM, seems consistent with the previous patch - thanks!

[dwblaikie]

Hmm, actually - does this fix address /other/ ways a pack could appear, like this? https://godbolt.org/z/oez8TbGqM

Presumably a pack could appear in a variety of expressions, not just wrapped in parens - could be in a function call (as in the above example), or nested arbitrarily more deeply in the expression in any number of other expressions?

[zyn0217]

Hmm, actually - does this fix address /other/ ways a pack could appear, like this? https://godbolt.org/z/oez8TbGqM

Presumably a pack could appear in a variety of expressions, not just wrapped in parens - could be in a function call (as in the above example), or nested arbitrarily more deeply in the expression in any number of other expressions?

It fixes it as well, although with an unused-expression warning.

warning: expression result unused [-Wunused-value]
    7 |   [&](auto... indexes) {
      |   ^~~~~~~~~~~~~~~~~~~~~~
    8 |     f1([&] {
      |     ~~~~~~~~
    9 |         (ts);
      |         ~~~~~
   10 |         indexes;
      |         ~~~~~~~~
   11 |       } ...);
      |       ~~~~~~~
   12 |   };
      |   ~

Aside: it crashes again if you turn the capture of the inner lambda to a pack, e.g. ts - that is a different story and is being tracked in #18873, which indicates that the capture of packs is still broken as of now.

[zyn0217]

@dwblaikie Feel free to checkout this patch locally and see if it resolves the original issue - I won't merge it until you confirm it works or discover another issue that goes beyond the scope of this patch. (e.g. another aforementioned issue)

[dwblaikie]

@dwblaikie Feel free to checkout this patch locally and see if it resolves the original issue - I won't merge it until you confirm it works or discover another issue that goes beyond the scope of this patch. (e.g. another aforementioned issue)

First glance it seems it does not resolve the original issue (still seeing a crash, with this patch applied, on the original source). I'll try to reduce another test case.

Hmm, actually - does this fix address /other/ ways a pack could appear, like this? https://godbolt.org/z/oez8TbGqM
Presumably a pack could appear in a variety of expressions, not just wrapped in parens - could be in a function call (as in the above example), or nested arbitrarily more deeply in the expression in any number of other expressions?

It fixes it as well, although with an unused-expression warning.

Oh, that surprises me - not sure how ignoring parens could help the case where it's a function call...

Oh, I was looking at the wrong parens, it's the line 9 (ts) parens. OK, change /that/ into a function call (and this time I've actually applied this patch locally and tested this, and it does seem to still crash, even with this patch applied):

template<typename... Ts>
void f1(Ts... ts);
template <typename... Ts>
void f1(Ts... ts) {
  [&](auto... indexes) {
    ([&] {
        f1(ts);
        indexes;
      }, ...);
  };
}
void f2() { f1(); }

But perhaps this ^ is what you're referring to / when you mention "if you turn the capture of the inner lambda to a pack, e.g. ts"

Aside: it crashes again if you turn the capture of the inner lambda to a pack, e.g. ts - that is a different story and is being tracked in #18873, which indicates that the capture of packs is still broken as of now.

But is it a different story? Whet I run the above example, with this patch applied, I get an identical stack trace:

...
#12 0x00005637d1f3b0f1 clang::Sema::DiagnoseUnexpandedParameterPack(clang::Expr*, clang::Sema::UnexpandedParameterPackContext) 
#13 0x00005637d155c166 clang::Sema::ActOnFinishFullExpr(clang::Expr*, clang::SourceLocation, bool, bool, bool) 
#14 0x00005637d19cf202 clang::Sema::ActOnExprStmt(clang::ActionResult<clang::Expr*, true>, bool) 
#15 0x00005637d1e352f8 clang::TreeTransform<(anonymous namespace)::TemplateInstantiator>::TransformStmt(clang::Stmt*, clang::TreeTransform<(anonymous namespace)::TemplateIn
stantiator>::StmtDiscardKind)
...

[dwblaikie]

Yep, the original code still crashes with this PR applied, and the reduced test case comes down to something identical to the code shown in #86401 (comment) with a stack trace that looks the same as the one caused by the test case this patch is addressing.

I tend to think this patch is insufficiently general - that there's any number of ways an unexpanded pack could appear inside an expression and ignoring parens is only one fairly narrow case of a broader issue?

[zyn0217]

@dwblaikie : You're right. The patch is insufficient because the FunctionParmPackExpr in your example could also be wrapped with e.g.

CallExpr 0x5555649b0cf0 '<dependent type>'
|-UnresolvedLookupExpr 0x5555649af8f8 '<overloaded function type>' lvalue (ADL) = 'f1' 0x555564990e98
`-FunctionParmPackExpr 0x5555649af9c0 'Ts...' lvalue

... a CallExpr.

Admittedly, I don't like the way that this and previous fixes have taken: the assertion we have run into really seems unnecessary because the following DiagnoseUnexpandedParameterPacks handles the empty pack cases. In my first attempt, I proposed to remove these assertions, while Corentin worried that they could hide other issues, which I have agreed with because, indeed, we have many bugs that the crash locations are not where the underlying problems lurk.

Either way, I think I'm going to take a deeper look, probably I should think of a more optimal approach instead of adding such "whack a mole" fixes. I will turn it to draft, probably these issues will get fixed together with my #86265 - I'll notify you when everything is ready.

[dwblaikie]

Thanks for continuing to look into this!

@cor3ntin - perhaps you've got some more thoughts on this too?