diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JUnitValueSource.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JUnitValueSource.java new file mode 100644 index 0000000000..e6fe6df852 --- /dev/null +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JUnitValueSource.java @@ -0,0 +1,307 @@ +package tech.picnic.errorprone.bugpatterns; + +import static com.google.errorprone.BugPattern.LinkType.CUSTOM; +import static com.google.errorprone.BugPattern.SeverityLevel.SUGGESTION; +import static com.google.errorprone.BugPattern.StandardTags.SIMPLIFICATION; +import static com.google.errorprone.matchers.ChildMultiMatcher.MatchType.ALL; +import static com.google.errorprone.matchers.ChildMultiMatcher.MatchType.AT_LEAST_ONE; +import static com.google.errorprone.matchers.Matchers.allOf; +import static com.google.errorprone.matchers.Matchers.anyOf; +import static com.google.errorprone.matchers.Matchers.anything; +import static com.google.errorprone.matchers.Matchers.argument; +import static com.google.errorprone.matchers.Matchers.argumentCount; +import static com.google.errorprone.matchers.Matchers.classLiteral; +import static com.google.errorprone.matchers.Matchers.hasArguments; +import static com.google.errorprone.matchers.Matchers.isPrimitiveOrBoxedPrimitiveType; +import static com.google.errorprone.matchers.Matchers.isSameType; +import static com.google.errorprone.matchers.Matchers.methodHasParameters; +import static com.google.errorprone.matchers.Matchers.staticMethod; +import static com.google.errorprone.matchers.Matchers.toType; +import static java.util.function.Predicate.not; +import static java.util.stream.Collectors.joining; +import static tech.picnic.errorprone.bugpatterns.util.Documentation.BUG_PATTERNS_BASE_URL; +import static tech.picnic.errorprone.bugpatterns.util.MoreJUnitMatchers.HAS_METHOD_SOURCE; +import static tech.picnic.errorprone.bugpatterns.util.MoreJUnitMatchers.getMethodSourceFactoryNames; + +import com.google.auto.service.AutoService; +import com.google.common.collect.Iterables; +import com.google.errorprone.BugPattern; +import com.google.errorprone.VisitorState; +import com.google.errorprone.bugpatterns.BugChecker; +import com.google.errorprone.bugpatterns.BugChecker.MethodTreeMatcher; +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.AnnotationTree; +import com.sun.source.tree.ClassTree; +import com.sun.source.tree.ExpressionTree; +import com.sun.source.tree.LambdaExpressionTree; +import com.sun.source.tree.MethodInvocationTree; +import com.sun.source.tree.MethodTree; +import com.sun.source.tree.NewArrayTree; +import com.sun.source.tree.ReturnTree; +import com.sun.source.util.TreeScanner; +import com.sun.tools.javac.code.Type; +import java.util.ArrayList; +import java.util.Collection; +import java.util.List; +import java.util.Locale; +import java.util.Optional; +import java.util.Set; +import java.util.function.Predicate; +import java.util.stream.DoubleStream; +import java.util.stream.IntStream; +import java.util.stream.LongStream; +import java.util.stream.Stream; +import org.jspecify.annotations.Nullable; +import tech.picnic.errorprone.bugpatterns.util.SourceCode; + +/** + * A {@link BugChecker} that flags JUnit tests with a {@link + * org.junit.jupiter.params.provider.MethodSource} annotation that can be replaced with an + * equivalent {@link org.junit.jupiter.params.provider.ValueSource} annotation. + */ +// XXX: Where applicable, also flag `@MethodSource` annotations that reference multiple value +// factory methods (or that repeat the same value factory method multiple times). +// XXX: Support inlining of overloaded value factory methods. +// XXX: Support inlining of value factory methods referenced by multiple `@MethodSource` +// annotations. +// XXX: Support value factory return expressions of the form `Stream.of(a, b, +// c).map(Arguments::argument)`. +// XXX: Support simplification of test methods that accept additional injected parameters such as +// `TestInfo`; such parameters should be ignored for the purpose of this check. +@AutoService(BugChecker.class) +@BugPattern( + summary = "Prefer `@ValueSource` over a `@MethodSource` where possible and reasonable", + linkType = CUSTOM, + link = BUG_PATTERNS_BASE_URL + "JUnitValueSource", + severity = SUGGESTION, + tags = SIMPLIFICATION) +public final class JUnitValueSource extends BugChecker implements MethodTreeMatcher { + private static final long serialVersionUID = 1L; + private static final Matcher SUPPORTED_VALUE_FACTORY_VALUES = + anyOf( + isArrayArgumentValueCandidate(), + toType( + MethodInvocationTree.class, + allOf( + staticMethod() + .onClass("org.junit.jupiter.params.provider.Arguments") + .namedAnyOf("arguments", "of"), + argumentCount(1), + argument(0, isArrayArgumentValueCandidate())))); + private static final Matcher ARRAY_OF_SUPPORTED_SINGLE_VALUE_ARGUMENTS = + isSingleDimensionArrayCreationWithAllElementsMatching(SUPPORTED_VALUE_FACTORY_VALUES); + private static final Matcher ENUMERATION_OF_SUPPORTED_SINGLE_VALUE_ARGUMENTS = + toType( + MethodInvocationTree.class, + allOf( + staticMethod() + .onClassAny( + Stream.class.getName(), + IntStream.class.getName(), + LongStream.class.getName(), + DoubleStream.class.getName(), + List.class.getName(), + Set.class.getName(), + "com.google.common.collect.ImmutableList", + "com.google.common.collect.ImmutableSet") + .named("of"), + hasArguments(AT_LEAST_ONE, anything()), + hasArguments(ALL, SUPPORTED_VALUE_FACTORY_VALUES))); + private static final Matcher IS_UNARY_METHOD_WITH_SUPPORTED_PARAMETER = + methodHasParameters( + anyOf( + isPrimitiveOrBoxedPrimitiveType(), + isSameType(String.class), + isSameType(state -> state.getSymtab().classType))); + + /** Instantiates a new {@link JUnitValueSource} instance. */ + public JUnitValueSource() {} + + @Override + public Description matchMethod(MethodTree tree, VisitorState state) { + if (!IS_UNARY_METHOD_WITH_SUPPORTED_PARAMETER.matches(tree, state)) { + return Description.NO_MATCH; + } + + Type parameterType = ASTHelpers.getType(Iterables.getOnlyElement(tree.getParameters())); + + return findMethodSourceAnnotation(tree, state) + .flatMap( + methodSourceAnnotation -> + getSoleLocalFactoryName(methodSourceAnnotation, tree) + .filter(factory -> !hasSiblingReferencingValueFactory(tree, factory, state)) + .flatMap(factory -> findSiblingWithName(tree, factory, state)) + .flatMap( + factoryMethod -> + tryConstructValueSourceFix( + parameterType, methodSourceAnnotation, factoryMethod, state)) + .map(fix -> describeMatch(methodSourceAnnotation, fix))) + .orElse(Description.NO_MATCH); + } + + /** + * Returns the name of the value factory method pointed to by the given {@code @MethodSource} + * annotation, if it (a) is the only one and (b) is a method in the same class as the annotated + * method. + */ + private static Optional getSoleLocalFactoryName( + AnnotationTree methodSourceAnnotation, MethodTree method) { + return getElementIfSingleton(getMethodSourceFactoryNames(methodSourceAnnotation, method)) + .filter(name -> name.indexOf('#') < 0); + } + + /** + * Tells whether the given method has a sibling method in the same class that depends on the + * specified value factory method. + */ + private static boolean hasSiblingReferencingValueFactory( + MethodTree tree, String valueFactory, VisitorState state) { + return findMatchingSibling(tree, m -> hasValueFactory(m, valueFactory, state), state) + .isPresent(); + } + + private static Optional findSiblingWithName( + MethodTree tree, String methodName, VisitorState state) { + return findMatchingSibling(tree, m -> m.getName().contentEquals(methodName), state); + } + + private static Optional findMatchingSibling( + MethodTree tree, Predicate predicate, VisitorState state) { + return state.findEnclosing(ClassTree.class).getMembers().stream() + .filter(MethodTree.class::isInstance) + .map(MethodTree.class::cast) + .filter(not(tree::equals)) + .filter(predicate) + .findFirst(); + } + + private static boolean hasValueFactory( + MethodTree tree, String valueFactoryMethodName, VisitorState state) { + return findMethodSourceAnnotation(tree, state).stream() + .anyMatch( + annotation -> + getMethodSourceFactoryNames(annotation, tree).contains(valueFactoryMethodName)); + } + + private static Optional findMethodSourceAnnotation( + MethodTree tree, VisitorState state) { + return HAS_METHOD_SOURCE.multiMatchResult(tree, state).matchingNodes().stream().findFirst(); + } + + private static Optional tryConstructValueSourceFix( + Type parameterType, + AnnotationTree methodSourceAnnotation, + MethodTree valueFactoryMethod, + VisitorState state) { + return getSingleReturnExpression(valueFactoryMethod) + .flatMap(expression -> tryExtractValueSourceAttributeValue(expression, state)) + .map( + valueSourceAttributeValue -> + SuggestedFix.builder() + .addImport("org.junit.jupiter.params.provider.ValueSource") + .replace( + methodSourceAnnotation, + String.format( + "@ValueSource(%s = %s)", + toValueSourceAttributeName(parameterType), valueSourceAttributeValue)) + .delete(valueFactoryMethod) + .build()); + } + + // XXX: This pattern also occurs a few times inside Error Prone; contribute upstream. + private static Optional getSingleReturnExpression(MethodTree methodTree) { + List returnExpressions = new ArrayList<>(); + new TreeScanner<@Nullable Void, @Nullable Void>() { + @Override + public @Nullable Void visitClass(ClassTree node, @Nullable Void unused) { + /* Ignore `return` statements inside anonymous/local classes. */ + return null; + } + + @Override + public @Nullable Void visitReturn(ReturnTree node, @Nullable Void unused) { + returnExpressions.add(node.getExpression()); + return super.visitReturn(node, unused); + } + + @Override + public @Nullable Void visitLambdaExpression( + LambdaExpressionTree node, @Nullable Void unused) { + /* Ignore `return` statements inside lambda expressions. */ + return null; + } + }.scan(methodTree, null); + + return getElementIfSingleton(returnExpressions); + } + + private static Optional tryExtractValueSourceAttributeValue( + ExpressionTree tree, VisitorState state) { + List arguments; + if (ENUMERATION_OF_SUPPORTED_SINGLE_VALUE_ARGUMENTS.matches(tree, state)) { + arguments = ((MethodInvocationTree) tree).getArguments(); + } else if (ARRAY_OF_SUPPORTED_SINGLE_VALUE_ARGUMENTS.matches(tree, state)) { + arguments = ((NewArrayTree) tree).getInitializers(); + } else { + return Optional.empty(); + } + + /* + * Join the values into a comma-separated string, unwrapping `Arguments` factory method + * invocations if applicable. + */ + return Optional.of( + arguments.stream() + .map( + arg -> + arg instanceof MethodInvocationTree + ? Iterables.getOnlyElement(((MethodInvocationTree) arg).getArguments()) + : arg) + .map(argument -> SourceCode.treeToString(argument, state)) + .collect(joining(", "))) + .map(value -> arguments.size() > 1 ? String.format("{%s}", value) : value); + } + + private static String toValueSourceAttributeName(Type type) { + String typeString = type.tsym.name.toString(); + + switch (typeString) { + case "Class": + return "classes"; + case "Character": + return "chars"; + case "Integer": + return "ints"; + default: + return typeString.toLowerCase(Locale.ROOT) + 's'; + } + } + + private static Optional getElementIfSingleton(Collection collection) { + return Optional.of(collection) + .filter(elements -> elements.size() == 1) + .map(Iterables::getOnlyElement); + } + + private static Matcher isSingleDimensionArrayCreationWithAllElementsMatching( + Matcher elementMatcher) { + return (tree, state) -> { + if (!(tree instanceof NewArrayTree)) { + return false; + } + + NewArrayTree newArray = (NewArrayTree) tree; + return newArray.getDimensions().isEmpty() + && !newArray.getInitializers().isEmpty() + && newArray.getInitializers().stream() + .allMatch(element -> elementMatcher.matches(element, state)); + }; + } + + private static Matcher isArrayArgumentValueCandidate() { + return anyOf(classLiteral(anything()), (tree, state) -> ASTHelpers.constValue(tree) != null); + } +} diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/RedundantStringConversion.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/RedundantStringConversion.java index 3fae404499..53c6e0f905 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/RedundantStringConversion.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/RedundantStringConversion.java @@ -7,6 +7,7 @@ import static com.google.errorprone.matchers.Matchers.allOf; import static com.google.errorprone.matchers.Matchers.anyMethod; import static com.google.errorprone.matchers.Matchers.anyOf; +import static com.google.errorprone.matchers.Matchers.anything; import static com.google.errorprone.matchers.Matchers.argumentCount; import static com.google.errorprone.matchers.Matchers.isNonNullUsingDataflow; import static com.google.errorprone.matchers.Matchers.isSameType; @@ -67,9 +68,7 @@ public final class RedundantStringConversion extends BugChecker private static final String EXTRA_STRING_CONVERSION_METHODS_FLAG = "RedundantStringConversion:ExtraConversionMethods"; - @SuppressWarnings("UnnecessaryLambda") - private static final Matcher ANY_EXPR = (t, s) -> true; - + private static final Matcher ANY_EXPR = anything(); private static final Matcher LOCALE = isSameType(Locale.class); private static final Matcher MARKER = isSubtypeOf("org.slf4j.Marker"); private static final Matcher STRING = isSameType(String.class); diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/util/MoreJUnitMatchers.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/util/MoreJUnitMatchers.java index c26bda471f..04b230b61e 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/util/MoreJUnitMatchers.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/util/MoreJUnitMatchers.java @@ -1,6 +1,6 @@ package tech.picnic.errorprone.bugpatterns.util; -import static com.google.common.collect.ImmutableSet.toImmutableSet; +import static com.google.common.collect.ImmutableList.toImmutableList; import static com.google.errorprone.matchers.ChildMultiMatcher.MatchType.AT_LEAST_ONE; import static com.google.errorprone.matchers.Matchers.annotations; import static com.google.errorprone.matchers.Matchers.anyOf; @@ -9,7 +9,7 @@ import static tech.picnic.errorprone.bugpatterns.util.MoreMatchers.hasMetaAnnotation; import com.google.common.base.Strings; -import com.google.common.collect.ImmutableSet; +import com.google.common.collect.ImmutableList; import com.google.errorprone.matchers.AnnotationMatcherUtils; import com.google.errorprone.matchers.Matcher; import com.google.errorprone.matchers.MultiMatcher; @@ -55,25 +55,59 @@ private MoreJUnitMatchers() {} * Returns the names of the JUnit value factory methods specified by the given {@link * org.junit.jupiter.params.provider.MethodSource} annotation. * + *

