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

[Jun Zhang via Phabricator via cfe-commits]

junaire added a comment.

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 <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.

> 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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119609