fix: render each import declaration instead of rebuilding it

ImportOrderer threw away the text of each import and re-synthesized it from the kind
and the name, so every comment in the declaration had to be relocated to wherever that
synthesized string could hold it -- after the semicolon -- and anything that did not
reduce to those two pieces was rejected:

    import module java . base;          error: Expected ; after import
    import module java./* why */base;   error: Could not parse imported name, at: /* why */

Both compile with `javac --release 25`, and `formatSource` formats both; only the
import-fixing entry points failed, which the Gradle plugin's Spotless step reaches
through FormatterService. The same cause made the earlier comment handling a patch on a
symptom: a relocated comment is a comment the author has to find again.

The declaration is now rendered once while it is scanned, normalizing the whitespace --
one import per line is the style -- and leaving each comment in the slot it was written
in. That removes the comment list, the line-terminator juggling around it, and the
special case for a `//` comment inside a declaration.

Imports that compare equal still collapse into one, but the comparator now breaks ties
on the rendered declaration, so two imports of the same name that are written
differently are both kept. Before, the second one vanished, and any comment it carried
vanished with it.

Consequences worth noting in review: `import com . foo . Second ;`, which an existing
row recorded as "syntactically valid, but we don't support it", now formats; and a
duplicate that differs only by a comment is no longer deduplicated. A duplicate that
differs only by a trailing comment after the semicolon still loses it, as on develop.

Javadoc inside or trailing an import is still rejected: the formatter moves a javadoc
comment onto a line of its own, which would separate the imports.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Author: asm0dey

palantir-java-format/src/main/java/com/palantir/javaformat/java/ImportOrderer.java

