Bug 156994 - Netbeans' equals() implementation is wrong
Netbeans' equals() implementation is wrong
Status: NEW
Product: java
Classification: Unclassified
Component: Hints
6.x
All All
: P1 with 6 votes (vote)
: 8.2
Assigned To: Svata Dedic
issues@java
: PLAN
: 163561 218064 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2009-01-17 20:46 UTC by _ gtzabari
Modified: 2016-08-09 14:14 UTC (History)
8 users (show)

See Also:
Issue Type: ENHANCEMENT
:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description _ gtzabari 2009-01-17 20:46:26 UTC
Netbeans' hints generate the following equals() implementation:

    if (obj == null)
      return false;
    if (getClass() != obj.getClass())
      return false;

According to Effective Java 2nd Edition Item 8 this is wrong because it violates the "Liskov substitution principle
[that] says that any important property of a type should also hold for its subtypes, so that any method written for the
type should work equally well on its subtypes". Joshua goes on to write:

"While there is no satisfactory way to extend an instantiable class and add a value component, there is a fine
workaround. Follow the advice of Item 16, 'Favor composition over inheritance.' Instead of having ColorPoint extend
Point, give ColorPoint a private Point field and a public view method (Item 5) that returns the point at the same
position as this color point"
Comment 1 Jan Lahoda 2009-01-17 21:58:21 UTC
(I unfortunately do not have Effective Java at hand, so this comment is based mostly on information from:
http://www.artima.com/intv/bloch17.html
)

Well, in Java, composition is unusable in some cases (using the example: an instance of ColorPoint cannot be assigned to
variable of type Point). Also, I am not sure how much is this recommendation actually followed in the practice.

The advantage of getClass() != obj.getClass() is that if it fails, it is likely to fail much more predictably. If the
symmetry contract of equals is broken because of use of "instanceof", the behavior of Collections (for example) is
likely to be pretty unpredictable.

Anyway, your patch will be very welcome. But, please introduce an option for the user to choose the behavior.
Comment 2 _ gtzabari 2009-01-17 22:20:56 UTC
I have no clue how to implement this. Is it possible to leave it open as an enhancement request and have someone in the
Netbeans team to add it?
Comment 3 _ gtzabari 2009-01-17 22:47:33 UTC
Here is an excerpt from the first edition of the book on exactly this topic:

http://java.sun.com/developer/Books/effectivejava/Chapter3.pdf

search for "There is simply no way to extend an instantiable class and add an aspect while preserving the equals contract"

In the second edition he goes to say:

"You may hear it said that you can extend an instantiable class and add a value component while preserving the equals
contract by using a getClass test in place of the instanceof test in the equals method:

// Broken - violates Liskov substitution principle (page 40)
@Override public boolean equals(Object o) {
  if (o == null || o.getClass() != getClass())
    return false;
  Point p = (Point) o;
  return p.x == x && p.y == y;
}

This has the effect of equating objects only if they have the same implementation class. While this may not seem so bad,
the consequences are unacceptable."

My understanding is that the "liskov substitution principle" simply states that object inheritance involves a "is-a"
relationship. When you use getClass() you violate the basic principal of object-oriented programming (inheritance).
Sure, the equals() contract doesn't explicitly discuss this, but it shouldn't have to: Java is a OOP language. Just my 2
cents.

Reassigning to the subcomponent owner since (as mentioned before) I have no experience working with the Netbeans code.
Comment 4 Max Sauer 2009-01-19 10:01:41 UTC
OK, we will have a look. This functionality should be optional.
Comment 5 thomasbarnett 2009-09-01 01:17:50 UTC
Product Version: NetBeans IDE 6.7.1 (Build 200907230233)

In addition, Bloch J. also says that:

"For each "significant" field in the class, check to see if that field of the argument matches the corresponding field
of this object. ... for float fields, translate to int values using Float.floatToIntBits and compare the int values
using the == operator; for double fields, translate to long values using Double.doubleToLongBits and compare the long
values using the == operator. (The special treatment of float and double fields is made necessary by the existence of
Float.NaN, -0.0f, and the analogous double constants; see the Float.equals documentation for details.) "

Auto inserted equals() method does not do that:

public class Clazz {

    float f;
    double d;

    public Clazz(float f, double d) {
        this.f = f;
        this.d = d;
    }
}

Auto inserted equals() method: (Alt+Insert -> equals() and hashCode()... -> Select float -> Generate)

