Skip to content

[Clang] Treat default template argument as constant expressions #107073

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 5 commits into from
Sep 4, 2024

Conversation

cor3ntin
Copy link
Contributor

@cor3ntin cor3ntin commented Sep 3, 2024

We only check that a default argument is a converted constant expression when using the default argument.

However, when parsing a default argument, we need to make sure to parse it as a constant expression such as not ODR-use variables. (otherwise, we would try to capture default template arguments of generic lambdas)

Fixes #107048

We only check that a default argument is a converted constant
expression when using the default argument.

However, when parsing a default argument, we need to make sure
to parse it as a constant expression such as not ODR-use variables.
(otherwise, we would try to capture default template arguments
of generic lambdas)

Fixes llvm#107048
@llvmbot llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Sep 3, 2024
@llvmbot
Copy link
Member

llvmbot commented Sep 3, 2024

@llvm/pr-subscribers-clang

Author: cor3ntin (cor3ntin)

Changes

We only check that a default argument is a converted constant expression when using the default argument.

However, when parsing a default argument, we need to make sure to parse it as a constant expression such as not ODR-use variables. (otherwise, we would try to capture default template arguments of generic lambdas)

Fixes #107048


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

3 Files Affected:

  • (modified) clang/docs/ReleaseNotes.rst (+1)
  • (modified) clang/lib/Parse/ParseTemplate.cpp (+2)
  • (modified) clang/test/SemaCXX/cxx2a-template-lambdas.cpp (+19)
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index fc940db4813948..e15414b994a36b 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -339,6 +339,7 @@ Bug Fixes to C++ Support
 - Template parameter names are considered in the name lookup of out-of-line class template
   specialization right before its declaration context. (#GH64082)
 - Fixed a constraint comparison bug for friend declarations. (#GH78101)
+- Clang no longer tries to capture default argument of template arguments of generic lambdas (#GH107048)
 
 Bug Fixes to AST Handling
 ^^^^^^^^^^^^^^^^^^^^^^^^^
diff --git a/clang/lib/Parse/ParseTemplate.cpp b/clang/lib/Parse/ParseTemplate.cpp
index 6ecfc15757f3d4..b80999f1b92e7f 100644
--- a/clang/lib/Parse/ParseTemplate.cpp
+++ b/clang/lib/Parse/ParseTemplate.cpp
@@ -960,6 +960,8 @@ Parser::ParseNonTypeTemplateParameter(unsigned Depth, unsigned Position) {
       EnterExpressionEvaluationContext ConstantEvaluated(
           Actions, Sema::ExpressionEvaluationContext::ConstantEvaluated);
       DefaultArg = Actions.CorrectDelayedTyposInExpr(ParseInitializer());
+      if (DefaultArg.isUsable())
+        DefaultArg = Actions.ActOnConstantExpression(DefaultArg);
       if (DefaultArg.isInvalid())
         SkipUntil(tok::comma, tok::greater, StopAtSemi | StopBeforeMatch);
     }
diff --git a/clang/test/SemaCXX/cxx2a-template-lambdas.cpp b/clang/test/SemaCXX/cxx2a-template-lambdas.cpp
index fff524e77d3bf4..ec373f30876295 100644
--- a/clang/test/SemaCXX/cxx2a-template-lambdas.cpp
+++ b/clang/test/SemaCXX/cxx2a-template-lambdas.cpp
@@ -97,3 +97,22 @@ void foo() {
 
 }
 #endif
+
+#if __cplusplus >= 202002L
+void GH107048() {
+  constexpr int x{};
+  const int y{};
+  auto b = []<int=x, int=y>{};
+  using A = decltype([]<int=x>{});
+
+  int z; // expected-note {{'z' declared here}}
+    auto c = []<int t=z>{
+    // expected-error@-1 {{no matching function for call to object of type}} \
+    // expected-error@-1 {{variable 'z' cannot be implicitly captured in a lambda with no capture-default specified}} \
+    // expected-note@-1 {{lambda expression begins here}} \
+    // expected-note@-1 4{{capture}} \
+    // expected-note@-1 {{candidate template ignored: substitution failure: reference to local variable 'z' declared in enclosing function}}
+    return t;
+  }();
+}
+#endif

Comment on lines 962 to 964
DefaultArg = Actions.CorrectDelayedTyposInExpr(ParseInitializer());
if (DefaultArg.isUsable())
DefaultArg = Actions.ActOnConstantExpression(DefaultArg);
Copy link
Contributor

Choose a reason for hiding this comment

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

How about

DefaultArg = Actions.ActOnConstantExpression(ParseInitializer())

Besides, I still don't understand why we shouldn't accept such code, according to the approach taken here

struct S {};

void foo()
{
    constexpr S x{};
    auto a = []<auto = x>{};
}

but rather accept only non-record-type cases

void foo()
{
    constexpr int x{};
    auto a = []<auto = x>{};
}

Can you add some standard quotes 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.

I am not sure what you mean. both should be valid

Copy link
Contributor

Choose a reason for hiding this comment

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

If you look at Sema::CheckLValueToRValueConversionOperand(), it would bail out if E is of a RecordType.
So the following would still be rejected

struct S {};

void foo()
{
    constexpr S x{};
    auto a = []<auto = x>{};
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

(I noticed that provision from the comment in that function, but I still want to confirm what the intention is here)
So... this is invalid? This discrepancy could be rather confusing from what i see :(

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe it is, yes (I agree it is deeply weird)

Per https://eel.is/c++draft/basic#def.odr-10, if x is odr-used, the program is ill-formed
(x is a local variable, there is an intervening lambda scope and x is not captured)

So x can't be odr used. And per https://eel.is/c++draft/basic#def.odr-5 - it would be

I agree that this is weird. It might be worth asking CWG
However, i think the change is correct regardless of further bugs with captures.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://compiler-explorer.com/z/Ybn4nbaz4 is another example of that where compilers agree

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, I'm sold. (The intervening scope concept is still a bit harder for me to understand at the moment, so I'll probably look into it at some point.)

Feel free to merge this after one nit, which makes it a bit simpler

DefaultArg = Actions.ActOnConstantExpression(ParseInitializer());

It would also be great if you could add the record-type case to the test. :)

auto class_type = []<S=s>{};
// expected-error@-1 {{variable 's' cannot be implicitly captured in a lambda with no capture-default specified}} \
// expected-note@-1 {{lambda expression begins here}} \
// expected-note@-1 4{{capture}}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Curious about these cases: https://godbolt.org/z/xWbWYfnf3

struct S {};
constexpr S s; 

void f() {
  auto class_type = []<S=s>{};
}

void h() {
  static constexpr S s;
  auto class_type = []<S=s>{};
}

template <S=s>
void g() {}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All of these work (because they are not local entities so don't need capturing https://eel.is/c++draft/basic#pre-7)

@cor3ntin cor3ntin merged commit 030e4d0 into llvm:main Sep 4, 2024
5 of 6 checks passed
@cor3ntin cor3ntin deleted the corentin/gh107048 branch September 4, 2024 08:38
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 error when using a constexpr variable as NTTP default value in lambda expression.
4 participants