Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
ZacSweers opened this issue Apr 8, 2019 · 3 comments
Closed
Assignees

Comments

@ZacSweers
Copy link
Contributor

These are common in tests

assertThat(foo).isNotNull();
foo.bar(); // <- will currently error
@lazaroclapp
Copy link
Collaborator

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?

@ZacSweers
Copy link
Contributor Author

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.

lazaroclapp pushed a commit that referenced this issue Apr 22, 2019
Adds handling of some nullness assertions to NullAway. Specifically, this adds support for "assertThat(...).isNotNull()" statements and proves an initial fix for Issue #301 (other test APIs to come).

This support is off by default, but can be enabled by passing `-XepOpt:NullAway:HandleTestAssertionLibraries=true`.
@msridhar
Copy link
Collaborator

msridhar commented Sep 2, 2023

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants