Skip to content

Skip UseMapOf when values may be null or keys may repeat #1382

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

Open
henriquejsza wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Skip UseMapOf when values may be null or keys may repeat #1382

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
58 changes: 56 additions & 2 deletions src/main/java/org/openrewrite/java/migrate/util/UseMapOf.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,9 +25,11 @@
import org.openrewrite.java.JavaTemplate;
import org.openrewrite.java.JavaVisitor;
import org.openrewrite.java.MethodMatcher;
import org.openrewrite.java.search.SemanticallyEqual;
import org.openrewrite.java.search.UsesJavaVersion;
import org.openrewrite.java.search.UsesMethod;
import org.openrewrite.java.tree.Expression;
import org.openrewrite.java.tree.Flag;
import org.openrewrite.java.tree.J;
import org.openrewrite.java.tree.JavaType;
import org.openrewrite.java.tree.Statement;
Expand Down Expand Up @@ -116,6 +118,7 @@ public J visitNewClass(J.NewClass newClass, ExecutionContext ctx) {
}
List<Statement> putStatements = ((J.Block) statement).getStatements();
List<Expression> args = new ArrayList<>();
List<Expression> keys = new ArrayList<>();
boolean useEntries = putStatements.size() > 10;
StringJoiner template = useEntries ?
new StringJoiner(", ", "Map.ofEntries(", ")") :
Expand All @@ -125,11 +128,23 @@ public J visitNewClass(J.NewClass newClass, ExecutionContext ctx) {
return n;
}
J.MethodInvocation put = (J.MethodInvocation) stat;
if (put.getArguments().size() != 2) {
return n;
}
// `Map.of(..)` and `Map.entry(..)` throw on null keys or values and on
// duplicate keys, where `HashMap.put(..)` tolerated both. Only rewrite
// when every key and value is provably non-null and the keys we can
// statically compare are distinct (issue #1307).
for (Expression arg : put.getArguments()) {
if (J.Literal.isLiteralValue(arg, null)) {
if (!isProvablyNonNull(arg)) {
return n;
}
}
Expression key = put.getArguments().get(0);
if (isDuplicateKey(key, keys)) {
return n;
}
keys.add(key);
args.addAll(put.getArguments());
if (useEntries) {
template.add("Map.entry(#{any()}, #{any()})");
Expand Down Expand Up @@ -260,6 +275,8 @@ private void identifyProseRewrites(

List<J.MethodInvocation> puts = new ArrayList<>();
List<UUID> absorbedHere = new ArrayList<>();
List<Expression> keys = new ArrayList<>();
boolean duplicateKey = false;
int j = i + 1;
while (j < stmts.size()) {
Statement next = stmts.get(j);
Expand All @@ -271,11 +288,19 @@ private void identifyProseRewrites(
expressionReferences(kv.get(1), targetName)) {
break;
}
// `Map.of(..)` / `Map.ofEntries(..)` throw on duplicate keys, where a
// `HashMap.put(..)` chain silently kept the last write. Bail the whole
// sequence when two keys we can statically compare are equal (issue #1307).
if (isDuplicateKey(kv.get(0), keys)) {
duplicateKey = true;
break;
}
keys.add(kv.get(0));
puts.add((J.MethodInvocation) next);
absorbedHere.add(next.getId());
j++;
}
if (puts.size() >= 2 && initializer != null) {
if (!duplicateKey && puts.size() >= 2 && initializer != null) {
rewrites.put(initializer.getId(), puts);
absorbedPutIds.addAll(absorbedHere);
i = j;
Expand Down Expand Up @@ -359,6 +384,19 @@ private boolean isProvablyNonNull(Expression expression) {
expression instanceof J.MemberReference) {
return true;
}
// A reference to an initialized `static final` constant (for example a String
// constant used as a key) is treated as non-null, matching the behaviour the
// anonymous-class form relied on before the nullability guard (issue #1307).
JavaType.Variable fieldType = null;
if (expression instanceof J.Identifier) {
fieldType = ((J.Identifier) expression).getFieldType();
} else if (expression instanceof J.FieldAccess) {
fieldType = ((J.FieldAccess) expression).getName().getFieldType();
}
if (fieldType != null &&
fieldType.hasFlags(Flag.Static, Flag.Final, Flag.HasInit)) {
return true;
}
JavaType type = expression.getType();
return type == JavaType.Primitive.Boolean ||
type == JavaType.Primitive.Byte ||
Expand All @@ -370,6 +408,22 @@ private boolean isProvablyNonNull(Expression expression) {
type == JavaType.Primitive.Short;
}

/**
* Returns {@code true} when {@code key} is semantically equal to a key already seen in
* the same map. {@code Map.of(..)} and {@code Map.ofEntries(..)} reject duplicate keys
* with an {@code IllegalArgumentException}, where the original {@code HashMap.put(..)}
* chain just kept the last write, so the rewrite has to be skipped in that case
* (issue #1307).
*/
private boolean isDuplicateKey(Expression key, List<Expression> seenKeys) {
for (Expression seen : seenKeys) {
if (SemanticallyEqual.areEqual(key, seen)) {
return true;
}
}
return false;
}

private boolean expressionReferences(Expression expr, String name) {
return new JavaIsoVisitor<AtomicBoolean>() {
@Override
Expand Down
130 changes: 130 additions & 0 deletions src/test/java/org/openrewrite/java/migrate/util/UseMapOfTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -689,4 +689,134 @@ Map<String, Object> prompt(Object card, Object attempt) {
)
);
}

@Issue("https://github.com/openrewrite/rewrite-migrate-java/issues/1307")
@Test
void doNotChangeWhenAnonymousClassValueMayBeNull() {
//language=java
rewriteRun(
java(
"""
import java.util.HashMap;
import java.util.Map;

class Test {
Map<String, Object> create(String dataOwner) {
return new HashMap<>() {{
put("owner", dataOwner);
put("id", "123");
}};
}
}
"""
)
);
}

@Issue("https://github.com/openrewrite/rewrite-migrate-java/issues/1307")
@Test
void doNotChangeAnonymousClassWithDuplicateKeys() {
//language=java
rewriteRun(
java(
"""
import java.util.HashMap;
import java.util.Map;

class Test {
Map<String, String> m = new HashMap<>() {{
put("key", "first");
put("key", "second");
}};
}
"""
)
);
}

@Issue("https://github.com/openrewrite/rewrite-migrate-java/issues/1307")
@Test
void doNotChangeProseChainWithDuplicateKeys() {
//language=java
rewriteRun(
java(
"""
import java.util.HashMap;
import java.util.Map;

class Test {
void m() {
Map<String, Integer> ages = new HashMap<>();
ages.put("Bob", 42);
ages.put("Bob", 30);
}
}
"""
)
);
}

@Issue("https://github.com/openrewrite/rewrite-migrate-java/issues/1307")
@Test
void changeAnonymousClassWithDistinctKeysAndNonNullValues() {
//language=java
rewriteRun(
java(
"""
import java.util.HashMap;
import java.util.Map;

class Test {
Map<String, String> m = new HashMap<>() {{
put("a", "1");
put("b", "2");
}};
}
""",
"""
import java.util.Map;

class Test {
Map<String, String> m = Map.of(
"a", "1",
"b", "2");
}
"""
)
);
}

@Issue("https://github.com/openrewrite/rewrite-migrate-java/issues/1307")
@Test
void changeProseChainWithDistinctKeys() {
//language=java
rewriteRun(
java(
"""
import java.util.HashMap;
import java.util.Map;

class Test {
void m() {
Map<String, Integer> ages = new HashMap<>();
ages.put("Bob", 42);
ages.put("alice", 30);
}
}
""",
"""
import java.util.HashMap;
import java.util.Map;

class Test {
void m() {
Map<String, Integer> ages = new HashMap<>(Map.of(
"Bob", 42,
"alice", 30));
}
}
"""
)
);
}
}