Avoid removing certain disambiguating parens from conditions (fixes #298)

[stackotter]

Essentially, after formatting the correct code in the snippet below, you end up with invalid Swift code (because of the parsing ambiguity introduced).

// Input
if ({ true }()) {}

// Output (fails to compile)
if { true }() {}

My fix adds another early exit condition (to the NoParensAroundConditions rule) that checks if the enclosed expression is a call to a closure expression. I have added a test for this new behaviour, and in doing so I found that there wasn't a test for the (already implemented) ambiguity check; so I added that to the test too.

Tl;dr

Merging this PR would fix the exact issue raised in #298. However, I have found that the existing ambiguity check (pre-PR) for trailing closures in conditions is purely cosmetic (and incomplete) in current Swift. Thus, I think the check for disambiguating parens should be reworked to work like so:

If a condition contains any trailing closures or immediately called closures that aren't otherwise enclosed within (), [], or {} (visually), the condition requires disambiguating parens around the whole thing. It is pretty conservative, but it's guaranteed to always be correct both in terms of compilation and visual disambiguation (as long as there aren't more patterns that could be ambiguous such as if/switch expressions).

I'd love to hear your opinions on this solution.

Caveats and complications

Although this PR fixes the issue in the most simple case, I have still managed to find some more obscure snippets that have a similar issue (which aren't fixed by this PR). A more complex recursive check would have to be implemented to fix these ambiguities, I discuss this more in the next section.

if ({ true }() && true) { print("true") }
if ({ true }() || true) { print("true") }
// etc.

My instinct is that if the left-most expression is an immediately called closure, parentheses are required, and otherwise they aren't (in terms of compilation). If you add a ! operator before an ambiguous called closure, the compiler successfully parses the code with or without parentheses.

Visual disambiguation parentheses (not required for correct parsing)

Interestingly, the existing check (pre-PR) for cases such as if (f(1) { false }) { print("hi") } is purely cosmetic in current Swift (it successfully compiles with or without parentheses around the condition). This begs the question, should parentheses also be left in in any of the following cases where they aren't strictly required for compilation?

if (true && f(1) { true }) {} // probably
if (f(1) { true } && true) {} // maybe?
if (!f(1) { true }) {} // the `!` makes the rule remove the parens, but it probably shouldn't

I believe that to truly fix this issue, we should implement a new (and probably recursive) heuristic for identifying when parens are required; which will require more clearly deciding what we count as ambiguous to humans (not just the compiler). However, these concerns are purely cosmetic and probably a separate issue, it'd be good to merge this fix before doing that because this fix is actually a functional issue not just a matter of preference. See the tldr for my proposed solution.

[allevato]

I think this is the right change for the specific case you've pinpointed here.

For the rest of your post (thanks for such a detailed discussion!), cases like removing the parentheses from if (f(1) { false }) { print("hi") } are tricky. I believe that they used to not compile at all in older versions of Swift (but I haven't tracked it closely). But even today, there are some subtleties. This compiles successfully when all on one line:

if f(1) { false } { print("hi") }

But when split across multiple lines, it no longer compiles and the error indicates that the parser wants to treat it as two independent statements:

if f(1) {
    false
} {
    print("Hi")
}

error: consecutive statements on a line must be separated by ';'
} {
 ^
 ;

Since the whitespace for whatever we do here will be resolved later on in the pretty printer, we don't have the future knowledge about how this will wrap to know whether we can drop the parentheses. But if we leave them, we'll be correct no matter what happens.

[stackotter]

Thank you! Do you think I should try fixing this for the other cases I found in a future PR (if I find the time)?

[allevato]

I'm happy to review changes that improve the logic of this rule, preferring parenthesis removal where possible, as long as it doesn't open the code up to parse ambiguities due to whitespace changes later on in the formatting pipeline.

[stackotter]

Awesome, thanks for reviewing and merging all of my PRs! I've been away so I only just saw these