chain-method-continuation shouldn't trigger on property access with only single method call

[FyiurAmron]

related to #2455 , mirroring the idea from #2455 (comment) somewhat; results from IMVHO excessive application of chain-method-continuation, coupled with the default value of https://pinterest.github.io/ktlint/1.0.0/rules/configuration-ktlint/#force-multiline-chained-methods-based-on-number-of-chain-operators

A practical example, single println (please ignore the fact it doesn't use string template : ) :

println("ver: " + libs.versions.our.lib.get())

Expected Behavior

Line is left verbatim. Length is OK, the property access via Gradle version catalog accessor it perfectly OK, and the getter for the Property<String> is unfortunately necessary due to Gradle's internals (no good toString() there) - however since we do not have a method chain (only a single explicit method is called, the rest is either accessors or fields or whatever, no () anyway), no chain-method-continuation rule should trigger - since there is no chained method nor any continuation happening)

Observed Behavior

a waterfall of tragedies follows:

1. first, the chain rule triggers, so it gets reformatted to

println("ver: " + libs.versions.our.lib
    .get())

2. then, the wrapping and multiline-expression-wrapping rules trigger, resulting in

println(
    "ver: " +
        libs.versions.our.lib
            .get(),
)

It's now a lot harder to read and handle - and, let's be honest - in total that's a real overkill, that could be safely prevented by a tweak to the chain-method-continuation logic :)

Steps to Reproduce

Take any expression that has >= 3 property etc. accesses (even with very short names!), and terminate with a method call.

Note that this won't happen for pure property access, nor for <= 2 property accesses like libs.versieeeeeons.ooooooooour.getLibrary()

Proposed solution

Don't treat a stream of tokens as a "chained method call" if only a single () method is called at the end.

Your Environment

ktlint ruleset 1.5.0

[paul-dingemans]

The name of this rule is not great. You're absolute right that it does take more elements into account than methods only. I also do acknowledge that code like:

println("ver: " + libs.versions.our.lib.get())

is more readable than

println(
    "ver: " +
        libs.versions.our.lib
            .get(),
)

Don't treat a stream of tokens as a "chained method call" if only a single () method is called at the end.

Thnx for the suggestion. Most likely it is a simple change. But I do think that it, once implemented, opens up a whole can of worms, and requests for more exceptions based on other specific examples in which the formatting reduces readability. I think it will be possible to create similar examples that produce code that is less readable for chains like libs.getLibrary().something.other or getLibrary().or.something.else.

The real question is whether the construct libs.versieeeeeons.ooooooooour.getLibrary() is used heavily in lots of code bases or just occassionally. I haven't notice it in my own code bases, but I might not see it as it apparently doesn't bother me.

If it just happens occasionally, I would say that either you rewrite the code a little, or just suppress the rule for a statement/file/directory etc.

I won't close the issue for now. Now I have seen your problem, I might not be able to unseen it in my own code bases anymore ;-)

[FyiurAmron]

Thnx for the suggestion. Most likely it is a simple change. But I do think that it, once implemented, opens up a whole can of worms, and requests for more exceptions based on other specific examples in which the formatting reduces readability.

I would only request and advocate for this particular simple case for precisely the reason you provided. Tail call in property chain is unfortunately quite common in Gradle Kotlin DSL, due to https://docs.gradle.org/current/javadoc/org/gradle/api/provider/Provider.html#get() and due to how Gradle Version Catalog DSL is generated. Also, it's a relatively common idiom by itself in OOP augmented by property pattern (e.g. in Kotlin), basically the extension of noun.verb() - since otherwise readability concerns would suggest splitting the chain into separate vars.

I think it will be possible to create similar examples that produce code that is less readable for chains like libs.getLibrary().something.other or getLibrary().or.something.else.

See above. noun.verb() is the idiom. The noun can be multi-part and it doesn't really increase cognitive effort. car.engine.start() is completely readable. Any other syntax or multiple verbs however fails the "noun-verb" parsing. I'd say the first one should be refactored to val lib = libs.getLibrary() /* */ lib.something.other and the 2nd one to val lib = getLibrary() /* */ lib.or.something.else - however, the original case I presented still holds (single verb at the end of the chain). I'd say those cases are rare enough to warrant refactoring via split anyway.

The real question is whether the construct libs.versieeeeeons.ooooooooour.getLibrary() is used heavily in lots of code bases or just occassionally. I haven't notice it in my own code bases, but I might not see it as it apparently doesn't bother me.

libs.versieeeeeons.ooooooooour.getLibrary() doesn't trigger this, that's my point here. It's lucky enough to be only 4 parts, and the trigger is on 5 :D

If it just happens occasionally, I would say that either you rewrite the code a little, or just suppress the rule for a statement/file/directory etc.

in this case, the "simplest" solution for me would be to just use a string template - but then readability suffers, I really hate to use templates for such complex expressions, I think they work best for top-level/local vars, single phrase with no dots etc.

I won't close the issue for now. Now I have seen your problem, I might not be able to unseen it in my own code bases anymore ;-)

TBH I still hope my noun-verb point above can convince you at least a bit :D

[paul-dingemans]

I won't close the issue for now. Now I have seen your problem, I might not be able to unseen it in my own code bases anymore ;-)

TBH I still hope my noun-verb point above can convince you at least a bit :D

Well at least I am very interested in what you call the noun-verb pattern, or property pattern (e.g. in Kotlin). I couldn't find any references to this pattern though. I am aware of the Gradle Kotlin DSL with the 'get` function (https://docs.gradle.org/current/javadoc/org/gradle/api/provider/Provider.html#get()).

The real question is whether the construct libs.versieeeeeons.ooooooooour.getLibrary() is used heavily in lots of code bases or just occassionally. I haven't notice it in my own code bases, but I might not see it as it apparently doesn't bother me.

libs.versieeeeeons.ooooooooour.getLibrary() doesn't trigger this, that's my point here. It's lucky enough to be only 4 parts, and the trigger is on 5 :D

Okay fair, I used a dot too less. The point that I tried making whether the construct for which you want an exc exception, e.g. my.personal.libs.versieeeeeons.ooooooooour.getLibrary() is used that much.

Suppose that this exception would be build in, I can imagine that the next request will be to also make an exception for construct below.

some.deeply.nested.collection.first { it.name == "ktlint" }

In this example first is definitely not a verb in natural language although construct above can be written as:

some.deeply.nested.collection.first({ it.name == "ktlint" })

What do you think about this?

[FyiurAmron]

Well at least I am very interested in what you call the noun-verb pattern

Depends on the context. Some would refer to it as "noun-verb paradigm", https://www.usabilityfirst.com/glossary/noun-verb-paradigm/index.html , but actually it's called https://en.wikipedia.org/wiki/Subject%E2%80%93verb%E2%80%93object_word_order . It's used by most natural languages used by humans, and thus is assumed to be the easiest to read and understand by most people. horse.eat(apples) is a direct translation to OOP paradigm, which is then assumed by OOP textbooks as the common expression style. It's also referenced as "the OOP approach of strictly prioritizing things (objects/nouns) before actions (methods/verbs)" (some first random reference from Wikipedia :) - that aside, most textbooks just assume this as common knowledge, and focus on the class-noun and method-verb isomorphism.

As to why just a single one -
https://guidohenkel.com/2022/06/zen-of-defensive-programming-one-operation-per-line/
#1078
Generic.Formatting.DisallowMultipleStatements.SameLine says that Each PHP statement must be on a line by itself
etc. etc. (I think all languages have this inspection in IDEs right now enabled by default in most code styles.

When you combine them, you get the arguably most readable and understandable structure used by OOP language code. Not my invention, but a description of patterns already in use for decades. Dunno if there's any more "specific" name for it or more "strict" reference, I just seen it so many times I never really wondered too much about it TBH.

What do you think about this?

I completely understand your point and agree that this can be seen as a slippery slope. Yet, while a lambda expression of any kind is "complex", the function can understandably always take complex input, even a nested one. That nesting would trigger reformatting of its own under ktlint rules I think, so I'm not seeing any real risk here, it would turn multi-line etc. if the length or nesting would exceed the limits, right?

Alternatively, maybe just treat a 0-arg method call like a property access? They are actually the same, there's no compiler or semantic difference between cat.color and cat.getColor() I think?

[paul-dingemans]

Thanks for the explanations. I keep it in consideration for a while.