EEAs applied incorrectly when using "Inherit null annotations" with NonNullByDefault

[sebthom]

Given:

1. An EEA file for Map containing:

   class java/util/Map
    <K:Ljava/lang/Object;V:Ljava/lang/Object;>
   
   get
    (Ljava/lang/Object;)TV;
    (Ljava/lang/Object;)T0V;

2. "Inherit null annotations" compiler option enabled

3. A test class with @NonNullByDefault

Then in the following example the compiler correctly determines that Map#get can return null values but incorrectly determines that HashMap#get or AbstractMap#get will always return non-null values.

package test;

import java.util.AbstractMap;
import java.util.HashMap;
import java.util.Map;

import org.eclipse.jdt.annotation.NonNullByDefault;

public class Test {

    @NonNullByDefault
    public static void main(final String[] args) {

        final HashMap<Object, Object> hashmap = new HashMap<>();
        if (hashmap.get("") == null) { // compiler warning: Redundant null check: comparing '@NonNull Object' against null
        }

        final AbstractMap<Object, Object> abstractmap = new HashMap<>();
        if (abstractmap.get("") == null) { // compiler warning: Redundant null check: comparing '@NonNull Object' against null
        }

        final Map<Object, Object> map = new HashMap<>();
        if (map.get("") == null) { // no warning
        }
    }
}

I am using Eclipse 2023-03 with these null analysis settings:

[stephan-herrmann]

Thanks for the clear report.

ecj handles different kinds of "implicit" annotations (i.e., not visible in the immediate source code):

• annotations from @NonNullByDefault
• external annotations
• inherited annotations
• annotations propagated through instantiation of type parameters

Unfortunately, we haven't specified how exactly those should interact in the given example.

What we know:

• Map.get() should be seen as @Nullable V get(Object) from external annotations
• Variable hashmap has type HashMap<@NonNull Object, @NonNull Object> from applying @NonNullByDefault

The exact contract of hasmap.get() depends on the order / precedence of evaluation:

• currently the implementation first substitutes type parameters to yield @NonNull Object get(@NonNull Object)
  • at this point the method is fully annotated and no null annotations will be inherited (this only happens for unannotated types)
• you seem to want the opposite: first propagate the external annotation from Map.get() to HashMap.get().
  • then we'd find the generic method in HashMap<K,V> to be @Nullable V get(Object), in which case instantiating V as @NonNull Object would create a conflict with the "explicit" @Nullable. For this particular case we implemented a "merge" such that the explicit annotation will win.

The first things I can say:

• I intentionally did not enable inheritance of null annotations by default, because I am convinced that the clarity of explicit annotations outweighs the burden of writing a few more annotations
• changing the semantics for this example is quite a challenge in our implementation, I need to take another look to assess whether this can be done without breaking things elsewhere.

For now you have several options:

• add .eaa for HashMap.get(), too.
• use the type Map wherever possible like you do in the last part of your example

[sebthom]

Very enlightening insights. Thanks for your prompt and thorough response!

[sebthom]

I thought a bit more about it.

If I am replicating the example from above with directly applying a @Nullable annotation to code the result is different.

package test;

import org.eclipse.jdt.annotation.*;

public class Test2 {

    interface Map<K, V> {
        @Nullable V get(Object key);
    }

    static abstract class AbstractMap<K, V> implements Map<K, V> {
        @Override
        public V get(final Object key) {
            return (V) null;
        }
    }

    static class HashMap<K, V> extends AbstractMap<K, V> {  }

    @NonNullByDefault
    public static void main(final String[] args) {
        final HashMap<String, Object> hashmap = new HashMap<>();
        if (hashmap.get("") == null) { // NO error: Redundant null check: comparing '@NonNull Object' against null
        }

        final AbstractMap<String, Object> abstractmap = new HashMap<>();
        if (abstractmap.get("") == null) { // NO error: Redundant null check: comparing '@NonNull Object' against null
        }

        final Map<String, Object> map = new HashMap<>();
        if (map.get("") == null) {
        }
    }
}

Isn't the idea that applying a nullable/nonnull annotation in code has the same effect as applying it via an EEA file?

[stephan-herrmann]

I thought a bit more about it. [...]

Thanks.

By tweaking this test a bit further I got to the bottom of the problem: Put Map and friends in separate files and compile separately, to force that compilation of the main method will use the binary (.class) version of them. Then you'll see the problems even with the explicit annotation. :(

Turns out the following combination doesn't work:

• a generic class using its type variables in method signatures
• compile the generic class separately from its client
• try to leverage inheritance of null annotations

Remove any of these factors and the error will go away.

If that could be fixed, perhaps the case of external annotations would automatically work, too.

[agentgt]

FWIW inheritance of annotations seems to be problematic in general across multiple null analysis tools. JSpecify has decided not support it and I agree with Stephan that I prefer the explicit annotation approach.

I wonder how bad it would be to remove that feature (inheritance of null annotations)? @sebthom do you rely on inheritance of null annotations?

[sebthom]

@agentgt I was, but I now changed the EEA generator/updater of https://github.com/vegardit/no-npe to do the inheritance analysis and propagate inherited null annotations to the generated EEA files.