Excluding Multi-line if/guard/while Conditions From Opening Brace Rule

[scottrhoyt]

Closes #355

/cc @watt

Here is my first naive take on how to accomplish this. I'm not sure this is the right way to do this. I would love some help testing for correctness and performance implications.

@jpsim I'm sure you can bring some functional goodness to this.

[scottrhoyt]

This might be a slightly better regex: ((?:if|guard|while)\n(?:[^\{]+\n)+\{).

[scottrhoyt]

@realm/swiftlint

Just rebased this. I don't want to leave this WIP PR out there much longer. Any thoughts on implementation?

As a recap, I initially tried to implement this by modifying the original regex but was unsuccessful. I then switched to an implementation that finds matches for the original regex, then finds matches for a new regex that is designed to detect the multi-line if/while/guard and excludes those from the original matches.

Primary areas of feedback are:

• exclusion regex correctness (mostly looking for false negatives, since it's function is one of subtraction)
• implementation style
• performance costs

I plan on checking these again myself. Thanks!

[jpsim]

internal?

[scottrhoyt]

Yep. Thanks.

[jpsim]

Seems quite reasonable to me, although this should really be benchmarked to see how expensive it is.

[scottrhoyt]

Just benchmarked 5x vs. master on Carthage. Difference in performance was < 0.5% with ~ 1% stdev in the results. So no significant change in performance by my measure, though I wouldn't mind a second pair of eyes.

[jpsim]

Could you add some triggering examples? Multi-line variants of the existing ones should be fine.

Have you looked into expanding the autocorrect scope to support multiline statements like these? It's fine if that's out of scope for this PR, but I'd like to make sure we're not leaving potentially low hanging fruit on the tree.

[scottrhoyt]

Definitely. I'll add the triggering examples, and take a look at autocorrect. I guess now is as good a time as any to dive into that side of things.

[scottrhoyt]

@jpsim, for autocorrect I decided to add corrections to test, but not to modify the existing behavior.

This...

if
  let a = b,
  let c = d{
  // do something
}

...will be corrected to this...

if
  let a = b,
  let c = d {
  // do something
}

...as was the behavior before.

Do you think we want to force this on people in autocorrect?

if
  let a = b,
  let c = d
{
  // do something
}

Or do we want that to be a personal style decision?

[jpsim]

I think sticking with the default behavior is fine, but I'm happy it's now tested!

[scottrhoyt]

Cool. I'll get this rebased and then good to go I think.

[jpsim]

👍 awesome, thanks @scottrhoyt!

[watt]

Crap, I missed all the notifications for this, sorry @jpsim. I will likely still look into making a PR to support syntax like the following:

if let
    foo = foo,
    bar = foo.bar
{
}

As well as guard statements like this:

guard let
    foo = foo,
    bar = foo.bar
else {
}

But this is a huge step in the right direction, so thanks for doing this!

[scottrhoyt]

That should be a pretty easy modification of the exclusion regex to allow an optional let between the (if|gaurd|while) and the newline, and an optional else before the {. But be sure to benchmark the performance on a couple of repos. I benchmarked the initial change on a single repo with little difference, but there was a huge regression on another repo.

[dimazen]

Any updates on it?

[jpsim]

This was shipped with 0.8.0: https://github.com/realm/SwiftLint/releases/tag/0.8.0

[dimazen]

@jpsim sorry, i was meant syntax, mentioned by Watt above:

if let 
    foo = foo,
    bar = bar
{ 
}

Running 0.9.2 and Swiftlint complains about opening brace for similar syntax :)

[jpsim]

Judging by the git history for that rule, your request hasn't been implemented yet.