idea: check proper use of declaration-site type variance about forbidden-apis

[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.

[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]

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 :-/

[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>.

[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).