pinterest / ktlint

An anti-bikeshedding Kotlin linter with built-in formatter
https://pinterest.github.io/ktlint/
MIT License
6.19k stars 505 forks source link

Separate lambda wrapping check from chain-method-continuation #2487

Closed mklnwt closed 9 months ago

mklnwt commented 9 months ago

Without chain-method-continuation the following code is 'valid'.

listOf(1, 2, 3).map {
    it * 2
}
    .filter {
        it > 2
    }

The wrapping after lambdas (i.e. .filter) should be checked in a separate rule from chain-method-continuation.

Expected Behavior

With chain-method-continuation disabled.

listOf(1, 2, 3).map {
    it * 2
}.filter {
    it > 2
}

Additional information

mklnwt commented 9 months ago

The point about allowing the first call to not be wrapper that I brought up in 2455 could also be relevant here.

The expected example save space over the current behavior of chain-method-continuation:

listOf(1, 2, 3)
    .map {
        it * 2
    }.filter {
        it > 2
    }

It allows for these chained lambda calls to all have one less indent.

paul-dingemans commented 9 months ago

Problem with code below:

fun foo() {
    listOf(1, 2, 3).map {
        it * 2
    }
        .filter {
            it > 2
        }
}

is related to the indent rule and not to chain-method-continuation. So there is no need to separate lambda wrapping check into separate rule.

I doubt whether I can or should solve this problem in the indent rule. I think this is a typical example where disabling some rules might have unintended side effects. Ktlint will never be able to ensure that any permutation of rules which are enabled/disabled will produce correct formatting.

mklnwt commented 9 months ago

It is indeed arguable. I would still say the rules should handle everything they're theoretically responsible for regardless of other rules as much as possible. I know sometimes it's not possible due to conflicts.

Since chain-method-continuation is still new/experimental I think this should be implemented in indent for now at least.

Furthermore should chain-method-continuation actually even check the indent in this case? It could just check that the method was chain wrapped and then let indent handle the check for the correct wrapping/indentation.

paul-dingemans commented 9 months ago

Furthermore should chain-method-continuation actually even check the indent in this case? It could just check that the method was chain wrapped and then let indent handle the check for the correct wrapping/indentation.

chain-method-continuation does not check/validate the indent. That is the responsibility of the indent rule.

Since chain-method-continuation is still new/experimental I think this should be implemented in indent for now at least.

As said, I doubt whether I can or should solve this problem in the indent rule. The indent rule for some reason accepts both statements below as ok:

fun foo1() {
    listOf(1, 2, 3).map {
        it * 2
    }.filter {
        it > 2
    }
}

fun foo2() {
    listOf(1, 2, 3).map {
        it * 2
    }
        .filter {
            it > 2
        }
}

We can have long debates about whether code should be allowed to be formatted as you request. In my opinion a chained method should have the . and ?. operators aligned in the same column. There is one exception to this rule that allows a small (configurable) number of operators to be on the same line.

I will have a look whether I can fix it in the indent rule, but I will not add much complexity to make this possible. One of the reasons for the chain-method-continuation rule is to resolve issues like this.

paul-dingemans commented 9 months ago

Problem with code below:

fun foo() {
    listOf(1, 2, 3).map {
        it * 2
    }
        .filter {
            it > 2
        }
}

is related to the indent rule and not to chain-method-continuation. So there is no need to separate lambda wrapping check into separate rule.

I doubt whether I can or should solve this problem in the indent rule. I think this is a typical example where disabling some rules might have unintended side effects. Ktlint will never be able to ensure that any permutation of rules which are enabled/disabled will produce correct formatting.

My analysis above is not entirely correct. The indent rule should never be responsible for removing the newline before .filter. The indentation of the last lambda is however unexpected. But actually it turns out that the indentation style below is compatible with default behavior of Intellij IDEA formatter:

val foo =
    listOf(1, 2, 3).map {
        it * 2
    }
        .filter {
            it > 2
        }

I have tried to fix it for the ktlint_official code style to code below, but failed at doing so with having a lot of unwanted side effects:

val foo =
    listOf(1, 2, 3).map {
        it * 2
    }
    .filter {
       it > 2
    }

As current formatting is consistent with default Intellij IDEA formatting, and the problem will be fixed when the chain-method-continuation rule is enabled, this problem will not be fixed by a separate fix in another rule.