Introduce `More{ASTHelpers,JUnitMatchers,Matchers}` utility classes by eric-picnic · Pull Request #335 · PicnicSupermarket/error-prone-support

eric-picnic · 2022-11-04T15:59:08Z

This PR breaks out common changes from #319, #214, and #188, to allow them to be merged in any order.

rickie

Left a few comments and questions. Let me know what you think, we can discuss this as well Monday :).

Should we link to the class they are actually "extending"? In the Javadoc I mean. For example, should MoreJUnitMatchers in a way link to JUnitMatchers?

Sorry for maybe being a bit nitpicky on some of the things, but these classes can potentially be used a lot in the future 👀.

rickie · 2022-11-05T14:58:08Z

+        .collect(toImmutableList());
+  }
+
+  static boolean isMethodInEnclosingClass(String methodName, VisitorState state) {


I'd say it makes sense to just make this public right?

Either way, it would be nice to add a Javadoc here :).

rickie · 2022-11-05T14:58:41Z

+
+/** A set of JUnit-specific helpers for working with the AST. */
+public final class MoreJUnitMatchers {
+  /** Matches JUnit test methods. */


Maybe we should be more specific and call it JUnit Jupiter, WDYT? The things in here are quite JUnit 5 specific

rickie · 2022-11-05T15:04:11Z

+import java.util.Optional;
+import javax.lang.model.type.TypeKind;
+
+/** A set of JUnit-specific helpers for working with the AST. */


Just for reference, the Javadoc from JUnitMatchers itself:

Matchers for code patterns which appear to be JUnit-based tests.

I think mentioning AST here isn't as accurate as we can be 😬.

Suggested change

/** A set of JUnit-specific helpers for working with the AST. */

/** A set of JUnit Jupiter-specific helper methods and {@link Matcher Matchers}. */

rickie · 2022-11-05T15:07:42Z

+   *
+   * @param enclosingClass The class to search in.
+   * @param methodName The method name to search for.
+   * @return The method trees of the methods with the given name in the class.


Suggested change

* @return The method trees of the methods with the given name in the class.

* @return The {@link MethodTree}s of the methods with the given name found in the class.

Maybe even "in the enclosing class"? Or "given class".

rickie · 2022-11-05T15:09:19Z

+              isType("org.junit.jupiter.api.BeforeEach")));
+
+  /**
+   * Matches methods which have a {@link org.junit.jupiter.params.provider.MethodSource} annotation.


Suggested change

* Matches methods which have a {@link org.junit.jupiter.params.provider.MethodSource} annotation.

* Matches methods that have a {@link org.junit.jupiter.params.provider.MethodSource} annotation.

Right?

eric-picnic · 2022-11-07T12:41:00Z

Should we link to the class they are actually "extending"? In the Javadoc I mean. For example, should MoreJUnitMatchers in a way link to JUnitMatchers?

I think that could be helpful. 👍 I have pushed a commit addressing this as well as implementing your other suggestions.

rickie

Added a commit. It looks good to me! 🚀

Suggested commit message:

Introduce `More{ASTHelpers,JUnitMatchers,Matchers}` utility classes (#335)

rickie · 2022-11-07T13:52:24Z

+   *
+   * @param methodSourceAnnotation The {@link org.junit.jupiter.params.provider.MethodSource}
+   *     annotation to extract a method name from.
+   * @return The name of the factory methods referred to in the annotation if there is only one, or


s/methods/method singular is important here :). Reflowed the sentence a bit, not really happy with the word "Alternatively" that I pushed. Feel free to tweak.

rickie · 2022-11-07T13:53:22Z

+import javax.lang.model.type.TypeKind;
+
+/**
+ * A set of JUnit Jupiter-specific helper methods and {@link Matcher Matchers}, adding on to the


The first sentence is usually the summary in the Javadoc and we try to keep that as concise as possible. I feel the second part "deserves" it's own sentence.

rickie · 2022-11-07T13:54:28Z

+import com.sun.source.tree.Tree;
+import com.sun.tools.javac.code.Symbol;
+
+/** A collection of methods to enhance the use of {@link Matcher}s. */


Here we are missing a link to Matchers (not to mix up with Matchers that is used in the line above 😅).

Will add it.

rickie

Re-requested review because I think we should also update the usages where we can. Will add a commit.

Added a commit with some other changes. LMKWYT , happy to discuss :).

Oh w.r.t. the state.findEnclosing(ClassTree.class).getMembers(), we could add a checkState there if we want to 👀.

rickie · 2022-11-07T14:37:38Z

