NullAway icon indicating copy to clipboard operation
NullAway copied to clipboard

Feature request: Recognize common "assertThat(...).isNotNull()" statements

Open ZacSweers opened this issue 5 years ago • 2 comments

These are common in tests

assertThat(foo).isNotNull();
foo.bar(); // <- will currently error

ZacSweers avatar Apr 08 '19 03:04 ZacSweers

This sounds doable, but only with a custom handler, since library models are not currently expressive enough to reason across multiple chained calls like that (e.g. isNotNull() performs the nullness check, but doesn't take the argument to be checked).

I think it should be a simple handler, the question is if it's one that we want enabled by default. Is assertThat(...) common in general, or common at Uber? We might want to make this handler optional/flag controlled.

Also, another quick question: what's the FQN of assertThat in here? org.junit.Assert.assertThat? Or is there an Uber internal version?

lazaroclapp avatar Apr 08 '19 15:04 lazaroclapp

I think covering the standard libraries for this is probably a safe start

Junit's is one, there's also AssertJ, Truth, probably some others. Maybe cover the common ones and add a hook for others to provide their own expressions? (not sure how simple that is)

assertThat is pretty common in general I'd say, but running nullaway in tests probably isn't. I could see a good reason to make this off by default and opt-in to enable only in tests.

ZacSweers avatar Apr 11 '19 09:04 ZacSweers

I'm pretty sure we fixed this one some time back

msridhar avatar Sep 02 '23 16:09 msridhar