Skip to content

Commit

Permalink
Refactor ArgumentSelectionDefectChecker and AssertEqualsArgumentOrder…
Browse files Browse the repository at this point in the history
…Checker to

share a library object rather than inherit from one another

RELNOTES: Move argument selection checking logic to a separate class

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=155563608
  • Loading branch information
andrewrice authored and ronshapiro committed May 23, 2017
1 parent b3fef65 commit bb3c426
Show file tree
Hide file tree
Showing 6 changed files with 307 additions and 161 deletions.
Original file line number Diff line number Diff line change
@@ -0,0 +1,124 @@
/*
* Copyright 2017 Google Inc. All Rights Reserved.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package com.google.errorprone.bugpatterns.argumentselectiondefects;

import com.google.auto.value.AutoValue;
import com.google.common.collect.ImmutableList;
import java.util.function.Function;

/**
* Instances of this class are used to find whether there should be argument swaps on a method
* invocation. It's general operation is through using the provided distance function to compute the
* similarity of a parameter name and an argument name. A change is a permutation of the original
* arguments which has the lowest total distance. This is computed as the minimum cost distance
* match on the bi-partite graph mapping parameters to assignable arguments.
*
* @author andrewrice@google.com (Andrew Rice)
*/
@AutoValue
abstract class ArgumentChangeFinder {

/**
* The distance function to use when comparing formal and actual parameters. The function should
* return 0 for highly similar names and larger positive values as names are more different.
*/
abstract Function<ParameterPair, Double> distanceFunction();

/** List of heuristics to apply to eliminate spurious suggestions. */
abstract ImmutableList<Heuristic> heuristics();

static Builder builder() {
return new AutoValue_ArgumentChangeFinder.Builder();
}

@AutoValue.Builder
abstract static class Builder {

/** Set the distance function that {@code ArgumentChangeFinder} should use. */
abstract Builder setDistanceFunction(Function<ParameterPair, Double> distanceFunction);

abstract ImmutableList.Builder<Heuristic> heuristicsBuilder();

/**
* Add the given heuristic to the list to be considered by {@code ArugmentChangeFinder} for
* eliminating spurious findings. Heuristics are applied in order so add more expensive checks
* last.
*/
Builder addHeuristic(Heuristic heuristic) {
heuristicsBuilder().add(heuristic);
return this;
}

abstract ArgumentChangeFinder build();
}

/**
* Find the optimal permutation of arguments for this method invocation or return {@code
* Changes.empty()} if no good permutations were found.
*/
Changes findChanges(InvocationInfo invocationInfo) {
/* Methods with one or fewer parameters cannot possibly have a swap */
if (invocationInfo.formalParameters().size() <= 1) {
return Changes.empty();
}

/* Sometimes we don't have enough actual parameters. This seems to happen sometimes with calls
* to super and javac finds two parameters arg0 and arg1 and no arguments */
if (invocationInfo.actualParameters().size() < invocationInfo.formalParameters().size()) {
return Changes.empty();
}

ImmutableList<Parameter> formals =
Parameter.createListFromVarSymbols(invocationInfo.formalParameters());
ImmutableList<Parameter> actuals =
Parameter.createListFromExpressionTrees(
invocationInfo.actualParameters().subList(0, invocationInfo.formalParameters().size()));

Costs costs = new Costs(formals, actuals);

/* Set the distance between a pair to Inf if not assignable */
costs
.viablePairs()
.filter(ParameterPair::isAlternativePairing)
.filter(p -> !p.actual().isAssignableTo(p.formal(), invocationInfo.state()))
.forEach(p -> costs.invalidatePair(p));

/* If there are no formal parameters which are assignable to any alternative actual parameters
then we can stop without trying to look for permutations */
if (costs.viablePairs().noneMatch(ParameterPair::isAlternativePairing)) {
return Changes.empty();
}

/* Set the lexical distance between pairs */
costs.viablePairs().forEach(p -> costs.updatePair(p, distanceFunction().apply(p)));

Changes changes = costs.computeAssignments();

if (changes.isEmpty()) {
return changes;
}

/* Only keep this change if all of the heuristics match */
for (Heuristic heuristic : heuristics()) {
if (!heuristic.isAcceptableChange(
changes, invocationInfo.tree(), invocationInfo.symbol(), invocationInfo.state())) {
return Changes.empty();
}
}
return changes;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@
import static com.google.errorprone.BugPattern.Category.JDK;
import static com.google.errorprone.BugPattern.SeverityLevel.WARNING;

import com.google.common.collect.ImmutableList;
import com.google.common.annotations.VisibleForTesting;
import com.google.errorprone.BugPattern;
import com.google.errorprone.VisitorState;
import com.google.errorprone.bugpatterns.BugChecker;
Expand All @@ -30,14 +30,9 @@
import com.google.errorprone.names.NamingConventions;
import com.google.errorprone.names.NeedlemanWunschEditDistance;
import com.google.errorprone.util.ASTHelpers;
import com.sun.source.tree.ExpressionTree;
import com.sun.source.tree.MethodInvocationTree;
import com.sun.source.tree.NewClassTree;
import com.sun.source.tree.Tree;
import com.sun.tools.javac.code.Symbol.MethodSymbol;
import com.sun.tools.javac.code.Symbol.VarSymbol;
import com.sun.tools.javac.tree.JCTree;
import java.util.List;
import java.util.function.Function;

/**
Expand Down Expand Up @@ -74,24 +69,23 @@
public class ArgumentSelectionDefectChecker extends BugChecker
implements MethodInvocationTreeMatcher, NewClassTreeMatcher {

private final ImmutableList<Heuristic> heuristics;
private final Function<ParameterPair, Double> nameDistanceFunction;
private final ArgumentChangeFinder argumentchangeFinder;

public ArgumentSelectionDefectChecker() {
this(
buildDefaultDistanceFunction(),
ImmutableList.of(
new LowInformationNameHeuristic(),
new PenaltyThresholdHeuristic(),
new EnclosedByReverseHeuristic(),
new CreatesDuplicateCallHeuristic(),
new NameInCommentHeuristic()));
ArgumentChangeFinder.builder()
.setDistanceFunction(buildDefaultDistanceFunction())
.addHeuristic(new LowInformationNameHeuristic())
.addHeuristic(new PenaltyThresholdHeuristic())
.addHeuristic(new EnclosedByReverseHeuristic())
.addHeuristic(new CreatesDuplicateCallHeuristic())
.addHeuristic(new NameInCommentHeuristic())
.build());
}

public ArgumentSelectionDefectChecker(
Function<ParameterPair, Double> nameDistanceFunction, ImmutableList<Heuristic> heuristics) {
this.nameDistanceFunction = nameDistanceFunction;
this.heuristics = heuristics;
@VisibleForTesting
ArgumentSelectionDefectChecker(ArgumentChangeFinder argumentChangeFinder) {
this.argumentchangeFinder = argumentChangeFinder;
}

@Override
Expand All @@ -100,7 +94,8 @@ public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState
if (symbol == null) {
return Description.NO_MATCH;
}
return visitNewClassOrMethodInvocation(tree, symbol, tree.getArguments(), state);
return visitNewClassOrMethodInvocation(
InvocationInfo.createFromMethodInvocation(tree, symbol, state));
}

@Override
Expand All @@ -109,92 +104,29 @@ public Description matchNewClass(NewClassTree tree, VisitorState state) {
if (symbol == null) {
return Description.NO_MATCH;
}
return visitNewClassOrMethodInvocation(tree, symbol, tree.getArguments(), state);
return visitNewClassOrMethodInvocation(InvocationInfo.createFromNewClass(tree, symbol, state));
}

private Description visitNewClassOrMethodInvocation(
Tree invokedMethodTree,
MethodSymbol invokedMethodSymbol,
List<? extends ExpressionTree> actualParameters,
VisitorState state) {
private Description visitNewClassOrMethodInvocation(InvocationInfo invocationInfo) {

Changes changes = findChanges(invokedMethodTree, invokedMethodSymbol, actualParameters, state);
Changes changes = argumentchangeFinder.findChanges(invocationInfo);

if (changes.isEmpty()) {
return Description.NO_MATCH;
}

// Fix1: permute the arguments as required
SuggestedFix permuteArgumentsFix = buildPermuteArgumentsFix(changes, actualParameters, state);
SuggestedFix permuteArgumentsFix = changes.buildPermuteArgumentsFix(invocationInfo);

// Fix2: apply comments with parameter names to potentially-swapped arguments of the method
SuggestedFix commentArgumentsFix =
buildCommentArgumentsFix(changes, invokedMethodSymbol, actualParameters);
SuggestedFix commentArgumentsFix = changes.buildCommentArgumentsFix(invocationInfo);

return buildDescription(invokedMethodTree)
return buildDescription(invocationInfo.tree())
.addFix(permuteArgumentsFix)
.addFix(commentArgumentsFix)
.build();
}

private Changes findChanges(
Tree invokedMethodTree,
MethodSymbol invokedMethodSymbol,
List<? extends ExpressionTree> actualParameters,
VisitorState state) {

List<VarSymbol> formalParameters = getFormalParametersWithoutVarArgs(invokedMethodSymbol);

/* Methods with one or fewer parameters cannot possibly have a swap */
if (formalParameters.size() <= 1) {
return Changes.empty();
}

/* Sometimes we don't have enough actual parameters. This seems to happen sometimes with calls
* to super and javac finds two parameters arg0 and arg1 and no arguments */
if (actualParameters.size() < formalParameters.size()) {
return Changes.empty();
}

ImmutableList<Parameter> formals = Parameter.createListFromVarSymbols(formalParameters);
ImmutableList<Parameter> actuals =
Parameter.createListFromExpressionTrees(
actualParameters.subList(0, formalParameters.size()));

Costs costs = new Costs(formals, actuals);

/* Set the distance between a pair to Inf if not assignable */
costs
.viablePairs()
.filter(ParameterPair::isAlternativePairing)
.filter(p -> !p.actual().isAssignableTo(p.formal(), state))
.forEach(p -> costs.invalidatePair(p));

/* If there are no formal parameters which are assignable to any alternative actual parameters
then we can stop without trying to look for permutations */
if (costs.viablePairs().noneMatch(ParameterPair::isAlternativePairing)) {
return Changes.empty();
}

/* Set the lexical distance between pairs */
costs.viablePairs().forEach(p -> costs.updatePair(p, nameDistanceFunction.apply(p)));

Changes changes = costs.computeAssignments();

if (changes.isEmpty()) {
return changes;
}

/* Only keep this change if all of the heuristics match */
for (Heuristic heuristic : heuristics) {
if (!heuristic.isAcceptableChange(changes, invokedMethodTree, invokedMethodSymbol, state)) {
return Changes.empty();
}
}

return changes;
}

/**
* Computes the distance between a formal and actual parameter. If either is a null literal then
* the distance is zero (null matches everything). If both have a name then we compute the
Expand Down Expand Up @@ -227,55 +159,4 @@ public Double apply(ParameterPair pair) {
}
};
}

private static List<VarSymbol> getFormalParametersWithoutVarArgs(
MethodSymbol invokedMethodSymbol) {
List<VarSymbol> formalParameters = invokedMethodSymbol.getParameters();

/* javac can get argument names from debugging symbols if they are not available from
other sources. When it does this for an inner class sometimes it returns the implicit this
pointer for the outer class as the first name (but not the first type). If we see this, then
just abort */
if (!formalParameters.isEmpty()
&& formalParameters.get(0).getSimpleName().toString().matches("this\\$[0-9]+")) {
return ImmutableList.of();
}

/* If we have a varargs method then just ignore the final parameter and trailing actual
parameters */
int size =
invokedMethodSymbol.isVarArgs() ? formalParameters.size() - 1 : formalParameters.size();

return formalParameters.subList(0, size);
}

