#1244 BC_EQUALS_METHOD_SHOULD_WORK_FOR_ALL_OBJECTS false positive when Class.isInstance(Object) is used.

[James Harrison]

Using Class.isInstance(Object) instead of instanceof in an equals method causes a false positive for BC_EQUALS_METHOD_SHOULD_WORK_FOR_ALL_OBJECTS.

[Tagir Valeev]

FindBugs considers Class.isInstance. And according to the git history this check was added many years ago. For example this code doesn't trigger BC_EQUALS_METHOD_SHOULD_WORK_FOR_ALL_OBJECTS:

public class Bug1244 {
    private final int a;

    public Bug1244(int a) {
        this.a = a;
    }

    @Override
    public boolean equals(Object obj) {
        if (this == obj)
            return true;
        if (obj == null)
            return false;
        if (!getClass().isInstance(obj))
            return false;
        Bug1244 other = (Bug1244) obj;
        if (a != other.a)
            return false;
        return true;
    }
}

However if you remove getClass().isInstance(obj) check, then bug will appear.

If you still experience any problems with isInstance, please provide a test case where false-positive actually occurs. Thanks.

[James Harrison]

Seeing false positive when using the class property instead of the getClass() method.

public class Bug1244 {
    private final int a;

    public Bug1244(int a) {
        this.a = a;
    }

    @Override
    public boolean equals(Object obj) {
        if (this == obj)
            return true;
        if (obj == null)
            return false;
        if (!Bug1244.class.isInstance(obj))
            return false;
        Bug1244 other = (Bug1244) obj;
        if (a != other.a)
            return false;
        return true;
    }
}

[Tagir Valeev]

Nope, neither this code triggers this warning. Have you actually tried? Which version of FindBugs are you using? This code is used to check for isInstance:

if (seen == INVOKEVIRTUAL && "isInstance".equals(getNameConstantOperand())
        && "java/lang/Class".equals(getClassConstantOperand())) {
    OpcodeStack.Item item = stack.getStackItem(0);
    if (item.getRegisterNumber() == 1) {
        sawInstanceofCheck = true;
    }
} 

Thus the only restriction is that the argument of isInstance is the parameter of equals method. There's no restriction on the class object itself.

[James Harrison]

I've found the code that lead to me originally creating this issue. The instance check was performed by a static method in the class. Something like...

public class Bug1244 {
    private final int a;

    public Bug1244(int a) {
        this.a = a;
    }

    public static final boolean isBug1244(Object obj) {
        return Bug1244.class.isInstance(obj);
    }

    @Override
    public boolean equals(Object obj) {
        if (this == obj)
            return true;
        if (obj == null)
            return false;
        if (!isBug1244(obj))
            return false;
        Bug1244 other = (Bug1244) obj;
        if (a != other.a)
            return false;
        return true;
    }
}

We are using FindBugs 3.0.0.

[Tagir Valeev]

I see. Well, such kind of interprocedural analysis would be quite hard to implement currently. FindBugs cannot automatically inline such calls now. Ok, I'm reopening this bug. Maybe someday it will be possible.