From 1b2e65784edbddad213ded063fd4edad445ca17b Mon Sep 17 00:00:00 2001 From: arimu1 <19286898+arimu1@users.noreply.github.com> Date: Thu, 6 Aug 2026 06:44:20 +0700 Subject: [PATCH] fix: avoid extra blank line after unused import removal (#1436) When every import between the package blank and the type blank is removed, collapse one of the stacked blank lines so a single CLI pass stays style-compliant (format-then-fix-imports path). --- .../java/RemoveUnusedImports.java | 55 ++++++++++++++- .../googlejavaformat/java/MainTest.java | 36 ++++++++++ .../java/RemoveUnusedImportsTest.java | 67 +++++++++++++++++++ 3 files changed, 155 insertions(+), 3 deletions(-) diff --git a/core/src/main/java/com/google/googlejavaformat/java/RemoveUnusedImports.java b/core/src/main/java/com/google/googlejavaformat/java/RemoveUnusedImports.java index 4b2211eae..01ca80298 100644 --- a/core/src/main/java/com/google/googlejavaformat/java/RemoveUnusedImports.java +++ b/core/src/main/java/com/google/googlejavaformat/java/RemoveUnusedImports.java @@ -234,6 +234,7 @@ private static RangeMap buildReplacements( Set usedNames, Multimap> usedInJavadoc) { RangeMap replacements = TreeRangeMap.create(); + String sep = Newlines.guessLineSeparator(contents); for (JCTree importTree : unit.getImports()) { if (isModuleImport(importTree)) { continue; @@ -245,14 +246,62 @@ private static RangeMap buildReplacements( // delete the import int endPosition = getEndPosition(importTree, unit); endPosition = 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 span so blank-line cleanup can see + // 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 (e.g. package → blank → imports → blank → type). Collapse one + // of those blanks so a single formatting pass stays style-compliant (#1436). + return collapseBlankLinesAroundDeletedImports(contents, replacements, sep); + } + + /** + * Extends contiguous deleted-import ranges so 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 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) { + // 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(JCTree importTree) { diff --git a/core/src/test/java/com/google/googlejavaformat/java/MainTest.java b/core/src/test/java/com/google/googlejavaformat/java/MainTest.java index 76b99baaf..2f026dd84 100644 --- a/core/src/test/java/com/google/googlejavaformat/java/MainTest.java +++ b/core/src/test/java/com/google/googlejavaformat/java/MainTest.java @@ -256,6 +256,42 @@ class Test extends ArrayList {} assertThat(out.toString()).isEqualTo(expected); } + // https://github.com/google/google-java-format/issues/1436 + @Test + public void unusedImportRemovalDoesNotLeaveDoubleBlankBeforeJavadoc() 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 {} + """; + + assertThat(new Formatter().formatSourceAndFixImports(input)).isEqualTo(expected); + + InputStream in = new ByteArrayInputStream(input.getBytes(UTF_8)); + StringWriter out = new StringWriter(); + Main main = + new Main( + new PrintWriter(out, true), + new PrintWriter(new BufferedWriter(new OutputStreamWriter(System.err, UTF_8)), true), + in); + assertThat(main.format("-")).isEqualTo(0); + assertThat(out.toString()).isEqualTo(expected); + } + // test that -lines handling works with import removal @Test public void importRemovalLines() throws Exception { diff --git a/core/src/test/java/com/google/googlejavaformat/java/RemoveUnusedImportsTest.java b/core/src/test/java/com/google/googlejavaformat/java/RemoveUnusedImportsTest.java index f186b846e..44b08673c 100644 --- a/core/src/test/java/com/google/googlejavaformat/java/RemoveUnusedImportsTest.java +++ b/core/src/test/java/com/google/googlejavaformat/java/RemoveUnusedImportsTest.java @@ -265,6 +265,73 @@ interface Test { private static void foo() {} } interface Test { private static void foo() {} } """, }, + // #1436: unused import between package and class Javadoc must not leave a double blank line + { + """ + 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) {