Skip to content

Commit

Permalink
Revise the argument swap algorithm
Browse files Browse the repository at this point in the history 
to be more flexible to fiuture revisions we have planned. In particular,
we can now change which expressions to consider.

MOE_MIGRATED_REVID=135414105
  • Loading branch information
@ciera @cushon
ciera authored and cushon committed Oct 7, 2016
1 parent 1ad1b16 commit 7a26f95
Show file tree
Hide file tree
Showing 3 changed files with 156 additions and 117 deletions.
175 changes: 79 additions & 96 deletions core/src/main/java/com/google/errorprone/bugpatterns/ArgumentParameterSwap.java
View file
Expand Up @@ -18,10 +18,12 @@
import static com.google.errorprone.BugPattern.Category.JDK;
import static com.google.errorprone.BugPattern.SeverityLevel.ERROR;

import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.CaseFormat;
import com.google.common.base.Joiner;
import com.google.common.base.Splitter;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Sets;
import com.google.errorprone.BugPattern;
import com.google.errorprone.VisitorState;
Expand All @@ -32,11 +34,11 @@
import com.google.errorprone.matchers.Description;
import com.google.errorprone.util.ASTHelpers;
import com.sun.source.tree.ExpressionTree;
import com.sun.source.tree.IdentifierTree;
import com.sun.source.tree.MemberSelectTree;
import com.sun.source.tree.MethodInvocationTree;
import com.sun.source.tree.NewClassTree;
import com.sun.source.tree.Tree;
import com.sun.source.tree.Tree.Kind;
import com.sun.tools.javac.code.Symbol.MethodSymbol;
import com.sun.tools.javac.code.Symbol.VarSymbol;
import com.sun.tools.javac.tree.JCTree;
Expand All @@ -63,6 +65,9 @@ public class ArgumentParameterSwap extends BugChecker
public static final List<String> IGNORE_PARAMS =
ImmutableList.of("message", "counter", "index", "object", "value");

static final Set<Kind> VALID_KINDS =
ImmutableSet.of(Kind.MEMBER_SELECT, Kind.IDENTIFIER, Kind.METHOD_INVOCATION);

