Introduce `MoreTypes` utility class #234

Stephan202 · 2022-09-11T18:07:57Z

Suggested commit message:

Introduce `MoreTypes` utility class (#234)

The static methods of this class allow one to construct complex types,
against which expression types can subsequently be matched.

I'll also push these changes to #224, to show how they can simplify that PR.

Stephan202 · 2022-10-02T12:39:44Z

Rebased and resolved conflict.

rickie · 2022-10-19T17:12:03Z

I'm wondering whether we should make the check that uses MoreTypes more generic. We can have a checker that contains a list of Suppliers that specify types that we don't want to have nested. Now we (almost) have two checks that uses this type. I think we can come up with many more types that we want to disallow.

Within that check, we could give different error messages based on the type.

Instead of double nested types like Optional<Optional<?>> we could (just as an example) disallow Optional<Flux<?>>. Apart from the weird example, we can maybe think of some use cases.

We could also do this later though.

(Started reviewing this one).

rickie

Added a commit.

Have some suggestions and questions 😄.
The PR itself is very cool! Nice one to review :).

rickie · 2022-10-20T07:46:12Z

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/util/MoreTypes.java

+  private MoreTypes() {}
+
+  /**
+   * Creates a supplier of the type with the given fully qualified name.


Any reason for not using {@link Supplier}? :)

rickie · 2022-10-20T07:47:45Z

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/util/MoreTypes.java

+/**
+ * A set of helper methods which together define a DSL for defining {@link Type types}.
+ *
+ * <p>These methods are meant to be statically imported. Example usage:


Let's add this class to StaticImport.

rickie · 2022-10-20T07:50:22Z

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/util/MoreTypes.java

+   * @return A supplier which returns the described type if available in the given state, and {@code
+   *     null} otherwise.
+   */
+  public static Supplier<Type> supOf(Supplier<Type> type) {


Suggested change

public static Supplier<Type> supOf(Supplier<Type> type) {

public static Supplier<Type> superOf(Supplier<Type> type) {

I think this makes the difference between sub and sup a bit more clear. WDYT?

I liked the symmetry, but: sure :)

rickie · 2022-10-20T07:58:19Z

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/util/MoreTypes.java

+  // method name. But the DSL wouldn't look as nice that way.
+  @SafeVarargs
+  @SuppressWarnings("varargs")
+  public static Supplier<Type> generic(Supplier<Type> type, Supplier<Type>... typeArgs) {


Suggested change

public static Supplier<Type> generic(Supplier<Type> type, Supplier<Type>... typeArgs) {

public static Supplier<Type> genericWithParams(Supplier<Type> type, Supplier<Type>... typeArgs) {

To me, generic could be a bit more descriptive. One needs to really dive into the MoreTypes before understanding how this works. genericWithParams would be a nice option.
Another option would be genericType{,WithParams} with String as first parameter. However, in the current situation we always pass Types as parameter.

I know MoreTypes.genericType(...... has a little duplication, but we statically import every method here of course.

But the DSL wouldn't look as nice that way.

IMHO the DSL is very concise (even if we would make some of the method names a little longer).

The given type should be a generic type

In EP I see checks for this:

boolean isGeneric = typeParams != null && !typeParams.isEmpty();

Maybe it'd be nice to add a check for that in this method? Using checkArgument we can enforce that others use this DSL correctly.

I'm not yet feeling the first suggestion: in practice the code will look like generic(type("fqcn"), ...), which should be clear enough. The class also has only a few methods, and there's an example at the top, which shows how this method can be used.

W.r.t. the second suggestion: since the type evaluation is deferred, I'm not sure an IllegalArgumentException is warranted; perhaps an IllegalStateException would fit. Even so: the type might not have a matching set of generic type parameters due to an unexpected type on the classpath (perhaps due to an unsupported dependency version), which IMHO should not cause an error, but simply cause the expression to yield false.

rickie · 2022-10-20T08:45:12Z

error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/util/MoreTypesTest.java

+final class MoreTypesTest {
+  private static final ImmutableSet<Supplier<Type>> TYPES =
+      ImmutableSet.of(
+          type("java.lang.Nonexistent"),


Maybe we should have a test that verifies use cases like this:

private static Stream<Arguments> incorrectTypesTestCases() { return Stream.of( arguments(type("")), arguments(type("java.lang.Nonexistent")), arguments(generic(type("java.util.Integer"), unbound()))); ... }

The first one throws an exception. The third can be added to the list here, as that will verify that (a) it doesn't match and (b) doesn't throw an exception. (Which I think it shouldn't, for the reason mentioned above. That will of course feel weird for java.util.Integer, but not for custom.package.TypeThatWasRefactoredToHaveADifferentNumberOfTypeArgs.)

The first one throws an exception.

Together with the other proposed change, that would be exactly what we could test 😄.

I now understand why it wouldn't be nice to add these extra checks, so I'm fine to leave it as is :).

rickie · 2022-10-20T09:02:48Z

Rebased and added a small commit.

Stephan202

Rebased and added a commit. Tnx for the critical review!

We could also do this later though.

Yeah, let's defer the generalization idea; need to think how useful/desirable that is (for example, on the face of it disallowing Flux<Flux<?>> seems to make sense, until one realizes that this type is produced with Flux#groupBy. The same might hold in a few other cases.)

Stephan202 · 2022-10-21T21:43:07Z

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/util/MoreTypes.java

+  private MoreTypes() {}
+
+  /**
+   * Creates a supplier of the type with the given fully qualified class name.


It doesn't need to be a class.

Suggested change

* Creates a supplier of the type with the given fully qualified class name.

* Creates a supplier of the type with the given fully qualified name.

Stephan202 · 2022-10-21T21:52:28Z

.github/workflows/build.yaml

@@ -20,7 +20,7 @@ jobs:
      - name: Check out code
        uses: actions/checkout@v3.1.0
      - name: Set up JDK
-        uses: actions/setup-java@v3.5.1
+        uses: actions/setup-java@v3.6.0


We have some stray commits here; will rebase.

Stephan202 · 2022-10-21T22:08:23Z

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/util/MoreTypes.java

+  // method name. But the DSL wouldn't look as nice that way.
+  @SafeVarargs
+  @SuppressWarnings("varargs")
+  public static Supplier<Type> generic(Supplier<Type> type, Supplier<Type>... typeArgs) {


I'm not yet feeling the first suggestion: in practice the code will look like generic(type("fqcn"), ...), which should be clear enough. The class also has only a few methods, and there's an example at the top, which shows how this method can be used.

Stephan202 · 2022-10-21T22:13:06Z

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/util/MoreTypes.java

+  // method name. But the DSL wouldn't look as nice that way.
+  @SafeVarargs
+  @SuppressWarnings("varargs")
+  public static Supplier<Type> generic(Supplier<Type> type, Supplier<Type>... typeArgs) {


W.r.t. the second suggestion: since the type evaluation is deferred, I'm not sure an IllegalArgumentException is warranted; perhaps an IllegalStateException would fit. Even so: the type might not have a matching set of generic type parameters due to an unexpected type on the classpath (perhaps due to an unsupported dependency version), which IMHO should not cause an error, but simply cause the expression to yield false.

Stephan202 · 2022-10-21T22:18:43Z

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/util/MoreTypes.java

+   * @return A supplier which returns the described type if available in the given state, and {@code
+   *     null} otherwise.
+   */
+  public static Supplier<Type> supOf(Supplier<Type> type) {


I liked the symmetry, but: sure :)

Stephan202 · 2022-10-21T22:23:28Z

error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/util/MoreTypesTest.java

+final class MoreTypesTest {
+  private static final ImmutableSet<Supplier<Type>> TYPES =
+      ImmutableSet.of(
+          type("java.lang.Nonexistent"),


The first one throws an exception. The third can be added to the list here, as that will verify that (a) it doesn't match and (b) doesn't throw an exception. (Which I think it shouldn't, for the reason mentioned above. That will of course feel weird for java.util.Integer, but not for custom.package.TypeThatWasRefactoredToHaveADifferentNumberOfTypeArgs.)

rickie

Thanks for the explanations!

Tweaks look good to me 😄.

rickie · 2022-10-23T15:27:08Z

error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/util/MoreTypesTest.java

+final class MoreTypesTest {
+  private static final ImmutableSet<Supplier<Type>> TYPES =
+      ImmutableSet.of(
+          type("java.lang.Nonexistent"),


The first one throws an exception.

Together with the other proposed change, that would be exactly what we could test 😄.

I now understand why it wouldn't be nice to add these extra checks, so I'm fine to leave it as is :).

ibabiankou

Looks good 👍 Don't have much to add, nothing caught my eye 🤷‍♂️

ibabiankou · 2022-10-25T09:25:58Z

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/util/MoreTypes.java

+            return null;
+          }
+
+          return state.getType(baseType, /* isArray= */ false, params);


isArray is always false because Suppliers.arrayOf(...) should be used to describe that type, right?

It's been a while since I wrote this, but indeed, I think Suppliers.arrayOf(...) would then be used to wrap the result of this method. 👍

The static methods of this class allow one to construct complex types, against which expression types can subsequently be matched.

Stephan202

Rebased; will merge once built.

Stephan202 · 2022-10-25T14:59:09Z

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/util/MoreTypes.java

+            return null;
+          }
+
+          return state.getType(baseType, /* isArray= */ false, params);


It's been a while since I wrote this, but indeed, I think Suppliers.arrayOf(...) would then be used to wrap the result of this method. 👍

Stephan202 added this to the 0.2.1 milestone Sep 11, 2022

Stephan202 requested a review from rickie September 11, 2022 18:07

Stephan202 force-pushed the sschroevers/introduce-moretypes branch from 7cda41b to 7990365 Compare September 18, 2022 14:05

rickie modified the milestones: 0.3.0, 0.4.0 Sep 19, 2022

Stephan202 force-pushed the sschroevers/introduce-moretypes branch from 7990365 to c267b8f Compare October 2, 2022 12:38

rickie modified the milestones: 0.4.0, 0.5.0 Oct 5, 2022

Stephan202 force-pushed the sschroevers/introduce-moretypes branch from c267b8f to 7602247 Compare October 15, 2022 11:55

rickie requested a review from werli October 19, 2022 15:41

rickie added the improvement label Oct 19, 2022

rickie requested changes Oct 20, 2022

View reviewed changes

Stephan202 force-pushed the sschroevers/introduce-moretypes branch from 26a75f2 to df41c50 Compare October 21, 2022 22:32

Stephan202 commented Oct 21, 2022

View reviewed changes

rickie approved these changes Oct 23, 2022

View reviewed changes

rickie requested review from oxkitsune and ibabiankou and removed request for werli October 23, 2022 15:40

ibabiankou approved these changes Oct 25, 2022

View reviewed changes

rickie removed the request for review from oxkitsune October 25, 2022 12:27

Stephan202 and others added 4 commits October 25, 2022 16:55

Introduce MoreTypes utility class

89c665c

The static methods of this class allow one to construct complex types, against which expression types can subsequently be matched.

Suggestions

8aa280e

Tweaks

d181337

Tweaks

79ee60b

Stephan202 force-pushed the sschroevers/introduce-moretypes branch from df41c50 to 79ee60b Compare October 25, 2022 15:07

Stephan202 commented Oct 25, 2022

View reviewed changes

Stephan202 merged commit 92f2b0a into master Oct 25, 2022

Stephan202 deleted the sschroevers/introduce-moretypes branch October 25, 2022 15:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce `MoreTypes` utility class #234

Introduce `MoreTypes` utility class #234

Stephan202 commented Sep 11, 2022 •

edited

Loading

Stephan202 commented Oct 2, 2022

rickie commented Oct 19, 2022

rickie left a comment

rickie Oct 20, 2022

rickie Oct 20, 2022

rickie Oct 20, 2022

Stephan202 Oct 21, 2022

rickie Oct 20, 2022

rickie Oct 20, 2022

rickie Oct 20, 2022

Stephan202 Oct 21, 2022

Stephan202 Oct 21, 2022

rickie Oct 20, 2022

Stephan202 Oct 21, 2022

rickie Oct 23, 2022

rickie commented Oct 20, 2022

Stephan202 left a comment

Stephan202 Oct 21, 2022

Stephan202 Oct 21, 2022

Stephan202 Oct 21, 2022

Stephan202 Oct 21, 2022

Stephan202 Oct 21, 2022

Stephan202 Oct 21, 2022

rickie left a comment

rickie Oct 23, 2022

ibabiankou left a comment

ibabiankou Oct 25, 2022

Stephan202 Oct 25, 2022

Stephan202 left a comment

Stephan202 Oct 25, 2022

	public static Supplier<Type> supOf(Supplier<Type> type) {
	public static Supplier<Type> superOf(Supplier<Type> type) {

	public static Supplier<Type> generic(Supplier<Type> type, Supplier<Type>... typeArgs) {
	public static Supplier<Type> genericWithParams(Supplier<Type> type, Supplier<Type>... typeArgs) {

	* Creates a supplier of the type with the given fully qualified class name.
	* Creates a supplier of the type with the given fully qualified name.

Introduce MoreTypes utility class #234

Introduce MoreTypes utility class #234

Conversation

Stephan202 commented Sep 11, 2022 • edited Loading

Stephan202 commented Oct 2, 2022

rickie commented Oct 19, 2022

rickie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rickie commented Oct 20, 2022

Stephan202 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rickie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ibabiankou left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Stephan202 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Introduce `MoreTypes` utility class #234

Introduce `MoreTypes` utility class #234

Stephan202 commented Sep 11, 2022 •

edited

Loading