@@ -130,7 +130,10 @@ private String reorderImports() throws FormatterException {
     private static final Comparator<Import> GOOGLE_IMPORT_COMPARATOR = Comparator.comparing(
                     Import::isStatic, trueFirst())
             .thenComparing(Import::isModule, trueFirst())
-            .thenComparing(Import::imported);
+            .thenComparing(Import::imported)
+            // Imports that compare equal collapse into one, so two that are written differently -- one of them
+            // carrying a comment, say -- must not compare equal, or that comment disappears with it.
+            .thenComparing(Import::declaration);
 
     /**
      * A {@link Comparator} that orders {@link Import}s by AOSP Style, defined at
@@ -144,7 +147,8 @@ private String reorderImports() throws FormatterException {
             .thenComparing(Import::isAndroid, trueFirst())
             .thenComparing(Import::isThirdParty, trueFirst())
             .thenComparing(Import::isJava, trueFirst())
-            .thenComparing(Import::imported);
+            .thenComparing(Import::imported)
+            .thenComparing(Import::declaration);
 
     /**
      * Determines whether to insert a blank line between the {@code prev} and {@code curr} {@link Import}s based on
@@ -196,14 +200,22 @@ class Import {
         private final boolean isStatic;
         private final boolean isModule;
         private final String trailing;
-        private final ImmutableList<String> comments;
+        private final String declaration;
 
-        Import(String imported, String trailing, boolean isStatic, boolean isModule, ImmutableList<String> comments) {
+        Import(String imported, String trailing, boolean isStatic, boolean isModule, String declaration) {
             this.imported = imported;
             this.trailing = trailing;
             this.isStatic = isStatic;
             this.isModule = isModule;
-            this.comments = comments;
+            this.declaration = declaration;
+        }
+
+        /**
+         * The declaration as it will be written: the {@code import} keyword through the semicolon, with whitespace
+         * normalized and any comments left in the slot the author wrote them in.
+         */
+        String declaration() {
+            return declaration;
         }
 
         /** The name being imported, for example {@code java.util.List}. */
@@ -262,45 +274,52 @@ public boolean isThirdParty() {
         @Override
         public String toString() {
             StringBuilder sb = new StringBuilder();
-            sb.append("import ");
-            if (isModule()) {
-                sb.append("module ");
-            } else if (isStatic()) {
-                sb.append("static ");
-            }
-            sb.append(imported()).append(';');
-            StringBuilder tail = new StringBuilder();
-            for (String comment : comments) {
-                if (!endsInNewline(tail)) {
-                    tail.append(' ');
-                }
-                tail.append(comment);
-                if (comment.startsWith("//")) {
-                    // A // comment swallows the rest of its line, so nothing may follow it there.
-                    tail.append(lineSeparator);
-                }
-            }
-            String trailingText = trailing();
-            if (endsInNewline(tail)) {
-                // Don't double the line terminator the trailing text already starts with.
-                int newline = Newlines.hasNewlineAt(trailingText, 0);
-                if (newline > 0) {
-                    trailingText = trailingText.substring(newline);
-                }
-            }
-            tail.append(trailingText);
-            if (tail.toString().trim().isEmpty()) {
+            sb.append(declaration());
+            if (trailing().trim().isEmpty()) {
                 sb.append(lineSeparator);
             } else {
-                sb.append(tail);
-                if (!endsInNewline(tail)) {
-                    sb.append(lineSeparator);
-                }
+                sb.append(trailing());
             }
             return sb.toString();
         }
     }
 
+    /**
+     * Renders one import declaration from the toks it is made of. Whitespace between the toks is normalized, since the
+     * style guide puts one import on a line of its own, but a comment stays in the slot the author wrote it in rather
+     * than being moved to wherever the rendered declaration can accommodate it.
+     */
+    private final class Declaration {
+        private final StringBuilder text = new StringBuilder();
+        private boolean atLineStart = false;
+
+        /** Appends one tok: a keyword, an identifier, {@code .}, {@code *}, {@code ;}, or a comment. */
+        void append(String piece) {
+            if (text.length() > 0 && !atLineStart && needsSpaceBefore(piece)) {
+                text.append(' ');
+            }
+            text.append(piece);
+            atLineStart = false;
+            if (piece.startsWith("//")) {
+                // A // comment swallows the rest of its line, so the declaration continues on the next one.
+                text.append(lineSeparator);
+                atLineStart = true;
+            }
+        }
+
+        private boolean needsSpaceBefore(String piece) {
+            if (piece.equals(".") || piece.equals(";") || piece.equals("*")) {
+                return false;
+            }
+            return text.charAt(text.length() - 1) != '.';
+        }
+
+        @Override
+        public String toString() {
+            return text.toString();
+        }
+    }
+
     private String tokString(int start, int end) {
         StringBuilder sb = new StringBuilder();
         for (int i = start; i < end; i++) {
@@ -341,31 +360,33 @@ private ImportsAndIndex scanImports(int i) throws FormatterException {
         // 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.
         while (i < toks.size() && tokenAt(i).equals("import")) {
+            Declaration declaration = new Declaration();
+            declaration.append(tokenAt(i));
             i++;
-            // Comments between the tokens of the import are collected and re-emitted after the
-            // semicolon, so nothing is dropped.
-            List<String> comments = new ArrayList<>();
-            i = skipIgnored(i, comments);
+            i = skipIgnored(i, declaration);
             boolean isModule = isModuleKeyword(i);
             if (isModule) {
+                declaration.append(tokenAt(i));
                 i++;
-                i = skipIgnored(i, comments);
+                i = skipIgnored(i, declaration);
             }
             boolean isStatic = !isModule && tokenAt(i).equals("static");
             if (isStatic) {
+                declaration.append(tokenAt(i));
                 i++;
-                i = skipIgnored(i, comments);
+                i = skipIgnored(i, declaration);
             }
             if (!isIdentifierToken(i)) {
                 throw new FormatterException("Unexpected token after import: " + tokenAt(i));
             }
-            StringAndIndex imported = scanImported(i);
+            StringAndIndex imported = scanImported(i, declaration);
             String importedName = imported.string;
             i = imported.index;
-            i = skipIgnored(i, comments);
+            i = skipIgnored(i, declaration);
             if (!tokenAt(i).equals(";")) {
                 throw new FormatterException("Expected ; after import");
             }
+            declaration.append(";");
             while (tokenAt(i).equals(";")) {
                 // Extra semicolons are not allowed by the JLS but are accepted by javac.
                 i++;
@@ -393,8 +414,7 @@ private ImportsAndIndex scanImports(int i) throws FormatterException {
                     i++;
                 }
             }
-            imports.add(
-                    new Import(importedName, trailing.toString(), isStatic, isModule, ImmutableList.copyOf(comments)));
+            imports.add(new Import(importedName, trailing.toString(), isStatic, isModule, declaration.toString()));
             // 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.
@@ -437,32 +457,39 @@ private static class StringAndIndex {
     }
 
     /**
-     * Scans the imported thing, the dot-separated name that comes after import [static] and before the semicolon. We
-     * don't allow spaces inside the dot-separated name. Wildcard imports are supported: if the input is {@code import
-     * java.util.*;} then the returned string will be {@code java.util.*}.
+     * Scans the imported thing, the dot-separated name that comes after import [static] and before the semicolon.
+     * Whitespace, line terminators and comments may appear between its parts, as they may anywhere else in the
+     * declaration; the returned name contains none of them. Wildcard imports are supported: if the input is
+     * {@code import java.util.*;} then the returned string will be {@code java.util.*}.
      *
      * @param start the index of the start of the identifier. If the import is {@code import java.util.List;} then this
      *     index points to the token {@code java}.
+     * @param declaration collects the toks scanned, so a comment between the parts of the name keeps its place
      * @return the parsed import ({@code java.util.List} in the example) and the index of the first token after the
      *     imported thing ({@code ;} in the example).
      * @throws FormatterException if the imported name could not be parsed.
      */
-    private StringAndIndex scanImported(int start) throws FormatterException {
+    private StringAndIndex scanImported(int start, Declaration declaration) throws FormatterException {
         int i = start;
         StringBuilder imported = new StringBuilder();
         // At the start of each iteration of this loop, i points to an identifier.
         // On exit from the loop, i points to a token after an identifier or after *.
         while (true) {
             Preconditions.checkState(isIdentifierToken(i));
             imported.append(tokenAt(i));
+            declaration.append(tokenAt(i));
             i++;
+            i = skipIgnored(i, declaration);
             if (!tokenAt(i).equals(".")) {
                 return new StringAndIndex(imported.toString(), i);
             }
             imported.append('.');
+            declaration.append(".");
             i++;
+            i = skipIgnored(i, declaration);
             if (tokenAt(i).equals("*")) {
                 imported.append('*');
+                declaration.append("*");
                 return new StringAndIndex(imported.toString(), i + 1);
             } else if (!isIdentifierToken(i)) {
                 throw new FormatterException("Could not parse imported name, at: " + tokenAt(i));
@@ -514,21 +541,21 @@ private boolean isModuleKeyword(int i) {
         if (!tokenAt(i).equals("module")) {
             return false;
         }
-        return isIdentifierToken(skipIgnored(i + 1, new ArrayList<>()));
+        return isIdentifierToken(skipIgnored(i + 1, new Declaration()));
     }
 
     /**
-     * Skips whitespace, line terminators and comments starting at {@code i}, appending the text of each comment to
-     * {@code comments}, and returns the index of the first token that is none of those. Javadoc comments are not
-     * skipped: the formatter moves them onto a line of their own, so an import carrying one is rejected, as it was
-     * before module imports were supported.
+     * Skips whitespace, line terminators and comments starting at {@code i}, appending each comment to
+     * {@code declaration}, and returns the index of the first token that is none of those. Javadoc comments are not
+     * skipped: the formatter moves them onto a line of their own, which would separate the imports, so an import
+     * carrying one is rejected, as it was before module imports were supported.
      */
-    private int skipIgnored(int i, List<String> comments) {
+    private int skipIgnored(int i, Declaration declaration) {
         while (i < toks.size()) {
             if (isSpaceToken(i) || isNewlineToken(i)) {
                 i++;
             } else if (isSlashSlashCommentToken(i) || isBlockCommentToken(i)) {
-                comments.add(tokenAt(i).trim());
+                declaration.append(tokenAt(i).trim());
                 i++;
             } else {
                 break;

palantir-java-format/src/test/java/com/palantir/javaformat/java/GoogleImportStyleTest.java

@@ -453,13 +453,15 @@ public static List<Object[]> parameters() {
                     "*/",
                 }
             },
+            // Whitespace may appear between the parts of a qualified name; it is normalized away.
             {
                 {
-                    "import com . foo . Second ;", // syntactically valid, but we don't support it
+                    "import com . foo . Second ;", //
                     "import com.foo.First;",
                 },
                 {
-                    "!!Expected ; after import",
+                    "import com.foo.First;", //
+                    "import com.foo.Second;",
                 }
             },
             {
@@ -613,7 +615,7 @@ public static List<Object[]> parameters() {
                 {
                     "package foo;",
                     "",
-                    "import module java.base; /* the base module */",
+                    "import module /* the base module */ java.base;",
                     "import module java.desktop;",
                     "",
                     "public class Blim {}",
@@ -635,15 +637,50 @@ public static List<Object[]> parameters() {
                 {
                     "package foo;",
                     "",
-                    "import static com.foo.First.first; /* a member */",
+                    "import static /* a member */ com.foo.First.first;",
                     "",
-                    "import com.foo.Second; /* a type */",
+                    "import /* a type */ com.foo.Second;",
                     "import com.foo.Third;",
                     "",
                     "public class Blim {}",
                 },
             },
 
+            // A comment may also sit between the parts of the name, and stays there. Only the
+            // whitespace around it is normalized.
+            {
+                {
+                    "package foo;", "", "import com.foo./* the second one */Second;", "", "public class Blim {}",
+                },
+                {
+                    "package foo;", "", "import com.foo./* the second one */ Second;", "", "public class Blim {}",
+                },
+            },
+
+            // Identical declarations collapse into one; ones that differ are both kept, so that a
+            // comment does not disappear with the copy that goes.
+            {
+                {
+                    "package foo;",
+                    "",
+                    "import com.foo.First;",
+                    "import com.foo.First;",
+                    "import /* explanation A */ com.foo.Second;",
+                    "import /* explanation B */ com.foo.Second;",
+                    "",
+                    "public class Blim {}",
+                },
+                {
+                    "package foo;",
+                    "",
+                    "import com.foo.First;",
+                    "import /* explanation A */ com.foo.Second;",
+                    "import /* explanation B */ com.foo.Second;",
+                    "",
+                    "public class Blim {}",
+                },
+            },
+
             // A package literally named `module` is an ordinary import, not a module import:
             // `module` only introduces one when another identifier follows it.
             {

palantir-java-format/src/test/java/com/palantir/javaformat/java/ModuleImportTest.java

@@ -75,7 +75,42 @@ public void fixesImportsOnlyFromTheCommandLine() throws Exception {
     @Test
     public void keepsACommentBetweenModuleAndTheModuleName() throws FormatterException {
         String input = "import module /* comment */ java.base;\n" + "class Example {}\n";
-        String expected = "import module java.base; /* comment */\n" + "\n" + "class Example {}\n";
+        String expected = "import module /* comment */ java.base;\n" + "\n" + "class Example {}\n";
+        assertFormats(input, expected);
+    }
+
+    @Test
+    public void keepsACommentBetweenThePartsOfTheModuleName() throws FormatterException {
+        String input = "import module java./* comment */base;\n" + "class Example {}\n";
+        // Reordering normalizes the whitespace around the comment and leaves it between the parts.
+        String reordered = "import module java./* comment */ base;\n" + "\n" + "class Example {}\n";
+        assertThat(Formatter.create().fixImports(input)).isEqualTo(reordered);
+        assertThat(Formatter.create().fixImports(reordered)).isEqualTo(reordered);
+
+        // Formatting then breaks the line after the dot, which is where the formatter puts a comment
+        // in a qualified name; that output is stable too.
+        String formatted = "import module java.\n" + "/* comment */ base;\n" + "\n" + "class Example {}\n";
+        assertThat(Formatter.create().formatSourceAndFixImports(input)).isEqualTo(formatted);
+        assertThat(Formatter.create().formatSourceAndFixImports(formatted)).isEqualTo(formatted);
+    }
+
+    @Test
+    public void normalizesWhitespaceInsideTheModuleName() throws FormatterException {
+        String input = "import module java . base;\n" + "class Example {}\n";
+        String expected = "import module java.base;\n" + "\n" + "class Example {}\n";
+        assertFormats(input, expected);
+    }
+
+    @Test
+    public void keepsBothCopiesOfADuplicateThatCarriesAComment() throws FormatterException {
+        // Identical declarations collapse; ones that differ are both kept, so no comment is dropped.
+        String input = "import module /* explanation A */ java.base;\n"
+                + "import module /* explanation B */ java.base;\n"
+                + "class Example {}\n";
+        String expected = "import module /* explanation A */ java.base;\n"
+                + "import module /* explanation B */ java.base;\n"
+                + "\n"
+                + "class Example {}\n";
         assertFormats(input, expected);
     }