pinterest / ktlint

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

Conflict between `binary-expression-wrapping` and `ktlint_standard_range-spacing` #2766

Closed mgroth0 closed 2 weeks ago

mgroth0 commented 1 month ago

Expected Behavior

I expect that ktlint_standard_range-spacing there will not be a conflict between binary-expression-wrapping and ktlint_standard_range-spacing. I am not sure what the best solution is.

Observed Behavior

If you have a very long expression with the .. operator, we get a conflict. binary-expression-wrapping tries to break it with a newline, and ktlint_standard_range-spacing then tries to remove that newline.

Steps to Reproduce

Here is the exact code that produced the issue for me:

range =
            (calculator as DoubleWrapperCalculator<U>).construct(value.start)..(calculator as DoubleWrapperCalculator<U>).construct(value.endInclusive)

Observe that we get an endless loop as each rule tries to fix the other.

Your Environment

paul-dingemans commented 1 month ago

I can not reproduce the problem. Note that the snippet as is, is not valid Kotlin code. The minimum to make it valid is to assign it to a variable, but I don't know how it is used in your context.

val range =
        (calculator as DoubleWrapperCalculator<U>).construct(value.start)..(calculator as DoubleWrapperCalculator<U>).construct(value.endInclusive)

With ktlint 1.3.1 this is formatted in one single go as:

val range =
    (calculator as DoubleWrapperCalculator<U>).construct(
        value.start,
    )..(calculator as DoubleWrapperCalculator<U>).construct(value.endInclusive)

given .editorconfig:

root = true

[*.{kt,kts}]
ktlint_code_style = ktlint_official
ktlint_standard_range-spacing = enabled
ktlint_standard_binary-expression-wrapping = enabled

Pleae provide additional details to make it reproducable.

mgroth0 commented 1 month ago

Sorry for not initially providing a valid reproducer. Please try this:

root = true

[*.{kt,kts}]
ktlint_standard = disabled
ktlint_standard_range-spacing = enabled
ktlint_standard_binary-expression-wrapping = enabled
val range =
    (calculator as DoubleWrapperCalculator<U>).construct(value.start)..(calculator as DoubleWrapperCalculator<U>).construct(value.endInclusive)

Note all other rules need to be disabled in order to reproduce this.

The output I get is:

WARN - com.pinterest.ktlint.rule.engine.internal.CodeFormatter - Format was not able to resolve all violations which (theoretically) can be autocorrected in file <stdin> in 3 consecutive runs of format.
paul-dingemans commented 4 weeks ago

Yes, now it is reproducable. Now I am only wondering, why you are using this specific ktlint configuration. I hope, it was for figuring out the problem only ;-)

When running in lint mode, the lint problem becomes clear:

Summary error count (descending) by rule: standard:binary-expression-wrapping: 1

* After manual fixing of the violation, the second run results in:

$ ktlint-1.3.1 --relative 11:48:57.225 [main] INFO com.pinterest.ktlint.cli.internal.KtlintCommandLine -- Enable default patterns [*/.kt, */.kts] src/main/kotlin/Foo.kt:2:72: Unexpected spacing after ".." (standard:range-spacing) 11:48:57.540 [main] WARN com.pinterest.ktlint.cli.internal.KtlintCommandLine -- Lint has found errors than can be autocorrected using 'ktlint --format'

Summary error count (descending) by rule: standard:range-spacing: 1


I wouldn't know how to autocorrect the problem generically. I would suggest to either refactor the code, or to suppress any of those rule for this specific statements. Examples:

val range = with(calculator as DoubleWrapperCalculator) { construct(value.start)..construct(value.endInclusive) }

@Suppress("ktlint:standard:binary-expression-wrapping") val range = (calculator as DoubleWrapperCalculator).construct(value.start)..(calculator as DoubleWrapperCalculator).construct(value.endInclusive)

@Suppress("ktlint:standard:range-spacing") val range = (calculator as DoubleWrapperCalculator).construct(value.start).. (calculator as DoubleWrapperCalculator).construct(value.endInclusive)```

mgroth0 commented 3 weeks ago

These aren't the only two rules I currently have enabled, but I just figured out this was the minimal reproducer. I still have many rules disabled as I am trying to "migrate" to ktlint formatting rules carefully and incrementally without destroying my old formatting (for example I like my horizontally aligned when statements, I don't care what anyone says!)

By the way, I found that I can fix this issue by also applying just one other rule: argument-list-wrapping. This rule is the one that breaks down the statement into multiple lines in such a way that satisfies both other rules.

If we were trying to eliminate all possible circularity issues, I'd say this is still unsolved. But I'm not sure if that is a realistic goal or if you have been trying to do that all, and personally my workaround of adding argument-list-wrapping works fine for me. So feel free to close if you want.

paul-dingemans commented 2 weeks ago

If we were trying to eliminate all possible circularity issues, I'd say this is still unsolved. But I'm not sure if that is a realistic goal or if you have been trying to do that all, and personally my workaround of adding argument-list-wrapping works fine for me.

It is not a goal to resolve all possible circulair violations. But whenever possible I try to resolve them. But in order to do so, I need a conflict resolution strategy that make things better. I will close the issue for now, until a good suggestion pops up.