[Clang][Sema] Tweak tryCaptureVariable for unevaluated lambdas

[zyn0217]

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

[llvmbot]

@llvm/pr-subscribers-clang

Author: Younan Zhang (zyn0217)

Changes

This patch picks up #78598 with the hope that we can address such crashes in tryCaptureVariable for unevaluated lambdas.

Fixes #88081
Fixes #69307
Fixes #91633
Fixes #90669
Fixes #89496

---

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

5 Files Affected:

• (modified) clang/include/clang/AST/DeclBase.h (+4)
• (modified) clang/lib/Sema/SemaExpr.cpp (+9)
• (modified) clang/lib/Sema/SemaLambda.cpp (+10)
• (modified) clang/lib/Sema/TreeTransform.h (+1-1)
• (modified) clang/test/SemaCXX/lambda-unevaluated.cpp (+136)

diff --git a/clang/include/clang/AST/DeclBase.h b/clang/include/clang/AST/DeclBase.h
index e43e812cd9455..3a311d4c55916 100644
--- a/clang/include/clang/AST/DeclBase.h
+++ b/clang/include/clang/AST/DeclBase.h
@@ -2148,6 +2148,10 @@ class DeclContext {
            getDeclKind() <= Decl::lastRecord;
   }
 
+  bool isRequiresExprBody() const {
+    return getDeclKind() == Decl::RequiresExprBody;
+  }
+
   bool isNamespace() const { return getDeclKind() == Decl::Namespace; }
 
   bool isStdNamespace() const;
