pinterest / ktlint

An anti-bikeshedding Kotlin linter with built-in formatter
https://pinterest.github.io/ktlint/
MIT License
6.23k stars 507 forks source link

Parameter wrapping should take priority over expression wrapping #2454

Closed mklnwt closed 10 months ago

mklnwt commented 11 months ago

function_signature_rule_force_multiline_when_parameter_count_greater_or_equal_than should take priority over function_signature_body_expression_wrapping.

This can save horizontal space at the cost of 1 line vertical space.

// Assume that the last allowed character is
// at the X character on the right                  X
fun foo(bar: String = "loooooooooong"): Foo = Foo.baz(a = "looooooooooooooooooooooooooooooooooooong", b = "looooooooooooooooooooooooooooooooooooong")

Expected Behavior

// Assume that the last allowed character is
// at the X character on the right                  X
fun foo(
    bar: String = "loooooooooong",
): Foo = Foo.baz(
    a = "looooooooooooooooooooooooooooooooooooong",
    b = "looooooooooooooooooooooooooooooooooooong",
)

Current Behavior

// Assume that the last allowed character is
// at the X character on the right                  X
fun foo(bar: String = "loooooooooong"): Foo =
    Foo.baz(
        a = "looooooooooooooooooooooooooooooooooooong",
        b = "looooooooooooooooooooooooooooooooooooong",
    )

Additional error:

Exceeded max line length (?) (cannot be auto-corrected) (standard:max-line-length)

Additional information

paul-dingemans commented 11 months ago

This can be achieved by setting ktlint_function_signature_body_expression_wrapping. See https://pinterest.github.io/ktlint/latest/rules/configuration-ktlint/#wrapping-the-expression-body-of-a-function

mklnwt commented 10 months ago

No setting of ktlint_function_signature_body_expression_wrapping achieves the expected result.

To clarify. For always it would make sense to first wrap the expression body.

For default (and probably multiline as well) it would make sense to let function_signature (including function_signature_rule_force_multiline_when_parameter_count_greater_or_equal_than) check the line length with expression body and wrap it if necessary.

Given this input:

// Assume that the last allowed character is
// at the X character on the right                  X
fun foo(
    bar: String = "loooooooooong",
): Foo = Foo.baz(
    a = "looooooooooooooooooooooooooooooooooooong",
    b = "looooooooooooooooooooooooooooooooooooong",
)

The parameter is joined first.

// Assume that the last allowed character is
// at the X character on the right                  X
fun foo(bar: String = "loooooooooong"): Foo = Foo.baz(
    a = "looooooooooooooooooooooooooooooooooooong",
    b = "looooooooooooooooooooooooooooooooooooong",
)

Then the expression is wrapped since it doesn't fit.

// Assume that the last allowed character is
// at the X character on the right                  X
fun foo(bar: String = "loooooooooong"): Foo =
    Foo.baz(
        a = "looooooooooooooooooooooooooooooooooooong",
        b = "looooooooooooooooooooooooooooooooooooong",
    )

Expected would be that the given should stay the same since joining it would make the line too long.

The same way the following should wrap the parameter before wrapping the expression body.

// Assume that the last allowed character is
// at the X character on the right                  X
fun foo(bar: String = "loooooooooong"): Foo = Foo.baz(a = "looooooooooooooooooooooooooooooooooooong", b = "looooooooooooooooooooooooooooooooooooong")
paul-dingemans commented 10 months ago

Let me investigate this once more.

paul-dingemans commented 10 months ago

Can you double check that you are using the configuration settings correctly? I see in this post several references to settings which are missing the ktlint_ prefix.

Given .editorconfig:

root = true

[*.{kt,kts}]
ktlint_code_style = ktlint_official
ktlint_function_signature_rule_force_multiline_when_parameter_count_greater_or_equal_than = 1
ktlint_function_signature_body_expression_wrapping = default
max_line_length = 53
ktlint_standard = enabled
ktlint_experimental = enabled

