Allow tracking non-static field accesses outside the this instance

[lazaroclapp]

We took the decision, back around the origins of NullAway, to not track arbitrary field-dereferences for nullness propagation. Instead, NullAway expects field accesses to be: a) specifically on fields of the current instance object (i.e. this), b) on static fields (e.g. constants/enums), or c) made through getters.

This means that, for example, the following code would produce a surprising NullAway error:

if (foo.o == null) {
    foo.o = new Object();
}
return foo.o.toString();

Assuming field o of Foo is @Nullable and an instance field, the code above will cause NullAway to complain that foo.o.toString() might be dereferencing a "nullable" expression foo.o, despite the check right before it. This is because NullAway doesn't track any access paths of the form (argument/local)+field, whereas it does track (argument/local)+method call. This is unintuitive.

While getters are good practice in Java, they aren't universal. This is already a problem for JDK11 codebases and only likely to become more obvious when dealing with JDK17 records.

This PR attempts to relax this requirement.

[lazaroclapp]

@msridhar Given that this revisits a five years old design decision in NullAway, and I am not sure if I am missing any pitfalls, I don't plan to land this until I have had a chance to test this is in our internal codebases, and ideally after you are back from vacation and can provide a review. But I don't see or remember an obvious reason to not support these kind of access paths 😅

[coveralls]

Pull Request Test Coverage Report for Build #892

• 0 of 0 changed or added relevant lines in 0 files are covered.
• 13 unchanged lines in 1 file lost coverage.
• Overall coverage increased (+0.07%) to 92.526%

---

Files with Coverage Reduction	New Missed Lines	%
../nullaway/src/main/java/com/uber/nullaway/dataflow/AccessPathNullnessPropagation.java	13	90.42%

Totals
Change from base Build #891:	0.07%
Covered Lines:	4803
Relevant Lines:	5191

---

💛 - Coveralls

[msridhar]

@msridhar Given that this revisits a five years old design decision in NullAway, and I am not sure if I am missing any pitfalls, I don't plan to land this until I have had a chance to test this is in our internal codebases, and ideally after you are back from vacation and can provide a review. But I don't see or remember an obvious reason to not support these kind of access paths 😅

I think the reason to not support this was mainly paranoia around performance overhead, as tracking more access paths leads to larger dataflow stores. I never rigorously measured the impact; it seemed we could get away with just not tracking these. I guess that has changed, and I agree the current behavior is confusing. So I'm open to this change assuming we don't see much of an overhead regression.

This is already a problem for JDK11 codebases and only likely to become more obvious when dealing with JDK17 records.

Why does this issue come up more for JDK11 codebases?

[lazaroclapp]

This is already a problem for JDK11 codebases and only likely to become more obvious when dealing with JDK17 records.
Why does this issue come up more for JDK11 codebases?

Sorry, those were two thoughts: a) this is likely to come up more for JDK17 vs JDK11/JDK9, b) we definitely see this in JDK11 code outside the Android world. I wasn't trying to imply that this is more common in Java 11 than 8/9.

[ketkarameya]

Change LGTM. Except a minor question.

[ketkarameya]

why did we remove the checkNotNull?

[lazaroclapp]

We are checking NullAway with NullAway now, and value is not marked @Nullable, so it should always be non-null. I think this is from before NullAway was running on itself 😄

[msridhar]

FYI, just looked at this change and it looks great 🙂