chain-method-continuation wrapping fully qualified function/constructor calls

[bcmedeiros]

Consider the following code:

fun foo(): Foo {
    return dev.foo.p1.p2.Foo("bar")
}

Expected Behavior

Code should be accepted as is.

Observed Behavior

chain-method-continuation rule fails.

Steps to Reproduce

#2796

[paul-dingemans]

Please see docs for configuration of the rule. Expression will be wrapped when the configured number of operators is reached. In case you do not want that, please set it to some high number (Integer.MAX for example).

So there is no need for fixing this.

[bcmedeiros]

I'm aware of the configuration.

Those are not operators, though, it's just a fully qualified package. Not sure if my example is clear enough.

[paul-dingemans]

Maybe this is some semantic interpretation that we are not aligned on. For me each .something is a dot-operation. So for me the chain starting with dev is followed by 3 'static' operators .foo, .p1, .p2 and 1 constructor call Foo("bar"). So 4 operators in total.

[bcmedeiros]

There's a clear semantic difference between my example and the ones the rule is supposed to affect.

The rule is called chain-method-continuation and those dots in my example are not calling any methods, they are just the notation used by the language when you don't import a class but rather refers to that class inline by its fully-qualified name.

fun foo(): Foo {
    return dev.foo.p1.p2.Foo("bar")
}

is the same as

import dev.foo.p1.p2.Foo

fun foo(): Foo {
    return Foo("bar")
}

I don't see why you are referring to the full package notation as "operators", they don't do anything but qualify the class reference. If those are operators, are the dots in import dev.foo.p1.p2.Foo also operators? Should the rule wrap them?

[bcmedeiros]

Just posting a real-world example (with some redacted words) is case that helps:

// we have lots of those, and we have a rule to have one operator per line when there's more than 3 operators.
// chain-method-continuation is a perfect rule to enforce this, and we would love to adopt it
val events = eventStore
    .withStreamType(CarStreamType)
    .loadStream(id)
    .map { it.event }
    .filter { it.producer == 1L }
    .sumBy { it.purchaseValue }


// This is also a very common pattern in one of my projects, we have classes with the same name 
// and we need to qualify both sides in the converter code only
// This code becomes just horrible if we consider packages as "operators" as the previous messages suggest
fun protocol.client.Game1Command.toDomain(): studio.ls.game1.package1.package2.command.Game1Command = when (this) {
    is protocol.client.CreateGameCommand -> studio.ls.game1.package1.package2.command.CreateGameCommand
    is protocol.client.StartCommand -> studio.ls.game1.package1.package2.command.StartCommand
    is protocol.client.PauseCommand -> studio.ls.game1.package1.package2.command.PauseCommand
    is protocol.client.ResumeCommand -> studio.ls.game1.package1.package2.command.ResumeCommand
    is protocol.client.EndGameCommand -> studio.ls.game1.package1.package2.command.EndGameCommand
}

If chain-method-continuation doesn't have a different handling for packages and just pretend they are method calls, I will have no other option other than disabling this rule, which is really sad because I really like it for real method chains.

[paul-dingemans]

It would have helped if you would have specified that you are running on an old ktlint version. In ktlint 1.3.1 this has been resolved.

With .editorconfig below:

root = true

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

your code sample (plus one addition to show that it does not resolve all) is formatted by 1.3.1 as follows:

// we have lots of those, and we have a rule to have one operator per line when there's more than 3 operators.
// chain-method-continuation is a perfect rule to enforce this, and we would love to adopt it
val events =
    eventStore
        .withStreamType(CarStreamType)
        .loadStream(id)
        .map { it.event }
        .filter { it.producer == 1L }
        .sumBy { it.purchaseValue }

// This is also a very common pattern in one of my projects, we have classes with the same name
// and we need to qualify both sides in the converter code only
// This code becomes just horrible if we consider packages as "operators" as the previous messages suggest
fun protocol.client.Game1Command.toDomain(): studio.ls.game1.package1.package2.command.Game1Command =
    when (this) {
        is protocol.client.CreateGameCommand -> studio.ls.game1.package1.package2.command.CreateGameCommand
        is protocol.client.StartCommand -> studio.ls.game1.package1.package2.command.StartCommand
        is protocol.client.PauseCommand -> studio.ls.game1.package1.package2.command.PauseCommand
        is protocol.client.ResumeCommand -> studio.ls.game1.package1.package2.command.ResumeCommand
        is protocol.client.EndGameCommand -> studio.ls.game1.package1.package2.command.EndGameCommand
    }

fun protocol.client.Game1Command.toDomain2(): studio.ls.game1.package1.package2.command.Game1Command =
    studio.ls.game1.package1.package2.command
        .Game1Command()

So the constructor call is still wrapped. I will have a look to see whether I can identify chains having only one method/constructor call and no other operators like map and filter to be excluded from wrapping.

[bcmedeiros]

My draft PR reproduces the issue against the master branch.

I actually picked not the best block, most of the conversions are with data classes, not data objects, so they actually all wrap

So the constructor call is still wrapped. I will have a look to see whether I can identify chains having only one method/constructor call and no other operators like map and filter to be excluded from wrapping.

Thank you. let me know if I can help.

[bcmedeiros]

9a10ec0 seems to be spot-on, @paul-dingemans!
Any planned releases? I don't exactly know how to test this before a release.

[paul-dingemans]

The change is not yet ready. It has some side effects that I don't like. Once PR is raised and merged a snapshot build will be published.

I do not expect to release before mid of October.

[paul-dingemans]

Unfortunately I am stuck. I can not identify the difference between fully qualified function/constructor calls versus nested property access.

The AST representention of the properties bar2 and baz2 in code sample below is identical beside the actual values of the identifier of course:

private val bar2 = bar.bar1.Bar2()

private data class Baz(val baz1: Baz1)
private class Baz1 {
    fun baz2() = "baz2"
}
private val baz = Baz(Baz1())

private val baz2 = baz.baz1.baz2()

bar.bar1 are package identifiers, while baz.baz1 are property identifiers. The AST representation of both is:

• DOT_QUALIFIED_EXPRESSION (bar.bar1.Bar2() or baz.baz1.baz2())
  • DOT_QUALIFIED_EXPRESSION (bar.bar1 or baz.baz1)
    • REFERENCE_EXPRESSION (bar or baz)
      • PsiElement(IDENTIFIER) (bar or baz)
    • PsiElement(DOT) (.)
    • REFERENCE_EXPRESSION (bar1 or baz1)
      • PsiElement(IDENTIFIER) (bar1 or baz1)
  • PsiElement(DOT) (.)
  • CALL_EXPRESSION (Bar2() or baz2())
    • PsiElement(IDENTIFIER) (Bar2 or baz2)
    • VALUE_ARGUMENT_LIST (())
      • PsiElement(LPAR) (()
      • PsiElement(RPAR) ())

As no distinction can be made based on the AST, I cannot exclude fully qualified function/constructor calls from this rule.

[bcmedeiros]

Idea: could we check if there is an import for leaf node? Assuming this is possible as ktlint is able to figure out unused imports, we could do the following:

• If the leaf node has an import, do the current behavior
• If not, then we prevent the line breaks to take place

[paul-dingemans]

Idea: could we check if there is an import for leaf node? Assuming this is possible as ktlint is able to figure out unused imports, we could do the following:

• If the leaf node has an import, do the current behavior
• If not, then we prevent the line breaks to take place

Tnx for the suggestion. I fail to understand what you mean. Can you write some unit tests that demo what you mean?