From c30bcdd515da06d475bac9f3be933d2ed1aa3ffe Mon Sep 17 00:00:00 2001 From: John-David Dalton Date: Sun, 9 Sep 2012 11:54:32 -0700 Subject: [PATCH] Ensure `_.isEqual` matches values with circular references correctly. Former-commit-id: 07968aeb430f56c32aab22dfda919706da840680 --- build/pre-compile.js | 6 ++-- lodash.js | 78 ++++++++++++++++++++++++-------------------- test/test.js | 26 ++++++++++----- 3 files changed, 63 insertions(+), 47 deletions(-) diff --git a/build/pre-compile.js b/build/pre-compile.js index 460bc209e..916ae92fb 100644 --- a/build/pre-compile.js +++ b/build/pre-compile.js @@ -300,10 +300,10 @@ // remove debug sourceURL use in `_.template` source = source.replace(/(?:\s*\/\/.*\n)* *if *\(useSourceURL[^}]+}/, ''); - // minify internal properties used by 'compareAscending', `_.clone`, `_.merge`, and `_.sortBy` + // minify internal properties used by 'compareAscending', `_.clone`, `_.isEqual`, `_.merge`, and `_.sortBy` (function() { - var properties = ['criteria', 'index', 'source', 'value'], - snippets = source.match(/( +)(?:function clone|function compareAscending|var merge|var sortBy)\b[\s\S]+?\n\1}/g); + var properties = ['criteria', 'index', 'isCircular', 'source', 'thorough', 'value'], + snippets = source.match(/( +)(?:function (?:clone|compareAscending|isEqual)|var merge|var sortBy)\b[\s\S]+?\n\1}/g); if (!snippets) { return; diff --git a/lodash.js b/lodash.js index c3eb71e7e..56c6c7a19 100644 --- a/lodash.js +++ b/lodash.js @@ -1063,10 +1063,9 @@ * @param {Boolean} deep A flag to indicate a deep clone. * @param {Object} [guard] Internally used to allow this method to work with * others like `_.map` without using their callback `index` argument for `deep`. - * @param {Array} [stack=[]] Internally used to keep track of traversed objects - * to avoid circular references. - * @param {Object} thorough Internally used to indicate whether or not to perform - * a more thorough clone of non-object values. + * @param {Object} [data={}] Internally used to track traversed objects to avoid + * circular references and indicate whether to perform a more thorough clone + * of non-object values. * @returns {Mixed} Returns the cloned `value`. * @example * @@ -1087,23 +1086,25 @@ * shallow[0] === stooges[0]; * // => false */ - function clone(value, deep, guard, stack, thorough) { + function clone(value, deep, guard, data) { if (value == null) { return value; } if (guard) { deep = false; } + // init internal data + data || (data = { 'thorough': null }); + // avoid slower checks on primitives - thorough || (thorough = { 'value': null }); - if (thorough.value == null) { + if (data.thorough == null) { // primitives passed from iframes use the primary document's native prototypes - thorough.value = !!(BoolProto.clone || NumberProto.clone || StringProto.clone); + data.thorough = !!(BoolProto.clone || NumberProto.clone || StringProto.clone); } // use custom `clone` method if available var isObj = objectTypes[typeof value]; - if ((isObj || thorough.value) && value.clone && isFunction(value.clone)) { - thorough.value = null; + if ((isObj || data.thorough) && value.clone && isFunction(value.clone)) { + data.thorough = null; return value.clone(deep); } // inspect [[Class]] @@ -1140,9 +1141,10 @@ return ctor(value.source, reFlags.exec(value)); } + var stack = data.stack || (data.stack = []), + length = stack.length; + // check for circular references and return corresponding clone - stack || (stack = []); - var length = stack.length; while (length--) { if (stack[length].source == value) { return stack[length].value; @@ -1150,8 +1152,7 @@ } // init cloned object - length = value.length; - var result = isArr ? ctor(length) : {}; + var result = isArr ? ctor(length = value.length) : {}; // add current clone and original source value to the stack of traversed objects stack.push({ 'value': result, 'source': value }); @@ -1160,11 +1161,11 @@ if (isArr) { var index = -1; while (++index < length) { - result[index] = clone(value[index], deep, null, stack, thorough); + result[index] = clone(value[index], deep, null, data); } } else { forOwn(value, function(objValue, key) { - result[key] = clone(objValue, deep, null, stack, thorough); + result[key] = clone(objValue, deep, null, data); }); } return result; @@ -1423,10 +1424,9 @@ * @category Objects * @param {Mixed} a The value to compare. * @param {Mixed} b The other value to compare. - * @param {Array} [stack=[]] Internally used to keep track of traversed objects - * to avoid circular references. - * @param {Object} thorough Internally used to indicate whether or not to perform - * a more thorough comparison of non-object values. + * @param {Object} [data={}] Internally used track traversed objects to avoid + * circular references and indicate whether to perform a more thorough comparison + * of non-object values. * @returns {Boolean} Returns `true` if the values are equvalent, else `false`. * @example * @@ -1439,29 +1439,31 @@ * _.isEqual(moe, clone); * // => true */ - function isEqual(a, b, stack, thorough) { + function isEqual(a, b, data) { // a strict comparison is necessary because `null == undefined` if (a == null || b == null) { return a === b; } + // init internal data + data || (data = { 'isCircular': false, 'thorough': null }); + // avoid slower checks on non-objects - thorough || (thorough = { 'value': null }); - if (thorough.value == null) { + if (data.thorough == null) { // primitives passed from iframes use the primary document's native prototypes - thorough.value = !!(BoolProto.isEqual || NumberProto.isEqual || StringProto.isEqual); + data.thorough = !!(BoolProto.isEqual || NumberProto.isEqual || StringProto.isEqual); } - if (objectTypes[typeof a] || objectTypes[typeof b] || thorough.value) { + if (objectTypes[typeof a] || objectTypes[typeof b] || data.thorough) { // unwrap any LoDash wrapped values a = a.__wrapped__ || a; b = b.__wrapped__ || b; // use custom `isEqual` method if available if (a.isEqual && isFunction(a.isEqual)) { - thorough.value = null; + data.thorough = null; return a.isEqual(b); } if (b.isEqual && isFunction(b.isEqual)) { - thorough.value = null; + data.thorough = null; return b.isEqual(a); } } @@ -1507,14 +1509,20 @@ return false; } + // exit if it's the second pass of a circular reference + if (data.isCircular) { + return true; + } // assume cyclic structures are equal // the algorithm for detecting cyclic structures is adapted from ES 5.1 // section 15.12.3, abstract operation `JO` (http://es5.github.com/#x15.12.3) - stack || (stack = []); - var length = stack.length; + var stack = data.stack || (data.stack = []), + length = stack.length; + while (length--) { if (stack[length] == a) { - return true; + data.isCircular = true; + break; } } @@ -1534,7 +1542,7 @@ if (result) { // deep compare the contents, ignoring non-numeric properties while (size--) { - if (!(result = isEqual(a[size], b[size], stack, thorough))) { + if (!(result = isEqual(a[size], b[size], data))) { break; } } @@ -1558,7 +1566,7 @@ // count the number of properties. size++; // deep compare each property value. - if (!(hasOwnProperty.call(b, prop) && isEqual(a[prop], b[prop], stack, thorough))) { + if (!(hasOwnProperty.call(b, prop) && isEqual(a[prop], b[prop], data))) { return false; } } @@ -1578,7 +1586,7 @@ while (++index < 7) { prop = shadowed[index]; if (hasOwnProperty.call(a, prop) && - !(hasOwnProperty.call(b, prop) && isEqual(a[prop], b[prop], stack, thorough))) { + !(hasOwnProperty.call(b, prop) && isEqual(a[prop], b[prop], data))) { return false; } } @@ -1797,8 +1805,8 @@ * @param {Object} [source1, source2, ...] The source objects. * @param {Object} [indicator] Internally used to indicate that the `stack` * argument is an array of traversed objects instead of another source object. - * @param {Array} [stack=[]] Internally used to keep track of traversed objects - * to avoid circular references. + * @param {Array} [stack=[]] Internally used to track traversed objects to avoid + * circular references. * @returns {Object} Returns the destination object. * @example * diff --git a/test/test.js b/test/test.js index 61b7e8475..5e0e4c923 100644 --- a/test/test.js +++ b/test/test.js @@ -770,15 +770,23 @@ equal(_.isEqual(shadowed, {}), false); }); - test('should return `true` for like-objects from different documents', function() { - // ensure `_._object` is assigned (unassigned in Opera 10.00) - if (_._object) { - var object = { 'a': 1, 'b': 2, 'c': 3 }; - equal(_.isEqual(object, _._object), true); - } - else { - skipTest(); - } + test('should use custom `isEqual` methods on primitives', function() { + Boolean.prototype.isEqual = function() { return true; }; + equal(_.isEqual(true, false), true); + delete Boolean.prototype.isEqual; + }); + + test('should return `false` when comparing values with circular references to unlike values', function() { + var array1 = ['a', null, 'c'], + array2 = ['a', [], 'c'], + object1 = { 'a': 1, 'b': null, 'c': 3 }, + object2 = { 'a': 1, 'b': {}, 'c': 3 }; + + array1[1] = array1; + equal(_.isEqual(array1, array2), false); + + object1.b = object1; + equal(_.isEqual(object1, object2), false); }); }());