+ * A set of JUnit Jupiter-specific helper methods and {@link Matcher Matchers}, adding on to the
+ * ones from {@link com.google.errorprone.matchers.JUnitMatchers}.
+ */
+public final class MoreJUnitJupiterMatchers {


I'm a bit on the fence about the naming.
Although within the class it's a good idea to make explicit it's about JUnit Jupiter things, it makes sense to me to name the class inline with the upstream one, meaning we call it MoreJUnitMatchers. WDYT?

Additionally, in the near future expect more things will be added not necessarily specific to JUnit Jupiter.

rickie · 2022-11-07T14:46:25Z

+   * @param methodName The method name to search for.
+   * @return Whether there are any methods with the given name in the given class.
+   */
+  public static boolean isMethodInEnclosingClass(ClassTree enclosingClass, String methodName) {


Changed this back to a VisitorState as that is how it is used in the other place. That also holds for the usage we are introducing in #319. As passing VisitorState to ASTHelpers's methods is quite common, I think it does make sense here. WDYT?

Sorry @eric-picnic can you maybe repeat here why this was changed to a ClassTree :)?

oxkitsune · 2022-11-08T09:29:43Z

+   * Matches methods that have a {@link org.junit.jupiter.params.provider.MethodSource} annotation.
+   */
+  public static final Matcher<MethodTree> HAS_METHOD_SOURCE =
+      allOf(annotations(AT_LEAST_ONE, isType("org.junit.jupiter.params.provider.MethodSource")));


Suggested change

allOf(annotations(AT_LEAST_ONE, isType("org.junit.jupiter.params.provider.MethodSource")));

annotations(AT_LEAST_ONE, isType("org.junit.jupiter.params.provider.MethodSource"));

Wouldn't this be sufficient?

rickie · 2022-11-08T13:13:02Z

Latest changes LGTM! Will definitely make the other review easier 🚀 !

The PR is ready for your 👁️ 👁️ @Stephan202.

rickie · 2022-11-10T21:15:32Z

Rebased and added a commit.

We discussed offline that we also want to have tests for the new utilities. I created the first one, will work on the rest as well ~soon.

Haven't written many of these types of tests, so let's discuss what / how I can improve them 😄.

Stephan202

Rebased and added a commit. Test review is TBD.

Stephan202 · 2022-11-13T13:35:20Z

+  /**
+   * Extracts the name of the JUnit factory method from a {@link
+   * org.junit.jupiter.params.provider.MethodSource} annotation.
+   *
+   * @param methodSourceAnnotation The {@link org.junit.jupiter.params.provider.MethodSource}
+   *     annotation to extract a method name from.
+   * @return The name of the factory method referred to in the annotation. Alternatively, {@link
+   *     Optional#empty()} if there is more than one.
+   */
+  public static Optional<String> extractSingleFactoryMethodName(
+      AnnotationTree methodSourceAnnotation) {
+    ExpressionTree attributeExpression =
+        ((AssignmentTree) Iterables.getOnlyElement(methodSourceAnnotation.getArguments()))
+            .getExpression();
+    Type attributeType = ASTHelpers.getType(attributeExpression);
+    return attributeType.getKind() == TypeKind.ARRAY
+        ? Optional.empty()
+        : Optional.of(attributeType.stringValue());
+  }


I flagged and resolved several issues with this implementation in #188. IMHO we should port the full functionality here:

Proper support for multiple factory method names.

Defaulting to the name of the annotated method if no factory method name is explicitly specified.

Stephan202 · 2022-11-13T13:54:02Z

+   *     to be found.
+   * @return Whether there are any methods with the given name in the enclosing class.
+   */
+  public static boolean isMethodInEnclosingClass(String methodName, VisitorState state) {


I guess methodExitsInEnclosingClass would be a clearer name.

(Even so: I wonder whether this method sound exist as-is: in general we'd also want to match the arguments.)

Updated the name. Ah yes that makes sense, but let's do that once we need it.

Stephan202 · 2022-11-13T13:57:40Z

+   * @param state A {@link VisitorState} describing the context in which the given {@link Tree} is
+   *     to be found.


👀 There is no given tree.

Stephan202

Added another commit. Did not yet review MoreASTHelpersTest; as discussed offline we should likely have separate test BugCheckers for each exposed method.

Stephan202 · 2022-11-13T17:02:57Z

+  /**
+   * Matches methods that have a {@link org.junit.jupiter.params.provider.MethodSource} annotation.
+   */
+  public static final Matcher<MethodTree> HAS_METHOD_SOURCE =


Suggested change

public static final Matcher<MethodTree> HAS_METHOD_SOURCE =

public static final MultiMatcher<MethodTree, AnnotationTree> HAS_METHOD_SOURCE =

Stephan202 · 2022-11-13T17:11:36Z

+
+      return diagnosticsMessages.isEmpty()
+          ? Description.NO_MATCH
+          : buildDescription(tree).setMessage(String.join(", ", diagnosticsMessages)).build();


Instead of String.join we should just do .toString(), so that with BUG: Diagnostic contains we can verify that the enumeration doesn't contain unexpected additional checks.

Stephan202 · 2022-11-13T17:43:05Z

+  /**
+   * Returns the names 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 names.
+   * @return One or more value factory names.
+   */
+  static ImmutableSet<String> getMethodSourceFactoryNames(
+      AnnotationTree methodSourceAnnotation, MethodTree method) {
+    ExpressionTree value = AnnotationMatcherUtils.getArgument(methodSourceAnnotation, "value");
+
+    if (!(value instanceof NewArrayTree)) {
+      return ImmutableSet.of(ASTHelpers.constValue(value, String.class));
+    }
+
+    NewArrayTree arrayTree = (NewArrayTree) value;
+    return arrayTree.getInitializers().isEmpty()
+        ? ImmutableSet.of(method.getName().toString())
+        : arrayTree.getInitializers().stream()
+            .map(name -> ASTHelpers.constValue(name, String.class))
+            .collect(toImmutableSet());
+  }


Turns out this logic still isn't correct. Will fix and add tests for all cases.

rickie

Splitted the test cases in MoreASTHelpersTest. The changes LGTM @Stephan202 , nice improvements. Let's get this one in 😄.

Rebased and added a commit.

Stephan202

Rebased and added three commits (over-all diff is small).

Suggested commit message LGTM :)

Stephan202 · 2022-11-22T06:43:33Z

+   * <p>This includes annotations inherited from superclasses due to {@link
+   * java.lang.annotation.Inherited}.


This claim should be tested.

Edit: added a test; it fails. The issue is that this method is misnamed: it's about annotations, not meta-annotations. The single use case we have does look for a meta-annotation, but that's only because this method is apply to a tree that itself represents an annotation. Ideally we directly use Matchers#hasAnnotation, but for some reason that doesn't work (didn't check why).

Edit 2: but arguably this Matcher should apply only to AnnotationTrees, in which case @Inherited does not apply. Reverted test, dropped the comment.

Stephan202 · 2022-11-22T06:44:24Z

+   *
+   * @param <T> The type of tree to match against.
+   * @param annotationType The binary type name of the annotation (e.g.
+   *     "org.jspecify.nullness.Nullable", or "some.package.OuterClassName$InnerClassName")


The JSpecify annotations were recently moved (not yet in the version we're depending on here).

Suggested change

* "org.jspecify.nullness.Nullable", or "some.package.OuterClassName$InnerClassName")

* "org.jspecify.annotations.Nullable", or "some.package.OuterClassName$InnerClassName")

Stephan202 · 2022-11-22T06:56:13Z

+
+  private static String createDiagnosticsMessage(
+      BiFunction<String, VisitorState, Object> function, VisitorState state) {
+    return ImmutableSet.of("foo", "bar", "baz").stream()
+        .map(
+            methodName ->
+                String.join(": ", methodName, String.valueOf(function.apply(methodName, state))))
+        .collect(joining(", "));
+  }


Although we generally "call down", in case of inner classes calling methods on the outer class, we place the inner classes at the bottom.

Stephan202 · 2022-11-22T07:04:28Z

+        .addSourceLines(
+            "A.java",
+            "class A {",
+            "  // BUG: Diagnostic contains: foo: 1, bar: 2, baz: 0",


I guess we can make sure that if any new method name is added, that all tests are updated by adding start/end markers. This also makes createDiagnosticsMessage simpler; will push a proposal.

rickie · 2022-11-22T12:34:31Z

Last changes LGTM. Nice to see that adding the tests improved the quality and the validity of the code 👍🏻.

eric-picnic requested review from oxkitsune and rickie November 4, 2022 15:59

eric-picnic mentioned this pull request Nov 4, 2022

Introduce JUnit factory method BugChecker #319

Open

rickie requested changes Nov 5, 2022

View reviewed changes

rickie approved these changes Nov 7, 2022

View reviewed changes

rickie requested review from Stephan202 and rickie November 7, 2022 14:10

rickie approved these changes Nov 7, 2022

View reviewed changes

oxkitsune approved these changes Nov 8, 2022

View reviewed changes

eric-picnic force-pushed the eric-picnic/more-matcher-classes branch 2 times, most recently from 54e7619 to a1c3556 Compare November 8, 2022 12:09

rickie mentioned this pull request Nov 8, 2022

Drop redundant Matchers.{allOf,anyOf} containing a single Matcher #340

Closed

2 tasks

rickie added the improvement label Nov 8, 2022

rickie added this to the 0.6.0 milestone Nov 8, 2022

rickie force-pushed the eric-picnic/more-matcher-classes branch from 6443b04 to ff44df7 Compare November 10, 2022 20:32

rickie self-requested a review November 10, 2022 20:32

rickie force-pushed the eric-picnic/more-matcher-classes branch 2 times, most recently from 5e2ab41 to c841481 Compare November 10, 2022 21:15

Stephan202 force-pushed the eric-picnic/more-matcher-classes branch from 541df42 to 6bcc3bd Compare November 13, 2022 14:08

Stephan202 reviewed Nov 13, 2022

View reviewed changes

Stephan202 mentioned this pull request Nov 13, 2022

Introduce JUnitValueSource check #188

Merged

rickie force-pushed the eric-picnic/more-matcher-classes branch from 3494a88 to 36c710d Compare November 18, 2022 13:38

rickie approved these changes Nov 18, 2022

View reviewed changes

eric-picnic and others added 3 commits November 22, 2022 06:53

Create JUnit matchers as preparation for additional JUnit BugCheckers

f03a686

Address review comments

b827884

Suggestions

8cbbe88

rickie and others added 13 commits November 22, 2022 06:53

Suggestions

eaef658

Address review comments

9f69d0c

Address review comments

3048ef4

Introduce MoreASTHelpersTest

bacb045

Introduce MoreJUnitMatchersTest

00ddd17

Introduce MoreMatcherTest and additional cleanup

cd614f3

Suggestions

160a2db

Suggestions

88e926e

Suggestions (2)

98babf2

Split the test cases in MoreASTHelpersTest and assorted improvements

0ad7bbb

Suggestions

b17d298

Introduce failing test

1299c5a

Revert failing test, change main code

0e7276b

Stephan202 force-pushed the eric-picnic/more-matcher-classes branch from 7f1dd5e to 0e7276b Compare November 22, 2022 07:23

Stephan202 approved these changes Nov 22, 2022

View reviewed changes

rickie changed the title ~~Create JUnit matchers as preparation for additional JUnit BugCheckers~~ Introduce More{ASTHelpers,JUnitMatchers,Matchers} utility classes Nov 22, 2022

rickie merged commit 84d425e into master Nov 22, 2022

rickie deleted the eric-picnic/more-matcher-classes branch November 22, 2022 12:35

oxkitsune added a commit that referenced this pull request Nov 22, 2022

Use utilities introduced in #335

a512faa

oxkitsune added a commit that referenced this pull request Nov 22, 2022

Use JUnit utilities introduced in #335

5b5dd64

Stephan202 pushed a commit that referenced this pull request Nov 27, 2022

Use utilities introduced in #335

5fa10c9

oxkitsune added a commit that referenced this pull request Dec 1, 2022

Use utilities introduced in #335

96c234c

Stephan202 pushed a commit that referenced this pull request Dec 3, 2022

Use utilities introduced in #335

4c53bf0

	/** A set of JUnit-specific helpers for working with the AST. */
	/** A set of JUnit Jupiter-specific helper methods and {@link Matcher Matchers}. */

	* @return The method trees of the methods with the given name in the class.
	* @return The {@link MethodTree}s of the methods with the given name found in the class.

	* Matches methods which have a {@link org.junit.jupiter.params.provider.MethodSource} annotation.
	* Matches methods that have a {@link org.junit.jupiter.params.provider.MethodSource} annotation.

	allOf(annotations(AT_LEAST_ONE, isType("org.junit.jupiter.params.provider.MethodSource")));
	annotations(AT_LEAST_ONE, isType("org.junit.jupiter.params.provider.MethodSource"));

		* @param state A {@link VisitorState} describing the context in which the given {@link Tree} is
		* to be found.

	public static final Matcher<MethodTree> HAS_METHOD_SOURCE =
	public static final MultiMatcher<MethodTree, AnnotationTree> HAS_METHOD_SOURCE =

		* <p>This includes annotations inherited from superclasses due to {@link
		* java.lang.annotation.Inherited}.

	* "org.jspecify.nullness.Nullable", or "some.package.OuterClassName$InnerClassName")
	* "org.jspecify.annotations.Nullable", or "some.package.OuterClassName$InnerClassName")

Conversation

eric-picnic commented Nov 4, 2022

Uh oh!

rickie left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eric-picnic commented Nov 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rickie left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rickie left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rickie commented Nov 8, 2022

Uh oh!

rickie commented Nov 10, 2022

Uh oh!

Stephan202 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Stephan202 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rickie left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Stephan202 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rickie commented Nov 22, 2022

eric-picnic commented Nov 7, 2022 •

edited

Loading

rickie left a comment •

edited

Loading

rickie left a comment •

edited

Loading