Skip to content

Commit

Permalink
Fix quadratic behaviour in DisctinctVarargsChecker
Browse files Browse the repository at this point in the history 
Do a less sophisticated comparison of source code for distinct entries, which
allows us to do set operations instead of calling `ASTHelpers.sameVariable`
`n^2` times.

PiperOrigin-RevId: 464876981
  • Loading branch information
@cushon
cushon authored and Error Prone Team committed Aug 2, 2022
1 parent e2309dc commit a398a3f
Show file tree
Hide file tree
Showing 2 changed files with 58 additions and 32 deletions.
66 changes: 34 additions & 32 deletions core/src/main/java/com/google/errorprone/bugpatterns/DistinctVarargsChecker.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,17 +20,19 @@
import static com.google.errorprone.matchers.Matchers.anyOf;
import static com.google.errorprone.matchers.method.MethodMatchers.staticMethod;

import com.google.common.collect.ImmutableListMultimap;
import com.google.errorprone.BugPattern;
import com.google.errorprone.VisitorState;
import com.google.errorprone.bugpatterns.BugChecker.MethodInvocationTreeMatcher;
import com.google.errorprone.fixes.SuggestedFix;
import com.google.errorprone.matchers.Description;
import com.google.errorprone.matchers.Matcher;
import com.google.errorprone.util.ASTHelpers;
import com.sun.source.tree.ExpressionTree;
import com.sun.source.tree.MethodInvocationTree;
import java.util.ArrayList;
import java.util.Collection;
import java.util.List;
import java.util.Map;

/**
* ErrorProne checker to generate warning when method expecting distinct varargs is invoked with
Expand Down Expand Up @@ -72,14 +74,14 @@ public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState
return checkDistinctArgumentsWithFix(tree, state);
}
if (ALL_DISTINCT_ARG_MATCHER.matches(tree, state)) {
return checkDistinctArguments(tree, tree.getArguments());
return checkDistinctArguments(state, tree.getArguments());
}
if (EVEN_PARITY_DISTINCT_ARG_MATCHER.matches(tree, state)) {
List<ExpressionTree> arguments = new ArrayList<>();
for (int index = 0; index < tree.getArguments().size(); index += 2) {
arguments.add(tree.getArguments().get(index));
}
return checkDistinctArguments(tree, arguments);
return checkDistinctArguments(state, arguments);
}
if (EVEN_AND_ODD_PARITY_DISTINCT_ARG_MATCHER.matches(tree, state)) {
List<ExpressionTree> evenParityArguments = new ArrayList<>();
Expand All @@ -91,29 +93,34 @@ public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState
oddParityArguments.add(tree.getArguments().get(index));
}
}
return checkDistinctArguments(tree, evenParityArguments, oddParityArguments);
return checkDistinctArguments(state, evenParityArguments, oddParityArguments);
}
return Description.NO_MATCH;
}

private static ImmutableListMultimap<String, Integer> argumentsByString(
VisitorState state, List<? extends ExpressionTree> arguments) {
ImmutableListMultimap.Builder<String, Integer> result = ImmutableListMultimap.builder();
for (int i = 0; i < arguments.size(); i++) {
result.put(state.getSourceForNode(arguments.get(i)), i);
}
return result.build();
}

private Description checkDistinctArgumentsWithFix(MethodInvocationTree tree, VisitorState state) {
SuggestedFix.Builder suggestedFix = SuggestedFix.builder();
for (int index = 1; index < tree.getArguments().size(); index++) {
boolean isDistinctArgument = true;
for (int prevElementIndex = 0; prevElementIndex < index; prevElementIndex++) {
if (ASTHelpers.sameVariable(
tree.getArguments().get(index), tree.getArguments().get(prevElementIndex))) {
isDistinctArgument = false;
break;
}
}
if (!isDistinctArgument) {
suggestedFix.merge(
SuggestedFix.replace(
state.getEndPosition(tree.getArguments().get(index - 1)),
state.getEndPosition(tree.getArguments().get(index)),
""));
}
List<? extends ExpressionTree> arguments = tree.getArguments();
ImmutableListMultimap<String, Integer> argumentsByString = argumentsByString(state, arguments);
for (Map.Entry<String, Collection<Integer>> entry : argumentsByString.asMap().entrySet()) {
entry.getValue().stream()
.skip(1)
.forEachOrdered(
index ->
suggestedFix.merge(
SuggestedFix.replace(
state.getEndPosition(arguments.get(index - 1)),
state.getEndPosition(arguments.get(index)),
"")));
}
if (suggestedFix.isEmpty()) {
return Description.NO_MATCH;
Expand All @@ -122,19 +129,14 @@ private Description checkDistinctArgumentsWithFix(MethodInvocationTree tree, Vis
}

private Description checkDistinctArguments(
MethodInvocationTree tree, List<? extends ExpressionTree>... argumentsList) {
VisitorState state, List<? extends ExpressionTree>... argumentsList) {
for (List<? extends ExpressionTree> arguments : argumentsList) {
for (int firstArgumentIndex = 0;
firstArgumentIndex < arguments.size();
firstArgumentIndex++) {
for (int secondArgumentIndex = firstArgumentIndex + 1;
secondArgumentIndex < arguments.size();
secondArgumentIndex++) {
if (ASTHelpers.sameVariable(
arguments.get(firstArgumentIndex), arguments.get(secondArgumentIndex))) {
return describeMatch(tree);
}
}
ImmutableListMultimap<String, Integer> argumentsByString =
argumentsByString(state, arguments);
for (Map.Entry<String, Collection<Integer>> entry : argumentsByString.asMap().entrySet()) {
entry.getValue().stream()
.skip(1)
.forEachOrdered(index -> state.reportMatch(describeMatch(arguments.get(index))));
}
}
return Description.NO_MATCH;
Expand Down
24 changes: 24 additions & 0 deletions core/src/test/java/com/google/errorprone/bugpatterns/DistinctVarargsCheckerTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,11 @@

package com.google.errorprone.bugpatterns;

import static java.util.stream.Collectors.joining;

import com.google.errorprone.BugCheckerRefactoringTestHelper;
import com.google.errorprone.CompilationTestHelper;
import java.util.stream.IntStream;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.runners.JUnit4;
Expand Down Expand Up @@ -195,4 +198,25 @@ public void distinctVarargsChecker_differentVarsInImmutableSetVarargsMethod_shou
"}")
.doTest();
}

@Test
public void negative_quadratic() {

String large =
IntStream.range(0, 7000)
.mapToObj(x -> String.format("\"%s\"", x))
.collect(joining(", ", "ImmutableSet.of(", ", \"0\");"));

compilationHelper
.addSourceLines(
"Test.java",
"import com.google.common.collect.ImmutableSet;",
"public class Test {",
" void testFunction() {",
" // BUG: Diagnostic contains: DistinctVarargsChecker",
large,
" }",
"}")
.doTest();
}
}

0 comments on commit a398a3f

Please sign in to comment.