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

[David Blaikie via cfe-commits]

dwblaikie wrote:

Thanks for continuing to look into this!

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

https://github.com/llvm/llvm-project/pull/86401
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Younan Zhang via cfe-commits]

https://github.com/zyn0217 converted_to_draft 
https://github.com/llvm/llvm-project/pull/86401
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Younan Zhang via cfe-commits]

zyn0217 wrote:

@dwblaikie : You're right. The patch is insufficient because the 
`FunctionParmPackExpr` in your example could also be wrapped with e.g.
```
CallExpr 0x649b0cf0 ''
|-UnresolvedLookupExpr 0x649af8f8 '' lvalue (ADL) 
= 'f1' 0x64990e98
`-FunctionParmPackExpr 0x649af9c0 '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 
https://github.com/llvm/llvm-project/pull/86265 - I'll notify you when 
everything is ready.


https://github.com/llvm/llvm-project/pull/86401
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[David Blaikie via cfe-commits]

dwblaikie wrote:

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 
https://github.com/llvm/llvm-project/pull/86401#issuecomment-2024151742 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?

https://github.com/llvm/llvm-project/pull/86401
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[David Blaikie via cfe-commits]

dwblaikie wrote:

> @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
void f1(Ts... ts);
template 
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 0x5637d1f3b0f1 
clang::Sema::DiagnoseUnexpandedParameterPack(clang::Expr*, 
clang::Sema::UnexpandedParameterPackContext) 
#13 0x5637d155c166 clang::Sema::ActOnFinishFullExpr(clang::Expr*, 
clang::SourceLocation, bool, bool, bool) 
#14 0x5637d19cf202 
clang::Sema::ActOnExprStmt(clang::ActionResult, bool) 
#15 0x5637d1e352f8 clang::TreeTransform<(anonymous 
namespace)::TemplateInstantiator>::TransformStmt(clang::Stmt*, 
clang::TreeTransform<(anonymous namespace)::TemplateIn
stantiator>::StmtDiscardKind)
...
```





https://github.com/llvm/llvm-project/pull/86401
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Younan Zhang via cfe-commits]

zyn0217 wrote:

@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)

https://github.com/llvm/llvm-project/pull/86401
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Younan Zhang via cfe-commits]

zyn0217 wrote:

> 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.
```cpp
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.

https://github.com/llvm/llvm-project/pull/86401
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[David Blaikie via cfe-commits]

dwblaikie wrote:

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?

https://github.com/llvm/llvm-project/pull/86401
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[David Blaikie via cfe-commits]

https://github.com/dwblaikie approved this pull request.

LGTM, seems consistent with the previous patch - thanks!

https://github.com/llvm/llvm-project/pull/86401
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[via cfe-commits]

github-actions[bot] wrote:



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

https://github.com/llvm/llvm-project/pull/86401
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[via cfe-commits]

github-actions[bot] wrote:



:white_check_mark: With the latest revision this PR passed the Python code 
formatter.

https://github.com/llvm/llvm-project/pull/86401
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[via cfe-commits]

llvmbot wrote:




@llvm/pr-subscribers-clang

Author: Younan Zhang (zyn0217)


Changes

This is a follow-up for https://github.com/llvm/llvm-project/pull/69224, where 
the previous fix failed to handle the parentheses around the expression.

Fixes https://github.com/llvm/llvm-project/issues/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
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(E) && getEnclosingLambda())
+  if (isa(E->IgnoreParens()) && getEnclosingLambda())
 return false;
 
   SmallVector 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  void g(Ts... p1s) {
   (void)[&](auto... p2s) { ([&] { p1s; p2s; }, ...); };
+  (void)[&](auto... p2s) { ([&] { (p1s); p2s; }, ...); };
 }
 
 void f1() {

``




https://github.com/llvm/llvm-project/pull/86401
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Younan Zhang via cfe-commits]

https://github.com/zyn0217 ready_for_review 
https://github.com/llvm/llvm-project/pull/86401
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Younan Zhang via cfe-commits]

https://github.com/zyn0217 updated 
https://github.com/llvm/llvm-project/pull/86401

>From 1a990278196bf9c8753fe318f060f17fb8d0e669 Mon Sep 17 00:00:00 2001
From: Younan Zhang 
Date: Sun, 24 Mar 2024 00:00:31 +0800
Subject: [PATCH] [clang][Sema] Ignore the parentheses in the guard of
 DiagnoseUnexpandedParameterPack

This is a follow-up for https://github.com/llvm/llvm-project/pull/69224,
where the previous fix failed to handle the parentheses around the
expression.

Fixes https://github.com/llvm/llvm-project/issues/86361.
---
 clang/docs/ReleaseNotes.rst | 2 ++
 clang/lib/Sema/SemaTemplateVariadic.cpp | 2 +-
 clang/test/SemaCXX/pr61460.cpp  | 1 +
 3 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 690fc7ed271a3d..9c3dec4d17e022 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -344,6 +344,8 @@ Bug Fixes to C++ Support
   when one of the function had more specialized templates.
   Fixes (`#82509 `_)
   and (`#74494 `_)
+- 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(E) && getEnclosingLambda())
+  if (isa(E->IgnoreParens()) && getEnclosingLambda())
 return false;
 
   SmallVector 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  void g(Ts... p1s) {
   (void)[&](auto... p2s) { ([&] { p1s; p2s; }, ...); };
+  (void)[&](auto... p2s) { ([&] { (p1s); p2s; }, ...); };
 }
 
 void f1() {

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Younan Zhang via cfe-commits]

https://github.com/zyn0217 created 
https://github.com/llvm/llvm-project/pull/86401

This is a follow-up for https://github.com/llvm/llvm-project/pull/69224, where 
the previous fix failed to handle the parentheses around the expression.

Fixes https://github.com/llvm/llvm-project/issues/86361.

>From 1a990278196bf9c8753fe318f060f17fb8d0e669 Mon Sep 17 00:00:00 2001
From: Younan Zhang 
Date: Sun, 24 Mar 2024 00:00:31 +0800
Subject: [PATCH] [clang][Sema] Ignore the parentheses in the guard of
 DiagnoseUnexpandedParameterPack

This is a follow-up for https://github.com/llvm/llvm-project/pull/69224,
where the previous fix failed to handle the parentheses around the
expression.

Fixes https://github.com/llvm/llvm-project/issues/86361.
---
 clang/docs/ReleaseNotes.rst | 2 ++
 clang/lib/Sema/SemaTemplateVariadic.cpp | 2 +-
 clang/test/SemaCXX/pr61460.cpp  | 1 +
 3 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 690fc7ed271a3d..9c3dec4d17e022 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -344,6 +344,8 @@ Bug Fixes to C++ Support
   when one of the function had more specialized templates.
   Fixes (`#82509 `_)
   and (`#74494 `_)
+- 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(E) && getEnclosingLambda())
+  if (isa(E->IgnoreParens()) && getEnclosingLambda())
 return false;
 
   SmallVector 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  void g(Ts... p1s) {
   (void)[&](auto... p2s) { ([&] { p1s; p2s; }, ...); };
+  (void)[&](auto... p2s) { ([&] { (p1s); p2s; }, ...); };
 }
 
 void f1() {

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits