Copy prototype bug with enumerable properties

[myitcv]

Per this thread

Creating a placeholder for a possible fix which includes associated tests.

[mary-poppins]

Thanks for the PR! Please check the items below to help us merge this faster. See the contributing docs for more information.

• Uses the issue template (#8032)

If you need to make changes to your pull request, you can update the commit with git commit --amend.
Then, update the pull request with git push -f.

Thanks again for your help!

[petebacondarwin]

@myitcv - Your solution now makes copies of all enumerable properties right up the inheritance chain, which means that copies get instance methods/variables whereas originals were getting these from their prototypes. This is not right. What happens if the prototype changes? Your copies would not receive the changes as they have overridden them.
See Yet Another CoffeeScript Example

[gentooboontoo]

You should not remove hasOwnProperty otherwise it will copy properties from prototype chain. I have replaced for...in and source.hasOwnProperty with Object.getOwnPropertyNames(source) to include non-enumerable properties in PR #8034 but I am not sure if this is what you want.

[myitcv]

@gentooboontoo - no, we only want to consider enumerable properties. I think we're close to a solution with this

[myitcv]

@petebacondarwin - understood, but I think we're also agreed that only copying direct properties is also not right.

Just as an aside, we are being bitten here by an 'issue' with CoffeeScript constructors on subclasses (code link):

class A
  constructor: ->

class B extends A
  constructor: ->

a = new A
console.log '(k for k of a):', (k for k, v of a)
b = new B
console.log '(k for k of b):', (k for k, v of b)

i.e. constructors are by default non-enumerable, except on sub classes. This issue is relevant here.

Back to angular.copy, the following solution might give us the middle ground we are after (code link):

class A
  constructor: ->

  Object.defineProperty @prototype, 'test', 
    get: -> @["_test"]
    set: (v) -> 
      Object.defineProperty @, '_test',
        writable: true
        enumerable: false
      @["_test"] = v
      console.log 'We just set test to', v
    enumerable: true

  a_meth: ->
    console.log 'Orig a_meth'

class B extends A
  constructor: ->

  b_meth: ->

class C extends B
  constructor: ->

  c_meth: ->

# ------------------

c = new C
c.test = 1
c.blah = 5

workingMakeCopy = (source) ->
  destination = Object.create(Object.getPrototypeOf(source))
  for key, value of source
    if source.hasOwnProperty(key) || !Object.getPrototypeOf(source)[key]?
      console.log 'copying', key
      destination[key] = value
  destination

d = workingMakeCopy c

c.a_meth()
d.a_meth()

A.prototype.a_meth = -> console.log 'New a_meth'

c.a_meth()
d.a_meth()

This solution takes advantage of an important feature of properties. From the Object.defineProperty() docs, para 2 under 'Description':

Property descriptors present in objects come in two main flavors: data descriptors and accessor descriptors. A data descriptor is a property that has a value, which may or may not be writable. An accessor descriptor is a property described by a getter-setter pair of functions. A descriptor must be one of these two flavors; it cannot be both.

A corollary: when an accessor is defined on a prototype, the value of the property when examined on the prototype will be undefined

Looking at the for key, value of source loop

• By the ECMAScript5.1 spec def the property identified by key is enumerable
• If source.hasOwnProperty(key) == false then the property is by definition defined on the prototype
• By the definition in the quoted paragraph above and the corollary, !Object.getPrototypeOf(source)[key]? will identify accessor properties

Therefore I think this gives us what we want, namely:

• copying of enumerable properties that are directly defined on the object
• copying of enumerable accessor property values that are defined on the prototype

[petebacondarwin]

Let's be clear what the problem is here: The current copy functionality applies the correct prototype to the copied object, so the copied object does have access to the test dynamic class property. And if you write to this property it will correctly update the _test instance property on the copied object.

What we are not getting is the _test property being copied, since it is not enumerable.

@myitcv - Your solution is trying to get around this by applying the dynamic class property test directly to the copied object, which then references the original object's _test so appears to work but it breaks the copying semantic because all inherited properties are applied to the copied object and the original prototype properties and methods are lost.

I think what we need to do is look at how your code is assigning the _test property. It seems like a bit of a strange design pattern anyway but I have not spent much time playing with defineProperty so I am not an authority here.

[IgorMinar]

I was going to close this one just as I did with #8034 but then I realized that b59b04f likely introduced a regression for deep dirty-checking.

previously we would flatten the property hierarchy of deeply watched objects when creating a copy, but that's not the case any more. so if anyone relied on being able to observe prototype properties then that doesn't work any more.

@myitcv did your usecase work as expected before b59b04f ?

[petebacondarwin]

@IgorMinar why would that be the case?

[petebacondarwin]

Actually I quite like #8034 - if means that a copy really is a copy with nothing left behind.

[petebacondarwin]

Do you mean like this:

At master : http://plnkr.co/edit/NUDQR9ZqYLi8kreoLcrk?p=preview
At 1.3.0-beta-13 : http://plnkr.co/edit/3cappcs8WVjBM6X1vsRn?p=preview

[petebacondarwin]

Yes, @IgorMinar - I agree this is a regression but I don't think it is really related to @myitcv's problem here. The problem with deep watches is that both the oldVal and newVal have the same prototype and so if a value on the prototype changes both oldVal and newVal change so it is not possible to trigger a watch handler.

[petebacondarwin]

Created a new issue for it #8040

[myitcv]

@petebacondarwin - I agree there are a few issues we are trying to tackle around this change. I also note #8040.

As a side note, I'm very happy to defer to both you and @IgorMinar on whether further changes are made to angular.copy, but share the same concern raised by @gentooboontoo. I do call angular.copy from my code but that's largely to prevent me reinventing the wheel. That said I can, and will, try to see whether I can remove my dependency on angular.copy and then see where things stand

To my mind angular.copy should be trying to do the following (again, happy to defer if the internal requirements for Angular core are different):

• create a new object with the same prototype
• deep copy all enumerable properties (recursive by definition) that are either directly defined on the object or that have accessor descriptors on the prototype

The caveat for accessor descriptors (incorrectly defined in my previous message as being identified by !Object.getPrototypeOf(source)[key]?) exists because setting values against an accessor does not mask the prototype, unlike for data descriptor properties:

class A
  constructor: ->
  a_meth: -> 42
  a_val: 10
  Object.defineProperty @prototype, 'test', 
    get: -> 999
    set: (v) -> 

b = new A
console.log b.test # 999
b.test = 4
console.log b.test # 999
delete b.test
console.log b.test # 999

console.log b.a_val # 10
b.a_val = 33
console.log b.a_val # 33
delete b.a_val
console.log b.a_val # 10

console.log b.a_meth() # 42
b.a_meth = -> 4
console.log b.a_meth() # 4
b.a_meth = 3
console.log b.a_meth # 3
delete b.a_meth
console.log b.a_meth() # 42

Referring back to your comment, I intentionally made _test non-enumerable, because I want any setting of test to always go via the getter/setter (the setter has other side effects, like triggering that a property has changed). So I don't want my copy function (again, acknowledging this may by a different requirement to core angular.copy) to copy _test; I want _test to be set as a side effect of it copying test (given my implementation of test). Notice that because test is an accessor descriptor property (as just demonstrated) the prototype definition is not masked when set on the copy - hence why this works.

So my final attempt at making this work is as follows. The change is in the use of getPropertyDescriptor(o,p) within the copy. Here's the code (code link):

class A
  constructor: ->

  Object.defineProperty @prototype, 'test', 
    get: -> @["_test"]
    set: (v) -> 
      Object.defineProperty @, '_test',
        writable: true
        enumerable: false
      @["_test"] = v
      console.log 'We just set test to', v
    enumerable: true

  a_meth: ->
    console.log 'Orig a_meth'

class B extends A
  constructor: ->

  b_meth: ->

class C extends B
  constructor: ->

  c_meth: ->

# ------------------

c = new C
c.test = 1 # output: We just set test to 1
c.blah = 5

getPropertyDescriptor = (o, p) ->
  # walks from the object up the prototype chain until it
  # finds the definition of p
  res = null
  loop
    break if !o? || res?
    res = Object.getOwnPropertyDescriptor(o,p)
    o = Object.getPrototypeOf(o)
  res

isAccessorProperty = (o, p) ->
  pd = getPropertyDescriptor o, p
  pd? && (pd.get? || pd.set?)

workingMakeCopy = (source) ->
  destination = Object.create(Object.getPrototypeOf(source))

  for key, value of source
    # notice the order is significant here; if we have the property
    # identified by key defined as a direct property we definitely 
    # want to copy it
    if source.hasOwnProperty(key) || isAccessorProperty(source, key)
      console.log 'copying', key
      destination[key] = value

  destination

d = workingMakeCopy c # output: We just set test to 1

c.a_meth()
d.a_meth()

A.prototype.a_meth = -> console.log 'New a_meth'

c.a_meth()
d.a_meth()

Again I totally respect that if Angular core needs a different version this may not be suitable.

[petebacondarwin]

@myitcv - thanks for the thought and effort you are putting into this subject. I will take a further look at your approach tonight.

[petebacondarwin]

@myitcv - I believe that we can get the same final result more simply by copying all the own properties (enumerable or not) from the source object and leaving the stuff defined on the prototype alone. See this coffeescript

This is basically what @gentooboontoo was suggesting in #8034.

copy2 = (source) ->
  destination = Object.create(Object.getPrototypeOf(source))
  Object.getOwnPropertyNames(source).forEach (key)->
    pd = Object.getOwnPropertyDescriptor(source, key)
    if pd.hasOwnProperty('value')
      console.log 'copying value', key, pd.value
      destination[key] = pd.value
    else
      console.log 'copying accessor', key
      Object.defineProperty(destination, key, pd);
  destination

[myitcv]

In this case (using the definition of test and _test) it will I agree. But I'm not sure we can generalise to all situations - I'll try and come up with a counter example.

As one example of something that could go wrong, here is part of the output from your last example:

...
copy1
copying blah
copying test
We just set test to 1
copy2
copying value _test 1
copying value blah 5 
...

This confirms a somewhat obvious point (given the implementation) that copy2 does not call the test setter.

If, as I do, you rely on side effects of the setter (which I have not included in the test code we are talking about) then this approach does not work.

But I think at this point it really does depend what you want your definition of copy to be (and as I've said I'm happy for mine to be different). My personal feeling is that you should not copy non-enumerable properties: after all, equals only looks to compare enumerable properties.

[myitcv]

Indeed perhaps the 'best' approach would be to allow for the definition of $clone as I proposed back in #7592. That way, you can work with a stock definition of copy but allow for variation from that on an object-by-object basis.

[petebacondarwin]

@myitcv - I'm sorry to say that we have decided that our version of copy should be as lightweight as possible while supporting our own needs. We did take another look at the idea of creating a $clone specification but since there is no industry standard specification for such a thing and it would add additional overhead to the copy function we have chosen not to implement that.

It looks like you will need to create your own version for this situation.

[myitcv]

@petebacondarwin - no problem at all. As I said a few messages ago, quite happy to defer to you, @IgorMinar and others on this one; even if it does mean a bit of duplication here and there.