Skip to content

Commit

Permalink
Fix bug in AssertEqualsArgumentOrderChecker which meant it considered
Browse files Browse the repository at this point in the history 
a method which returns an enum value as a constant and so swapped it
to the 'expected' position

RELNOTES: Fix bug in AssertEqualsArgumentChecker handling methods returning an
enum

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=155413441
  • Loading branch information
@ronshapiro
andrewrice authored and ronshapiro committed May 24, 2017
1 parent 71b874b commit d7f9187
Show file tree
Hide file tree
Showing 6 changed files with 131 additions and 36 deletions.
2 changes: 1 addition & 1 deletion ...oogle/errorprone/bugpatterns/argumentselectiondefects/ArgumentSelectionDefectChecker.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -209,7 +209,7 @@ public Double apply(ParameterPair pair) {
return 0.0;
}

if (pair.formal().isNamed() && pair.actual().isNamed()) {
if (!pair.formal().isUnknownName() && !pair.actual().isUnknownName()) {
String normalizedSource =
NamingConventions.convertToLowerUnderscore(pair.formal().name());
String normalizedTarget =
Expand Down
17 changes: 12 additions & 5 deletions ...gle/errorprone/bugpatterns/argumentselectiondefects/AssertEqualsArgumentOrderChecker.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,6 @@
import com.sun.source.tree.NewClassTree;
import com.sun.tools.javac.code.Symbol.TypeSymbol;
import com.sun.tools.javac.code.Symbol.VarSymbol;
import com.sun.tools.javac.code.Type;
import java.util.List;
import java.util.function.Function;
import java.util.regex.Pattern;
Expand Down Expand Up @@ -163,7 +162,7 @@ public Double apply(ParameterPair parameterPair) {
String actualName = actual.name();

if (formalName.equals("expected")) {
if (actual.isConstant() || isEnumType(actual.type())) {
if (actual.constant() || isEnumIdentifier(actual)) {
return 0.0;
}
if (actualName.startsWith("expected")) {
Expand All @@ -173,7 +172,7 @@ public Double apply(ParameterPair parameterPair) {
}

if (formalName.equals("actual")) {
if (actual.isConstant() || isEnumType(actual.type())) {
if (actual.constant() || isEnumIdentifier(actual)) {
return 1.0;
}
if (actualName.startsWith("actual")) {
Expand All @@ -187,8 +186,16 @@ public Double apply(ParameterPair parameterPair) {
};
}

private static boolean isEnumType(Type t) {
TypeSymbol typeSymbol = t.tsym;
/** Returns true if this parameter is an enum identifier */
private static boolean isEnumIdentifier(Parameter parameter) {
switch (parameter.kind()) {
case IDENTIFIER:
case MEMBER_SELECT:
break;
default:
return false;
}
TypeSymbol typeSymbol = parameter.type().tsym;
if (typeSymbol != null) {
return typeSymbol.getKind() == ElementKind.ENUM;
}
Expand Down
21 changes: 21 additions & 0 deletions core/src/main/java/com/google/errorprone/bugpatterns/argumentselectiondefects/Costs.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -117,4 +117,25 @@ void updatePair(ParameterPair p, double cost) {
void invalidatePair(ParameterPair p) {
updatePair(p, Double.POSITIVE_INFINITY);
}

@Override
public String toString() {
StringBuilder builder = new StringBuilder("Costs:\n");
builder.append("formals=").append(formals).append("\n");
builder.append("actuals=").append(actuals).append("\n");
builder.append("costMatrix=\n");
builder.append(String.format("%20s", ""));
for (int j = 0; j < costMatrix[0].length; j++) {
builder.append(String.format("%20s", actuals.get(j).name()));
}
builder.append("\n");
for (int i = 0; i < costMatrix.length; i++) {
builder.append(String.format("%20s", formals.get(i).name()));
for (int j = 0; j < costMatrix[i].length; j++) {
builder.append(String.format("%20.1f", costMatrix[i][j]));
}
builder.append("\n");
}
return builder.toString();
}
}
42 changes: 19 additions & 23 deletions core/src/main/java/com/google/errorprone/bugpatterns/argumentselectiondefects/Parameter.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
import com.sun.source.tree.MemberSelectTree;
import com.sun.source.tree.MethodInvocationTree;
import com.sun.source.tree.NewClassTree;
import com.sun.source.tree.Tree.Kind;
import com.sun.source.tree.VariableTree;
import com.sun.tools.javac.code.Symbol;
import com.sun.tools.javac.code.Symbol.ClassSymbol;
Expand All @@ -52,17 +53,12 @@ abstract class Parameter {

private static final ImmutableSet<String> METHODNAME_PREFIXES_TO_REMOVE =
ImmutableSet.of("get", "set", "is");

/** We use this placeholder to indicate a name which is a null literal. */
@VisibleForTesting static final String NAME_NULL = "*NULL*";

/** We use this placeholder to indicate a name which we couldn't get a canonical string for. */
@VisibleForTesting static final String NAME_UNKNOWN = "*UNKNOWN*";

/**
* We use this placeholder to indicate a name which is a compile-time constant (other than null).
*/
@VisibleForTesting static final String NAME_CONSTANT = "*CONSTANT*";
@VisibleForTesting static final String NAME_NOT_PRESENT = "*NOT_PRESENT*";

abstract String name();

Expand All @@ -72,6 +68,10 @@ abstract class Parameter {

abstract String text();

abstract Kind kind();

abstract boolean constant();

static ImmutableList<Parameter> createListFromVarSymbols(List<VarSymbol> varSymbols) {
return Streams.mapWithIndex(
varSymbols.stream(),
Expand All @@ -80,7 +80,9 @@ static ImmutableList<Parameter> createListFromVarSymbols(List<VarSymbol> varSymb
s.getSimpleName().toString(),
s.asType(),
(int) i,
s.getSimpleName().toString()))
s.getSimpleName().toString(),
Kind.IDENTIFIER,
false))
.collect(toImmutableList());
}

Expand All @@ -93,7 +95,9 @@ static ImmutableList<Parameter> createListFromExpressionTrees(
getArgumentName(t),
Optional.ofNullable(ASTHelpers.getResultType(t)).orElse(Type.noType),
(int) i,
t.toString()))
t.toString(),
t.getKind(),
ASTHelpers.constValue(t) != null))
.collect(toImmutableList());
}

Expand Down Expand Up @@ -124,15 +128,7 @@ boolean isNullLiteral() {
}

boolean isUnknownName() {
return name().equals(NAME_UNKNOWN);
}

boolean isConstant() {
return name().equals(NAME_CONSTANT);
}

boolean isNamed() {
return !isNullLiteral() && !isUnknownName() && !isConstant();
return name().equals(NAME_NOT_PRESENT);
}

private static String getClassName(ClassSymbol s) {
Expand Down Expand Up @@ -174,7 +170,7 @@ static String getArgumentName(ExpressionTree expressionTree) {
if (idTree.getName().contentEquals("this")) {
// for the 'this' keyword the argument name is the name of the object's class
Symbol sym = ASTHelpers.getSymbol(idTree);
return sym != null ? getClassName(ASTHelpers.enclosingClass(sym)) : NAME_UNKNOWN;
return sym != null ? getClassName(ASTHelpers.enclosingClass(sym)) : NAME_NOT_PRESENT;
} else {
// if we have a variable, just extract its name
return idTree.getName().toString();
Expand All @@ -201,15 +197,15 @@ static String getArgumentName(ExpressionTree expressionTree) {
return name;
}
} else {
return NAME_UNKNOWN;
return NAME_NOT_PRESENT;
}
case NEW_CLASS:
MethodSymbol constructorSym = ASTHelpers.getSymbol((NewClassTree) expressionTree);
return constructorSym != null && constructorSym.owner != null
? getClassName((ClassSymbol) constructorSym.owner)
: NAME_UNKNOWN;
: NAME_NOT_PRESENT;
default:
return ASTHelpers.constValue(expressionTree) != null ? NAME_CONSTANT : NAME_UNKNOWN;
return NAME_NOT_PRESENT;
}
}
}
16 changes: 16 additions & 0 deletions ...errorprone/bugpatterns/argumentselectiondefects/AssertEqualsArgumentOrderCheckerTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -239,4 +239,20 @@ public void assertEqualsCheck_makesNoChange_whenArgumentIsEnumMember() throws Ex
"}")
.doTest();
}

@Test
public void assertEqualsCheck_makesNoChange_withReturnedEnum() throws Exception {
compilationHelper
.addSourceLines(
"Test.java",
"abstract class Test {",
" static void assertEquals(Object expected, Object actual) {};",
" enum MyEnum {}",
" abstract MyEnum enumValue();",
" void test(Object other) {",
" assertEquals(other, enumValue());",
" }",
"}")
.doTest();
}
}
69 changes: 62 additions & 7 deletions ...c/test/java/com/google/errorprone/bugpatterns/argumentselectiondefects/ParameterTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -273,6 +273,7 @@ public void getName_usesOwner_fromGetMethodInAnonymousClass() {
.doTest();
}


@Test
public void getName_returnsNull_withNullLiteral() {
CompilationTestHelper.newInstance(PrintNameOfFirstArgument.class, getClass())
Expand All @@ -289,33 +290,87 @@ public void getName_returnsNull_withNullLiteral() {
}

@Test
public void getName_returnsConstant_withConstant() {
public void getName_returnsUnknown_withTerneryIf() {
CompilationTestHelper.newInstance(PrintNameOfFirstArgument.class, getClass())
.addSourceLines(
"Test.java",
"abstract class Test {",
" abstract void target(Object o);",
" void test(boolean flag) {",
" // BUG: Diagnostic contains: " + Parameter.NAME_NOT_PRESENT,
" target(flag ? 1 : 0);",
" }",
"}")
.doTest();
}

/** A {@link BugChecker} that prints whether the first argument is constant */
@BugPattern(
name = "PrintIsConstantFirstArgument",
category = Category.ONE_OFF,
severity = SeverityLevel.ERROR,
summary = "Print whether the first argument is constant"
)
public static class PrintIsConstantFirstArgument extends BugChecker
implements MethodInvocationTreeMatcher {

@Override
public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) {
List<? extends ExpressionTree> arguments = tree.getArguments();
if (arguments.isEmpty()) {
return Description.NO_MATCH;
}
List<Parameter> parameters = Parameter.createListFromExpressionTrees(arguments);
Parameter first = Iterables.getFirst(parameters, null);
return buildDescription(tree).setMessage(String.valueOf(first.constant())).build();
}
}

@Test
public void getName_returnsConstant_withConstant() {
CompilationTestHelper.newInstance(PrintIsConstantFirstArgument.class, getClass())
.addSourceLines(
"Test.java",
"abstract class Test {",
" abstract void target(Object o);",
" void test() {",
" // BUG: Diagnostic contains: " + Parameter.NAME_CONSTANT,
" // BUG: Diagnostic contains: true",
" target(1);",
" }",
"}")
.doTest();
}

@Test
public void getName_returnsUnknown_withTerneryIf() {
CompilationTestHelper.newInstance(PrintNameOfFirstArgument.class, getClass())
public void getName_returnsConstant_withConstantFromOtherClass() {
CompilationTestHelper.newInstance(PrintIsConstantFirstArgument.class, getClass())
.addSourceLines(
"Test.java",
"abstract class Test {",
" abstract void target(Object o);",
" void test(boolean flag) {",
" // BUG: Diagnostic contains: " + Parameter.NAME_UNKNOWN,
" target(flag ? 1 : 0);",
" void test() {",
" // BUG: Diagnostic contains: true",
" target(Double.MAX_VALUE);",
" }",
"}")
.doTest();
}


@Test
public void getName_returnsNotConstant_withVariable() {
CompilationTestHelper.newInstance(PrintIsConstantFirstArgument.class, getClass())
.addSourceLines(
"Test.java",
"abstract class Test {",
" abstract void target(Object o);",
" void test(Object o) {",
" // BUG: Diagnostic contains: false",
" target(o);",
" }",
"}")
.doTest();
}


}

0 comments on commit d7f9187

Please sign in to comment.