Skip to content

Commit cdfa8b8

Browse files
java-team-github-botError Prone Team
java-team-github-bot
authored and
Error Prone Team
committed
Add the DistinctVarargs BugChecker. This will generate warning when method expecting distinct varargs is invoked with same variable argument.
PiperOrigin-RevId: 405668886
1 parent 122e512 commit cdfa8b8

File tree

4 files changed

+373
-0
lines changed

4 files changed

+373
-0
lines changed

‎core/src/main/java/com/google/errorprone/bugpatterns/DistinctVarargsChecker.java

+147
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,147 @@
1+
/*
2+
* Copyright 2021 The Error Prone Authors.
3+
*
4+
* Licensed under the Apache License, Version 2.0 (the "License");
5+
* you may not use this file except in compliance with the License.
6+
* You may obtain a copy of the License at
7+
*
8+
* http://www.apache.org/licenses/LICENSE-2.0
9+
*
10+
* Unless required by applicable law or agreed to in writing, software
11+
* distributed under the License is distributed on an "AS IS" BASIS,
12+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
* See the License for the specific language governing permissions and
14+
* limitations under the License.
15+
*/
16+
17+
package com.google.errorprone.bugpatterns;
18+
19+
import static com.google.errorprone.BugPattern.SeverityLevel.WARNING;
20+
import static com.google.errorprone.matchers.Matchers.anyOf;
21+
import static com.google.errorprone.matchers.method.MethodMatchers.staticMethod;
22+
23+
import com.google.errorprone.BugPattern;
24+
import com.google.errorprone.VisitorState;
25+
import com.google.errorprone.bugpatterns.BugChecker.MethodInvocationTreeMatcher;
26+
import com.google.errorprone.fixes.SuggestedFix;
27+
import com.google.errorprone.matchers.Description;
28+
import com.google.errorprone.matchers.Matcher;
29+
import com.google.errorprone.util.ASTHelpers;
30+
import com.sun.source.tree.ExpressionTree;
31+
import com.sun.source.tree.MethodInvocationTree;
32+
import java.util.ArrayList;
33+
import java.util.List;
34+
35+
/**
36+
* ErrorProne checker to generate warning when method expecting distinct varargs is invoked with
37+
* same variable argument.
38+
*/
39+
@BugPattern(
40+
name = "DistinctVarargsChecker",
41+
summary = "Method expects distinct arguments at some/all positions",
42+
severity = WARNING)
43+
public final class DistinctVarargsChecker extends BugChecker
44+
implements MethodInvocationTreeMatcher {
45+
46+
private static final Matcher<ExpressionTree> IMMUTABLE_SET_VARARGS_MATCHER =
47+
anyOf(
48+
staticMethod().onClass("com.google.common.collect.ImmutableSet").named("of"),
49+
staticMethod().onClass("com.google.common.collect.ImmutableSortedSet").named("of"));
50+
private static final Matcher<ExpressionTree> ALL_DISTINCT_ARG_MATCHER =
51+
anyOf(
52+
staticMethod()
53+
.onClass("com.google.common.util.concurrent.Futures")
54+
.withSignature(
55+
"<V>whenAllSucceed(com.google.common.util.concurrent.ListenableFuture<? extends"
56+
+ " V>...)"),
57+
staticMethod()
58+
.onClass("com.google.common.util.concurrent.Futures")
59+
.withSignature(
60+
"<V>whenAllComplete(com.google.common.util.concurrent.ListenableFuture<? extends"
61+
+ " V>...)"),
62+
staticMethod()
63+
.onClass("com.google.common.collect.Ordering")
64+
.withSignature("<T>explicit(T,T...)"));
65+
private static final Matcher<ExpressionTree> EVEN_PARITY_DISTINCT_ARG_MATCHER =
66+
anyOf(
67+
staticMethod().onClass("com.google.common.collect.ImmutableMap").named("of"),
68+
staticMethod().onClass("com.google.common.collect.ImmutableSortedMap").named("of"));
69+
private static final Matcher<ExpressionTree> EVEN_AND_ODD_PARITY_DISTINCT_ARG_MATCHER =
70+
staticMethod().onClass("com.google.common.collect.ImmutableBiMap").named("of");
71+
72+
@Override
73+
public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) {
74+
// For ImmutableSet and ImmutableSortedSet fix can be constructed. For all other methods,
75+
// non-distinct arguments will result in the runtime exceptions.
76+
if (IMMUTABLE_SET_VARARGS_MATCHER.matches(tree, state)) {
77+
return checkDistinctArgumentsWithFix(tree, state);
78+
}
79+
if (ALL_DISTINCT_ARG_MATCHER.matches(tree, state)) {
80+
return checkDistinctArguments(tree, tree.getArguments());
81+
}
82+
if (EVEN_PARITY_DISTINCT_ARG_MATCHER.matches(tree, state)) {
83+
List<ExpressionTree> arguments = new ArrayList<>();
84+
for (int index = 0; index < tree.getArguments().size(); index += 2) {
85+
arguments.add(tree.getArguments().get(index));
86+
}
87+
return checkDistinctArguments(tree, arguments);
88+
}
89+
if (EVEN_AND_ODD_PARITY_DISTINCT_ARG_MATCHER.matches(tree, state)) {
90+
List<ExpressionTree> evenParityArguments = new ArrayList<>();
91+
List<ExpressionTree> oddParityArguments = new ArrayList<>();
92+
for (int index = 0; index < tree.getArguments().size(); index++) {
93+
if (index % 2 == 0) {
94+
evenParityArguments.add(tree.getArguments().get(index));
95+
} else {
96+
oddParityArguments.add(tree.getArguments().get(index));
97+
}
98+
}
99+
return checkDistinctArguments(tree, evenParityArguments, oddParityArguments);
100+
}
101+
return Description.NO_MATCH;
102+
}
103+
104+
private Description checkDistinctArgumentsWithFix(MethodInvocationTree tree, VisitorState state) {
105+
SuggestedFix.Builder suggestedFix = SuggestedFix.builder();
106+
for (int index = 1; index < tree.getArguments().size(); index++) {
107+
boolean isDistinctArgument = true;
108+
for (int prevElementIndex = 0; prevElementIndex < index; prevElementIndex++) {
109+
if (ASTHelpers.sameVariable(
110+
tree.getArguments().get(index), tree.getArguments().get(prevElementIndex))) {
111+
isDistinctArgument = false;
112+
break;
113+
}
114+
}
115+
if (!isDistinctArgument) {
116+
suggestedFix.merge(
117+
SuggestedFix.replace(
118+
state.getEndPosition(tree.getArguments().get(index - 1)),
119+
state.getEndPosition(tree.getArguments().get(index)),
120+
""));
121+
}
122+
}
123+
if (suggestedFix.isEmpty()) {
124+
return Description.NO_MATCH;
125+
}
126+
return describeMatch(tree, suggestedFix.build());
127+
}
128+
129+
private Description checkDistinctArguments(
130+
MethodInvocationTree tree, List<? extends ExpressionTree>... argumentsList) {
131+
for (List<? extends ExpressionTree> arguments : argumentsList) {
132+
for (int firstArgumentIndex = 0;
133+
firstArgumentIndex < arguments.size();
134+
firstArgumentIndex++) {
135+
for (int secondArgumentIndex = firstArgumentIndex + 1;
136+
secondArgumentIndex < arguments.size();
137+
secondArgumentIndex++) {
138+
if (ASTHelpers.sameVariable(
139+
arguments.get(firstArgumentIndex), arguments.get(secondArgumentIndex))) {
140+
return describeMatch(tree);
141+
}
142+
}
143+
}
144+
}
145+
return Description.NO_MATCH;
146+
}
147+
}

