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
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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.
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
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
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.
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
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.
We're happy to help!
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::ParseLambdaExpressionAfterIntroducerWhen 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.
clang/include/clang/Basic/DiagnosticSemaKinds.td | ||
---|---|---|
4384 | 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 | ||
7073 | 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 | ||
19 ↗ | (On Diff #413195) | 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). |
clang/test/Sema/err-expr-stmt-in-default-arg.cpp | ||
---|---|---|
19 ↗ | (On Diff #413195) | Note that clang is already rejected the code: https://godbolt.org/z/57c4qaaPo |
clang/test/Sema/err-expr-stmt-in-default-arg.cpp | ||
---|---|---|
19 ↗ | (On Diff #413195) | I more mean an example like this one: 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. |
- Make sure Parser can diagnostic remaining error after the expr statement.
- Prohibit expression statement in template argument
- Add more tests
- Better diagnostic 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 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?
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.
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.
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 | ||
---|---|---|
3 ↗ | (On Diff #419965) | Do we also prohibit in a template argument? |
19 ↗ | (On Diff #413195) | I don't see the test I requested here, I'd still like to see that to make sure the parser still diagnoses' k'. |
clang/test/Sema/err-expr-stmt-in-default-arg.cpp | ||
---|---|---|
10 ↗ | (On Diff #419965) | @erichkeane Do you mean this one? Or I get you wrong? |
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 | ||
---|---|---|
3 ↗ | (On Diff #419965) | Template args were already disallowed, right? Presumably there is a test for that? |
10 ↗ | (On Diff #419965) | Ah, missed that, thank you! This is sufficient for my concerns. |
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 | ||
---|---|---|
4383–4384 | 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 | ||
7073 | ||
clang/test/Sema/err-expr-stmt-in-default-arg.cpp | ||
1 ↗ | (On Diff #420178) | You should rename this test to stmt-expr-in-default-arg.cpp |
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 | ||
---|---|---|
4384 | You should re-wrap this to the usual 80-col limit. |
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".