diff --git a/clang/lib/Sema/SemaExpr.cpp b/clang/lib/Sema/SemaExpr.cpp
index e6c3fa51d54da..bde4fcec69041 100644
--- a/clang/lib/Sema/SemaExpr.cpp
+++ b/clang/lib/Sema/SemaExpr.cpp
@@ -18831,6 +18831,9 @@ bool Sema::tryCaptureVariable(
   DeclContext *VarDC = Var->getDeclContext();
   DeclContext *DC = CurContext;
 
+  while (DC->isRequiresExprBody())
+    DC = DC->getParent();
+
   // tryCaptureVariable is called every time a DeclRef is formed,
   // it can therefore have non-negigible impact on performances.
   // For local variables and when there is no capturing scope,
@@ -18838,6 +18841,12 @@ bool Sema::tryCaptureVariable(
   if (CapturingFunctionScopes == 0 && (!BuildAndDiagnose || VarDC == DC))
     return true;
 
+  // Exception: We have not entered the function scope yet, but we're
+  // referencing the function parameters e.g. at the end of the function
+  // parameter list.
+  if (isa<ParmVarDecl>(Var) && !VarDC->isFunctionOrMethod())
+    return true;
+
   const auto *VD = dyn_cast<VarDecl>(Var);
   if (VD) {
     if (VD->isInitCapture())
diff --git a/clang/lib/Sema/SemaLambda.cpp b/clang/lib/Sema/SemaLambda.cpp
index 1743afaf15287..009c8b3bc57e5 100644
--- a/clang/lib/Sema/SemaLambda.cpp
+++ b/clang/lib/Sema/SemaLambda.cpp
@@ -1036,6 +1036,7 @@ void Sema::ActOnLambdaExpressionAfterIntroducer(LambdaIntroducer &Intro,
   // be dependent, because there are template parameters in scope.
   CXXRecordDecl::LambdaDependencyKind LambdaDependencyKind =
       CXXRecordDecl::LDK_Unknown;
+
   if (LSI->NumExplicitTemplateParams > 0) {
     Scope *TemplateParamScope = CurScope->getTemplateParamParent();
     assert(TemplateParamScope &&
@@ -1046,6 +1047,15 @@ void Sema::ActOnLambdaExpressionAfterIntroducer(LambdaIntroducer &Intro,
       LambdaDependencyKind = CXXRecordDecl::LDK_AlwaysDependent;
   } else if (CurScope->getTemplateParamParent() != nullptr) {
     LambdaDependencyKind = CXXRecordDecl::LDK_AlwaysDependent;
+  } else if (Scope *P = CurScope->getParent()) {
+    while (P->getEntity() && P->getEntity()->isRequiresExprBody())
+      P = P->getParent();
+    if (P->isFunctionDeclarationScope() &&
+        llvm::any_of(P->decls(), [](Decl *D) {
+          return isa<ParmVarDecl>(D) &&
+                 cast<ParmVarDecl>(D)->getType()->isTemplateTypeParmType();
+        }))
+      LambdaDependencyKind = CXXRecordDecl::LDK_AlwaysDependent;
   }
 
   CXXRecordDecl *Class = createLambdaClosureType(
diff --git a/clang/lib/Sema/TreeTransform.h b/clang/lib/Sema/TreeTransform.h
index c039b95293af2..bff4377e34110 100644
--- a/clang/lib/Sema/TreeTransform.h
+++ b/clang/lib/Sema/TreeTransform.h
@@ -14232,7 +14232,7 @@ TreeTransform<Derived>::TransformLambdaExpr(LambdaExpr *E) {
   // will be deemed as dependent even if there are no dependent template
   // arguments.
   // (A ClassTemplateSpecializationDecl is always a dependent context.)
-  while (DC->getDeclKind() == Decl::Kind::RequiresExprBody)
+  while (DC->isRequiresExprBody())
     DC = DC->getParent();
   if ((getSema().isUnevaluatedContext() ||
        getSema().isConstantEvaluatedContext()) &&
diff --git a/clang/test/SemaCXX/lambda-unevaluated.cpp b/clang/test/SemaCXX/lambda-unevaluated.cpp
index 10d4c2228ec9b..9892825067700 100644
--- a/clang/test/SemaCXX/lambda-unevaluated.cpp
+++ b/clang/test/SemaCXX/lambda-unevaluated.cpp
@@ -189,3 +189,139 @@ void recursive() {
 
 }
 }
+
+namespace GH88081 {
+
+struct S { //#S
+  S(auto value) //#S-ctor
+  requires requires { [&] -> decltype(value) { return 2; }; } {} // #S-requires
+
+  static auto foo(auto value) -> decltype([&]() -> decltype(value) {}()) { return {}; } // #S-foo
+
+  static auto bar(auto value) -> decltype([&] { return value; }()) {
+    return "a"; // #bar-body
+  }
+};
+
+S s("a"); // #use
+// expected-error@#S-requires {{cannot initialize return object of type 'decltype(value)' (aka 'const char *') with an rvalue of type 'int'}}
+// expected-error@#use {{no matching constructor}}
+// expected-note@#S-requires {{substituting into a lambda expression here}}
+// expected-note@#S-requires {{substituting template arguments into constraint expression here}}
+// expected-note@#S-requires {{in instantiation of requirement here}}
+// expected-note@#use {{checking constraint satisfaction for template 'S<const char *>' required here}}
+// expected-note@#use {{requested here}}
+// expected-note-re@#S 2{{candidate constructor {{.*}} not viable}}
+// expected-note@#S-ctor {{constraints not satisfied}}
+// expected-note-re@#S-requires {{because {{.*}} would be invalid}}
+
+void func() {
+  S::foo(42);
+  S::bar("str");
+  S::bar(0.618);
+  // expected-error-re@#bar-body {{cannot initialize return object of type {{.*}} (aka 'double') with an lvalue of type 'const char[2]'}}
+  // expected-note@-2 {{requested here}}
+}
+
+} // namespace GH88081
+
+namespace GH69307 {
+
+constexpr auto ICE() {
+  constexpr auto b = 1;
+  return [=](auto c) -> int
+           requires requires { b + c; }
+  { return 1; };
+};
+
+constexpr auto Ret = ICE()(1);
+
+constexpr auto ICE(auto a) { // template function, not lambda
+  return [=]()
+    requires requires { a; }
+  { return 1; };
+};
+
+} // namespace GH69307
+
+namespace GH91633 {
+
+struct __normal_iterator {};
+
+template <typename _Iterator>
+void operator==(_Iterator __lhs, _Iterator) // expected-note {{declared here}}
+  requires requires { __lhs; };
+
+__normal_iterator finder();
+
+template <typename >
+void findDetail() {
+  auto makeResult = [&](auto foo) -> void {
+    finder() != foo;
+    // expected-error@-1 {{function for rewritten '!=' comparison is not 'bool'}}
+  };
+  makeResult(__normal_iterator{}); // expected-note {{requested here}}
+}
+
+void find() {
+  findDetail<void>(); // expected-note {{requested here}}
+}
+} // namespace GH91633
+
+namespace GH90669 {
+
+struct __normal_iterator {};
+
+struct vector {
+  __normal_iterator begin(); // #begin
+  int end();
+};
+
+template <typename _IteratorR>
+bool operator==(_IteratorR __lhs, int)
+  requires requires { __lhs; }
+{}
+
+template <typename PrepareFunc> void queued_for_each(PrepareFunc prep) {
+  prep(vector{}); //#prep
+}
+
+void scan() {
+  queued_for_each([&](auto ino) -> int { // #queued-for-each
+    for (auto e : ino) { // #for-each
+      // expected-error@#for-each {{cannot increment value of type '__normal_iterator'}}
+      // expected-note-re@#prep {{instantiation {{.*}} requested here}}
+      // expected-note-re@#queued-for-each {{instantiation {{.*}} requested here}}
+      // expected-note@#for-each {{implicit call to 'operator++'}}
+      // expected-note@#begin {{selected 'begin' function}}
+    };
+  });
+}
+} // namespace GH90669
+
+namespace GH89496 {
+template <class Iter> struct RevIter {
+  Iter current;
+  constexpr explicit RevIter(Iter x) : current(x) {}
+  inline constexpr bool operator==(const RevIter<Iter> &other) const
+    requires requires {
+      // { current == other.current } -> std::convertible_to<bool>;
+      { other };
+    }
+  {
+    return true;
+  }
+};
+struct Foo {
+  Foo() {};
+};
+void CrashFunc() {
+  auto lambda = [&](auto from, auto to) -> Foo {
+    (void)(from == to);
+    return Foo();
+  };
+  auto from = RevIter<int *>(nullptr);
+  auto to = RevIter<int *>(nullptr);
+  lambda(from, to);
+}
+} // namespace pr89496

[zyn0217]

Unfortunately, @l1nxy told me privately that he will not take over that patch as anticipated. So I hope I can continue the work.

I'll add a release note and supplement some comments tomorrow.

[zyn0217]

Adding a blank line so I can comment here: I found it interesting that we asserted in line 1033 that LSI->NumExplicitTemplateParams is always 0... so the lines in the first branch of the if below are useless?
@cor3ntin

[cor3ntin]

This looks like dead code from a past refactor, we should remove it
(template parameters are set later, in ActOnLambdaExplicitTemplateParameterList

[zyn0217]

(What's going on with the pre-commit CI? Does it disappear?)

EDIT: I pushed a commit again and it's back.

[erichkeane]

I hope @cor3ntin comes and checks this out, but it generally looks OK to me. 1 oddity I'd like to hear about (why we can't do ActOnLambdaClosureQualifiers in the same place).

[erichkeane]

Is there any good reason to do this in 2 places, rather than just delay the above one to here? @cor3ntin ?

[zyn0217]

Yeah, looking into ActOnLambdaClosureQualifiers, I don't see anything that (currently) depends on lambda parameters. I guess we can combine these two calls? I'd love to open a separate NFC PR for doing so, before landing this patch. :)

@erichkeane I retrospected your comment in #78598 (comment) and I appreciate the diagnostics for non-ODR uses - however that's much like a QoI issue to me and would it be OK to leave it as a follow-up? Let me know what you think and I'll add a FIXME then.

[erichkeane]

I think I'd be ok with this as a followup. But @cor3ntin is the one who knows the most about lambda stuff (particularly parsing, etc), so I'd like to see him approve this.

[cor3ntin]

Yes, we need to set whether the lambda has a mutable keyword as soon as the parameters, if present, are parsed, as that affect the meaning of subsequent id-expressions

When there are no parens, there can be a noexcept clause for example, and parsing that does requires knowing whether the lambda is mutable, so that we can adjust the type of any reference-to-capture.

[zyn0217]

Yes, we need to set whether the lambda has a mutable keyword as soon as the parameters, if present, are parsed, as that affect the meaning of subsequent id-expressions
When there are no parens, there can be a noexcept clause for example, and parsing that does requires knowing whether the lambda is mutable, so that we can adjust the type of any reference-to-capture.

Thanks, that makes sense! We currently set LSI->Mutable in ActOnLambdaClosureQualifiers() , and while I think it's possible to move it out of the function, I'm not opposed to leaving it as-is.

[zyn0217]

@cor3ntin I have reduced the test cases (removed duplicate ones) and hopefully they look better. Can you please take another look? thanks!

[cor3ntin]

I think it makes sense.
I kinda wish there would be a more generic way to skip over requires expressions but I also can't think of other contexts we'd want to skip over, so this looks fine.

Thanks for fixing a large number of issues

[tobim]

It would be great if this fix could be backported.

[cor3ntin]

It would be great if this fix could be backported.

I think it might be too late. cc @tstellar

[zyn0217]

+1 for being too late because 18.1.7 just got rolled out a few days ago (I thought 18.1.6 was the final version of 18.x!), and there's probably no 18.1.8.

[FireBurn]

I've just bisected to the faulty commit and eventaully to this fix

+1 to get this added to 18.1.9 if there ever is one - especially if the Android NDK moves to it - the 27 betas fail for me too

[zyn0217]

+1 to get this added to 18.1.9 if there ever is one - especially if the Android NDK moves to it - the 27 betas fail for me too

Sorry, I was unaware we had a scheduled release of 18.1.8 last week.

@tstellar @cor3ntin do you think this is worth another 18.x release (presumably 18.1.9), though we're near the end of the 19 development cycle? I can issue one if the situation is warranted.