This method differs from {@link #getMethodSourceFactoryDescriptors(AnnotationTree, + * MethodTree)} in that it drops any parenthesized method parameter type enumerations. That is, + * method descriptors such as {@code factoryMethod()} and {@code factoryMethod(java.lang.String)} + * are both simplified to just {@code factoryMethod}. This also means that the returned method + * names may not unambiguously reference a single value factory method; in such a case JUnit will + * throw an error at runtime. + * * @param methodSourceAnnotation The annotation from which to extract value factory method names. - * @return One or more value factory names. + * @param method The method on which the annotation is located. + * @return One or more value factory descriptors, in the order defined. + * @see #getMethodSourceFactoryDescriptors(AnnotationTree, MethodTree) + */ + // XXX: Drop this method in favour of `#getMethodSourceFactoryDescriptors`. That will require + // callers to either explicitly drop information, or perform a more advanced analysis. + public static ImmutableList getMethodSourceFactoryNames( + AnnotationTree methodSourceAnnotation, MethodTree method) { + return getMethodSourceFactoryDescriptors(methodSourceAnnotation, method).stream() + .map( + descriptor -> { + int index = descriptor.indexOf('('); + return index < 0 ? descriptor : descriptor.substring(0, index); + }) + .collect(toImmutableList()); + } + + /** + * Returns the descriptors of the JUnit value factory methods specified by the given {@link + * org.junit.jupiter.params.provider.MethodSource} annotation. + * + * @param methodSourceAnnotation The annotation from which to extract value factory method + * descriptors. + * @param method The method on which the annotation is located. + * @return One or more value factory descriptors, in the order defined. + * @see #getMethodSourceFactoryNames(AnnotationTree, MethodTree) */ - static ImmutableSet getMethodSourceFactoryNames( + // XXX: Rather than strings, have this method return instances of a value type capable of + // resolving the value factory method pointed to. + public static ImmutableList getMethodSourceFactoryDescriptors( AnnotationTree methodSourceAnnotation, MethodTree method) { String methodName = method.getName().toString(); ExpressionTree value = AnnotationMatcherUtils.getArgument(methodSourceAnnotation, "value"); if (!(value instanceof NewArrayTree)) { - return ImmutableSet.of(toMethodSourceFactoryName(value, methodName)); + return ImmutableList.of(toMethodSourceFactoryDescriptor(value, methodName)); } return ((NewArrayTree) value) .getInitializers().stream() - .map(name -> toMethodSourceFactoryName(name, methodName)) - .collect(toImmutableSet()); + .map(name -> toMethodSourceFactoryDescriptor(name, methodName)) + .collect(toImmutableList()); } - private static String toMethodSourceFactoryName( + private static String toMethodSourceFactoryDescriptor( @Nullable ExpressionTree tree, String annotatedMethodName) { return requireNonNullElse( Strings.emptyToNull(ASTHelpers.constValue(tree, String.class)), annotatedMethodName); diff --git a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/JUnitValueSourceTest.java b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/JUnitValueSourceTest.java new file mode 100644 index 0000000000..13ed3fa24d --- /dev/null +++ b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/JUnitValueSourceTest.java @@ -0,0 +1,496 @@ +package tech.picnic.errorprone.bugpatterns; + +import com.google.errorprone.BugCheckerRefactoringTestHelper; +import com.google.errorprone.BugCheckerRefactoringTestHelper.TestMode; +import com.google.errorprone.CompilationTestHelper; +import org.junit.jupiter.api.Test; + +final class JUnitValueSourceTest { + @Test + void identification() { + CompilationTestHelper.newInstance(JUnitValueSource.class, getClass()) + .addSourceLines( + "A.java", + "import static org.junit.jupiter.params.provider.Arguments.arguments;", + "", + "import java.util.Optional;", + "import java.util.stream.Stream;", + "import org.junit.jupiter.params.ParameterizedTest;", + "import org.junit.jupiter.params.provider.Arguments;", + "import org.junit.jupiter.params.provider.MethodSource;", + "", + "class A {", + " private static Stream identificationTestCases() {", + " return Stream.of(arguments(1), Arguments.of(2));", + " }", + "", + " @ParameterizedTest", + " // BUG: Diagnostic contains:", + " @MethodSource(\"identificationTestCases\")", + " void identification(int foo) {}", + "", + " private static int[] identificationWithParensTestCases() {", + " return new int[] {1, 2};", + " }", + "", + " @ParameterizedTest", + " // BUG: Diagnostic contains:", + " @MethodSource(\"identificationWithParensTestCases()\")", + " void identificationWithParens(int foo) {}", + "", + " @ParameterizedTest", + " @MethodSource(\"valueFactoryMissingTestCases\")", + " void valueFactoryMissing(int foo) {}", + "", + " private static Stream multipleUsagesTestCases() {", + " return Stream.of(arguments(1), Arguments.of(2));", + " }", + "", + " @ParameterizedTest", + " @MethodSource(\"multipleUsagesTestCases\")", + " void multipleUsages1(int foo) {}", + "", + " @ParameterizedTest", + " @MethodSource(\"multipleUsagesTestCases()\")", + " void multipleUsages2(int bar) {}", + "", + " private static Stream valueFactoryRepeatedTestCases() {", + " return Stream.of(arguments(1), arguments(2));", + " }", + "", + " @ParameterizedTest", + " @MethodSource({\"valueFactoryRepeatedTestCases\", \"valueFactoryRepeatedTestCases\"})", + " void valueFactoryRepeated(int foo) {}", + "", + " private static Stream multipleParametersTestCases() {", + " return Stream.of(arguments(1, 2), arguments(3, 4));", + " }", + "", + " @ParameterizedTest", + " @MethodSource(\"multipleParametersTestCases\")", + " void multipleParameters(int first, int second) {}", + "", + " private static int[] arrayWithoutInitializersTestCases() {", + " return new int[1];", + " }", + "", + " @ParameterizedTest", + " @MethodSource(\"arrayWithoutInitializersTestCases\")", + " void arrayWithoutInitializers(int foo) {}", + "", + " private static Stream runtimeValueTestCases() {", + " int second = 2;", + " return Stream.of(arguments(1), arguments(second));", + " }", + "", + " @ParameterizedTest", + " @MethodSource(\"runtimeValueTestCases\")", + " void runtimeValue(int foo) {}", + "", + " private static Stream streamChainTestCases() {", + " return Stream.of(1, 2).map(Arguments::arguments);", + " }", + "", + " @ParameterizedTest", + " @MethodSource(\"streamChainTestCases\")", + " void streamChain(int number) {}", + "", + " private static Stream multipleReturnsTestCases() {", + " if (true) {", + " return Stream.of(arguments(1), arguments(2));", + " } else {", + " return Stream.of(arguments(3), arguments(4));", + " }", + " }", + "", + " @ParameterizedTest", + " @MethodSource(\"multipleReturnsTestCases\")", + " void multipleReturns(int number) {}", + "", + " private static Stream multipleFactoriesFooTestCases() {", + " return Stream.of(arguments(1));", + " }", + "", + " private static Stream multipleFactoriesBarTestCases() {", + " return Stream.of(arguments(1));", + " }", + "", + " @ParameterizedTest", + " @MethodSource({\"multipleFactoriesFooTestCases\", \"multipleFactoriesBarTestCases\"})", + " void multipleFactories(int i) {}", + "", + " private static Stream extraArgsTestCases() {", + " return Stream.of(arguments(1), arguments(1, 2));", + " }", + "", + " @ParameterizedTest", + " @MethodSource(\"extraArgsTestCases\")", + " void extraArgs(int... i) {}", + "", + " private static Stream localClassTestCases() {", + " class Foo {", + " Stream foo() {", + " return Stream.of(arguments(1), arguments(2));", + " }", + " }", + " return Stream.of(arguments(1), arguments(2));", + " }", + "", + " @ParameterizedTest", + " // BUG: Diagnostic contains:", + " @MethodSource(\"localClassTestCases\")", + " void localClass(int i) {}", + "", + " private static Stream lambdaReturnTestCases() {", + " int foo =", + " Optional.of(10)", + " .map(", + " i -> {", + " return i / 2;", + " })", + " .orElse(0);", + " return Stream.of(arguments(1), arguments(1));", + " }", + "", + " @ParameterizedTest", + " // BUG: Diagnostic contains:", + " @MethodSource(\"lambdaReturnTestCases\")", + " void lambdaReturn(int i) {}", + "", + " @ParameterizedTest", + " @MethodSource(\"tech.picnic.errorprone.Foo#fooTestCases\")", + " void staticMethodReference(int foo) {}", + "", + " private static Stream valueFactoryWithArgumentTestCases(int amount) {", + " return Stream.of(arguments(1), arguments(2));", + " }", + "", + " @ParameterizedTest", + " // BUG: Diagnostic contains:", + " @MethodSource(\"valueFactoryWithArgumentTestCases\")", + " void valueFactoryWithArgument(int foo) {}", + "", + " private static Arguments[] emptyArrayValueFactoryTestCases() {", + " return new Arguments[] {};", + " }", + "", + " @ParameterizedTest", + " @MethodSource(\"emptyArrayValueFactoryTestCases\")", + " void emptyArrayValueFactory(int foo) {}", + "", + " private static Stream emptyStreamValueFactoryTestCases() {", + " return Stream.of();", + " }", + "", + " @ParameterizedTest", + " @MethodSource(\"emptyStreamValueFactoryTestCases\")", + " void emptyStreamValueFactory(int foo) {}", + "", + " private static Arguments[] invalidValueFactoryArgumentsTestCases() {", + " return new Arguments[] {arguments(1), arguments(new Object() {})};", + " }", + "", + " @ParameterizedTest", + " @MethodSource(\"invalidValueFactoryArgumentsTestCases\")", + " void invalidValueFactoryArguments(int foo) {}", + "}") + .doTest(); + } + + @Test + void replacement() { + BugCheckerRefactoringTestHelper.newInstance(JUnitValueSource.class, getClass()) + .addInputLines( + "A.java", + "import static org.junit.jupiter.params.provider.Arguments.arguments;", + "", + "import com.google.common.collect.ImmutableList;", + "import com.google.common.collect.ImmutableSet;", + "import java.util.List;", + "import java.util.Set;", + "import java.util.stream.DoubleStream;", + "import java.util.stream.IntStream;", + "import java.util.stream.LongStream;", + "import java.util.stream.Stream;", + "import org.junit.jupiter.params.ParameterizedTest;", + "import org.junit.jupiter.params.provider.Arguments;", + "import org.junit.jupiter.params.provider.MethodSource;", + "", + "class A {", + " private static final boolean CONST_BOOLEAN = false;", + " private static final byte CONST_BYTE = 42;", + " private static final char CONST_CHARACTER = 'a';", + " private static final short CONST_SHORT = 42;", + " private static final int CONST_INTEGER = 42;", + " private static final long CONST_LONG = 42;", + " private static final float CONST_FLOAT = 42;", + " private static final double CONST_DOUBLE = 42;", + " private static final String CONST_STRING = \"foo\";", + "", + " private static Stream streamOfBooleanArguments() {", + " return Stream.of(arguments(false), arguments(true), arguments(CONST_BOOLEAN));", + " }", + "", + " @ParameterizedTest", + " @MethodSource(\"streamOfBooleanArguments\")", + " void primitiveBoolean(boolean b) {}", + "", + " private static Stream streamOfBooleansAndBooleanArguments() {", + " return Stream.of(false, arguments(true), CONST_BOOLEAN);", + " }", + "", + " @ParameterizedTest", + " @MethodSource(\"streamOfBooleansAndBooleanArguments\")", + " void boxedBoolean(Boolean b) {}", + "", + " private static List listOfByteArguments() {", + " return List.of(arguments((byte) 0), arguments((byte) 1), arguments(CONST_BYTE));", + " }", + "", + " @ParameterizedTest", + " @MethodSource(\"listOfByteArguments\")", + " void primitiveByte(byte b) {}", + "", + " private static List listOfBytesAndByteArguments() {", + " return List.of((byte) 0, arguments((byte) 1), CONST_BYTE);", + " }", + "", + " @ParameterizedTest", + " @MethodSource(\"listOfBytesAndByteArguments\")", + " void boxedByte(Byte b) {}", + "", + " private static Set setOfCharacterArguments() {", + " return Set.of(arguments((char) 0), arguments((char) 1), arguments(CONST_CHARACTER));", + " }", + "", + " @ParameterizedTest", + " @MethodSource(\"setOfCharacterArguments\")", + " void primitiveCharacter(char c) {}", + "", + " private static Set setOfCharactersAndCharacterArguments() {", + " return Set.of((char) 0, arguments((char) 1), CONST_CHARACTER);", + " }", + "", + " @ParameterizedTest", + " @MethodSource(\"setOfCharactersAndCharacterArguments\")", + " void boxedCharacter(Character c) {}", + "", + " private static Arguments[] arrayOfShortArguments() {", + " return new Arguments[] {arguments((short) 0), arguments((short) 1), arguments(CONST_SHORT)};", + " }", + "", + " @ParameterizedTest", + " @MethodSource(\"arrayOfShortArguments\")", + " void primitiveShort(short s) {}", + "", + " private static Object[] arrayOfShortsAndShortArguments() {", + " return new Object[] {(short) 0, arguments((short) 1), CONST_SHORT};", + " }", + "", + " @ParameterizedTest", + " @MethodSource(\"arrayOfShortsAndShortArguments\")", + " void boxedShort(Short s) {}", + "", + " private static IntStream intStream() {", + " return IntStream.of(0, 1, CONST_INTEGER);", + " }", + "", + " @ParameterizedTest", + " @MethodSource(\"intStream\")", + " void primitiveInteger(int i) {}", + "", + " private static int[] intArray() {", + " return new int[] {0, 1, CONST_INTEGER};", + " }", + "", + " @ParameterizedTest", + " @MethodSource(\"intArray\")", + " void boxedInteger(Integer i) {}", + "", + " private static LongStream longStream() {", + " return LongStream.of(0, 1, CONST_LONG);", + " }", + "", + " @ParameterizedTest", + " @MethodSource(\"longStream\")", + " void primitiveLong(long l) {}", + "", + " private static long[] longArray() {", + " return new long[] {0, 1, CONST_LONG};", + " }", + "", + " @ParameterizedTest", + " @MethodSource(\"longArray\")", + " void boxedLong(Long l) {}", + "", + " private static ImmutableList immutableListOfFloatArguments() {", + " return ImmutableList.of(arguments(0.0F), arguments(1.0F), arguments(CONST_FLOAT));", + " }", + "", + " @ParameterizedTest", + " @MethodSource(\"immutableListOfFloatArguments\")", + " void primitiveFloat(float f) {}", + "", + " private static Stream streamOfFloatsAndFloatArguments() {", + " return Stream.of(0.0F, arguments(1.0F), CONST_FLOAT);", + " }", + "", + " @ParameterizedTest", + " @MethodSource(\"streamOfFloatsAndFloatArguments\")", + " void boxedFloat(Float f) {}", + "", + " private static DoubleStream doubleStream() {", + " return DoubleStream.of(0, 1, CONST_DOUBLE);", + " }", + "", + " @ParameterizedTest", + " @MethodSource(\"doubleStream\")", + " void primitiveDouble(double d) {}", + "", + " private static double[] doubleArray() {", + " return new double[] {0, 1, CONST_DOUBLE};", + " }", + "", + " @ParameterizedTest", + " @MethodSource(\"doubleArray\")", + " void boxedDouble(Double d) {}", + "", + " private static ImmutableSet immutableSetOfStringArguments() {", + " return ImmutableSet.of(arguments(\"foo\"), arguments(\"bar\"), arguments(CONST_STRING));", + " }", + "", + " @ParameterizedTest", + " @MethodSource(\"immutableSetOfStringArguments\")", + " void string(String s) {}", + "", + " private static Stream> streamOfClasses() {", + " return Stream.of(Stream.class, java.util.Map.class);", + " }", + "", + " @ParameterizedTest", + " @MethodSource(\"streamOfClasses\")", + " void clazz(Class c) {}", + "", + " private static Stream sameNameFactoryTestCases() {", + " return Stream.of(arguments(1));", + " }", + "", + " private static Stream sameNameFactoryTestCases(int overload) {", + " return Stream.of(arguments(overload));", + " }", + "", + " @ParameterizedTest", + " @MethodSource(\"sameNameFactoryTestCases\")", + " void sameNameFactory(int i) {}", + "}") + .addOutputLines( + "A.java", + "import static org.junit.jupiter.params.provider.Arguments.arguments;", + "", + "import com.google.common.collect.ImmutableList;", + "import com.google.common.collect.ImmutableSet;", + "import java.util.List;", + "import java.util.Set;", + "import java.util.stream.DoubleStream;", + "import java.util.stream.IntStream;", + "import java.util.stream.LongStream;", + "import java.util.stream.Stream;", + "import org.junit.jupiter.params.ParameterizedTest;", + "import org.junit.jupiter.params.provider.Arguments;", + "import org.junit.jupiter.params.provider.MethodSource;", + "import org.junit.jupiter.params.provider.ValueSource;", + "", + "class A {", + " private static final boolean CONST_BOOLEAN = false;", + " private static final byte CONST_BYTE = 42;", + " private static final char CONST_CHARACTER = 'a';", + " private static final short CONST_SHORT = 42;", + " private static final int CONST_INTEGER = 42;", + " private static final long CONST_LONG = 42;", + " private static final float CONST_FLOAT = 42;", + " private static final double CONST_DOUBLE = 42;", + " private static final String CONST_STRING = \"foo\";", + "", + " @ParameterizedTest", + " @ValueSource(booleans = {false, true, CONST_BOOLEAN})", + " void primitiveBoolean(boolean b) {}", + "", + " @ParameterizedTest", + " @ValueSource(booleans = {false, true, CONST_BOOLEAN})", + " void boxedBoolean(Boolean b) {}", + "", + " @ParameterizedTest", + " @ValueSource(bytes = {(byte) 0, (byte) 1, CONST_BYTE})", + " void primitiveByte(byte b) {}", + "", + " @ParameterizedTest", + " @ValueSource(bytes = {(byte) 0, (byte) 1, CONST_BYTE})", + " void boxedByte(Byte b) {}", + "", + " @ParameterizedTest", + " @ValueSource(chars = {(char) 0, (char) 1, CONST_CHARACTER})", + " void primitiveCharacter(char c) {}", + "", + " @ParameterizedTest", + " @ValueSource(chars = {(char) 0, (char) 1, CONST_CHARACTER})", + " void boxedCharacter(Character c) {}", + "", + " @ParameterizedTest", + " @ValueSource(shorts = {(short) 0, (short) 1, CONST_SHORT})", + " void primitiveShort(short s) {}", + "", + " @ParameterizedTest", + " @ValueSource(shorts = {(short) 0, (short) 1, CONST_SHORT})", + " void boxedShort(Short s) {}", + "", + " @ParameterizedTest", + " @ValueSource(ints = {0, 1, CONST_INTEGER})", + " void primitiveInteger(int i) {}", + "", + " @ParameterizedTest", + " @ValueSource(ints = {0, 1, CONST_INTEGER})", + " void boxedInteger(Integer i) {}", + "", + " @ParameterizedTest", + " @ValueSource(longs = {0, 1, CONST_LONG})", + " void primitiveLong(long l) {}", + "", + " @ParameterizedTest", + " @ValueSource(longs = {0, 1, CONST_LONG})", + " void boxedLong(Long l) {}", + "", + " @ParameterizedTest", + " @ValueSource(floats = {0.0F, 1.0F, CONST_FLOAT})", + " void primitiveFloat(float f) {}", + "", + " @ParameterizedTest", + " @ValueSource(floats = {0.0F, 1.0F, CONST_FLOAT})", + " void boxedFloat(Float f) {}", + "", + " @ParameterizedTest", + " @ValueSource(doubles = {0, 1, CONST_DOUBLE})", + " void primitiveDouble(double d) {}", + "", + " @ParameterizedTest", + " @ValueSource(doubles = {0, 1, CONST_DOUBLE})", + " void boxedDouble(Double d) {}", + "", + " @ParameterizedTest", + " @ValueSource(strings = {\"foo\", \"bar\", CONST_STRING})", + " void string(String s) {}", + "", + " @ParameterizedTest", + " @ValueSource(classes = {Stream.class, java.util.Map.class})", + " void clazz(Class c) {}", + "", + " private static Stream sameNameFactoryTestCases(int overload) {", + " return Stream.of(arguments(overload));", + " }", + "", + " @ParameterizedTest", + " @ValueSource(ints = 1)", + " void sameNameFactory(int i) {}", + "}") + .doTest(TestMode.TEXT_MATCH); + } +} diff --git a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/util/MoreJUnitMatchersTest.java b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/util/MoreJUnitMatchersTest.java index caab722793..f21d57c608 100644 --- a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/util/MoreJUnitMatchersTest.java +++ b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/util/MoreJUnitMatchersTest.java @@ -83,6 +83,40 @@ void methodMatchers() { @Test void getMethodSourceFactoryNames() { CompilationTestHelper.newInstance(MethodSourceFactoryNamesTestChecker.class, getClass()) + .addSourceLines( + "A.java", + "import org.junit.jupiter.params.provider.MethodSource;", + "", + "class A {", + " @MethodSource", + " // BUG: Diagnostic contains: [matchingMethodSource]", + " void matchingMethodSource(boolean b) {}", + "", + " @MethodSource(\"myValueFactory\")", + " // BUG: Diagnostic contains: [myValueFactory]", + " void singleCustomMethodSource(boolean b) {}", + "", + " @MethodSource({", + " \"nullary()\",", + " \"nullary()\",", + " \"\",", + " \"withStringParam(java.lang.String)\",", + " \"paramsUnspecified\"", + " })", + " // BUG: Diagnostic contains: [nullary, nullary, multipleMethodSources, withStringParam,", + " // paramsUnspecified]", + " void multipleMethodSources(boolean b) {}", + "", + " @MethodSource({\"foo\", \"()\", \"bar\"})", + " // BUG: Diagnostic contains: [foo, , bar]", + " void methodSourceWithoutName(boolean b) {}", + "}") + .doTest(); + } + + @Test + void getMethodSourceFactoryDescriptors() { + CompilationTestHelper.newInstance(MethodSourceFactoryDescriptorsTestChecker.class, getClass()) .addSourceLines( "A.java", "import org.junit.jupiter.params.provider.MethodSource;", @@ -119,6 +153,14 @@ void getMethodSourceFactoryNames() { " @MethodSource({\"myValueFactory\", \"\"})", " // BUG: Diagnostic contains: [myValueFactory, customAndMatchingMethodSources]", " void customAndMatchingMethodSources(boolean b) {}", + "", + " @MethodSource({\"factory\", \"\", \"factory\", \"\"})", + " // BUG: Diagnostic contains: [factory, repeatedMethodSources, factory, repeatedMethodSources]", + " void repeatedMethodSources(boolean b) {}", + "", + " @MethodSource({\"nullary()\", \"withStringParam(java.lang.String)\"})", + " // BUG: Diagnostic contains: [nullary(), withStringParam(java.lang.String)]", + " void methodSourcesWithParameterSpecification(boolean b) {}", "}") .doTest(); } @@ -170,4 +212,25 @@ public Description matchMethod(MethodTree tree, VisitorState state) { .build(); } } + + /** + * A {@link BugChecker} that flags methods with a JUnit {@code @MethodSource} annotation by + * enumerating the associated value factory method descriptors. + */ + @BugPattern(summary = "Interacts with `MoreJUnitMatchers` for testing purposes", severity = ERROR) + public static final class MethodSourceFactoryDescriptorsTestChecker extends BugChecker + implements MethodTreeMatcher { + private static final long serialVersionUID = 1L; + + @Override + public Description matchMethod(MethodTree tree, VisitorState state) { + AnnotationTree annotation = + Iterables.getOnlyElement(HAS_METHOD_SOURCE.multiMatchResult(tree, state).matchingNodes()); + + return buildDescription(tree) + .setMessage( + MoreJUnitMatchers.getMethodSourceFactoryDescriptors(annotation, tree).toString()) + .build(); + } + } }