New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Object spread does not handle property execution order properly. #6735

Open
loganfsmyth opened this Issue Nov 3, 2017 · 4 comments

Comments

Projects
None yet
4 participants
@loganfsmyth
Member

loganfsmyth commented Nov 3, 2017

Pass-through issue for tc39/proposal-object-rest-spread#54

Assuming the result of that issue ends up being that no spec changes are made, then we have a bug to fix, somehow. Not taking the time to consider options right now, but figured I should file it.

@Andarist

This comment has been minimized.

Show comment
Hide comment
@Andarist

Andarist Nov 3, 2017

Member

While some may disagree I am really thankful for the loose mode 😄

Member

Andarist commented Nov 3, 2017

While some may disagree I am really thankful for the loose mode 😄

@zoecarver

This comment has been minimized.

Show comment
Hide comment
@zoecarver

zoecarver Nov 27, 2017

Contributor

@loganfsmyth I am trying to find where this is occurring. It seems like it should be here, but I can't find where the t.identifier is. (eg. in the const b = _extends({}, a, { c: getC(a) }); example where a is located). Am I looking in the wrong place? Thanks!

Contributor

zoecarver commented Nov 27, 2017

@loganfsmyth I am trying to find where this is occurring. It seems like it should be here, but I can't find where the t.identifier is. (eg. in the const b = _extends({}, a, { c: getC(a) }); example where a is located). Am I looking in the wrong place? Thanks!

@Kovensky

This comment has been minimized.

Show comment
Hide comment
@Kovensky

Kovensky Jan 5, 2018

Member

Oneliner that demonstrates the issue:

(a => ({ ...a, ...{ get c () { return ++a.b }, d: ++a.b }, e: ++a.b }))({ b: 1 })
  • Real engine (SM, v8, JSC): { b: 1, c: 3, d: 2, e: 4 }
  • Babel: { b: 3, c: 4, d: 2, e: 3 }

I'm not sure there's anything we can do about it other than adding a nested call to _extend for each spread object; and even then handling .e correctly will be a challenge... we can't just build it as another object because setters/getters need to be preserved.

Member

Kovensky commented Jan 5, 2018

Oneliner that demonstrates the issue:

(a => ({ ...a, ...{ get c () { return ++a.b }, d: ++a.b }, e: ++a.b }))({ b: 1 })
  • Real engine (SM, v8, JSC): { b: 1, c: 3, d: 2, e: 4 }
  • Babel: { b: 3, c: 4, d: 2, e: 3 }

I'm not sure there's anything we can do about it other than adding a nested call to _extend for each spread object; and even then handling .e correctly will be a challenge... we can't just build it as another object because setters/getters need to be preserved.

@Kovensky

This comment has been minimized.

Show comment
Hide comment
@Kovensky

Kovensky Jan 5, 2018

Member

(RFC) Possible way to deal with it:

_mergeDescriptors(
  {},
  _getProperties(a),
  _getProperties({ get c () { return ++a.b }, d: ++a.b }),
  { e: ++a.b }
)

mergeDescriptors would return the first argument with all descriptors merged. This is so we can preserve getters/setters declared on the "top level" object, which would be clobbered by a more honest CopyDataProperties implementation. I considered making it just always return a new object, but we can't do that if the object being spread into has a __proto__ key.

getProperties is the helper that would perform the CopyDataProperties algorithm, but into a new object. This would allow mergeDescriptors to safely just copy the descriptors without accidentally preserving a getter/setter from the spread objects.

Member

Kovensky commented Jan 5, 2018

(RFC) Possible way to deal with it:

_mergeDescriptors(
  {},
  _getProperties(a),
  _getProperties({ get c () { return ++a.b }, d: ++a.b }),
  { e: ++a.b }
)

mergeDescriptors would return the first argument with all descriptors merged. This is so we can preserve getters/setters declared on the "top level" object, which would be clobbered by a more honest CopyDataProperties implementation. I considered making it just always return a new object, but we can't do that if the object being spread into has a __proto__ key.

getProperties is the helper that would perform the CopyDataProperties algorithm, but into a new object. This would allow mergeDescriptors to safely just copy the descriptors without accidentally preserving a getter/setter from the spread objects.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment