Skip to content

[Clang] Add captures to the instantiation scope for noexcept specifiers #97166

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
Jul 7, 2024

Conversation

zyn0217
Copy link
Contributor

@zyn0217 zyn0217 commented Jun 29, 2024

The noexcept specifiers of dependent lambdas would be transformed and rebuilt, where the map of instantiation should also contain captured variables in case they are used from the noexcept specifier.

I also uncovered another assertion failure while at it. However, I decided to leave it as-is because 1) that doesn't appear to be the case in the release version and 2) fixing that might lead to ABI breakage. Anyhow, the case has been added to the test comment.

Fixes #95735

@zyn0217 zyn0217 requested a review from cor3ntin June 29, 2024 13:46
@llvmbot llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Jun 29, 2024
@llvmbot
Copy link
Member

llvmbot commented Jun 29, 2024

@llvm/pr-subscribers-clang

Author: Younan Zhang (zyn0217)

Changes

The noexcept specifiers of dependent lambdas would be transformed and rebuilt, where the map of instantiation should also contain captured variables in case they are used from the noexcept specifier.

I also uncovered another assertion failure while at it. However, I decided to leave it as-is because 1) that doesn't appear to be the case in the release version and 2) fixing that might lead to ABI breakage. Anyhow, the case has been added to the test comment.

Fixes #95735


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

3 Files Affected:

  • (modified) clang/docs/ReleaseNotes.rst (+1)
  • (modified) clang/lib/Sema/SemaTemplateInstantiateDecl.cpp (+6)
  • (modified) clang/test/SemaTemplate/generic-lambda.cpp (+23)
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index da967fcdda808..8c5bd989c60ce 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -935,6 +935,7 @@ Bug Fixes to C++ Support
 - Fix an assertion failure caused by parsing a lambda used as a default argument for the value of a
   forward-declared class. (#GH93512).
 - Fixed a bug in access checking inside return-type-requirement of compound requirements. (#GH93788).
+- Fixed a bug where references to lambda capture inside a ``noexcept`` specifier were not correctly instantiated. (#GH95735).
 
 Bug Fixes to AST Handling
 ^^^^^^^^^^^^^^^^^^^^^^^^^
diff --git a/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp b/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
index 0681520764d9a..d33db60d78448 100644
--- a/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
+++ b/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
@@ -4719,6 +4719,12 @@ void Sema::InstantiateExceptionSpec(SourceLocation PointOfInstantiation,
     return;
   }
 
+  // The noexcept specification could reference any lambda captures. Ensure
+  // those are added to the LocalInstantiationScope.
+  LambdaScopeForCallOperatorInstantiationRAII PushLambdaCaptures(
+      *this, Decl, TemplateArgs, Scope,
+      /*ShouldAddDeclsFromParentScope=*/false);
+
   SubstExceptionSpec(Decl, Template->getType()->castAs<FunctionProtoType>(),
                      TemplateArgs);
 }
diff --git a/clang/test/SemaTemplate/generic-lambda.cpp b/clang/test/SemaTemplate/generic-lambda.cpp
index fb5fa09ebcc1f..804eeaa29d6a1 100644
--- a/clang/test/SemaTemplate/generic-lambda.cpp
+++ b/clang/test/SemaTemplate/generic-lambda.cpp
@@ -60,3 +60,26 @@ template<class T1> C1<X<X<T1>>> auto t3() {
 template C1<X<X<int>>> auto t3<int>();
 static_assert(is_same<decltype(t3<int>()), X<X<X<int>>>>);
 #endif
+
+namespace GH95735 {
+
+int g(int fn) {
+  return [f = fn](auto tpl) noexcept(noexcept(f)) { return f; }(0);
+}
+
+int foo(auto... fn) {
+  // FIXME: This one hits the assertion "if the exception specification is dependent,
+  // then the noexcept expression should be value-dependent" in the constructor of
+  // FunctionProtoType.
+  // One possible solution is to update Sema::canThrow() to consider expressions
+  // (e.g. DeclRefExpr/FunctionParmPackExpr) involving unexpanded parameters as Dependent.
+  // This would effectively add an extra value-dependent flag to the noexcept expression.
+  // However, I'm afraid that would also cause ABI breakage.
+  // [...f = fn](auto tpl) noexcept(noexcept(f)) { return 0; }(0);
+  [...f = fn](auto tpl) noexcept(noexcept(g(fn...))) { return 0; }(0);
+  return [...f = fn](auto tpl) noexcept(noexcept(g(f...))) { return 0; }(0);
+}
+
+int v = foo(42);
+
+} // namespace GH95735

// (e.g. DeclRefExpr/FunctionParmPackExpr) involving unexpanded parameters as Dependent.
// This would effectively add an extra value-dependent flag to the noexcept expression.
// However, I'm afraid that would also cause ABI breakage.
// [...f = fn](auto tpl) noexcept(noexcept(f)) { return 0; }(0);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we missing an unexpanded pack check here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have that check: with a non-assertion build, we would correctly diagnose against the unexpanded pack here.
However, that check happens after we form a FunctionProtoType, where an assertion would have been encountered.

Copy link
Contributor

@cor3ntin cor3ntin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!
I think your fixme can be resolved in a follow up PR

@zyn0217 zyn0217 merged commit f4c7811 into llvm:main Jul 7, 2024
5 of 7 checks passed
@zyn0217
Copy link
Contributor Author

zyn0217 commented Jul 7, 2024

Thanks for the review.

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Clang]: clang rejects valid codes when it trying capturing something never occuring in lambda body
3 participants