‎core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java

+2
Original file line numberDiff line numberDiff line change
@@ -97,6 +97,7 @@
9797
import com.google.errorprone.bugpatterns.DescribeMatch;
9898
import com.google.errorprone.bugpatterns.DifferentNameButSame;
9999
import com.google.errorprone.bugpatterns.DiscardedPostfixExpression;
100+
import com.google.errorprone.bugpatterns.DistinctVarargsChecker;
100101
import com.google.errorprone.bugpatterns.DivZero;
101102
import com.google.errorprone.bugpatterns.DoNotCallChecker;
102103
import com.google.errorprone.bugpatterns.DoNotCallSuggester;
@@ -788,6 +789,7 @@ public static ScannerSupplier errorChecks() {
788789
DefaultCharset.class,
789790
DefaultPackage.class,
790791
DeprecatedVariable.class,
792+
DistinctVarargsChecker.class,
791793
DoNotCallSuggester.class,
792794
DoNotClaimAnnotations.class,
793795
DoNotMockAutoValue.class,

‎core/src/test/java/com/google/errorprone/bugpatterns/DistinctVarargsCheckerTest.java

+206
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,206 @@
1+
/*
2+
* Copyright 2021 The Error Prone Authors.
3+
*
4+
* Licensed under the Apache License, Version 2.0 (the "License");
5+
* you may not use this file except in compliance with the License.
6+
* You may obtain a copy of the License at
7+
*
8+
* http://www.apache.org/licenses/LICENSE-2.0
9+
*
10+
* Unless required by applicable law or agreed to in writing, software
11+
* distributed under the License is distributed on an "AS IS" BASIS,
12+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
* See the License for the specific language governing permissions and
14+
* limitations under the License.
15+
*/
16+
17+
package com.google.errorprone.bugpatterns;
18+
19+
import com.google.errorprone.BugCheckerRefactoringTestHelper;
20+
import com.google.errorprone.CompilationTestHelper;
21+
import org.junit.Test;
22+
import org.junit.runner.RunWith;
23+
import org.junit.runners.JUnit4;
24+
25+
/** {@link DistinctVarargsChecker}Test */
26+
@RunWith(JUnit4.class)
27+
public class DistinctVarargsCheckerTest {
28+
29+
private final CompilationTestHelper compilationHelper =
30+
CompilationTestHelper.newInstance(DistinctVarargsChecker.class, getClass());
31+
private final BugCheckerRefactoringTestHelper refactoringHelper =
32+
BugCheckerRefactoringTestHelper.newInstance(DistinctVarargsChecker.class, getClass());
33+
34+
@Test
35+
public void distinctVarargsChecker_sameVariableInFuturesVaragsMethods_shouldFlag() {
36+
compilationHelper
37+
.addSourceLines(
38+
"Test.java",
39+
"import com.google.common.util.concurrent.Futures;",
40+
"import com.google.common.util.concurrent.ListenableFuture;",
41+
"public class Test {",
42+
" void testFunction() {",
43+
" ListenableFuture firstFuture = null, secondFuture = null;",
44+
" // BUG: Diagnostic contains: DistinctVarargsChecker",
45+
" Futures.whenAllSucceed(firstFuture, firstFuture);",
46+
" // BUG: Diagnostic contains: DistinctVarargsChecker",
47+
" Futures.whenAllSucceed(firstFuture, firstFuture, secondFuture);",
48+
" // BUG: Diagnostic contains: DistinctVarargsChecker",
49+
" Futures.whenAllComplete(firstFuture, firstFuture);",
50+
" // BUG: Diagnostic contains: DistinctVarargsChecker",
51+
" Futures.whenAllComplete(firstFuture, firstFuture, secondFuture);",
52+
" }",
53+
"}")
54+
.doTest();
55+
}
56+
57+
@Test
58+
public void distinctVarargsCheckerdifferentVariableInFuturesVaragsMethods_shouldNotFlag() {
59+
compilationHelper
60+
.addSourceLines(
61+
"Test.java",
62+
"import com.google.common.util.concurrent.Futures;",
63+
"import com.google.common.util.concurrent.ListenableFuture;",
64+
"public class Test {",
65+
" void testFunction() {",
66+
" ListenableFuture firstFuture = null, secondFuture = null;",
67+
" Futures.whenAllComplete(firstFuture);",
68+
" Futures.whenAllSucceed(firstFuture, secondFuture);",
69+
" Futures.whenAllComplete(firstFuture);",
70+
" Futures.whenAllComplete(firstFuture, secondFuture);",
71+
" }",
72+
"}")
73+
.doTest();
74+
}
75+
76+
@Test
77+
public void distinctVarargsChecker_sameVariableInGuavaVarargMethods_shouldFlag() {
78+
compilationHelper
79+
.addSourceLines(
80+
"Test.java",
81+
"import com.google.common.collect.Ordering;",
82+
"import com.google.common.collect.ImmutableBiMap;",
83+
"import com.google.common.collect.ImmutableMap;",
84+
"import com.google.common.collect.ImmutableSortedMap;",
85+
"import com.google.common.collect.ImmutableSet;",
86+
"import com.google.common.collect.ImmutableSortedSet;",
87+
"public class Test {",
88+
" void testFunction() {",
89+
" int first = 1, second = 2;",
90+
" // BUG: Diagnostic contains: DistinctVarargsChecker",
91+
" Ordering.explicit(first, first);",
92+
" // BUG: Diagnostic contains: DistinctVarargsChecker",
93+
" Ordering.explicit(first, first, second);",
94+
" // BUG: Diagnostic contains: DistinctVarargsChecker",
95+
" ImmutableMap.of(first, second, first, second);",
96+
" // BUG: Diagnostic contains: DistinctVarargsChecker",
97+
" ImmutableSortedMap.of(first, second, first, second);",
98+
" // BUG: Diagnostic contains: DistinctVarargsChecker",
99+
" ImmutableBiMap.of(first, second, first, second);",
100+
" // BUG: Diagnostic contains: DistinctVarargsChecker",
101+
" ImmutableBiMap.of(first, second, second, second);",
102+
" // BUG: Diagnostic contains: DistinctVarargsChecker",
103+
" ImmutableSet.of(first, first);",
104+
" // BUG: Diagnostic contains: DistinctVarargsChecker",
105+
" ImmutableSet.of(first, first, second);",
106+
" // BUG: Diagnostic contains: DistinctVarargsChecker",
107+
" ImmutableSortedSet.of(first, first);",
108+
" // BUG: Diagnostic contains: DistinctVarargsChecker",
109+
" ImmutableSortedSet.of(first, first, second);",
110+
" }",
111+
"}")
112+
.doTest();
113+
}
114+
115+
@Test
116+
public void distinctVarargsChecker_differentVariableInGuavaVarargMethods_shouldNotFlag() {
117+
compilationHelper
118+
.addSourceLines(
119+
"Test.java",
120+
"import com.google.common.collect.Ordering;",
121+
"import com.google.common.collect.ImmutableBiMap;",
122+
"import com.google.common.collect.ImmutableMap;",
123+
"import com.google.common.collect.ImmutableSortedMap;",
124+
"import com.google.common.collect.ImmutableSet;",
125+
"import com.google.common.collect.ImmutableSortedSet;",
126+
"public class Test {",
127+
" void testFunction() {",
128+
" int first = 1, second = 2, third = 3, fourth = 4;",
129+
" Ordering.explicit(first);",
130+
" Ordering.explicit(first, second);",
131+
" ImmutableMap.of(first, second);",
132+
" ImmutableSortedMap.of(first, second);",
133+
" ImmutableBiMap.of(first, second, third, fourth);",
134+
" ImmutableSet.of(first);",
135+
" ImmutableSet.of(first, second);",
136+
" ImmutableSortedSet.of(first);",
137+
" ImmutableSortedSet.of(first, second);",
138+
" }",
139+
"}")
140+
.doTest();
141+
}
142+
143+
@Test
144+
public void distinctVarargsChecker_sameVariableInImmutableSetVarargsMethod_shouldRefactor() {
145+
refactoringHelper
146+
.addInputLines(
147+
"Test.java",
148+
"import com.google.common.collect.ImmutableSet;",
149+
"import com.google.common.collect.ImmutableSortedSet;",
150+
"public class Test {",
151+
" void testFunction() {",
152+
" int first = 1, second = 2;",
153+
" ImmutableSet.of(first, first);",
154+
" ImmutableSet.of(first, first, second);",
155+
" ImmutableSortedSet.of(first, first);",
156+
" ImmutableSortedSet.of(first, first, second);",
157+
" }",
158+
"}")
159+
.addOutputLines(
160+
"Test.java",
161+
"import com.google.common.collect.ImmutableSet;",
162+
"import com.google.common.collect.ImmutableSortedSet;",
163+
"public class Test {",
164+
" void testFunction() {",
165+
" int first = 1, second = 2;",
166+
" ImmutableSet.of(first);",
167+
" ImmutableSet.of(first, second);",
168+
" ImmutableSortedSet.of(first);",
169+
" ImmutableSortedSet.of(first, second);",
170+
" }",
171+
"}")
172+
.doTest();
173+
}
174+
175+
@Test
176+
public void distinctVarargsChecker_differentVarsInImmutableSetVarargsMethod_shouldNotRefactor() {
177+
refactoringHelper
178+
.addInputLines(
179+
"Test.java",
180+
"import com.google.common.collect.ImmutableSet;",
181+
"import com.google.common.collect.ImmutableSortedSet;",
182+
"public class Test {",
183+
" void testFunction() {",
184+
" int first = 1, second = 2;",
185+
" ImmutableSet.of(first);",
186+
" ImmutableSet.of(first, second);",
187+
" ImmutableSortedSet.of(first);",
188+
" ImmutableSortedSet.of(first, second);",
189+
" }",
190+
"}")
191+
.addOutputLines(
192+
"Test.java",
193+
"import com.google.common.collect.ImmutableSet;",
194+
"import com.google.common.collect.ImmutableSortedSet;",
195+
"public class Test {",
196+
" void testFunction() {",
197+
" int first = 1, second = 2;",
198+
" ImmutableSet.of(first);",
199+
" ImmutableSet.of(first, second);",
200+
" ImmutableSortedSet.of(first);",
201+
" ImmutableSortedSet.of(first, second);",
202+
" }",
203+
"}")
204+
.doTest();
205+
}
206+
}

‎docs/bugpattern/DistinctVarargsChecker.md

+18
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,18 @@
1+
Various methods which take variable-length arguments throw the runtime
2+
exceptions like `IllegalArgumentException` when the arguments are not distinct.
3+
4+
This checker warns on using the non-distinct parameters in various varargs
5+
method when the usage is redundant or will either result in the runtime
6+
exception.
7+
8+
Bad:
9+
10+
```java
11+
ImmutableSet.of(first, second, second, third);
12+
```
13+
14+
Good:
15+
16+
```java
17+
ImmutableSet.of(first, second, third);
18+
```

0 commit comments

Comments
 (0)