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

CodeBlock composition: statement within statement is not supported #1753

Open
fejesjoco opened this issue Dec 2, 2023 · 13 comments
Open

CodeBlock composition: statement within statement is not supported #1753

fejesjoco opened this issue Dec 2, 2023 · 13 comments

Comments

@fejesjoco
Copy link
Contributor

Describe the bug
I frequently have to deal with generating big function bodies, where I resort to composing multiple smaller CodeBlock's together. In these cases it can happen that a statement is nested in another one, e.g. via lambdas, anonymous classes, etc. By default this throws an error but it's easy to workaround and the workaround could be done in KotlinPoet itself.

To Reproduce

  val cb0 = buildCodeBlock {
    beginControlFlow("{ foobar ->")
    addStatement("println(foobar)")
    endControlFlow()
  }
  val cb1 = buildCodeBlock {
    addStatement("callWithLambda %L", cb0)
  }
  FileSpec.builder("com.test", "HelloWorld")
    .addType(
      TypeSpec.classBuilder("HelloWorld")
        .addFunction(FunSpec.builder("hello1").addCode(cb1).build())
        .build()
    )
    .build()
    .writeTo(System.out)

Expected behavior
If done correctly, this should print something like:

package com.test

public class HelloWorld {
  public fun hello1() {
    callWithLambda { foobar ->
      println(foobar)
    }
  }
}

Additional context
This code currently throws Can't open a new statement until the current statement is closed. If I use cb0.toString() instead of cb0, it works reasonably well and even follows the current indentation level. Supporting nesting with stack data structures could work too but I think that would be overkill compared to the simplicity of the string solution.

@fejesjoco fejesjoco added the bug label Dec 2, 2023
@Egorand
Copy link
Collaborator

Egorand commented Dec 4, 2023

It would make a lot of sense to squash:

val cb0 = buildCodeBlock {
  beginControlFlow("{ foobar ->")
  addStatement("println(foobar)")
  endControlFlow()
}
val cb1 = buildCodeBlock {
  addStatement("callWithLambda %L", cb0)
}

into:

val cb1 = buildCodeBlock {
  beginControlFlow("callWithLambda { foobar ->")
  addStatement("println(foobar)")
  endControlFlow()
}

Is it not done in your real project because of reusability?

The lack of support for nested statements is not really a bug, it's a limitation of the library.

@JakeWharton
Copy link
Collaborator

And it's been around since its JavaPoet roots.

@Egorand
Copy link
Collaborator

Egorand commented Dec 21, 2023

Don't think we'll consider adding support for nested statements, as it'll likely be complex to implement without any clear benefits. I'm gonna close this for now.

@Egorand Egorand closed this as not planned Won't fix, can't repro, duplicate, stale Dec 21, 2023
@fejesjoco
Copy link
Contributor Author

Nested statements is just one possible solution.

As an alternative, you could simply call CodeBlock.toString() before adding it into another CodeBlock.

@Egorand
Copy link
Collaborator

Egorand commented Dec 21, 2023

Not really sure doing toString() will be correct or desirable in all scenarios, might be worth exploring though, I'll reopen the ticket. Please feel free to send us PRs if you'd like to take a crack at it.

@Egorand Egorand reopened this Dec 21, 2023
@Egorand Egorand added enhancement and removed bug labels Jan 5, 2024
@adrianimboden
Copy link

toString() breaks when the lines get too long. The line will then be broken up inside strings. "long text" may be broken up like this:

"long
text"

to be honest, some design decision begin to annoy me. Most of the annoyances would just go away when kotlinpoet would not try to do a half-baked formatting by itself and just let us apply a proper formatter after kotlinpoet is done.

@JakeWharton
Copy link
Collaborator

just let us apply a proper formatter after kotlinpoet is done

You have always been and continue to be able to do this.

@adrianimboden
Copy link

no that is sadly not true. The half baked 100 column limit makes it impossible because the code is broken before we even try to apply a code formatter.

I had to lookup quite some stuff this week and it almost always got down to "we won't add this configuration because we know what is best".

This CodeBlock inside CodeBlock is the perfect example. It seems that this issue here is not getting a solution anytime soon and calling CodeBlock.toString() does not work because the line-limiter does its thing (see my example before).

All this problems would just vanish, when we could set columnLimit to IntMax on File.writeTo. But this has been rejected here: #1020.

Why exactly is it, that kotlinpoet tries to do formatting stuff anyways? It gets in the way much too often. You say correctness is more important than nice-looking code. These choices say something else.

@JakeWharton
Copy link
Collaborator

JakeWharton commented Mar 6, 2024

no that is sadly not true.

Then you have a broken formatter and should seek a new one immediately. Both ktlint and ktfmt handle formatting the string output of FileSpec just fine.

The half baked 100 column limit makes it impossible because the code is broken before we even try to apply a code formatter.

Perhaps your hyperbolism is what's breaking KotlinPoet? The wrapping is orthogonal to your ability to post-process the output.

Why exactly is it, that kotlinpoet tries to do formatting stuff anyways?

Feel free to thumbs up #1314.

@adrianimboden
Copy link

adrianimboden commented Mar 6, 2024

It is the kotlinpoet formatter (the 100 column line limit) that breaks stuff. The example from @fejesjoco breaks when we change it a little bit:

  val cb0 = buildCodeBlock {
    beginControlFlow("{ foobar ->")
    addStatement("println(foobar)")
    addStatement("println(%S)", "a very long text with many spaces in between")
    endControlFlow()
  }
  val cb1 = buildCodeBlock {
    addStatement("callWithLambda %L", cb0.toString())
  }
  FileSpec.builder("com.test", "HelloWorld")
    .addType(
      TypeSpec.classBuilder("HelloWorld")
        .addFunction(FunSpec.builder("hello1").addCode(cb1).build())
        .build()
    )
    .build()
    .writeTo(System.out)

result will be something like this:

package com.test

public class HelloWorld {
  public fun hello1() {
    callWithLambda { foobar ->
      println(foobar)
      println("a very long text with many spaces
      in between")
    }
  }

@JakeWharton
Copy link
Collaborator

That is working as designed currently. This behavior is documented, along with how to opt-out of wrapping. I have already linked you to an issue to vote for which will change the behavior.

@adrianimboden
Copy link

Thanks for the hint. Sorry and thanks for your patience.

For everyone that also runs into this issue. The current workaround is this:

  val cb0 = buildCodeBlock {
    beginControlFlow("{ foobar ->")
    addStatement("println(foobar)")
    addStatement("println(%S)", "a very long text with many spaces in between")
    endControlFlow()
  }
  val cb1 = buildCodeBlock {
    addStatement("callWithLambda %L", cb0.toString().replace(" ", "·"))
  }
  FileSpec.builder("com.test", "HelloWorld")
    .addType(
      TypeSpec.classBuilder("HelloWorld")
        .addFunction(FunSpec.builder("hello1").addCode(cb1).build())
        .build()
    )
    .build()
    .writeTo(System.out)

in short:
cb.toString() is not enough, but cb.toString().replace(" ", "·") will do it.

@JakeWharton
Copy link
Collaborator

And this issue is specifically about collapsing CodeBlock's properly (i.e., not through toString()). This would also negate any need to post-process CodeBlock.toString(), but does not obviate the need to prevent wrapping of strings normally. I don't think anyone is opposed to solving CodeBlock composition properly and it's something that could be done today (separately from the wrapping default).

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

4 participants