@Override
public boolean equals(Object obj) {
	if (obj == null) {
		return false;
	}
	if (getClass() != obj.getClass()) {
		return false;
	}
	final Clazz other = (Clazz) obj;
	if (this.f != other.f) {
		return false;
	}
	if (this.d != other.d) {
		return false;
	}
	return true;
}
Comment 6 thomasbarnett 2009-09-01 01:39:12 UTC
To add a test case, using the Clazz class above:

Clazz x = new Clazz(Float.NaN);
System.out.print(x.equals(x)); // returns false

However, the Object equals() documentation [1] states that: 

"The equals method implements an equivalence relation:
- It is reflexive: For any reference value x, x.equals(x) must return true"


[1] http://java.sun.com/javase/6/docs/api/java/lang/Object.html#equals(java.lang.Object)
Comment 7 Quality Engineering 2010-02-11 21:23:25 UTC
Integrated into 'main-golden', will be available in build *201002120200* on http://bits.netbeans.org/dev/nightly/ (upload may still be in progress)
Changeset: http://hg.netbeans.org/main/rev/701520b26162
User: Jan Lahoda <jlahoda@netbeans.org>
Log: #156994: use Float.floatToIntBits and Double.doubleToLongBits to compare doubles and floats.
Comment 8 Dusan Balek 2012-09-11 14:55:27 UTC
*** Bug 218064 has been marked as a duplicate of this bug. ***
Comment 9 _ gtzabari 2012-09-11 15:20:07 UTC
Jan,

This issue was closed as FIXED back in 6.7 but it did not actually fix the problem I reported. It's nice that you fixed handling for float and double, but equals() is still wrong (violates Liskov's substitution principle).

