Potential false positive in complex method

[ben-manes]

I have a complex method that, sadly, seems worse when broke apart. So while none of my tests fail, I cannot be certain myself that there is not a bug.

BoundedLocalCache.java:589: error: [NullAway] dereferenced expression candidate is @Nullable
        candidate.getPreviousInAccessOrder();
                 ^

However, the loop terminates earlier on the condition that both candidate and victim are null. This error is reported in a block when the victim is null, so therefore the candidate cannot be. I would consider this a perfect example where I'd want to trust a data flow analysis over my own, but I don't think it is correct. Due to the reassignments and looping, it may have gotten confused.

if (victim == null) {
  candidates--;
  Node<K, V> evict = candidate;
  candidate = candidate.getPreviousInAccessOrder();
  evictEntry(evict, RemovalCause.SIZE, 0L);
  continue;
} else if (candidate == null) { ... }

[msridhar]

Reading the code, I agree that the access is safe. The invariant that holds just before the if condition is (victim != null) || (candidate != null). Since under the if we know that victim == null, we also know candidate != null. This kind of logical reasoning is beyond what NullAway's data flow analysis can do right now. But, as discussed in #98, eventually we could add a more precise data flow analysis that runs when the existing analysis reports an error, and that more precise analysis could catch this case as well, depending on how it is designed.

Going to mark as low priority for now, though it'd be great to handle this case eventually.