the code below:

// Assume that the last allowed character is
// at the X character on the right                  X
fun foo(bar: String = "loooooooooong"): Foo = Foo.baz(
    a = "looooooooooooooooooooooooooooooooooooong",
    b = "looooooooooooooooooooooooooooooooooooong",
)

is formatted as:

// Assume that the last allowed character is
// at the X character on the right                  X
fun foo(
    bar: String = "loooooooooong",
): Foo = Foo.baz(
    a = "looooooooooooooooooooooooooooooooooooong",
    b = "looooooooooooooooooooooooooooooooooooong",
)
mklnwt commented 10 months ago

I have not set ktlint_function_signature_rule_force_multiline_when_parameter_count_greater_or_equal_than = 1 and left it with the default of 2.

Imo it should still prefer to break the argument over breaking the expression since the line is too long.

paul-dingemans commented 10 months ago

On response to my suggestion:

This can be achieved by setting ktlint_function_signature_body_expression_wrapping. See https://pinterest.github.io/ktlint/latest/rules/configuration-ktlint/#wrapping-the-expression-body-of-a-function

you said that:

No setting of ktlint_function_signature_body_expression_wrapping achieves the expected result.

You can achieve what you want, by setting it to 1.

mklnwt commented 10 months ago

I'm trying to say that it should be wrapped based on line length, not because of the parameter number.

With the default setting (2) both of the following should be correctly formatted.

// Assume that the last allowed character is
// at the X character on the right                  X
fun foo(
    bar: String = "loooooooooong",
): Foo = Foo.baz(
    a = "looooooooooooooooooooooooooooooooooooong",
    b = "looooooooooooooooooooooooooooooooooooong",
)
// Assume that the last allowed character is
// at the X character on the right                  X
fun foo(bar: String = "short"): Foo = Foo.baz(
    a = "looooooooooooooooooooooooooooooooooooong",
    b = "looooooooooooooooooooooooooooooooooooong",
)
paul-dingemans commented 10 months ago

Please provide your .editorconfig settings.

When I use:

root = true

[*.{kt,kts}]
ktlint_code_style = intellij_idea
max_line_length = 53
ktlint_standard = enabled
ktlint_experimental = enabled

code is formatted as you requested. Note that max_line_length must be set explicitly when using code style intellij_idea.

mklnwt commented 10 months ago

Minimal example .editorconfig:

root = true

[*.{kt,kts}]
max_line_length = 53
ktlint_code_style = ktlint_official
ktlint_standard = enabled
ktlint_experimental = enabled
paul-dingemans commented 10 months ago

You need to overwrite ktlint_function_signature_body_expression_wrapping.

root = true

[*.{kt,kts}]
max_line_length = 53
ktlint_code_style = ktlint_official
ktlint_standard = enabled
ktlint_experimental = enabled
ktlint_function_signature_body_expression_wrapping = default
mklnwt commented 10 months ago

Actually while producing a minimal example I might've accidentally omitted that part and also forgot another crucial overwrite, Here is a complete picture that I'm now running on 1.1.1:

[*.{kt,kts}]
max_line_length = 53
ktlint_standard = enabled
ktlint_experimental = enabled
ktlint_code_style = ktlint_official
ktlint_function_signature_body_expression_wrapping = default
ktlint_standard_multiline-expression-wrapping = disabled
ktlint_standard_string-template-indent = disabled # depends on multiline-expression-wrapping

Still, my examples show violations:

// Assume that the last allowed character is
// at the X character on the right                  X
fun foo(
    bar: String = "loooooooooong",
): Foo = Foo.baz(
    a = "looooooooooooooooooooooooooooooooooooong",
    b = "looooooooooooooooooooooooooooooooooooong",
)

4:5 No whitespace expected between opening parenthesis and first parameter name (standard:function-signature) 4:35 No whitespace expected between last parameter and closing parenthesis (standard:function-signature) 5:10 Newline expected before expression body (standard:function-signature)

paul-dingemans commented 10 months ago