@Override
public Description matchNewClass(NewClassTree tree, VisitorState state) {
MethodSymbol symbol = ASTHelpers.getSymbol(tree);
Expand All @@ -85,51 +90,37 @@ public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState

private Description findSwaps(
ExpressionTree[] args, VarSymbol[] params, Tree tree, VisitorState state) {
LexicalSimilarityMatcher matcher = new LexicalSimilarityMatcher(args, params, state);
boolean bestMatch = true;

// TODO(eaftan): Capture the list of potential matches for a parameter based on what is in
// scope with the same type and kind.

// TODO(ciera): Instead of the algorithm below, capture all of the potential
// matches for each argument, then select the best match and store it. Return a match
// if the suggested replacement is different from the existing
for (int ndx = 0; ndx < args.length; ndx++) {
if (!(args[ndx] instanceof IdentifierTree)
&& !(args[ndx] instanceof MemberSelectTree)
&& !(args[ndx] instanceof MethodInvocationTree)) {
continue;
}
String[] suggestion = new String[params.length];

if (params[ndx].getSimpleName().toString().length() <= 4) {
// Create a list of suggested best arguments for the method in question.
for (int ndx = 0; ndx < params.length; ndx++) {
String paramName = params[ndx].getSimpleName().toString();
String argName = getRelevantName(args[ndx]);
// If the parameter for this argument is under 4 characters or in our ignore list, just
// presume it is correct.
if (paramName.length() <= 4 || IGNORE_PARAMS.contains(paramName)) {
suggestion[ndx] = args[ndx].toString();
continue;
}

if (IGNORE_PARAMS.contains(params[ndx].getSimpleName().toString())) {
continue;
}
// Get all possible matches for the parameter in question that have the same kind as the
// argument and get the relevant part of their name for matching up.
ExpressionTree[] possibleMatches =
getPossibleMatches(params[ndx], args, args[ndx].getKind(), state);
String[] relevantMatchNames =
Arrays.stream(possibleMatches).map(exp -> getRelevantName(exp)).toArray(String[]::new);

if (matcher.findBestArgument(ndx) != ndx) {
bestMatch = false;
break;
}
// Figure out which one is best. Use the existing if nothing else works.
int best = findBestMatch(relevantMatchNames, argName, paramName);
suggestion[ndx] = best == -1 ? args[ndx].toString() : possibleMatches[best].toString();
}

if (bestMatch) {
// If the suggestions match what we already have, then don't suggest any changes.
String[] current = Arrays.stream(args).map(ExpressionTree::toString).toArray(String[]::new);
if (Arrays.equals(current, suggestion)) {
return Description.NO_MATCH;
}

String[] suggestion = new String[params.length];
for (int pNdx = 0; pNdx < params.length; pNdx++) {
int match = pNdx;
if (args[pNdx] instanceof IdentifierTree
|| args[pNdx] instanceof MemberSelectTree
|| args[pNdx] instanceof MethodInvocationTree) {
match = matcher.findBestArgument(pNdx);
}
suggestion[pNdx] = args[match].toString();
}

Fix fix =
SuggestedFix.replace(
((JCTree) args[0]).getStartPosition(),
Expand All @@ -138,81 +129,73 @@ private Description findSwaps(
return describeMatch(tree, fix);
}

static class LexicalSimilarityMatcher {
private final double[][] simMatrix;
private final VisitorState state;
private final VarSymbol[] params;
private final ExpressionTree[] args;

private LexicalSimilarityMatcher(
ExpressionTree[] args, VarSymbol[] params, VisitorState state) {
this.state = state;
this.params = params;
this.args = args;

String[] paramNames =
Arrays.stream(params)
.map(param -> param.getSimpleName().toString())
.toArray(String[]::new);
String[] argNames = new String[args.length];
for (int ndx = 0; ndx < args.length; ndx++) {
ExpressionTree tree = args[ndx];
if (tree instanceof MemberSelectTree) {
argNames[ndx] = ((MemberSelectTree) tree).getIdentifier().toString();
} else if (tree instanceof MethodInvocationTree) {
argNames[ndx] = ASTHelpers.getSymbol(tree).getSimpleName().toString();
} else {
argNames[ndx] = tree.toString();
}
}

this.simMatrix = calculateSimilarityMatrix(argNames, paramNames);
/** Gets the part of the expression that represents the "name" for each value in expressions. */
private String getRelevantName(ExpressionTree exp) {
if (exp instanceof MemberSelectTree) {
return ((MemberSelectTree) exp).getIdentifier().toString();
} else if (exp instanceof MethodInvocationTree) {
return ASTHelpers.getSymbol(exp).getSimpleName().toString();
} else {
return exp.toString();
}
}
/**
* Finds all possible matches for param that have the same kind as the original argument, have a
* kind that we can find replacements for, and are subtype compatible with the parameter.
*/
private ExpressionTree[] getPossibleMatches(
VarSymbol param, ExpressionTree[] args, Kind originalKind, VisitorState state) {
// TODO(eaftan): Capture the list of potential matches for a parameter based on what is in
// scope with the same type and kind.
return Arrays.stream(args)
.filter(
arg ->
(arg.getKind().equals(originalKind)
&& VALID_KINDS.contains(arg.getKind())
&& state.getTypes().isSubtype(ASTHelpers.getType(arg), param.asType())))
.toArray(ExpressionTree[]::new);
}

/**
* Given a parameter index, returns the argument index with the highest match. If there is a
* tie, it will always favor the original index, followed by the first highest index.
*/
int findBestArgument(int pNdx) {
int maxNdx = pNdx;
for (int aNdx = 0; aNdx < params.length; aNdx++) {
if (!state.getTypes().isSubtype(ASTHelpers.getType(args[aNdx]), params[pNdx].asType())) {
continue;
}
// TODO(ciera): Use a beta value and require that anything better than existing must be at
// least beta better than existing.
if (simMatrix[aNdx][pNdx] > simMatrix[maxNdx][pNdx]) {
maxNdx = aNdx;
}
/**
* Given a parameter index, returns the argument index with the highest match. If there is a tie,
* it will always favor the original index, followed by the first highest index.
*/
@VisibleForTesting
static int findBestMatch(String[] potentialMatches, String original, String param) {
double maxMatch = calculateSimilarity(original, param);
int maxNdx = -1;
for (int ndx = 0; ndx < potentialMatches.length; ndx++) {
// TODO(andrewrice): Use a beta value and require that anything better than existing must be
// at least beta better than existing.
double similarity = calculateSimilarity(potentialMatches[ndx], param);
if (similarity > maxMatch) {
maxNdx = ndx;
}
return maxNdx;
}
return maxNdx;
}

/**
* Calculates, for each argument/parameter pair, how close the argument and parameter are to each
* other based on the formula |argTerms intersect paramTerms| / (|argTerms| + |paramTerms|) * 2
* Calculates, for the provided argument and parameter, how close the argument and parameter are
* to each other based on the formula |argTerms intersect paramTerms| / (|argTerms| +
* |paramTerms|) * 2
*/
static double[][] calculateSimilarityMatrix(String[] args, String[] params) {
@VisibleForTesting
static double calculateSimilarity(String arg, String param) {
// TODO(ciera): consider also using edit distance on individual words.
double[][] similarity = new double[args.length][params.length];
for (int aNdx = 0; aNdx < args.length; aNdx++) {
Set<String> argSplit = splitStringTerms(args[aNdx]);
for (int pNdx = 0; pNdx < params.length; pNdx++) {
Set<String> paramSplit = splitStringTerms(params[pNdx]);
// TODO(ciera): Handle the substring cases so that lenList matches listLength
double commonTerms = Sets.intersection(argSplit, paramSplit).size() * 2;
double totalTerms = argSplit.size() + paramSplit.size();
similarity[aNdx][pNdx] = commonTerms / totalTerms;
}
}
return similarity;
Set<String> argSplit = splitStringTerms(arg);
Set<String> paramSplit = splitStringTerms(param);
// TODO(andrewrice): Handle the substring cases so that lenList matches listLength
double commonTerms = Sets.intersection(argSplit, paramSplit).size() * 2;
double totalTerms = argSplit.size() + paramSplit.size();
return commonTerms / totalTerms;
}

/**
* Splits a string into a Set of terms. If the name starts with a lower-case letter, it is
* presumed to be in lowerCamelCase format. Otherwise, it presumes UPPER_UNDERSCORE format.
*/
@VisibleForTesting
static Set<String> splitStringTerms(String name) {
// TODO(ciera): Handle lower_underscore
CaseFormat caseFormat =
Expand Down
96 changes: 76 additions & 20 deletions core/src/test/java/com/google/errorprone/bugpatterns/ArgumentParameterSwapTest.java
View file
Expand Up @@ -29,6 +29,7 @@
@RunWith(JUnit4.class)
public class ArgumentParameterSwapTest {
private CompilationTestHelper compilationHelper;
private static final double EPSILON = 0.001;

@Before
public void setUp() {
Expand All @@ -46,39 +47,94 @@ public void testNegativeCase() throws Exception {
}

@Test
public void calculateSimilarityMatrix() throws Exception {
String[] args = {"foo", "fooBarFoo", "fooBar"};
String[] params = {"foo", "bar", "barFoo"};
double[][] simMatrix = ArgumentParameterSwap.calculateSimilarityMatrix(args, params);

assertThat(simMatrix[0]).hasValuesWithin(0.001).of(new double[] {1.0, 0.0, 0.6666});
assertThat(simMatrix[1]).hasValuesWithin(0.001).of(new double[] {0.6666, 0.6666, 1.0});
assertThat(simMatrix[2]).hasValuesWithin(0.001).of(new double[] {0.6666, 0.6666, 1.0});
public void findBestMatch_original() {
assertThat(ArgumentParameterSwap.findBestMatch(new String[] {"baz", "other"}, "foo", "fooBar"))
.isEqualTo(-1);
}

@Test
public void calculateSimilarityMatrix2() throws Exception {
String[] args = {"extraPath", "keepPath", "dropPath"};
String[] params = {"keepPath", "dropPath", "extraPath"};
double[][] simMatrix = ArgumentParameterSwap.calculateSimilarityMatrix(args, params);

assertThat(simMatrix[0]).hasValuesWithin(0.001).of(new double[] {0.5, 0.5, 1.0});
assertThat(simMatrix[1]).hasValuesWithin(0.001).of(new double[] {1.0, 0.5, 0.5});
assertThat(simMatrix[2]).hasValuesWithin(0.001).of(new double[] {0.5, 1.0, 0.5});
public void findBestMatch_first() {
assertThat(ArgumentParameterSwap.findBestMatch(new String[] {"bar", "other"}, "baz", "fooBar"))
.isEqualTo(0);
}

@Test
public void splitStringTermsToSet() throws Exception {
public void findBestMatch_last() {
assertThat(ArgumentParameterSwap.findBestMatch(new String[] {"baz", "foo"}, "baz", "fooBar"))
.isEqualTo(1);
}

@Test
public void findBestMatch_same() {
assertThat(ArgumentParameterSwap.findBestMatch(new String[] {"bar", "other"}, "foo", "fooBar"))
.isEqualTo(-1);
}

@Test
public void calculateSimilarity_same() throws Exception {
assertThat(ArgumentParameterSwap.calculateSimilarity("foo", "foo")).isWithin(EPSILON).of(1.0);
}

@Test
public void calculateSimilarity_different() throws Exception {
assertThat(ArgumentParameterSwap.calculateSimilarity("foo", "bar")).isWithin(EPSILON).of(0.0);
}

@Test
public void calculateSimilarity_partialSmaller() throws Exception {
assertThat(ArgumentParameterSwap.calculateSimilarity("foo", "barFoo"))
.isWithin(EPSILON)
.of(0.6667);
}

@Test
public void calculateSimilarity_partialLarger() throws Exception {
assertThat(ArgumentParameterSwap.calculateSimilarity("fooBar", "bar"))
.isWithin(EPSILON)
.of(0.6667);
}

@Test
public void calculateSimilarity_partialRepeated() throws Exception {
assertThat(ArgumentParameterSwap.calculateSimilarity("fooBarFoo", "foo"))
.isWithin(EPSILON)
.of(0.6667);
}

@Test
public void calculateSimilarity_partialSame() throws Exception {
assertThat(ArgumentParameterSwap.calculateSimilarity("fooBar", "barBaz"))
.isWithin(EPSILON)
.of(0.5);
}

@Test
public void splitStringTerms_lower() throws Exception {
Set<String> terms = ArgumentParameterSwap.splitStringTerms("foo");
assertThat(terms).containsExactly("foo");
}

terms = ArgumentParameterSwap.splitStringTerms("fooBar");
@Test
public void splitStringTerms_camel() throws Exception {
Set<String> terms = ArgumentParameterSwap.splitStringTerms("fooBar");
assertThat(terms).containsExactly("foo", "bar");
}

@Test
public void splitStringTerms_upper() throws Exception {
Set<String> terms = ArgumentParameterSwap.splitStringTerms("FOO_BAR");
assertThat(terms).containsExactly("foo", "bar");
}

terms = ArgumentParameterSwap.splitStringTerms("fooBarFoo");
@Test
public void splitStringTerms_repeated() throws Exception {
Set<String> terms = ArgumentParameterSwap.splitStringTerms("fooBarFoo");
assertThat(terms).containsExactly("foo", "bar");
}

terms = ArgumentParameterSwap.splitStringTerms("fooBarID");
@Test
public void splitStringTerms_single() throws Exception {
Set<String> terms = ArgumentParameterSwap.splitStringTerms("fooBarID");
assertThat(terms).containsExactly("foo", "bar", "i", "d");
}

Expand Down
2 changes: 1 addition & 1 deletion ...t/java/com/google/errorprone/bugpatterns/testdata/ArgumentParameterSwapPositiveCases.java
View file
Expand Up @@ -18,7 +18,7 @@
/** @author yulissa@google.com (Yulissa Arroyo-Paredes) */
public class ArgumentParameterSwapPositiveCases {
// names are identical but they are swapped
public String doSomething(String[] keepPath, String[] dropPath, String top, String bottom) {
public String doSomething(String[] keepPath, String[] dropPath, String topVal, String bottom) {
return "wrong";
}

Expand Down

0 comments on commit 7a26f95

Please sign in to comment.