Crash when comment separate imports

[bethcutler]

import foo
// This comment is pointless
import bar

leads to error:

Imports not contiguous (perhaps a comment separates them?)

We think it is not ideal for the formatter to crash on legal, compilable code. It's also not good to remove comments, or to attach them to the next line (what if the next line is an unused import that would be removed during formatting?)

We propose sorting all the comments within the import list to the top of the import list. It may not be exactly what the user intended, but it's not bad. What do you think?

[cgrushko]

We copied GJF's behavior here; has it changed?
I think we shouldn't touch imports at all if we can't do it safely.
Maybe format the rest of the code? it means we won't delete unused imports.

[bethcutler]

google-java-format does not crash when comments are in the middle of the import list; I don't know if that is a recent change.

The java formatter seems to attach the comment to the import before the comment (which seems backwards to me), and does not change the comment if imports are deleted before or after it.

[strulovich]

Looks like a recent change then. Our current local version crashes:

    Imports not contiguous (perhaps a comment separates them?)

We can try to fix it, and try to smartly attach a comment to an import, but I think it's a bit problematic:

It's unclear which import owns the comment:

import com.foo.Foo // comment1
// comment2
import com.foo.Bar

Is comment 2 & 1 associated import decided by a newline?

And that happens when a user tried to comment a group of imports:

// Foos
import com.foo.FooImpl
import com.foo.AbstractFoo

The comment will no longer work for the group after us sorting it.

[nreid260]

In the past, I've found that a good way to approach these decisions is to think about what invariants/goals one wants in a tool. I'd propose the following (incomplete) list, which seems to cover all the concerns presented:

1. ktfmt doesn't crash on compilable input
2. ktfmt doesn't delete input information
3. ktfmt output is canonical (including that imports are sorted)
4. ktfmt output never mixes comments and imports

Today, we can say that (4) is satisfied, since currently ktfmt doesn't produce output when the input has import comments. We sidestep the potentially problematic cases by throwing an error. Unfortunately, that causes a conflict with (1).

@bethcutler proposes a behaviour that satisfies the entire list, so that would seem like an improvement. The question is only, whether we agree about the invariants and their definitions. Of course, there could be more invariants, that I missed, which would constrain the correct behaviour even more.

One potential question about (2) is "what constitutes input information". It could be argued that rearranging comments deletes something, by changing the association between a comment and an import (or range). A solution there is to define away the problem, by saying that ktfmt doesn't recognize any such associations; we're then free to move the comments around, as long as we preserve their content. That might seem like cheating, but ktfmt does the same thing in many format choices. KDoc layout, whitespace, semicolons, unused imports, all of these are "deleted" because we defined them not to be meaningful.

[cgrushko]

Thoughts -

1. My concern is that we'll mess something up. I can imagine somebody trying to add headings to comments (which I think is a bad idea, but).
2. According to internal data, 97% of times this crash happens it's because of a commented-out import.

Suggestion:

1. If the comment is a commented-out import, use @bethcutler 's code, or whatever is simplest (sort as if not a comment, put at the top, put at the bottom, whatever)
2. Otherwise, skip the import sorting/unused-removal, format the rest of the code.

What do you think?

[nreid260]

Suggestion:

1. If the comment is a commented-out import, use @bethcutler 's code, or whatever is simplest (sort as if not a comment, put at the top, put at the bottom, whatever)

If we want to special case commented imports, I think it would be friendlier to sort them along with live imports.

But I'm also not sold on why these comments are useful. Extra imports (AFAIK) aren't a build error, and adding imports is a one-click operation in an IDE (text editor users probably aren't running kftmt often enough to be bothered by it removing temporarily unused imports).

2. Otherwise, skip the import sorting/unused-removal, format the rest of the code.

I don't like that this behaviour is:

• inconsistent: We want to use ktfmt to enforce a standard on all our code, and this makes it easy to disable sorting
• surprising: Users probably won't realize they've triggered this path, and if they do, they'll see it as a bug.
• messy: Other tools, such as build-dep cleaners or large scale refactorings, won't work as well with unformatted imports

[cgrushko]

Let's step back -

• We all agree that ktfmt shouldn't lose information.
• I may be too conservative here, but I consider moving comments away from imports as potentially losing information.
  • IMO, If we can prove we're not moving comments away (i.e., by reliably detecting what the comment belongs to), or that the comments don't carry information (e.g., they're commented-out imports), only then we should reformat imports.

But I'm also not sold on why these comments are useful.

On the contrary, they (commented-out imports) aren't useful, and therefore can be moved around, or even deleted.

adding imports is a one-click operation in an IDE

That... might not always be true in large enough projects :)

[nreid260]

• I may be too conservative here, but I consider moving comments away from imports as potentially losing information.

I think this is where we disagree :) With my present experience, I'm willing to declare comments on imports as zero-information. Not only is this convenient for the ktfmt usecase, I also think it's a healthier style for Kotlin code, because it removes the temptation to try to put information in them.

That... might not always be true in large enough projects :)

Haha, sorry. I must have let the Google bubble get to me here. Our Kotlin+Java projects are often 100k bazel targets in size, and imports are still one-click.

[strulovich]

Me and @cgrushko chatted today and we think the proposed solution from @bethcutler is 👍. So we will just go ahead with approving that. Thanks!