Handle assertThat(...).isNotNull() statements

[navahgar]

• This adds handling of some nullness assertions to NullAway. Specifically, this adds support for "assertThat(...).isNotNull()" statements.
• Issue Feature request: Recognize common "assertThat(...).isNotNull()" statements #301
• This change also includes unit tests for this functionality.

[CLAassistant]

All committers have signed the CLA.

[lazaroclapp]

A few comments, particularly around that Name initialization we are trying to do and about putting this handler behind a Config flag. The rest are just nits, I think. Really nice overall! :)

[lazaroclapp]

Actually, upon reflection, this whole thing is not thread safe, since this could execute concurrent with initializeMethodNames and return true when say isNotNullOwner and the rest are still uninitialized. So the other method being synchronized is mostly pointless.

My thoughts:

• I remain unsure whether the same BugChecker instance, and therefore this class, will be called in parallel, we could just leave both methods unprotected and the then fix it if it turns out it can. (cc: @msridhar , do you know the answer here? )
• We could conservatively move this check to initializeMethodNames and just have that method return early if isNotNull is already set, then call initializeMethodNames unconditionally from onDataflowVisitMethodInvocation
• We could use toString() to match the methods, measure the overhead and leave it at that if it's not high. Early optimization being the root of all evil and all that :)

What do you think?

[msridhar]

Concurrency is only an issue with static fields, which we should avoid unless they are immutable values. There should be a single BugChecker instance per javac instance and it should not be getting called from multiple threads.

Since the only static fields here are final Strings we should be fine. But the synchronized on the private method is unnecessary.

[navahgar]

Thanks for the clarification. Removed synchronized on the method.

[lazaroclapp]

👍

[msridhar]

Looks good overall! I approve once @lazaroclapp's comments are addressed

[msridhar]

Concurrency is only an issue with static fields, which we should avoid unless they are immutable values. There should be a single BugChecker instance per javac instance and it should not be getting called from multiple threads.

Since the only static fields here are final Strings we should be fine. But the synchronized on the private method is unnecessary.

[shubhamugare]

After adding the flag, do add a positive test that returns an error on the same code with the flag turned off.

[navahgar]

Added a test with flag turned off.

[lazaroclapp]

Looks good to me if tests pass!

Happy to land this and then add the JUnit API support before perf testing it internally, particularly since it's off by default.

🎉