Add option for opening_brace rule not to trigger with multiline statements

[leonardosrodrigues0]

In case of multiline if and while statements, many adopt the style of putting the opening brace in a new line. This change add a new option to the rule to allow that style.

Closes #3720

[SwiftLintBot]

	17 Messages
📖	Linting Aerial with this PR took 0.91s vs 0.92s on main (1% faster)
📖	Linting Alamofire with this PR took 1.26s vs 1.27s on main (0% faster)
📖	Linting Brave with this PR took 7.42s vs 7.4s on main (0% slower)
📖	Linting DuckDuckGo with this PR took 4.71s vs 4.69s on main (0% slower)
📖	Linting Firefox with this PR took 10.86s vs 10.82s on main (0% slower)
📖	Linting Kickstarter with this PR took 9.83s vs 9.85s on main (0% faster)
📖	Linting Moya with this PR took 0.53s vs 0.52s on main (1% slower)
📖	Linting NetNewsWire with this PR took 2.54s vs 2.54s on main (0% slower)
📖	Linting Nimble with this PR took 0.76s vs 0.76s on main (0% slower)
📖	Linting PocketCasts with this PR took 8.53s vs 8.53s on main (0% slower)
📖	Linting Quick with this PR took 0.43s vs 0.43s on main (0% slower)
📖	Linting Realm with this PR took 4.62s vs 4.64s on main (0% faster)
📖	Linting Sourcery with this PR took 2.35s vs 2.34s on main (0% slower)
📖	Linting Swift with this PR took 4.56s vs 4.56s on main (0% slower)
📖	Linting VLC with this PR took 1.24s vs 1.24s on main (0% slower)
📖	Linting Wire with this PR took 18.02s vs 18.03s on main (0% faster)
📖	Linting WordPress with this PR took 11.77s vs 11.77s on main (0% slower)

Generated by 🚫 Danger

[leonardosrodrigues0]

@SimplyDanny Could you take a look at this when you have some time? It would benefit many that are used to adopting this style.

I left the new option on by default just to check with the OSS projects, all the changes are the expected fixed violations.

[SimplyDanny]

I wonder why this new option does not apply likewise to guard and for (and maybe other statements). Any reason for that?

I left the new option on by default just to check with the OSS projects, all the changes are the expected fixed violations.

Very good! That helps a lot in assessment.

[leonardosrodrigues0]

I wonder why this new option does not apply likewise to guard and for (and maybe other statements). Any reason for that?

From the issue and my experience, the main focus would be if, while and guard, but guard has the else keyword that is used in the same line as the opening brace in this type of cases:

guard
    let child = parent.children.first,
    let grandchild = child.children.first,
    let greatGrandchild = grandchild.children.first
else {
    // Some code
}

I did, however, take a look at the OSS differences when we ignore the rule for multiline "predecessors of the body" for all statements affected by the rule. I checked all differences and it seems that this style is frequently adopted in type declarations (and extensions), as in:

extension RawRepresentable
where
    Self: AtomicOptionalRepresentable,
    RawValue: AtomicOptionalRepresentable
{
    // Some code
}

Considering this, I think that the new direction should be to let the option ignore all statements that are "multiline before the opening brace" (except for single keywords like do and defer). And if we go this way, I don't see a reason why we would leave functions and initializers (that are already ignored by the allowMultilineFunc option) with an independent option of all other statements.

Changing the existing option name and make it affect all statements would be a breaking change, and I don't have experience on how these are made in this project. Would we still support allowMultilineFunc but deprecate it?

Additionally, the new option name could be something like ignoreMultilineBracePredecessors, ignoreMultilineOpeningBracePredecessors or ignoreMultilineBodyPredecessors. I'm not sure if "predecessors" fits well here, but it is the best description I could create. On the other hand, ignoreMultilineStatements would be simpler but imprecise, as you pointed out.

[SimplyDanny]

Thank you for the write-up! So the topic applies to functions/initializers (already supported), types and statements (implemented by this PR).

I think that an existing option shouldn't suddenly support much more than it was intended for. But we may rename allow_multiline_func to something better matching while still supporting the current name for the time being.

Statements like if, while, guard and for should be associated with a separate option.

Types might be yet another category. We could merge them with the control-flow statements from the previous group (as both groups will be newly introduced). However, this doesn't really match, which would already be problematic when looking for an option name.

Talking about names: How about ignoreMultilineFunctionSignatures, ignoreMultilineStatementConditions and ignoreMultilineTypeHeaders?

[leonardosrodrigues0]

I like the functions/initializers, types and statements separation and the name for the configuration options, I'll implement that.

But we may rename allow_multiline_func to something better matching while still supporting the current name for the time being.

How would we do that? I found this example being removed in #5107, should we mark the option as deprecated like this?

mutating func apply(configuration: Any) throws {
    // ...
    } else if let statementLevelConfiguration = configurationDict["statement_level"] {
        queuedPrintError(
            """
            warning: 'statement_level' has been renamed to 'function_level' and will be completely removed \
            in a future release.
            """
        )
        try functionLevel.apply(configuration: statementLevelConfiguration)
    }
    // ...
}

[SimplyDanny]