private static SuggestedFix buildCommentArgumentsFix(
Changes changes,
MethodSymbol invokedMethodSymbol,
List<? extends ExpressionTree> actualParameters) {
SuggestedFix.Builder commentArgumentsFixBuilder = SuggestedFix.builder();
List<VarSymbol> formalParameters = getFormalParametersWithoutVarArgs(invokedMethodSymbol);
for (ParameterPair change : changes.changedPairs()) {
int index = change.formal().index();
ExpressionTree actual = actualParameters.get(index);
int startPosition = ((JCTree) actual).getStartPosition();
String formal = formalParameters.get(index).getSimpleName().toString();
commentArgumentsFixBuilder.replace(
startPosition, startPosition, NamedParameterComment.toCommentText(formal));
}
return commentArgumentsFixBuilder.build();
}

private static SuggestedFix buildPermuteArgumentsFix(
Changes changes, List<? extends ExpressionTree> actualParameters, VisitorState state) {
SuggestedFix.Builder permuteArgumentsFixBuilder = SuggestedFix.builder();
for (ParameterPair pair : changes.changedPairs()) {
permuteArgumentsFixBuilder.replace(
actualParameters.get(pair.formal().index()),
// use getSourceForNode to avoid javac pretty printing the replacement (pretty printing
// converts unicode characters to unicode escapes)
state.getSourceForNode(actualParameters.get(pair.actual().index())));
}
return permuteArgumentsFixBuilder.build();
}
}
Loading

0 comments on commit bb3c426

Please sign in to comment.