Skip to content

Fix object spread runtime semantics #32514

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

Merged
merged 1 commit into from
Jul 22, 2019
Merged

Fix object spread runtime semantics #32514

merged 1 commit into from
Jul 22, 2019

Conversation

rbuckton
Copy link
Contributor

This fixes the runtime semantics for object spread for ES2017 and earlier.

We cannot call __assign with more than two elements, since any element could cause side effects. For
example:

var k = { a: 1, b: 2 };
var o = { a: 3, ...k, b: k.a++ };
// expected: { a: 1, b: 1 }

If we translate the above to __assign({ a: 3 }, k, { b: k.a++ }), the k.a++ will evaluate before
k is spread and we end up with { a: 2, b: 1 }.

This also occurs for spread elements, not just property assignments:

var k = { a: 1, get b() { l = { z: 9 }; return 2; } };
var l = { c: 3 };
var o = { ...k, ...l };
// expected: { a: 1, b: 2, z: 9 }

If we translate the above to __assign({}, k, l), the l will evaluate before k is spread and we
end up with { a: 1, b: 2, c: 3 }

Fixes #31469

@rbuckton
Copy link
Contributor Author

CC: @weswigham, @RyanCavanaugh
It looks like the GitHub suggested-reviewers API is down again so I can't add reviewers.

Copy link
Member

@weswigham weswigham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we simplify the __assign helper, since it no longer needs to be variadic?

@rbuckton
Copy link
Contributor Author

I wouldn't recommend it since you could, in theory, load two different TypeScript built outputs as scripts into the global scope and one would conflict with the other. If we changed the semantics to accept only a fixed set of arguments we'd probably need to rename the function and ship both in tslib.

@rbuckton rbuckton merged commit 47e3fed into master Jul 22, 2019
@rbuckton rbuckton deleted the fix31469 branch July 22, 2019 23:46
@weswigham
Copy link
Member

Yich, the compatibility/versioning story for tslib (and global helpers emitted by tsc) is pretty ugly then, isn't it. We could minimally use a simplified helper in module output, but....

@weswigham
Copy link
Member

@rbuckton can we merge an RWC baseline update that's just the changes caused by this? I think I see some other unexpected baseline changes outstanding for RWC, but they're hard to pick out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Destructuring object polyfill isn't equivalent to new JS
2 participants