chain-method-continuation should ignore nested classes

[mklnwt]

fun buildBar(): Foo.Bar = Foo.Bar.builder().baz().build()

Expected Behavior

fun buildBar(): Foo.Bar = Foo.Bar
    .builder()
    .baz()
    .build()

Current Behavior

fun buildBar(): Foo.Bar = Foo
    .Bar
    .builder()
    .baz()
    .build()

Additional information

• Current version of ktlint: 1.1.0

[paul-dingemans]

Not everything is related to the chain-method-continuation rule. Please run ktlint CLI to see which rule is causing 'troubles':

src/main/kotlin/Foo.kt:1:27: A multiline expression should start on a new line (standard:multiline-expression-wrapping)
src/main/kotlin/Foo.kt:1:27: Newline expected before expression body (standard:function-signature)

Usually, it is also helpful when you specify your .editorconfig settings.

Ktlint can not retrieve enough information from the AST / PSI to determine whether Bar is referring to a nested class inside Foo or something else. Both elements are considered to be an identifier.

[mklnwt]

I'm running it in the ChainMethodContinuationRuleTest.

I also added another case:

fun buildBar(): Foo.Bar = Foo
    .Bar
    .bazFactory
    .builder()
    .baz()
    .build()

Bar & bazFactory seem to be . -> REFERENCE_EXPRESSION -> IDENTIFIER.
builder, baz & build seem to be . -> CALL_EXPRESSION -> REFERENCE_EXPRESSION -> IDENTIFIER.

With this we can differentiate between methods and other references. Since this rule is called ChainMethodContinuation, we should probably ignore the other references.

In IntelliJ 'Chained function calls' with 'Chop down if too long' will wrap it as follows:

// Without 'wrap first call'
fun buildBar(): Foo.Bar =
    Foo.Bar.bazFactory.builder()
        .baz()
        .build()

// With 'wrap first call'
fun buildBar(): Foo.Bar =
    Foo.Bar.bazFactory
        .builder()
        .baz()
        .build()

It might make sense to add 'wrap first call' as configuration as well.
Then we'll have to consider making it 'do not wrap first call if it fits' though.

[mklnwt]

Some examples from IntelliJ (Code Style > Kotlin > Wrapping and Braces):

// Assume that the last
// allowed character is
// at the X character
// on the right              X
fun buildBar(): Foo.Bar = Foo.Bar.foo().bar().baz.foo().bar().baz()

// Without 'wrap first call'
fun buildBar(): Foo.Bar =
    Foo.Bar.foo()
        .bar().baz.foo()
        .bar()
        .baz()

// With 'wrap first call'
fun buildBar(): Foo.Bar =
    Foo.Bar.foo().bar().baz
        .foo()
        .bar()
        .baz()

and

// Assume that the last
// allowed character is
// at the X character
// on the right              X
fun buildBar(): Foo.Bar = Foo.Bar.baz.foo().bar().baz().foo.bar.baz.foo().bar().baz()

// Without 'wrap first call'
fun buildBar(): Foo.Bar =
    Foo.Bar.baz.foo()
        .bar()
        .baz().foo.bar.baz.foo()
        .bar()
        .baz()

fun buildBar(): Foo.Bar =
    Foo.Bar.baz
        .foo()
        .bar()
        .baz().foo.bar.baz
        .foo()
        .bar()
        .baz()

[mklnwt]

Maybe it makes sense to add a separate rule chain-property-continuation or similar as well.
That should run after chain-method-continuation.

Still the question about properties and classes remains. I could only think of using the first letter (if capital).

Lastly if we look at the second example above, would it make sense (and be possible) to only allow class/property access to not be chopped if it's the first within a mixed chain.
I.e. Foo.Bar.baz is allowed, but .baz().foo.bar.baz is not.

[paul-dingemans]

Maybe it makes sense to add a separate rule chain-property-continuation or similar as well. That should run after chain-method-continuation.

I do understand this mind jump. Do you care to explain?

