Merge pull request #44 from openjavaformat/comments-between-imports

Keep comments between imports with the import after them

Author: abashev

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

@@ -186,10 +186,12 @@ private ImportOrderer(String text, ImmutableList<Tok> 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,18 +294,22 @@ private static class ImportsAndIndex {
      *
      * <pre>{@code
      * <imports> -> (<end-of-line> | <import>)*
-     * <import> -> "import" <whitespace> ("static" <whitespace>)?
+     * <import> -> <comments-and-line-breaks>? "import" <whitespace> ("static" <whitespace>)?
      *    <identifier> ("." <identifier>)* ("." "*")? <whitespace>? ";"
-     *    <whitespace>? <end-of-line>? (<line-comment> <end-of-line>)*
+     *    <whitespace>? (<block-comment> <whitespace>?)? <end-of-line>? (<line-comment> <end-of-line>)*
      * }</pre>
      *
+     * 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.
      */
     private ImportsAndIndex scanImports(int i) throws FormatterException {
         int afterLastImport = i;
         ImmutableSortedSet.Builder<Import> 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,14 +369,25 @@ 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.
             afterLastImport = i;
             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();
     }

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

@@ -391,25 +391,109 @@ public static List<Object[]> 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 {}",
                 }
             },
             {

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.<String>builder()
                         .add(Paths.get(System.getProperty("java.home"))