diff --git a/src/main/java/org/openrewrite/staticanalysis/UseCollectionInterfaces.java b/src/main/java/org/openrewrite/staticanalysis/UseCollectionInterfaces.java index 5faefb4563..0127d899da 100644 --- a/src/main/java/org/openrewrite/staticanalysis/UseCollectionInterfaces.java +++ b/src/main/java/org/openrewrite/staticanalysis/UseCollectionInterfaces.java @@ -24,12 +24,10 @@ import org.openrewrite.marker.Markers; import java.time.Duration; -import java.util.HashMap; -import java.util.Map; -import java.util.Set; +import java.util.*; +import java.util.concurrent.atomic.AtomicBoolean; -import static java.util.Collections.emptyList; -import static java.util.Collections.singleton; +import static java.util.Collections.*; import static java.util.Objects.requireNonNull; import static org.openrewrite.Tree.randomId; @@ -43,7 +41,7 @@ public String getDisplayName() { @Override public String getDescription() { return "Use `Deque`, `List`, `Map`, `ConcurrentMap`, `Queue`, and `Set` instead of implemented collections. " + - "Replaces the return type of public method declarations and the variable type public variable declarations."; + "Replaces the return type of public method declarations and the variable type public variable declarations."; } @Override @@ -56,7 +54,7 @@ public Duration getEstimatedEffortPerOccurrence() { return Duration.ofMinutes(10); } - public static final Map rspecRulesReplaceTypeMap = new HashMap<>(); + static final Map rspecRulesReplaceTypeMap = new HashMap<>(); static { rspecRulesReplaceTypeMap.put("java.util.ArrayDeque", "java.util.Deque"); @@ -66,6 +64,7 @@ public Duration getEstimatedEffortPerOccurrence() { rspecRulesReplaceTypeMap.put("java.util.AbstractSequentialList", "java.util.List"); rspecRulesReplaceTypeMap.put("java.util.ArrayList", "java.util.List"); rspecRulesReplaceTypeMap.put("java.util.concurrent.CopyOnWriteArrayList", "java.util.List"); + rspecRulesReplaceTypeMap.put("java.util.Vector", "java.util.List"); // Map rspecRulesReplaceTypeMap.put("java.util.AbstractMap", "java.util.Map"); rspecRulesReplaceTypeMap.put("java.util.EnumMap", "java.util.Map"); @@ -100,6 +99,11 @@ public boolean isAcceptable(SourceFile sourceFile, ExecutionContext ctx) { public J visit(@Nullable Tree tree, ExecutionContext ctx) { if (tree instanceof JavaSourceFile) { JavaSourceFile cu = (JavaSourceFile) requireNonNull(tree); + + if (new InterfaceIncompatibleMethodDetector().reduce(tree, new AtomicBoolean(false)).get()) { + return cu; + } + for (JavaType type : cu.getTypesInUse().getTypesInUse()) { JavaType.FullyQualified fq = TypeUtils.asFullyQualified(type); if (fq != null && rspecRulesReplaceTypeMap.containsKey(fq.getFullyQualifiedName())) { @@ -114,7 +118,7 @@ public J visit(@Nullable Tree tree, ExecutionContext ctx) { public J.MethodDeclaration visitMethodDeclaration(J.MethodDeclaration method, ExecutionContext ctx) { J.MethodDeclaration m = super.visitMethodDeclaration(method, ctx); if ((m.hasModifier(J.Modifier.Type.Public) || m.hasModifier(J.Modifier.Type.Private) || m.getModifiers().isEmpty()) && - m.getReturnTypeExpression() != null) { + m.getReturnTypeExpression() != null) { JavaType.FullyQualified originalType = TypeUtils.asFullyQualified(m.getReturnTypeExpression().getType()); if (originalType != null && rspecRulesReplaceTypeMap.containsKey(originalType.getFullyQualifiedName())) { @@ -177,7 +181,7 @@ public J.VariableDeclarations visitVariableDeclarations(J.VariableDeclarations m J.VariableDeclarations mv = super.visitVariableDeclarations(multiVariable, ctx); JavaType.FullyQualified originalType = TypeUtils.asFullyQualified(mv.getType()); if ((mv.hasModifier(J.Modifier.Type.Public) || mv.hasModifier(J.Modifier.Type.Private) || mv.getModifiers().isEmpty()) && - originalType != null && rspecRulesReplaceTypeMap.containsKey(originalType.getFullyQualifiedName())) { + originalType != null && rspecRulesReplaceTypeMap.containsKey(originalType.getFullyQualifiedName())) { if (mv.getTypeExpression() instanceof J.Identifier && "var".equals(((J.Identifier) mv.getTypeExpression()).getSimpleName())) { return mv; } @@ -255,4 +259,33 @@ private TypeTree removeFromParameterizedType(JavaType.FullyQualified newType, } }; } + + private static class InterfaceIncompatibleMethodDetector extends JavaIsoVisitor { + private static final Map> nonInterfaceMethods = new HashMap<>(); + + static { + nonInterfaceMethods.put("java.util.Hashtable", unmodifiableSet(new HashSet<>(Arrays.asList( + "contains", "elements", "keys")))); + nonInterfaceMethods.put("java.util.Vector", unmodifiableSet(new HashSet<>(Arrays.asList( + "addElement", "capacity", "copyInto", "elementAt", "elements", "ensureCapacity", "insertElementAt", "removeAllElements", "removeElement", "removeElementAt", "setElementAt", "setSize", "trimToSize")))); + nonInterfaceMethods.put("java.util.ArrayList", unmodifiableSet(new HashSet<>(Arrays.asList( + "ensureCapacity", "trimToSize")))); + } + + @Override + public J.MethodInvocation visitMethodInvocation(J.MethodInvocation method, AtomicBoolean foundNonInterfaceMethod) { + if (foundNonInterfaceMethod.get()) { + return method; + } + if (method.getSelect() != null) { + JavaType.FullyQualified fqType = TypeUtils.asFullyQualified(method.getSelect().getType()); + if (fqType != null && nonInterfaceMethods.getOrDefault(fqType.getFullyQualifiedName(), emptySet()) + .contains(method.getSimpleName())) { + foundNonInterfaceMethod.set(true); + return method; + } + } + return super.visitMethodInvocation(method, foundNonInterfaceMethod); + } + } } diff --git a/src/test/java/org/openrewrite/staticanalysis/UseCollectionInterfacesTest.java b/src/test/java/org/openrewrite/staticanalysis/UseCollectionInterfacesTest.java index 7dbe84f3ae..6429936454 100644 --- a/src/test/java/org/openrewrite/staticanalysis/UseCollectionInterfacesTest.java +++ b/src/test/java/org/openrewrite/staticanalysis/UseCollectionInterfacesTest.java @@ -16,7 +16,6 @@ package org.openrewrite.staticanalysis; import org.junit.jupiter.api.Test; -import org.junitpioneer.jupiter.ExpectedToFail; import org.openrewrite.DocumentExample; import org.openrewrite.Issue; import org.openrewrite.java.JavaParser; @@ -935,7 +934,6 @@ Set method() { ); } - @ExpectedToFail @Issue("https://github.com/openrewrite/rewrite/issues/2973") @Test void explicitImplementationClassInApi() { @@ -948,7 +946,7 @@ void explicitImplementationClassInApi() { class Test { List m() { - List result = new ArrayList<>(); + ArrayList result = new ArrayList<>(); m2(result); return result; } @@ -1112,4 +1110,105 @@ public int method(Set input) { ) ); } + + @Test + void hashtableWithOnlyMapMethods() { + rewriteRun( + java( + """ + import java.util.Hashtable; + + class A { + Hashtable useOnlyMapMethods() { + Hashtable table = new Hashtable<>(); + table.put("key", 1); + return table; + } + } + """, + """ + import java.util.Hashtable; + import java.util.Map; + + class A { + Map useOnlyMapMethods() { + Map table = new Hashtable<>(); + table.put("key", 1); + return table; + } + } + """ + ) + ); + } + + @Issue("https://github.com/openrewrite/rewrite-static-analysis/issues/688") + @Test + void hashtableMethodNotOnInterface() { + rewriteRun( + java( + """ + import java.util.Enumeration; + import java.util.Hashtable; + + class A { + Enumeration usesMethodNotOnInterface() { + Hashtable table = new Hashtable<>(); + return table.keys(); + } + } + """ + ) + ); + } + + @Test + void vectorWithOnlyListMethods() { + rewriteRun( + java( + """ + import java.util.Vector; + + class A { + Vector getData() { + Vector vector = new Vector<>(); + vector.add("item"); + return vector; + } + } + """, + """ + import java.util.List; + import java.util.Vector; + + class A { + List getData() { + List vector = new Vector<>(); + vector.add("item"); + return vector; + } + } + """ + ) + ); + } + + @Test + void usesVectorElementsMethod() { + rewriteRun( + java( + """ + import java.util.Enumeration; + import java.util.Vector; + + class A { + Enumeration usesVectorElements() { + Vector vector = new Vector<>(); + return vector.elements(); + } + } + """ + ) + ); + } }