Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

opening_braces allows some multiline if statements, others get flagged #5923

Open
2 tasks done
dhoerl-lp opened this issue Dec 28, 2024 · 10 comments
Open
2 tasks done
Labels
discussion Topics that cannot be categorized as bugs or enhancements yet. They require further discussions. enhancement Ideas for improvements of existing features and rules.

Comments

@dhoerl-lp
Copy link

dhoerl-lp commented Dec 28, 2024

New Issue Checklist

Bug Description

Related issues: #5521, #5602

Some multiline if statements with a starting brace don't trigger an error, but more complicated lines do. For instance no warning:

// Instance variables
var foo = 5
var goo = 7

if
    self.foo == 9,
    self.goo == 10
{
    print("SUCCESS")
} else {
    fatalError()
}

More usual code triggers an opening_brace error:

    if
        let rootVC = AppDelegate.myDelegate.window?.rootViewController as? XYZ,
        let abcVC = rootVC.myArray.first as? ABC
    {
        ...
    } else {
        fatalError()
    }

Environment

  • SwiftLint version 0.57.1
  • Xcode version 16.2
  • Installation method used Swift Package
  • Configuration file:
# Case-sensitive paths to include during linting. Directory paths supplied on the
# command line will be ignored.
included: 
  - abc
  - def
excluded: # case-sensitive paths to ignore during linting. Takes precedence over `included`
  - 1111

# By default, SwiftLint uses a set of sensible default rules you can adjust:
disabled_rules: # rule identifiers turned on by default to exclude from running
  - trailing_whitespace
  - line_length
  - force_cast
  - cyclomatic_complexity
  - function_body_length
  - identifier_name
  - file_length
  - function_parameter_count
  - large_tuple
  - type_name
  - type_body_length
  - todo
  - no_fallthrough_only
  - static_over_final_class
  - inclusive_language
  - legacy_random
  - nesting
  - nsobject_prefer_isequal
  - force_try
  - closure_parameter_position

opt_in_rules: # some rules are turned off by default, so you need to opt-in
  - empty_count # find all the available rules by running: `swiftlint rules`

analyzer_rules: # rules run by `swiftlint analyze`
  - explicit_self

# If true, SwiftLint will not fail if no lintable files are found.
allow_zero_lintable_files: false

# If true, SwiftLint will treat all warnings as errors.
strict: false

# If true, SwiftLint will treat all errors as warnings.
lenient: false

# If true, SwiftLint will check for updates after linting or analyzing.
check_for_updates: true

reporter: "xcode" # reporter type (xcode, json, csv, checkstyle, codeclimate, junit, html, emoji, sonarqube, markdown, github-actions-logging, summary)

colon:
  severity: error

comma:
  severity: error

control_statement:
  severity: error

trailing_comma:
  severity: error
  mandatory_comma: true

Are you using nested configurations? If so, paste their
relative paths and respective contents. N/A

@SimplyDanny
Copy link
Collaborator

I'm unable to reproduce this issue. Both examples trigger with version 0.57.1 on my side. With the option ignore_multiline_statement_conditions set to true there's no violation. Please double check.

@SimplyDanny SimplyDanny added the repro-needed Issues that cannot be reproduced or miss proper descriptive examples. label Dec 29, 2024
@dhoerl-lp
Copy link
Author

dhoerl-lp commented Dec 30, 2024

I'm unable to reproduce this issue. Both examples trigger with version 0.57.1 on my side. With the option ignore_multiline_statement_conditions set to true there's no violation. Please double check.

You are correct. I tried to create a simple test file but in that case every attempt generates the error. While I had read of that new rule you mentioned, it's not in the documentation file, so I was unsure it was actually implemented (of course today on a deep search I see it added to the announcement of 0.57.0).

Can you re-use this ticket so that it gets added to https://realm.github.io/SwiftLint/rule-directory.html? If so I'll edit the title.

@SimplyDanny
Copy link
Collaborator

It's not a rule but an option of opening_brace. See its documentation.

@dhoerl-lp
Copy link
Author

dhoerl-lp commented Dec 30, 2024

It's not a rule but an option of opening_brace. See its documentation.

Documentation for those options would be super helpful, but again understand this is a volunteer effort.

Thank you for responses here and sorry that I wasted your time with the initial issue. I thought I had verified the one non-complain but obviously I had not.

@SimplyDanny
Copy link
Collaborator

Adding documentation for the various options of all rules is a long term effort, but definitely desired.

At the moment, though, there's no infrastructure prepared to do that.

@dhoerl-lp
Copy link
Author

Adding documentation for the various options of all rules is a long term effort, but definitely desired.

At the moment, though, there's no infrastructure prepared to do that.

It could get added to the page on open_braces itself. I looked but there is no way to contribute to the project. I'd gladly fund someone to add additional documentation on all those opens to open_brace, as this issue I reported has generated a lot of discussions in my company.

What we would really like is a way to only allow the brace to appear after the predictates on its own line. Turning on ignore_multiline_statement_conditions opens the door for much more.

Would it be unreasonable to request "allows_single_brace_on_its_own_line_for_multiline_predicates"? Could we fund that effort?

@SimplyDanny
Copy link
Collaborator

Adding documentation for the various options of all rules is a long term effort, but definitely desired.
At the moment, though, there's no infrastructure prepared to do that.

It could get added to the page on open_braces itself. I looked but there is no way to contribute to the project. I'd gladly fund someone to add additional documentation on all those opens to open_brace, as this issue I reported has generated a lot of discussions in my company.

The documentation is fully auto-generated. One cannot just add manual comments afterwards. That's why I mentioned the need for the right infrastructure first.

What we would really like is a way to only allow the brace to appear after the predictates on its own line. Turning on ignore_multiline_statement_conditions opens the door for much more.

Would it be unreasonable to request "allows_single_brace_on_its_own_line_for_multiline_predicates"? Could we fund that effort?

What would be the difference between ignore_multiline_statement_conditions and allows_single_brace_on_its_own_line_for_multiline_predicates? Is contrasted_opening_brace of any help?

@SimplyDanny SimplyDanny added help Questions or user problems that require more explanation rather than code changes. and removed repro-needed Issues that cannot be reproduced or miss proper descriptive examples. labels Dec 31, 2024
@SimplyDanny SimplyDanny reopened this Dec 31, 2024
@dhoerl-lp
Copy link
Author

dhoerl-lp commented Jan 1, 2025

Again, thank you so much for engaging with me in this ticket!

The contrasted_opening_brace is absolutely not what we want! Our coding style should allow:
if a {
do something
}

and disallow

if a
{
do something
}

The sole reason for ignore_multiline_statement_conditions is that we want he brace on a new line ONLY if there are more than one conditions in the for loop:

if
a,
b
{
do something
}

and probably

if a,
b
{
do something
}

Our issue with ignore_multiline_statement_conditions is we don't understand what other side effects setting this does. Will it let developers do crazy things if there multiline conditionals and a brace?

I didn't try but guess I could, is this now allowed

if
a,
b,
{
do something
}

Even the google style guide requires that open and close braces be at the same tab stop if the are preceded by a multiline conditional.

@SimplyDanny
Copy link
Collaborator

So your point is that you don't just want to ignore braces after multiline conditions but enforce them to be on their own line. Correct?

@SimplyDanny SimplyDanny added enhancement Ideas for improvements of existing features and rules. discussion Topics that cannot be categorized as bugs or enhancements yet. They require further discussions. and removed help Questions or user problems that require more explanation rather than code changes. labels Jan 2, 2025
@dhoerl-lp
Copy link
Author

So your point is that you don't just want to ignore braces after multiline conditions but enforce them to be on their own line. Correct?

Yes, but more - they must vertically align with the "if" statement.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion Topics that cannot be categorized as bugs or enhancements yet. They require further discussions. enhancement Ideas for improvements of existing features and rules.
Projects
None yet
Development

No branches or pull requests

2 participants