JSpecify JDK models shadow an inherited library model: HashMap.getOrDefault is @Nullable where Map.getOrDefault is not #1729

Version

NullAway 0.14.0, and current master (21f02dd). Error Prone 2.50.0, JDK 21.

Flags: -XepOpt:NullAway:JSpecifyMode=true -XepOpt:NullAway:JSpecifyJDKModels=true -XDaddTypeAnnotationsToSymbol=true. JSpecifyJDKModels defaults to JSpecifyExperimental.

Summary

Map.getOrDefault(key, nonNullDefault) reads as non-null, which is right. The same call through HashMap, LinkedHashMap, Hashtable, or ConcurrentHashMap reads as @Nullable. So the static type at the call site decides, and declaring a field HashMap<K, V> rather than Map<K, V> turns a clean call into a finding.

-XepOpt:NullAway:JSpecifyJDKModels=false makes every one of them clean, so the JSpecify JDK models are what introduce it.

Reproducer

package foo;

import java.util.HashMap;
import java.util.Hashtable;
import java.util.LinkedHashMap;
import java.util.Map;
import java.util.TreeMap;
import java.util.concurrent.ConcurrentHashMap;
import org.jspecify.annotations.NullMarked;

@NullMarked
class Test {
  static String viaInterface(Map<Long, String> m, long k) {
    return m.getOrDefault(k, "");
  }

  static String viaTreeMap(TreeMap<Long, String> m, long k) {
    return m.getOrDefault(k, "");
  }

  static String viaHashMap(HashMap<Long, String> m, long k) {
    // BUG: Diagnostic contains: returning @Nullable expression from method with @NonNull return type
    return m.getOrDefault(k, "");
  }

  static String viaLinkedHashMap(LinkedHashMap<Long, String> m, long k) {
    // BUG: Diagnostic contains: returning @Nullable expression from method with @NonNull return type
    return m.getOrDefault(k, "");
  }

  static String viaHashtable(Hashtable<Long, String> m, long k) {
    // BUG: Diagnostic contains: returning @Nullable expression from method with @NonNull return type
    return m.getOrDefault(k, "");
  }

  static String viaConcurrentHashMap(ConcurrentHashMap<Long, String> m, long k) {
    // BUG: Diagnostic contains: returning @Nullable expression from method with @NonNull return type
    return m.getOrDefault(k, "");
  }
}

Map and TreeMap are clean; the other four report. TreeMap and AbstractMap inherit Map.getOrDefault rather than declaring their own, which lines up with what follows.

What appears to be going on

The non-null result comes from the built-in library model, LibraryModelsHandler.NULL_IMPLIES_NULL_PARAMETERS:

.put(methodRef("java.util.Map", "getOrDefault(java.lang.Object,V)"), 1)

That model is inherited: with the JDK models switched off, every class above resolves through it and reports nothing.

jspecify-jdk.astubx then supplies a direct entry on each overriding class. Dumping the parsed cache gives identical records for the interface and for the two classes:

java.util.Map:V getOrDefault(java.lang.Object,V)          ==> {-1=[Nullable], 0=[Nullable], 1=[Nullable]}
java.util.HashMap:V getOrDefault(java.lang.Object,V)      ==> {-1=[Nullable], 0=[Nullable], 1=[Nullable]}
java.util.LinkedHashMap:V getOrDefault(java.lang.Object,V) ==> {-1=[Nullable], 0=[Nullable], 1=[Nullable]}

(-1 is the return.) TreeMap and AbstractMap have no entry.

So on java.util.Map the built-in NULL_IMPLIES_NULL model still wins over the astubx @Nullable return, and on a subclass the astubx entry wins instead. Whichever of the two is intended to take precedence, the same call should not resolve differently depending on the static type it goes through.

The comment already sitting next to that model shows the shape of the problem is known:

// We add ImmutableMap.getOrDefault explicitly, since when
// AcknowledgeRestrictiveAnnotations is enabled, the explicit annotations in the code
// override the inherited library model

Enumerating the JDK's Map implementations the same way would paper over getOrDefault, but the interaction is general — see #85 for the long-standing request to consider inheritance when checking models, and #723 for the original Map.getOrDefault support.

Expected

A call resolves to the same nullness whichever static type it goes through, since every override here has the same contract as the interface method it implements.

Workaround

Declare the variable as Map, or call get and handle null:

BitSet edges = map.get(key);
return edges == null ? new BitSet() : edges;

Where we hit it

HyperGraph keeps four HashMap<Long, BitSet> fields and reads them with getOrDefault(key, new BitSet()) at 14 call sites, all of which report. Found while replacing the Checker Framework with NullAway in Apache Calcite (apache/calcite#5213).