Skip to content

PR for llvm/llvm-project#79568 #80120

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Feb 7, 2024
Merged

Conversation

zyn0217
Copy link
Contributor

@zyn0217 zyn0217 commented Jan 31, 2024

Backporting #79568 to clang 18.

Verified

This commit was signed with the committer’s verified signature.
…nside a constraint scope (llvm#79568)

We preserve the trailing requires-expression during the lambda
expression transformation. In order to get those referenced parameters
inside a requires-expression properly resolved to the instantiated
decls, we intended to inject these 'original' `ParmVarDecls` to the
current instantiaion scope, at `Sema::SetupConstraintScope`.

The previous approach seems to overlook nested instantiation chains,
leading to the crash within a nested lambda followed by a requires
clause.

This fixes llvm#73418.
@zyn0217 zyn0217 added clang:frontend Language frontend issues, e.g. anything involving "Sema" release:backport labels Jan 31, 2024
@zyn0217 zyn0217 added this to the LLVM 18.X Release milestone Jan 31, 2024
@zyn0217 zyn0217 requested a review from erichkeane January 31, 2024 08:51
@llvmbot llvmbot added the clang Clang issues not falling into any other category label Jan 31, 2024
@llvmbot
Copy link
Member

llvmbot commented Jan 31, 2024

@llvm/pr-subscribers-clang

Author: Younan Zhang (zyn0217)

Changes

Backporting #79568 to clang 18.


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

3 Files Affected:

  • (modified) clang/docs/ReleaseNotes.rst (+4)
  • (modified) clang/lib/Sema/SemaConcept.cpp (+6-2)
  • (modified) clang/test/SemaTemplate/concepts-lambda.cpp (+18)
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 060bc7669b72a..da52d5ac4d3c6 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -1054,6 +1054,10 @@ Bug Fixes to C++ Support
   Fixes (`#78830 <https://github.com/llvm/llvm-project/issues/78830>`_)
   Fixes (`#60085 <https://github.com/llvm/llvm-project/issues/60085>`_)
 
+- Fixed a bug where variables referenced by requires-clauses inside
+  nested generic lambdas were not properly injected into the constraint scope.
+  (`#73418 <https://github.com/llvm/llvm-project/issues/73418>`_)
+
 Bug Fixes to AST Handling
 ^^^^^^^^^^^^^^^^^^^^^^^^^
 - Fixed an import failure of recursive friend class template.
diff --git a/clang/lib/Sema/SemaConcept.cpp b/clang/lib/Sema/SemaConcept.cpp
index acfc00f412540..88fc846c89e42 100644
--- a/clang/lib/Sema/SemaConcept.cpp
+++ b/clang/lib/Sema/SemaConcept.cpp
@@ -612,8 +612,12 @@ bool Sema::SetupConstraintScope(
 
     // If this is a member function, make sure we get the parameters that
     // reference the original primary template.
-    if (const auto *FromMemTempl =
-            PrimaryTemplate->getInstantiatedFromMemberTemplate()) {
+    // We walk up the instantiated template chain so that nested lambdas get
+    // handled properly.
+    for (FunctionTemplateDecl *FromMemTempl =
+             PrimaryTemplate->getInstantiatedFromMemberTemplate();
+         FromMemTempl;
+         FromMemTempl = FromMemTempl->getInstantiatedFromMemberTemplate()) {
       if (addInstantiatedParametersToScope(FD, FromMemTempl->getTemplatedDecl(),
                                            Scope, MLTAL))
         return true;
diff --git a/clang/test/SemaTemplate/concepts-lambda.cpp b/clang/test/SemaTemplate/concepts-lambda.cpp
index 7e431529427df..0b7580f91043c 100644
--- a/clang/test/SemaTemplate/concepts-lambda.cpp
+++ b/clang/test/SemaTemplate/concepts-lambda.cpp
@@ -149,3 +149,21 @@ void foo() {
   auto caller = make_caller.operator()<&S1::f1>();
 }
 } // namespace ReturnTypeRequirementInLambda
+
+namespace GH73418 {
+void foo() {
+  int x;
+  [&x](auto) {
+    return [](auto y) {
+      return [](auto obj, auto... params)
+        requires requires {
+          sizeof...(params);
+          [](auto... pack) {
+            return sizeof...(pack);
+          }(params...);
+        }
+      { return false; }(y);
+    }(x);
+  }(x);
+}
+} // namespace GH73418

@erichkeane
Copy link
Collaborator

Does this fix a regression against 17? I didn't think it did?

@zyn0217
Copy link
Contributor Author

zyn0217 commented Jan 31, 2024

I think this is a regression from clang 16. https://cpp1.godbolt.org/z/1PnWbvY1r

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@zyn0217
Copy link
Contributor Author

zyn0217 commented Feb 6, 2024

@tstellar Would you mind merging this PR? Thank you in advance!

@tstellar tstellar merged commit d0e9101 into llvm:release/18.x Feb 7, 2024
tstellar pushed a commit to tstellar/llvm-project that referenced this pull request Feb 14, 2024

Verified

This commit was signed with the committer’s verified signature.
Backporting llvm#79568 to clang 18.
tstellar pushed a commit to tstellar/llvm-project that referenced this pull request Feb 14, 2024

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
Backporting llvm#79568 to clang 18.
tstellar pushed a commit to tstellar/llvm-project that referenced this pull request Feb 14, 2024

Unverified

This user has not yet uploaded their public signing key.
Backporting llvm#79568 to clang 18.
tstellar pushed a commit to tstellar/llvm-project that referenced this pull request Feb 14, 2024
Backporting llvm#79568 to clang 18.
@pointhex pointhex mentioned this pull request May 7, 2024
xgupta pushed a commit to xgupta/llvm-project that referenced this pull request Sep 9, 2024
Backporting llvm#79568 to clang 18.
xgupta pushed a commit to xgupta/llvm-project that referenced this pull request Oct 10, 2024
Backporting llvm#79568 to clang 18.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:frontend Language frontend issues, e.g. anything involving "Sema" clang Clang issues not falling into any other category release:backport
Projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants