[Clang][Sema] Prohibit statement expression in the default argument

[junaire]

Summary

As statement expression makes no sense in the default argument,
this patch tries to disable it in all cases.

Please note that the statement expression is a GNU extension, which
means that Clang should be consistent with GCC. However, there's no
response from GCC devs since we have raised the issue for several weeks.
In this case, I think we can disallow statement expressions as a default
parameter in general for now, and relax the restriction if GCC folks
decide to retain the feature for functions but not lambdas in the
future.

Related discussion: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104765

Fixes https://github.com/llvm/llvm-project/issues/53488

[junaire]

gentle ping~

[junaire]

In D119609#3331257, @junaire wrote:

gentle ping~

Gentle ping~

[erichkeane]

Does this break a case like this? https://godbolt.org/z/Tc9Ko7hWY

it seems the problem is with the Expression statements, and I'm not sure why we are doing "ActOnCapScopeReturnStmt" when we aren't really in a capture scope anymore, right? I guess we properly diagnose the use of a capture in this scope, but I don't think we should be here in the 1st place.

[aaron.ballman]

Thank you for looking into this! I'm not certain this is the correct approach, however. The comment on the function is:

/// ActOnCapScopeReturnStmt - Utility routine to type-check return statements
/// for capturing scopes.

however, there's not a capture scope in the test case -- the GNU expression statement is within the parameter list, which can't capture anything: https://godbolt.org/z/3KrEx8hEW. I think the issue might be whatever causes us to call ActOnCapScopeReturnStmt() in the first place.

But there's a design question there as to whether expression statements in a lambda default parameter value makes sense *at all*, because the lambda is an object that can be passed around to others to call. e.g., https://godbolt.org/z/3Edeqv9qv

[erichkeane]

Another example that is... concerning: https://godbolt.org/z/84b8Yb7ne

template<typename T>
void call_lambda(T&& L) {

L();

}

int g() {

auto x = [](char c = ({return 5;'c';})){};

call_lambda(x);
return 0;

}

int main() {

return g();

}

I filed a bug against GCC for this, so we should probably see what they want to do: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104765

[junaire]

In D119609#3355368, @erichkeane wrote:

Does this break a case like this? https://godbolt.org/z/Tc9Ko7hWY

No, after applying the patch, clang accepts the code just like gcc

it seems the problem is with the Expression statements, and I'm not sure why we are doing "ActOnCapScopeReturnStmt" when we aren't really in a capture scope anymore, right? I guess we properly diagnose the use of a capture in this scope, but I don't think we should be here in the 1st place.

I found that we will call ActOnCapScopeReturnStmt no matter we have captures or not. I think it is by design.

[junaire]

Frankly speaking, this is really a randomly written patch, and I have never thought it would raise so much concern. But I believe I can handle it if you guys can give me some trust and little guidance! <3

In D119609#3355409, @aaron.ballman wrote:

Thank you for looking into this! I'm not certain this is the correct approach, however. The comment on the function is:

/// ActOnCapScopeReturnStmt - Utility routine to type-check return statements
/// for capturing scopes.

however, there's not a capture scope in the test case -- the GNU expression statement is within the parameter list, which can't capture anything: https://godbolt.org/z/3KrEx8hEW. I think the issue might be whatever causes us to call ActOnCapScopeReturnStmt() in the first place.

Yes, you're correct. From the call stack we can see:

clang::Sema::ActOnCapScopeReturnStmt
clang::Sema::BuildReturnStmt // bad things happened here :(
clang::Sema::ActOnReturnStmt

clang::Parser::ParseStatementOrDeclarationAfterAttributes
clang::Parser::ParseStatementOrDeclaration
clang::Parser::ParseCompoundStatementBody
clang::Parser::ParseCompoundStatement
clang::Parser::ParseParenExpression
clang::Parser::ParseCastExpression
clang::Parser::ParseCastExpression
clang::Parser::ParseAssignmentExpression
clang::Parser::ParseParameterDeclarationClause

clang::Parser::ParseLambdaExpressionAfterIntroducer

When clang is parsing a lambda and hit a ReturnStmt, it will do something like:

if (isa<CapturingScopeInfo>(getCurFunction()))
  return ActOnCapScopeReturnStmt(ReturnLoc, RetValExp, NRInfo,
                                 SupressSimplerImplicitMoves);

However, in this case, though what getCurFunction() returns is a CapturingScopeInfo, we're not finished building the lambda! so clang will crash when we try to use its CallOperator:

bool HasDeducedReturnType =
    CurLambda && hasDeducedReturnType(CurLambda->CallOperator);

So my original patch is to add an extra check when we decide to jump into ActOnCapScopeReturnStmt, however, I can't pass all the tests. Then I discovered that CurLambda can be nullptr, and we still need to call ActOnCapScopeReurnStmt, so I changed my patch. I'm not really sure why this would happen, maybe I need to dig into the code more.

But there's a design question there as to whether expression statements in a lambda default parameter value makes sense *at all*, because the lambda is an object that can be passed around to others to call. e.g., https://godbolt.org/z/3Edeqv9qv

Not sure about this, but at least gcc accepts code below:

void g() {
  auto whatever = [=](int foo = ({ return;5; })) {};
}

I agree to listen to what gcc folks think about it.

[aaron.ballman]

In D119609#3355878, @junaire wrote:

Frankly speaking, this is really a randomly written patch, and I have never thought it would raise so much concern. But I believe I can handle it if you guys can give me some trust and little guidance! <3

We're happy to help!

In D119609#3355409, @aaron.ballman wrote:

Thank you for looking into this! I'm not certain this is the correct approach, however. The comment on the function is:

/// ActOnCapScopeReturnStmt - Utility routine to type-check return statements
/// for capturing scopes.

however, there's not a capture scope in the test case -- the GNU expression statement is within the parameter list, which can't capture anything: https://godbolt.org/z/3KrEx8hEW. I think the issue might be whatever causes us to call ActOnCapScopeReturnStmt() in the first place.

Yes, you're correct. From the call stack we can see:

clang::Sema::ActOnCapScopeReturnStmt
clang::Sema::BuildReturnStmt // bad things happened here :(
clang::Sema::ActOnReturnStmt

clang::Parser::ParseStatementOrDeclarationAfterAttributes
clang::Parser::ParseStatementOrDeclaration
clang::Parser::ParseCompoundStatementBody
clang::Parser::ParseCompoundStatement
clang::Parser::ParseParenExpression
clang::Parser::ParseCastExpression
clang::Parser::ParseCastExpression
clang::Parser::ParseAssignmentExpression
clang::Parser::ParseParameterDeclarationClause

clang::Parser::ParseLambdaExpressionAfterIntroducer

When clang is parsing a lambda and hit a ReturnStmt, it will do something like:

if (isa<CapturingScopeInfo>(getCurFunction()))
  return ActOnCapScopeReturnStmt(ReturnLoc, RetValExp, NRInfo,
                                 SupressSimplerImplicitMoves);

However, in this case, though what getCurFunction() returns is a CapturingScopeInfo, we're not finished building the lambda! so clang will crash when we try to use its CallOperator:

bool HasDeducedReturnType =
    CurLambda && hasDeducedReturnType(CurLambda->CallOperator);

So my original patch is to add an extra check when we decide to jump into ActOnCapScopeReturnStmt, however, I can't pass all the tests. Then I discovered that CurLambda can be nullptr, and we still need to call ActOnCapScopeReurnStmt, so I changed my patch. I'm not really sure why this would happen, maybe I need to dig into the code more.

Thanks for the explanation! Looking at BuildReturnStmt(), I now see why we're getting into this capturing return statement.

But there's a design question there as to whether expression statements in a lambda default parameter value makes sense *at all*, because the lambda is an object that can be passed around to others to call. e.g., https://godbolt.org/z/3Edeqv9qv

Not sure about this, but at least gcc accepts code below:

void g() {
  auto whatever = [=](int foo = ({ return;5; })) {};
}

I agree to listen to what gcc folks think about it.

I'm not certain they intended this to work as they give some interesting diagnostics before crashing: https://godbolt.org/z/nWTGEc1dW. I added that information to the bug report, but it seems to be gaining steam for rejecting the use of a statement expression in a lambda's default argument.

[junaire]

This update proibit any use of ({}) in the default argument.

[erichkeane]

clang/include/clang/Basic/DiagnosticSemaKinds.td

4366

I'm not a fan of this text here(for 1, contractions aren't permitted I think), but I suspect Aaron is the best one to bikeshed. Until then, I might suggest something like:

"expression statement not permitted in default argument".

clang/lib/Parse/ParseDecl.cpp

7092

Can we have a test to make sure this properly captures everything/doesn't get confused with parens/braces inside the expression statement?

clang/test/Sema/err-expr-stmt-in-default-arg.cpp

20

For completeness, I'd like to see an in-function-defined-struct-member-function test here as well.

As for the above question about the recovery, something like:

void fn(int i, int j = ({ {},{},{,} }), int k = ""); I think checks all the issues I can think of. We want to make sure that 'k' still diagnoses its error correctly (and that we have just skipped all of the expression statement stuff).

[junaire]

clang/test/Sema/err-expr-stmt-in-default-arg.cpp

20

Note that clang is already rejected the code: https://godbolt.org/z/57c4qaaPo

[erichkeane]

clang/test/Sema/err-expr-stmt-in-default-arg.cpp

20

I more mean an example like this one:
https://godbolt.org/z/nf7W1zznh

I see that we already reject it, however you are doing a 'skip until' here, and I want to make sure that the error on 'k' diagnoses correctly still.

With your change, I would expect the 1st two errors there to go away and be replaced by your new error. BUT the 'k' type would still be there.

The point of this is to make sure that your error doesn't leave the parser in a 'bad' state.

[erichkeane]

Also, please update the commit-message/title to better reflect the change.

[junaire]

• Make sure Parser can diagnostic remaining error after the expr statement.
• Prohibit expression statement in template argument
• Add more tests
• Better diagnostic message

[junaire]

I just realized that something is not very clear here. Should we prohibit expression statements in all default arguments or just in lambda and templates? I wonder how gcc folks think about it as well.

[erichkeane]

The commit message isn't quite accurate, we're prohibiting it in ALL default arguments.

I just realized that something is not very clear here. Should we prohibit expression statements in all default arguments or just in lambda and templates? I wonder how gcc folks think about it as well.

The latest patch was to prohibit it ONLY in the lambda default parameters, but I though the discussion on the GCC bug had the patch author show preference for all cases. Can you please ping the thread to see what they plan to do so we can align?

[junaire]

In D119609#3369816, @erichkeane wrote:

The commit message isn't quite accurate, we're prohibiting it in ALL default arguments.

Yeah, I just realize this issue after changing the message.

I just realized that something is not very clear here. Should we prohibit expression statements in all default arguments or just in lambda and templates? I wonder how gcc folks think about it as well.

The latest patch was to prohibit it ONLY in the lambda default parameters, but I though the discussion on the GCC bug had the patch author show preference for all cases. Can you please ping the thread to see what they plan to do so we can align?

OK, done. let's wait the feedback before we continue the patch.

[junaire]

Hi @aaron.ballman @erichkeane, I have already left a comment in GCC's Bugzilla, but unfortunately there's been no response for almost a week. I'm even not sure there's really a bug fix for GCC or not. Can you guys decide how to deal with this issue?

[aaron.ballman]

In D119609#3384568, @junaire wrote:

Hi @aaron.ballman @erichkeane, I have already left a comment in GCC's Bugzilla, but unfortunately there's been no response for almost a week. I'm even not sure there's really a bug fix for GCC or not. Can you guys decide how to deal with this issue?

Because statement expressions are a GCC extension that Clang tries to emulate, I'd feel most comfortable moving after the GCC devs make their decision. FWIW, the way I read that patch is there is some support for disabling in all default parameters for consistency reasons, and I could get behind that assuming it doesn't break significant code. I would recommend preparing your patch as if the GCC folks agree to disallow statement expressions as a default parameter in general, we can review that, and if GCC devs still haven't made a decision in a few weeks, I think we can land it. If GCC eventually decides to retain the feature for functions but not lambdas, we can re-evaluate whether we want to relax the restriction to match or not.

[junaire]

In D119609#3385671, @aaron.ballman wrote:

In D119609#3384568, @junaire wrote:

Hi @aaron.ballman @erichkeane, I have already left a comment in GCC's Bugzilla, but unfortunately there's been no response for almost a week. I'm even not sure there's really a bug fix for GCC or not. Can you guys decide how to deal with this issue?

Because statement expressions are a GCC extension that Clang tries to emulate, I'd feel most comfortable moving after the GCC devs make their decision. FWIW, the way I read that patch is there is some support for disabling in all default parameters for consistency reasons, and I could get behind that assuming it doesn't break significant code. I would recommend preparing your patch as if the GCC folks agree to disallow statement expressions as a default parameter in general, we can review that, and if GCC devs still haven't made a decision in a few weeks, I think we can land it. If GCC eventually decides to retain the feature for functions but not lambdas, we can re-evaluate whether we want to relax the restriction to match or not.

Thanks for your comment, I would wait a few weeks to see what we can do.

[junaire]

• Rebase.
• Update commit message.

[junaire]

Hi @aaron.ballman, do you think it's time to consider reviewing this?

I don't why there's no response or progress in GCC, but I think I can submit a patch for them if they think this issue is low prior.

[erichkeane]

In D119609#3424434, @junaire wrote:

Hi @aaron.ballman, do you think it's time to consider reviewing this?

I don't why there's no response or progress in GCC, but I think I can submit a patch for them if they think this issue is low prior.

I think I'm ok with rejecting both cases now, and doing this review. Though I'm still concerned with the 'skip-until' losing some diagnostics, so please add another test for that as I requested before.

clang/test/Sema/err-expr-stmt-in-default-arg.cpp

4

Do we also prohibit in a template argument?

20

I don't see the test I requested here, I'd still like to see that to make sure the parser still diagnoses' k'.

[junaire]

clang/test/Sema/err-expr-stmt-in-default-arg.cpp

11

@erichkeane Do you mean this one? Or I get you wrong?

[erichkeane]

I think LGTM on technical perspective, Please don't commit until @aaron.ballman confirms he is OK with the direction, or would like to wait longer for GCC.

clang/test/Sema/err-expr-stmt-in-default-arg.cpp

4

Template args were already disallowed, right? Presumably there is a test for that?

11

Ah, missed that, thank you! This is sufficient for my concerns.

[junaire]

Add tests for template arguments.

[aaron.ballman]

In D119609#3426279, @erichkeane wrote:

I think LGTM on technical perspective, Please don't commit until @aaron.ballman confirms he is OK with the direction, or would like to wait longer for GCC.

I spotted some technical issues with the diagnostic wording. As for waiting for GCC, my concern there is largely with the fact that people use statement expressions in macros so that they can define local variables that don't impact the outer scope where the macro is expanded. I could see such a macro being used as a default argument, so I worry a bit about breaking code in that case. However, C doesn't have default arguments, so this isn't an issue there. And C++ has lambdas which can be called in a default argument, so users have some ability to fix their code in the event of an error so long as they're in C++11 or later. Because of that, I would be okay moving forward with this, but cautiously in case someone reports code that breaks and can't easily be fixed (or significant breakage in an important 3rd party header).

clang/include/clang/Basic/DiagnosticSemaKinds.td

4365–4366

I get mixed up every time, but I double-checked -- these are statement expressions, not expression statements. I also think the diagnostic should be more clear about default argument vs default non-type template argument.

clang/lib/Parse/ParseDecl.cpp

7092

clang/test/Sema/err-expr-stmt-in-default-arg.cpp

2

You should rename this test to stmt-expr-in-default-arg.cpp

[junaire]

Wording issues.

[aaron.ballman]

This should also have a release note that explains the change in behavior, since we're now being more restrictive with what programs we accept. Aside from the release note and the column wrapping, this LGTM!

clang/include/clang/Basic/DiagnosticSemaKinds.td

4366

You should re-wrap this to the usual 80-col limit.

[junaire]

• Add release note.
• re-wrap line.

clang/include/clang/Basic/DiagnosticSemaKinds.td

 def err_uninitialized_member_in_ctor : Error<
   "%select{constructor for %1|"
   "implicit default constructor for %1|"
   "cannot use constructor inherited from %1:}0 must explicitly "
   "initialize the %select{reference|const}2 member %3">;
 def err_default_arg_makes_ctor_special : Error<
   "addition of default argument on redeclaration makes this constructor a "
   "%select{default|copy|move}0 constructor">;
+def err_expr_statement_in_default_arg : Error<
+  "expression statement not permitted in default argument">;
 
 def err_use_of_default_argument_to_function_declared_later : Error<
   "use of default argument to function %0 that is declared later in class %1">;
 def note_default_argument_declared_here : Note<
   "default argument declared here">;
 def err_recursive_default_argument : Error<"recursive evaluation of default argument">;
 def note_recursive_default_argument_used_here : Note<
   "default argument used here">;

clang/lib/Parse/ParseDecl.cpp

