SEO proposes unsafe optimizations

[tazle]

Example code:

arrayExtractedFromValue = value.getArray();
if (doSomeChecks(value) && arrayExtractedFromValue[0] == 1) {
  doStruff();
}

    private static boolean doSomeChecks(MyType value) {
        return value.getArray().length > 0;
    }

SEO proposes to reverse the conditions to arrayExtractedFromValue[0] == 1 && doSomeChecks(value). This happens to be unsafe, since doSomeChecks(value) is what guarantees that the extracted array is not zero-length.

I guess the root cause is that the plugin does not detect aliasing between arrayExtractedFromValue and the result of value.getArray() in doSomeChecks.

Is it possible for FindBugs to detect such potential aliasing and act conservatively?

[mebigfatguy]

thanks for the report, I agree it is a bug in fb, and i'll look to address. But i'd also say that code is poorly written.

it would be better for the reader if it were

if (doSomeChecks(value) {
    arrayExtractedFromValue = value.getArray();
    if (arrayExtractedFromValue[0] == 1) {
       doStruff();
    }
}

[tazle]

Oh, absolutely.

That just happened to be a slightly simplified version of something we hit in reality, when somebody blindly changed the order of the tests and nothing bad happened for a few weeks, since the array typically did have values in it :)

[mebigfatguy]

haven't forgotten this, i just haven't thought up an reasonable way to do this yet.

[ThrawnCA]

Truthfully, I think that your best tool here is unit test coverage. If you had unit tests covering the three branches - first half fails, second half fails, both halves succeed - then that would have picked up the problem.

Without that, it's difficult for even a human to recognise that the order of operations matters, without digging through the code to see what doSomeChecks actually entails. And the fact that the checks are being run on the parent object, not on the array that depends on them, is...not great.

Actually, I would recommend not changing the detector, but instead changing the bug message. When code looks like the above, it should be flagged IMO, but with the flag acknowledging that the solution may be to reverse the order of operations, or may alternatively be to refactor things so that humans and machines can easily understand why things are done this way.