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

3.x
pending-works-for-me
nobody
None
5
2014-11-10
2014-01-19
James Harrison
No

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

Discussion

    • Group: 3.0.0 --> 3.0.1
     
  • Tagir Valeev
    Tagir Valeev
    2014-11-10

    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.

     
  • Tagir Valeev
    Tagir Valeev
    2014-11-10

    • status: open --> pending-works-for-me
     
  • James Harrison
    James Harrison
    2014-11-10

    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
    Tagir Valeev
    2014-11-10

    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
    James Harrison
    2014-11-10

    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
    Tagir Valeev
    2014-11-10

    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.

     
  • Tagir Valeev
    Tagir Valeev
    2014-11-10

    • status: pending-works-for-me --> open-later
     
  • William Pugh
    William Pugh
    2014-11-10

    • status: open-later --> pending-works-for-me