Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unnecessary newline before "(" for nullable lambda definitions. #1255

Closed
ggajews opened this issue Oct 20, 2021 · 4 comments
Closed

Unnecessary newline before "(" for nullable lambda definitions. #1255

ggajews opened this issue Oct 20, 2021 · 4 comments

Comments

@ggajews
Copy link

ggajews commented Oct 20, 2021

Expected Behavior

No error reported

Observed Behavior

ktlint is formatting following line
var changesListener: ((width: Double?, depth: Double?, length: Double?, area: Double?) -> Unit)? = null
like this

var changesListener: (
        (
            width: Double?,
            depth: Double?,
            length: Double?,
            area: Double?
        ) -> Unit
    )? = null

but at the same time it reports an issue:
xxx.kt:21:9: Unnecessary newline before "("

Your Environment

  • Version of ktlint used: 0.42.1
  • Name and version (or code for custom task) of integration used (Gradle plugin, Maven plugin, command line, custom Gradle task): org.jlleitschuh.gradle:ktlint-gradle:10.2.0
@paul-dingemans
Copy link
Collaborator

I am not able to reproduce the problem with the given example. Also the output xxx.kt:21:9: Unnecessary newline before "(" refers to line 21 which I can not map on the example as it only has one line ;-)

Please put your sample code in a new file and run ktlint again on that file with options "--verbose --debug".

@ggajews
Copy link
Author

ggajews commented Oct 20, 2021

This is a minimal file where I see this issue:

package com.sample.test

class TestFormatting {

    var changesListener: (
        (
            width: Double?,
            depth: Double?,
            length: Double?,
            area: Double?
        ) -> Unit
    )? = null
}

TestFormatting.kt:6:8 Unnecessary newline before "("

I don't see anything unusual in the output

Loaded .editorconfig: [indent_style: space, indent_size: 4, end_of_line: lf, charset: utf-8, trim_trailing_whitespace: true, insert_final_newline: true, tab_width: 4]
Resolving .editorconfig files for {...}/TestFormatting.kt file path

For the reference pasting my .editorconfig:

# http://editorconfig.org
root = true

[*]
indent_style = space
indent_size = 2
end_of_line = lf
charset = utf-8
trim_trailing_whitespace = true
insert_final_newline = true

[*.{java,kt,kts,scala,rs,xml}]
indent_size = 4

[*.md]
trim_trailing_whitespace = false

[*.json]
insert_final_newline = false

@paul-dingemans
Copy link
Collaborator

Thanks. I can now reproduce the problem. The indent and parameter-list-wrapping are having a conflict about whether the newline is required. The indent rule is fired later then the parameter-list-wrapping and therefore determines the end result.

In order to resolve this, it should be ensured that rules only have one task and that no two rules exist that are doing the same task. In my opinion the wrapping functionality should be extracted from the indent, parameter-list-wrapping, argument-list-wrapping (and maybe from other rules as well). This however would require a more sophisticated way to be able to create dependencies between rules (see #1229).

@paul-dingemans
Copy link
Collaborator

In this case the parameter-list-wrapping rule is a bit too aggressive. Because the max-line-length is exceeded, this rule decides to move each parameter to a separate line but this should not have been applied to the parameters of the lambda as well. So the code should have been wrapped as follows:

var changesListener: (
    (width: Double?, depth: Double?, length: Double?, area: Double?) -> Unit
)? = null

This formatting is also accepted by the default IntelliJ IDEA formatting.

It becomes more interestingly when the lambda parameter would still exceed the max-line-lenght. In that case, I suggest to not wrap the rule (this strategy is used in other rules as well). Reason for this is that the default IntelliJ IDEA formatting is not able, to format this properly. For example:

var changesListener: (
    ( width: Double?,
      depth: Double?,
      length: Double?,
      area: Double?
    ) -> Unit
 )? = null

will be changed to code below by default IntelliJ IDEA:

var changesListener: (
   (
   width: Double?,
   depth: Double?,
   length: Double?,
   area: Double?
) -> Unit
)? = null

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants