Adding @PolyNull to generic type hierarchy including subclass specialization for a particular type

[Hunter Knepshield]

Hi all,

I have an existing library that provides an API like this:

static IFoo<String> stringValue(String defaultValue) { ... }

static IStringSetFoo stringSetValue(ImmutableSet<String> defaultValue) { ... }

interface IFoo<T> {

  T get();

  void put(T t);

}

/** Change the type of #get to ImmutableSet since it represents something on-disk that was just read. */

interface IStringSetFoo extends IFoo<Set<String>> {

  ImmutableSet<String> get();

}

Conceptually, the defaultValue parameters to the stringValue and stringSetValue methods can be @Nullable, but IFoo#put can not take null. So I'm looking at making approximately this change:

static IFoo<@PolyNull String> stringValue(@PolyNull String defaultValue) { ... }

// Here's where things start to get weird - where do we specify @PolyNull in the return type?

static IStringSetFoo stringSetValue(@PolyNull ImmutableSet<String> defaultValue) { ... }

interface IFoo<@PolyNull T> {

  @PolyNull T get();  // If defaultValue was null, this might return null; otherwise, @NonNull

  void put(@NonNull T t);  // This can never take null

}

interface IStringSetFoo extends IFoo<@PolyNull Set<String>> {

  @PolyNull ImmutableSet<String> get();

}

This slightly tweaked code compiles fine and the nullness checker on the library itself passes, but when I try to use this from a client and run the checker:

IStringSetFoo subclassNonNull = stringSetValue(ImmutableSet.of());

IStringSetFoo subclassNullable = stringSetValue(null);

static void doThingWithSubclass(IStringSetFoo foo) { foo.get(); }

static void doThingWithSuperclass(IFoo<Set<String>> foo) { foo.get(); }

static void doThingWithSuperclassNullable(IFoo<@Nullable Set<String>> foo) { foo.get(); }

static void doThingWithSuperclassNonNull(IFoo<@NonNull Set<String>> foo) { foo.get(); }

static void doThingWithSuperclassPolyNull(IFoo<@PolyNull Set<String>> foo) { foo.get(); }

// What happens when we use the implicit @NonNull variant?

doThingWithSubclass(subclassNonNull);  // OK

// found   : @Initialized @NonNull IStringSetFoo

// required: @Initialized @NonNull IFoo<@Initialized @NonNull Set<@Initialized @NonNull String>>

doThingWithSuperclass(subclassNonNull);  // Should conceptually be fine

// found   : @Initialized @NonNull IStringSetFoo

// required: @Initialized @NonNull IFoo<@Initialized @Nullable Set<@Initialized @NonNull String>>

doThingWithSuperclassNullable(subclassNonNull);  // Should conceptually be fine

// found   : @Initialized @NonNull IStringSetFoo

// required: @Initialized @NonNull IFoo<@Initialized @NonNull Set<@Initialized @NonNull String>>

doThingWithSuperclassNonNull(subclassNonNull);  // Should conceptually be fine

doThingWithSuperclassPolyNull(subclassNonNull);  // OK

// What about the @Nullable variant?

doThingWithSubclass(subclassNullable);  // OK

// found   : @Initialized @NonNull IStringSetFoo

// required: @Initialized @NonNull IFoo<@Initialized @NonNull Set<@Initialized @NonNull String>>

doThingWithSuperclass(nullable);  // Should probably conceptually be an error, because there's an implicit @NonNull on the generic type in the method signature

// found   : @Initialized @NonNull IStringSetFoo

// required: @Initialized @NonNull IFoo<@Initialized @Nullable Set<@Initialized @NonNull String>>

doThingWithSuperclassNullable(subclassNullable);  // Should conceptually be fine

// found   : @Initialized @NonNull IStringSetFoo

// required: @Initialized @NonNull IFoo<@Initialized @NonNull Set<@Initialized @NonNull String>>

doThingWithSuperclassNonNull(subclassNullable);  // Certainly a real error

doThingWithSuperclassPolyNull(subclassNullable);  // OK

I think this is all due to the fact that the covariance of IStringSetFoo erases the generic type parameter that IFoo uses, so there's nothing for @PolyNull to "latch on to" in IStringSetFoo usages. How can I go about resolving this? It feels like @HasQualifierParameter(Nullable.class) is ultimately what I want to add to IStringSetFoo (and all its concrete subclasses), but I can't seem to find the right combination of annotations to satisfy the nullness checker.

Thanks,

/Hunter

[Michael Ernst]

Hunter-

I'm sorry you are having trouble.  Thanks for getting in touch.

One problem is that the annotations you have written don't make sense.  You wrote

interface IFoo<@PolyNull T> { ... }

but the manual section on polymorphic qualifiers says "If you can use generics, you typically do not need to use a polymorphic qualifier such as @PolyNull" and also says that polymorphic qualifiers such as @PolyNull "may not be used on a class declaration."
(Regarding the latter proscription, the Checker Framework apparently does not warn you about that illegal usage.  Perhaps this misled you.  Sorry about that!  I opened an issue to inform users about such misuses in the future.)

Another problem is that you seem to have started the annotation process from client methods and then tried (in your words) "to find the right combination of annotations to satisfy the nullness checker."  Section 2.4.1 in the manual recommends against this approach, which generally leads to frustration and nonsense annotations.  

Instead, I suggest starting with the abstraction, such as a class or method.  Don't even think about client calls until the abstraction is specified.

In your case, there is the interface IFoo<T>.  What does that represent, and what should T be?  Does IFoo represent/wrap a possibly-null value or a non-null value?  From your message, it seems that T should be a non-null type.  (My guess might be wrong.  I realize that you simplified your code, which has benefits such as making the bug report shorter and concealing confidential details.  But the missing details might be important to the specification.)

So, you should declare IFoo to be instantiable only with a non-null type argument:

interface IFoo<T extends @NonNull Object> {
  @Nullable T get();
  void put(T t);
}

Notice that you don't need any polymorphic qualifiers at all.  Java generics do all the work for you.

(You could also write interface IFoo<T extends Object> { ... } without the @NonNull annotation, since "Object" is the same as "@NonNull Object".  This is explained in section 28.1.2 in the manual.)

Declaring IStringSetFoo is trickier.  You wrote

interface IStringSetFoo extends IFoo<@PolyNull Set<String>> {
  @PolyNull ImmutableSet<String> get();
}

but note again that that is illegal syntax.

Here, there is nowhere in the type IStringSetFoo to write a type qualifier on Set<String>, because Set<String> does not appear in the type IStringSetFoo.
This is exactly what class qualifier parameters are for (see section 28.3 of the manual).  So, you will want to write @HasQualifierParameter on the declaration of IStringSetFoo.  The manual section "Class qualifier parameters" tells you how.

Generics and polymorphism can be tricky in regular Java, and even more so in a richer type system.

I think you have enough information to proceed with a better specification of the types, and from there you can proceed to the clients.  Please ask if you have questions that are not answered (or not answered clearly) in the manual.

Not every code pattern can be typechecked without warnings.
Another approach would be to write out IFoo<Set<String>> rather than `IStringSetFoo`; the former permits you to write annotations anywhere you like.

-Mike

--

---
You received this message because you are subscribed to the Google Groups "Checker Framework discussion" group.
To unsubscribe from this group and stop receiving emails from it, send an email to checker-framework-...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/checker-framework-discuss/274b0678-4eec-4c89-8be2-04d06fd66609n%40googlegroups.com.

[Hunter Knepshield]

Hi Mike, thanks for the pointers. I believe I've gotten the library into shape now with your suggestions and helpful links. One additional question for you at the bottom.

> In your case, there is the interface IFoo<T>.  What does that represent, and what should T be?

T is essentially a simple value type (e.g. boolean, int, String, Set<String>) stored on disk, and that data is associated with a particular key for lookup. The data on-disk is always @NonNull, but if the key isn't present in the underlying file, we also take a fallback default value to be returned when the lookup fails, which may be null if the caller wants it to be. So overall, the nullability of T is dictated by the nullability of the defaultValue parameter in e.g. the #stringValue method to create the IFoo instance.

The trick was not putting @PolyNull or @Nullable on IFoo's generic type parameter at all like you mention. This is how the API netted out:

interface IFoo<T> {
  T get();

  void put(@NonNull T t);

}

/** Change the type of #get to ImmutableSet since it represents something on-disk that was just read. */

@HasQualifierParameter(Nullable.class)

interface IStringSetFoo extends IFoo<@PolyNull Set<String>> {
  @PolyNull ImmutableSet<String> get();
}

static IFoo<@PolyNull String> stringValue(@PolyNull String defaultValue) { ... }

static IStringSetFoo stringSetValue(@PolyNull ImmutableSet<String> defaultValue) { ... }

Additional question: IFoo has another method I hadn't previously considered:

interface IFoo<T> {

  T get();

  void put(@NonNull T t);

  // The returned value has the same nullability as T

  default String getValueString() {

    T value = get();

    if (value == null) {

      return null;

    }

    return value.toString();

  }

}

Is there a way I can make the nullability of getValueString's return type inherit the nullability of IFoo's generic type parameter T? Currently I'm just declaring the method to return @Nullable String, but that's too broad. If T is @NonNull, then so is #getValueString's return. Writing @PolyNull doesn't seem to be the right thing in this case, since it's not associated with T in any way and IFoo doesn't need @HasQualifierParameter on it. Conceptually I'm looking to write something like @InheritQualifierFrom<T>(Nullable.class) String, though I know that's invalid Java.

/Hunter

You received this message because you are subscribed to a topic in the Google Groups "Checker Framework discussion" group.
To unsubscribe from this topic, visit https://groups.google.com/d/topic/checker-framework-discuss/PvBqo9I5phg/unsubscribe.
To unsubscribe from this group and all its topics, send an email to checker-framework-...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/checker-framework-discuss/CAAJCdQTmB5vOqB5Ey9VwbVGmwJpzm6objxS%3DHOLd4BY08rsbpg%40mail.gmail.com.

[Hunter Knepshield]

Yep, it seemed like a pretty obscure/uncommon pattern. Just leaving getValueString returning a @Nullable String is fine for our usages, since it's intended for human-readable logging and not as critical as null correctness on the actual #get value.

/Hunter

On Thu, Apr 1, 2021 at 9:07 PM Michael Ernst <mer...@cs.washington.edu> wrote:

Hunter-

I'm glad that is working now.  The current Checker Framework release (3.12.0) warns if polymorphic annotations are written on type argument declarations or on class headers (that is, outside the body).

There is not a way to declare that two unrelated types have the same nullness, without using @HasQualifierParameter.  That would require a new form of polymorphism beyond what the Checker Framework supports.  It has come up before, but not frequently enough to justify the implementation effort and the added complexity in the manual and in the implementation.  I suggest suppressing warnings if needed.

Mike