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

Forbid unfinished chained call in function header without using function-signature #2593

Closed pkubowicz closed 6 months ago

pkubowicz commented 7 months ago

I would like to have a rule forbidding code accepted with ktlint_function_signature_body_expression_wrapping=default:

// no readability, should be forbidden
fun someFunction(a: Any, b: Any): String = "some-result"
    .uppercase()

without doing other things function-signature does.

This is really horrible formatting from a cognitive load point of view. People scan code vertically (this is why we have line length limits). In the code above, "some-result" has lots of common with .uppercase() but the visual distance is big and there is no visual link guiding your eyes (it's not adjacent if you move your eyes horizontally or vertically). Neither of those 2 has much affinity to the function signature, but the visual layout forces the reader to look for some relationship with the function signature.

I know it's what the IntelliJ formatter does, so I am not asking of changing intellij_idea style.

I'm not able to set ktlint_function_signature_body_expression_wrapping=multiline because the way function-signature works makes it unacceptable to me.

Expected Behavior

Code above is rejected. Optionally, automatic formatting is applied, resulting in

// makes sense from readability point of view
fun someFunction(a: Any, b: Any): String =
    "some-result"
        .uppercase()

Current Behavior

ktlint_function_signature_body_expression_wrapping=multiline with function-signature rejects the bad code above, but I cannot accept enabling function-signature.

Problem with function-signature

function-signature makes functions behave like unstable substances, with 2 locally stable but very different states. Plus there is the 'butterfly effect', where a tiny change far away can cause radical change in local code.

Example:

fun otherFunction(a: SomeObject, b: String): String =
  a.one(SECRET_61).withUsedElement(b).baz(SECRET_69)

Now imagine that someone edits a different file (SomeObject.kt) and automatically renames withUsedElement to useIt. function-signature notices someFunction now fits into 100 line length limit. The current file is reformatted to look completely different:

fun otherFunction(a: SomeObject, b: String): String = a.one(SECRET_61).useIt(b).baz(SECRET_69)

Rename useIt back and you will start shotgun surgery returning you back to 2 lines.

Additional information

root=true
[*]
indent_style = space
indent_size = 4
max_line_length = 100

[{*.kt,*.kts}]
ktlint_code_style=intellij_idea
ktlint_function_signature_body_expression_wrapping = multiline
pkubowicz commented 7 months ago

Maybe what I want to forbid is only related to chaining in functions using expression body, and not as generic as function-signature, so deserves a new rule.

paul-dingemans commented 6 months ago

I have read your issues multiple times. I am puzzled what your problem is, and how to resolve it.

One thing that we seem to agree on, is that code below (ktlint_function_signature_body_expression_wrapping=default) is not well readable:

// no readability, should be forbidden
fun someFunction(a: Any, b: Any): String = "some-result"
    .uppercase()

To improve readability, the "some-result" is to be wrapped, resulting in code:

fun someFunction(a: Any, b: Any): String = 
    "some-result"
        .uppercase()

This is either done by ktlint_function_signature_body_expression_wrapping=multiline or ktlint_function_signature_body_expression_wrapping=always.

But this code is unacceptable to you. So what code would you have expected?

pkubowicz commented 6 months ago

If you read again the section 'Expected Behavior' at the top of the page, you will find exactly the same code you included in 'resulting in code'. I don't think we disagree on the code that is expected.

What I disagree with is the way to achieve this code.

If you show me a configuration that reformats someFunction in expected way while avoiding the butterfly effect in otherFunction(1), I will be more than happy. My opinion is that the current implementation of function-signature has unacceptable characteristics.

Those characteristics are: making function formatting overly sensitive to renames, with functions jumping from 2 lines to 1 and back. Adding noise to code: create 2 functions like otherFunction that are identical, but make 1 of them 5 characters longer (to cross line length boundary). function-signature will make those 2 almost identical functions wildly different: one will be 1-line, the other 2-line.

A formatting where line is broken after= is much less prone to unstability, as you get more 'buffer' before reaching max line length. But with the current implementation, I see no way to configure function-signature to avoid compressing the function into 1 line.

(1) I renamed the second function (originally also someFunction), to avoid confusion.

paul-dingemans commented 6 months ago

A formatting where line is broken after= is much less prone to unstability, as you get more 'buffer' before reaching max line length. But with the current implementation, I see no way to configure function-signature to avoid compressing the function into 1 line.

use ktlint_function_signature_body_expression_wrapping=always. But I do not expect you want to use that as multiline is already not acceptable.

I get the feeling that your expectations can never be met by Ktlint. Ktlint is not tracking what the user is doing. It looks on a file by file base at the content of the file, and then just lints/format that file without considering the context that caused the changed.

pkubowicz commented 6 months ago

Ok, I think ktlint_function_signature_body_expression_wrapping=always is the best solution for function-signature.