NullAway doesn't recognize Guava Preconditions checks

[MatteoJoliveau]

I'm using Google Guava Preconditions to raise errors on null parameters. So basically, when I write something like:

Preconditions.checkArgument(item != null, "Unable to find an element with name '%s'.", name);
return item;

item is null safe, but NullAway still raises an error.

[msridhar]

Thanks for the report! I think we should add default support for Preconditions.checkNotNull; we have this supported internally. checkArgument is trickier since the API does more than nullness checking. @lazaroclapp could you look into this?

[lazaroclapp]

I believe com.google.common.base.Preconditions.checkNotNull is already supported in open source NullAway too. @MatteoJoliveau If you can use that method, then that's the fastest workaround right now. If this does not work, please let me know.

Supporting Preconditions.checkArgument can certainly be done with a handler, similar to how we propagate nullness information from stream .filter() operations. I will take a look at this when I can. It is worth thinking about the general case of adding library models where we know execution will terminate if a certain argument evaluates to either true or false, and then treat this method as an special case. As always, handlers come with a performance cost, however. Are there any checkArgument calls which check for nullness and which would be harder to express as checkNotNull?

[msridhar]

Ah, I didn't realize we already support checkNotNull. We can think about supporting checkArgument in the future, but I think it will be low priority given that checkNotNull is available, due to performance concerns.

[MatteoJoliveau]

I will try to use checkNotNull since it's not that different from what I'm already doing (checking item != null is basically a check not null).
But if you could support checkArgument too in the future it would be great, it gives a bit more freedom to the end users when using Preconditions.

[msridhar]

Yes, that is true. If we can find a way to support checkArgument without too much performance penalty we will give it a shot.

[ben-manes]

Objects.requireNonNull should also be supported.

[jrepe]

Hey, this will be a blast from the past, but is there any plans to support checkArgument precondition ? :)

[msridhar]

@jrepe no concrete plans to prioritize this. I would be open to accepting a PR if we could show it doesn't impact overhead too much (and I don't think it should).

[ketkarameya]

Another flavor of this is scenario is-
Preconditions.checkArgument(item != null && someOtherCondition(item), "Unable to find an element with name '%s'.", name);
(Note I have a conjunction expression as the first argument).

[lazaroclapp]

This shows up enough internally and asked about externally often enough that we should probably prioritize it above lowpriority. Most cases seem to be reducible to Preconditions.checkNotNull(...). But, as @ketkarameya 's example shows... some are a bit more involved.

There are also cases like this, where there is no null check, but there is a call that implies null checking:

Preconditions.checkArgument(Strings.isNullOrEmpty(s), "blah")

[msridhar]

I wonder if in the Checker Framework CFG, there are stores corresponding to the program point after evaluation of the first expression passed as the first argument to these methods. It seems possible that if we can find that store, we might be able to magically extract the right facts from the "then" store and propagate them to the correct successor store for the checkArgument call.

[lazaroclapp]

So, I see AnalysisResult.getStoreBefore(...) and .getStoreAfter(...) can take a Tree or a Node and retrieve the store at that point, but the problem is that we need to query this while performing the analysis. Any way to get an AnalysisResult that I can find requires... well, running the analysis on the method first, which at best leaves us repeating work twice for each method.

I think a better alternative is to use handlers to move the APs around. The easy part of that is using .onDataflowVisitMethodInvocation(...) here to re-add them to the stores after the Preconditions call, getting it from some map maintained inside the handler. The hard part is finding all the types of visitX for expressions that can be passed to that Preconditions.checkArgument(...) call and saving the stores then...

[msridhar]

In the Checker Framework, the standard transfer function type CFTransfer keeps a reference to the Analysis object:

https://github.com/typetools/checker-framework/blob/95dd4483eeba563aa6755c9a4559a90db2e606a6/framework/src/main/java/org/checkerframework/framework/flow/CFTransfer.java

The Analysis type allows for getting values of Nodes and Trees while the analysis is running:

https://github.com/typetools/checker-framework/blob/95dd4483eeba563aa6755c9a4559a90db2e606a6/dataflow/src/main/java/org/checkerframework/dataflow/analysis/Analysis.java#L118-L136

I think the right thing to do here may be to re-jigger things so that AccessPathNullnessPropagation has a reference to the running Analysis. This way we can get the current result for nodes without analyzing methods twice.