Still the question about properties and classes remains. I could only think of using the first letter (if capital).

Technically it is simple to check the first letter of the identifier. But, would it also apply on constants like Foo.BAR.

Lastly if we look at the second example above, would it make sense (and be possible) to only allow class/property access to not be chopped if it's the first within a mixed chain. I.e. Foo.Bar.baz is allowed, but .baz().foo.bar.baz is not.

Mwah, not a fan of this.

For all options, I have troubles to understand what the rationale would be for this change. I want to avoid to end up with tends or even hundreds of configuration possibilities as this goes against the bikeshedding principle. The current rule might seem rigid, but it as least easy to understand when and when not the chain is wrapped.

[mklnwt]

I do understand this mind jump. Do you care to explain?

chain-method-continuation hint to only chopping chained methods and not other references. Adding another rule could make sense to allow for chopping other references as well.

Technically it is simple to check the first letter of the identifier. But, would it also apply on constants like Foo.BAR.

My point is to differentiate between properties and constant access (if you would count nested class access & enums as constant).

val x = Foo.BAR
    .chained()
    .method()
    .calls()

seems a lot nicer to me than

val x = Foo
    .BAR
    .chained()
    .method()
    .calls()

For all options, I have troubles to understand what the rationale would be for this change. I want to avoid to end up with tends or even hundreds of configuration possibilities as this goes against the bikeshedding principle. The current rule might seem rigid, but it as least easy to understand when and when not the chain is wrapped.

My two main concerns are:

1. There are some frameworks that generate nested classes with builders (e.g. GRPC/protobuf). With these it is quite common to have calls like FooOuterClass.Foo.newBuilder() followed by a chainned builder pattern. It's a lot nicer having the static calls (and potentially the first method call) sticking together. Same goes for the enum example above.
2. Sticking a bit closer to IntelliJ. I know that ktlint_official has stopped adhering to IntelliJ default formatter, but matching this rule with IntelliJ seems natural to me. Then with an additional rule for other references, the current result could be maintained at the same time.

[mklnwt]

Leaving aside the 'wrap first call', I think it's probably good to have a differentiation between methods and properties. As the current rule name suggests, it's for methods.

This might also be necessary for the intellij style.
But then also the default of IntelliJ is to 'chop down if too long' and not 'wrap first call'.

Having two rules might make dealing with a mix of methods and properties a bit tricky though.

Apart from the differentiation between methods and properties, I would also suggest again to check properties (or non-method) for first capital letter. It would be neat to not wrap static value access (including enums).

[paul-dingemans]

Leaving aside the 'wrap first call', I think it's probably good to have a differentiation between methods and properties. As the current rule name suggests, it's for methods.

This is indeed a valid reasoning to ignore simple reference expressions by this rule. At this moment, I think of a simple reference expression as it is preceded by another reference expression. Note that in example below the .baz is still wrapped as it is preceded by a function/method call:

    @Test
    fun `Issue 2455 - Given a chained method including some simple reference expressions then do not wrap simple reference expressions`() {
        val code =
            """
            // $MAX_LINE_LENGTH_MARKER      $EOL_CHAR
            fun buildBar(): Foo.Bar = Foo.Bar.builder().baz().baz.build()
            """.trimIndent()
        val formattedCode =
            """
            // $MAX_LINE_LENGTH_MARKER      $EOL_CHAR
            fun buildBar(): Foo.Bar = Foo.Bar
                .builder()
                .baz()
                .baz
                .build()
            """.trimIndent()
        chainMethodContinuationRuleAssertThat(code)
            .setMaxLineLength()
            .addAdditionalRuleProvider { MaxLineLengthRule() }
            .hasLintViolations(
                LintViolation(2, 34, "Expected newline before '.'"),
                LintViolation(2, 44, "Expected newline before '.'"),
                LintViolation(2, 50, "Expected newline before '.'"),
                LintViolation(2, 54, "Expected newline before '.'"),
            ).isFormattedAs(formattedCode)
    }