Update to NullAway 0.14.0 and fix new warnings - #37188
Conversation
Signed-off-by: Manu Sridharan <msridhar@gmail.com>
| * @param conversionService the conversion service | ||
| * @param targetType the target type | ||
| */ | ||
| @SuppressWarnings("NullAway") // Retain support for comparators that handle a null conversion result |
There was a problem hiding this comment.
Here is Codex's explanation for this warning suppression. I read through it and it makes sense to me, but I'm not an expert on this code. I'm not sure what is the best fix (that is not a suppression).
The suppression covers a real mismatch between the legacy runtime behavior and the declared generic types.
The constructor adapts a ConversionService into a Converter:
public ConvertingComparator(
Comparator<T> comparator,
ConversionService conversionService,
Class<? extends T> targetType) {
this(comparator,
new ConversionServiceConverter<>(conversionService, targetType));
}The delegated constructor expects:
ConvertingComparator(Comparator<T>, Converter<S, T>)But ConversionService.convert(...) is explicitly nullable:
<T> @Nullable T convert(@Nullable Object source, Class<T> targetType);Consequently, the adapter is:
Converter<S, @Nullable T>NullAway correctly observes that this cannot safely become Converter<S, T>. During comparison, the result is passed directly to the comparator:
T converted = this.converter.convert(source);
return this.comparator.compare(converted1, converted2);If conversion returns null:
- A null-aware comparator such as
Comparator.nullsFirst(...)handles it correctly. - A comparator that does not support null will likely throw
NullPointerException.
The existing API allows both possibilities and leaves responsibility with the supplied comparator. That relationship is not accurately represented by the constructor’s Comparator<T> type.
The comment therefore means:
Do not force the conversion result to be non-null, because existing callers may intentionally supply a comparator that supports null.
For example, adding this would satisfy NullAway:
return Objects.requireNonNull(
this.conversionService.convert(source, this.targetType));But it would change behavior by rejecting null before a null-aware comparator could process it.
Accurately modeling this would require a larger API redesign, likely requiring this constructor to produce a ConvertingComparator<S, @Nullable T> and accept a comparator whose input type is nullable. Constructors cannot independently change the enclosing class’s type argument, so a static factory or internal class redesign would probably be needed.
Thus, this suppression is:
- Narrowly scoped to the affected constructor.
- Preserving established runtime behavior.
- Not claiming that NullAway is wrong.
- Documenting that null handling is delegated to the caller-provided comparator.
| } | ||
|
|
||
| nullability { | ||
| nullAwayVersion = "0.14.0" |
There was a problem hiding this comment.
Alternately, we could wait for a new nullability plugin release that defaults to 0.14.0. The other changes would remain valid.
Most of the new warnings concern making upper bounds of type variables
@Nullablefor consistency, and the fixes are straightforward. There is one case where we suppress a warning, as I'm unsure of the desired fix; I will add a more detailed PR comment on that change.