Skip to content

Commit

Permalink
Avoid ambiguity in assertions on iterable maps (#874)
Browse files Browse the repository at this point in the history 
Avoid ambiguity in assertions on iterable maps
  • Loading branch information
@carterkozak @bulldozer-bot
carterkozak authored and bulldozer-bot[bot] committed Sep 20, 2019
1 parent 8031bb8 commit 3914ea0
Show file tree
Hide file tree
Showing 3 changed files with 181 additions and 471 deletions.
149 changes: 85 additions & 64 deletions baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/PreferAssertj.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@
import java.util.Optional;
import java.util.function.BiConsumer;
import java.util.stream.Collectors;
import javax.lang.model.type.TypeKind;

/**
* {@link PreferAssertj} provides an automated path from legacy test libraries to AssertJ. Our goal is to migrate
Expand Down Expand Up @@ -223,61 +224,51 @@ public final class PreferAssertj extends BugChecker implements BugChecker.Method
@SuppressWarnings({"CyclomaticComplexity", "MethodLength"})
public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) {
if (ASSERT_TRUE.matches(tree, state)) {
return withAssertThat(tree, state, (assertThat, fix) ->
fix.replace(tree, assertThat + "(" + argSource(tree, state, 0) + ").isTrue()"));
return withAssertThat(tree, state, 0, (assertThat, fix) -> fix.replace(tree, assertThat + ".isTrue()"));
}
if (ASSERT_TRUE_DESCRIPTION.matches(tree, state)) {
return withAssertThat(tree, state, (assertThat, fix) ->
fix.replace(tree, assertThat + "(" + argSource(tree, state, 1)
+ ").describedAs(" + argSource(tree, state, 0) + ").isTrue()"));
return withAssertThat(tree, state, 1, (assertThat, fix) ->
fix.replace(tree, assertThat + ".describedAs(" + argSource(tree, state, 0) + ").isTrue()"));
}
if (ASSERT_FALSE.matches(tree, state)) {
return withAssertThat(tree, state, (assertThat, fix) ->
fix.replace(tree, assertThat + "(" + argSource(tree, state, 0) + ").isFalse()"));
return withAssertThat(tree, state, 0, (assertThat, fix) -> fix.replace(tree, assertThat + ".isFalse()"));
}
if (ASSERT_FALSE_DESCRIPTION.matches(tree, state)) {
return withAssertThat(tree, state, (assertThat, fix) ->
fix.replace(tree, assertThat + "(" + argSource(tree, state, 1)
+ ").describedAs(" + argSource(tree, state, 0) + ").isFalse()"));
return withAssertThat(tree, state, 1, (assertThat, fix) ->
fix.replace(tree, assertThat + ".describedAs(" + argSource(tree, state, 0) + ").isFalse()"));
}
if (ASSERT_NULL.matches(tree, state)) {
return withAssertThat(tree, state, (assertThat, fix) ->
fix.replace(tree, assertThat + "(" + argSource(tree, state, 0) + ").isNull()"));
return withAssertThat(tree, state, 0, (assertThat, fix) -> fix.replace(tree, assertThat + ".isNull()"));
}
if (ASSERT_NULL_DESCRIPTION.matches(tree, state)) {
return withAssertThat(tree, state, (assertThat, fix) ->
fix.replace(tree, assertThat + "(" + argSource(tree, state, 1)
+ ").describedAs(" + argSource(tree, state, 0) + ").isNull()"));
return withAssertThat(tree, state, 1, (assertThat, fix) ->
fix.replace(tree, assertThat + ".describedAs(" + argSource(tree, state, 0) + ").isNull()"));
}
if (ASSERT_NOT_NULL.matches(tree, state)) {
return withAssertThat(tree, state, (assertThat, fix) ->
fix.replace(tree, assertThat + "(" + argSource(tree, state, 0) + ").isNotNull()"));
return withAssertThat(tree, state, 0, (assertThat, fix) -> fix.replace(tree, assertThat + ".isNotNull()"));
}
if (ASSERT_NOT_NULL_DESCRIPTION.matches(tree, state)) {
return withAssertThat(tree, state, (assertThat, fix) ->
fix.replace(tree, assertThat + "(" + argSource(tree, state, 1)
+ ").describedAs(" + argSource(tree, state, 0) + ").isNotNull()"));
return withAssertThat(tree, state, 1, (assertThat, fix) ->
fix.replace(tree, assertThat + ".describedAs(" + argSource(tree, state, 0) + ").isNotNull()"));
}
if (ASSERT_SAME.matches(tree, state)) {
return withAssertThat(tree, state, (assertThat, fix) ->
fix.replace(tree, assertThat + "(" + argSource(tree, state, 1)
+ ").isSameAs(" + argSource(tree, state, 0) + ")"));
return withAssertThat(tree, state, 1, (assertThat, fix) ->
fix.replace(tree, assertThat + ".isSameAs(" + argSource(tree, state, 0) + ")"));
}
if (ASSERT_SAME_DESCRIPTION.matches(tree, state)) {
return withAssertThat(tree, state, (assertThat, fix) ->
fix.replace(tree, assertThat + "(" + argSource(tree, state, 2)
+ ").describedAs(" + argSource(tree, state, 0) + ").isSameAs("
return withAssertThat(tree, state, 2, (assertThat, fix) ->
fix.replace(tree, assertThat
+ ".describedAs(" + argSource(tree, state, 0) + ").isSameAs("
+ argSource(tree, state, 1) + ")"));
}
if (ASSERT_NOT_SAME.matches(tree, state)) {
return withAssertThat(tree, state, (assertThat, fix) ->
fix.replace(tree, assertThat + "(" + argSource(tree, state, 1)
+ ").isNotSameAs(" + argSource(tree, state, 0) + ")"));
return withAssertThat(tree, state, 1, (assertThat, fix) ->
fix.replace(tree, assertThat + ".isNotSameAs(" + argSource(tree, state, 0) + ")"));
}
if (ASSERT_NOT_SAME_DESCRIPTION.matches(tree, state)) {
return withAssertThat(tree, state, (assertThat, fix) ->
fix.replace(tree, assertThat + "(" + argSource(tree, state, 2)
+ ").describedAs(" + argSource(tree, state, 0) + ").isNotSameAs("
return withAssertThat(tree, state, 2, (assertThat, fix) ->
fix.replace(tree, assertThat
+ ".describedAs(" + argSource(tree, state, 0) + ").isNotSameAs("
+ argSource(tree, state, 1) + ")"));
}
if (FAIL_DESCRIPTION.matches(tree, state) || FAIL.matches(tree, state)) {
Expand All @@ -293,45 +284,41 @@ public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState

}
if (ASSERT_EQUALS_FLOATING.matches(tree, state)) {
return withAssertThat(tree, state, (assertThat, fix) -> fix
return withAssertThat(tree, state, 1, (assertThat, fix) -> fix
.addStaticImport("org.assertj.core.api.Assertions.within")
.replace(tree, String.format("%s(%s)%s",
.replace(tree, String.format("%s%s",
assertThat,
argSource(tree, state, 1),
isConstantZero(tree.getArguments().get(2))
? String.format(".isEqualTo(%s)", argSource(tree, state, 0))
: String.format(".isCloseTo(%s, within(%s))",
argSource(tree, state, 0), argSource(tree, state, 2)))));
}
if (ASSERT_EQUALS_FLOATING_DESCRIPTION.matches(tree, state)) {
return withAssertThat(tree, state, (assertThat, fix) -> fix
return withAssertThat(tree, state, 2, (assertThat, fix) -> fix
.addStaticImport("org.assertj.core.api.Assertions.within")
.replace(tree, String.format("%s(%s).describedAs(%s)%s",
.replace(tree, String.format("%s.describedAs(%s)%s",
assertThat,
argSource(tree, state, 2),
argSource(tree, state, 0),
isConstantZero(tree.getArguments().get(3))
? String.format(".isEqualTo(%s)", argSource(tree, state, 1))
: String.format(".isCloseTo(%s, within(%s))",
argSource(tree, state, 1), argSource(tree, state, 3)))));
}
if (ASSERT_NOT_EQUALS_FLOATING.matches(tree, state)) {
return withAssertThat(tree, state, (assertThat, fix) -> fix
return withAssertThat(tree, state, 1, (assertThat, fix) -> fix
.addStaticImport("org.assertj.core.api.Assertions.within")
.replace(tree, String.format("%s(%s)%s",
.replace(tree, String.format("%s%s",
assertThat,
argSource(tree, state, 1),
isConstantZero(tree.getArguments().get(2))
? String.format(".isNotEqualTo(%s)", argSource(tree, state, 0))
: String.format(".isNotCloseTo(%s, within(%s))",
argSource(tree, state, 0), argSource(tree, state, 2)))));
}
if (ASSERT_NOT_EQUALS_FLOATING_DESCRIPTION.matches(tree, state)) {
return withAssertThat(tree, state, (assertThat, fix) -> fix
return withAssertThat(tree, state, 2, (assertThat, fix) -> fix
.addStaticImport("org.assertj.core.api.Assertions.within")
.replace(tree, String.format("%s(%s).describedAs(%s)%s",
.replace(tree, String.format("%s.describedAs(%s)%s",
assertThat,
argSource(tree, state, 2),
argSource(tree, state, 0),
isConstantZero(tree.getArguments().get(3))
? String.format(".isNotEqualTo(%s)", argSource(tree, state, 1))
Expand All @@ -341,8 +328,8 @@ public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState
if (ASSERT_THAT.matches(tree, state)) {
Optional<String> replacement = tree.getArguments().get(1)
.accept(HamcrestVisitor.INSTANCE, state);
return withAssertThat(tree, state, (assertThat, fix) ->
fix.replace(tree, assertThat + "(" + argSource(tree, state, 0) + ")"
return withAssertThat(tree, state, 0, (assertThat, fix) ->
fix.replace(tree, assertThat
+ replacement.orElseGet(() ->
".is(new "
+ SuggestedFixes.qualifyType(state, fix, "org.assertj.core.api.HamcrestCondition")
Expand All @@ -352,26 +339,26 @@ public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState
if (ASSERT_THAT_DESCRIPTION.matches(tree, state)) {
Optional<String> replacement = tree.getArguments().get(2)
.accept(HamcrestVisitor.INSTANCE, state);
return withAssertThat(tree, state, (assertThat, fix) ->
fix.replace(tree, assertThat + "(" + argSource(tree, state, 1)
+ ").describedAs(" + argSource(tree, state, 0) + ")"
return withAssertThat(tree, state, 1, (assertThat, fix) ->
fix.replace(tree, assertThat
+ ".describedAs(" + argSource(tree, state, 0) + ")"
+ replacement.orElseGet(() -> ".is(new "
+ SuggestedFixes.qualifyType(state, fix, "org.assertj.core.api.HamcrestCondition")
+ "<>(" + argSource(tree, state, 2) + "))")));
}
if (ASSERT_EQUALS_CATCHALL.matches(tree, state)) {
int parameters = tree.getArguments().size();
if (parameters == 2) {
return withAssertThat(tree, state, (assertThat, fix) ->
fix.replace(tree, assertThat + "(" + argSource(tree, state, 1)
+ ").isEqualTo(" + argSource(tree, state, 0) + ")"));
return withAssertThat(tree, state, 1, (assertThat, fix) ->
fix.replace(tree, assertThat
+ ".isEqualTo(" + argSource(tree, state, 0) + ")"));
} else if (parameters == 3 && ASTHelpers.isSameType(
ASTHelpers.getType(tree.getArguments().get(0)),
state.getTypeFromString(String.class.getName()),
state)) {
return withAssertThat(tree, state, (assertThat, fix) ->
fix.replace(tree, assertThat + "(" + argSource(tree, state, 2)
+ ").describedAs(" + argSource(tree, state, 0) + ").isEqualTo("
return withAssertThat(tree, state, 2, (assertThat, fix) ->
fix.replace(tree, assertThat
+ ".describedAs(" + argSource(tree, state, 0) + ").isEqualTo("
+ argSource(tree, state, 1) + ")"));
} else {
// Does not fix assertArrayEquals(double[], double[], double)
Expand All @@ -382,16 +369,16 @@ public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState
if (ASSERT_NOT_EQUALS_CATCHALL.matches(tree, state)) {
int parameters = tree.getArguments().size();
if (parameters == 2) {
return withAssertThat(tree, state, (assertThat, fix) ->
fix.replace(tree, assertThat + "(" + argSource(tree, state, 1)
+ ").isNotEqualTo(" + argSource(tree, state, 0) + ")"));
return withAssertThat(tree, state, 1, (assertThat, fix) ->
fix.replace(tree, assertThat
+ ".isNotEqualTo(" + argSource(tree, state, 0) + ")"));
} else if (parameters == 3 && ASTHelpers.isSameType(
ASTHelpers.getType(tree.getArguments().get(0)),
state.getTypeFromString(String.class.getName()),
state)) {
return withAssertThat(tree, state, (assertThat, fix) ->
fix.replace(tree, assertThat + "(" + argSource(tree, state, 2)
+ ").describedAs(" + argSource(tree, state, 0) + ").isNotEqualTo("
return withAssertThat(tree, state, 2, (assertThat, fix) ->
fix.replace(tree, assertThat
+ ".describedAs(" + argSource(tree, state, 0) + ").isNotEqualTo("
+ argSource(tree, state, 1) + ")"));
} else {
// I'm not aware of anything that should hit this.
Expand All @@ -408,6 +395,7 @@ public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState
private Description withAssertThat(
MethodInvocationTree tree,
VisitorState state,
int actualIndex,
BiConsumer<String, SuggestedFix.Builder> assertThat) {
SuggestedFix.Builder fix = SuggestedFix.builder();
String qualified;
Expand All @@ -419,12 +407,32 @@ private Description withAssertThat(
} else {
qualified = SuggestedFixes.qualifyType(state, fix, "org.assertj.core.api.Assertions.assertThat");
}
assertThat.accept(qualified, fix);

String actualArgumentString = argSource(tree, state, actualIndex);
ExpressionTree actualArgument = tree.getArguments().get(actualIndex);
if (isIterableMap(actualArgument, state)) {
actualArgumentString = String.format("(%s<?, ?>) %s",
SuggestedFixes.qualifyType(state, fix, "java.util.Map"),
actualArgumentString);
}
assertThat.accept(qualified + '(' + actualArgumentString + ')', fix);
return buildDescription(tree)
.addFix(fix.build())
.build();
}

private static boolean isIterableMap(ExpressionTree value, VisitorState state) {
return isSubtype(value, "java.lang.Iterable", state)
&& isSubtype(value, "java.util.Map", state);
}

private static boolean isSubtype(ExpressionTree value, String castableTo, VisitorState state) {
return ASTHelpers.isSubtype(
ASTHelpers.getType(value),
state.getTypeFromString(castableTo),
state);
}

private static boolean useStaticAssertjImport(VisitorState state) {
for (ImportTree importTree : state.getPath().getCompilationUnit().getImports()) {
if (!importTree.isStatic()) {
Expand Down Expand Up @@ -462,11 +470,24 @@ private static boolean isExpressionSameType(VisitorState state, MemberSelectTree
state);
}

private static String argSource(MethodInvocationTree invocation, VisitorState state, int index) {
private static String argSource(
MethodInvocationTree invocation,
VisitorState state,
int index) {
checkArgument(index >= 0, "Index must be non-negative");
List<? extends ExpressionTree> arguments = checkNotNull(invocation, "MethodInvocationTree").getArguments();
checkArgument(index < arguments.size(), "Index is out of bounds");
return checkNotNull(state.getSourceForNode(arguments.get(index)), "Failed to find argument source");
ExpressionTree argument = arguments.get(index);
Symbol.VarSymbol symbol = checkNotNull(ASTHelpers.getSymbol(invocation), "symbol").getParameters().get(index);
String argumentSource = checkNotNull(state.getSourceForNode(argument), "Failed to find argument source");
if (symbol.type.isPrimitive()
// Limit to only float and double because assertEquals for ints uses assertEquals(long, long),
// we don't want to cast everything to long unnecessarily.
&& (symbol.type.getKind() == TypeKind.FLOAT || symbol.type.getKind() == TypeKind.DOUBLE)
&& !ASTHelpers.isSameType(symbol.type, ASTHelpers.getType(argument), state)) {
return String.format("(%s) %s", symbol.type.toString(), argumentSource);
}
return argumentSource;
}

private static final class HamcrestVisitor extends SimpleTreeVisitor<Optional<String>, VisitorState> {
Expand Down
Loading

0 comments on commit 3914ea0

Please sign in to comment.