diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/ExplicitArgumentEnumeration.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/ExplicitArgumentEnumeration.java index a71abc13d5..49dc17b1f9 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/ExplicitArgumentEnumeration.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/ExplicitArgumentEnumeration.java @@ -25,9 +25,9 @@ import com.google.errorprone.matchers.Description; import com.google.errorprone.matchers.Matcher; import com.google.errorprone.util.ASTHelpers; +import com.google.errorprone.util.Visibility; import com.sun.source.tree.ExpressionTree; import com.sun.source.tree.MethodInvocationTree; -import com.sun.tools.javac.code.Symbol; import com.sun.tools.javac.code.Symbol.MethodSymbol; import com.sun.tools.javac.code.Symbol.VarSymbol; import java.util.Arrays; @@ -35,7 +35,7 @@ import java.util.Map; import java.util.Optional; import java.util.Set; -import javax.lang.model.element.Modifier; +import javax.lang.model.element.Element; import tech.picnic.errorprone.utils.SourceCode; /** @@ -91,6 +91,8 @@ public final class ExplicitArgumentEnumeration extends BugChecker OBJECT_ENUMERABLE_ASSERT, "containsExactlyInAnyOrderElementsOf", "containsExactlyInAnyOrder") + .put(OBJECT_ENUMERABLE_ASSERT, "containsOnlyElementsOf", "containsOnly") + .put(OBJECT_ENUMERABLE_ASSERT, "containsOnlyOnceElementsOf", "containsOnlyOnce") .put(OBJECT_ENUMERABLE_ASSERT, "doesNotContainAnyElementsOf", "doesNotContain") .put(OBJECT_ENUMERABLE_ASSERT, "hasSameElementsAs", "containsOnly") .put(STEP_VERIFIER_STEP, "expectNextSequence", "expectNext") @@ -102,6 +104,7 @@ public ExplicitArgumentEnumeration() {} @Override public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) { if (tree.getArguments().size() != 1) { + /* Performance optimization: non-unary method invocations cannot be simplified. */ return Description.NO_MATCH; } @@ -111,8 +114,7 @@ public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState } ExpressionTree argument = tree.getArguments().get(0); - if (!(argument instanceof MethodInvocationTree) - || !EXPLICIT_ITERABLE_CREATOR.matches(argument, state)) { + if (!EXPLICIT_ITERABLE_CREATOR.matches(argument, state)) { return Description.NO_MATCH; } @@ -131,10 +133,14 @@ private static boolean isUnaryIterableAcceptingMethod(MethodSymbol method, Visit private static Optional trySuggestCallingVarargsOverload( MethodSymbol method, MethodInvocationTree argument, VisitorState state) { + /* + * Collect all overloads of the given method that we are sure to be able to call. Note that the + * `isAtLeastAsVisible` check is conservative heuristic. + */ ImmutableList overloads = ASTHelpers.matchingMethods( method.getSimpleName(), - m -> isPublic(m) && !m.equals(method), + m -> isAtLeastAsVisible(m, method), method.enclClass().type, state.getTypes()) .collect(toImmutableList()); @@ -176,25 +182,19 @@ private static Optional trySuggestCallingCustomAlternative( MethodInvocationTree argument, VisitorState state, Map alternatives) { - String name = ASTHelpers.getSymbol(tree).getSimpleName().toString(); - String alternative = alternatives.get(name); - - if (alternative == null) { - return Optional.empty(); - } - - SuggestedFix fix = SourceCode.unwrapMethodInvocation(argument, state); - return Optional.of( - alternative.equals(name) - ? fix - : SuggestedFix.builder() - .merge(SuggestedFixes.renameMethodInvocation(tree, alternative, state)) - .merge(fix) - .build()); + return Optional.ofNullable( + alternatives.get(ASTHelpers.getSymbol(tree).getSimpleName().toString())) + .map( + replacement -> + SuggestedFix.builder() + .merge(SuggestedFixes.renameMethodInvocation(tree, replacement, state)) + .merge(SourceCode.unwrapMethodInvocation(argument, state)) + .build()); } - // XXX: Once we target JDK 14+, drop this method in favour of `Symbol#isPublic()`. - private static boolean isPublic(Symbol symbol) { - return symbol.getModifiers().contains(Modifier.PUBLIC); + private static boolean isAtLeastAsVisible(Element symbol, Element reference) { + return Visibility.fromModifiers(symbol.getModifiers()) + .compareTo(Visibility.fromModifiers(reference.getModifiers())) + >= 0; } } diff --git a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/ExplicitArgumentEnumerationTest.java b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/ExplicitArgumentEnumerationTest.java index 8108422b69..7cade5422a 100644 --- a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/ExplicitArgumentEnumerationTest.java +++ b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/ExplicitArgumentEnumerationTest.java @@ -29,6 +29,11 @@ void identification() { "", " DSL.row(ImmutableList.of(1, 2));", "", + " // BUG: Diagnostic contains:", + " unaryMethod(ImmutableList.of(1, 2));", + " unaryMethodWithLessVisibleOverload(ImmutableList.of(1, 2));", + " binaryMethod(ImmutableList.of(1, 2), 3);", + "", " ImmutableList.builder()", " // BUG: Diagnostic contains:", " .addAll(ImmutableList.of())", @@ -36,7 +41,9 @@ void identification() { "", " assertThat(ImmutableList.of(1))", " // BUG: Diagnostic contains:", - " .containsAnyElementsOf(ImmutableList.of(1));", + " .containsAnyElementsOf(ImmutableList.of(1))", + " // BUG: Diagnostic contains:", + " .isSubsetOf(ImmutableList.of(1));", "", " Flux.just(1, 2)", " .as(StepVerifier::create)", @@ -49,6 +56,18 @@ void identification() { " .setArgs(ImmutableList.of(\"foo\"))", " .withClasspath();", " }", + "", + " private void unaryMethod(ImmutableList args) {}", + "", + " private void unaryMethod(Integer... args) {}", + "", + " void unaryMethodWithLessVisibleOverload(ImmutableList args) {}", + "", + " private void unaryMethodWithLessVisibleOverload(Integer... args) {}", + "", + " private void binaryMethod(ImmutableList args, int extraArg) {}", + "", + " private void binaryMethod(Integer... args) {}", "}") .doTest(); }