Generic-typed fields include erroneous non-null check

[kennknowles]

Discovered during apache/beam#16721 here is a summary example:

@AutoValue
public abstract class ValueInSingleWindow<T> {
  T getValue();
  ... other fields ...
}

It is valid to create ValueInSingleWindow<@Nullable String> in the usual way via ValueInSingleWindow.<@Nullable String>of(null, ...).

However, autovalue creates a check that the value is non-null, rejecting type-safe code.

[eamonnmcmanus]

Do you have a suggestion for what you think should happen? The generated code can't know whether T is @Nullable.

Perhaps we could say that if there is a @Nullable bound (class ValueInSingleWindow<T extends @Nullable Object>) then we treat T as @Nullable, under the assumption that static checking will eliminate incorrect null values. That use of bounds would be consistent with Checker Framework and JSpecify. In those systems, ValueInSingleWindow<@Nullable String> is not allowed if the declaration is class ValueInSingleWindow<T> without the @Nullable bound.

[kennknowles]

T is a type parameter that can be instantiated either to either nullable or non-nullable types. Autovalue isn't capable of knowing, indeed, so must treat all values as a black box.

[kennknowles]

You clearly have a good analysis, however the default is not what you think. From that page:

The Nullness type system is an example.

class MyClass<T> == class MyClass<T extends @Nullable Object>
class MyClass<T extends Object> == class MyClass<T extends @NonNull Object>

The rationale for this choice is:

The “<T>” in MyClass<T> means “fully unconstrained”, and the rules maintain that, without the need for a programmer to change existing code.
The “Object” in MyClass<T extends Object> is treated exactly like every other occurrence of Object in the program — it would be confusing for different occurrences of Object to mean different annotated types.

So a bare type variable <T> can be instantiated to any type at all, including nullable types.

[eamonnmcmanus]

Oh dear. That's an inconsistency between Checker Framework and JSpecify, then. Regardless, <T extends @Nullable Object> does mean that T can be a @Nullable type in both systems. So people could at least write that if they ran into this issue.

[kennknowles]

Yea, that's a helpful workaround. Glad we had this exchange!

[eamonnmcmanus]

To be clear, they can't currently employ that workaround, because AutoValue doesn't currently have logic to treat T foo() as @Nullable if <T extends @Nullable Object>. I do think we should add that logic, and I plan to do so fairly soon. (Unfortunately the compiler treatment of type annotations isn't always very reliable, but I think this case will probably work.)

[rkraneis]

Hi @eamonnmcmanus, any news on the planned implementation?