How would we do that? I found this example being removed in #5107, should we mark the option as deprecated like this?

The new macro approach doesn't support deprecation warnings well yet. I'm working on it though. For your PR, just leave the existing option as is for now. I'll take care of the rename once I found a good solution.

[SimplyDanny]

How would we do that? I found this example being removed in #5107, should we mark the option as deprecated like this?

The new macro approach doesn't support deprecation warnings well yet. I'm working on it though. For our PR, just leave the existing option as is for now. I'll take care of the rename once I found a good solution.

I've added a way to mark options as deprecated in #5540. Just in case you want to handle the rename here as well ... 😉

[roksollc]

Hello everyone! I've never contributed here, but my team would also like to see these changes. Great idea! While looking at the proposed solution, I added a couple of suggestions / questions. Thank you @leonardosrodrigues0 for your efforts!

[SimplyDanny]

@leonardosrodrigues0: Do you fancy to continue with this PR?

[leonardosrodrigues0]

Hi @SimplyDanny, I apologize for the long absence, I want to continue with this from now on. Thanks @roksollc for your POV as well.

I saw recent changes with the addition of brace_on_new_line, it's a great new option in my opinion. Before diving into the implementation, I'm considering how this option would interact with the "ignore" ones. It seems to me that if brace_on_new_line is enabled, then the "ignore" options shouldn't be considered: being multiline or not, the opening brace should be aligned with the parent's start column and one line after the previous location. This is different from the current interaction, in which allow_multiline_func works even with brace_on_new_line.

This made me think if these new options should actually enforce the opening brace position similar to brace_on_new_line instead of simply ignoring the rule for those multiline cases. My first conclusion, however, is that I'm overcomplicating this a bit, and ignoring should be fine. Let me know if you guys disagree.

[SimplyDanny]

Hi @SimplyDanny, I apologize for the long absence, I want to continue with this from now on. Thanks @roksollc for your POV as well.

Nothing to apologize for. Glad you are back!

I saw recent changes with the addition of brace_on_new_line, it's a great new option in my opinion. Before diving into the implementation, I'm considering how this option would interact with the "ignore" ones. It seems to me that if brace_on_new_line is enabled, then the "ignore" options shouldn't be considered: being multiline or not, the opening brace should be aligned with the parent's start column and one line after the previous location. This is different from the current interaction, in which allow_multiline_func works even with brace_on_new_line.

This made me think if these new options should actually enforce the opening brace position similar to brace_on_new_line instead of simply ignoring the rule for those multiline cases. My first conclusion, however, is that I'm overcomplicating this a bit, and ignoring should be fine. Let me know if you guys disagree.

I see your point. With the introduction of brace_on_new_line this gets slightly confusing. We basically have three modes:

1. Braces always on the same line with the declaration.
2. Braces always on a single line after the declaration disregarding other options.
3. Braces always on the same line with the declaration with "don't care" exceptions.

The "don't care", i.e. no real enforcement of a certain style, is a bit strange, but it's like it has always been. brace_on_new_line ignoring all other options also isn't too nice. Probably, brace_on_new_line should be its own rule to avoid these quirks.

[SimplyDanny]

Probably, brace_on_new_line should be its own rule to avoid these quirks.

That will be done with #5723. So you don't have to worry your brain on brace_on_new_line when continuing here, @leonardosrodrigues0. 😉

[leonardosrodrigues0]

That will be done with #5723. So you don't have to worry your brain on brace_on_new_line when continuing here, @leonardosrodrigues0. 😉

@SimplyDanny thank you!!

I just pushed a new version with the two new options. Also renamed the old allow_multiline_func one to ignore_multiline_function_signatures and marked the former as deprecated. I added a computed variable shouldIgnoreMultilineFunctionSignatures to keep supporting the original option for now, let me know in case that isn't the best approach.

Once again, the new options are set to true just to check the changes in the OSS projects, which I did, and all the differences are expected.

[SimplyDanny]

Looks good. Well done!

What about guard? Any reason why it's ignored?

[leonardosrodrigues0]

Looks good. Well done!

What about guard? Any reason why it's ignored?

Thanks! Yes, guard's opening braces are always preceded by else, so this rule doesn't affect those who adopt this style for multiline statements. Borrowing one of the examples in the issue:

guard
    let child = parent.children.first,
    let grandchild = child.children.first,
    let greatGrandchild = grandchild.children.first
else {
    // Some code
}

[SimplyDanny]

Looks good from my side. Ready to merge? Or is there anything you'd like to adapt?

[leonardosrodrigues0]

Ready to merge! Thanks for the reviews and the patience with the delay 😅

[pyrtsa]

This made me think if these new options should actually enforce the opening brace position similar to brace_on_new_line instead of simply ignoring the rule for those multiline cases.

I think what we'd need is another brace_on_new_line_after_multiline_statement option to opening_brace rule.

[SimplyDanny]

This made me think if these new options should actually enforce the opening brace position similar to brace_on_new_line instead of simply ignoring the rule for those multiline cases.

I think what we'd need is another brace_on_new_line_after_multiline_statement option to opening_brace rule.

Please open a ticket to discuss this, @pyrtsa.