13

Is there any tool that can warn me against the following sort of code:

if ( someClass.equals( someString ))

For example:

if ( myObject.getClass().equals( myClassName ))

Such a thing is legal Java (equals takes an Object) but will never evaluate to true (a class can never equal a String) so is almost certainly a bug.

I have checked Eclipse, FindBugs and PMD but none seem to support this feature?

Mark Peters
  • 80,126
  • 17
  • 159
  • 190
Richard Kennard
  • 1,325
  • 11
  • 20
  • No, it's not a bug. It will return false if they are different types, and that is as it should be. No need for a warning in my mind. – Hovercraft Full Of Eels Oct 26 '11 at 03:00
  • How is this a bug? `2` is not equal to `"foo"`. That doesn't mean the fact that I can determine that is a bug. – Adam Robinson Oct 26 '11 at 03:00
  • 4
    I *think* Richard is saying that any *code* that tries to do this represents a likely bug, and thus it would be useful if a tool (such as Eclipse) could warn against it. Is that what you're asking, Richard? – Dan Tao Oct 26 '11 at 03:04
  • 1
    It's not a bug *however*, except in a few specialized cases of specially created subclasses, equality across different run-time types is *not what is really desired*. This behavior is due to the virtual-nature of `Object.equals(Object)`. –  Oct 26 '11 at 03:05
  • 1
    If not a bug it's no doubt unwanted code, as is any if condition that can never, ever be true. Dead code is bad and should be removed. – Mark Peters Oct 26 '11 at 03:07
  • Dan: yes, that is what I am asking. – Richard Kennard Oct 26 '11 at 03:20
  • I seem to remember having seen a FindBugs warning for using equals on two different types, so maybe you need to check your FindBugs config. – Mark Rotteveel Oct 26 '11 at 12:25
  • Mark Rotteveel: the 'EC: Call to equals() comparing different types' (identified by Mark Peters) does not seem to be exposed in the Eclipse FindBugs plugin? Can you confirm? – Richard Kennard Oct 26 '11 at 20:34
  • FYI: found it under 'detector id' FindRefComparison – Richard Kennard Oct 27 '11 at 03:28

3 Answers3

8

Yes, IntelliJ IDEA has such an inspection that I believe is enabled by default. It flags the following:

Class<?> clazz = String.class;
if (clazz.equals("foo")) {
   //...
}

With the warning:

'equals()' between objects of inconvertible types.

The inspection can be enabled/disabled through Settings->Project Settings->Inspections, then under Probable Bugs check/uncheck "'equals()' between objects of inconvertible types."

FindBugs also should catch this with the "EC: Call to equals() comparing different types" bug check. It can be integrated with Eclipse as it appears you are aware.

Neither is a silver bullet though; they can't read your mind. The best you can hope for is that it will favour false positives rather than false negatives.

