idea: check and fix use of declaration-site type variance

[vlsi]

See:

• https://github.com/jspecify/jspecify/issues/72

Currently, Java requires use-site type variance, so if someone has Function<IN, OUT> method parameter, it should rather be Function<? super IN, ? extends OUT>.

Unfortunately, it is not easy to notice that ? super and ? extends is missing, so it would be nice if there was a tool that could detect missing variance and suggest adding it.

The list of well-known classes could be hard-coded within OpenRewrite: Function, Predicate, BiFunction, Consumer, Supplier, and so on.

Here is a recent case:

• https://github.com/assertj/assertj/pull/2905

WDYT?

Corner cases:

• https://github.com/JetBrains/intellij-community/blob/cd82cab9aff47787e684b4a58be902d1b917f264/plugins/InspectionGadgets/src/com/siyeh/ig/style/BoundedWildcardInspection.java
• https://youtrack.jetbrains.com/issue/IDEA-235069/False-positive-in-Can-use-bounded-wildcard
• https://github.com/JetBrains/intellij-community/blob/cd82cab9aff47787e684b4a58be902d1b917f264/plugins/InspectionGadgets/src/com/siyeh/ig/style/VarianceUtil.java#L125-L133

See also:

• Discussion in Error Prone: https://github.com/google/error-prone/issues/3711
• Discussion in forbidden-apis: https://github.com/policeman-tools/forbidden-apis/issues/216

[jkschneider]

I'm a big fan of this for a set of hard-coded well-known classes.

[jkschneider]

Are there also a series of types for which we wouldn't add variance? For example: Function<String, Car> should be Function<String, ? extends Car> not Function<? super String, ? extends Car> right?

[vlsi]

I think so. However, I suggest discussing meta-issues in jspecify issue (the first link) to avoid splitting the questions/findings into several issues ;)

[jkschneider]

Automatically added variance should skip type parameters that are final classes as well.

[jkschneider]

Note that when a method overrides another method, it cannot use declaration-site type variance without changing the overridden method. Nor can on overridding method remove declaration-site type variance when overriding one that has it.

interface Test {
    void test(Function<List, List> f);
}

class TestImpl implements Test {
    @Override
    public void test(Function<? super List, ? extends List> f) {
    }
}

[vlsi]

A class can become non-final, so having variance for final classes might be helpful. However, it is unlikely String can become open, so ? extends String is probably useless

[jkschneider]

You can tinker with the current form of this recipe at https://public.moderne.io/recipes/org.openrewrite.java.cleanup.CommonDeclarationSiteTypeVariances.

[vlsi]

I think Consumer<? super String> should be suggested even though String is final, so rewrite should recommend wildcards even for final classes in IN positions

[jkschneider]

In the latest commit I added an option excludeFinalClasses so you can control whether variance is added to final classes.

[vlsi]

What I mean is ? super variance should be added for https://github.com/google/error-prone/issues/3711#issuecomment-1377732638 https://github.com/google/error-prone/issues/3711#issuecomment-1377732638 all the classes no matter if they are final or not.

? super Object is special: https://github.com/google/error-prone/issues/3711#issuecomment-1377732638

[jkschneider]

By setting excludeFinalClasses to false (which is the default), ? super variance will be added for all the classes no matter if they are final.

[vlsi]

What is the point of skipping ? super variance if somebody sets excludeFinalClasses=true?

[jkschneider]

I'm just coming around on this -- my intuition was originally that ? super Integer just looked overly pedantic? And that maybe there wouldn't be any harm in going part way. But maybe I'm just wrong altogether.

[vlsi]

Not really. Consider

void act(Consumer<Integer> intConsumer) { }
...
Consumer<Object> tst=...;
act(tst); <-- this will fail to compile.
// act(Consumer<? super Integer> ...) will heal the case

[jkschneider]

I see, so you were literally meaning just the contravariant case? Makes sense. Should we avoid ? extends on final classes?

[vlsi]

I am not sure regarding ? extends for final classes. For instance, Supplier<? extends String> would look weird, so it might make sense to use simpler signatures like Supplier<String>, especially for well-known final classes that will never become open.

I guess it might be great to have an option like suppressOutVarianceForClasses:..., and put String, Integer, there by default. Disabling out variance for non-jdk classes sounds too fragile.

[vlsi]

An unfortunate case is List which should be List<IN> for read-only case, and List<INVARIANT> for read-write (Kotlin's MutableList). Automatic fix sounds complicated for that case

[jkschneider]

An unfortunate case is List

And similar for every collection type likely...

[jkschneider]

Honestly these results are looking pretty good https://public.moderne.io/results/aX1Tl. Once b47d9b34cb5b4742ebc73f0028569f90c61e0df7 finishes building we'll have the ability to suppress out variance for final classes (as well as in and out variance for a specified set of excluded glob patterns of fully qualified types).

[jkschneider]

One more thing: this recipe shouldn't modify any parameter that is assigned to a field in the method body.

[vlsi]

Why? On contrary, it should modify the type of the field as well.

[jkschneider]

For completeness yes. Trying to limit complexity somewhat. The assignment may be to a base class coming from a binary dependency, but easy enough to check for.

[vlsi]

Iterable<OUT>
Iterator<OUT>
Stream<OUT>

[vlsi]

Can the recipie be suppressed for a particular placeholder?

E.g. Consumer<@SuppressWarnings("wildcards") String>

[vlsi]

-MicrometerCollector collector, HistogramSupport histogramSupport, Supplier<Exemplar[]> exemplarsSupplier,
+MicrometerCollector collector, HistogramSupport histogramSupport, Supplier<? extends Exemplar[]> exemplarsSupplier,

It looks like a false-positive. One can't inherit arrays, so ? extends NonNullableArrayType should definitely be suppressed.

However, Supplier<? extends Exemplar @Nullable []> should indeed be converted to Supplier<? extends Exemplar @Nullable []>. In other words, if we accept Supplier that produces Exemplar[] or a null reference, then we should accept even such suppliers that produce non-null Exemplar[].

Just in case, Supplier<@Nullable Exemplar[]> should be kept intact as @Nullable in that position is related to the element type.

The same goes for String: Supplier<@Nullable String> should be converted to Supplier<? extends @Nullable String>.

[vlsi]

One more idea: I guess the recepie should work both ways: if it detects Supplier<? extends String>, then it should suggest simplifying the type to Supplier<String>.

[kunli2]

I found there is already a recipe there: DeclarationSiteTypeVariance. so closing this out.