Nested function calls with named parameters is incredibly difficult to read and results in unnecessary nesting

[snowe2010]

Given an input like:

fun f() {
    val request =
        namedArgsMethodLevel1(
            nestedParam1Level1 = namedArgsMethod1Level2(),
            nestedParam2Level1 = namedArgsMethod2Level2(
                nestedParam1Level2 = namedArgsMethodLevel3(
                    nestedParam1Level3 = AnEnum.Foo,
                    nestedParam2Level3 = listOf(
                        namedArgsMethodLevel4(
                            nestedParam1Level4 = AnEnum.Foo,
                            nestedParam2Level4 = "",
                            nestedParam3Level4 = 1,
                        ),
                    ),
                ),
            )
        )
}

ktfmt reformats it as:

fun f() {
    val request =
        namedArgsMethodLevel1(
            nestedParam1Level1 = namedArgsMethod1Level2(),
            nestedParam2Level1 =
                namedArgsMethod2Level2(
                    nestedParam1Level2 =
                        namedArgsMethodLevel3(
                            nestedParam1Level3 = AnEnum.Foo,
                            nestedParam2Level3 =
                                listOf(
                                    namedArgsMethodLevel4(
                                        nestedParam1Level4 = AnEnum.Foo,
                                        nestedParam2Level4 = "",
                                        nestedParam3Level4 = 1,
                                    ),
                                ),
                        ),
                ))
}

(depending on line length)

This is incredibly hard to read, and incredibly hard to track what goes where, whereas the original is quite easy to parse, just treat the () as {} and it reads just like a DSL. With the formatting changes in the current version of ktfmt, it nests too deeply to be legible. This is currently a major problem in our codebase, and my teammates are getting incredibly frustrated with ktfmt.

I would like to submit a PR, but I do not want to submit it without an accompanying discussion alongside the PR. From looking over the code, this decision was on purpose. I don't know why though. I see the original commit adding this logic was c57a9b2c95695a46cf1e4f3a4efb885f99b6b4c2 and was done by @cgrushko and reviewed by @strulovich. If they could chime in on the reasoning for the original logic it would be appreciated.

I can post some examples of what the current changes I have made do to the code in the tests, if that's desired.

[nreid260]

The main reason for this is that ktfmt is based on google-java-format, which implicitly implements the "rectangle rule". It's possible to override this rule, but it's a constant fight against the underlying formatting algorithm.

Case and point, we do break the rectangle rule for "scope functions", and there's a ton of special case logic to handle the switch between block-like vs expression formatting, and edge cases like trailing comments.

val foo = someFun {
  // I'm violating the rectangle rule
  // `val foo = ` is on the same line as my opening `{`, and my closing `}` is misaligned
}

[snowe2010]

@nreid260 hm. it looks like it was strictly coded in ktfmt to act different than google-java-format does automatically. The only test that 'looks bad' is the Arguments are blocks test, and it ends up looking like this (left side is current code):

The rest look much easier to read (IMO):

Note that the right side I'm pretty sure is the default logic from google-java-format after I removed the code that explicitly adds a newline for named arguments.

So from judging the code it looks like it would be trading one piece of code for another. We'd have to remove the code that's explicitly adding newlines for named args, but then we'd have to fix the case of Arguments are blocks where we've got an expression that doesn't really follow the rectangle rule.

Would you and the rest of the contributors be opposed to me submitting a PR to make this change? If we still need to have more discussion about all the cases this would affect I can come up with a list. I tried doing that last night, but markdown/html tables with code in them on Github are just terrible so I gave up.

[nreid260]

I agree with your assessment which cases look better; the first case is by far the most common though so we need to make sure that one looks good too.

I recall trying to add support for block-like scope functions to named args, but the PR died. One issue was the way the spread operator (e.g. *arr) is modeled in the AST.

It would be wise to add some tests for those cases as well. Also put comments in all the tests all over the place. Many times I've had PRs that seemed great, until I put a comment in the test code, and it ended up wrong.

[snowe2010]

@nreid260 ah ok, that's good to know. Yeah it seems like my change should have broken more tests so I definitely will add more tests. And the note about the comments is a good one. It does seem like a significant portion of ktfmt is coding to handle comments 😂 Thank you.

[snowe2010]

@nreid260 I spent a bunch of time trying to figure out why the first case doesn't look good with my changes, and finally just reverted to the current main branch and tested a new test case that shows that it's actually current state that makes it look bad. See this example:

Not sure if I should touch this now. I can just make the initial pr 'solve' the nested arguments without touching the existing 'issue' around properties overflowing their lines (tbh I am not sure I've ever seen that case at all, and we have hundreds of thousands of lines of kotlin code.

[nreid260]

I don't really know how the fluent-chain formatting works today. I wrote a sort-of-spec for how is should work, but didn't get permission to implement it myself. (I recall wanting to add a class to model an entire chain).

Since you've gone to the trouble of writing test cases, you should mail them, to show what the current behaviour is. That might make it easier to rouse support for changes, or at least prevent future regressions.

[snowe2010]

Since you've gone to the trouble of writing test cases, you should mail them, to show what the current behaviour is. That might make it easier to rouse support for changes, or at least prevent future regressions.

that's a good idea. I'll do that!

[snowe2010]

https://github.com/facebook/ktfmt/pull/488