if (DS.isEmpty() && ParmDeclarator.getIdentifier() == nullptr &&
               Actions,
               Sema::ExpressionEvaluationContext::PotentiallyEvaluatedIfUsed,
               Param);
 
           ExprResult DefArgResult;
           if (getLangOpts().CPlusPlus11 && Tok.is(tok::l_brace)) {
             Diag(Tok, diag::warn_cxx98_compat_generalized_initializer_lists);
             DefArgResult = ParseBraceInitializer();
-          } else
+          } else {
+            if (Tok.is(tok::l_paren) && NextToken().is(tok::l_brace)) {
+              Diag(Tok, diag::err_expr_statement_in_default_arg);
+              Actions.ActOnParamDefaultArgumentError(Param, EqualLoc);
+              // Skip the expression statement and continue parsing
+              SkipUntil(tok::comma, StopBeforeMatch);
+              continue;
+            }
             DefArgResult = ParseAssignmentExpression();
+          }
           DefArgResult = Actions.CorrectDelayedTyposInExpr(DefArgResult);
           if (DefArgResult.isInvalid()) {
             Actions.ActOnParamDefaultArgumentError(Param, EqualLoc);
             SkipUntil(tok::comma, tok::r_paren, StopAtSemi | StopBeforeMatch);
           } else {
             // Inform the actions module about the default argument
             Actions.ActOnParamDefaultArgument(Param, EqualLoc,
                                               DefArgResult.get());

clang/lib/Parse/ParseTemplate.cpp

 #include "clang/AST/DeclTemplate.h"
 #include "clang/AST/ExprCXX.h"
 #include "clang/Parse/ParseDiagnostic.h"
 #include "clang/Parse/Parser.h"
 #include "clang/Parse/RAIIObjectsForParser.h"
 #include "clang/Sema/DeclSpec.h"
 #include "clang/Sema/ParsedTemplate.h"
 #include "clang/Sema/Scope.h"
+#include "clang/Sema/SemaDiagnostic.h"
 #include "llvm/Support/TimeProfiler.h"
 using namespace clang;
 
 /// Re-enter a possible template scope, creating as many template parameter
 /// scopes as necessary.
 /// \return The number of template parameter scopes entered.
 unsigned Parser::ReenterTemplateScopes(MultiParseScope &S, Decl *D) {
   return Actions.ActOnReenterTemplateScope(D, [&] {
if (TryConsumeToken(tok::ellipsis, EllipsisLoc))
     DiagnoseMisplacedEllipsisInDeclarator(EllipsisLoc, ParamDecl);
 
   // If there is a default value, parse it.
   // Per C++0x [basic.scope.pdecl]p9, we parse the default argument before
   // we introduce the template parameter into the local scope.
   SourceLocation EqualLoc;
   ExprResult DefaultArg;
   if (TryConsumeToken(tok::equal, EqualLoc)) {
+    if (Tok.is(tok::l_paren) && NextToken().is(tok::l_brace)) {
+      Diag(Tok.getLocation(), diag::err_expr_statement_in_default_arg);
+      SkipUntil(tok::comma, tok::greater, StopAtSemi | StopBeforeMatch);
+    } else {
       // C++ [temp.param]p15:
       //   When parsing a default template-argument for a non-type
       //   template-parameter, the first non-nested > is taken as the
       //   end of the template-parameter-list rather than a greater-than
       //   operator.
       GreaterThanIsOperatorScope G(GreaterThanIsOperator, false);
       EnterExpressionEvaluationContext ConstantEvaluated(
           Actions, Sema::ExpressionEvaluationContext::ConstantEvaluated);
-
-    DefaultArg = Actions.CorrectDelayedTyposInExpr(ParseAssignmentExpression());
+      DefaultArg =
+          Actions.CorrectDelayedTyposInExpr(ParseAssignmentExpression());
       if (DefaultArg.isInvalid())
         SkipUntil(tok::comma, tok::greater, StopAtSemi | StopBeforeMatch);
     }
+  }
 
   // Create the parameter.
   return Actions.ActOnNonTypeTemplateParameter(getCurScope(), ParamDecl,
                                                Depth, Position, EqualLoc,
                                                DefaultArg.get());
 }
 
 void Parser::DiagnoseMisplacedEllipsis(SourceLocation EllipsisLoc,

clang/test/Sema/err-expr-stmt-in-default-arg.cpp

+// RUN: %clang_cc1 %s -fsyntax-only -verify -std=c++20
+
+void foo() {
+  void fn(int i, int = ({ 1; })); // expected-error {{expression statement not permitted in default argument}}
+
+  auto a = [](int = ({ 1; })) {}; // expected-error {{expression statement not permitted in default argument}}
+
+  auto b = []<int = ({ 1; })>(){}; // expected-error {{expression statement not permitted in default argument}}
+
+  void fn(int i, int j = ({{}, {}, {,}}), int k = ""); // expected-error {{expression statement not permitted in default argument}} expected-error {{cannot initialize a parameter of type 'int' with an lvalue of type 'const char[1]'}} expected-note {{passing argument to parameter 'k' here}}
+}
+
+template <typename Callable>
+int bar(Callable &&Call) {
+  return Call();
+}
+
+int baz() {
+  auto l = [](int a = ({ int x = 12; x; })) { // expected-error {{expression statement not permitted in default argument}}
+    return 1;
+  };
+  return bar(l);
+}

clang/test/SemaTemplate/dependent-expr.cpp

template<typename> auto g() {
     using T = decltype(c(0.0, 0));
     using T = void;
     return c(0, 0);
   }
   using U = decltype(g<int>()); // expected-note {{previous}}
   using U = float; // expected-error {{different types ('float' vs 'decltype(g<int>())' (aka 'void'))}}
 
   void h(auto a, decltype(g<char>())*) {} // expected-note {{previous}}
-  void h(auto a, void*) {} // expected-error {{redefinition}}
-
-  void i(auto a) {
-    [](auto a, int = ({decltype(a) i; i * 2;})){}(a); // expected-error {{invalid operands to binary expression ('decltype(a)' (aka 'void *') and 'int')}} expected-note {{in instantiation of}}
-  }
-  void use_i() {
-    i(0);
-    i((void*)0); // expected-note {{instantiation of}}
-  }
+  void h(auto a, void *) {} // expected-error {{redefinition}}
 }
 
 namespace BindingInStmtExpr {
   template<class ...Ts> struct overload : Ts... {
     overload(Ts ...ts) : Ts(decltype(ts)(ts))... {}
     using Ts::operator()...;
   };