fix(copy): do not copy the same object twice

Author: jbedard

Diff for: src/Angular.js

@@ -743,19 +743,33 @@ function copy(source, destination, stackSource, stackDest) {
 
   if (!destination) {
     destination = source;
-    if (source) {
+    if (isObject(source)) {
+      var index;
+      if (stackSource && (index = stackSource.indexOf(source)) !== -1) {
+        return stackDest[index];
+      }
+
+      // TypedArray, Date and RegExp have specific copy functionality and must be
+      // pushed onto the stack before returning.
+      // Array and other objects create the base object and recurse to copy child
+      // objects. The array/object will be pushed onto the stack when recursed.
       if (isArray(source)) {
-        destination = copy(source, [], stackSource, stackDest);
+        return copy(source, [], stackSource, stackDest);
       } else if (isTypedArray(source)) {
         destination = new source.constructor(source);
       } else if (isDate(source)) {
         destination = new Date(source.getTime());
       } else if (isRegExp(source)) {
         destination = new RegExp(source.source, source.toString().match(/[^\/]*$/)[0]);
         destination.lastIndex = source.lastIndex;
-      } else if (isObject(source)) {
+      } else {
         var emptyObject = Object.create(getPrototypeOf(source));
-        destination = copy(source, emptyObject, stackSource, stackDest);
+        return copy(source, emptyObject, stackSource, stackDest);
+      }
+
+      if (stackDest) {
+        stackSource.push(source);
+        stackDest.push(destination);
       }
     }
   } else {
@@ -766,9 +780,6 @@ function copy(source, destination, stackSource, stackDest) {
     stackDest = stackDest || [];
 
     if (isObject(source)) {
-      var index = stackSource.indexOf(source);
-      if (index !== -1) return stackDest[index];
-
       stackSource.push(source);
       stackDest.push(destination);
     }
@@ -777,12 +788,7 @@ function copy(source, destination, stackSource, stackDest) {
     if (isArray(source)) {
       destination.length = 0;
       for (var i = 0; i < source.length; i++) {
-        result = copy(source[i], null, stackSource, stackDest);
-        if (isObject(source[i])) {
-          stackSource.push(source[i]);
-          stackDest.push(result);
-        }
-        destination.push(result);
+        destination.push(copy(source[i], null, stackSource, stackDest));
       }
     } else {
       var h = destination.$$hashKey;
@@ -796,37 +802,27 @@ function copy(source, destination, stackSource, stackDest) {
       if (isBlankObject(source)) {
         // createMap() fast path --- Safe to avoid hasOwnProperty check because prototype chain is empty
         for (key in source) {
-          putValue(key, source[key], destination, stackSource, stackDest);
+          destination[key] = copy(source[key], null, stackSource, stackDest);
         }
       } else if (source && typeof source.hasOwnProperty === 'function') {
         // Slow path, which must rely on hasOwnProperty
         for (key in source) {
           if (source.hasOwnProperty(key)) {
-            putValue(key, source[key], destination, stackSource, stackDest);
+            destination[key] = copy(source[key], null, stackSource, stackDest);
           }
         }
       } else {
         // Slowest path --- hasOwnProperty can't be called as a method
         for (key in source) {
           if (hasOwnProperty.call(source, key)) {
-            putValue(key, source[key], destination, stackSource, stackDest);
+            destination[key] = copy(source[key], null, stackSource, stackDest);
           }
         }
       }
       setHashKey(destination,h);
     }
   }
   return destination;
-
-  function putValue(key, val, destination, stackSource, stackDest) {
-    // No context allocation, trivial outer scope, easily inlined
-    var result = copy(val, null, stackSource, stackDest);
-    if (isObject(val)) {
-      stackSource.push(val);
-      stackDest.push(result);
-    }
-    destination[key] = result;
-  }
 }
 
 /**

Diff for: test/AngularSpec.js

@@ -336,7 +336,7 @@ describe('angular', function() {
       expect(hashKey(dst)).not.toEqual(hashKey(src));
     });
 
-    it('should retain the previous $$hashKey', function() {
+    it('should retain the previous $$hashKey when copying object with hashKey', function() {
       var src,dst,h;
       src = {};
       dst = {};
@@ -351,7 +351,21 @@ describe('angular', function() {
       expect(hashKey(dst)).toEqual(h);
     });
 
-    it('should handle circular references when circularRefs is turned on', function() {
+    it('should retain the previous $$hashKey when copying non-object', function() {
+      var dst = {};
+      var h = hashKey(dst);
+
+      copy(null, dst);
+      expect(hashKey(dst)).toEqual(h);
+
+      copy(42, dst);
+      expect(hashKey(dst)).toEqual(h);
+
+      copy(new Date(), dst);
+      expect(hashKey(dst)).toEqual(h);
+    });
+
+    it('should handle circular references', function() {
       var a = {b: {a: null}, self: null, selfs: [null, null, [null]]};
       a.b.a = a;
       a.self = a;
@@ -362,13 +376,60 @@ describe('angular', function() {
 
       expect(aCopy).not.toBe(a);
       expect(aCopy).toBe(aCopy.self);
+      expect(aCopy).toBe(aCopy.selfs[2][0]);
       expect(aCopy.selfs[2]).not.toBe(a.selfs[2]);
+
+      var copyTo = [];
+      aCopy = copy(a, copyTo);
+      expect(aCopy).toBe(copyTo);
+      expect(aCopy).not.toBe(a);
+      expect(aCopy).toBe(aCopy.self);
+    });
+
+    it('should handle objects with multiple references', function() {
+      var b = {};
+      var a = [b, -1, b];
+
+      var aCopy = copy(a);
+      expect(aCopy[0]).not.toBe(a[0]);
+      expect(aCopy[0]).toBe(aCopy[2]);
+
+      var copyTo = [];
+      aCopy = copy(a, copyTo);
+      expect(aCopy).toBe(copyTo);
+      expect(aCopy[0]).not.toBe(a[0]);
+      expect(aCopy[0]).toBe(aCopy[2]);
+    });
+
+    it('should handle date/regex objects with multiple references', function() {
+      var re = /foo/;
+      var d = new Date();
+      var o = {re: re, re2: re, d: d, d2: d};
+
+      var oCopy = copy(o);
+      expect(oCopy.re).toBe(oCopy.re2);
+      expect(oCopy.d).toBe(oCopy.d2);
+
+      oCopy = copy(o, {});
+      expect(oCopy.re).toBe(oCopy.re2);
+      expect(oCopy.d).toBe(oCopy.d2);
     });
 
     it('should clear destination arrays correctly when source is non-array', function() {
       expect(copy(null, [1,2,3])).toEqual([]);
       expect(copy(undefined, [1,2,3])).toEqual([]);
       expect(copy({0: 1, 1: 2}, [1,2,3])).toEqual([1,2]);
+      expect(copy(new Date(), [1,2,3])).toEqual([]);
+      expect(copy(/a/, [1,2,3])).toEqual([]);
+      expect(copy(true, [1,2,3])).toEqual([]);
+    });
+
+    it('should clear destination objects correctly when source is non-array', function() {
+      expect(copy(null, {0:1,1:2,2:3})).toEqual({});
+      expect(copy(undefined, {0:1,1:2,2:3})).toEqual({});
+      expect(copy(new Date(), {0:1,1:2,2:3})).toEqual({});
+      expect(copy(/a/, {0:1,1:2,2:3})).toEqual({});
+      expect(copy(true, {0:1,1:2,2:3})).toEqual({});
     });
 
     it('should copy objects with no prototype parent', function() {