Analysis Fails On Conditional Initialization

[naturalwarren]

void method(@Nullable Dep dep1, @Nullable Dep dep2) {
  Thing thing = null;
  if (dep1 == null || dep2 == null) {
    thing = new Thing();
  }
  if (dep1 == null) {
  // Passing thing as a parameter to a @NonNull API causes NullAway to report a nullability error 
  }
  if (dep2 == null) {
  // Passing thing as a parameter to a @NonNull API causes NullAway to report a nullability error 
  }
}

In the example above dep1 and dep2 are both nullable. If they're both present I want them to share the same thing. If neither are present I don't want to instantiate thing.

[msridhar]

Hrm, NullAway doesn't currently track this kind of conditional non-nullness. Adding it would be some work and it might significantly impact performance. Besides suppression, the one workaround I can think of is to put all the code using thing under the if condition where it is initialized. Is that a possibility?

[naturalwarren]

I can do that. I try to prevent wrapping if statements as much as possible for readability but for this case the nesting is only two levels so the impact is minimal.

Since conditional nullness is some work and might impact performance could we try to detect this scenario upfront and mention in the error message NullAway can't verify the expression is safe? I'm not sure how easy this would be.

An interesting datapoint is that Intellij didn't flag passing thing as a potential NPE. This may just be because it's optimistic for expressions it doesn't/can't evaluate.

[msridhar]

Not sure what IntelliJ does exactly; it may well track more precise info than we do. I don't immediately see a way to figure out when this case is happening in the error message without doing the analysis itself 😄 One thing we could do eventually is to run a more precise path-sensitive analysis only if our current analysis can't prove safety. Since the common case is no error, this shouldn't affect performance too much. Still not clear it's worth the implementation effort, though. We can keep this issue open and see if this kind of thing comes up frequently. FWIW, on our internal code base I don't think this code pattern is a major cause of warning suppressions (/cc @lazaroclapp)