[PATCH] D119609: [Clang][Sema] Don't act on ReturnStmt when parsing the lambda declarator.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D119609#3355878 <https://reviews.llvm.org/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 <https://reviews.llvm.org/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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D119609/new/

https://reviews.llvm.org/D119609