I still can not reproduce this. With .editorconfig and code sample above, I get following:

17:56:26.944 [main] INFO com.pinterest.ktlint.cli.internal.KtlintCommandLine -- Enable default patterns [**/*.kt, **/*.kts]
src/main/kotlin/Foo.kt:5:54: Exceeded max line length (53) (standard:max-line-length)
src/main/kotlin/Foo.kt:6:54: Exceeded max line length (53) (standard:max-line-length)

Summary error count (descending) by rule:
  standard:max-line-length: 2

But, I do note that your .editorconfig is missing line below which should be at the top of the file:

root = true

Without this property, you might actually pick-up an .editorconfig file at a higher level in your directory structure.

mklnwt commented 10 months ago

I tried it with and without root.

Did you run check or format? With format I get the same error as you, but the code is reformatted as stated above.

fun foo(bar: String = "loooooooooong"): Foo =
    Foo.baz(
        a = "looooooooooooooooooooooooooooooooooooong",
        b = "looooooooooooooooooooooooooooooooooooong",
    )
paul-dingemans commented 10 months ago

With format I get the same error as you, but the code is reformatted as stated above.

Did you mean "below" instead of "above"?

Please try to be more explicit, because I getting confused and need to guess. So please, specify:

mklnwt commented 10 months ago

Sorry for the confusion. I wrote tests instead.

If ktlint_function_signature_body_expression_wrapping is set to multiline or always, everything works as expected. If it is set to default, argument-list-wrapping should wrap regardless of the value of ktlint_function_signature_rule_force_multiline_when_parameter_count_greater_or_equal_than, because the remainder after = doesn't fit into the line.

After trying a few more examples, though, I think my expected cases would cause too much complexity. For this to work argument-list-wrapping & function_signature would have to interact with each other.

I think it's better to leave the issue closed, but for completeness sake and future reference I'll leave the expectation and test cases here.

Ideally both of the following would be allowed:

fun foo(
    bar: String
): Foo = Foo.baz(
    a = "looooooooooooooooooooong",
    b = "looooooooooooooooooooong"
)

fun foo(bar: String): String =
    Foo.baz(a = "a", b = "b")

.editorconfig

root = true

[*.{kt,kts}]
max_line_length = 35
ktlint_standard = enabled
ktlint_function_signature_body_expression_wrapping = default
ktlint_standard_multiline-expression-wrapping = disabled
ktlint_standard_string-template-indent = disabled # depends on multiline-expression-wrapping

Test cases

class ExampleTest {
    private val argumentListWrappingRuleAssertThat =
        assertThatRuleBuilder { ArgumentListWrappingRule() }
            .addRequiredRuleProviderDependenciesFrom(StandardRuleSetProvider())
            .assertThat()

    @Test
    fun `Expected signature_body_expression_wrapping=default (unformatted)`() {
        val code =
            """
            fun foo(bar: String): Foo = Foo.baz(a = "looooooooooooooooooooong", b = "looooooooooooooooooooong")
            """.trimIndent()
        val formattedCode =
            """
            fun foo(
                bar: String
            ): Foo = Foo.baz(
                a = "looooooooooooooooooooong",
                b = "looooooooooooooooooooong"
            )
            """.trimIndent()
        argumentListWrappingRuleAssertThat(code)
            .addAdditionalRuleProviders(
                { FunctionSignatureRule() },
                { IndentationRule() },
            ).withEditorConfigOverride(MAX_LINE_LENGTH_PROPERTY to 35)
            .withEditorConfigOverride(FUNCTION_BODY_EXPRESSION_WRAPPING_PROPERTY to "default")
            .isFormattedAs(formattedCode)
    }

