Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Static import is removed and leaves ambiguous reference behind #4450

Merged
merged 14 commits into from
Sep 6, 2024
+129 −9
Merged

Static import is removed and leaves ambiguous reference behind #4450

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
7c95896
Test case that shows static import is removed leaving ambiguous refer…
Laurens-W Aug 27, 2024
bd6dee1
Merge branch 'main' into remove-unused-imports-ambiguous
timtebeek Aug 27, 2024
0817925
Merge branch 'main' into remove-unused-imports-ambiguous
timtebeek Aug 27, 2024
90882b8
Merge branch 'main' into remove-unused-imports-ambiguous
Laurens-W Aug 28, 2024
f1d3f30
Merge branch 'main' into remove-unused-imports-ambiguous
Laurens-W Aug 28, 2024
f425e0f
Prevent imports of static members from being removed when this causes…
Laurens-W Aug 29, 2024
93b3fcd
First small fix
Laurens-W Sep 2, 2024
1fac922
Merge branch 'main' into remove-unused-imports-ambiguous
Laurens-W Sep 2, 2024
ac3484e
Destream and invert
Laurens-W Sep 3, 2024
27fd9a1
Merge branch 'main' into remove-unused-imports-ambiguous
Laurens-W Sep 3, 2024
ea73d4f
Fixed final test
Laurens-W Sep 5, 2024
f514606
Merge branch 'main' into remove-unused-imports-ambiguous
Laurens-W Sep 5, 2024
40108a0
Merge branch 'main' into remove-unused-imports-ambiguous
timtebeek Sep 6, 2024
ff379fa
Revert change to TypeInUse and detect from variables only
timtebeek Sep 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
100 changes: 96 additions & 4 deletions rewrite-java-test/src/test/java/org/openrewrite/java/RemoveUnusedImportsTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
import org.openrewrite.style.NamedStyles;
import org.openrewrite.test.RecipeSpec;
import org.openrewrite.test.RewriteTest;
import org.openrewrite.test.SourceSpec;
import org.openrewrite.test.TypeValidation;

