SwiftParser allows closures in condition lists where the compiler doesn't

[allevato]

Issue Kind

Bad Diagnostic Produced

Source Code

if {x}() {}
if x.map{$0} {}

Description

While I would love to have these examples permitted, Swift forbids them but they currently succeed in the new Swift parser without diagnostics and produce the following valid parse tree:

SourceFileSyntax children=2
  0: CodeBlockItemListSyntax children=2        // if {x}() {}
    0: CodeBlockItemSyntax children=1
      0: IfStmtSyntax children=3
        0: ifKeyword
        1: ConditionElementListSyntax children=1
          0: ConditionElementSyntax children=1
            0: FunctionCallExprSyntax children=4
              0: ClosureExprSyntax children=3
                0: leftBrace
                1: CodeBlockItemListSyntax children=1
                  0: CodeBlockItemSyntax children=1
                    0: IdentifierExprSyntax children=1
                      0: identifier("x")
                2: rightBrace
              1: leftParen
              2: TupleExprElementListSyntax
              3: rightParen
        2: CodeBlockSyntax children=3
          0: leftBrace
          1: CodeBlockItemListSyntax
          2: rightBrace
    1: CodeBlockItemSyntax children=1        // if x.map{$0} {}
      0: IfStmtSyntax children=3
        0: ifKeyword
        1: ConditionElementListSyntax children=1
          0: ConditionElementSyntax children=1
            0: FunctionCallExprSyntax children=3
              0: MemberAccessExprSyntax children=3
                0: IdentifierExprSyntax children=1
                  0: identifier("x")
                1: period
                2: identifier("map")
              1: TupleExprElementListSyntax
              2: ClosureExprSyntax children=3
                0: leftBrace
                1: CodeBlockItemListSyntax children=1
                  0: CodeBlockItemSyntax children=1
                    0: IdentifierExprSyntax children=1
                      0: dollarIdentifier("$0")
                2: rightBrace
        2: CodeBlockSyntax children=3
          0: leftBrace
          1: CodeBlockItemListSyntax
          2: rightBrace
  1: eof

[ahoppen]

rdar://99948919

[bnbarham]

if {x}() {}

IMO this is a bug in the C++ parser and should parse (as it now does).

if x.map{$0} {}

This was invalid for a short time but was downgraded to a warning shortly after (swiftlang/swift#28362). We should make sure to emit the warning.

[rintaro]

if {x}() {}

IMO this is a bug in the C++ parser and should parse (as it now does).

I agree.

if let _ = { x }() {}

is accepted by the current C++ parser. and I don't see any ambiguity in if {x}() { ... }

[ahoppen]

To resurrect this thread:

if {x}() {}

The new parser produces an error here saying missing condition in 'if' statement. I think this is good because disallowing { to start an if condition greatly improves recovery.

---

if x.map{$0} {}

This is now accepted by the new parser and I don’t see a reason why it shouldn’t be accepted.

---

So, I’m leaning towards saying that everything behaves as expected. Any objections @allevato @bnbarham @rintaro?

[allevato]

Both open braces are still required to be on the same line, right? i.e., we don't accept this today and still wouldn't:

if x.map{
  $0
}{}

If so, that seems fine to me. We have to be careful in swift-format to not produce invalid code when wrapping, but I imagine that the arbitrary lookahead to make that work in the parser would be even worse for performance/complexity.

[ahoppen]

Yes, we only accept it if the token after { is on the same line as the {.

https://github.com/apple/swift-syntax/blob/107650464b1d09224cb70d586650849f1f8610ac/Sources/SwiftParser/Expressions.swift#L2215-L2225

[ahoppen]

I think a good rule would be that we allow it if the closing } is on the same line as the opening {. That seems more consistent to me than basing it on the line of the token following the {. Because under the current rules the following is allowed

if closure { arg in
  arg
} {}

but the shorthand is disallowed, which seems surprising.

if closure {
  $0
} {}

[ahoppen]

OK, I finally took some real time to look into this. It appears that SwiftParser does match the behavior of the C++ parser. I’m adding test cases to ensure this in #2104 (which also uncovered a BasicFormat bug).

Changing the rules of where trailing closures are allowed would be a separate issue. I’m welcome to suggestions of how to change the rules but I am personally not planning on drafting anything in that direction anymore.