From bff74a00cf54e5c45d93ad9ef076e514c82277a1 Mon Sep 17 00:00:00 2001 From: Zayan Khan <108294002+ZayanKhan-12@users.noreply.github.com> Date: Tue, 15 Sep 2026 13:31:06 -0400 Subject: [PATCH 1/2] Keep java.lang imports that disambiguate a simple name `RemoveUnusedImports` deleted every import qualified by `java.lang` unconditionally. That is unsound: per JLS 6.4.1 a single-type-import declaration shadows both types declared elsewhere in the current package and types brought in by an import-on-demand, so `import java.lang.Byte;` can be the only thing binding `Byte` to the JDK class. Removing it either makes the reference ambiguous: error: reference to Byte is ambiguous both class com.company.project.foo.Byte in com.company.project.foo and class java.lang.Byte in java.lang match or silently rebinds the name to a same-package class, which then fails to compile against the wrong type. `RemoveUnusedImports` analyses one compilation unit with no classpath and no symbol resolution, so it cannot prove that no clashing type exists. Only remove a `java.lang` single-type-import when its simple name is not referenced at all. Unchanged: `import java.lang.*;` is still removed (java.lang is implicitly imported on demand), unused `java.lang` imports are still removed, and imports of same-package types are still removed -- package members already shadow import-on-demand declarations, so those can never be disambiguating. One existing test case encoded the old behaviour and has been updated. Fixes #1334 Co-Authored-By: Claude Opus 5 (1M context) (cherry picked from commit c9d7b75d3436d25ee99e3ff82fe6ca70c71c7f98) --- .../javaformat/java/RemoveUnusedImports.java | 37 ++++++--- .../java/RemoveUnusedImportsTest.java | 75 +++++++++++++++++++ 2 files changed, 100 insertions(+), 12 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 1d2fae6d5..a42aee5ec 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 @@ -327,26 +327,39 @@ private static boolean isUnused( String qualifier = ((JCFieldAccess) importTree.getQualifiedIdentifier()) .getExpression() .toString(); + boolean onDemand = isOnDemand(importTree); if (qualifier.equals("java.lang")) { - return true; + // `import java.lang.*;` is always redundant, because java.lang is implicitly imported on demand. + // + // A single-type-import such as `import java.lang.Byte;` is *not* always redundant: JLS 6.4.1 says it + // shadows both types named `Byte` declared elsewhere in the current package and types named `Byte` + // brought in by an import-on-demand. Dropping it can therefore change which type a simple name resolves + // to, or make the reference ambiguous and fail to compile. We only ever see a single compilation unit + // here, so we cannot prove that no such clash exists; keep the import whenever its name is referenced. + return onDemand || !isUsed(usedNames, usedInJavadoc, simpleName); } if (unit.getPackageName() != null && unit.getPackageName().toString().equals(qualifier)) { + // Importing a type from the current package is always redundant: types declared in the current package + // already shadow import-on-demand declarations (JLS 6.4.1 - an import-on-demand never shadows anything), + // so the import cannot be disambiguating anything. return true; } - if (importTree.getQualifiedIdentifier() instanceof JCFieldAccess - && ((JCFieldAccess) importTree.getQualifiedIdentifier()) - .getIdentifier() - .contentEquals("*")) { + if (onDemand) { return false; } + return !isUsed(usedNames, usedInJavadoc, simpleName); + } - if (usedNames.contains(simpleName)) { - return false; - } - if (usedInJavadoc.containsKey(simpleName)) { - return false; - } - return true; + private static boolean isOnDemand(ImportTree importTree) { + return importTree.getQualifiedIdentifier() instanceof JCFieldAccess + && ((JCFieldAccess) importTree.getQualifiedIdentifier()) + .getIdentifier() + .contentEquals("*"); + } + + private static boolean isUsed( + Set usedNames, Multimap> usedInJavadoc, String simpleName) { + return usedNames.contains(simpleName) || usedInJavadoc.containsKey(simpleName); } /** Applies the replacements to the given source, and re-format any edited javadoc. */ 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 4981763f7..509ffd39b 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 @@ -240,7 +240,11 @@ public static List parameters() { "public class Test implements Foo, Bar, Baz, Bork {}", }, { + // `import java.lang.Foo;` is kept: `p` may also declare a `Foo`, which the + // single-type-import shadows (JLS 6.4.1). `import p.Baz;` is still dropped, since a type + // in the current package never needs importing. "package p;", + "import java.lang.Foo;", "import java.lang.Foo.Bar;", "import p.Baz.Bork;", "public class Test implements Foo, Bar, Baz, Bork {}", @@ -338,6 +342,77 @@ public static List parameters() { "}", }, }, + // https://github.com/palantir/palantir-java-format/issues/1334: a single-type-import of a java.lang + // type shadows same-package types and on-demand imports, so it must not be removed while it is used. + { + // disambiguates `Byte` against an on-demand import + { + "package com.company.project.bar;", + "import com.company.project.foo.*;", + "import java.lang.Byte;", + "public class Test { Byte foo() { return null; } }", + }, + { + "package com.company.project.bar;", + "import com.company.project.foo.*;", + "import java.lang.Byte;", + "public class Test { Byte foo() { return null; } }", + }, + }, + { + // disambiguates `Byte` against a class declared in the same package + { + "package com.company.project.bar;", + "import java.lang.Byte;", + "public class Test { Byte foo() { return null; } }", + }, + { + "package com.company.project.bar;", + "import java.lang.Byte;", + "public class Test { Byte foo() { return null; } }", + }, + }, + { + // a java.lang import that is only referenced from javadoc is still load-bearing + { + "package p;", // + "import java.lang.Byte;", + "/** {@link Byte} */", + "public class Test {}", + }, + { + "package p;", // + "import java.lang.Byte;", + "/** {@link Byte} */", + "public class Test {}", + }, + }, + { + // `import java.lang.*;` is always redundant, java.lang is implicitly imported on demand + { + "package p;", // + "import java.lang.*;", + "public class Test { Byte foo() { return null; } }", + }, + { + "package p;", // + "public class Test { Byte foo() { return null; } }", + }, + }, + { + // an unused java.lang import is still removed, even alongside an on-demand import + { + "package p;", + "import java.lang.Byte;", + "import q.*;", + "public class Test { Short foo() { return null; } }", + }, + { + "package p;", // + "import q.*;", + "public class Test { Short foo() { return null; } }", + }, + }, }; ImmutableList.Builder builder = ImmutableList.builder(); for (String[][] inputAndOutput : inputsOutputs) { From e66ee55fd7ea0fa67ce20a2711c2ad830e059f71 Mon Sep 17 00:00:00 2001 From: Alex Abashev Date: Fri, 30 Oct 2026 19:37:18 +0300 Subject: [PATCH 2/2] Move the tests of unused import removal into case files RemoveUnusedImportsTest was a table inherited from google-java-format: each row an array of input lines and one of output lines, joined with \n and run through RemoveUnusedImports alone. 19 of its 23 rows are now cases under test-cases/imports, one file each, named after what it checks, and so is the one test of RemoveUnusedImportsCaseLabelsTest. A case runs the whole formatter, so its after block is the row's expected output, formatted: for every row the formatted input and the formatted expectation are the same text, and a second run changes nothing. One row's input was formatted already, so its case lays the same code out another way and has something to format. Four rows stay in RemoveUnusedImportsTest, as plain tests with text blocks. The three of #37 cannot be cases: a case fixes the imports before it formats, and formatting takes a doubled blank line away by itself, while the command line formats first. The module import parses from JDK 23 on, and the cases run on JDK 21 as well. With the RemoveUnusedImports of 21.X.X, the same four cases fail as the rows did: the ones with a used import from java.lang. Tests on JDK 21: FormatterCasesTest 887 -> 907, RemoveUnusedImportsTest 23 -> 4 (the module import skipped), RemoveUnusedImportsCaseLabelsTest 1 -> gone; 1413 tests in the module, as before. On JDK 25 the four tests of RemoveUnusedImportsTest all run, and the 907 cases pass. --- .../RemoveUnusedImportsCaseLabelsTest.java | 48 -- .../java/RemoveUnusedImportsTest.java | 492 +++--------------- ...ualifiedJavadocReferenceNeedsNoImport.case | 11 + .../imports/importOnDemandStays.case | 12 + .../importUsedOnlyByAJavadocLinkStays.case | 17 + .../imports/importsFromTheOwnPackageGo.case | 25 + ...ortsUsedByJavadocMemberReferencesStay.case | 12 + .../imports/javaLangImportOnDemandGoes.case | 13 + .../javaLangImportUsedByJavadocStays.case | 13 + ...ParameterKeepsTheImportOfItsQualifier.case | 11 + ...javadocLinkOnALaterLineKeepsItsImport.case | 18 + ...renceToANestedTypeKeepsTheOuterImport.case | 10 + ...docReferenceToOwnMemberWithoutImports.case | 8 + .../imports/javadocTagsKeepTheirImports.case | 41 ++ .../typesInCaseLabelsKeepTheirImports.case | 29 ++ .../unusedImportGoesFromBesideAUsedOne.case | 18 + .../unusedImportsOnTheLineOfAUsedOneGo.case | 14 + .../imports/unusedJavaLangImportGoes.case | 8 + ...aLangImportGoesBesideAnImportOnDemand.case | 16 + ...LangImportShadowsATypeOfTheOwnPackage.case | 16 + ...JavaLangImportShadowsAnImportOnDemand.case | 18 + ...vaLangImportStaysOwnPackageImportGoes.case | 18 + 22 files changed, 406 insertions(+), 462 deletions(-) delete mode 100644 open-java-format/src/test/java/com/palantir/javaformat/java/RemoveUnusedImportsCaseLabelsTest.java create mode 100644 open-java-format/src/test/resources/test-cases/imports/fullyQualifiedJavadocReferenceNeedsNoImport.case create mode 100644 open-java-format/src/test/resources/test-cases/imports/importOnDemandStays.case create mode 100644 open-java-format/src/test/resources/test-cases/imports/importUsedOnlyByAJavadocLinkStays.case create mode 100644 open-java-format/src/test/resources/test-cases/imports/importsFromTheOwnPackageGo.case create mode 100644 open-java-format/src/test/resources/test-cases/imports/importsUsedByJavadocMemberReferencesStay.case create mode 100644 open-java-format/src/test/resources/test-cases/imports/javaLangImportOnDemandGoes.case create mode 100644 open-java-format/src/test/resources/test-cases/imports/javaLangImportUsedByJavadocStays.case create mode 100644 open-java-format/src/test/resources/test-cases/imports/javadocArrayParameterKeepsTheImportOfItsQualifier.case create mode 100644 open-java-format/src/test/resources/test-cases/imports/javadocLinkOnALaterLineKeepsItsImport.case create mode 100644 open-java-format/src/test/resources/test-cases/imports/javadocReferenceToANestedTypeKeepsTheOuterImport.case create mode 100644 open-java-format/src/test/resources/test-cases/imports/javadocReferenceToOwnMemberWithoutImports.case create mode 100644 open-java-format/src/test/resources/test-cases/imports/javadocTagsKeepTheirImports.case create mode 100644 open-java-format/src/test/resources/test-cases/imports/typesInCaseLabelsKeepTheirImports.case create mode 100644 open-java-format/src/test/resources/test-cases/imports/unusedImportGoesFromBesideAUsedOne.case create mode 100644 open-java-format/src/test/resources/test-cases/imports/unusedImportsOnTheLineOfAUsedOneGo.case create mode 100644 open-java-format/src/test/resources/test-cases/imports/unusedJavaLangImportGoes.case create mode 100644 open-java-format/src/test/resources/test-cases/imports/unusedJavaLangImportGoesBesideAnImportOnDemand.case create mode 100644 open-java-format/src/test/resources/test-cases/imports/usedJavaLangImportShadowsATypeOfTheOwnPackage.case create mode 100644 open-java-format/src/test/resources/test-cases/imports/usedJavaLangImportShadowsAnImportOnDemand.case create mode 100644 open-java-format/src/test/resources/test-cases/imports/usedJavaLangImportStaysOwnPackageImportGoes.case diff --git a/open-java-format/src/test/java/com/palantir/javaformat/java/RemoveUnusedImportsCaseLabelsTest.java b/open-java-format/src/test/java/com/palantir/javaformat/java/RemoveUnusedImportsCaseLabelsTest.java deleted file mode 100644 index d855d7d85..000000000 --- a/open-java-format/src/test/java/com/palantir/javaformat/java/RemoveUnusedImportsCaseLabelsTest.java +++ /dev/null @@ -1,48 +0,0 @@ -/* - * (c) Copyright 2022 Palantir Technologies Inc. All rights reserved. - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -package com.palantir.javaformat.java; - -import static com.google.common.truth.Truth.assertThat; -import static com.palantir.javaformat.java.RemoveUnusedImports.removeUnusedImports; - -import com.google.common.base.Joiner; -import org.junit.jupiter.api.Assumptions; -import org.junit.jupiter.api.Test; - -final class RemoveUnusedImportsCaseLabelsTest { - - @Test - void preserveTypesInCaseLabels() throws FormatterException { - Assumptions.assumeTrue(Formatter.getRuntimeVersion() >= 17, "Not running on jdk 17 or later"); - @SuppressWarnings("for-rollout:StringConcatToTextBlock") - String input = Joiner.on('\n') - .join( - "package example;", - "import example.model.SealedInterface;", - "import example.model.TypeA;", - "import example.model.TypeB;", - "public class Main {", - " public void apply(SealedInterface sealedInterface) {", - " switch(sealedInterface) {", - " case TypeA a -> System.out.println(\"A!\");", - " case TypeB b -> System.out.println(\"B!\");", - " }", - " }", - "}"); - assertThat(removeUnusedImports(input)).isEqualTo(input); - } -} 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 509ffd39b..16d12f887 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 @@ -15,432 +15,96 @@ package com.palantir.javaformat.java; import static com.google.common.truth.Truth.assertThat; +import static com.palantir.javaformat.java.RemoveUnusedImports.removeUnusedImports; -import com.google.common.base.Joiner; -import com.google.common.collect.ImmutableList; -import com.google.common.truth.Truth; -import com.palantir.javaformat.jupiter.ParameterizedClass; -import java.util.List; import org.junit.jupiter.api.Assumptions; -import org.junit.jupiter.api.TestTemplate; -import org.junit.jupiter.api.extension.ExtendWith; +import org.junit.jupiter.api.Test; import org.junit.jupiter.api.parallel.Execution; import org.junit.jupiter.api.parallel.ExecutionMode; -/** {@link RemoveUnusedImports}Test */ +/** + * {@link RemoveUnusedImports} on its own, where a case under {@code test-cases/imports} cannot show it. A case fixes the + * imports before it formats, and formatting takes a doubled blank line away by itself, while the command line formats + * first. And the cases run on JDK 21, where a module import does not parse. + */ @Execution(ExecutionMode.CONCURRENT) -@ExtendWith(ParameterizedClass.class) -public class RemoveUnusedImportsTest { +final class RemoveUnusedImportsTest { + // 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). + @Test + void unusedImportBetweenBlankLinesTakesOneOfThem() throws FormatterException { + var input = """ + package com.example; + + import static io.grpc.MethodDescriptor.generateFullMethodName; + + /** + * Javadoc for class. + */ + public class TestBug {} + """; + + assertThat(removeUnusedImports(input)).isEqualTo(""" + package com.example; - @ParameterizedClass.Parameters(name = "{index}: {0}") - public static List 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.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 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 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 java.util.Map;", "/** {@link Map.Entry#containsKey(Object)} } */", "class Test {}", - }, - { - "import java.util.Map;", "/** {@link Map.Entry#containsKey(Object)} } */", "class Test {}", - }, - }, - { - { - "/** {@link #containsKey(Object)} } */", // - "class Test {}", - }, - { - "/** {@link #containsKey(Object)} } */", // - "class Test {}", - }, - }, - { - { - "import java.util.*;", // - "class Test {", - " List xs;", - "}", - }, - { - "import java.util.*;", // - "class Test {", - " List xs;", - "}", - }, - }, - { - { - "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.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 java.util.Map;", // - "/** {@link #foo(Map.Entry[])} */", - "public 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 {}", - }, - }, - { - { - "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.lang.Foo;` is kept: `p` may also declare a `Foo`, which the - // single-type-import shadows (JLS 6.4.1). `import p.Baz;` is still dropped, since a type - // in the current package never needs importing. - "package p;", - "import java.lang.Foo;", - "import java.lang.Foo.Bar;", - "import p.Baz.Bork;", - "public class Test implements Foo, Bar, Baz, Bork {}", - }, - }, - { - { - "import java.lang.Foo;", // - "interface Test { private static void foo() {} }", - }, - { - "interface Test { private static void foo() {} }", - }, - }, - { - // Module imports (JEP 511, `import module foo.bar;`) parse to JCModuleImport, a - // sibling of JCImport rather than a subtype. They must never be reported as unused - // (see isUnused), and must not crash buildReplacements when mixed with ordinary - // imports, some used and some not. - { - "import module java.base;", - "import java.util.List;", - "import java.util.Map;", - "class T { List xs; }", - }, - { - "import module java.base;", "import java.util.List;", "class T { List xs; }", - }, - }, - // 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;", - "}", - }, - }, - // https://github.com/palantir/palantir-java-format/issues/1334: a single-type-import of a java.lang - // type shadows same-package types and on-demand imports, so it must not be removed while it is used. - { - // disambiguates `Byte` against an on-demand import - { - "package com.company.project.bar;", - "import com.company.project.foo.*;", - "import java.lang.Byte;", - "public class Test { Byte foo() { return null; } }", - }, - { - "package com.company.project.bar;", - "import com.company.project.foo.*;", - "import java.lang.Byte;", - "public class Test { Byte foo() { return null; } }", - }, - }, - { - // disambiguates `Byte` against a class declared in the same package - { - "package com.company.project.bar;", - "import java.lang.Byte;", - "public class Test { Byte foo() { return null; } }", - }, - { - "package com.company.project.bar;", - "import java.lang.Byte;", - "public class Test { Byte foo() { return null; } }", - }, - }, - { - // a java.lang import that is only referenced from javadoc is still load-bearing - { - "package p;", // - "import java.lang.Byte;", - "/** {@link Byte} */", - "public class Test {}", - }, - { - "package p;", // - "import java.lang.Byte;", - "/** {@link Byte} */", - "public class Test {}", - }, - }, - { - // `import java.lang.*;` is always redundant, java.lang is implicitly imported on demand - { - "package p;", // - "import java.lang.*;", - "public class Test { Byte foo() { return null; } }", - }, - { - "package p;", // - "public class Test { Byte foo() { return null; } }", - }, - }, - { - // an unused java.lang import is still removed, even alongside an on-demand import - { - "package p;", - "import java.lang.Byte;", - "import q.*;", - "public class Test { Short foo() { return null; } }", - }, - { - "package p;", // - "import q.*;", - "public class Test { Short foo() { return null; } }", - }, - }, - }; - ImmutableList.Builder builder = ImmutableList.builder(); - for (String[][] inputAndOutput : inputsOutputs) { - assertThat(inputAndOutput.length).isEqualTo(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); - } - return builder.build(); + /** + * Javadoc for class. + */ + public class TestBug {} + """); } - private final String input; - private final String expected; + @Test + void unusedImportsBetweenBlankLinesTakeOneOfThem() throws FormatterException { + var input = """ + package com.example; + + import com.foo.Unused1; + import com.foo.Unused2; + + public class TestBug {} + """; + + assertThat(removeUnusedImports(input)).isEqualTo(""" + package com.example; - public RemoveUnusedImportsTest(String input, String expected) { - this.input = input; - this.expected = expected; + public class TestBug {} + """); } - @TestTemplate - public void removeUnused() throws FormatterException { - // Module imports (JEP 511) parse from JDK 23 on, as preview there; the formatter enables preview. + @Test + void unusedImportAtTheStartTakesTheBlankLineAfterIt() throws FormatterException { + var input = """ + import com.foo.Unused; + + public class TestBug {} + """; + + assertThat(removeUnusedImports(input)).isEqualTo(""" + public class TestBug {} + """); + } + + // Module imports (JEP 511, `import module foo.bar;`) parse to JCModuleImport, a sibling of JCImport rather than a + // subtype. They must never be reported as unused (see isUnused), and must not crash buildReplacements when mixed + // with ordinary imports, some used and some not. + @Test + void moduleImportStaysAmongOrdinaryImports() throws FormatterException { + // Module imports parse from JDK 23 on, as preview there; the formatter enables preview. Assumptions.assumeTrue( - !input.contains("import module") || Formatter.getRuntimeVersion() >= 23, - "import module requires running on JDK 23 or later"); - Truth.assertThat(RemoveUnusedImports.removeUnusedImports(input)).isEqualTo(expected); + Formatter.getRuntimeVersion() >= 23, "import module requires running on JDK 23 or later"); + + var input = """ + import module java.base; + import java.util.List; + import java.util.Map; + class T { List xs; } + """; + + assertThat(removeUnusedImports(input)).isEqualTo(""" + import module java.base; + import java.util.List; + class T { List xs; } + """); } } diff --git a/open-java-format/src/test/resources/test-cases/imports/fullyQualifiedJavadocReferenceNeedsNoImport.case b/open-java-format/src/test/resources/test-cases/imports/fullyQualifiedJavadocReferenceNeedsNoImport.case new file mode 100644 index 000000000..eae033034 --- /dev/null +++ b/open-java-format/src/test/resources/test-cases/imports/fullyQualifiedJavadocReferenceNeedsNoImport.case @@ -0,0 +1,11 @@ +# A Javadoc reference written with its full name needs no import, while the type of its parameter keeps its own. +---------- +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 {} diff --git a/open-java-format/src/test/resources/test-cases/imports/importOnDemandStays.case b/open-java-format/src/test/resources/test-cases/imports/importOnDemandStays.case new file mode 100644 index 000000000..46d3c2eb3 --- /dev/null +++ b/open-java-format/src/test/resources/test-cases/imports/importOnDemandStays.case @@ -0,0 +1,12 @@ +# An import on demand stays: nothing tells which of its types the file uses. +---------- +import java.util.*; +class Test { + List xs; +} +---------- +import java.util.*; + +class Test { + List xs; +} diff --git a/open-java-format/src/test/resources/test-cases/imports/importUsedOnlyByAJavadocLinkStays.case b/open-java-format/src/test/resources/test-cases/imports/importUsedOnlyByAJavadocLinkStays.case new file mode 100644 index 000000000..faf5e9644 --- /dev/null +++ b/open-java-format/src/test/resources/test-cases/imports/importUsedOnlyByAJavadocLinkStays.case @@ -0,0 +1,17 @@ +# An import that only a {@link} in a Javadoc comment uses stays. +---------- +import java.util.List; +import java.util.ArrayList; + +class Test { + /** could be an {@link ArrayList} */ + List xs; +} +---------- +import java.util.ArrayList; +import java.util.List; + +class Test { + /** could be an {@link ArrayList} */ + List xs; +} diff --git a/open-java-format/src/test/resources/test-cases/imports/importsFromTheOwnPackageGo.case b/open-java-format/src/test/resources/test-cases/imports/importsFromTheOwnPackageGo.case new file mode 100644 index 000000000..0fa42b52a --- /dev/null +++ b/open-java-format/src/test/resources/test-cases/imports/importsFromTheOwnPackageGo.case @@ -0,0 +1,25 @@ +# Imports from the file's own package go, an import on demand of it too, while a static import from it and an import +# from another package stay. +---------- +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.bar.C; + +class Test { + A a; + B b; + C c; +} diff --git a/open-java-format/src/test/resources/test-cases/imports/importsUsedByJavadocMemberReferencesStay.case b/open-java-format/src/test/resources/test-cases/imports/importsUsedByJavadocMemberReferencesStay.case new file mode 100644 index 000000000..ddc9b3680 --- /dev/null +++ b/open-java-format/src/test/resources/test-cases/imports/importsUsedByJavadocMemberReferencesStay.case @@ -0,0 +1,12 @@ +# Imports that only Javadoc references to members use stay, with or without a parameter list. +---------- +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 {} diff --git a/open-java-format/src/test/resources/test-cases/imports/javaLangImportOnDemandGoes.case b/open-java-format/src/test/resources/test-cases/imports/javaLangImportOnDemandGoes.case new file mode 100644 index 000000000..a27b26412 --- /dev/null +++ b/open-java-format/src/test/resources/test-cases/imports/javaLangImportOnDemandGoes.case @@ -0,0 +1,13 @@ +# An import on demand of java.lang goes, since java.lang is imported on demand anyway (palantir-java-format#1778, #149). +---------- +package p; +import java.lang.*; +public class Test { Byte foo() { return null; } } +---------- +package p; + +public class Test { + Byte foo() { + return null; + } +} diff --git a/open-java-format/src/test/resources/test-cases/imports/javaLangImportUsedByJavadocStays.case b/open-java-format/src/test/resources/test-cases/imports/javaLangImportUsedByJavadocStays.case new file mode 100644 index 000000000..d034076a0 --- /dev/null +++ b/open-java-format/src/test/resources/test-cases/imports/javaLangImportUsedByJavadocStays.case @@ -0,0 +1,13 @@ +# A single-type import from java.lang that only a Javadoc comment uses stays (palantir-java-format#1778, #149). +---------- +package p; +import java.lang.Byte; +/** {@link Byte} */ +public class Test {} +---------- +package p; + +import java.lang.Byte; + +/** {@link Byte} */ +public class Test {} diff --git a/open-java-format/src/test/resources/test-cases/imports/javadocArrayParameterKeepsTheImportOfItsQualifier.case b/open-java-format/src/test/resources/test-cases/imports/javadocArrayParameterKeepsTheImportOfItsQualifier.case new file mode 100644 index 000000000..b041bbbf9 --- /dev/null +++ b/open-java-format/src/test/resources/test-cases/imports/javadocArrayParameterKeepsTheImportOfItsQualifier.case @@ -0,0 +1,11 @@ +# A Javadoc parameter type written as Map.Entry[] keeps the import of Map, and the unused import of Map.Entry goes. +---------- +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 {} diff --git a/open-java-format/src/test/resources/test-cases/imports/javadocLinkOnALaterLineKeepsItsImport.case b/open-java-format/src/test/resources/test-cases/imports/javadocLinkOnALaterLineKeepsItsImport.case new file mode 100644 index 000000000..f3dea8bd4 --- /dev/null +++ b/open-java-format/src/test/resources/test-cases/imports/javadocLinkOnALaterLineKeepsItsImport.case @@ -0,0 +1,18 @@ +# A {@link} on a later line of a Javadoc comment keeps its import, and the imports nothing uses go. +---------- +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() {} +} diff --git a/open-java-format/src/test/resources/test-cases/imports/javadocReferenceToANestedTypeKeepsTheOuterImport.case b/open-java-format/src/test/resources/test-cases/imports/javadocReferenceToANestedTypeKeepsTheOuterImport.case new file mode 100644 index 000000000..1c91c3f60 --- /dev/null +++ b/open-java-format/src/test/resources/test-cases/imports/javadocReferenceToANestedTypeKeepsTheOuterImport.case @@ -0,0 +1,10 @@ +# A Javadoc reference to a member of a nested type keeps the import of the type around it. +---------- +import java.util.Map; +/** {@link Map.Entry#containsKey(Object)} } */ +class Test {} +---------- +import java.util.Map; + +/** {@link Map.Entry#containsKey(Object)} } */ +class Test {} diff --git a/open-java-format/src/test/resources/test-cases/imports/javadocReferenceToOwnMemberWithoutImports.case b/open-java-format/src/test/resources/test-cases/imports/javadocReferenceToOwnMemberWithoutImports.case new file mode 100644 index 000000000..9e1d3bdac --- /dev/null +++ b/open-java-format/src/test/resources/test-cases/imports/javadocReferenceToOwnMemberWithoutImports.case @@ -0,0 +1,8 @@ +# A Javadoc reference to a member of the class itself needs no import, and a file without imports keeps its Javadoc as +# it is. +---------- +/** {@link #containsKey(Object)} } */ class Test { +} +---------- +/** {@link #containsKey(Object)} } */ +class Test {} diff --git a/open-java-format/src/test/resources/test-cases/imports/javadocTagsKeepTheirImports.case b/open-java-format/src/test/resources/test-cases/imports/javadocTagsKeepTheirImports.case new file mode 100644 index 000000000..f78e5243e --- /dev/null +++ b/open-java-format/src/test/resources/test-cases/imports/javadocTagsKeepTheirImports.case @@ -0,0 +1,41 @@ +# {@link}, {@linkplain}, {@value}, @exception, @throws and @see keep the imports of the types they name, and the import +# that nothing names goes. +---------- +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 {} diff --git a/open-java-format/src/test/resources/test-cases/imports/typesInCaseLabelsKeepTheirImports.case b/open-java-format/src/test/resources/test-cases/imports/typesInCaseLabelsKeepTheirImports.case new file mode 100644 index 000000000..b29e19186 --- /dev/null +++ b/open-java-format/src/test/resources/test-cases/imports/typesInCaseLabelsKeepTheirImports.case @@ -0,0 +1,29 @@ +# Types that only the patterns of case labels name keep their imports. +---------- +package example; +import example.model.SealedInterface; +import example.model.TypeA; +import example.model.TypeB; +public class Main { + public void apply(SealedInterface sealedInterface) { + switch(sealedInterface) { + case TypeA a -> System.out.println("A!"); + case TypeB b -> System.out.println("B!"); + } + } +} +---------- +package example; + +import example.model.SealedInterface; +import example.model.TypeA; +import example.model.TypeB; + +public class Main { + public void apply(SealedInterface sealedInterface) { + switch (sealedInterface) { + case TypeA a -> System.out.println("A!"); + case TypeB b -> System.out.println("B!"); + } + } +} diff --git a/open-java-format/src/test/resources/test-cases/imports/unusedImportGoesFromBesideAUsedOne.case b/open-java-format/src/test/resources/test-cases/imports/unusedImportGoesFromBesideAUsedOne.case new file mode 100644 index 000000000..a360bd48b --- /dev/null +++ b/open-java-format/src/test/resources/test-cases/imports/unusedImportGoesFromBesideAUsedOne.case @@ -0,0 +1,18 @@ +# An unused import goes, and the used import before it stays. +---------- +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; +} diff --git a/open-java-format/src/test/resources/test-cases/imports/unusedImportsOnTheLineOfAUsedOneGo.case b/open-java-format/src/test/resources/test-cases/imports/unusedImportsOnTheLineOfAUsedOneGo.case new file mode 100644 index 000000000..aadeebf14 --- /dev/null +++ b/open-java-format/src/test/resources/test-cases/imports/unusedImportsOnTheLineOfAUsedOneGo.case @@ -0,0 +1,14 @@ +# Unused imports go even where they share a line with a used one, and the comment after the last one stays. +---------- +import a.A;import a.B; +import a.C; // hello +class Test { + B b; +} +---------- +import a.B; +// hello + +class Test { + B b; +} diff --git a/open-java-format/src/test/resources/test-cases/imports/unusedJavaLangImportGoes.case b/open-java-format/src/test/resources/test-cases/imports/unusedJavaLangImportGoes.case new file mode 100644 index 000000000..d65ef73b7 --- /dev/null +++ b/open-java-format/src/test/resources/test-cases/imports/unusedJavaLangImportGoes.case @@ -0,0 +1,8 @@ +# An unused import from java.lang goes. +---------- +import java.lang.Foo; +interface Test { private static void foo() {} } +---------- +interface Test { + private static void foo() {} +} diff --git a/open-java-format/src/test/resources/test-cases/imports/unusedJavaLangImportGoesBesideAnImportOnDemand.case b/open-java-format/src/test/resources/test-cases/imports/unusedJavaLangImportGoesBesideAnImportOnDemand.case new file mode 100644 index 000000000..f6ce5a24a --- /dev/null +++ b/open-java-format/src/test/resources/test-cases/imports/unusedJavaLangImportGoesBesideAnImportOnDemand.case @@ -0,0 +1,16 @@ +# An unused single-type import from java.lang goes, also beside an import on demand (palantir-java-format#1778, #149). +---------- +package p; +import java.lang.Byte; +import q.*; +public class Test { Short foo() { return null; } } +---------- +package p; + +import q.*; + +public class Test { + Short foo() { + return null; + } +} diff --git a/open-java-format/src/test/resources/test-cases/imports/usedJavaLangImportShadowsATypeOfTheOwnPackage.case b/open-java-format/src/test/resources/test-cases/imports/usedJavaLangImportShadowsATypeOfTheOwnPackage.case new file mode 100644 index 000000000..60587a7db --- /dev/null +++ b/open-java-format/src/test/resources/test-cases/imports/usedJavaLangImportShadowsATypeOfTheOwnPackage.case @@ -0,0 +1,16 @@ +# A used single-type import from java.lang stays: it shadows a Byte that the file's own package may declare +# (palantir-java-format#1778, #149). +---------- +package com.company.project.bar; +import java.lang.Byte; +public class Test { Byte foo() { return null; } } +---------- +package com.company.project.bar; + +import java.lang.Byte; + +public class Test { + Byte foo() { + return null; + } +} diff --git a/open-java-format/src/test/resources/test-cases/imports/usedJavaLangImportShadowsAnImportOnDemand.case b/open-java-format/src/test/resources/test-cases/imports/usedJavaLangImportShadowsAnImportOnDemand.case new file mode 100644 index 000000000..ccac8b801 --- /dev/null +++ b/open-java-format/src/test/resources/test-cases/imports/usedJavaLangImportShadowsAnImportOnDemand.case @@ -0,0 +1,18 @@ +# A used single-type import from java.lang stays: it shadows the Byte that an import on demand brings in, and without it +# the name is ambiguous (palantir-java-format#1778, #149). +---------- +package com.company.project.bar; +import com.company.project.foo.*; +import java.lang.Byte; +public class Test { Byte foo() { return null; } } +---------- +package com.company.project.bar; + +import com.company.project.foo.*; +import java.lang.Byte; + +public class Test { + Byte foo() { + return null; + } +} diff --git a/open-java-format/src/test/resources/test-cases/imports/usedJavaLangImportStaysOwnPackageImportGoes.case b/open-java-format/src/test/resources/test-cases/imports/usedJavaLangImportStaysOwnPackageImportGoes.case new file mode 100644 index 000000000..177182b00 --- /dev/null +++ b/open-java-format/src/test/resources/test-cases/imports/usedJavaLangImportStaysOwnPackageImportGoes.case @@ -0,0 +1,18 @@ +# A used single-type import from java.lang stays, since it shadows a type of the same name in the package +# (palantir-java-format#1778, #149). The import of a type of the package itself goes, the import of a type nested in one +# stays. +---------- +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 {} +---------- +package p; + +import java.lang.Foo; +import java.lang.Foo.Bar; +import p.Baz.Bork; + +public class Test implements Foo, Bar, Baz, Bork {}