Should I leave this issue as reopened or create a separate issue?
Comment 10 Jan Lahoda 2012-09-11 15:44:48 UTC
(In reply to comment #9)
> Jan,
> 
> This issue was closed as FIXED back in 6.7 but it did not actually fix the

I don't see this issue to be marked as fixed or resolved, and history does not say it ever was. True, the Target Milestone is set to 6.7, which I am going to reset.

> problem I reported. It's nice that you fixed handling for float and double, 

I have fixed float and double as the handling was wrong without any discussion.

but
> equals() is still wrong (violates Liskov's substitution principle).

First, I assume these are the equals methods you are proposing to generate:
--------------
public class Test {
    
    public static void main(String... args) {
        A a = new A();
        B b = new B();
        
        assert a.equals(b) == b.equals(a);
    }
    
    static class A {
        private int i = 0;

        @Override
        public int hashCode() {
            int hash = 7;
            hash = 53 * hash + this.i;
            return hash;
        }

        @Override
        public boolean equals(Object obj) {
            if (!(obj instanceof A)) {
                return false;
            }
            final A other = (A) obj;
            if (this.i != other.i) {
                return false;
            }
            return true;
        }
        
    }
    
    static class B extends A {
        private int j = 0;

        @Override
        public int hashCode() {
            int hash = 3;
            hash = 61 * hash + this.j;
            return hash;
        }

        @Override
        public boolean equals(Object obj) {
            if (!super.equals(obj)) return false;
            if (!(obj instanceof B)) {
                return false;
            }
            final B other = (B) obj;
            if (this.j != other.j) {
                return false;
            }
            return true;
        }
    }
    
}
--------------

But, these equals methods fail to fulfil the symmetry contract, and the behaviour of such objects is quite unpredictable in Collections - how are we going to respond to bugs claiming (validly, IMO), that the equals implementation is wrong?

Having said that, I am not excluding possibility to eventually introduce a checkbox "Generate equals violating its contract" - you can make that happen faster by providing a patch, though.
Comment 11 _ gtzabari 2012-09-11 16:23:10 UTC
Jan,

Can you please clarify how instanceof comparison violates equals() for
Collections?

Also, it's worth noting that the getClass() implementation of equals() violates
symmetry for sub-types that add convenience methods without modifying state.
Comment 12 Jan Lahoda 2012-09-11 22:11:10 UTC
(In reply to comment #11)
> Jan,
> 
> Can you please clarify how instanceof comparison violates equals() for
> Collections?

It can cause surprising or hardly predictable behaviour. Consider this example (an extension of the above):
------
    public static void main(String... args) {
        A a = new A();
        B b = new B();
        
        List<Object> l1 = new ArrayList<Object>();
        List<Object> l2 = new ArrayList<Object>();
        
        addUnique(l1, a);
        addUnique(l1, b);
        
        addUnique(l2, b);
        addUnique(l2, a);
        
        System.err.println("l1.size=" + l1.size());
        System.err.println("l2.size=" + l2.size());
    }
    
    private static <T> void addUnique(Collection<T> addTo, T toAdd) {
        if (!addTo.contains(toAdd)) addTo.add(toAdd);
    }
------

It prints this for me:
l1.size=2
l2.size=1

Which I would say is surprising at least. The example is artificial, because I tried to avoid dealing with hashcodes (which begs a question: should "a" and "b" from the example have the same hashcode or not?) - not following the hashcode&equals contracts for objects that are put into HashSet (or used as keys in HashMap) typically leads to hard-to-track bugs.

I realize there are usecases where the subclass is only modifying behavior, not data, where using instanceof is more or less OK. But in other usecases, it is likely to lead to "random" problems, and I prefer generating code that works in most cases, and fails more predictably (allowing to track and fix the failure) over code that works in most cases and fails less predictably.
Comment 13 _ gtzabari 2012-09-12 06:29:20 UTC
Good example. I still think you're wrong and I think I've got the proof to back it up...

All object-oriented languages are bound by the rules of Liskov Substitution Principal (LSP) as defined here: http://en.wikipedia.org/wiki/Liskov_substitution_principle. The principal is similar to Design by Contract.

Among other things, it states the following:

1. A method's specification (contract) consists of pre-conditions, post-conditions and invariants.
2. When a subclass inherits from a superclass, it cannot strengthen its pre-conditions, weaken its post-conditions or violate its invariants.

The equals() example you quoted is similar to the often-quoted "A square is not a rectangle" problem. Quoting http://stefanoricciardi.com/2009/07/17/code-contracts-and-inheritance/

"According to geometry, a square is a rectangle, but when it comes to software design... it is not!

The basic problem is that a square has an additional invariant with respect to the rectangle, which is that all the sides are equal and because of that you cannot independently change the height and the width like you would do with a rectangle"

LSP states that "if S is a subtype of T, then objects of type T may be replaced with objects of type S (i.e., objects of type S may be substituted for objects of type T) without altering any of the desirable properties of that program (correctness, task performed, etc.)"

A square may not inherit from rectangle because replacing a Rectangle with a Square would violate its contract.

In your example, B.equals() violates Object.equals()'s contract because:

1. Object.equals()'s contract of symmetry requires that "A.equals(B) should return true if and only if B.equals(A) returns true".
2. LSP states that if B inherits from A, then you must be able to substitute A with B without any noticeable difference.
3. Taking these two into consideration, A.equals(C) should return the same result as B.equals(C), but it does not.


I'll give you a real-life example of why violating LSP is *very* bad. A few years ago I tried using the RXTX library for serial-port communication. Everything was fine until I wrapped their InputStream implementation in a BufferedInputStream. All of a sudden the code began to blow up at random locations (sometimes inside the JDK codebase!). The specification for InputStream.read() clearly states that -1 denotes the end of stream, but they decided that -1 should mean "there is no data for you to read right now, try again later". Now, because they violated LSP you could not substitute their implementation in place of InputStream. When BufferedInputStream saw -1 it naturally assumed that the end-of-stream had been reached. It would cache this result and never invoke -1 again. Because class inheritance cannot work without LSP.


Here is a final link to prove my point (taking an example from Collections): http://docs.oracle.com/javase/7/docs/api/java/util/Collection.html#equals%28java.lang.Object%29

"The general contract for the Object.equals method states that equals must be symmetric (in other words, a.equals(b) if and only if b.equals(a)). The contracts for List.equals and Set.equals state that lists are only equal to other lists, and sets to other sets. Thus, a custom equals method for a collection class that implements neither the List nor Set interface must return false when this collection is compared to any list or set. (By the same logic, it is not possible to write a class that correctly implements both the Set and List interfaces.)"

Notice:

1. They remind you that Lists must be equal to other Lists and Sets must be equal to other Sets. They imply that when you implement equals(), you must ensure that this remains true (i.e. don't break symmetry!)

2. You can't break symmetry with respect to interfaces. For example, class "Collection" is not relevant to this discussion because it has no concrete implementation of equals().

3. All concrete classes (e.g. ArrayList, HashMap, TreeMap) extend base classes such as AbstractList or AbstractSet.

4. If you examine their implementation, you will discover that the base classes implement equals() using "instanceof" and none of the subclasses override equals(), precisely to avoid violating LSP.

Phew, that was a long post.

So in summary: code that violates LSP will cause unexpected behavior. equals() is just the tip of the iceberg. using getClass() in equals() is wrong because:

1. It violates LSP itself.
2. Programming errors (such as code that violates LSP) should fail-fast (the earlier the better). We shouldn't be accommodating them any more than we should be accommodating

int count = 5++;

throwing NullPointerException. They *should* cause the application to explode.
Comment 14 matthies 2013-06-27 11:25:30 UTC
I strongly agree with gtzabari. The current equals() implementation generated by NetBeans is dangerous and simply broken, and teaches a wrong pattern. An equals() implementation should by default work like this:

    if (obj == null)
        return false;
    if (obj instanceof ThisType) {
        ThisType other = (ThisType) obj;
        return this.field == null ? other.field == null : this.field.equals(other.field);
        // extend the above to all relevant fields
    }
    return false;
Comment 15 matthies 2013-06-27 11:27:55 UTC
(In reply to comment #14)
> I strongly agree with gtzabari. The current equals() implementation generated
> by NetBeans is dangerous and simply broken, and teaches a wrong pattern. An
> equals() implementation should by default work like this:
> 
>     if (obj == null)
>         return false;

Correction; these two lines should be:

 if (obj == this)
        return true;

>     if (obj instanceof ThisType) {
>         ThisType other = (ThisType) obj;
>         return this.field == null ? other.field == null :
> this.field.equals(other.field);
>         // extend the above to all relevant fields
>     }
>     return false;
Comment 16 Jiri Kovalsky 2013-07-08 14:52:16 UTC
It would be nice to have this RFE implemented by having the checkbox (not checked by default) in the NetBeans 8.
Comment 17 _ gtzabari 2013-07-08 14:59:19 UTC
(In reply to comment #16)
> It would be nice to have this RFE implemented by having the checkbox (not
> checked by default) in the NetBeans 8.

I personally consider this issue to be a bug, not an enhancement request (we are violating the Object.equals() contract). As such, please explain why you feel why the fix should be disabled by default.
Comment 18 Jan Lahoda 2013-07-08 15:15:19 UTC
(In reply to comment #17)
> (In reply to comment #16)
> > It would be nice to have this RFE implemented by having the checkbox (not
> > checked by default) in the NetBeans 8.
> 
> I personally consider this issue to be a bug, not an enhancement request (we
> are violating the Object.equals() contract). As such, please explain why you

Could you please quote the part of the Object.equals contract (i.e. the part of Object.equals javadoc) the current implementation breaks? Liskov principle is *not* part of the Object.equals explicit contract (read: its javadoc). Using instanceof is (in general) breaking symmetry and symmetry *is* part of the Object.equals contract.

> feel why the fix should be disabled by default.

Because instanceof of is in general breaking Object.equals contract.
Comment 19 _ gtzabari 2013-07-08 16:24:03 UTC
(In reply to comment #18)
> Could you please quote the part of the Object.equals contract (i.e. the part of
> Object.equals javadoc) the current implementation breaks? Liskov principle is
> *not* part of the Object.equals explicit contract (read: its javadoc). Using
> instanceof is (in general) breaking symmetry and symmetry *is* part of the
> Object.equals contract.
> 
> > feel why the fix should be disabled by default.
> 
> Because instanceof of is in general breaking Object.equals contract.

Jan,

You won't find Liskov substitution principle mentioned in any Javadoc. It is an architectural-level invariant of all object-oriented languages and affects much more than just Object.equals(). I encourage you to re-read comment #13 for a detailed explanation.

Here is an additional point (on top of comment #13 which I hope you've read by now):

"getClass() != obj.getClass()" prevents you from subclassing. Certainly you can extend a class and make equals() return false, but then you're not really subclassing. What you are doing is implementation inheritance, not behavior inheritance. What you *should* be using is composition: http://en.wikipedia.org/wiki/Composition_over_inheritance. This is true for C++ as much as it is true for Java.
Comment 20 matthies 2013-07-08 16:30:10 UTC
(In reply to comment #18)
> instanceof of is in general breaking Object.equals contract.

It only does if the wrong type is used on the right-hand side of instanceof. You always have some interface type or common base class that specifies the concrete equals() contract (e.g. "two lists are equal if and only if..."). It's clear that this type is the one that must be used on the RHS of instanceof.

Generally, there are two types of situations:

1) You have a simple value class with just that one implementation. In general such a class should be final. In that case, instanceof is perfectly symmetric and cannot go wrong.

2) You have a base type that specifies an equals() contract with multiple concrete implementations. In this case, the "this.getClass() == obj.getClass()" is likely to be wrong, unless the subclasses happen to represent, without exception, a partioning of the value space of the base type, which in my experience is not too common. It's more likely to have special-case subclasses similar to how for List you have empty-list and singleton-list implementations without every empty or singleton list necessarily being an instanceof the respective class, or to have added-guarantee or added-functionality subclasses like for example how LinkedHashSet is to HashSet.

The problem I have with the "this.getClass() == obj.getClass()" implementation is that it prevents creating subclasses with a compatible and symmetric equals() implementation. Suppose for example that Set was a concrete class instead of an interface (with an implementation like HashSet) and whose equals() implementation would use this test. Then it wouldn't be possible to create a LinkedSet or EnumSet subclass that would be compatible with Set in terms of equals(). All these type-2 situations require use of instanceof.

To me, the rule when implementing equals() is quite simple: Determine which is the type that specifies the particular equals() contract. Then test for instanceof that type. This also has the added benefit that the implementation is self-documenting in which type is the anchor of the equals() contract. When you see "this.getClass() == obj.getClass()", you always have to wonder whether the author did really think things through.

Ideally, the IDE would ask the user for that base type, for the purpose of creating an equals() implementation. Furthermore, the IDE would warn the user if a super class other than Object already provides an equals() implementation.
Comment 21 _ gtzabari 2013-07-08 16:45:52 UTC
> Using instanceof is (in general) breaking symmetry and symmetry *is* part of the
Object.equals contract.

I think this example will help get the point across:

1. I create class Rectangle which honors the Object.equals() contract.
2. I create class Square which extends class Rectangle. At this point, Square either violates Object.equals() or LSP.
3. If Rectangle.equals() uses instanceof, then Square violates symmetry.
4. If Rectangle.equals() uses getClass(), then Square violates LSP.

Why does violating LSP matter so much? Consider the following code:

class Test
{
  public static void main(String[] args)
  {
    Square square = new Square(10);

    // violates Square invariant which states that width == height
    rectangle.setWidth(20);
  }
}

Square is exposing implementation details, methods setWidth(), setHeight(), which violate its own invariants. What you should be doing instead is using composition:

class Square
{
  private final Rectangle rectangle = ...;

  public void setSize(int size)
  {
    rectangle.setWidth(size);
    rectangle.setHeight(size);
  }
}
Comment 22 matthies 2013-07-08 17:12:50 UTC
(In reply to comment #21)
> 3. If Rectangle.equals() uses instanceof, then Square violates symmetry.

Huh, why should that be the case? 

What would violate symmetry is if Square would override Rectangle's equals() implementation and test for "instanceof Square", because then "new Rectangle(5, 5).equals(new Square(5))" would yield true while "new Square(5).equals(new Rectangle(5, 5))" would yield false. However, there is no necessity for Square to override equals(); and if it does want to override equals(), it should test "instanceof Rectangle" and implement equals() in terms of shape equality to the other Rectangle.

> What you should be doing instead is using composition:

Rather, make Rectangle and Square immutable to achieve that a Square can actually be a Rectangle :)

(With mutability, a Rectangle is really a storage-for-any-rectangle and a Square is really a storage-for-any-square. You then get in trouble with subtyping because a storage-for-any-square is not a storage-for-any-rectangle.)
Comment 23 Jan Lahoda 2013-07-08 17:16:09 UTC
Regarding LSP: let us imagine a property "an object is only 'equals' to itself". This is definitely true for j.l.Object. But is not true for many subclasses of j.l.Object, which implement their own equals - why is this *not* a violation of LSP?

Regarding Collection.equals and like: yes, (sometimes), there is a way to implement correct equals in that case using instanceof over a super interface. But that is generally not what this feature is doing: this feature is generating equals based on the fields, but doing Collection.equals-like comparison generally requires usage of methods (rather than fields) and very deep understanding of the desired behaviour (List's and Set's equals apparently must be different). This feature is simply intended to relieve the tedious task of creating equals for simple value-holding classes.

Regarding composition: I don't know how it relates to this discussion. Sure, creating Square like above is wrong. Does the IDE suggest to create such Square? How does it do so? What the IDE is trying to do (and should do that by default in the future, IMO) is creating a code that will be behave more predictably if misused. Or is this meant that the IDE should only generate equals for final classes, to encourage composition over inheritance (which seems to be the only feasible explanation for bringing this theme up over and over)? I doubt that would work very well.

Having said all that - I understand there are technologies that basically require instanceof based equals. But that is the only argument so far that seems valid to me to add an option like this.
Comment 24 matthies 2013-07-08 17:49:34 UTC
(In reply to comment #23)
> Regarding LSP: let us imagine a property "an object is only 'equals' to
> itself". This is definitely true for j.l.Object. But is not true for many
> subclasses of j.l.Object, which implement their own equals - why is this *not*
> a violation of LSP?

Object provides a _default_ equals() implentation for all types that do not define a different equivalence relation. The contract of Object is NOT "an object is only equal to itself". Rather, that is just the default implementation provided by the Object class; it is not part of Object's interface contract. The contract of Object#equals() is specified by the five bullet points in the Javadoc of the method. Subtypes that, in their contract, define a more concrete equivalence relationship still conform to Object's contract, hence no LSP violation here.

[...]
> This feature is simply intended to relieve the tedious task
> of creating equals for simple value-holding classes.

In my opinion instanceof is perfectly correct for simple value-holding classes.

What I can understand to some degree is if you want to cater to inexperienced developers who don't really understand equals(), and provide a default implementation of equals() that is at least formally correct in the sense that it attempts to guarantee symmetry. However, this is to the detriment to developers like myself who use instanceof according to the principles I outlined in comment #20, and, more importantly, it teaches a pattern which is incorrect in the general case. It is almost never a good idea to have the getClass()-based equality test in a class designed for subclassing; at the same time the use of getClass() in the generated code suggests that it is in particular intended for the case of subclassing.
Comment 25 _ gtzabari 2013-07-08 18:01:54 UTC
(In reply to comment #22)
> (In reply to comment #21)
> > 3. If Rectangle.equals() uses instanceof, then Square violates symmetry.
> 
> Huh, why should that be the case? 

My mistake. instanceof does not violate symmetry but this case would still violate LSP: "Invariants of the supertype must be preserved in a subtype."

> > What you should be doing instead is using composition:
> 
> Rather, make Rectangle and Square immutable to achieve that a Square can
> actually be a Rectangle :)
> 
> (With mutability, a Rectangle is really a storage-for-any-rectangle and a
> Square is really a storage-for-any-square. You then get in trouble with
> subtyping because a storage-for-any-square is not a storage-for-any-rectangle.)

Immutability wouldn't help.

1. Rectangle.setWidth() must change the width independent of the height. 
2. Rectangle.setHeight() must change the height independent of the width. 
3. Square.setSize() must change both dimensions.
4. Square width/height must be equal.

Even if the setters were to return a new (immutable) Rectangle or Square, there would be no way for you to prevent a user from violating #4.

(In reply to comment #23)
> Regarding LSP: let us imagine a property "an object is only 'equals' to
> itself". This is definitely true for j.l.Object. But is not true for many
> subclasses of j.l.Object, which implement their own equals - why is this *not*
> a violation of LSP?

You're confusing implementation details with the method contract. LSP only states that the class and its sub-types must implement the same contract. It does not state that the underlying implementation must remain the same. More specifically, can you provide an example where A is an Object and B is a sub-type, A.equals(B) but !B.equals(A)?

> Having said all that - I understand there are technologies that basically
> require instanceof based equals. But that is the only argument so far that
> seems valid to me to add an option like this.

No. You are missing a fundamental point here: implementations *must* honor LSP, otherwise you are breaking the basic premise behind OOP. The *only* time it is acceptable to use a getClass()-based equals is for final classes. That makes them the exception to the rule, not the opposite.
Comment 26 matthies 2013-07-09 09:51:09 UTC
(In reply to comment #25)
> (In reply to comment #22)
> > (With mutability, a Rectangle is really a storage-for-any-rectangle and a
> > Square is really a storage-for-any-square. You then get in trouble with
> > subtyping because a storage-for-any-square is not a storage-for-any-rectangle.)
> 
> Immutability wouldn't help.
> 
> 1. Rectangle.setWidth() must change the width independent of the height. 
> 2. Rectangle.setHeight() must change the height independent of the width. 
> 3. Square.setSize() must change both dimensions.
> 4. Square width/height must be equal.
> 
> Even if the setters were to return a new (immutable) Rectangle or Square, there
> would be no way for you to prevent a user from violating #4.

For an immutable solution you simply wouldn't provide setters. The client code would pass the dimensions of any desired new instance to the constructor, and that's it.
Comment 27 _ gtzabari 2013-07-09 15:49:29 UTC
(In reply to comment #26)
> For an immutable solution you simply wouldn't provide setters. The client code
> would pass the dimensions of any desired new instance to the constructor, and
> that's it.

Fair enough, but to reiterate my point: you should never use class inheritance for implementation reuse. In the aforementioned case, the design would be better off using composition and only exposing Square-specific methods. Using immutability doesn't protect you from having to expose Rectangle-specific methods to all subclasses. Once you add a couple of levels of implementation inheritance you end up with a method soup, as we have in Swing.

Getting back to the point. This issue is assigned to Jan and found the sound of it, he's yet to agree that LSP is a fundamental requirement of the Java language. Jan, if this is true, would it be possible to get two other high-ranking committers to weigh in (we need an odd number of people to break a tie)?
Comment 28 Jiri Kovalsky 2013-07-10 22:33:27 UTC
Let's not forget about it into the next NetBeans version.
Comment 29 markiewb 2013-08-10 13:20:55 UTC
*** Bug 163561 has been marked as a duplicate of this bug. ***
Comment 30 struberg 2013-10-29 21:09:43 UTC
There is just no way to get equals() and hashCode() done right IF you do let JPA set the Id. 

The only way it works is by either use a UUID or a manually managed sequence counter and set the Id yourself _before_ you do anything with that entity.

Thus I filed an issue to completely remove equals() and hashCode() at all.

https://netbeans.org/bugzilla/show_bug.cgi?id=237780
Comment 31 _ gtzabari 2013-10-29 22:14:23 UTC
@struberg,

I disagree for two reasons:

1. Non-JPA code is not affected (and it makes up the majority of Java code).
2. I consider JPA, as a technology, to be completely broken. See http://blogs.tedneward.com/2006/06/26/The+Vietnam+Of+Computer+Science.aspx. As such, I don't use it at all. Take it from someone who used Hibernate for over 4 years, it will cause more pain than it'll solve.
Comment 32 _ gtzabari 2015-07-28 03:26:06 UTC
Please update Target Milestone (it is now out of date).
Comment 33 Zom-B 2015-11-11 13:46:17 UTC
This thread is tl;dr so here goes nothing:

I'd never consider two concrete classes of different `getclass()` types to be equal regardless of `instanceof` relationships.

Take for example EJ Item 8: I consider `Point` and `CounterPoint` instances as always different (`equals()` returns false) just because `Point` has two properties and `CounterPoint` in total has 3 properties.

Another example: I consider `JButton` to be different from `MyJButton extends JButton` even if I only add functionality and no extra properties. If I wanted to know if the properties of my `MyJButton` are the same as that of another `JButton`, I'd compare properties instead of using `equals()`. This also has the advantage of deciding which properties contribute to your definition of 'equality' instead of having the library designers decide that for you.
Comment 34 _ gtzabari 2015-11-11 16:40:41 UTC
Zom,

I understand what it is you want, but this is not what equals() is designed for. What you are asking for violates equals() requirement of symmetry (JButton equals MyJButton but the latter is not true).

If you want to add sameClass() to all your implementations so be it, but you can't implement equals() this way without violating the specification. If you do so, you *will* run into problems.

This issue is about Netbeans respecting the Java Specification for Object.equals(). Users who want something different are welcome to declare a different method.
Comment 35 offbynull 2016-01-13 05:32:12 UTC
This was reported back in 09. Is there any way that we can get an option to generate the alternate version?
Comment 36 Svata Dedic 2016-01-13 08:45:53 UTC
Still there's no conclusion to be implemented, most of the thread tries to present one of the options as the "optimal", but none of them is.

There probably is no good way to implement "correct equals". IMHO, arguments as "this is not what equals was desinged for" are misleading, because in the first place, the Object.equals contract was wrongly designed from the start, as Effective Java book and discussion in this thread shows: equals on subclasses will either break subtyping (use getClass()) or will break symmetry (equals contract - use instanceof).

Given that I do not consider this issue a defect, rather an enhancement because we lack features, the code as it is designed is working well.

In a very few selected cases, it is possible to instanceof compare to base class, but those cases are IMHO not detectable by the IDE.

So the here's the solution:
* add a fix that Object.equals generates instanceof. Add a checkbox to the 'generate hashCode and equals' feature. Add option to hint to choose from instanceof or getClass()

* issue a warning when generating an overriding equals(), while adding some new felds (it would be best to show the warning only if the superclass uses instanceof)

I would go even so far to suggest that the instanceof should be preferred, because for subclasses with not-important fields, instanceof works while getClass() not. But what is a key or unimportant key is a matter of design and THAT's why we choose fields to be included in equals().

It is not effective to add a hint/warning which would warn the user against equals() in a non-final class, since this is how we all write usually.

It COULD be possible to warn if an instance of class A extends B:
1/ is used in a collection, whose type parameter is a class B or superclass of B
2/ type parameter class defines equals
3/ A also defines equals
(4/ or some other subclass of B defines equals - requires index access)

In this case, it is just a matter of optimization of collection operations, whether A.equals(B) or B.equals(A) is called. Sure this does catch only a very few situations and is rather complex.
Comment 37 _ gtzabari 2016-01-13 10:02:39 UTC
Svata,

+1. I like the overall direction you are taking this in.
Comment 38 offbynull 2016-01-14 01:50:19 UTC
Leave it up the user to decide which form of equals is correct / the one they want.

- A checkbox to use instanceof
- A checkbox to test against super

If instanceof is chosen, we can have a little warning message saying that it breaks the contract for equals / may have undesired side-effects. Also, retain the state of the checkboxes across the running NetBeans session -- in case the user is adding equals/hashCode for a batch of similar classes.
Comment 39 offbynull 2016-01-14 01:53:23 UTC
'test against super' checkbox would also apply to hashCode. It would be nice to have 'super' as an option when generating toString as well.

I can try writing a patch for this if someone can point me in the right direction.
Comment 40 matthies 2016-01-14 03:20:13 UTC
I would suggest to not warn against instanceof, at least not unconditionally. Instead I would make instanceof the default as proposed by Svata. Warnings, if any, rather belong in the subclass, like for example FindBugs' EQ_OVERRIDING_EQUALS_NOT_SYMMETRIC and EQ_DOESNT_OVERRIDE_EQUALS.

One way I see it is this:

– Using getClass() prohibits equals()-compatible subclasses
– Using instanceof prohibits equals()-incompatible subclasses (unless the class is abstract)
– For final classes, it doesn't make a difference.

(By equals()-compatible, I mean the situation we have with subclasses of List or Set, for example. By equals()-incompatible, I mean the partitioning of the equality relation that results from using getClass().)

So which option anyone prefers depends on whether one prefers equals()-compatibility or equals()-incompatibility across subclasses. As I believe that classes should only be extended for implementing a subset of the value space of the super class (= LSP-compatible class specialization), and that implementation-only inheritance should be replaced by delegation, I am firmly in the equals()-compatibility camp.

Now, I don't expect to convince everyone to join that camp. But please recognize that it is a sound design philosophy, and that following it _requires_ using instanceof. Hence a general warning against it would be counterproductive.
Comment 41 _ gtzabari 2016-01-14 08:34:02 UTC
I second matthies and svata's suggestion [1] but I suggest against using loaded terminology such as "equals-compatible" because users who favor getClass() dispute that their approach cannot be compatible (it is... but for a smaller subset of designs than instanceof).

The key difference between instanceof and getClass() is that the requirement that implementations be "symmetric" (if A equals B, then B must equal A). instanceof can be symmetric in some cases, getClass() can never be.

[1] I am in favor of:

1. Making "instanceof" the default.
2. Warning if a class uses "instanceof" and subclasses override equals/hashCode.
3. Allowing users to configure whether they prefer instanceof or getClass()-based implementations of equals/hashcode on a project-level.
4. Warning if a class uses "getClass()" and has any subclasses.
5. Warnings should appear on the subclasses, not the parent class.
Comment 42 offbynull 2016-01-15 02:19:06 UTC
What's being proposed, in terms of FB-style analysis and what not, sounds like a lot of work.

Just give the user the relevant options, with the established standard being the default. Correct me if I'm wrong, but generating equals() with getClass() seems to be the established standard across major Java IDEs -- it's the only option in NetBeans and the default option in Eclipse.
Comment 43 _ gtzabari 2016-01-16 12:18:50 UTC
offbynull,

It's not that simple. The entirety of the JDK source-code and many libraries in the wild (e.g. Guava) contradict what you consider to be "standard". I encourage you to read the entire discussion thread to gain a better understanding of why that is.
Comment 44 offbynull 2016-01-19 01:55:13 UTC
I think the most important thing at this point is to just get an implementation going. We can worry about finer details later.


By use of this website, you agree to the NetBeans Policies and Terms of Use. © 2014, Oracle Corporation and/or its affiliates. Sponsored by Oracle logo