diff --git a/open-java-format/src/main/java/com/palantir/javaformat/java/RemoveUnusedImports.java b/open-java-format/src/main/java/com/palantir/javaformat/java/RemoveUnusedImports.java index a66b6b559..be322711f 100644 --- a/open-java-format/src/main/java/com/palantir/javaformat/java/RemoveUnusedImports.java +++ b/open-java-format/src/main/java/com/palantir/javaformat/java/RemoveUnusedImports.java @@ -226,6 +226,7 @@ private static RangeMap buildReplacements( Set usedNames, Multimap> usedInJavadoc) { RangeMap 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 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 collapseBlankLinesAroundDeletedImports( + String contents, RangeMap replacements, String sep) { + if (replacements.asMapOfRanges().isEmpty()) { + return replacements; + } + RangeMap adjusted = TreeRangeMap.create(); + for (Range 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) { 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..8c4427c41 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 @@ -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 { diff --git a/open-java-format/src/test/java/com/palantir/javaformat/java/RemoveUnusedImportsTest.java b/open-java-format/src/test/java/com/palantir/javaformat/java/RemoveUnusedImportsTest.java index cdea6fe04..0efac4771 100644 --- a/open-java-format/src/test/java/com/palantir/javaformat/java/RemoveUnusedImportsTest.java +++ b/open-java-format/src/test/java/com/palantir/javaformat/java/RemoveUnusedImportsTest.java @@ -254,6 +254,74 @@ public static List 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 xs;", + "}", + }, + { + "package com.example;", + "", + "import java.util.List;", + "", + "public class TestBug {", + " List xs;", + "}", + }, + }, }; ImmutableList.Builder builder = ImmutableList.builder(); for (String[][] inputAndOutput : inputsOutputs) {