Different approach for param analysis

[shubhamugare]

Using a different approach for param nullness analysis in jar-infer
• Pre-calculate all the params that are guaranteed to be non-null in each basic block ( Using the already existing checks)
• looks for a path from entry to exit that doesn't dereference the parameter, if it doesn't exist, it's @NonNull
• This covers all the cases from the current approach. Might take more time in execution than the current approach. computeDerefParamList uses this approach and computeDerefParamListUsingPDom uses the previous approach. Constant flag USE_EXTENDED_APPROACH can be used to switch these approaches.

Added tests

[lazaroclapp]

Nice approach!

One thing, though: the comments and description don't really match what the code is doing, and I was actually confused for a while trying to build a test case that would fail given the described approach. The actual approach is "look for a path from entry to exit that doesn't dereference the parameter, if it doesn't exist, it's @NonNull". That seems right to me (or, at least, strictly more right than the original approach), but please change the description and comments to more properly document it.

[lazaroclapp]

So, not sure this comment is right, the way I understand the code below, it says:

"For each param p, if no path exists from the entry block to the exit block which traverses only basic blocks which do not dereference p, then mark p as @NonNull"

[lazaroclapp]

Other than the comment being confusing, this approach actually looks broadly correct to me, with the only issues being:

a) Code sets the argument p when it originally is null such as:

public void test(String s, String t) {
   Objects.requireNonNull(s);
   if (t == null) {
     t = s;
   }
   t.toString();
}

(It's probably fine that we don't handle this, but worth checking if this produces a lot of new false positives for monorepo)

b) The whole other issue about exceptional control flow being pruned that might start to matter the more stuff our analysis catches otherwise.

Also, probably worth checking the performance impact of this vs the original approach. I don't think it should be too bad, since all the time probably went into building the IR and GFG either way, but I think it's worth keeping track.

[shubhamugare]

a) works because of SSA, since variables are assigned only one time. A new variable(v8) is created

public void test(String s, String t) {
   Objects.requireNonNull(s);
   if (t == null) {
     t = s;
   }
   Objects.requireNonNull(t);
}

13:41:12.302 [DEBUG] [TestEventLogger]                v8 = phi  v3,v2
13:41:12.303 [DEBUG] [TestEventLogger]     9   v10 = invokestatic < Application, Ljava/util/Objects, requireNonNull(Ljava/lang/Object;)Ljava/lang/Object; > v8 @12 exception:v9(line 17)

[lazaroclapp]

Oh, right, that makes sense. Nice! Could we add that example as a test then? :)

[shubhamugare]

Yes.. adding.

[lazaroclapp]

nit, slightly clearer: "find which params are dereferenced (and thus non-null) in each block"

[shubhamugare]

actually, we test more things than just dereferencing, like precondition and assertions, hence I phrased it like this. And maybe in future we might add more checks which are not just deref too.

[lazaroclapp]

Maybe "find which params are treated as being non-null inside each basic block", then? :)

And for the comment on the filter:

"For each param p, if no path exists from the entry block to the exit block which traverses only basic blocks which do not require p being non-null (e.g. by dereferencing it), then mark p as @NonNull"

The main problem I have with "params are definitely non-null in each block" is that it's not clear that they are expected to be non-null on entry vs guaranteed to be non-null by the block.

[shubhamugare]

Yeah..makes sense

[navahgar]

LGTM. Nice Work. Once Lazaro's comments are addressed, I am okay with accepting this.

[lazaroclapp]

@shas19 Travis CI should be fixed by #322 . Can you rebase, please? 😅

[navahgar]

LGTM

[lazaroclapp]

Approved!