[clang] reject to capture variable in `RequiresExprBodyDecl` #78598

HerrCai0907 · 2024-01-18T15:16:07Z

RequiresExprBodyDecl will be in CurContext. Then is will cause mismatch between CurContext and FunctionScopes.
This patch wants to ignore RequiresExprBodyDecl in CurContext.
Fixes: #69307, #76593.

llvmbot · 2024-01-18T15:16:42Z

@llvm/pr-subscribers-clang

Author: Congcong Cai (HerrCai0907)

Changes

Expression in RequiresExprBodyDecl is resolved as constants and should not be captured.
Fixes: #69307, #76593.

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

2 Files Affected:

(modified) clang/lib/Sema/SemaExpr.cpp (+3-1)
(added) clang/test/SemaCXX/requires-expression-with-capture-variable.cpp (+25)

diff --git a/clang/lib/Sema/SemaExpr.cpp b/clang/lib/Sema/SemaExpr.cpp
index 6413a48f809ac9..767fc3787cb22f 100644
--- a/clang/lib/Sema/SemaExpr.cpp
+++ b/clang/lib/Sema/SemaExpr.cpp
@@ -19719,7 +19719,9 @@ bool Sema::tryCaptureVariable(
   // we can bailout early.
   if (CapturingFunctionScopes == 0 && (!BuildAndDiagnose || VarDC == DC))
     return true;
-
+  // Expression in `RequiresExprBodyDecl` should not be captured.
+  if (isa<RequiresExprBodyDecl>(CurContext))
+    return true;
   const auto *VD = dyn_cast<VarDecl>(Var);
   if (VD) {
     if (VD->isInitCapture())
diff --git a/clang/test/SemaCXX/requires-expression-with-capture-variable.cpp b/clang/test/SemaCXX/requires-expression-with-capture-variable.cpp
new file mode 100644
index 00000000000000..d01a54133f6c39
--- /dev/null
+++ b/clang/test/SemaCXX/requires-expression-with-capture-variable.cpp
@@ -0,0 +1,25 @@
+// RUN: %clang -fsyntax-only -std=c++20 -Xclang -verify %s
+
+// expected-no-diagnostics
+
+auto GH69307_Func_1() {
+  constexpr auto b = 1;
+  return [&](auto c) -> int
+           requires requires { b + c; }
+  { return 1; };
+};
+auto GH69307_Func_Ret = GH69307_Func_1()(1);
+
+auto GH69307_Lambda_1 = []() {
+  return [&](auto c) -> int
+           requires requires { c; }
+  { return 1; };
+};
+auto GH69307_Lambda_1_Ret = GH69307_Lambda_1()(1);
+
+auto GH69307_Lambda_2 = [](auto c) {
+  return [&]() -> int
+           requires requires { c; }
+  { return 1; };
+};
+auto GH69307_Lambda_2_Ret = GH69307_Lambda_2(1)();

cor3ntin · 2024-01-18T16:15:37Z

Thanks for the patch!

Requires Expressions should be in an unevaluated context, so we should never try to capture variables they mentioned.

Maybe we need to test the evaluation context in either tryCaptureVariable or NeedToCaptureVariable, rather than a specific handling for requires expressions

https://eel.is/c++draft/expr.prim.lambda.capture#7.sentence-2

WDYT?

@erichkeane

zyn0217 · 2024-01-18T17:08:50Z

I think this also fixes #63845 and the example in #41751 (comment), and even #67260!

Expression in `RequiresExprBodyDecl` is resolved as constants and should not be captured. Fixes: #69307, #76593.

github-actions · 2024-01-20T05:48:24Z

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

HerrCai0907 · 2024-01-20T06:01:52Z

Thanks for the patch!

Requires Expressions should be in an unevaluated context, so we should never try to capture variables they mentioned.

Maybe we need to test the evaluation context in either tryCaptureVariable or NeedToCaptureVariable, rather than a specific handling for requires expressions

https://eel.is/c++draft/expr.prim.lambda.capture#7.sentence-2

WDYT?

@erichkeane

We will use tryCaptureVariable to diagnose -Wunused-lambda-capture. So simply ignoring them in an unevaluated context is not a good idea. I have changed the PR to match this function work even for RequiresExprBodyDecl.

…some-certain-requires-clauses-in-calculateconstraintsatisfaction

HerrCai0907 · 2024-02-04T01:32:52Z

ping @erichkeane @cor3ntin

cor3ntin · 2024-02-04T09:31:03Z

Thanks for the patch!
Requires Expressions should be in an unevaluated context, so we should never try to capture variables they mentioned.
Maybe we need to test the evaluation context in either tryCaptureVariable or NeedToCaptureVariable, rather than a specific handling for requires expressions
https://eel.is/c++draft/expr.prim.lambda.capture#7.sentence-2
WDYT?
@erichkeane

We will use tryCaptureVariable to diagnose -Wunused-lambda-capture. So simply ignoring them in an unevaluated context is not a good idea. I have changed the PR to match this function work even for RequiresExprBodyDecl.

You have an example of that? There may be further bugs there.
id-expression that appear in any unevaluated context should not be captured ( typeid being an exception
https://eel.is/c++draft/expr.prim.lambda.capture#7.sentence-2 )

HerrCai0907 · 2024-02-04T15:11:20Z

You have an example of that? There may be further bugs there.
id-expression that appear in any unevaluated context should not be captured ( typeid being an exception

I think it is from standard, but from user-friendly site. Now clang will hint user some variables is not need to capture in unevaluated context. The current implement is clang will try to capture it and then throw a diagnose and then give up to capture.
https://godbolt.org/z/9jnx93r6e

auto explicit_by_value_unused_decltype = [i] { decltype(i) j = 0; };

erichkeane · 2024-02-05T15:40:20Z

clang/lib/Sema/SemaExpr.cpp

@@ -19706,22 +19706,29 @@ static void buildLambdaCaptureFixit(Sema &Sema, LambdaScopeInfo *LSI,
  }
 }

+static DeclContext *ignoreReuquiresBodyDecl(DeclContext *DC) {


Suggested change

static DeclContext *ignoreReuquiresBodyDecl(DeclContext *DC) {

static DeclContext *ignoreRequiresBodyDecl(DeclContext *DC) {

Name here doesn't match what you're doing here. Perhaps something closer to getParentIgnoringRequiresBodyDecl? Not a great name either, but better I think?

I think inline it will be better since we only call it once.

erichkeane · 2024-02-05T15:42:38Z

clang/test/SemaCXX/requires-expression-with-capture-variable.cpp

+
+auto GH69307_Func_1() {
+  constexpr auto b = 1;
+  return [&](auto c) -> int


Can you do a similar test with NO capture here? To show that they aren't needed.

erichkeane · 2024-02-05T15:44:23Z

clang/test/SemaCXX/warn-unused-lambda-capture-cxx20.cpp

+
+void test() {
+  int i;
+  auto explicit_by_value_unused_requires = [i] (auto) requires requires { i; } {}; // expected-warning{{lambda capture 'i' is not required to be captured for this use}}


It would be PARTICULARLY good if we could 'remember' that it was 'used' in the requires clause so we can add a note that says something like (please consider wording and write better:)), note: use in requires expr doesn't use captures because they are special.

That would be interesting. I will try to do it later in festival.

LYP951018 · 2024-02-09T18:40:14Z

This PR should also be able to fix #63972.

zyn0217 · 2024-02-27T01:52:30Z

Could you please explain why you're closing the PR?

cor3ntin · 2024-04-21T12:58:26Z

If @HerrCai0907 doesn't reply, we should take over
@shafik @erichkeane @Endilll

l1nxy · 2024-04-21T16:21:18Z

@cor3ntin @shafik Hi, I want to take charge of this issue and submit a PR for the fix.

shafik · 2024-05-01T02:19:23Z

@cor3ntin @shafik Hi, I want to take charge of this issue and submit a PR for the fix.

I would open a new PR and reference this one in the summary for completeness. It looks like this one is not going to picked up by the author and so if you can take it over and finish it that would be very much appreciated.

There are a lot of issues that crash similarly and so we likely have plenty of tests to try against a fix :-)

This patch picks up llvm#78598 with the hope that we can address such crashes in tryCaptureVariable for unevaluated lambdas. In addition to tryCaptureVariable, this also contains several other fixes on e.g. lambda parsing/dependencies.

This patch picks up #78598 with the hope that we can address such crashes in `tryCaptureVariable()` for unevaluated lambdas. In addition to `tryCaptureVariable()`, this also contains several other fixes on e.g. lambda parsing/dependencies. Fixes #63845 Fixes #67260 Fixes #69307 Fixes #88081 Fixes #89496 Fixes #90669 Fixes #91633

HerrCai0907 linked an issue Jan 18, 2024 that may be closed by this pull request

[clang] ICE when dealing with some certain requires clauses in calculateConstraintSatisfaction #69307

Closed

llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Jan 18, 2024

HerrCai0907 added 2 commits January 20, 2024 11:36

[clang] reject to capture variable in RequiresExprBodyDecl

8fa3dc4

Expression in `RequiresExprBodyDecl` is resolved as constants and should not be captured. Fixes: #69307, #76593.

another test

a99f16f

HerrCai0907 force-pushed the users/ccc/69307-clang-ice-when-dealing-with-some-certain-requires-clauses-in-calculateconstraintsatisfaction branch from 33db497 to a99f16f Compare January 20, 2024 05:46

format

b27d0ac

HerrCai0907 requested a review from cor3ntin January 20, 2024 05:56

Merge branch 'main' into users/ccc/69307-clang-ice-when-dealing-with-…

402c84a

…some-certain-requires-clauses-in-calculateconstraintsatisfaction

HerrCai0907 requested review from erichkeane and shafik January 26, 2024 11:03

erichkeane reviewed Feb 5, 2024

View reviewed changes

add test

6cda600

HerrCai0907 closed this Feb 27, 2024

HerrCai0907 deleted the users/ccc/69307-clang-ice-when-dealing-with-some-certain-requires-clauses-in-calculateconstraintsatisfaction branch February 27, 2024 01:36

zyn0217 mentioned this pull request Apr 9, 2024

[LLVM-18.1.3] Parser CRASHING on requires expression #88081

Closed

zyn0217 mentioned this pull request Apr 20, 2024

Clang crashes with libc++ vector::reverse_iterator in calculateConstraintSatisfaction #89496

Closed

zyn0217 mentioned this pull request Apr 30, 2024

clang-18 frontend segfault (tryCaptureVariable) #90669

Closed

zyn0217 mentioned this pull request May 23, 2024

[Clang][Sema] Tweak tryCaptureVariable for unevaluated lambdas #93206

Merged

	static DeclContext ignoreReuquiresBodyDecl(DeclContext DC) {
	static DeclContext ignoreRequiresBodyDecl(DeclContext DC) {

[clang] reject to capture variable in RequiresExprBodyDecl #78598

[clang] reject to capture variable in RequiresExprBodyDecl #78598

Uh oh!

Conversation

HerrCai0907 commented Jan 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Jan 18, 2024

Uh oh!

cor3ntin commented Jan 18, 2024

Uh oh!

zyn0217 commented Jan 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Jan 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

HerrCai0907 commented Jan 20, 2024

Uh oh!

HerrCai0907 commented Feb 4, 2024

Uh oh!

cor3ntin commented Feb 4, 2024

Uh oh!

HerrCai0907 commented Feb 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

erichkeane Feb 5, 2024

Choose a reason for hiding this comment

Uh oh!

erichkeane Feb 5, 2024

Choose a reason for hiding this comment

Uh oh!

HerrCai0907 Feb 6, 2024

Choose a reason for hiding this comment

Uh oh!

erichkeane Feb 5, 2024

Choose a reason for hiding this comment

Uh oh!

erichkeane Feb 5, 2024

Choose a reason for hiding this comment

Uh oh!

HerrCai0907 Feb 6, 2024

Choose a reason for hiding this comment

Uh oh!

LYP951018 commented Feb 9, 2024

Uh oh!

zyn0217 commented Feb 27, 2024

Uh oh!

cor3ntin commented Apr 21, 2024

Uh oh!

l1nxy commented Apr 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shafik commented May 1, 2024

Uh oh!

Uh oh!

[clang] reject to capture variable in `RequiresExprBodyDecl` #78598

[clang] reject to capture variable in `RequiresExprBodyDecl` #78598

HerrCai0907 commented Jan 18, 2024 •

edited

Loading

zyn0217 commented Jan 18, 2024 •

edited

Loading

github-actions bot commented Jan 20, 2024 •

edited

Loading

HerrCai0907 commented Feb 4, 2024 •

edited

Loading

l1nxy commented Apr 21, 2024 •

edited

Loading