[SR-12583] Added checks to catch 'foo() {} {}' and 'foo {} () {}'

[dfsweeney]

Addresses two parse problems in SR-12583, where (single) trailing closures should not be parsed in the neighborhood of other trailing closures. The parse trees for both forms are identical but the Parser takes two paths to get there. @jckarter entered the original bug.

Several questions:

1. Is this the right place/method to do this? I just put an if(condition) check in two places in the parser and emitted a diagnostic. Should the check be in the CallExpr::create() instead of in the parser?

2. I'm not completely sure that there's not a valid parse like this. The parse tree looks like the below. It indents poorly here but it's a call with a trailing closure containing another call with a trailing closure. I'm still thinking through whether there's some legitimate Swift code that could generate that:
   (call_expr (call_expr (paren_expr trailing-closure)) (paren_expr trailing-closure))

3. The diagnostic just says 'double trailing closures' right now, which is accurate but not clear enough. Any suggestions to improve the wording are welcome. I will think about it further.

4. This may interfere or interact with work on multiple trailing closures that @xedin (I think) is working on now.

5. The swift in the examples is kind of aesthetically displeasing but logically OK. I'm not sure this should actually be disallowed.

Resolves SR-12583.

[rintaro]

Thank you for tackling this!

Is this the right place/method to do this? I just put an if(condition) check in two places in the parser and emitted a diagnostic. Should the check be in the CallExpr::create() instead of in the parser?

If we diagnose this, yes, the parser is the right place.

I'm not completely sure that there's not a valid parse like this. The parse tree looks like the below. It indents poorly here but it's a call with a trailing closure containing another call with a trailing closure. I'm still thinking through whether there's some legitimate Swift code that could generate that:
(call_expr (call_expr (paren_expr trailing-closure)) (paren_expr trailing-closure))

At least in the current grammar, they should be valid syntax.
expr {} {}, expr {} () {}, expr() {} {} and expr() {} () {} all should be parsed as

(call_expr
  (call_expr
    "expr"
    (paren_expr trailing-closure))
  (paren_expr trailing-closure))

This may interfere or interact with work on multiple trailing closures

It should be "OK" because multiple trailing closures require argument labels. For example
expr {} label: {} {}, we can parse this as (expr {} label: {}) {} because the last closure doesn't have an argument label.

The swift in the examples is kind of aesthetically displeasing but logically OK. I'm not sure this should actually be disallowed.

For now, we shouldn't disallow any currently accepted syntax.
As Chris says in the (SR-12583)[https://bugs.swift.org/browse/SR-12583?focusedCommentId=55629], we might want to change the grammar in the future, but for now, we can't.

So @dfsweeney, could you consider changing parser to accept expr {} {} as the valid Swift code?

[dfsweeney]

I think I misunderstood Chris's comments and thought that we should prevent the parse, so that's what I put in the code in this PR (assuming the code actually does that, haha).

It makes sense for expr {} {} to parse consistently--I will look at allowing that.

Should I close this PR? I think allowing expr {} {} to parse as above probably touches different code--I could make a new PR for that once I identify it. What do you think?

[jckarter]

Disallowing any form seems in line with the "spirit" if not the letter of the grammar, but it would be a source-breaking change. I'll bring this up with the Core Team to decide what the best approach is.

[jckarter]

@swift-ci Please test source compatibility

[dfsweeney]

I'm going to close this one for now. I have the code if we want to bring it back up at some point. It did not take that long to do and I learned a bunch of stuff.