Add support for comparing cyclic structures #244

ghost · 2011-07-13T01:45:00Z

As per the discussion in issue #240, I've added support for comparing cyclic structures to _.isEqual. All the unit tests, including my nine additions, pass.

missing but Array.prototype.forEach exists.

#177

Fixed any/some formatting to be consistent with the rest of underscore.js

michaelficarra · 2011-07-13T03:41:31Z

underscore.js

    if ((!a && b) || (a && !b)) return false;
+    // `NaN` values are toxic.
+    if (_.isNaN(a) || _.isNaN(b)) return false;


I think this should be an egal comparison:

if (_.isNaN(a)) return _.isNaN(b);

This isn't backward-compatible, but I agree nonetheless.

michaelficarra · 2011-07-13T04:10:10Z

LGTM other than my comments. +1 from me.

michaelficarra · 2011-07-13T04:30:30Z

underscore.js

+  // Internal recursive comparison function.
+  function eq(a, b, stack) {
+    // Identical objects are equal.
+    if (a === b) return a != 0 || 1 / a == 1 / b;


Now it requires two divisions for any strictly equal falsey values a and b instead of just 0 or -0 as my suggestion would have done. Example: a = b = false. Use !==.

Edit: looks like @kflorence beat me to it.

Actually, the division would only be executed for the value false...null !=0 and void 0 != 0. I agree, though; it's unnecessary. The latest commit should fix this. Thank you for your other suggestions as well.

Empty string: '' == 0. But you're right that not all falsey values would execute the division operations.

Ah, yes. I'd forgotten about the empty string. Thanks!

…d in the prototype chain.

jashkenas · 2011-07-13T15:32:36Z

underscore.js

-    for (var key in a) if (!(key in b) || !_.isEqual(a[key], b[key])) return false;
-    return true;
+    // Ensure that both objects contain the same number of properties.
+    var result = aKeys.length == bKeys.length;


What's the point of comparing the relative lengths of just the own keys? All keys, including prototypal ones, are going to be compared in the next step.

This was in the original _.isEqual function, so I assumed that it was intentional. If it wasn't, then we can have the function compare either own properties or all properties. The deepEqual method in the CommonJS unit testing specification adopts the former approach, but this is clearly problematic for objects that have no own properties and different prototype chains. I use the latter approach in Spec and Maddy, since objects with different inherited properties are clearly not equivalent. This will be more performant as well, since it won't require a dependency on _.keys, and won't loop twice for objects with different sizes. Want me to patch it?

Yes ... but these sort of changes / tweaks are why I'm still uneasy with merging in a homebrew deep-equal comparison function. It would be awfully nice to have something like this tested in the wild for a while before merging. But so it goes.

Done. I can see why you're uneasy, but I also think it'd be useful for other developers to have a deep comparison function to use, rather than writing their own.

…he beginning.

This reverts commit 174c897.

…mplementation, but oh well. #245

boucher · 2011-07-13T19:32:18Z

underscore.js

+  // Internal recursive comparison function.
+  function eq(a, b, stack) {
+    // Identical objects are equal.
+    if (a === b) return a !== 0 || 1 / a == 1 / b;


0===0, so I don't quite understand the logic behind the rest of this line.

0 === -0, but the two aren't identical. 1 / 0 == Infinity; 1 / -0 == -Infinity.

Interesting. Hadn't considered -0. I think it's confusing enough to warrant
a comment though.

On Wed, Jul 13, 2011 at 12:35 PM, kitgoncharov <
reply@reply.github.com>wrote:

@@ -595,44 +593,67 @@
return obj;
};

// Perform a deep comparison to check if two objects are equal.

_.isEqual = function(a, b) {

// Check object identity.

if (a === b) return true;

// Different types?

var atype = typeof(a), btype = typeof(b);

if (atype != btype) return false;

// Basic equality test (watch out for coercions).

// Internal recursive comparison function.

function eq(a, b, stack) {

// Identical objects are equal.

if (a === b) return a !== 0 || 1 / a == 1 / b;

0 === -0, but the two aren't identical. 1 / 0 == Infinity; 1 / -0 == -Infinity.

Reply to this email directly or view it on GitHub:
https://github.com/documentcloud/underscore/pull/244/files#r64357

Sure. I'll add it in the next commit.

You should probably include a link to harmony:egal in the comment

…native implementation, but oh well. #245" This reverts commit f099038.

This reverts commit 47fb3d7.

michaelficarra · 2011-07-13T20:17:10Z

underscore.js

    // Unwrap any wrapped objects.
    if (a._chain) a = a._wrapped;
    if (b._chain) b = b._wrapped;
-    // One of them implements an isEqual()?
+    // Invoke a custom `isEqual` method if one is provided.
    if (a.isEqual) return a.isEqual(b);
    if (b.isEqual) return b.isEqual(a);


If a does not have an isEqual method (or it returns false), then we should just return false upon seeing this condition, right? Unless we foresee someone wanting non-commutative equality. But that's almost as crazy as irreflexive equality, and nobody would be crazy enough to do that...

edit: added links for @jdalton

@michaelficarra: Like this? if (a.isEqual && !a.isEqual(b)) return false;.

@kitgoncharov: like this:

if(typeof a.isEqual == 'function') return a.isEqual(b); if(typeof b.isEqual == 'function') return false;

PS: I always make sure [[Call]] is implemented before attempting to call something that may not be a function. And any object with [[Call]] will respond to typeof with 'function'.

padolsey · 2011-07-13T21:09:48Z

underscore.js

    if (a == b) return true;
-    // One is falsy and the other truthy.
+    // Optimization; ensure that both values are truthy or falsy.
    if ((!a && b) || (a && !b)) return false;


Alternative solution:

if (!a != !b) return false;

For more clarity, this also works:

if (!a == !!b) return false;

+1 first alternative solution

jashkenas · 2011-07-15T13:29:32Z

It would be worth making sure that whatever algorithm we want to adopt here also passes the QUnit suite of deep equality tests:

https://github.com/jquery/qunit/blob/master/test/same.js

... and we should take a look at the implementation of QUnit.equiv, as well.

Link to Underscore.php in docs

bman654 · 2011-08-12T07:09:44Z

underscore.js

+    // Compare object types.
+    var typeA = typeof a;
+    if (typeA != typeof b) return false;
+    // The type comparison above prevents unwanted type coercion.
    if (a == b) return true;


Isn't a == b guaranteed to be false at this point?
We already know that a !== b && typeof a != typeof b. Doesn't that imply a != b?

@bman654: no:

$ node > a = 0 0 > b = '' '' > a !== b true > typeof a != typeof b true > a == b true >

edit: also, sorry for taking so long to respond. I literally had this tab open since you asked that question and didn't have a chance to get back to you.

edit again: it looks like your "what we know" was mistyped and I found a counterexample to that instead of the real source. You meant "we already know that a !== b && typeof a == typeof b. Hmm, let me think about this one. It looks like you're right, though.

@michaelficarra The only case I can think of where that may present a problem is an object with a custom valueOf method, such as {valueOf: function () { return 1; }}. Since the == algorithm uses an identity comparison if both operands are objects, however, a == b is never true.

@kitcambridge: I'm pretty confident that @bman654 was right. I'd make the change.

@michaelficarra

Merging @michaelficarra's additions...

michaelficarra · 2011-09-04T23:40:17Z

@jashkenas (and other participants, collaborators, and maintainers): I think this pull is ready to be reviewed now.

ghost · 2011-09-05T16:41:42Z

@michaelficarra Not quite; I need to fix some last-minute bugs and add more unit tests. Temporarily closing the pull request...

michaelficarra · 2011-09-05T17:11:24Z

@kitcambridge: whoops, sorry for jumping the gun. I thought it was ready.

…. Ignore inherited properties when deep comparing objects. Use a more efficient `while` loop for comparing arrays and array-like objects.

ghost · 2011-09-05T18:39:36Z

underscore.js

+    var isNumberA = _.isNumber(a), isNumberB = _.isNumber(b);
+    if (isNumberA || isNumberB) return isNumberA && isNumberB && +a == +b;
+    // Compare boolean objects by value. The value of `true` is 1; the value of `false` is 0.
+    var isBooleanA = toString.call(a) == '[object Boolean]', isBooleanB = toString.call(b) == '[object Boolean]';


Note that this [[Class]] check is used to compare boolean object wrappers, not primitives..._.isBoolean returns false for boolean objects. I'd suggest adding this check to _.isBoolean instead:

_.isBoolean = function(obj) { return obj === true || obj === false || typeof obj == 'object' && toString.call(obj) == '[object Boolean]'; };

Thoughts?

ghost · 2011-09-05T21:52:56Z

Okay, I think this is now ready for review. Thanks for your help, everyone.

ghost · 2011-09-05T22:06:41Z

underscore.js

+    // Optimization; ensure that both values are truthy or falsy.
+    if (!a != !b) return false;
+    // `NaN` values are equal.
+    if (_.isNaN(a)) return _.isNaN(b);


My previous comment proposed adding the expression if (a != b) return false; below this line. This would have worked in one of my local branches, where I attempted to consolidate this series of checks into a single do...while loop, but would have clearly failed in this context. My apologies for any confusion; I'm not sure why I even posted that.

Yeah, I was confused so I decided to temporarily ignore it. Good thing, I guess.

jashkenas · 2011-10-04T19:22:14Z

Two further requests on this ... Can we remove the change to _.isBoolean, and make this _.deepEqual instead of changing the current meaning of _.isEqual ?

jashkenas · 2011-10-04T19:28:01Z

Nevermind ... the change to the semantics is fine -- I'll tweak the boolean.

ghost · 2011-10-04T19:40:40Z

@jashkenas Thanks. I'll be off to class shortly, so I'm afraid I won't be able to make any changes for the next hour and a half. When I return, though, would you like for me to update the documentation as well?

jashkenas · 2011-10-04T19:45:20Z

No worries -- I'm working through it all now.

jashkenas · 2011-10-04T19:57:10Z

@kitcambridge and @michaelficarra -- thanks for all the great work on this. Landed on master.

ghost · 2011-10-04T21:24:16Z

Fantastic. Thank you!

michaelficarra · 2011-10-04T22:03:23Z

Awesome. I loved this pull. Many thanks to @kitcambridge, this wouldn't have been started without you. Underscore's isEqual is infinitely more useful thanks to these efforts.

albemuth and others added 4 commits May 3, 2011 21:16

Add optional iterator to _.uniq

b930716

Fixing _.any returning an incorrect result when Array.prototype.some is

6846d71

missing but Array.prototype.forEach exists.

Refactored fix as suggested in

5141a51

#177

Modified any/some test case to demonstrate issue #177

40af165

Fixed any/some formatting to be consistent with the rest of underscore.js

michaelficarra reviewed Jul 13, 2011
View reviewed changes

Issue #243. _.functions now also returns the names of function define…

cd236d4

…d in the prototype chain.

jashkenas reviewed Jul 13, 2011
View reviewed changes

jashkenas and others added 9 commits July 13, 2011 12:21

Issue #103, change initial value detection for reduce()

174c897

Issue #209, fix reduce without a memo for arrays that are sparse at t…

5edc376

…he beginning.

Unit tests for sparse arrays.

7573ac7

Merge branch 'underscore.any' of https://github.com/shinuza/underscore

779a28d

function formatting.

347d729

Revert "Issue #103, change initial value detection for reduce()"

19beb29

This reverts commit 174c897.

reverting a non-important change that broke reduceRight in IE.

7eb7b08

Edited underscore.js via GitHub

47fb3d7

merging in faster indexOf patch ... arguably faster than the native i…

f099038

…mplementation, but oh well. #245

boucher reviewed Jul 13, 2011
View reviewed changes

jashkenas added 2 commits July 13, 2011 15:57

Revert "merging in faster indexOf patch ... arguably faster than the …

a6cba68

…native implementation, but oh well. #245" This reverts commit f099038.

Revert "Edited underscore.js via GitHub"

c1a5562

This reverts commit 47fb3d7.

michaelficarra reviewed Jul 13, 2011
View reviewed changes

jashkenas added 2 commits July 13, 2011 17:02

Underscore.js 1.1.7

39b07d7

fixing silly typo.

9e8fc83

padolsey reviewed Jul 13, 2011
View reviewed changes

jashkenas and others added 5 commits August 3, 2011 11:16

Issue #252 #154 #148 documenting numeric length key caveat

75b2195

Merging in #194, adding an iterator to _.uniq

03b341d

Added link to Underscore.php in the docs

6cf6475

Merge pull request #267 from brianhaveri/master

f43a790

Link to Underscore.php in docs

Adding Underscore-perl

610b347

bman654 reviewed Aug 12, 2011
View reviewed changes

Merge pull request #1 from michaelficarra/isequal

d447165

Merging @michaelficarra's additions...

ghost closed this Sep 5, 2011

Kit Cambridge added 2 commits September 5, 2011 12:25

Add support for comparing string, number, and boolean object wrappers…

6f62f25

…. Ignore inherited properties when deep comparing objects. Use a more efficient `while` loop for comparing arrays and array-like objects.

Add a comprehensive test suite for isEqual.

e9faa40

ghost reopened this Sep 5, 2011

_.isEqual: Add an early comparison for NaN values.

54245bc

ghost reviewed Sep 5, 2011
View reviewed changes

_.isBoolean should return true for boolean object wrappers.

4fa97eb

ghost reviewed Sep 5, 2011
View reviewed changes

Merge git://github.com/documentcloud/underscore into isequal

7760e6a

jashkenas merged commit 7760e6a into jashkenas:master Oct 4, 2011

Add support for comparing cyclic structures #244

Add support for comparing cyclic structures #244

Conversation

ghost commented Jul 13, 2011

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelficarra commented Jul 13, 2011

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jashkenas commented Jul 15, 2011

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelficarra commented Sep 4, 2011

ghost commented Sep 5, 2011

michaelficarra commented Sep 5, 2011

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Sep 5, 2011

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jashkenas commented Oct 4, 2011

jashkenas commented Oct 4, 2011

ghost commented Oct 4, 2011

jashkenas commented Oct 4, 2011

jashkenas commented Oct 4, 2011

ghost commented Oct 4, 2011

michaelficarra commented Oct 4, 2011