import static java.util.Collections.emptySet;
Expand Down Expand Up @@ -1380,22 +1381,24 @@ void removeMultipleWildcardImports() {
import java.util.*;

import static java.util.Collections.emptyList;
import static java.util.Collections.singletonList;

class A {
Collection<Integer> c = emptyList();
Set<Integer> s = new HashSet<>();
List<String> l = singletonList("a","b","c");
List<String> l = singletonList("a");
}
""",
"""
import java.util.*;

import static java.util.Collections.emptyList;
import static java.util.Collections.singletonList;

class A {
Collection<Integer> c = emptyList();
Set<Integer> s = new HashSet<>();
List<String> l = singletonList("a","b","c");
List<String> l = singletonList("a");
}
"""
)
Expand Down Expand Up @@ -1449,7 +1452,7 @@ void removeDirectImportWithWildcardImport() {
class A {
Collection<Integer> c = emptyList();
Set<Integer> s = emptySet();
List<String> l = singletonList("c","b","a");
List<String> l = singletonList("c");
Iterator<Short> i = emptyIterator();
}
""",
Expand All @@ -1461,7 +1464,7 @@ class A {
class A {
Collection<Integer> c = emptyList();
Set<Integer> s = emptySet();
List<String> l = singletonList("c","b","a");
List<String> l = singletonList("c");
Iterator<Short> i = emptyIterator();
}
"""
Expand Down Expand Up @@ -1814,4 +1817,93 @@ public abstract class Foo2 {
)
);
}

@Test
void staticAmbiguousImport() {
// language=java
rewriteRun(
java(
"""
package org.a;
public class Abc {
public static String A = "A";
public static String B = "B";
public static String C = "C";
public static String ALL = "%s%s%s".formatted(A, B, C);
}
""",
SourceSpec::skip
),
java(
"""
package org.b;
public class Def {
public static String D = "D";
public static String E = "E";
public static String F = "F";
public static String ALL = "%s%s%s".formatted(D, E, F);
}
""",
SourceSpec::skip
),
// No change when removal would cause ambiguity
java(
"""
package org.test;

import static org.a.Abc.*;
import static org.a.Abc.ALL;
import static org.b.Def.*;

public class Foo {
private String abc = ALL;
private String a = A;
private String b = B;
private String c = C;
private String d = D;
private String e = E;
private String f = F;
}
"""
),
// Do still remove unambiguous imports
java(
"""
package org.test;

import static org.a.Abc.*;
import static org.a.Abc.A;
import static org.a.Abc.ALL;
import static org.b.Def.*;

public class Bar {
private String abc = ALL;
private String a = A;
private String b = B;
private String c = C;
private String d = D;
private String e = E;
private String f = F;
}
""",
"""
package org.test;

import static org.a.Abc.*;
import static org.a.Abc.ALL;
import static org.b.Def.*;

public class Bar {
private String abc = ALL;
private String a = A;
private String b = B;
private String c = C;
private String d = D;
private String e = E;
private String f = F;
}
"""
)
);
}
}
38 changes: 33 additions & 5 deletions rewrite-java/src/main/java/org/openrewrite/java/RemoveUnusedImports.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -169,7 +169,7 @@ public J.CompilationUnit visitCompilationUnit(J.CompilationUnit cu, ExecutionCon
anImport.used = true;
usedStaticWildcardImports.add(elem.getTypeName());
} else if (((methodsAndFields == null ? 0 : methodsAndFields.size()) +
(staticClasses == null ? 0 : staticClasses.size())) < layoutStyle.getNameCountToUseStarImport()) {
(staticClasses == null ? 0 : staticClasses.size())) < layoutStyle.getNameCountToUseStarImport()) {
// replacing the star with a series of unfolded imports
anImport.imports.clear();

Expand Down Expand Up @@ -199,8 +199,8 @@ public J.CompilationUnit visitCompilationUnit(J.CompilationUnit cu, ExecutionCon
usedStaticWildcardImports.add(elem.getTypeName());
}
} else if (staticClasses != null && staticClasses.stream().anyMatch(c -> elem.getTypeName().equals(c.getFullyQualifiedName())) ||
(methodsAndFields != null && methodsAndFields.contains(qualid.getSimpleName())) ||
(targetMethodsAndFields != null && targetMethodsAndFields.contains(qualid.getSimpleName()))) {
(methodsAndFields != null && methodsAndFields.contains(qualid.getSimpleName())) ||
(targetMethodsAndFields != null && targetMethodsAndFields.contains(qualid.getSimpleName()))) {
anImport.used = true;
} else {
anImport.used = false;
Expand Down Expand Up @@ -252,11 +252,13 @@ public J.CompilationUnit visitCompilationUnit(J.CompilationUnit cu, ExecutionCon
}

// Do not use direct imports that are imported by a wildcard import
Set<String> ambiguousStaticImportNames = getAmbiguousStaticImportNames(cu);
for (ImportUsage anImport : importUsage) {
J.Import elem = anImport.imports.get(0).getElement();
if (!"*".equals(elem.getQualid().getSimpleName())) {
if (elem.isStatic()) {
if (usedStaticWildcardImports.contains(elem.getTypeName())) {
if (usedStaticWildcardImports.contains(elem.getTypeName()) &&
!ambiguousStaticImportNames.contains(elem.getQualid().getSimpleName())) {
anImport.used = false;
changed = true;
}
Expand All @@ -278,7 +280,7 @@ public J.CompilationUnit visitCompilationUnit(J.CompilationUnit cu, ExecutionCon
for (int i = 0; i < importGroup.size(); i++) {
JRightPadded<J.Import> anImport = importGroup.get(i);
if (i == 0 && lastUnusedImportSpace != null && anImport.getElement().getPrefix().getLastWhitespace()
.chars().filter(c -> c == '\n').count() <= 1) {
.chars().filter(c -> c == '\n').count() <= 1) {
anImport = anImport.withElement(anImport.getElement().withPrefix(lastUnusedImportSpace));
}
imports.add(anImport);
Expand All @@ -298,6 +300,32 @@ public J.CompilationUnit visitCompilationUnit(J.CompilationUnit cu, ExecutionCon
return cu;
}

private static Set<String> getAmbiguousStaticImportNames(J.CompilationUnit cu) {
Set<String> typesWithWildcardImport = new HashSet<>();
for (J.Import elem : cu.getImports()) {
if ("*".equals(elem.getQualid().getSimpleName())) {
typesWithWildcardImport.add(elem.getTypeName());
}
}
Set<JavaType.FullyQualified> qualifiedTypes = new HashSet<>();
for (JavaType.Variable variable : cu.getTypesInUse().getVariables()) {
JavaType.FullyQualified fq = TypeUtils.asFullyQualified(variable.getOwner());
if (fq != null && typesWithWildcardImport.contains(fq.getFullyQualifiedName())) {
qualifiedTypes.add(fq);
}
}
Set<String> seen = new HashSet<>();
Set<String> ambiguous = new HashSet<>();
for (JavaType.FullyQualified fq : qualifiedTypes) {
for (JavaType.Variable member : fq.getMembers()) {
if (!seen.add(member.getName())) {
ambiguous.add(member.getName());
}
}
}
return ambiguous;
}

private static final Set<String> JAVA_LANG_CLASS_NAMES = new HashSet<>(Arrays.asList(
"AbstractMethodError",
"Appendable",
Expand Down
Loading