[lazaroclapp]

So, we already can get values for Nodes and Trees without that, just from the TransferInput<Nullness, NullnessStore> parameter here. You can then do input.getValueOfSubNode([node]), which should be the same as Analysis.getValue(...) in this case, I think.

Even more, TransferInput<Nullness, NullnessStore> input has input.getThenStore(), which seems to work in this case:

Preconditions.checkArgument(a != null && !a.equals(this));
a.toString();

As printing input.getThenStore() inside a modified handler.onDataflowVisitMethodInvocation(...) gives:

Found Preconditions.checkArgument check at node: Preconditions.checkArgument(((a != null) && (!a.equals(this))))
inputThenStore: {AccessPath{root=a, elements=[], mapGetArg=null}=Non-null}
inputElseStore: {AccessPath{root=a, elements=[], mapGetArg=null}=Nullable}

Which we can use to set **both**Updates for the method call.

The problem is, this only works for the complex boolean expression case (due to the implicit control flow).

In these cases:

Preconditions.checkArgument(a != null, "a ought to be non-null");
Preconditions.checkArgument(!Strings.isNullOrEmpty(a));

We simply get:

inputThenStore: {AccessPath{root=a, elements=[], mapGetArg=null}=Nullable}

As no branching is taking place inside the argument to the call.

I suppose worst case we'd have to handle three cases semi-redundantly:

• Single == and != expressions
• Calls to methods that return false if their argument is null
• Pass all other expressions to the store above

But even that might miss something.

p.s. The NullnessStore handles cases like Preconditions.checkArgument(this.hashCode() != 5 && a != null) too, so we don't need to do anything special for the "final" component of a long conditional, it seems.

[lazaroclapp]

Actually, never mind, the above explanation is wrong.

The GOOD news is: that input.getThenStore() works for all three cases as desired!

The BAD news is: what breaks that approach is the presence of an explanation string in the Preconditions call, since in:

Preconditions.checkArgument(a != null && !a.equals(this), "a ought to be non-null");

The "previous" thing to be evaluated is the string "a ought to be non-null" and not a != null && !a.equals(this)

Edit: Not at all ready for review, but #607 shows this approach in more detail.

[msridhar]

The "previous" thing to be evaluated is the string "a ought to be non-null" and not a != null && !a.equals(this)

I don't understand this. I would expect that the approach would be to query the store(s) for the first actual parameter passed to checkArgument. So why does the "previous" thing to be evaluated come into play?

[lazaroclapp]

It doesn't query the store at an arbitrary node, it queries it on input to invoke* Preconditions. checkArgument, see here.

If we had a way to do stop and do a callback inside AccessPathNullnessPropagation after visiting the expression inside the method argument, then we could use that, but that would be a ton of different visitX methods, rather than a single place, as far as I can tell.

Another option is, having the node, try to find its corresponding block, and then use a reference to the analysis to call getInput(Block b)

[msridhar]

Ah, I get it. With this API, we can ask about the right Node, but not necessarily at the right program point (immediately after the node is evaluated).

Not obvious how to get exactly what we want in a clean way. May need to step through with a debugger and see if the relevant state is around somewhere.

[lazaroclapp]

Another option is, having the node, try to find its corresponding block, and then use a reference to the analysis to call getInput(Block b)

This also doesn't seem to work, since the whole thing is in the same block, when asking about the block corresponding to a != null, I get:

Found Preconditions.checkArgument check at node: Preconditions.checkArgument((a != null), "a ought to be non-null")
firstArgBlock: RegularBlock([Preconditions.checkArgument, a, null, (a != null), "a ought to be non-null"])
Successors: [ExceptionBlock(Preconditions.checkArgument((a != null), "a ought to be non-null"))]

So, getting the store either before or after that whole RegularBlock([Preconditions.checkArgument, a, null, (a != null), "a ought to be non-null"]) is no use to us. What we need is the store right after the (a != null) node, and I think we only have access to it from visitMethodInvocation in the case where that expression is the last argument and it just happen to coincidentally be the "input" node to the invoke.

[msridhar]

We could think about adding an API to Checker dataflow and opening a PR; I think they would be open to it if we show the change is useful.

[msridhar]

This issue is fixed in NullAway 0.9.8! Let us know if you still see problems.