Skip to content

Commit fd11d2b

Browse files
Stephan202Stephan202
authored
Extend ExplicitArgumentEnumeration beyond unary method invocations (#2049)
While there, slightly improve the heuristics that determine whether a suitable varargs overload is likely to exist and be selected upon unwrapping an explicitly enumerated `Iterable`. This change generalizes a subset of the `Immutable{List,Set}MultimapBuilderPut` Refaster rules.
1 parent 8f90e2b commit fd11d2b

File tree

8 files changed

+148
-56
lines changed

8 files changed

+148
-56
lines changed

‎error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/ExplicitArgumentEnumeration.java‎

Lines changed: 69 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,7 @@
3535
import com.sun.tools.javac.code.Symbol.MethodSymbol;
3636
import com.sun.tools.javac.code.Symbol.VarSymbol;
3737
import com.sun.tools.javac.code.Type;
38+
import com.sun.tools.javac.code.Type.WildcardType;
3839
import com.sun.tools.javac.code.Types;
3940
import java.util.Arrays;
4041
import java.util.List;
@@ -128,37 +129,37 @@ public ExplicitArgumentEnumeration() {}
128129

129130
@Override
130131
public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) {
131-
if (tree.getArguments().size() != 1) {
132-
/* Performance optimization: non-unary method invocations cannot be simplified. */
133-
return Description.NO_MATCH;
134-
}
135-
136132
MethodSymbol method = ASTHelpers.getSymbol(tree);
137-
if (!isUnaryIterableAcceptingMethod(method, state) || isLocalOverload(method, state)) {
133+
if (!acceptsIterableAsFinalArg(method, state) || isLocalOverload(method, state)) {
138134
/*
139135
* This isn't a method invocation we can simplify, or it's an invocation of a local overload.
140136
* The latter type of invocation we do not suggest replacing, as this is fairly likely to
141137
* introduce an unbounded recursive call chain.
138+
*
139+
* NB: Note that strictly speaking we don't need to check whether the invoked method's final
140+
* parameter is `Iterable`, as the `EXPLICIT_ITERABLE_CREATOR` matcher employed below will
141+
* only match `Iterable` arguments. However, this check serves as a cheap early filter.
142142
*/
143143
return Description.NO_MATCH;
144144
}
145145

146-
ExpressionTree argument = tree.getArguments().getFirst();
147-
if (!EXPLICIT_ITERABLE_CREATOR.matches(argument, state)) {
146+
ExpressionTree argument = tree.getArguments().getLast();
147+
if (!(argument instanceof MethodInvocationTree methodInvocation)
148+
|| !EXPLICIT_ITERABLE_CREATOR.matches(argument, state)) {
148149
return Description.NO_MATCH;
149150
}
150151

151-
return trySuggestCallingVarargsOverload(method, (MethodInvocationTree) argument, state)
152-
.or(() -> trySuggestCallingCustomAlternative(tree, (MethodInvocationTree) argument, state))
152+
return trySuggestCallingVarargsOverload(method, methodInvocation, state)
153+
.or(() -> trySuggestCallingCustomAlternative(tree, methodInvocation, state))
153154
.map(fix -> describeMatch(tree, fix))
154155
.orElse(Description.NO_MATCH);
155156
}
156157

157-
private static boolean isUnaryIterableAcceptingMethod(MethodSymbol method, VisitorState state) {
158+
private static boolean acceptsIterableAsFinalArg(MethodSymbol method, VisitorState state) {
158159
List<VarSymbol> params = method.params();
159160
return !method.isVarArgs()
160-
&& params.size() == 1
161-
&& ASTHelpers.isSubtype(params.getFirst().type, state.getSymtab().iterableType, state);
161+
&& !params.isEmpty()
162+
&& ASTHelpers.isSubtype(params.getLast().type, state.getSymtab().iterableType, state);
162163
}
163164

164165
private static boolean isLocalOverload(MethodSymbol calledMethod, VisitorState state) {
@@ -192,25 +193,35 @@ private static Optional<SuggestedFix> trySuggestCallingVarargsOverload(
192193
}
193194

194195
/**
195-
* Tells whether it is likely that, if the argument to the given method is unwrapped, a suitable
196-
* varargs overload will be invoked instead.
196+
* Tells whether it is likely that, if the final argument to the given method is unwrapped, a
197+
* suitable varargs overload will be invoked instead.
197198
*
198-
* <p>If all overloads have a single parameter, and at least one of them is a suitably-typed
199-
* varargs method, then we assume that unwrapping the iterable argument will cause a suitable
200-
* overload to be invoked. (Note that there may be multiple varargs overloads due to method
201-
* overriding; this check does not attempt to determine which exact method or overload will be
202-
* invoked as a result of the suggested simplification.)
199+
* <p>If none of the overloads has a larger number of parameters than the given method, and at
200+
* least one of them is a suitably-typed varargs method, then we assume that unwrapping the
201+
* iterable argument will cause a suitable overload to be invoked. (Note that there may be
202+
* multiple varargs overloads due to method overriding; this check does not attempt to determine
203+
* which exact method or overload will be invoked as a result of the suggested simplification.)
203204
*
204205
* <p>Note that this is a (highly!) imperfect heuristic, but it is sufficient to prevent e.g.
205206
* unwrapping of arguments to `org.jooq.impl.DSL#row`, which can cause the expression's return
206207
* type to change from `RowN` to (e.g.) `Row2`.
207208
*/
208209
// XXX: There are certainly cases where it _would_ be nice to unwrap the arguments to
209210
// `org.jooq.impl.DSL#row(Collection<?>)`. Look into this.
210-
// XXX: Ideally we validate that eligible overloads have compatible return types.
211211
private static boolean hasLikelySuitableVarargsOverload(
212212
MethodSymbol method, ImmutableList<MethodSymbol> overloads, VisitorState state) {
213-
Types types = state.getTypes();
213+
List<VarSymbol> parameters = method.getParameters();
214+
if (overloads.stream().anyMatch(m -> m.params().size() > parameters.size())) {
215+
/* Some overloads accept more parameters; back off, as this is an unusual setup. */
216+
return false;
217+
}
218+
219+
ImmutableList<Type> initialParameterTypes =
220+
parameters.stream()
221+
.limit(parameters.size() - 1)
222+
.map(p -> p.type)
223+
.collect(toImmutableList());
224+
214225
// XXX: This logic is fragile, as it assumes that the method parameter's type is of the form
215226
// `X<T>`, where `T` is the type of the explicitly enumerated values passed to the expression to
216227
// be unwrapped. This should generally hold, given the types returned by the
@@ -219,15 +230,43 @@ private static boolean hasLikelySuitableVarargsOverload(
219230
// ArrayList<String>` or a `ListMap<K, V>` defined as `abstract ListMap<K, V> extends
220231
// AbstractMap<K, V> implements List<V>`. Raw types are handled by assuming `Object` as the
221232
// parameter type.
222-
Type parameterType =
233+
Type finalParameterType =
223234
Iterables.getOnlyElement(
224-
Iterables.getOnlyElement(method.getParameters()).type.getTypeArguments(),
225-
state.getSymtab().objectType);
226-
return overloads.stream().allMatch(m -> m.params().size() == 1)
227-
&& overloads.stream()
228-
.filter(MethodSymbol::isVarArgs)
229-
.map(m -> types.elemtype(Iterables.getOnlyElement(m.getParameters()).type))
230-
.anyMatch(varArgsType -> types.containsType(parameterType, varArgsType));
235+
parameters.getLast().type.getTypeArguments(), state.getSymtab().objectType);
236+
return overloads.stream()
237+
.anyMatch(
238+
m ->
239+
isCompatibleVarargsOverload(
240+
m, method.getReturnType(), initialParameterTypes, finalParameterType, state));
241+
}
242+
243+
private static boolean isCompatibleVarargsOverload(
244+
MethodSymbol overload,
245+
Type returnType,
246+
ImmutableList<Type> initialParameterTypes,
247+
Type iterableElementType,
248+
VisitorState state) {
249+
Types types = state.getTypes();
250+
251+
List<VarSymbol> parameters = overload.getParameters();
252+
if (!overload.isVarArgs()
253+
|| parameters.size() != initialParameterTypes.size() + 1
254+
|| !types.isSubtype(overload.getReturnType(), returnType)) {
255+
return false;
256+
}
257+
258+
for (int i = 0; i < initialParameterTypes.size(); i++) {
259+
if (!types.isSubtype(initialParameterTypes.get(i), parameters.get(i).type)) {
260+
return false;
261+
}
262+
}
263+
264+
Type actualVarargsType = types.elemtype(parameters.getLast().type);
265+
return switch (iterableElementType) {
266+
case WildcardType wildcard when wildcard.isExtendsBound() ->
267+
types.isSubtype(wildcard.getExtendsBound(), actualVarargsType);
268+
default -> types.isSubtype(iterableElementType, actualVarargsType);
269+
};
231270
}
232271

233272
private static Optional<SuggestedFix> trySuggestCallingCustomAlternative(

‎error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableListMultimapRules.java‎

Lines changed: 1 addition & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,6 @@
44
import static com.google.common.collect.ImmutableListMultimap.toImmutableListMultimap;
55
import static com.google.errorprone.refaster.ImportPolicy.STATIC_IMPORT_ALWAYS;
66

7-
import com.google.common.collect.ImmutableList;
87
import com.google.common.collect.ImmutableListMultimap;
98
import com.google.common.collect.ImmutableMultimap;
109
import com.google.common.collect.ListMultimap;
@@ -287,12 +286,7 @@ static final class ImmutableListMultimapBuilderPut<K, V> {
287286
@SuppressWarnings("unchecked" /* Safe generic array type creation. */)
288287
ImmutableListMultimap.Builder<K, V> before(
289288
ImmutableListMultimap.Builder<K, V> builder, K key, V value) {
290-
// XXX: Drop the `ImmutableList` case in favour of generalizing the
291-
// `ExplicitArgumentEnumeration` check, or add variants for other collection types as well.
292-
return Refaster.anyOf(
293-
builder.put(Map.entry(key, value)),
294-
builder.putAll(key, value),
295-
builder.putAll(key, ImmutableList.of(value)));
289+
return Refaster.anyOf(builder.put(Map.entry(key, value)), builder.putAll(key, value));
296290
}
297291

298292
@AfterTemplate

‎error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ImmutableSetMultimapRules.java‎

Lines changed: 1 addition & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,6 @@
44
import static com.google.common.collect.ImmutableSetMultimap.toImmutableSetMultimap;
55
import static com.google.errorprone.refaster.ImportPolicy.STATIC_IMPORT_ALWAYS;
66

7-
import com.google.common.collect.ImmutableSet;
87
import com.google.common.collect.ImmutableSetMultimap;
98
import com.google.common.collect.ListMultimap;
109
import com.google.common.collect.Multimap;
@@ -227,12 +226,7 @@ static final class ImmutableSetMultimapBuilderPut<K, V> {
227226
@SuppressWarnings("unchecked" /* Safe generic array type creation. */)
228227
ImmutableSetMultimap.Builder<K, V> before(
229228
ImmutableSetMultimap.Builder<K, V> builder, K key, V value) {
230-
// XXX: Drop the `ImmutableSet` case in favour of generalizing the
231-
// `ExplicitArgumentEnumeration` check, or add variants for other collection types as well.
232-
return Refaster.anyOf(
233-
builder.put(Map.entry(key, value)),
234-
builder.putAll(key, value),
235-
builder.putAll(key, ImmutableSet.of(value)));
229+
return Refaster.anyOf(builder.put(Map.entry(key, value)), builder.putAll(key, value));
236230
}
237231

238232
@AfterTemplate

‎error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/ExplicitArgumentEnumerationTest.java‎

Lines changed: 73 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,9 @@ void identification() {
1717
"import static org.assertj.core.api.Assertions.assertThat;",
1818
"",
1919
"import com.google.common.collect.ImmutableList;",
20+
"import com.google.common.collect.ImmutableListMultimap;",
21+
"import com.google.common.collect.ImmutableSet;",
22+
"import com.google.common.collect.ImmutableSetMultimap;",
2023
"import com.google.errorprone.CompilationTestHelper;",
2124
"import com.google.errorprone.bugpatterns.BugChecker;",
2225
"import io.micrometer.core.instrument.Counter;",
@@ -38,8 +41,16 @@ void identification() {
3841
"",
3942
" // BUG: Diagnostic contains:",
4043
" unaryMethod(ImmutableList.of(1, 2));",
44+
" unaryMethod(1, 2);",
4145
" unaryMethodWithLessVisibleOverload(ImmutableList.of(1, 2));",
42-
" binaryMethod(ImmutableList.of(1, 2), 3);",
46+
" unaryMethodWithIncompatibleReturnType(ImmutableList.of(1, 2));",
47+
" unaryMethodWithExtendsIncompatibleVarargsOverload(ImmutableList.of(1, 2));",
48+
" unaryMethodWithSuperIncompatibleVarargsOverload(ImmutableList.of(1, 2));",
49+
" // BUG: Diagnostic contains:",
50+
" binaryMethod(0, ImmutableList.of(1, 2));",
51+
" binaryMethod(0, 1, 2);",
52+
" binaryMethodWithIncompatibleOverload(0, ImmutableList.of(1, 2));",
53+
" binaryMethodWithUnaryOverload(new Integer[0], ImmutableList.of(2, 3));",
4354
" // BUG: Diagnostic contains:",
4455
" rawListMethod(ImmutableList.of(1, 2));",
4556
" rawListMethodWithTypeRestrictedOverload(ImmutableList.of(1, 2));",
@@ -71,6 +82,13 @@ void identification() {
7182
" // BUG: Diagnostic contains:",
7283
" .setArgs(ImmutableList.of(\"foo\"))",
7384
" .withClasspath();",
85+
"",
86+
" ImmutableListMultimap.builder()",
87+
" // BUG: Diagnostic contains:",
88+
" .putAll(1, ImmutableList.of(2));",
89+
" ImmutableSetMultimap.<String, Integer>builder()",
90+
" // BUG: Diagnostic contains:",
91+
" .putAll(\"foo\", ImmutableSet.of(1, 2));",
7492
" }",
7593
"",
7694
" private int unaryMethod(ImmutableList<Integer> args) {",
@@ -87,10 +105,51 @@ void identification() {
87105
" unaryMethodWithLessVisibleOverload(ImmutableList.copyOf(args));",
88106
" }",
89107
"",
90-
" private void binaryMethod(ImmutableList<Integer> args, int extraArg) {}",
108+
" private Integer unaryMethodWithIncompatibleReturnType(ImmutableList<Integer> args) {",
109+
" return 0;",
110+
" }",
111+
"",
112+
" private Object unaryMethodWithIncompatibleReturnType(Integer... args) {",
113+
" return unaryMethodWithIncompatibleReturnType(ImmutableList.copyOf(args));",
114+
" }",
115+
"",
116+
" private int unaryMethodWithExtendsIncompatibleVarargsOverload(",
117+
" ImmutableList<? extends Number> args) {",
118+
" return 0;",
119+
" }",
120+
"",
121+
" private int unaryMethodWithExtendsIncompatibleVarargsOverload(Integer... args) {",
122+
" return unaryMethodWithExtendsIncompatibleVarargsOverload(ImmutableList.copyOf(args));",
123+
" }",
124+
"",
125+
" private int unaryMethodWithSuperIncompatibleVarargsOverload(ImmutableList<? super Integer> args) {",
126+
" return 0;",
127+
" }",
128+
"",
129+
" private int unaryMethodWithSuperIncompatibleVarargsOverload(Integer... args) {",
130+
" return unaryMethodWithSuperIncompatibleVarargsOverload(ImmutableList.copyOf(args));",
131+
" }",
132+
"",
133+
" private int binaryMethod(Integer extraArg, ImmutableList<Integer> args) {",
134+
" return 0;",
135+
" }",
136+
"",
137+
" private int binaryMethod(Object extraArg, Integer... args) {",
138+
" return binaryMethod(0, ImmutableList.copyOf(args));",
139+
" }",
140+
"",
141+
" private int binaryMethodWithIncompatibleOverload(Object extraArg, ImmutableList<Integer> args) {",
142+
" return 0;",
143+
" }",
144+
"",
145+
" private int binaryMethodWithIncompatibleOverload(Integer extraArg, Integer... args) {",
146+
" return binaryMethodWithIncompatibleOverload(0, ImmutableList.copyOf(args));",
147+
" }",
148+
"",
149+
" private void binaryMethodWithUnaryOverload(Integer[] extraArg, ImmutableList<Integer> args) {}",
91150
"",
92-
" private void binaryMethod(Integer... args) {",
93-
" binaryMethod(ImmutableList.copyOf(args), 0);",
151+
" private void binaryMethodWithUnaryOverload(Integer... args) {",
152+
" binaryMethodWithUnaryOverload(args, ImmutableList.copyOf(args));",
94153
" }",
95154
"",
96155
" private void rawListMethod(ImmutableList args) {}",
@@ -133,8 +192,10 @@ void replacement() {
133192
"import static org.assertj.core.api.Assertions.assertThat;",
134193
"",
135194
"import com.google.common.collect.ImmutableList;",
195+
"import com.google.common.collect.ImmutableListMultimap;",
136196
"import com.google.common.collect.ImmutableMultiset;",
137197
"import com.google.common.collect.ImmutableSet;",
198+
"import com.google.common.collect.ImmutableSetMultimap;",
138199
"import com.google.errorprone.BugCheckerRefactoringTestHelper;",
139200
"import com.google.errorprone.CompilationTestHelper;",
140201
"import com.google.errorprone.bugpatterns.BugChecker;",
@@ -171,15 +232,20 @@ void replacement() {
171232
" .setArgs(ImmutableList.of(\"foo\", \"bar\"));",
172233
" CompilationTestHelper.newInstance(BugChecker.class, getClass())",
173234
" .setArgs(ImmutableList.of(\"foo\", \"bar\"));",
235+
"",
236+
" ImmutableListMultimap.builder().putAll(1, ImmutableList.of(2));",
237+
" ImmutableSetMultimap.<String, Integer>builder().putAll(\"foo\", ImmutableSet.of(2));",
174238
" }",
175239
"}")
176240
.addOutputLines(
177241
"A.java",
178242
"import static org.assertj.core.api.Assertions.assertThat;",
179243
"",
180244
"import com.google.common.collect.ImmutableList;",
245+
"import com.google.common.collect.ImmutableListMultimap;",
181246
"import com.google.common.collect.ImmutableMultiset;",
182247
"import com.google.common.collect.ImmutableSet;",
248+
"import com.google.common.collect.ImmutableSetMultimap;",
183249
"import com.google.errorprone.BugCheckerRefactoringTestHelper;",
184250
"import com.google.errorprone.CompilationTestHelper;",
185251
"import com.google.errorprone.bugpatterns.BugChecker;",
@@ -211,6 +277,9 @@ void replacement() {
211277
"",
212278
" BugCheckerRefactoringTestHelper.newInstance(BugChecker.class, getClass()).setArgs(\"foo\", \"bar\");",
213279
" CompilationTestHelper.newInstance(BugChecker.class, getClass()).setArgs(\"foo\", \"bar\");",
280+
"",
281+
" ImmutableListMultimap.builder().putAll(1, 2);",
282+
" ImmutableSetMultimap.<String, Integer>builder().putAll(\"foo\", 2);",
214283
" }",
215284
"}")
216285
.doTest(TestMode.TEXT_MATCH);

‎error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ImmutableListMultimapRulesTestInput.java‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -130,7 +130,6 @@ ImmutableListMultimap<String, Integer> testTransformMultimapValuesToImmutableLis
130130
testImmutableListMultimapBuilderPut() {
131131
return ImmutableSet.of(
132132
ImmutableListMultimap.<String, Integer>builder().put(Map.entry("foo", 1)),
133-
ImmutableListMultimap.<String, Integer>builder().putAll("bar", 2),
134-
ImmutableListMultimap.<String, Integer>builder().putAll("baz", ImmutableList.of(3)));
133+
ImmutableListMultimap.<String, Integer>builder().putAll("bar", 2));
135134
}
136135
}

‎error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ImmutableListMultimapRulesTestOutput.java‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -103,7 +103,6 @@ ImmutableListMultimap<String, Integer> testTransformMultimapValuesToImmutableLis
103103
testImmutableListMultimapBuilderPut() {
104104
return ImmutableSet.of(
105105
ImmutableListMultimap.<String, Integer>builder().put("foo", 1),
106-
ImmutableListMultimap.<String, Integer>builder().put("bar", 2),
107-
ImmutableListMultimap.<String, Integer>builder().put("baz", 3));
106+
ImmutableListMultimap.<String, Integer>builder().put("bar", 2));
108107
}
109108
}

‎error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ImmutableSetMultimapRulesTestInput.java‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -96,7 +96,6 @@ ImmutableSetMultimap<String, Integer> testTransformMultimapValuesToImmutableSetM
9696
ImmutableSet<ImmutableSetMultimap.Builder<String, Integer>> testImmutableSetMultimapBuilderPut() {
9797
return ImmutableSet.of(
9898
ImmutableSetMultimap.<String, Integer>builder().put(Map.entry("foo", 1)),
99-
ImmutableSetMultimap.<String, Integer>builder().putAll("bar", 2),
100-
ImmutableSetMultimap.<String, Integer>builder().putAll("baz", ImmutableSet.of(3)));
99+
ImmutableSetMultimap.<String, Integer>builder().putAll("bar", 2));
101100
}
102101
}

‎error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ImmutableSetMultimapRulesTestOutput.java‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -77,7 +77,6 @@ ImmutableSetMultimap<String, Integer> testTransformMultimapValuesToImmutableSetM
7777
ImmutableSet<ImmutableSetMultimap.Builder<String, Integer>> testImmutableSetMultimapBuilderPut() {
7878
return ImmutableSet.of(
7979
ImmutableSetMultimap.<String, Integer>builder().put("foo", 1),
80-
ImmutableSetMultimap.<String, Integer>builder().put("bar", 2),
81-
ImmutableSetMultimap.<String, Integer>builder().put("baz", 3));
80+
ImmutableSetMultimap.<String, Integer>builder().put("bar", 2));
8281
}
8382
}

0 commit comments

Comments
 (0)