From 56c7a016240d6a0f4333313e39cc9f5a7f443903 Mon Sep 17 00:00:00 2001 From: Alex Abashev Date: Wed, 23 Sep 2026 10:40:47 +0300 Subject: [PATCH] Keep comments between imports with the import after them ImportOrderer.scanImports read imports while the next token was "import" and took along only the rest of an import's line and the // comments right under it. A blank line followed by a comment, or any block comment, ended the scan, and the import after it made reorderImports throw "Imports not contiguous", so the whole file went unformatted (#39, from google/google-java-format#424 and google/google-java-format#546). 18 files of the JDK 21 sources failed this way, with group headings such as "// Javadoc imports:", commented-out imports, and a block comment around an import. Comments between two imports now go with the import after them and move with it when the imports are sorted. A block comment on an import's own line stays with that import, as a // comment there already did. Comments after the last import still belong to what follows. Only files that failed before change: of the 15,747 JDK 21 files, the 18 now format and no other file changes. Two of the 18 change again on a second run, in string concatenations away from the imports, as other files of the JDK already do. The two import tests that expected the error now expect the sorted imports. --- .../javaformat/java/ImportOrderer.java | 62 ++++++++++-- .../java/GoogleImportStyleTest.java | 94 ++++++++++++++++++- .../palantir/javaformat/java/MainTest.java | 21 +++++ 3 files changed, 163 insertions(+), 14 deletions(-) diff --git a/open-java-format/src/main/java/com/palantir/javaformat/java/ImportOrderer.java b/open-java-format/src/main/java/com/palantir/javaformat/java/ImportOrderer.java index df69a51f0..4a0fdd46d 100644 --- a/open-java-format/src/main/java/com/palantir/javaformat/java/ImportOrderer.java +++ b/open-java-format/src/main/java/com/palantir/javaformat/java/ImportOrderer.java @@ -186,10 +186,12 @@ private ImportOrderer(String text, ImmutableList toks, Style style) { class Import { private final String imported; private final boolean isStatic; + private final String leading; private final String trailing; - Import(String imported, String trailing, boolean isStatic) { + Import(String imported, String leading, String trailing, boolean isStatic) { this.imported = imported; + this.leading = leading; this.trailing = trailing; this.isStatic = isStatic; } @@ -227,9 +229,18 @@ boolean isJava() { } /** - * The {@code //} comment lines after the final {@code ;}, up to and including the line terminator of the last - * one. Note: In case two imports were separated by a space (which is disallowed by the style guide), the - * trailing whitespace of the first import does not include a line terminator. + * The comments that stood between the previous import and this one, including the line terminator after the + * last of them, or empty. They move with this import. + */ + String leading() { + return leading; + } + + /** + * A block comment on the import's own line and the {@code //} comment lines after the final {@code ;}, up to + * and including the line terminator of the last one. Note: In case two imports were separated by a space + * (which is disallowed by the style guide), the trailing whitespace of the first import does not include a + * line terminator. */ String trailing() { return trailing; @@ -240,11 +251,12 @@ public boolean isThirdParty() { return !(isAndroid() || isJava()); } - // One or multiple lines, the import itself and following comments, including the line - // terminator. + // One or multiple lines, the comments before the import, the import itself and following comments, including + // the line terminator. @Override public String toString() { StringBuilder sb = new StringBuilder(); + sb.append(leading()); sb.append("import "); if (isStatic()) { sb.append("static "); @@ -282,11 +294,14 @@ private static class ImportsAndIndex { * * 
{@code
      *  -> ( | )*
-     *  -> "import"  ("static" )?
+     *  -> ? "import"  ("static" )?
      *     ("." )* ("." "*")? ? ";"
-     *    ? ? ( )*
+     *    ? ( ?)? ? ( )*
      * }
* + * The comments before an import are the ones between it and the previous import, so the first import has none: the + * text before it is left where it is. + * * @param i the index to start parsing at. * @return the result of parsing the imports. * @throws FormatterException if imports could not parsed according to the grammar. @@ -294,6 +309,7 @@ private static class ImportsAndIndex { private ImportsAndIndex scanImports(int i) throws FormatterException { int afterLastImport = i; ImmutableSortedSet.Builder imports = ImmutableSortedSet.orderedBy(importComparator); + String leading = ""; // JavaInput.buildToks appends a zero-width EOF token after all tokens. It won't match any // of our tests here and protects us from running off the end of the toks list. Since it is // zero-width it doesn't matter if we include it in our string concatenation at the end. @@ -330,6 +346,15 @@ private ImportsAndIndex scanImports(int i) throws FormatterException { trailing.append(tokenAt(i)); i++; } + // A block comment on the import's own line stays with the import, as a line comment there does. + if (isBlockCommentToken(i)) { + trailing.append(tokenAt(i)); + i++; + if (isSpaceToken(i)) { + trailing.append(tokenAt(i)); + i++; + } + } if (isNewlineToken(i)) { trailing.append(tokenAt(i)); i++; @@ -344,7 +369,7 @@ private ImportsAndIndex scanImports(int i) throws FormatterException { i++; } } - imports.add(new Import(importedName, trailing.toString(), isStatic)); + imports.add(new Import(importedName, leading, trailing.toString(), isStatic)); // Remember the position just after the import we just saw, before skipping blank lines. // If the next thing after the blank lines is not another import then we don't want to // include those blank lines in the text to be replaced. @@ -352,6 +377,17 @@ private ImportsAndIndex scanImports(int i) throws FormatterException { while (isNewlineToken(i) || isSpaceToken(i)) { i++; } + // Comments between this import and the next one go with the next one, so they move with it when the + // imports are sorted. Comments after the last import belong to whatever follows it. + leading = ""; + int next = i; + while (isCommentToken(next) || isNewlineToken(next) || isSpaceToken(next)) { + next++; + } + if (next > i && tokenAt(next).equals("import")) { + leading = CharMatcher.whitespace().trimTrailingFrom(tokString(i, next)) + lineSeparator; + i = next; + } } return new ImportsAndIndex(imports.build(), afterLastImport); } @@ -470,6 +506,14 @@ private boolean isSlashSlashCommentToken(int i) { return toks.get(i).isSlashSlashComment(); } + private boolean isBlockCommentToken(int i) { + return toks.get(i).isComment() && !toks.get(i).isSlashSlashComment(); + } + + private boolean isCommentToken(int i) { + return toks.get(i).isComment(); + } + private boolean isNewlineToken(int i) { return toks.get(i).isNewline(); } diff --git a/open-java-format/src/test/java/com/palantir/javaformat/java/GoogleImportStyleTest.java b/open-java-format/src/test/java/com/palantir/javaformat/java/GoogleImportStyleTest.java index e3b0cb412..4124ae4c9 100644 --- a/open-java-format/src/test/java/com/palantir/javaformat/java/GoogleImportStyleTest.java +++ b/open-java-format/src/test/java/com/palantir/javaformat/java/GoogleImportStyleTest.java @@ -391,25 +391,109 @@ public static List parameters() { "!!Could not parse imported name, at: ", } }, + // A comment between two imports goes with the import after it (#39, from + // google/google-java-format#424); a comment on an import's own line stays with that import. { { "import com.foo.Second;", "import com.foo.First;", - "/* we don't support block comments", - " between imports either */", + "/* A block comment between imports", + " goes with the import after it. */", "import com.foo.Third;", }, { - "!!Imports not contiguous (perhaps a comment separates them?)", + "import com.foo.First;", + "import com.foo.Second;", + "/* A block comment between imports", + " goes with the import after it. */", + "import com.foo.Third;", } }, { { - "import com.foo.Second; /* no block comments after imports */", // + "import com.foo.Second; /* A block comment after an import stays with it. */", // "import com.foo.First;", }, { - "!!Imports not contiguous (perhaps a comment separates them?)", + "import com.foo.First;", // + "import com.foo.Second; /* A block comment after an import stays with it. */", + } + }, + { + { + "import b.B;", // + "", + "// why we need A", + "import a.A;", + "", + "class T {}", + }, + { + "// why we need A", // + "import a.A;", + "import b.B;", + "", + "class T {}", + } + }, + { + { + "package foo;", + "", + "import groovy.transform.CompileStatic;", + "", + "/**", + " * Created.", + " */", + "import java.util.ArrayList;", + "", + "/**", + " * Created.", + " */", + "@CompileStatic", + "public class Broken {", + " ArrayList list;", + "}", + }, + { + "package foo;", + "", + "import groovy.transform.CompileStatic;", + "/**", + " * Created.", + " */", + "import java.util.ArrayList;", + "", + "/**", + " * Created.", + " */", + "@CompileStatic", + "public class Broken {", + " ArrayList list;", + "}", + } + }, + { + { + "import java.lang.reflect.Field;", + "", + "//import org.jline.nativ.JLineLibrary;", + "//import org.jline.nativ.JLineNativeLoader;", + "import org.jline.terminal.Attributes;", + "", + "import static org.jline.terminal.TerminalBuilder.PROP_NON_BLOCKING_READS;", + "", + "class T {}", + }, + { + "import static org.jline.terminal.TerminalBuilder.PROP_NON_BLOCKING_READS;", + "", + "import java.lang.reflect.Field;", + "//import org.jline.nativ.JLineLibrary;", + "//import org.jline.nativ.JLineNativeLoader;", + "import org.jline.terminal.Attributes;", + "", + "class T {}", } }, { diff --git a/open-java-format/src/test/java/com/palantir/javaformat/java/MainTest.java b/open-java-format/src/test/java/com/palantir/javaformat/java/MainTest.java index cbbccee6a..86647266f 100644 --- a/open-java-format/src/test/java/com/palantir/javaformat/java/MainTest.java +++ b/open-java-format/src/test/java/com/palantir/javaformat/java/MainTest.java @@ -605,6 +605,27 @@ public void noReflowLongStrings() throws Exception { assertThat(out.toString()).isEqualTo(joiner.join(expected)); } + // A comment between two imports goes with the import after it, where it used to fail the whole file with "Imports + // not contiguous" (#39, from google/google-java-format#424). A second run leaves the result alone. + @Test + public void commentBetweenImportsMovesWithTheImportAfterIt() throws Exception { + String[] input = { + "import b.B;", "", "// why we need A", "import a.A;", "", "class T {", " A a;", " B b;", "}", "", + }; + String[] expected = { + "// why we need A", "import a.A;", "import b.B;", "", "class T {", " A a;", " B b;", "}", "", + }; + for (String[] source : ImmutableList.of(input, expected)) { + StringWriter out = new StringWriter(); + Main main = new Main( + new PrintWriter(out, true), + new PrintWriter(new BufferedWriter(new OutputStreamWriter(System.err, UTF_8)), true), + new ByteArrayInputStream(joiner.join(source).getBytes(UTF_8))); + assertThat(main.format("-")).isEqualTo(0); + assertThat(out.toString()).isEqualTo(joiner.join(expected)); + } + } + private static ProcessBuilder formatterMain(String... args) { return new ProcessBuilder(ImmutableList.builder() .add(Paths.get(System.getProperty("java.home"))