False positive with field nullability stored in local variable

[ben-manes]

In this case, a @Nullable field is stored to a local variable. When the field is used in a null condition an error occurs, but if the variable is used then it does not. It seems the data flow analysis does not realize they are the same.

error: [NullAway] returning @Nullable expression from method with @NonNull return type
    return (writer == null) ? CacheWriter.disabledWriter() : castedWriter;
    ^

<K1 extends K, V1 extends V> CacheWriter<K1, V1> getCacheWriter() {
  @SuppressWarnings("unchecked")
  CacheWriter<K1, V1> castedWriter = (CacheWriter<K1, V1>) writer;
  return (writer == null) ? CacheWriter.disabledWriter() : castedWriter;
}

[msridhar]

Yeah, NullAway does not track local variable aliases so it misses this case. Like #35 I think we could support this eventually by doing a more precise analysis in cases where we are going to report an error. Going to mark this as low priority for now but it would be a good project to take on eventually

[msridhar]

BTW thanks for all the reports @ben-manes! Please keep them coming 😄

[ben-manes]

Unfortunately I think that I have to give up on the target project since it is non-idiomatic for performance reasons. It uses a lot of nullable fields that are enabled in certain configurations, as indicated by code generated subclasses. Since those usage paths can only occur when non-null, and NullAway cannot track deeply enough, many false positives are hard to easily resolve without turning the whole thing off. But it was a nice experiment to try this out!

[msridhar]

Ok no problem! One thing is if you control the code generator, you can tweak it to generate downcasts or other warning suppressions so you can still use NullAway on other more idiomatic parts of the code base. (The downcasts may not be tolerable if the code is super performance critical.) We'll keep working on improving NullAway in the meantime.

[ben-manes]

Yes, though the codegen is only for minimizing fields and the logic is in the base class. I would expect inlining to remove the downcasts, so it would be free. But the project has exceeded its complexity budget and this would add obscurity when already challenging. But this is a good trick to know if I decide to introduce NullAway to a work project. Thanks!

[ben-manes]

Making progress using your trick, so we'll see how it comes out :)

Another case where this issue occurs in the pattern,

boolean create = (foo == null);
if (!create) {
  foo.bar()
}

This scenario is about readability of the code by naming the condition when used multiple times in a method. Using the explicit check each time would be okay, but convey less when maintaining and need reparsing of what null means. But due to not tracking the invocation on foo is a false positive.

[msridhar]

Making progress using your trick, so we'll see how it comes out :)

Awesome! :) Regarding your create example, yeah, this would be good to handle too. If you have any rough categorization of how often this case happens vs. the first cast example that could be helpful for prioritization. The solutions are different: the cast example requires tracking equalities between variables / fields, while the create example requires tracking conditional (non-)nullness, facts like "if create is false then foo is not null.

[ben-manes]

In this case it was only once, but that is because I usually extract those out to query methods. Since method tracking isn't supported, that has been a largest case of false positives.

My present goal is to have it pass in order to review the benefits. There are a lot more suppressions than I'd like since inference is too locally scoped. But I also think it may have found one or two possible errors in a JSR adapter, so it will be worthwhile regardless. Since this requires more extensive annotations just like the Checker Framework does, I'm curious to run their analysis afterwards to see how it compares.

[ben-manes]

Everything works nicely! I had forgotten why the CheckerFramework is painful, so this is a nice compromise. :)

[msridhar]

Great, so glad you got things working! And thanks again for all the reports. We'll dig through them more in the new year.

Regarding the query methods, I think we can actually support those more easily with a new annotation, rather than requiring a library model. I'll re-open another issue specifically around that one.

[ben-manes]

Joy, integrated into my project. There are 50 targeted suppressions and neutral impact to code readability. Thanks for all of the hard work :)

[msridhar]

Joy, integrated into my project. There are 50 targeted suppressions and neutral impact to code readability. Thanks for all of the hard work :)

Awesome! Thanks for all your reports and feedback!