ktfmt
ktfmt copied to clipboard
facebook
Formatting isn't idempotent, and repeated invocations produce bad comment formatting
The first time ktfmt is run here it splits the comment across 2 lines in a reasonable way. However, not only is the formatter not idempotent (which makes it harder to use as a commit check), but the second time it is run it reformats the comment poorly.
$ cat test.kt
class Foo { // This is a very long comment that is very long and needs to be line broken because it is long
}
$ java -jar ktfmt-0.47/core/target/ktfmt-0.47-jar-with-dependencies.jar --kotlinlang-style test.kt
Done formatting test.kt
$ cat test.kt
class Foo { // This is a very long comment that is very long and needs to be line broken because it
// is long
}
$ java -jar ktfmt-0.47/core/target/ktfmt-0.47-jar-with-dependencies.jar --kotlinlang-style test.kt
Done formatting test.kt
$ cat test.kt
class Foo { // This is a very long comment that is very long and needs to be line broken because it
// is long
}
$
Interesting example. I wonder what the expected outcome would be. I'd say the second version (even though worse for this example) should be preferred.
If you'd have a class like this
class Foo { // This is a very long comment that is very long and needs to be line broken because it
// comment
val s: String
}
you probably wouldn't want to align the comment on the member with the comment next to the brace like this:
class Foo { // This is a very long comment that is very long and needs to be line broken because it
// comment
val s: String
}
@igfoo do you have a more realistic example where this is an issue? In most cases, I'd say a long comment should be put above the class definition anyway...
So my personal approach to this issue would be making sure formatting is always producing the second version but only running once. Don't know how hard that is with the current codebase, but maybe one of the maintainers can chime in here...
@igfoo do you have a more realistic example where this is an issue?
I don't know if this would be more realistic, but it happened several times for real in https://github.com/github/codeql/commit/bf611feab3df711a80c8f5a3ea2b5c70b71737f7; for example in KotlinFileExtractor.kt (you'll probably need to press a button to load the diff for that file if looking at it on GH):
when(it) {
is IrClass -> when {
it.typeParameters.isNotEmpty() -> true // Type parameters visible to this class -- extract an enclosing bound or raw type.
!(it.isInner || it.isLocal) -> false // No type parameters seen yet, and this is a static class -- extract an enclosing unbound type.
else -> null // No type parameters seen here, but may be visible enclosing type parameters; keep searching.
}
else -> null // Look through enclosing non-class entities (this may need to change)
}
became
when (it) {
is IrClass ->
when {
it.typeParameters.isNotEmpty() ->
true // Type parameters visible to this class -- extract an
// enclosing bound or raw type.
!(it.isInner || it.isLocal) ->
false // No type parameters seen yet, and this is a static
// class -- extract an enclosing unbound type.
else ->
null // No type parameters seen here, but may be visible
// enclosing type parameters; keep searching.
}
else ->
null // Look through enclosing non-class entities (this may need to
// change)
}
and now wants to become
when (it) {
is IrClass ->
when {
it.typeParameters.isNotEmpty() ->
true // Type parameters visible to this class -- extract an
// enclosing bound or raw type.
!(it.isInner || it.isLocal) ->
false // No type parameters seen yet, and this is a static
// class -- extract an enclosing unbound type.
else -> null // No type parameters seen here, but may be visible
// enclosing type parameters; keep searching.
}
else ->
null // Look through enclosing non-class entities (this may need to
// change)
Thanks for looking that up!
Now for this example, the first variant is definitely the preferred result... I guess there's no simple way to distinguish between comments referencing the line below (that should stay on the same indentation level as the code below) and comments that have been split and moved to the next line. I don't know if there's data on what scenario is more common, but I suppose ktfmt should try to fit the more common scenario better.
I think it's necessary a maintainer takes a look at this and clarify if comments should align with trailing comments of the previous line or with the next line. Before that has happened, it probably doesn't make a ton of sense to fix the bug regarding idempotency.
PS: My two cents on the code: Regardless of ktfmt, I think I'd first opt for making the code a bit more self descriptive (e.g. by wrapping the conditions in a properly named property). If such changes are for whatever reason not to be made, I'd probably advocate for putting the comments on a separate line and maybe separating each case in the when by a newline.
**UPDATE: ** I also took a look around the code and it seems to me, the comment formatting is done by google-java-format. So I just might take a look there and see if something similar happens there when running two passes over source code like the one you've provided.
I also installed the IDE plugin today. Ran it twice on my code. The first time I got 10 files changed, then ran it again and had another two files that changed. Something doesn't seem idempotent... hrmmm.
@ColtonIdle can you provide code samples? I suppose that could be helpful to see if it is actually the same issue as this one or maybe a different one
Alright. So here's the "problematic" file. Running latest ktfmt plugin
If I have ktfmt plugin enabled with default style, this is my original file
package com.example.myapp
import android.content.Intent
private fun sendEmail(context: android.content.Context) {
val emailIntent = Intent(Intent.ACTION_SENDTO).apply {
data = Uri.parse("mailto:[email protected]") // Replace with your email address
}
if (emailIntent.resolveActivity(context.packageManager) != null) {
println("hello")
context.startActivity(emailIntent)
} else {
// Handle the case where no email app is installed
println("user doesn't have an email app installed")
// You might want to show a message to the user or provide an alternative way to contact you
}
}
then i get this
package com.example.myapp
import android.content.Intent
private fun sendEmail(context: android.content.Context) {
val emailIntent =
Intent(Intent.ACTION_SENDTO).apply {
data =
Uri.parse(
"mailto:[email protected]") // Replace with
// your email
// address
}
if (emailIntent.resolveActivity(context.packageManager) != null) {
println("hello")
context.startActivity(emailIntent)
} else {
// Handle the case where no email app is installed
println("user doesn't have an email app installed")
// You might want to show a message to the user or provide an alternative way to contact you
}
}
then if i run it again i get this
package com.example.myapp
import android.content.Intent
private fun sendEmail(context: android.content.Context) {
val emailIntent =
Intent(Intent.ACTION_SENDTO).apply {
data =
Uri.parse(
"mailto:[email protected]") // Replace with
// your email
// address
}
if (emailIntent.resolveActivity(context.packageManager) != null) {
println("hello")
context.startActivity(emailIntent)
} else {
// Handle the case where no email app is installed
println("user doesn't have an email app installed")
// You might want to show a message to the user or provide an alternative way to contact you
}
}
Wow, breaking that comment into three is really ugly... But it seems to be the same issue of comment alignment after breaking a single-line comment in to a multi-line one.
In general, breaking up comments that are on the same line as a block of code starts to look awkward. 🤔
Still would like to know what the desired behavior would be. (@hick209 @cgrushko @strulovich maybe?) Just running the formatter once more to achieve idempotence might be slow. Also: Do we want to align comments that have been split the same way they are aligned after running the formatter a second time?
Maybe its an issue with https://github.com/tnorbye/kdoc-formatter/ which ktfmt uses?
This is most certainly a bug, and looks like happens due to kdoc, since turning it off fixes the issues reported here.
Thanks @ColtonIdle for filing https://github.com/tnorbye/kdoc-formatter/issues/97 (cc @tnorbye).
BTW @ColtonIdle I was not able to repro the issue you reported here on v0.50 (I used https://facebook.github.io/ktfmt/ and local tests). Please let us know in case that particular use case was indeed fixed or if we're missing something.
Nevermind, @tnorbye helped me see that the class where the issue happens actually belongs to ktfmt (introduced by @davidtorosyan), only I missed that.
I've narrowed down the issue here to KDocCommentsHelper.wrapLineComments. I'm still looking into it, hopefully it's a simple fix.
I guess I could have started with this here. It looks like this might be an issue with Google Java Format, as I can repro this there.
Original
➜ cat Test.java
public class Test { // This is a very long comment that is very long and needs to be line broken because it is long
}
1st format pass
➜ java -jar google-java-format-1.22.0-all-deps.jar Test.java
public
class Test { // This is a very long comment that is very long and needs to be line broken because it
// is long
}
2nd format pass
➜ java -jar google-java-format-1.22.0-all-deps.jar Test.java
public
class Test { // This is a very long comment that is very long and needs to be line broken because
// it is long
}
Commented on https://github.com/google/google-java-format/issues/211 which is where the issue needs to be fixed. Closing this here as a no-op