    @Test
    fun `Expected signature_body_expression_wrapping=default (pre-formatted)`() {
        val code =
            """
            fun foo(
                bar: String
            ): Foo = Foo.baz(
                a = "looooooooooooooooooooong",
                b = "looooooooooooooooooooong"
            )
            """.trimIndent()
        argumentListWrappingRuleAssertThat(code)
            .addAdditionalRuleProviders(
                { FunctionSignatureRule() },
                { IndentationRule() },
            ).withEditorConfigOverride(MAX_LINE_LENGTH_PROPERTY to 35)
            .withEditorConfigOverride(FUNCTION_BODY_EXPRESSION_WRAPPING_PROPERTY to "default")
            .hasNoLintViolations()
    }

    @Test
    fun `Current signature_body_expression_wrapping=default (pre-formatted)`() {
        val code =
            """
            fun foo(
                bar: String
            ): Foo = Foo.baz(
                a = "looooooooooooooooooooong",
                b = "looooooooooooooooooooong"
            )
            """.trimIndent()
        val formattedCode =
            """
            fun foo(bar: String): Foo =
                Foo.baz(
                    a = "looooooooooooooooooooong",
                    b = "looooooooooooooooooooong"
                )
            """.trimIndent()
        argumentListWrappingRuleAssertThat(code)
            .addAdditionalRuleProviders(
                { FunctionSignatureRule() },
                { IndentationRule() },
            ).withEditorConfigOverride(MAX_LINE_LENGTH_PROPERTY to 35)
            .withEditorConfigOverride(FUNCTION_BODY_EXPRESSION_WRAPPING_PROPERTY to "default")
            .hasLintViolationsForAdditionalRule(
                LintViolation(2, 5, "No whitespace expected between opening parenthesis and first parameter name", true),
                LintViolation(2, 16, "No whitespace expected between last parameter and closing parenthesis", true),
                LintViolation(3, 10, "Newline expected before expression body", true),
            ).isFormattedAs(formattedCode)
    }

    @Test
    fun `Current & Expected signature_body_expression_wrapping=multiline`() {
        val code =
            """
            fun foo(
                bar: String
            ): Foo = Foo.baz(
                a = "looooooooooooooooooooong",
                b = "looooooooooooooooooooong"
            )
            """.trimIndent()
        val formattedCode =
            """
            fun foo(bar: String): Foo =
                Foo.baz(
                    a = "looooooooooooooooooooong",
                    b = "looooooooooooooooooooong"
                )
            """.trimIndent()
        argumentListWrappingRuleAssertThat(code)
            .addAdditionalRuleProviders(
                { FunctionSignatureRule() },
                { IndentationRule() },
            ).withEditorConfigOverride(MAX_LINE_LENGTH_PROPERTY to 35)
            .withEditorConfigOverride(FUNCTION_BODY_EXPRESSION_WRAPPING_PROPERTY to "multiline")
            .hasLintViolationsForAdditionalRule(
                LintViolation(2, 5, "No whitespace expected between opening parenthesis and first parameter name", true),
                LintViolation(2, 16, "No whitespace expected between last parameter and closing parenthesis", true),
                LintViolation(3, 10, "Newline expected before expression body", true),
            ).isFormattedAs(formattedCode)
    }

    @Test
    fun `Current & Expected where argument-list-wrapping would lead to additional breaks signature_body_expression_wrapping=default (pre-formatted)`() {
        val code =
            """
            fun foo(bar: String): String =
                Foo.baz(a = "a", b = "b")
            """.trimIndent()
        argumentListWrappingRuleAssertThat(code)
            .addAdditionalRuleProviders(
                { FunctionSignatureRule() },
                { IndentationRule() },
            ).withEditorConfigOverride(MAX_LINE_LENGTH_PROPERTY to 35)
            .withEditorConfigOverride(FUNCTION_BODY_EXPRESSION_WRAPPING_PROPERTY to "default")
            .hasNoLintViolations()
    }
}
paul-dingemans commented 8 months ago

It took a while before I had a chance to look at this again. It is not that difficult to achieve now you already provided as set of tests that would statisfy you. The argument-list-wrapping rule already had a dependency on the class-signature rule for more or less the same reason. This has now been added for the function-signature rule as well.