Fully enable NullAway:JSpecifyMode checking #2270
Conversation
Generate changelog in
|
✅ Successfully generated changelog entry!What happened?Your changelog entries have been stored in the database as part of our migration to ChangelogV3. Need to regenerate?Simply interact with the changelog bot comment again to regenerate these entries. |
schlosna
force-pushed
the
davids/NullAway-JSpecifyMode
branch
from
54101d3 to
acffc69
Compare
| static class CompositeInvocationContext extends DefaultInvocationContext { | ||
|
|
||
| private final InvocationContext[] contexts; | ||
| private final @Nullable InvocationContext @NonNull [] contexts; |
There was a problem hiding this comment.
with jspecify this is equivalent to saying that the contexts array itself is not null; however, the InvocationContext elements may be null.
There was a problem hiding this comment.
These are pretty confusing to be honest. I can't imagine a better way to annotate these (maybe it would have been nicer to have a @NullableElements annotations, but I don't imagine these is one). For what it's worth, I've always assumed the annotation to apply to the field itself, so I would likely naturally read this the exact opposite (@Nullable applying to the field, and @NonNull applying to the elements in the array)
It's clearer when you read through https://jspecify.dev/docs/user-guide/#type-use-annotation-syntax but can certainly get folks to shoot themselves in the foot :(
It's unfortunate that this pattern is in so many places, as I think a quick comment explaining what this means would be great here.
For instance, below,
@Nullable
InvocationContext @NonNull [] getContexts() {
clearly reads to me as the return value of getContexts()` can be null.
There was a problem hiding this comment.
Does it work if we move the @NonNull annotation to the field instead?
There was a problem hiding this comment.
I'm going to switch this back to draft as there are some places that were not previously annotated correctly and some places where NullAway does not seem to properly handle propagation with arrays in jspecify mode right now.
| String key = keys[i]; | ||
| values[valuesIndex] = key; | ||
| values[valuesIndex + 1] = data.get(key); | ||
| values[valuesIndex + 1] = Strings.nullToEmpty(data.get(key)); |
There was a problem hiding this comment.
this is a slight behavior change where previously we may have allowed null values through, we now convert to an empty string tag value "", though previously we would likely trigger a NullPointerException when copying the tag map to an ImmutableMap<String, String>
There was a problem hiding this comment.
I don't like silently replacing null with the empty string. These are semantically different values and this implicit conversion can be confusing for consumers.
| static class CompositeInvocationContext extends DefaultInvocationContext { | ||
|
|
||
| private final InvocationContext[] contexts; | ||
| private final @Nullable InvocationContext @NonNull [] contexts; |
There was a problem hiding this comment.
These are pretty confusing to be honest. I can't imagine a better way to annotate these (maybe it would have been nicer to have a @NullableElements annotations, but I don't imagine these is one). For what it's worth, I've always assumed the annotation to apply to the field itself, so I would likely naturally read this the exact opposite (@Nullable applying to the field, and @NonNull applying to the elements in the array)
It's clearer when you read through https://jspecify.dev/docs/user-guide/#type-use-annotation-syntax but can certainly get folks to shoot themselves in the foot :(
It's unfortunate that this pattern is in so many places, as I think a quick comment explaining what this means would be great here.
For instance, below,
@Nullable
InvocationContext @NonNull [] getContexts() {
clearly reads to me as the return value of getContexts()` can be null.
| @Override | ||
| @Nullable | ||
| public final Object invoke(Object proxy, Method method, @Nullable Object[] nullableArgs) throws Throwable { | ||
| public final Object invoke(Object proxy, Method method, Object @Nullable [] nullableArgs) throws Throwable { |
There was a problem hiding this comment.
This is an example of what I described in my other comment as being confusing. And makes me wonder if places like
are correct or notThere was a problem hiding this comment.
I think this should actually be:
| public final Object invoke(Object proxy, Method method, @Nullable Object[] nullableArgs) throws Throwable { | |
| public final Object invoke(Object proxy, Method method, Object @Nullable [] nullableArgs) throws Throwable { | |
| public final Object invoke(Object proxy, Method method, @Nullable Object @Nullable [] nullableArgs) throws Throwable { |
3 participants
Before this PR
We were not using the full JSpecify mode of NullAway and several packages were not fully
@NullMarked.After this PR
==COMMIT_MSG==
All tritium packages are fully
@NullMarkedwith jspecify@Nullableor@NonNullannotations as needed by APIs. Enable JSpecify mode for stricter NullAway nullness checking.See https://github.com/uber/NullAway/wiki/JSpecify-Support and https://github.com/uber/NullAway/wiki/How-NullAway-Works#jspecify
==COMMIT_MSG==
Possible downsides?