[Generics] Switching the order of extended interfaces alters the result of @NonNull annotation analysis

[stephan-herrmann]

• report unchecked warning for unannotated 'this' argument
• recover the actual contract that introduces an unmet constraint

Partly fixes #4297

[stephan-herrmann]

This PR fixes some but not all problems observed in #4297

Let's classify use cases:

The common situation consists of two independent interfaces that declare methods which are equivalent in terms of JLS, but have different annotations (which JLS doesn't care about). The following considerations hold if those annotations are null annotations evaluated by ecj; 3rd party annotations are out off scope here:

• Already in released versions we force any class explicitly implementing both interfaces to provide an implementation that is conform to all inherited contracts. For any parameters with different null annotations, the class will be forced to declare a @Nullable parameter (or unannotated for the price of an unchecked warning). With this the implementation will accept all values that are legal by any of the contracts. ✔️
• ecj doesn't check this for an interface extending both interfaces (see [Generics] Switching the order of extended interfaces alters the result of @NonNull annotation analysis #4297 (comment)), but still any class implementing that combined interface will be checked by the above rule. ✔️
• Similarly, an intersection type a & b isn't immediately checked for conflicts (see the original [Generics] Switching the order of extended interfaces alters the result of @NonNull annotation analysis #4297 (comment)), but the above two items should "normally" ensure that no implementation of such an intersection type exists (modulo an implementation with unchecked warnings) this part was wrong, back to [Generics] Switching the order of extended interfaces alters the result of @NonNull annotation analysis #4297 (comment). but any implementation of the intersection type is checked ( ✔️ )
• There is a bug highlighted by @fedejeanne's witness from [Generics] Switching the order of extended interfaces alters the result of @NonNull annotation analysis #4297 (comment) so we need to specifically check method references / lambdas as implementations for a type with null conflicts. This is what the current PR addresses. ❌

We need to further distinguish the position of the ambiguous annotation:

This variant produces a warning already in 4.39:

import org.eclipse.jdt.annotation.*;

public class GenericsOrder {
    public <T extends b & a> void test(@NonNull T my) {
        my.method(null);
    }

    public <T extends I> void test(@NonNull T my) {
        my.method(null); // The call goes through here
    }

    public static void main(String[] args) {
        new GenericsOrder().test(GenericsOrder::mImpl); // problem against regular param 's'
    }

    public static int mImpl(@NonNull String s) { return 0; }
}

interface a { int method(@NonNull String a); }

interface b { int method(String a); }

interface I extends b, a { }

ecj complains:

1. WARNING in /tmp/GenericsOrder.java (at line 13)
        new GenericsOrder().test(GenericsOrder::mImpl); // problem against regular param 's'
                                 ^^^^^^^^^^^^^^^^^^^^
Null type safety: parameter 1 provided via method descriptor b.method(String) needs unchecked conversion to conform to '@NonNull String'

This part is independent of interface order. ✔️

There still is order dependence inside both test() methods, complaining about the null argument in one case, but not with swapped order. This, however, can be seen as a secondary problem, as the declared parameter type is inconsistent, and there is no clean way to invoke either of the test() methods.

The original witness from #4297 (comment), however, adds one more trick to the mix: here we are passing a reference to an instance method, where the first parameter of the function type will be implicitly mapped to a this receiver of the target method. For this situation released versions of ecj only checked if the function type declared a @Nullable parameter, which would trigger NPE against the method receiver. We failed to complain if the receiver is mapped to an unannotated parameter.

This is the main fix in this PR: warn when an unannotated parameter is mapped to the receiver of a reference to an instance method.

That witness will now raise

1. WARNING in /tmp/GenericsOrder.java (at line 13)
        new GenericsOrder().test(String::length);
                                 ^^^^^^^^^^^^^^
Null type safety: parameter 'this' provided via method descriptor b.method(String) needs unchecked conversion to conform to '@NonNull String'

Swapping interface order will still change warnings against the bodies of test() methods, but it is no longer possible to make a "clean" call to test(), so, as argued above, the implementation of test() is of reduced interest.

As a side issue, the PR also fixes the exact message in this warning (and its sibling error in case of @Nullable instead of unannotated): Previously, we could mention the wrong contract a.method(String), instead of b.method(String) due to the underlying arbitrary selection. This is fixed by searching (again) for the contract that has the exact parameter type we're complaining about.

With this I believe the issue is resolved for method reference as argument to an inconsistent functional interface. 👍

When using the lambda form of the same example, ecj has no extra knowledge about a this parameter that definitely must be nonnull. Hence we keep silent about dereferencing the lambda argument, just as we always do for unannotated variables with no flow information. If @Nullable is added for b.method() then the expected error is raised. Anyway, this case is not order dependent 😄

[stephan-herrmann]

I was about to propose this fix for RC1, but at a closer look we are fixing an extreme corner case, depending on all of these conditions:

• an interface (explicit or implicit as an intersection type) inherits several equivalent abstract methods, which differ only in annotations
• those annotations are null annotations interpreted by ecj
• the method is invoked with a reference to an instance method
• the implicit this parameter will be mapped to a parameter of the function type, for which a @NonNull and an unannotated version exist, but no inherited contract declares it as @Nullable.
• interfaces are listed in that order, which will prefer the unannotated parameter over the @NonNull one

In this very specific case a relevant problem is not reported, thus hiding the fact that no conflict-free implementation of that functional interface is possible.

The bigger problem is that JLS is silent about all this, and thus for 3rd party annotations, for which ecj has no insider knowledge, order dependence cannot be avoided without a fix in JLS.

In that light I guess this fix can wait till after 4.40 GA.