idea: check proper use of declaration-site type variance

[vlsi]

See:

• @In/@Out annotations for declaration-site variance -- kind of jspecify/jspecify#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 forbidden-apis: Function, Predicate, BiFunction, Consumer, Supplier, and so on.

Here a recent case:

• Make nestable condition factory accept conditions on super types assertj/assertj#2905

WDYT?

See also:

• Discussion in OpenRewrite: idea: check and fix use of declaration-site type variance openrewrite/rewrite#2621
• Discussion in Error Prone: idea: check proper use of declaration-site type variance google/error-prone#3711

[Stephan202]

Great idea. In terms of "how to implement": likely Error Prone is more suitable for such a check (with the added benefit that it can automatically emit appropriate fixes).

NB: A low-false positive check would have to both consider method overrides, as well as constraints imposed by the APIs (if any) to which the Function (etc.) parameters are be passed.

[vlsi]

with the added benefit that it can automatically emit appropriate fixes

I incline OpenRewrite might be slightly better at suggesting the fixes.
However, suggesting a fix is complicated as adding variance to the parameter would likely require changes subsequent code: it might require adding variance to the fields (e.g. if the Function is passed in constructor and stored to a field), it might require adding variance to the local variables, and so on.

likely Error Prone is more suitable for such a check

Technically speaking, the set of checks for Error Prone and forbidden-apis overlap, however, I find that forbidden-apis is lightweight (it does not require extra compiler, etc), so I consider forbidden-apis as a reasonable location for the check even though I understand there will be no "automatic code fix".

However, I agree variance check might be a stretch for forbidden-apis goals.
On the other hand, Function<A, B> is used a lot, and it would be nice if the invariant usages could be detected early.

NB: A low-false positive check would have to both consider method overrides, as well as constraints imposed by the APIs (if any) to which the Function (etc.) parameters are be passed.

You are right. It should probably avoid cases like Consumer<? extends String>.

[Stephan202]

I incline OpenRewrite might be slightly better at suggesting the fixes.

Perhaps; as a long-time user of Error Prone and one of the primary devs behind Error Prone Support I'm likely biased. ;)

I do agree that OpenRewrite is better at handling the transitive (cross-compilation unit) rewrites that may be required for an automated fix.

[vlsi]

Perhaps; as a long-time user of Error Prone and one of the primary devs behind Error Prone Support I'm likely biased. ;)

Ah, nice. I tend adding Error Prone to all Java projects I'm working with (I'm quite happy with performance and output messages), so thanks for keeping it afloat. It is sad error-prone does not work for Kotlin though :-/

[uschindler]

Hi,
the problem is that the information you are requesting may not be available in the bytecode. Forbiddenapis is for parsing mathod invocations in bytecode, but it does not handle the java syntax.

I am not sure if this can be implemented in forbiddenapis at all, as this is not related to "forbidden method invocations".

[uschindler]

Great idea. In terms of "how to implement": likely Error Prone is more suitable for such a check (with the added benefit that it can automatically emit appropriate fixes).

NB: A low-false positive check would have to both consider method overrides, as well as constraints imposed by the APIs (if any) to which the Function (etc.) parameters are be passed.

I agree with that.

Focus on forbiddenapis is to quickly parse bytecode and detect invalid patterns (method invocations and class usage from there). Because it works on bytecode/class files, it is good at parsing output of any compiler (so kotlin and scala is known to work perfectly with forbiddenapis, Groovy also kinda works), but the limitation to bytecode checks makes it not a good match for such type-based checks that require source code or more detailed type information that is erased from byte code.

I am not fully sure, but for method declarations in class files bytecode saves the so-called "signature" including generics as an attribute, but at moment this one is not checked. So there is possiblility to check the usage of those classes in code, but its another code path than general forbidden-apis checks as implemented at moment.

[vlsi]

the problem is that the information you are requesting may not be available in the bytecode

Generic signatures are very well available on the bytecode, see https://docs.oracle.com/javase/specs/jvms/se7/html/jvms-4.html#jvms-4.7.9

[vlsi]

Because it works on bytecode/class files, it is good at parsing output of any compiler (so kotlin and scala is known to work perfectly with forbiddenapis, Groovy also kinda works), but the limitation to bytecode checks makes it not a good match for such type-based checks that require source code or more detailed type information that is erased from byte code.

That is exactly why I believe adding check in forbidden-apis would be great for both Java and Kotlin users.

Suppose someone adds a function in Kotlin, and they receive java.util.function.Function as a parameter.
Since java.util.function.Function has no declaration-site variance, Kotlin does not automatically generate Function<? super....
So Kotlin users need to remember to use Function<in INPUT, out OUTPUT> when using Java-based interfaces.

In other words, the check would be useful for Java, Kotlin, and Scala, users, and it would be non-trivial to achieve in Error Prone, and OpenRewrite.

Focus on forbiddenapis is to quickly parse bytecode and detect invalid patterns

What I suggest here is to parse signatures of methods only, so it would be way faster than the current "parse method body".
However, it might require "superclass" analysis (e.g. to check if a method overrides another method).