Mark Peters
  • 80,126
  • 17
  • 159
  • 190
  • Whoops looks like I might have misread the findbugs check that I cited. Looking for a real match now. I definitely tested the IntelliJ one but don't have a findbugs environment to test. – Mark Peters Oct 26 '11 at 03:15
  • 1
    Ok I have updated the findbugs link now to what I think should catch this bug. – Mark Peters Oct 26 '11 at 03:19
  • Perfect, thanks Mark. I had found the first link you mentioned (which wasn't it) but not the second. – Richard Kennard Oct 26 '11 at 03:21
  • Gah! Mark I cannot find this bug check (EC_UNRELATED_TYPES) in the FindBugs Eclipse plugin? Could you help? – Richard Kennard Oct 26 '11 at 03:43
  • @Richard: Sorry, I tried installing the FindBugs plugin for Eclipse and it keeps failing (seems like the update site is fairly shoddy). I won't be able to give you that direct help in the near future. – Mark Peters Oct 26 '11 at 04:32
  • No probs. Thanks so much for your help to date. – Richard Kennard Oct 26 '11 at 05:28
1

What you are checking for is not necessarily a "problem": equals() is declared in the Object class, and takes and Object as its parameter. Classes override this method, and their implementation may well allow an object of a different class to "equal" the target object.

I have done this a few times myself, for example for allowing an object to "equal" another object if the other object (say a String) matches the key field of the target:

class MyClass {
    private String id;

    public boolean equals(Object obj) {
        // Compare as if "this" is the id field
        return id.equals(obj instanceof MyClass ? ((MyClass)obj).id : obj);
    }

    public int hashCode() {
        return id.hashCode(); // so hashCode() agrees with equals()
    }
}

It's actually pretty handy, because the following code will work as desired:

List<MyClass> list = new ArrayList<MyClass>();
// collection methods will work with instances:
list.contains(someInstance);
list.remove(someInstance);
list.indexOf(someInstance);
// and with keys!
// handy if you can only get the key, for example from a web url parameter
list.contains("somekey");
list.remove("somekey");
list.indexOf("somekey");
Bohemian
  • 412,405
  • 93
  • 575
  • 722
  • The general case may not *always* be a problem (though I'd say it's suspect enough to warrant a warning). The specific case the OP mentioned is *definitely* a problem since `String` and `Class` are both final types and have well defined `equals()` methods that ensure that a `Class` instance can **never** be equal to a `String` instance. – Mark Peters Oct 26 '11 at 03:21
  • 3
    I had to downvote because what you call "handy" is actually very bad practice and is in fact a violation of the "symmetry" requirement for a compliant equals method. In short, if you switched the operands (`myString.equals(myClass)`) then it wouldn't be true, but `myClass.equals(myString)` would be. That's not allowed. Your code is completely relying on all implementations of `ArrayList` to always call the latter but that's not documented anywhere to be true. See Chapter 3 of Effective Java for a good read: http://java.sun.com/developer/Books/effectivejava/ – Mark Peters Oct 26 '11 at 04:34
  • @MarkPeters Oooooh "Not allowed", "very bad", etc... That might apply to "general" classes whose usage is unpredictable, but if the usage of a class is narrow, and it is known with absolute certainty that the reverse equals (ie str.equals(myclass)) will *never* occur, then in my professional opinion as a lead java architect, it's fine. And I have coded this several times for commercial, financial applications and have never had a problem. The amount of repetitive java cruft removed by doing this is large and resulting code is clearer - and that makes it good code IMHO. – Bohemian Oct 26 '11 at 05:20
  • 3
    Bohemain: Mark is just trying to be helpful. The problem, as he explained, is that you cannot "know with absolute certainty that the reverse equals will never occur". In your example you are using ArrayList. Oracle could conceivably change the implementation of ArrayList at any time and your code would break. Even a point release of the JDK could break you. There are no guarantees on ordering of equals. In fact, the guarantees are exactly the opposite: that equals must be symmetrical. – Richard Kennard Oct 26 '11 at 06:46
1

This is the idea behind the IEquatable<T> interface in .NET: providing a mechanism for types to implement what I'll call strongly typed equality. There is also the IEqualityComparer<T> interface for allowing this logic to be implemented in a separate type.

According to this StackOverflow question (answered by Jon Skeet, who generally seems to know what he's talking about), there doesn't seem to be any equivalent in Java.

Of course, you can always implement such a thing yourself for your own types, but it won't do you much good with types that are part of Java's base class libraries. For compile-time detection of such issues, your best bet is some sort of analysis tool (Mark Peters indicates there is apparently one built in to IntelliJ IDEA) that can suggest to you that certain code might be suspect. In general, assuming you aren't one to ignore warnings, this ought to be good enough.

Community
  • 1
  • 1
Dan Tao
  • 125,917
  • 54
  • 300
  • 447
  • There is roughly in Java - [Comparable](http://download.oracle.com/javase/6/docs/api/java/lang/Comparable.html). .NET has this same "problem" in that `object.Equals(object)` is *virtual*. The use of IEquatable/Comparable can be used to make [some] code more type-safe, but do not address this case directly. However, in C#, the `==` operator is often used (for non-generic compile-time types, including pretty much all structure types) and that is *non-virtual* and is thus "better typed" (e.g. able to generate compile errors). –  Oct 26 '11 at 03:09
  • @pst: Yes, but the upside in .NET is that if you have, e.g., a function that takes an `IEquatable` param, then you've assured the strongly-typed `Equals` over the virtual one inherited from `object`. As for the `==` operator, I actually find the fact that it is static (and thus non-virtual) to be *un*desirable, as at least you can override `object.Equals` but if you use `==` in such a way that the compiler cannot infer the implementing type, you might end up checking for reference equality without realizing it (if you don't know what you're doing). – Dan Tao Oct 26 '11 at 03:15
  • @pst: As for the `Comparable` interface in Java, I view that as analogous to .NET's `IComparable`, which I agree is *almost* the same but, obviously, not quite (at least conceptually, though one value may be neither "greater than" nor "less than" another, it does not necessarily imply they are *equal*, right?). – Dan Tao Oct 26 '11 at 03:17
  • *And the same goes for `Comparable`* :-) However, this doesn't address the issue. The note from Comparable is "...it is strongly *recommended, but not strictly required that (x.compareTo(y)==0) == (x.equals(y))*...." –  Oct 26 '11 at 03:17
  • @pst: Agreed, we're discussing .NET which doesn't even relate to the OP's question! ;) – Dan Tao Oct 26 '11 at 03:18