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

experimental:multiline-if-else treats all if statements as multiline #812

Closed
pkubowicz opened this issue Jul 13, 2020 · 11 comments · Fixed by #1893
Closed

experimental:multiline-if-else treats all if statements as multiline #812

pkubowicz opened this issue Jul 13, 2020 · 11 comments · Fixed by #1893

Comments

@pkubowicz
Copy link

Expected Behavior

MultiLineIfElseRule.kt respects its own JavaDoc, i.e. quoting Kotlin Coding Conventions:

If the condition of an if or when statement is multiline, always use curly braces around the body of the statement.

Observed Behavior

The current code considers any if statement followed by a newline as needing curly braces.

Steps to Reproduce

        if (System.getenv("foo") != null)
            println("ok")

Your Environment

  • Version of ktlint used: 0.37.2
  • Name and version (or code for custom task) of integration used (Gradle plugin, Maven plugin, command line, custom Gradle task): Gradle plugin
  • Version of Gradle used (if applicable): 6.5.1
  • Operating System and version: Linux 5.4.0-40-generic amd64
  • Link to your project (if it's a public repository):
@romtsn
Copy link
Collaborator

romtsn commented Jul 20, 2020

We shouldn't change this when the --android option is passed though, since it is marked as wrong there. Honestly, I'm in favour of Android styleguide in this case, not sure why kotlin's one does not clearly define anything here

@BraisGabin
Copy link

Honestly, I'm in favour of Android styleguide in this case, not sure why kotlin's one does not clearly define anything here

Same here. It's a common practice in several languages.

@pkubowicz
Copy link
Author

It's a common practice in several languages.

In languages that have C-like syntax. Kotlin has an option to write code in a more functional style.

I don't want to explain Kotlin team decisions as I don't have any knowledge about their reasoning, I may only guess. Take a function:

fun createFoo(arg: Int) =
    "a".repeat(arg)

It's short, consistent and does not use any braces. Now let's evolve this function by making its behaviour dependent on the argument value:

fun createFoo(arg: Int) =
    if (arg < 5)
        throw IllegalArgumentException("Too long to keep the whole method body in 1 line")
    else
        "a".repeat(arg)

One can argue that adding braces to the if statement would make this simple function no longer consistent, no longer functional style but rather having an imperative smell:

fun createFoo(arg: Int) =
    if (arg < 5) {
        throw IllegalArgumentException("Too long to keep whole method body in 1 line")
    } else {
        "a".repeat(arg)
    }

Maybe yes, maybe no – I'm not a functional purist. One can also argue that it's weird to have braces in a function which body does not have braces - there is no closing brace on "level 0". For people used to Java braces style it looks like the code should not compile because there is a closing brace missing.

@romtsn
Copy link
Collaborator

romtsn commented Aug 13, 2020

Created an issue to get JB's opinion on that (hopefully with the styleguide update) https://youtrack.jetbrains.com/issue/KT-41070

@Kantis
Copy link
Contributor

Kantis commented Jul 1, 2022

My 2 cents is that I don't think this is something that needs to be strictly linted. I think the main benefit of using Ktlint is to get rid of formatting changes happening back and forth in PRs which just contribute noise. In this case there's not gonna be any auto formatting, and it can be left to the programmer to decide what best fits the use case.

So my opinion is that all can be considered correct.

if (condition) 1 else 0 // correct

if (condition) // correct
    1
else
    0

if (condition) // correct
    1

if (condition) { // correct
    1
} else {
    0
}

@paul-dingemans
Copy link
Collaborator

The new ktlint_official code style contains rules that:

  • enforces a multline if statement to wrap each branch in a block
  • disallows a single line if statement to have more than one else branch
  • disallows a single line if statement to wrap a branch in a block
if (condition) 1 else 0 // allowed

if (condition) { 1 } else 0 // disallowed

if (condition) 1 else if (otherCondition) 2 else 0 // disallowed

if (condition) // disallowed
    1
else
    0

if (condition) // disallowed
    1

if (condition) { // allowed
    1
} else {
    0
}

paul-dingemans added a commit that referenced this issue Mar 26, 2023
Enforces that single line if-statements are kept simple. A single line if-statement is allowed only when it has at most one else branch. Also, the branches of such an if statement may not be wrapped in a block.

Closes #812

Add discouraged comment locations to keep rule simpler.
paul-dingemans added a commit that referenced this issue Mar 26, 2023
* Add rule `if-else-wrapping`

Enforces that single line if-statements are kept simple. A single line if-statement is allowed only when it has at most one else branch. Also, the branches of such an if statement may not be wrapped in a block.

Closes #812

* Add discouraged comment locations to keep rule simpler.
@Kantis
Copy link
Contributor

Kantis commented Apr 21, 2023

@paul-dingemans will this still be allowed?

if (condition) 1
else 0

@paul-dingemans
Copy link
Collaborator

No, it is a multiline statement. But if (condition) 1 else 0 is accepted.

@spacemase
Copy link

spacemase commented May 9, 2023

So how does one correctly format this code from above?

fun createFoo(arg: Int) =
    if (arg < 5)
        throw IllegalArgumentException("Too long to keep the whole method body in 1 line")
    else
        "a".repeat(arg)

or worse:

createFoo(arg = 
    if (bar < 5) 
        throw IllegalArgumentException("Too long to keep the conditional on 1 line")
    else 
        0
)

@paul-dingemans
Copy link
Collaborator

It depends on your configuration. But running Ktlint CLI (ktlint -F) will show you, what the expected format is.

Given .editorconfig below:

root = true

[{*.kt,*.kts}]
ktlint_code_style = ktlint_official
ktlint_standard = enabled
ktlint_experimental = enabled

the code should be formatted as:

fun createFoo(arg: Int) =
    if (arg < 5) {
        throw IllegalArgumentException("Too long to keep the whole method body in 1 line")
    } else {
        "a".repeat(arg)
    }

fun bar() {
    createFoo(
        arg =
            if (bar < 5) {
                throw IllegalArgumentException("Too long to keep the conditional on 1 line")
            } else {
                0
            },
    )
}

(I have wrapped the second example into function bar() as code is otherwise can not be processed by embedded compiler)

@spacemase
Copy link

I did not know the -F flag! THANK YOU for replying on this old thread. Addressing this scenario put me at the peak of frustration.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants