#1408 BIT_SIGNED_CHECK not reliable

[Uwe Plonus]

BIT_SIGNED_CHECK does only work when the second operand of the logical & is either a static final or final member. Also it is only found when this variable is not negative.

This check should work always independent of the current value, the modifiers (should also work for static or pure member) and if the first or second value is meant. Also it should work when the value is completely unknown.

Examples where it works:

private static final int flag = 0x40000000;
public boolean bug(int value) {
    boolean flagSet = ((value & flag) > 0);
    ...
}

private final int flag = 0x40000000;
public boolean bug(int value) {
    boolean flagSet = ((value & flag) > 0);
    ...
}

Examples where it should work (but does not at the moment):

private static final int flag = 0x40000000;
public boolean bug(int value) {
    boolean flagSet = ((flag & value) > 0);
    ...
}

Because the flag is at the first place.

private final int flag = 0x40000000;
public boolean bug(int value) {
    boolean flagSet = ((flag & value) > 0);
    ...
}

Because the flag is at the first place.

private static int flag = 0x40000000;
public boolean bug(int value) {
    boolean flagSet = ((value & flag) > 0);
    ...
}

Because the flag is not final.

private int flag = 0x40000000;
public boolean bug(int value) {
    boolean flagSet = ((value & flag) > 0);
    ...
}

Because the flag is not final.

public boolean bug(int value, int flag) {
    boolean flagSet = ((value & flag) > 0);
    ...
}

Because the flag is a parameter.

private static final int flag = 0x80000000;
public boolean bug(int value) {
    boolean flagSet = ((value & flag) > 0);
    ...
}

Because the value of flag is negative and this raises the error BIT_SIGNED_CHECK_HIGH_BIT.

private final int flag = 0x40000000;
public boolean bug(int value) {
    boolean flagSet = ((value & flag) > 0);
    ...
}

Because the value of flag is negative and this raises the error BIT_SIGNED_CHECK_HIGH_BIT.

I think that there should be no difference between BIT_SIGNED_CHECK_HIGH_BIT and BIT_SIGNED_CHECK because the code in error is the same in both cases.

[Tagir Valeev]

The IncompatMask detector which produces these reports (along with BIT_IOR, BIT_AND and BIT_AND_ZZ) is poorly written: it relies on very specific bytecode patterns. I will rewrite it to use OpcodeStack, so it will be more robust and cover the cases you report.

[Tagir Valeev]

This case we won't fix as it's too generic:

public boolean bug(int value, int flag) {
    boolean flagSet = ((value & flag) > 0);
    return flagSet;
}

As we cannot determine the exact flag value, we do not issue the warning. Otherwise we high end up having too many false-positives. Also we won't merge BIT_SIGNED_CHECK_HIGH_BIT and BIT_SIGNED_CHECK. The high-bit pattern is separate, because it has higher priority. The value & 0x8000_0000 cannot be greater than zero regardless of value, thus it's the correctness problem, while BIT_SIGNED_CHECK is just a bad practice (it may never lead to real problem). Probably the bug messages should be updated though.

[Tagir Valeev]

Fixed: https://github.com/findbugsproject/findbugs/commit/a4680ab114b99d98e319f254792047ba9fdcb90a

Now all the patterns of this detector work in a more reliable manner, independent on operand order and constant source (if it's possible to extract the constant). I modified the bug messages a little bit: now BIT_SIGNED_CHECK_HIGH_BIT clearly states that there's more dangerous situation. Also I took the liberty to remove "Boris Bokowsky" signature from bug descriptions as usually bug pattern author is not lited there. Instead I added Boris to the contributors list in manual.xml.

Newly found bugs in Eclipse JDT:
BindingLabelProvider.java#249

if ((flags & JavaElementLabels.M_PARAMETER_TYPES | JavaElementLabels.M_PARAMETER_NAMES) != 0) { ... }

Clearly a bug: | JavaElementLabels.M_PARAMETER_NAMES will make the whole expression non-zero, thus condition is always true. Seems that parentheses are missing.

JavaEditorBreadcrumb.java#522

(delta.getFlags() & IJavaElementDelta.F_CONTENT | IJavaElementDelta.F_FINE_GRAINED) == IJavaElementDelta.F_CONTENT

Likely the similar problem: this condition is never true.