Wrong indentation in a lambda

[JavierSegoviaCordoba]

Introduced in 0.20

class SomeClass {
    fun aLongMethodWhereLambdaIsNotInTheSameLine(foo: String, bar: String, baz: String) =
        someLambda {
        someClass.someMethod(foo = foo, bar = bar, baz = baz)
    }
}

should be

class SomeClass {
    fun aLongMethodWhereLambdaIsNotInTheSameLine(foo: String, bar: String, baz: String) =
        someLambda {
            someClass.someMethod(foo = foo, bar = bar, baz = baz)
        }
}

[cgrushko]

Mmm, looks like it's a result of 6c7dd8d

I assume you're using dropbox style? it's lacking the extra indent we added for google-style. See the example at the very bottom of the commit. I think the way forward is to get rid of dropbox style, since it's not really maintained, and switch to 4/4 google style (and call it kotlinlang-style, maybe).

[JavierSegoviaCordoba]

Yeah, I am using dropbox-style. But currently, there is no way to use Google Style with any Gradle plugin no?

I reproduced it in a test:

    @Test
    fun `handle lambda when it breaks line after an equal in a function`() =
        assertFormatted(
            """
      |fun aLongMethodWhereLambdaDoesntFitInTheSameLine(foo: String, bar: String, baz: String) =
      |  someLambda {
      |    someClass.someMethod(foo = foo, bar = bar, baz = baz)
      |  }
      |""".trimMargin())

Result:

# Input: 
####################
fun aLongMethodWhereLambdaDoesntFitInTheSameLine(foo: String, bar: String, baz: String) =
  someLambda {
    someClass.someMethod(foo = foo, bar = bar, baz = baz)
  }


# Output: 
####################
fun aLongMethodWhereLambdaDoesntFitInTheSameLine(foo: String, bar: String, baz: String) =
    someLambda {
  someClass.someMethod(foo = foo, bar = bar, baz = baz)
}

[JavierSegoviaCordoba]

I am checking the commit:

  @Test
  fun `assignment of a scoping function`() =
      assertFormatted(
          """
      |----------------------------
      |fun longName() =
      |    coroutineScope {
      |  foo()
      |  //
      |}
      |""".trimMargin(),
          formattingOptions = GOOGLE_FORMAT,
          deduceMaxWidth = true)

This is strange because the lambda content is not indented based on the lambda, it is indented based on the function

fun longName() =
    coroutineScope {
  foo()
  //
}

shouldn't be this?

fun longName() =
    coroutineScope {
        foo()
        //
    }

or with 2 indent:

fun longName() =
  coroutineScope {
    foo()
    //
  }

[JavierSegoviaCordoba]

IMO moving to kotlinlang-style is perfect, indeed I created a post in discussions.

I am currently introducing ktfmt in the company I work, should be great if you could expose this style this week so I can test it in the PR I am already creating. Do you think it is possible or it is very soon?

[cgrushko]

@JavierSegoviaCordoba we'll try :) in the meanwhile, don't wait for us - clone the repo and build at head and see if it's of use for you at all.

[JavierSegoviaCordoba]

I am trying this again with the latest IDEA plugin and the 4 styles:

default

Current:

Should be:

dropbox

Current:

Should be

google

Current:

Should be (I am not sure if println should have 2 or 4 spaces):

• kotlinlang

Current:

Should be:

I am not sure about the 8 space indent, I think it should be 4 like Dropbox does.

[cgrushko]

This weirdness is a limitation of the underlying engine: google/google-java-format#556.
I agree that your alternative forms are better, but I'm not sure how get it right now.

[JavierSegoviaCordoba]

I think some time ago it was not happening 🤔

If it is caused by this 6c7dd8d cannot be improved/reverted?

[cgrushko]

Indeed, 6c7dd8d is the root cause.
Improving it is something I'd like to do, but haven't found a straightforward way to do so right now.
As for reverting - I think the benefits outweigh the cost. Do you disagree? do you run into this problem a lot?

[JavierSegoviaCordoba]

I agree and I can live with it trying to changing some large names to avoid this problem, don't worry :)

[bethcutler]

Chiming in to say this issue has been reported internally within Google as well. Would be nice to find a fix, though I can respect the fact that it's complicated!

[JavierSegoviaCordoba]

@bethcutler do you know if it is fixed there? I have checked that this issue happens a lot when you are using ktor client

[JavierSegoviaCordoba]

Not a good new... google/google-java-format#19 (comment), not sure if it is the same.

[ColtonIdle]

I wonder if this is also the cause of this issue with compose code which relies heavily on trailing lambdas: #249

[nreid260]

On the Google side, we're considering that it might be better to revert. We'd prefer a formatting that is consistently adequate vs one that is usually good but fails a few percent of the time. We're also not sure whether presenting scoping function names on the same line is more readable, or just more terse.

I did some digging into how to fix this bug, and while it's possible, it's pretty hacky. The underlying problem is that google_java_format strongly adheres to the rectangle rule, which 6c7dd8d violates. In order to express 6c7dd8d formatting correctly, we need to futz around with conditional indents across many syntactic levels.

This issue is common to all "block-like expressions", which Kotlin has several of (e.g. if, when). If we could add support for such constructs directly into google_java_format, it might make sense, but it currently isn't obvious how to do that. Until then, consistency across all block-like expressions is also desirable.

[cgrushko]

For tracking, this is how it looks like in 0.31 using kotlinlang style:

class SomeClass {
    fun aLongMethodWhereLambdaIsNotInTheSameLine(foo: String, bar: String, baz: String) =
            someLambda {
        someClass.someMethod(foo = foo, bar = bar, baz = baz)
    }
}

cc @strulovich

[JavierSegoviaCordoba]

What do you think about reverting @cgrushko. Personally don't see a lot of them

// preferable but I don't see this a lot in the code
fun foo() = {
  doItOnce()
  doItTwice()
}

// I like this because it is very Kotlin like
fun foo() = someLambda {
  doItOnce()
  doItTwice()
}

// I can live with this instead of having the issue
// because I see more of the issue than this use case
fun foo() =
  {
    doItOnce()
    doItTwice()
  }
  
// I have no issues with this and has consistence with other block formats
// even it not being very Kotlin like.
fun foo() =
  someLambda {
    doItOnce()
    doItTwice()
  }

[cgrushko]

We (I) actually prefer reverting this - it was a feature request by Google, actually :)
(it complicates the code and I agree it produces inconsistencies)
@nreid260 - let me know.

[nreid260]

I've sent #280 to fix this issue, at least for the majority of cases.

I'm still not sure the special case is worth it overall, but the fix isn't notably more complex than the current implementation. It might be easier to debate philosophy when there's no clear bug.

[JavierSegoviaCordoba]

I think this is already fixed, @cgrushko feels free to reopen if not.