LGTM.com - false positive - Dereferenced variable may be null

[chrjohn]

https://lgtm.com/projects/g/quickfix-j/quickfixj/snapshot/9657839bf46838645b670486e57852de40d34f04/files/quickfixj-core/src/main/java/quickfix/Message.java?sort=name&dir=ASC&mode=heatmap#xaced2743c6d7c82:1

To be honest it is quite tricky to find out that the variable group cannot be null. This is conveyed via boolean firstFieldFound which is only set when group has been assigned. See here:

https://lgtm.com/projects/g/quickfix-j/quickfixj/snapshot/9657839bf46838645b670486e57852de40d34f04/files/quickfixj-core/src/main/java/quickfix/Message.java?sort=name&dir=ASC&mode=heatmap#L707

But if firstFieldFound is false an Exception would be thrown from here:
https://lgtm.com/projects/g/quickfix-j/quickfixj/snapshot/9657839bf46838645b670486e57852de40d34f04/files/quickfixj-core/src/main/java/quickfix/Message.java?sort=name&dir=ASC&mode=heatmap#L720

However, I wanted to make the null-check easier to spot for LGTM by removing the boolean variable and simply check for group!=null in checkFirstFieldFound() but the alert is still not removed. This can be seen here:

https://lgtm.com/projects/g/quickfix-j/quickfixj/rev/pr-e6092b79d8f5ba3d89e1644bd0153b8c5b22f5bb

The only alert not fixed by that PR is the possible null pointer dereference:
https://lgtm.com/projects/g/quickfix-j/quickfixj/snapshot/9657839bf46838645b670486e57852de40d34f04/files/quickfixj-core/src/main/java/quickfix/Message.java?sort=name&dir=ASC&mode=heatmap#xaced2743c6d7c82:1

[aschackmull]

The nullness analysis is currently mostly intra-procedural, so if the check on firstFieldFound was inlined then it would likely work. Note that the analysis is able to rule out group being null a few lines above - likely due to an inferred correlation with previousOffset being -1.

[chrjohn]

I didn't want to inline the check on firstFieldFound because it is used in two different places.

Note that the analysis is able to rule out group being null a few lines above

You are right. E.g. Netbeans is not as clever and is also claiming a possible NP derefence there.

[aschackmull]

In any case, thank you for your FP report. I think we can reasonably extend the analysis to cover both the original code and your proposed fixup - they each require different extensions to the analysis as the inferences required are somewhat different. However fixing these is not at the top of the priority list at the moment so it may take me some time to get to.

[aschackmull]

For now, I've made a PR (#2640) to document the two FPs as unit tests.

[chrjohn]

No problem, thanks for your reply.