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!

[wbadam]

@msridhar, what are the chances of this being picked up? We at Canva are rolling out NullAway in our code base, and there's been pushback from some engineers finding it cumbersome to work around NullAway's limitation. Here's an example that's similar to the create example above:

String potentiallyNull = // ...
boolean definitelyNotNull = potentiallyNull != null;

if (definitelyNotNull) {
  int length = potentiallyNull.length();  // <-- NullAway warning
}

if (potentiallyNull != null) {
  int length = potentiallyNull.length();  // <-- No warning
}

We'd also be happy to contribute if that's the quickest way forward, just let me know how to help.

[msridhar]

@wbadam exciting to hear you are working on rolling out NullAway at Canva! Given the number of times this has come up, I would be open to trying to address cases like your example. Unfortunately it may not be a super-quick thing to implement. An over-simplified view is that right now, NullAway keeps track of a set of variables that are @Nullable and a set of variables that are @NonNull before and after each line of code in a method. We would need to change this representation so that we could track that a variable is @NonNull if some other condition holds, like some boolean variable is true. I was always hesitant to add such support since the dataflow analysis that computes this information is performance critical for NullAway, and we've always tried to keep the compile-time overhead of NullAway as low as possible, so like other Error Prone checks it can be run on every build. I think it would take some careful design and measurement to add this extra reasoning without compromising performance too much (it's ok to compromise a bit). Maybe we could use a flag to control whether the extra reasoning is enabled, but that might make the code excessively ugly, so hopefully we wouldn't need that.

Do you or one of your teammates feel willing / able to dig into this a bit? Having more help would definitely make the exploration go faster. I am slammed for at least the next couple of days and won't have time to look more closely. But I can try to give pointers to the relevant code.

[wbadam]

@msridhar certainly happy to look into it, a direction to the relevant code would be much appreciated!

[msridhar]

I have renamed this issue to focus on the false positives stemming from cases like those in #98 (comment).

@msridhar certainly happy to look into it, a direction to the relevant code would be much appreciated!

Thanks, @wbadam, I appreciate it. I still need to do some thinking and exploration as to the best way to support this. Right now, the state tracked by NullAway during local dataflow analysis is a NullnessStore, which maps access paths to their nullness state. To support this use case we need to instead track something like "conditional nullness", i.e., facts of the form, "if this variable is true/false, then this access path is nullable / non-null." This would involve updating the store data structure to be able to hold such conditional nullness facts (and to properly compute least upper bounds on the stores), updating the transfer functions in AccessPathNullnessPropagation to generate the facts, and also updating relevant APIs that query the dataflow analysis for nullness information at different program points. And, I would like to do this in a way that does not hurt NullAway performance too much in cases where conditional nullness facts do not need to be tracked.

I have some other deadlines and an upcoming holiday, so I won't be able to dig into this more until after mid-April. If anyone has cycles and wants to attempt some prototyping based on the above, feel free. Otherwise, I will take a look when I have some time available.

[cpovirk]

Our C++ colleagues at Google have done some work to handle variables like the definitelyNotNull example above. My understanding is that they're currently using satisfiability checking but that they are considering alternative approaches that might perform better, including essentially "inlining" potentiallyNull != null in place of usages of definitelyNotNull (potentially inlining further recursively). (I'm sure there would be caveats around cases in which potentiallyNull might change value, for example.) I'm told that one approach (similar but not identical to "inlining") is being demoed in llvm/llvm-project#82950.

[msridhar]

Thanks a lot @cpovirk! That's very interesting. The inlining approach is appealing in terms of trying to make the results of dataflow not depend on whether you store certain types of conditions in an (effectively final?) local variable or check them directly. I'm still unsure of the best way to proceed for NullAway. I hope to be able to put in more time on this soon.