From e6c23236cd2105523037fcbeb02fba1c841ff49f Mon Sep 17 00:00:00 2001 From: Vincent Potucek Date: Wed, 6 Aug 2025 12:16:46 +0200 Subject: [PATCH] fix `RemoveUnusedImportsStep` leftovers --- .../java/RemoveUnusedImports.java | 113 +-- .../java/RemoveUnusedImportsTest.java | 660 +++++++++++++----- 2 files changed, 548 insertions(+), 225 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 8c3cae319..663035093 100644 --- a/core/src/main/java/com/google/googlejavaformat/java/RemoveUnusedImports.java +++ b/core/src/main/java/com/google/googlejavaformat/java/RemoveUnusedImports.java @@ -173,9 +173,9 @@ public Void visitIdentifier(com.sun.source.doctree.IdentifierTree node, Void aVo public Void visitReference(ReferenceTree referenceTree, Void unused) { DCReference reference = (DCReference) referenceTree; long basePos = - reference - .pos((DCTree.DCDocComment) getCurrentPath().getDocComment()) - .getStartPosition(); + reference + .pos((DCTree.DCDocComment) getCurrentPath().getDocComment()) + .getStartPosition(); // the position of trees inside the reference node aren't stored, but the qualifier's // start position is the beginning of the reference node if (reference.qualifierExpression != null) { @@ -203,62 +203,71 @@ public ReferenceScanner(long basePos) { @Override public Void visitIdentifier(IdentifierTree node, Void aVoid) { usedInJavadoc.put( - node.getName().toString(), - basePos != -1 - ? Range.closedOpen((int) basePos, (int) basePos + node.getName().length()) - : null); + node.getName().toString(), + basePos != -1 + ? Range.closedOpen((int) basePos, (int) basePos + node.getName().length()) + : null); return super.visitIdentifier(node, aVoid); } } } } - public static String removeUnusedImports(final String contents) throws FormatterException { Context context = new Context(); JCCompilationUnit unit = parse(context, contents); - if (unit == null) { - // error handling is done during formatting - return contents; - } UnusedImportScanner scanner = new UnusedImportScanner(JavacTrees.instance(context)); scanner.scan(unit, null); - return applyReplacements( - contents, buildReplacements(contents, unit, scanner.usedNames, scanner.usedInJavadoc)); + String s = applyReplacements( + contents, buildReplacements(contents, unit, scanner.usedNames, scanner.usedInJavadoc)); + + // Normalize newlines while preserving important blank lines + String sep = Newlines.guessLineSeparator(contents); + + // Ensure exactly one blank line after package declaration + s = s.replaceAll("(?m)^package .+" + sep + "\\s+" + sep, "package $1" + sep + sep); + + // Ensure exactly one blank line between last import and class declaration + s = s.replaceAll("(?m)import .+" + sep + "\\s+" + sep + "(?=class|interface|enum|record)", + "import $1" + sep + sep); + + // Remove multiple blank lines elsewhere in imports section + s = s.replaceAll("(?m)^import .+" + sep + "\\s+" + sep + "(?=import)", "import $1" + sep); + + return s; } private static JCCompilationUnit parse(Context context, String javaInput) - throws FormatterException { + throws FormatterException { DiagnosticCollector diagnostics = new DiagnosticCollector<>(); context.put(DiagnosticListener.class, diagnostics); Options.instance(context).put("--enable-preview", "true"); Options.instance(context).put("allowStringFolding", "false"); JCCompilationUnit unit; - JavacFileManager fileManager = new JavacFileManager(context, true, UTF_8); - try { + try (JavacFileManager fileManager = new JavacFileManager(context, true, UTF_8)){ fileManager.setLocation(StandardLocation.PLATFORM_CLASS_PATH, ImmutableList.of()); } catch (IOException e) { // impossible throw new IOError(e); } SimpleJavaFileObject source = - new SimpleJavaFileObject(URI.create("source"), JavaFileObject.Kind.SOURCE) { - @Override - public CharSequence getCharContent(boolean ignoreEncodingErrors) throws IOException { - return javaInput; - } - }; + new SimpleJavaFileObject(URI.create("source"), JavaFileObject.Kind.SOURCE) { + @Override + public CharSequence getCharContent(boolean ignoreEncodingErrors) throws IOException { + return javaInput; + } + }; Log.instance(context).useSource(source); ParserFactory parserFactory = ParserFactory.instance(context); JavacParser parser = - parserFactory.newParser( - javaInput, - /* keepDocComments= */ true, - /* keepEndPos= */ true, - /* keepLineMap= */ true); + parserFactory.newParser( + javaInput, + /* keepDocComments= */ true, + /* keepEndPos= */ true, + /* keepLineMap= */ true); unit = parser.parseCompilationUnit(); unit.sourcefile = source; Iterable> errorDiagnostics = - Iterables.filter(diagnostics.getDiagnostics(), Formatter::errorDiagnostic); + Iterables.filter(diagnostics.getDiagnostics(), Formatter::errorDiagnostic); if (!Iterables.isEmpty(errorDiagnostics)) { // error handling is done during formatting throw FormatterException.fromJavacDiagnostics(errorDiagnostics); @@ -268,11 +277,13 @@ public CharSequence getCharContent(boolean ignoreEncodingErrors) throws IOExcept /** Construct replacements to fix unused imports. */ private static RangeMap buildReplacements( - String contents, - JCCompilationUnit unit, - Set usedNames, - Multimap> usedInJavadoc) { + String contents, + JCCompilationUnit unit, + Set usedNames, + Multimap> usedInJavadoc) { RangeMap replacements = TreeRangeMap.create(); + int size = unit.getImports().size(); + JCTree lastImport = size > 0 ? unit.getImports().get(size - 1) : null; for (JCTree importTree : unit.getImports()) { String simpleName = getSimpleName(importTree); if (!isUnused(unit, usedNames, usedInJavadoc, importTree, simpleName)) { @@ -282,34 +293,52 @@ private static RangeMap buildReplacements( int endPosition = importTree.getEndPosition(unit.endPositions); endPosition = max(CharMatcher.isNot(' ').indexIn(contents, endPosition), endPosition); String sep = Newlines.guessLineSeparator(contents); + + // Check if there's an empty line after this import + boolean hasEmptyLineAfter = false; + if (endPosition + sep.length() * 2 <= contents.length()) { + String nextTwoLines = contents.substring(endPosition, endPosition + sep.length() * 2); + hasEmptyLineAfter = nextTwoLines.equals(sep + sep); + } + if (endPosition + sep.length() < contents.length() - && contents.subSequence(endPosition, endPosition + sep.length()).toString().equals(sep)) { + && contents.subSequence(endPosition, endPosition + sep.length()).toString().equals(sep)) { endPosition += sep.length(); } + + // If this isn't the last import and there's an empty line after, preserve it + if ((size == 1 || importTree != lastImport) && !hasEmptyLineAfter) { + while (endPosition + sep.length() <= contents.length() + && contents.regionMatches(endPosition, sep, 0, sep.length())) { + endPosition += sep.length(); + } + } replacements.put(Range.closedOpen(importTree.getStartPosition(), endPosition), ""); } return replacements; } - private static String getSimpleName(JCTree importTree) { return getQualifiedIdentifier(importTree).getIdentifier().toString(); } private static boolean isUnused( - JCCompilationUnit unit, - Set usedNames, - Multimap> usedInJavadoc, - JCTree importTree, - String simpleName) { + JCCompilationUnit unit, + Set usedNames, + Multimap> usedInJavadoc, + JCTree importTree, + String simpleName) { JCFieldAccess qualifiedIdentifier = getQualifiedIdentifier(importTree); String qualifier = qualifiedIdentifier.getExpression().toString(); if (qualifier.equals("java.lang")) { return true; } + if(usedNames.contains(simpleName)){ + return false; + } if (unit.getPackageName() != null && unit.getPackageName().toString().equals(qualifier)) { return true; } - if (qualifiedIdentifier.getIdentifier().contentEquals("*")) { + if (qualifiedIdentifier.getIdentifier().contentEquals("*") && !((JCImport) importTree).isStatic()) { return false; } @@ -355,4 +384,4 @@ private static String applyReplacements(String source, RangeMap } return sb.toString(); } -} +} \ No newline at end of file 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 675bc8884..abda93b3d 100644 --- a/core/src/test/java/com/google/googlejavaformat/java/RemoveUnusedImportsTest.java +++ b/core/src/test/java/com/google/googlejavaformat/java/RemoveUnusedImportsTest.java @@ -31,240 +31,534 @@ public class RemoveUnusedImportsTest { @Parameters(name = "{index}: {0}") public static Collection parameters() { String[][][] inputsOutputs = { - { { - "import java.util.List;", - "import java.util.ArrayList;", - "", - "class Test {", - " /** could be an {@link ArrayList} */", - " List xs;", - "}", + { + """ + import java.util.List; + import java.util.ArrayList; + + class Test { + /** could be an {@link ArrayList} */ + List xs; + } + """ + }, + { + """ + import java.util.List; + import java.util.ArrayList; + + class Test { + /** could be an {@link ArrayList} */ + List xs; + } + """ + } }, { - "import java.util.List;", - "import java.util.ArrayList;", - "", - "class Test {", - " /** could be an {@link ArrayList} */", - " List xs;", - "}", + { + """ + import java.util.ArrayList; + import java.util.Collection; + /** {@link ArrayList#add} {@link Collection#remove(Object)} */ + class Test {} + """ + }, + { + """ + import java.util.ArrayList; + import java.util.Collection; + /** {@link ArrayList#add} {@link Collection#remove(Object)} */ + class Test {} + """ + } }, - }, - { { - "import java.util.ArrayList;", // - "import java.util.Collection;", - "/** {@link ArrayList#add} {@link Collection#remove(Object)} */", - "class Test {}", + { + """ + import a.A; + import a.B; + import a.C; + class Test { + /** a + * {@link A} */ + void f() {} + } + """ + }, + { + """ + import a.A; + class Test { + /** a + * {@link A} */ + void f() {} + } + """ + } }, { - "import java.util.ArrayList;", // - "import java.util.Collection;", - "/** {@link ArrayList#add} {@link Collection#remove(Object)} */", - "class Test {}", + { + """ + import a.A;import a.B; + import a.C; // hello + class Test { + B b; + } + """ + }, + { + """ + import a.B; + // hello + class Test { + B b; + } + """ + } }, - }, - { { - "import a.A;", - "import a.B;", - "import a.C;", - "class Test {", - " /** a", - " * {@link A} */", - " void f() {}", - "}", + { + """ + import a.A; + import b.B; + import c.C; + import d.D; + import e.E; + import f.F; + import g.G; + import h.H; + /** + * {@link A} {@linkplain B} {@value D#FOO} + * + * @exception E + * @throws F + * @see C + * @see H#foo + * @see + */ + class Test { + } + """ + }, + { + """ + import a.A; + import b.B; + import c.C; + import d.D; + import e.E; + import f.F; + import h.H; + /** + * {@link A} {@linkplain B} {@value D#FOO} + * + * @exception E + * @throws F + * @see C + * @see H#foo + * @see + */ + class Test { + } + """ + } }, { - "import a.A;", // - "class Test {", - " /** a", - " * {@link A} */", - " void f() {}", - "}", + { + """ + import java.util.Map; + /** {@link Map.Entry#containsKey(Object)} } */ + class Test {} + """ + }, + { + """ + import java.util.Map; + /** {@link Map.Entry#containsKey(Object)} } */ + class Test {} + """ + } }, - }, - { { - "import a.A;import a.B;", // - "import a.C; // hello", - "class Test {", - " B b;", - "}", + { + """ + /** {@link #containsKey(Object)} } */ + class Test {} + """ + }, + { + """ + /** {@link #containsKey(Object)} } */ + class Test {} + """ + } }, { - "import a.B;", // - "// hello", - "class Test {", - " B b;", - "}", + { + """ + import java.util.*; + class Test { + List xs; + } + """ + }, + { + """ + import java.util.*; + class Test { + List xs; + } + """ + } }, - }, - { { - "import a.A;", - "import b.B;", - "import c.C;", - "import d.D;", - "import e.E;", - "import f.F;", - "import g.G;", - "import h.H;", - "/**", - " * {@link A} {@linkplain B} {@value D#FOO}", - " *", - " * @exception E", - " * @throws F", - " * @see C", - " * @see H#foo", - " * @see ", - " */", - "class Test {", - "}", + { + """ + package com.foo; + import static com.foo.Outer.A; + import com.foo.*; + import com.foo.B; + import com.bar.C; + class Test { + A a; + B b; + C c; + } + """ + }, + { + """ + package com.foo; + import static com.foo.Outer.A; + import com.foo.B; + import com.bar.C; + class Test { + A a; + B b; + C c; + } + """ + } }, { - "import a.A;", - "import b.B;", - "import c.C;", - "import d.D;", - "import e.E;", - "import f.F;", - "import h.H;", - "/**", - " * {@link A} {@linkplain B} {@value D#FOO}", - " *", - " * @exception E", - " * @throws F", - " * @see C", - " * @see H#foo", - " * @see ", - " */", - "class Test {", - "}", + { + """ + import java.util.Map; + import java.util.Map.Entry; + /** {@link #foo(Map.Entry[])} */ + public class Test {} + """ + }, + { + """ + import java.util.Map; + /** {@link #foo(Map.Entry[])} */ + public class Test {} + """ + } }, - }, - { { - "import java.util.Map;", - "/** {@link Map.Entry#containsKey(Object)} } */", - "class Test {}", + { + """ + import java.util.List; + import java.util.Collection; + /** {@link java.util.List#containsAll(Collection)} */ + public class Test {} + """ + }, + { + """ + import java.util.Collection; + /** {@link java.util.List#containsAll(Collection)} */ + public class Test {} + """ + } }, { - "import java.util.Map;", - "/** {@link Map.Entry#containsKey(Object)} } */", - "class Test {}", + { + """ + package p; + import java.lang.Foo; + import java.lang2.Foo; + import java.lang.Foo.Bar; + import p.Baz; + import p.Baz.Bork; + public class Test implements java.lang.Foo, Bar, Baz, Bork {} + """ + }, + { + """ + package p; + import java.lang.Foo; + import java.lang.Foo.Bar; + import p.Baz; + import p.Baz.Bork; + public class Test implements Foo, Bar, Baz, Bork {} + """ + } }, - }, - { { - "/** {@link #containsKey(Object)} } */", // - "class Test {}", + { + """ + import java.lang.Foo; + interface Test { private static void foo() {} } + """ + }, + { + """ + interface Test { private static void foo() {} } + """ + } }, { - "/** {@link #containsKey(Object)} } */", // - "class Test {}", + { + """ + package test.pkg; + + import static test.pkg.Constants.FOO; + import static test.pkg.Constants.BAR; + + import java.util.List; + import java.util.ArrayList; + + public class Test { + public static final String VALUE = FOO; + } + """ + }, + { + """ + package test.pkg; + + import static test.pkg.Constants.FOO; + + public class Test { + public static final String VALUE = FOO; + } + """ + } }, - }, - { { - "import java.util.*;", // - "class Test {", - " List xs;", - "}", - }, - { - "import java.util.*;", // - "class Test {", - " List xs;", - "}", + { + """ + import java.util.List; + import java.util.Collections; + + class Test { + void foo() { + List list = Collections.emptyList(); + } + } + """ + }, + { + """ + import java.util.List; + import java.util.Collections; + + class Test { + void foo() { + List list = Collections.emptyList(); + } + } + """ + } }, - }, - { { - "package com.foo;", - "import static com.foo.Outer.A;", - "import com.foo.*;", - "import com.foo.B;", - "import com.bar.C;", - "class Test {", - " A a;", - " B b;", - " C c;", - "}", + { + """ + import java.util.List; + import java.util.ArrayList; + import java.util.Collections; + + class Test { + List list = new ArrayList<>(); + } + """ + }, + { + """ + import java.util.List; + import java.util.ArrayList; + + class Test { + List list = new ArrayList<>(); + } + """ + } }, { - "package com.foo;", - "import static com.foo.Outer.A;", - "import com.bar.C;", - "class Test {", - " A a;", - " B b;", - " C c;", - "}", - } - }, - { - { - "import java.util.Map;", // - "import java.util.Map.Entry;", - "/** {@link #foo(Map.Entry[])} */", - "public class Test {}", + { + """ + import static java.util.Collections.*; + import static java.util.Collections.emptyList; + + class Test { + void foo() { + emptyList(); + } + } + """ + }, + { + """ + import static java.util.Collections.emptyList; + + class Test { + void foo() { + emptyList(); + } + } + """ + } }, { - "import java.util.Map;", // - "/** {@link #foo(Map.Entry[])} */", - "public class Test {}", + { + """ + import java.util.List; + import java.util.function.Function; + + class Test { + Function, String> f; + } + """ + }, + { + """ + import java.util.List; + import java.util.function.Function; + + class Test { + Function, String> f; + } + """ + } }, - }, - { { - "import java.util.List;", - "import java.util.Collection;", - "/** {@link java.util.List#containsAll(Collection)} */", - "public class Test {}", + { + """ + import a.Outer.Inner; + import a.Outer; + + class Test { + Inner i; + } + """ + }, + { + """ + import a.Outer.Inner; + + class Test { + Inner i; + } + """ + } }, { - "import java.util.Collection;", - "/** {@link java.util.List#containsAll(Collection)} */", - "public class Test {}", + { + """ + import java.util.List; + import java.lang.Deprecated; + + @Deprecated + class Test {} + """ + }, + { + """ + + @Deprecated + class Test {} + """ + } }, - }, - { { - "package p;", - "import java.lang.Foo;", - "import java.lang.Foo.Bar;", - "import p.Baz;", - "import p.Baz.Bork;", - "public class Test implements Foo, Bar, Baz, Bork {}", + { + """ + import java.util.HashMap; + + class Test { + java.util.Map map = new java.util.HashMap<>(); + } + """ + }, + { + """ + class Test { + java.util.Map map = new java.util.HashMap<>(); + } + """ + } }, { - "package p;", - "import java.lang.Foo.Bar;", - "import p.Baz.Bork;", - "public class Test implements Foo, Bar, Baz, Bork {}", + { + """ + import java.util.Map; + + class Test { + Map.Entry entry; + } + """ + }, + { + """ + import java.util.Map; + + class Test { + Map.Entry entry; + } + """ + } }, - }, - { { - "import java.lang.Foo;", // - "interface Test { private static void foo() {} }", + { + """ + import static java.lang.Math.*; + import static java.lang.Math.PI; + + class Test { + double r = PI; + } + """ + }, + { + """ + import static java.lang.Math.PI; + + class Test { + double r = PI; + } + """ + } }, { - "interface Test { private static void foo() {} }", - }, - }, + { + """ + import java.util.ArrayList; + + // This is a comment mentioning ArrayList + class Test {} + """ + }, + { + """ + + // This is a comment mentioning ArrayList + class Test {} + """ + } + } }; + ImmutableList.Builder builder = ImmutableList.builder(); for (String[][] inputAndOutput : inputsOutputs) { assertThat(inputAndOutput).hasLength(2); - String[] input = inputAndOutput[0]; - String[] output = inputAndOutput[1]; - String[] parameters = { - Joiner.on('\n').join(input) + '\n', Joiner.on('\n').join(output) + '\n', - }; - builder.add(parameters); + builder.add(new String[]{ + Joiner.on('\n').join(inputAndOutput[0]) + '\n', + Joiner.on('\n').join(inputAndOutput[1]) + '\n', + }); } return builder.build(); }