Merge pull request #43 from openjavaformat/unused-import-blank-line

Stop an unused import from leaving two blank lines behind

Author: abashev

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

@@ -226,6 +226,7 @@ private static RangeMap<Integer, String> buildReplacements(
             Set<String> usedNames,
             Multimap<String, Range<Integer>> usedInJavadoc) {
         RangeMap<Integer, String> replacements = TreeRangeMap.create();
+        String sep = Newlines.guessLineSeparator(contents);
         for (JCImport importTree : unit.getImports()) {
             String simpleName = getSimpleName(importTree);
             if (!isUnused(unit, usedNames, usedInJavadoc, importTree, simpleName)) {
@@ -234,16 +235,62 @@ private static RangeMap<Integer, String> buildReplacements(
             // delete the import
             int endPosition = importTree.getEndPosition(unit.endPositions);
             endPosition = Math.max(CharMatcher.isNot(' ').indexIn(contents, endPosition), endPosition);
-            String sep = Newlines.guessLineSeparator(contents);
             if (endPosition + sep.length() < contents.length()
                     && contents.subSequence(endPosition, endPosition + sep.length())
                             .toString()
                             .equals(sep)) {
                 endPosition += sep.length();
             }
-            replacements.put(Range.closedOpen(importTree.getStartPosition(), endPosition), "");
+            // putCoalescing merges adjacent unused imports into one range, so the blank-line cleanup below sees the
+            // whole deleted import block (TreeRangeMap.put does not coalesce).
+            replacements.putCoalescing(Range.closedOpen(importTree.getStartPosition(), endPosition), "");
         }
-        return replacements;
+        // Removing a whole import block can leave the blank line that preceded it stacked on the blank line that
+        // followed it (package, blank, imports, blank, type). Collapse one of them, so a single formatting pass leaves
+        // one blank line, as the second one did.
+        return collapseBlankLinesAroundDeletedImports(contents, replacements, sep);
+    }
+
+    /**
+     * Extends contiguous deleted-import ranges so that a blank line that both preceded and followed the imports is not
+     * left doubled after the deletion.
+     */
+    private static RangeMap<Integer, String> collapseBlankLinesAroundDeletedImports(
+            String contents, RangeMap<Integer, String> replacements, String sep) {
+        if (replacements.asMapOfRanges().isEmpty()) {
+            return replacements;
+        }
+        RangeMap<Integer, String> adjusted = TreeRangeMap.create();
+        for (Range<Integer> range : replacements.asMapOfRanges().keySet()) {
+            int start = range.lowerEndpoint();
+            int end = range.upperEndpoint();
+            // Eat one trailing blank line when the deletion sits between blank lines, or at the start of the file,
+            // where a leading blank line would otherwise remain after the last import is removed.
+            if (isBlankLineAfter(contents, end, sep) && (start == 0 || isBlankLineBefore(contents, start, sep))) {
+                end += sep.length();
+            }
+            adjusted.putCoalescing(Range.closedOpen(start, end), "");
+        }
+        return adjusted;
+    }
+
+    /** True if {@code pos} is immediately preceded by an empty line. */
+    private static boolean isBlankLineBefore(String contents, int pos, String sep) {
+        if (pos < sep.length() || !contents.regionMatches(pos - sep.length(), sep, 0, sep.length())) {
+            return false;
+        }
+        int endOfPreviousLine = pos - sep.length();
+        if (endOfPreviousLine == 0) {
+            // The file begins with a blank line before the deleted import.
+            return true;
+        }
+        return endOfPreviousLine >= sep.length()
+                && contents.regionMatches(endOfPreviousLine - sep.length(), sep, 0, sep.length());
+    }
+
+    /** True if {@code pos} is immediately followed by an empty line (a line break). */
+    private static boolean isBlankLineAfter(String contents, int pos, String sep) {
+        return pos + sep.length() <= contents.length() && contents.regionMatches(pos, sep, 0, sep.length());
     }
 
     private static String getSimpleName(ImportTree importTree) {

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

@@ -266,6 +266,46 @@ public void importRemovalLines() throws Exception {
         assertThat(out.toString()).isEqualTo(joiner.join(expected));
     }
 
+    // An unused import between two blank lines must not leave both of them behind: one run of the command line gives
+    // what a second run would, and what the entry point of the Gradle and Spotless step gives (#37, from
+    // google/google-java-format#1436).
+    @Test
+    public void unusedImportRemovalLeavesOneBlankLine() throws Exception {
+        String[] input = {
+            "package com.example;",
+            "",
+            "import static io.grpc.MethodDescriptor.generateFullMethodName;",
+            "",
+            "/**",
+            " * Javadoc for class.",
+            " */",
+            "public class TestBug {",
+            "}",
+            "",
+        };
+        String[] expected = {
+            "package com.example;", //
+            "",
+            "/**",
+            " * Javadoc for class.",
+            " */",
+            "public class TestBug {}",
+            "",
+        };
+        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(input).getBytes(UTF_8)));
+        assertThat(main.format("-")).isEqualTo(0);
+        assertThat(out.toString()).isEqualTo(joiner.join(expected));
+
+        Formatter formatter = Formatter.createFormatter(JavaFormatterOptions.builder()
+                .style(JavaFormatterOptions.Style.OJF)
+                .build());
+        assertThat(formatter.formatSourceAndFixImports(joiner.join(input))).isEqualTo(joiner.join(expected));
+    }
+
     // test that errors are reported on the right line when imports are removed
     @Test
     public void importRemoveErrorParseError() throws Exception {

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

@@ -254,6 +254,74 @@ public static List<Object[]> parameters() {
                     "interface Test { private static void foo() {} }",
                 },
             },
+            // An unused import between blank lines takes one of them with it (#37, from
+            // google/google-java-format#1436 and google/google-java-format#1437).
+            {
+                {
+                    "package com.example;",
+                    "",
+                    "import static io.grpc.MethodDescriptor.generateFullMethodName;",
+                    "",
+                    "/**",
+                    " * Javadoc for class.",
+                    " */",
+                    "public class TestBug {}",
+                },
+                {
+                    "package com.example;", //
+                    "",
+                    "/**",
+                    " * Javadoc for class.",
+                    " */",
+                    "public class TestBug {}",
+                },
+            },
+            {
+                {
+                    "package com.example;",
+                    "",
+                    "import com.foo.Unused1;",
+                    "import com.foo.Unused2;",
+                    "",
+                    "public class TestBug {}",
+                },
+                {
+                    "package com.example;", //
+                    "",
+                    "public class TestBug {}",
+                },
+            },
+            {
+                {
+                    "import com.foo.Unused;", //
+                    "",
+                    "public class TestBug {}",
+                },
+                {
+                    "public class TestBug {}",
+                },
+            },
+            {
+                {
+                    "package com.example;",
+                    "",
+                    "import java.util.List;",
+                    "import com.foo.Unused;",
+                    "",
+                    "public class TestBug {",
+                    "    List<String> xs;",
+                    "}",
+                },
+                {
+                    "package com.example;",
+                    "",
+                    "import java.util.List;",
+                    "",
+                    "public class TestBug {",
+                    "    List<String> xs;",
+                    "}",
+                },
+            },
         };
         ImmutableList.Builder<Object[]> builder = ImmutableList.builder();
         for (String[][] inputAndOutput : inputsOutputs) {