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

Rule: [function-signature] Function parameter wrapping behavior is overly aggressive and does not match documentation #2571

Closed ty1824 closed 7 months ago

ty1824 commented 7 months ago

To be clear up front as there were other issues closed without much investigation, there are two issues I am seeing. Firstly, the behavior does not match the documentation. Secondly, I believe the behavior itself is far too aggressive and should be curtailed to match the documentation.

Expected Behavior

According to the documentation for function-signature

Rewrites the function signature to a single line when possible (e.g. when not exceeding the max_line_length property) or a multiline signature otherwise.

The rule for parameter-list-wrapping isn't the issue in my case, but also mentions the same thing:

When class/function signature doesn't fit on a single line, each parameter must be on a separate line.

Given this, I would expect both of these functions to be formatted correctly based on a standard max_line_length:

fun foo(a: Int): Int {
    return a
}

fun bar(a: Int, b: Int): Int {
    return a - b
}

Observed Behavior

Any function with more than a single parameter is being reformatted to have multi-line parameters.

// This function is fine
fun foo(a: Int): Int {
    return a
}

// This function throws lint errors & reformats to multi-line
fun bar(a: Int, b: Int): Int {
    return a - b
}

// This is the expected formatting
fun bar2(
    a: Int,
    b: Int,
): Int {
    return a - b
}

IntelliJ Plugin:

Screenshot 2024-02-28 at 8 55 43 AM

Kotlinter output:

.../src/main/kotlin/Test.kt:1:9: Lint error > [standard:function-signature] Newline expected after opening parenthesis
.../src/main/kotlin/Test.kt:1:17: Lint error > [standard:function-signature] Parameter should start on a newline
.../src/main/kotlin/Test.kt:1:23: Lint error > [standard:function-signature] Newline expected before closing parenthesis

In this case, max_line_length is set to 140, the line length in question is 31 characters.

There are several other similar issues I noticed. This one was a similar (but not the same) documentation concern. This next one seems to have been given a workaround, but this definitely seems like unexpected behavior and was closed without being investigated.

Steps to Reproduce

Using the KtLint IntelliJ plugin (0.20.1) or Kotlinter Gradle plugin (4.2.0, with gradle version 8.6), write the code as described:

fun bar(a: Int, b: Int): Int {
    return a - b
}

.editorconfig:

[*.{kt,kts}]
max_line_length = 140
paul-dingemans commented 7 months ago

According to the documentation for function-signature

Rewrites the function signature to a single line when possible (e.g. when not exceeding the max_line_length property) or a multiline signature otherwise.

The rule for parameter-list-wrapping isn't the issue in my case, but also mentions the same thing:

When class/function signature doesn't fit on a single line, each parameter must be on a separate line.

The documentation is more extensive than you have quoted above. The rule description is indeed short and can be misleading. The code sample and description of the configuration parameters however clearly indicate that there line length is not the only thing that is taken into account. Screenshot 2024-02-28 at 20 04 04

Any function with more than a single parameter is being reformatted to have multi-line parameters.

Intended behavior depending on the code style. If you have not configured the code style, you will use ktlint_official code style which indeed starts wrapping functions having 2 or more parameters. If you have been using ktlint 0.x before, you might have missed the announcement in the changelog and documentation.

There are several other similar issues I noticed. https://github.com/pinterest/ktlint/issues/2296 was a similar (but not the same) documentation concern. https://github.com/pinterest/ktlint/issues/2424 seems to have been given a workaround, but this definitely seems like unexpected behavior and was closed without being investigated.

Each issue is investigated as careful as possible. If I see no further action to be taken on my side, I close the issue (which I also will do with this issue). If new information is provided, the investigation continues.

You're always welcome to submit a PR to improve the documentation or the code.

ty1824 commented 7 months ago

Sorry, I didn't notice the property description. The code sample actually doesn't clearly display this behavior and in fact adds to the confusion by showing the unset behavior without highlighting the new default behavior:

// Assume that the last allowed character is
// at the X character on the right           X
fun foooooooo(
    a: Any,
    b: Any,
    c: Any,
): String {
    // body
}

// Assume that the last allowed character is
// at the X character on the right           X
fun bar(a: Any, b: Any, c: Any): String {
    // body
}

It seems reasonable that the feature is configurable but I also believe these behaviors should be apparent in the rule description. Myself and other coworkers all were confused by this even after consulting the documentation (primarily because the code example matched the description but not the behavior of the linter).

paul-dingemans commented 7 months ago

Contributions to make it more clear are always welcome. It is always a struggle to decide what should be in the description, and what in the code sample. Basically almost all unit test could be added to the documentation as they all test some specific details. Problematic is that we currently have 2600+ unit test ;-)