-
-
Notifications
You must be signed in to change notification settings - Fork 5.7k
Fix evaluation order with object spread #11412
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
Conversation
Can we limit this deoptimization to when |
z = { | ||
x, | ||
w: _objectSpread({}, y) | ||
w: _objectSpread(_objectSpread({}), y) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change seems unnecessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, scope.isPure
is not working as intended. What's the difference between scope.isPure
and path.isPure
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😵 My bad , scope.isPure
is working.
...es/babel-plugin-proposal-object-rest-spread/test/fixtures/object-spread/expression/output.js
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
I have left a comment for another bug, that we can fix in a separate PR.
|
||
}, pureB, {}, pureC, {}), impureFunc(), {}, pureD, { | ||
pureD | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would expect this output to be
var output = { ...pureA, get foo() {}, get bar() {}, ...pureB, ...pureC, ...impureFunc(), ...pureD, pureD }
var output = _objectSpread(
_objectSpread(
{},
pureA,
{ get foo() {}, get bar() {} },
pureB,
{},
pureC,
{}
),
impureFunc(),
{},
pureD,
{ pureD }
);
I think that it's a bug in the isPure
implementation, that only handles isClassMethod
instead of isMethod
(that also catches object methods/accessors).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🙆♂️ I'm gonna give it a try.
Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/21217/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks.
When reviewing babel#11412, I noticed there are more cases where the intermediate values can mutate the spread reference. It's best demonstrated in the [TypeScript](https://github.com/microsoft/TypeScript/blob/eb105efdcd6db8a73f5b983bf329cb7a5eee55e1/src/compiler/transformers/es2018.ts#L272) examples.
TLDR: currently, the code below is not transpiled correctly. REPL
expected
o.a
to be 1.Long story: I found weird
__assign
js code generated bytsc
. Out of curiosity, I checked the relevant transformer code and feed the test to babel 💥. This fix is migrated from TypeScript. Original Issue