ktfmt rearranges comments on top of imports statements

[ColtonIdle]

Let's say I have

import x
import y
//noinspection BannedImport
import z

ktfmt reformats it as

//noinspection BannedImport
import x
import y
import z

This is problematic because some linting tools use a comment to opt out of errors on an import statement. (for example android lint allows you to opt out of a import error with a comment, but then you run ktfmt and it rearranges the comments therefore causing lint to break again.

[cgrushko]

@bethcutler @nreid260

[nreid260]

We had a discussion about this change #291 Previously, the example input would have crashed ktfmt. There are more complex approaches that could be used to address the crash, but none of them seemed worth the trade offs.

As a workaround, you should be able to suppress using @file:Suppress("BannedImport") at the top of the file.

[ColtonIdle]

As a workaround its fine, but it's a little heavy handed (suprresses for the entire file), but anyway. just wanted to get it on your radar. thanks

[nreid260]

For future readers, I'd also point out that these comments are a bit of a hack in Android Lint.

AL primarily expresses suppressions using annotations (i.e. @Suppress("NameOfCheck")); however, for some reason import statements can't be annotated. That forced the need for some other syntax to expression suppressions for checks on import statements.

From personal experience, I can say that encoding metadata in comments leads to sad times.

[ColtonIdle]

Fair point. Filed this if you'd like to star. https://issuetracker.google.com/issues/225529525

[ColtonIdle]

The word from the lint team is "Kotlin syntax doesn't allow annotations on imports"
With that said, it seems like it makes sense for ktfmt to be able to handle comments on import statements without being re-ordered. /shruggie

[nreid260]

That comment was from me. Both the AL team and I work at Google.

The word from the lint team is "Kotlin syntax doesn't allow annotations on imports" With that said, it seems like it makes sense for ktfmt to be able to handle comments on import statements without being re-ordered. /shruggie

[cgrushko]

I think we should revert this change as it causes data corruption for some users.
A partial workaround exists, but (1) it doesn't do exactly the same thing, and (2) not all users will look for it.

We can still implement my suggestion of detecting whether a comment looks like an import, which should avoid cases like this issue.
If we don't want to do that, let's wrap this behavior in a flag so ktfmt is safer by default.

Thoughts?

[nreid260]

Thoughts?

For this example, I'd describe reverting as caving to a hack in another project.

We can't have all these simultaneously:

• comments are user data
• ktfmt preserves all user data
• ktfmt doesn't crash on valid Kotlin
• ktfmt sorts and deletes imports

[strulovich]

If there's some structure for this comments that we can handle, I'm down with doing that.

Reverting the change won't solve the problem - I think Ktfmt will just not format the file, so it's as bad as the heavy handed solution.

@nreid260 , @ColtonIdle - what does Google Java Format do in the same case for you guys? My go to will be to have the tools do the same.

[nreid260]

From what I can tell, GJF "glues" comments to the preceding import:

• if that import moves, the comment moves with it
• if that import is deleted, that comment stays in place

import a.b.Apple;
// I'm a comment on Apple
import a.b.Ball;

This has weird effects though:

• (in my opinion) comments should be associated with the line below, not above
• deleting an import changes which import the comment is glued to, and subsequently it can be sorted to a new location
• ranges of comments are meaningless because new imports will pop into the middle if that's the right sort location

I think what's going on is GJF optimizes for trailing comments, and sees comments on the next line as a degenerate form of trailing.

import a.b.Apple; // I'm a comment on Apple
import a.b.Ball;

Example of a bad thing that happens:

import a.b.Apple;
import a.b.Ball; // Some comment (on Ball)

    // Ball is unused for a second

import a.b.Apple;
// Some comment (on Apple)

    // Ball is used again

import a.b.Apple;
// Some comment (on Apple)
import a.b.Ball;

[strulovich]

Yeah, that's not that useful. Can you find the POC for GJF and question them about how they got to this? Maybe they can shed some context instead of us going through all the fixes & comments they went through. Happy to chat with them myself if you know who that is.

[nreid260]

From talking to the current maintainers:

• Comments on imports are more-or-less best effort
• Support has existed for at least 2 years
• The original focus was trailing comments
• There was initially a warning for comments on their own line, but it was disabled for ease-of-use
• There was minimal pushback on the "gluing" behaviour because
  • The Google repo is the primary target
  • The GJF authors have a lot of control over the tools running over Java code within Google
• No eval was done on alternative behaviors

At this point, if someone wants to make ktfmt behave like GJF, with gluing, that seems fine to me. As long as ktfmt doesn't crash, or delete the comments, it works for us. Just be aware that there's also weirdness in that approach.

Fundamentally, associating comments with other syntax is unspecified, so there will always be tools that chose to do it in a different way.