From 3632838042f990a085769ecae0f1a5ade49fbebd Mon Sep 17 00:00:00 2001 From: arimu1 <19286898+arimu1@users.noreply.github.com> Date: Wed, 23 Sep 2026 10:34:13 +0300 Subject: [PATCH] Stop an unused import from leaving two blank lines behind The command line formats first and fixes imports afterwards. When RemoveUnusedImports deleted an import that sat between two blank lines, such as the only import between the package line and the class, it left both blank lines, and nothing ran after it to collapse them. A second run did, so --replace wrote files that --set-exit-if-changed then reported (#37, from google/google-java-format#598 and google/google-java-format#1436). The Gradle and Spotless entry point fixes imports before it formats and was not affected, so the two disagreed about these files. This ports google/google-java-format#1437 by arimu1: adjacent unused imports are deleted as one range, and a range that sits between blank lines, or at the start of the file, takes one of them along. The code is adapted to this codebase. The four RemoveUnusedImportsTest cases come from the upstream PR, and a MainTest checks that the command line and formatSourceAndFixImports now give the same file. Of the 15,747 files of the JDK 21 sources, 304 format differently, by 305 blank lines fewer in all and no other change. All 304 are files the old code changed again on a second run, and the new output is stable on every one of them. --- .../javaformat/java/RemoveUnusedImports.java | 53 ++++++++++++++- .../palantir/javaformat/java/MainTest.java | 40 +++++++++++ .../java/RemoveUnusedImportsTest.java | 68 +++++++++++++++++++ 3 files changed, 158 insertions(+), 3 deletions(-) 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) {