Value equality

[Brian Goetz]

So, its worth pointing out here that the only disagreement is on a 
pretty cornery (and fundamentally messy) case -- arrays as components of 
a value.

I think there's no dispute over:
  - primitives components compared with ==(p,p)
  - value components compared recursively with ==(v,v)
  - non-array reference components compared recursively with .equals()

So the only remaining question is what to do with arrays (and, for 
equals vs deepEquals, these differ only when arrays are nested within 
arrays within values -- very much a corner case).  Options include:
  - reference equality / Object.equals()
  - Arrays.equals on primitive / value arrays, but reference equality on 
object arrays
- Arrays.equals
  - Arrays.deepEquals

The first says "are they the same array".  The second lifts equality 
onto components known to be immutable, but punts on mutable array 
components.  The third lifts .equals() equality onto potentially-mutable 
array components; the third goes farther and recursively lifts this 
definition of equality onto elements that are actually arrays (which 
must be done reflectively -- see the nasty code below which is called on 
each element.)

In what cases will you have an array nested in an array as a value 
component?  I'm having a hard time thinking of any -- can you dig up 
examples?

The cases where I'd have an ordinary array in a value fall into two 
(opposite) categories:
  - Where the array is effectively immutable and holds the state of the 
value, such as a string-like value class (holding a char[]) or a 
BigDecimal-like value class (holding a long[]);
  - Where the value is behavior like a cursor into an array.

In the first case, two BigDecimal are equals() if they have the same 
array contents, so at first we might think Arrays.equals() is what we 
want (and same for the String case.)  But if I were coding a BigDecimal 
value, I wouldn't want this default anyway -- since I want to treat 1.0 
and 1.00 as equal, which the default won't do.  So while the default is 
OK, I'm still going to override it (and similarly for String), because 
Arrays.equals still produces false negatives.

In the cursor case, we definitely want to compare the arrays by 
reference -- two cursors represent the same thing if they point to the 
same element in the SAME array, not equivalent arrays. And, same with an 
Optional<Foo[]>.

So, grading the possibilities:
  - reference equality -- produces right answer for cursor, produces 
wrong answer for BigDecimal/String cases.
  - Arrays.equals -- produces wrong answer for cursor, produces 
tempting-seeming but still wrong answer for BigDecimal/String.

So both cases produce the wrong answer for the value-like uses of arrays 
-- and the user has to override the default anyway. The only one that 
produces the right answer in any case is reference equality (which is 
.equals()).  Maybe there's more cases, but these are the ones that come 
immediately to mind.

For arrays, it seems that trying to read the user's mind here is a 
losing (and at the same time, expensive) game.  Arrays are what they 
are; trying to turn them into acts-like-lists when you don't know the 
semantics of the array in their enclosing value seems pretty 
questionable.  Which leads me to prefer the following simpler default 
for equals() on values:

  - compare primitive and components with ==
  - compare reference components with .equals()

Do you have data on what percentage of your AutoValue objects contains 
arrays?

Secondary question: to what degree is the "if you don't like the 
default, write your own" mitigated by helpers like Guava's 
Objects.equals?  (It does kind of suck that if you disagree with the 
default treatment of one field, you have to rewrite the whole 
equals/hashCode methods.)




FYI, nasty and expensive method called on every element by deepEquals:

static boolean deepEquals0(Object e1, Object e2) {
     assert e1 !=null;
     boolean eq;
     if (e1instanceof Object[] && e2instanceof Object[])
         eq =deepEquals ((Object[]) e1, (Object[]) e2);
     else if (e1instanceof byte[] && e2instanceof byte[])
         eq =equals((byte[]) e1, (byte[]) e2);
     else if (e1instanceof short[] && e2instanceof short[])
         eq =equals((short[]) e1, (short[]) e2);
     else if (e1instanceof int[] && e2instanceof int[])
         eq =equals((int[]) e1, (int[]) e2);
     else if (e1instanceof long[] && e2instanceof long[])
         eq =equals((long[]) e1, (long[]) e2);
     else if (e1instanceof char[] && e2instanceof char[])
         eq =equals((char[]) e1, (char[]) e2);
     else if (e1instanceof float[] && e2instanceof float[])
         eq =equals((float[]) e1, (float[]) e2);
     else if (e1instanceof double[] && e2instanceof double[])
         eq =equals((double[]) e1, (double[]) e2);
     else if (e1instanceof boolean[] && e2instanceof boolean[])
         eq =equals((boolean[]) e1, (boolean[]) e2);
     else eq = e1.equals(e2);
     return eq;
}




On 5/17/2016 1:06 AM, John Rose wrote:
> On May 16, 2016, at 8:29 PM, Kevin Bourrillion <kevinb at google.com 
> <mailto:kevinb at google.com>> wrote:
>>
>> And by "deep equals" I mean equivalent behavior to
>>
>> https://docs.oracle.com/javase/8/docs/api/java/util/Arrays.html#deepEquals-java.lang.Object:A-java.lang.Object:A-
>>
>> at least that is what AutoValue has been doing.
>
> To the extent I understand this, it seems wrong:  deepEquals treats 
> arrays as if they are lists, which is an abstraction shift.  Surely 
> you aren't suggesting cracking a component reference in a subfield and 
> treating its object as a List of its components, and so on 
> recursively?  Because that's what deepEquals does.
>
> Or do you mean that, just as deepEquals avoids using op==(ref,ref) on 
> array components, so "deep equals" should avoid using op==(ref,ref) on 
> value components?
>
> You can avoid op==(ref,ref) by replacing it by a call to 
> ref.equals(ref), or you can avoid op==(ref,ref) by cracking the refs 
> and recursing.  That latter breaks an abstraction, so I think we agree 
> it's bad, but *that* is a more "equivalent behavior" to Arrays.deepEquals.
>
> — John