493363 - Several issues in Utils.deepEquals()

Status: RESOLVED FIXED

(Cloud Services :: General, defect)

Description

[Wladimir Palant]

Apparently, my issue in bug 493256 was partially caused by Utils.deepEquals() not doing an adequate comparison. One issue is that all of the following will return true:

Utils.deepEquals({foo: false}, {})
Utils.deepEquals({foo: false}, {foo: null})
Utils.deepEquals({foo: false}, {foo: 0})

These cases are all handled by these checks at the beginning of the function:

    if (!a && !b)
      return true;
    if (!a || !b)
      return false;

I think the idea here is to handle null in a special way because of |typeof null == "object"|. But treating false, undefined, 0, "" in the same way shouldn't be intentional. Better:

    if (a === null && b === null)
      return true;
    if (a === null || b === null)
      return false;

Note the use of === operator because of |null == undefined|.

The other issue that these checks also return true:

Utils.deepEquals({foo: 1}, {foo: true})
Utils.deepEquals({foo: 1}, {foo: "1"})

That's because of the implicit type conversion done by the comparison here:

    if (typeof(a) != "object" && typeof(b) != "object")
      return a == b;

Using === instead of == fixes the issue.

Finally, this check also returns true:

Utils.deepEquals({foo: 1}, {foo: 1, bar: 2})

This is because this code only considers the keys of the first argument:

      for (let key in a) {
        if (!Utils.deepEquals(a[key], b[key]))
          return false;
      }

The assumption seems to be that the structure of the objects compared will always be the same. However, depending on the record this might actually not be the case - and Weave doesn't have control over the records being passed to it, there is an API that extensions might use.

Comment 1

[Wladimir Palant]

Attachment: Proposed patch

Comment 2

[Dan Mills [:thunder]]

Haven't read the patch closely, but looks good.  Thanks!  I think it makes sense for deepEquals to do strict equality (===).

Comment 3

[Ed Lee :Mardak]

Comment on attachment 378265 [details] [diff] [review]
Proposed patch

Wladimir, thanks for the patch. I see a diagonal of 1s with my little test script:
d1 = [NaN, undefined, null, true, false, Infinity, 0, 1, "a", "b", {a: 1}, {a: "a"}, [{a: 1}], [{a: true}], {a: 1, b: 2}, [1, 2], [1, 2, 3]];
d2 = [NaN, ... same as above];

d1.forEach(function(a) { o = []; d2.forEach(function(b) o.push(Utils.deepEquals(a,b) ? 1 : 0)); print(o.concat([typeof a, a, JSON.stringify([a])]).join(" ")) })

r=Mardak

It works, but we can do even better.. patch coming up!

Comment 4

[Ed Lee :Mardak]

http://hg.mozilla.org/labs/weave/rev/3cbddf0d4d41

Use triple-equals to determine if two things are the same; otherwise, check if both are objects with the same keys and same values for the keys.

Comment 5

[Ed Lee :Mardak]

If anybody is interested.. the original deepEquals code had a result like..

1 1 1 0 1 0 1 0 0 0 0 0 0 0 0 0 0 number NaN [null]
1 1 1 0 1 0 1 0 0 0 0 0 0 0 0 0 0 undefined  [null]
1 1 1 0 1 0 1 0 0 0 0 0 0 0 0 0 0 object  [null]
0 0 0 1 0 0 0 1 0 0 0 0 0 0 0 0 0 boolean true [true]
1 1 1 0 1 0 1 0 0 0 0 0 0 0 0 0 0 boolean false [false]
0 0 0 0 0 1 0 0 0 0 0 0 0 0 0 0 0 number Infinity [null]
1 1 1 0 1 0 1 0 0 0 0 0 0 0 0 0 0 number 0 [0]
0 0 0 1 0 0 0 1 0 0 0 0 0 0 0 0 0 number 1 [1]
0 0 0 0 0 0 0 0 1 0 0 0 0 0 0 0 0 string a ["a"]
0 0 0 0 0 0 0 0 0 1 0 0 0 0 0 0 0 string b ["b"]
0 0 0 0 0 0 0 0 0 0 1 0 0 0 1 0 0 object [object Object] [{"a":1}]
0 0 0 0 0 0 0 0 0 0 0 1 0 0 0 0 0 object [object Object] [{"a":"a"}]
0 0 0 0 0 0 0 0 0 0 0 0 1 1 0 0 0 object [object Object] [[{"a":1}]]
0 0 0 0 0 0 0 0 0 0 0 0 1 1 0 0 0 object [object Object] [[{"a":true}]]
0 0 0 0 0 0 0 0 0 0 0 0 0 0 1 0 0 object [object Object] [{"a":1,"b":2}]
0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 1 0 object 1,2 [[1,2]]
0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 1 object 1,2,3 [[1,2,3]]

Comment 6

[Ed Lee :Mardak]

Oh and one difference is that the new deepEquals(NaN, NaN) is false while the old one is true... not sure what it should be because NaN is supposed to always be != to anything.. even another NaN.

Comment 7

[Ed Lee :Mardak]

Oh duh. I should have made that debug output print " " instead of 0 ;)

d1 = [NaN, undefined, null, true, false, Infinity, 0, 1, "a", "b", {a: 1}, {a: "a"}, [{a: 1}], [{a: true}], {a: 1, b: 2}, [1, 2], [1, 2, 3]];
d2 = [NaN, undefined, null, true, false, Infinity, 0, 1, "a", "b", {a: 1}, {a: "a"}, [{a: 1}], [{a: true}], {a: 1, b: 2}, [1, 2], [1, 2, 3]];

Utils.deepEquals = function eq(a, b) {
  if (a === b)
    return true;

  let [A, B] = [[i for (i in a)], [i for (i in b)]];
  if (typeof a != "object" || typeof b != "object" || A.length != B.length)
    return false;

  return A.every(function(A) B.some(function(B) A == B && eq(a[A], b[B])));
};

d1.forEach(function(a) { o = []; d2.forEach(function(b) o.push(Utils.deepEquals(a,b) ? 1 : " ")); print(o.concat([typeof a, a, JSON.stringify([a])]).join(" ")) })

                                  number NaN [null]
  1                               undefined  [null]
    1                             object  [null]
      1                           boolean true [true]
        1                         boolean false [false]
          1                       number Infinity [null]
            1                     number 0 [0]
              1                   number 1 [1]
                1                 string a ["a"]
                  1               string b ["b"]
                    1             object [object Object] [{"a":1}]
                      1           object [object Object] [{"a":"a"}]
                        1         object [object Object] [[{"a":1}]]
                          1       object [object Object] [[{"a":true}]]
                            1     object [object Object] [{"a":1,"b":2}]
                              1   object 1,2 [[1,2]]
                                1 object 1,2,3 [[1,2,3]]

Comment 8

[Dan Mills [:thunder]]

This seems like a good candidate for a unit test!

Comment 9

[Ed Lee :Mardak]

http://hg.mozilla.org/labs/weave/rev/89b98bf9e119

Add test for Utils.deepEquals