Question: Best use of NullAway in specific scenarios

[mauricioaniche]

Dear, NullAway sourcerers! First of all, thanks for such a great tool.

I have two questions related to NullAway issues I've been seeing in our codebase.

First, if a class has methods like hasX() which checks that field X (which is nullable) isn't null, NullAway doesn't understand it. And so, it complains about follow-up usage of X. The @Contract annotation doesn't work in this case, as no parameters are passed to hasX. I also don't think the annotation has support for fields in its specification, to be honest.

Second, it's truly great that NullAway understands that the get() operation of a Map can return null. However, in some pieces of code, the algorithm ensures that the get() will always returns a value. Extracting the get() to a private method which does the checks or wrapping the call up with Objects.requireNonNull works but doesn't look nice, aesthetically speaking.

I wonder if there's a best way to solve these two issues without having to suppress the warning. If you see some improvement that can happen in NullAway to support these cases, I'm happy to follow your guidance and try and do it myself.

Thanks for the help!

[msridhar]

Thanks for the questions!

First, if a class has methods like hasX() which checks that field X (which is nullable) isn't null, NullAway doesn't understand it. And so, it complains about follow-up usage of X. The @Contract annotation doesn't work in this case, as no parameters are passed to hasX. I also don't think the annotation has support for fields in its specification, to be honest.

You're correct we don't have support for this right now. We actually do support @RequiresNonNull and @EnsuresNonNull for instance fields (I'm working on updating the wiki docs now), but you would need something like Checker Framework's @EnsuresNonNullIf for this case. I'm open to supporting this if you'd like to make a contribution!

Second, it's truly great that NullAway understands that the get() operation of a Map can return null. However, in some pieces of code, the algorithm ensures that the get() will always returns a value. Extracting the get() to a private method which does the checks or wrapping the call up with Objects.requireNonNull works but doesn't look nice, aesthetically speaking.

Here I'd like to see an example to understand better. We do have logic to support get() calls that are guarded under a call to containsKey(). But it sounds like your case is something different.

[mauricioaniche]

Hi, @msridhar, thanks for the response!

For 1, EnsuresNonNull solves our case! Great!

For 2, we do have a good number of maps that we create based on, say, enum keys, and later, we get() based on key, so, we are sure there's always a value for that key.

[msridhar]

For 1, EnsuresNonNull solves our case! Great!

Honestly I'm a bit surprised :-) If hasX() is just returning a boolean, but is not actually initializing the field if it is null, I would expect an error reported on the hasX() method (since it doesn't ensure the field is non-null). I can test this later.

For 2, we do have a good number of maps that we create based on, say, enum keys, and later, we get() based on key, so, we are sure there's always a value for that key.

Yeah, unfortunately we don't have built-in handling for this. My best suggestion is adding a castToNonNull method and using that. If you pass the -XepOpt:NullAway:CastToNonNullMethod option, NullAway will make sure that you only cast expressions that are @Nullable, which can be a good sanity check.

[mauricioaniche]

For 1, I said "it solved our case", but I haven't tried that in practice yet. I'll reach out to the engineer that had this issue and try it together with him 😆 I'll get back to you.

For 2, yeah, that's the solution I also see! Ok, I'll be suggesting that!

[mauricioaniche]

@msridhar It indeed doesn't work! If you give me a few pointers, I'd like to try implementing something like VerifiesNonNull("field") boolean hasField() which then tells the method that field can't be null after the call.

[msridhar]

A quick brain dump of thoughts here.

• I think we'd want to call the annotation @EnsuresNonNullIf to match the Checker Framework naming. See their Javadoc. We can just support instance fields of the receiver to start I think.
• We need to use the information from this annotation at call sites to learn that a field is non-null if the call returns a certain boolean. You can see how things are done for EnsuresNonNull here. We'd probably want an EnsuresNonNullIfHandler that does something similar, but only modifies the thenUpdates or elseUpdates as appropriate.
• We need to verify the annotation is correct for the annotated method. This requires checking that for all return statements, if they may return the boolean indicated in the annotation, then the relevant fields are definitely not null. I don't know if we have code exactly like this, but the code for checking nullability for filter operations on streams might be the closest thing, here. We could do this checking in the same EnsuresNonNullIfHandler.

Hope that's enough to get you started 🙂

[mauricioaniche]

@msridhar I have a simple draft ready, with a few open questions! Should we continue the conversation in the PR?

[msridhar]

Yup, I've commented there!

[mauricioaniche]

I am closing this issue since it's solved by PR #1044!