Bug in angular.equals following #7618

[myitcv]

Following #7618 it is now possible to use objects which contains circular references (thanks for adding this). This test uses the Jasmine object equality check to verify that the copy of an object that contains circular references has indeed worked.

However, two such objects should also be angular.equals. Currently, this is not possible as the stack explodes:

it('should handle circular references when circularRefs is turned on', function () {
  var a = {b: {a: null}, self: null, selfs: [null, null, [null]]};
  a.b.a = a;
  a.self = a;
  a.selfs = [a, a.b, [a]];

  var aCopy = copy(a, null);
  expect(aCopy).toEqual(a);
  expect(equals(a, aCopy)).toBeTruthy(); // ** EXPLODES **
});

A fix similar to this does the job. Let me know if you want me to formulate a PR for this.

Thanks

[myitcv]

Should have included a practical example of the impact of this bug in my first post.

With angular.equals broken in this way, it isn't possible to $watch objects that have circular references. A minimal example in test format would be:

it("should watch circular reference objects", inject(function($rootScope) {
  var a, spy;
  spy = jasmine.createSpy();
  $rootScope.$watch("name", spy, true);
  $rootScope.$digest();
  spy.reset();
  expect(spy).not.wasCalled();
  $rootScope.$digest();
  expect(spy).not.wasCalled();
  a = {};
  a.x = 5;
  a.a = a;
  $rootScope.name = a;
  $rootScope.$digest();
  return expect(spy).wasCalledWith(a, undefined, $rootScope);
}));

This fails with a Maximum call stack size exceeded error

[rodyhaddad]

Yes, do send a PR.

Here's my take on what you've already done:

• You can follow a similar approach to angular.copy. No need for a equals_impl to be created. Simply make equals take 4 arguments and leave the last 2 parameters undocumented, just to stay consistent in the codebase.
• Add specific tests to the equal section
• I'm not sure why your comparisons array is two dimensional. Can't it just be two flat arrays named stackA and stackB, similar to .copy? I feel this is not needed and the implementation can be simpler.
  Maybe I'm missing something. Some tests can prove me wrong :-)

Go ahead and send a PR. If you have any questions or can't make it, let me know.
It would be nice to have this fix before 1.3.0 gets released.

[myitcv]

Great. Will pull together a PR tomorrow (taking into account your points) unless you need it before? Indeed what's the timeline for 1.3.0?

[myitcv]

Created as #7839

In response to your points:

• Thanks for the pointer confirming you're happy with that approach
• Done
• The reason I did this as a 2D array is largely for reasons of 'efficiency' (quotes intentional). Regardless of the size of object we are checking, we need to keep track of the pairs that we have encountered already. Given the symmetry of the equals relation, I keep track in both directions. So seen_list is a list of all the objects o at index i we have seen, comparisons at index i is the list of objects we have compared against o. Hence given two objects a and b, it is enough to check whether we have seen a and then if we have previously compared against b without then needing to check b == a. Difficult to quantify what 'efficiency' means here however, because there isn't per se a standard complex object against which we can benchmark.

Very happy to defer to a superior implementation however!

[IgorMinar]

I responded here: #7839 (comment)