From 3b947aa6aa600b8115bfb8b30259803466db4064 Mon Sep 17 00:00:00 2001 From: Justin Ridgewell Date: Fri, 24 Apr 2020 00:00:21 -0400 Subject: [PATCH] Fix evaluation order with object spread, 2 When reviewing https://github.com/babel/babel/pull/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. --- .../src/index.js | 96 +++++++------------ .../object-spread/expression/output.js | 10 +- .../object-spread/side-effect/exec.js | 6 +- .../object-spread/side-effect/output.js | 6 +- 4 files changed, 46 insertions(+), 72 deletions(-) diff --git a/packages/babel-plugin-proposal-object-rest-spread/src/index.js b/packages/babel-plugin-proposal-object-rest-spread/src/index.js index 5899233567c4..1ad0862a55cd 100644 --- a/packages/babel-plugin-proposal-object-rest-spread/src/index.js +++ b/packages/babel-plugin-proposal-object-rest-spread/src/index.js @@ -281,6 +281,7 @@ export default declare((api, opts) => { ); } }, + // adapted from transform-destructuring/src/index.js#pushObjectRest // const { a, ...b } = c; VariableDeclarator(path, file) { @@ -385,6 +386,7 @@ export default declare((api, opts) => { } }); }, + // taken from transform-destructuring/src/index.js#visitor // export var { a, ...b } = c; ExportNamedDeclaration(path) { @@ -410,11 +412,13 @@ export default declare((api, opts) => { path.replaceWith(declaration.node); path.insertAfter(t.exportNamedDeclaration(null, specifiers)); }, + // try {} catch ({a, ...b}) {} CatchClause(path) { const paramPath = path.get("param"); replaceRestElement(paramPath.parentPath, paramPath); }, + // ({a, ...b} = c); AssignmentExpression(path, file) { const leftPath = path.get("left"); @@ -457,6 +461,7 @@ export default declare((api, opts) => { path.replaceWithMultiple(nodes); } }, + // taken from transform-destructuring/src/index.js#visitor ForXStatement(path) { const { node, scope } = path; @@ -506,6 +511,7 @@ export default declare((api, opts) => { ); } }, + // [{a, ...b}] = c; ArrayPattern(path) { const objectPatterns = []; @@ -537,34 +543,11 @@ export default declare((api, opts) => { ); } }, + // var a = { ...b, ...c } ObjectExpression(path, file) { if (!hasSpread(path.node)) return; - const { scope } = path; - - // a non-SpreadElement and SpreadElement striped array - const args = []; - let props = []; - - function push() { - args.push(t.objectExpression(props)); - props = []; - } - - for (const prop of (path.node.properties: Array)) { - if (t.isSpreadElement(prop)) { - push(); - args.push(prop.argument); - } else { - props.push(prop); - } - } - - if (props.length) { - push(); - } - let helper; if (loose) { helper = getExtendsHelper(file); @@ -582,53 +565,40 @@ export default declare((api, opts) => { helper = file.addHelper("objectSpread"); } } - // We cannot call _objectSpread with more than two elements directly, 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 `_objectSpread({ a: 3 }, k, { b: k.a++ })`, the `k.a++` will evaluate before - // `k` is spread and we end up with `{ a: 2, b: 1 }`. - // adapted from https://github.com/microsoft/TypeScript/blob/eb105efdcd6db8a73f5b983bf329cb7a5eee55e1/src/compiler/transformers/es2018.ts#L272 - const chunks = []; - let currentChunk = []; - for (let i = 0; i < args.length; i++) { - currentChunk.push(args[i]); - const isCurrentChunkEmptyObject = - currentChunk.length === 1 && - t.isObjectExpression(args[i]) && - args[i].properties.length === 0; - const isNextArgEffectful = - i < args.length - 1 && !scope.isPure(args[i + 1]); - // prevent current chunk from pollution unless current chunk is an empty object - if (!isCurrentChunkEmptyObject && isNextArgEffectful) { - chunks.push(currentChunk); - currentChunk = []; + + let exp = null; + let props = []; + + function make() { + const hadProps = props.length > 0; + const obj = t.objectExpression(props); + props = []; + + if (!exp) { + exp = t.callExpression(helper, [obj]); + return; } - } - if (currentChunk.length) { - chunks.push(currentChunk); - currentChunk = []; + exp = t.callExpression(t.cloneNode(helper), [ + exp, + // If we have static props, we need to insert an empty object + // becuase the odd arguments are copied with [[Get]], not + // [[GetOwnProperty]] + ...(hadProps ? [t.objectExpression([]), obj] : []), + ]); } - let exp = t.callExpression(helper, chunks[0]); - let nthArg = chunks[0].length; - for (let i = 1; i < chunks.length; i++) { - // reference: packages/babel-helpers/src/helpers.js#objectSpread2 - if (nthArg % 2) { - exp = t.callExpression(helper, [exp, ...chunks[i]]); + for (const prop of (path.node.properties: Array)) { + if (t.isSpreadElement(prop)) { + make(); + exp.arguments.push(prop.argument); } else { - exp = t.callExpression(helper, [ - exp, - t.objectExpression([]), - ...chunks[i], - ]); + props.push(prop); } - - nthArg += chunks[i].length; } + if (props.length) make(); + path.replaceWith(exp); }, }, diff --git a/packages/babel-plugin-proposal-object-rest-spread/test/fixtures/object-spread/expression/output.js b/packages/babel-plugin-proposal-object-rest-spread/test/fixtures/object-spread/expression/output.js index b846f4badffe..da325b840aa2 100644 --- a/packages/babel-plugin-proposal-object-rest-spread/test/fixtures/object-spread/expression/output.js +++ b/packages/babel-plugin-proposal-object-rest-spread/test/fixtures/object-spread/expression/output.js @@ -11,11 +11,11 @@ var d; var x; var y; -_objectSpread({ +_objectSpread(_objectSpread(_objectSpread({ x -}, y, { +}, y), {}, { a -}, b, { +}, b), {}, { c }); @@ -25,9 +25,9 @@ _objectSpread({}, { foo: 'bar' }); -_objectSpread({}, { +_objectSpread(_objectSpread({}, { foo: 'bar' -}, {}, { +}), { bar: 'baz' }); diff --git a/packages/babel-plugin-proposal-object-rest-spread/test/fixtures/object-spread/side-effect/exec.js b/packages/babel-plugin-proposal-object-rest-spread/test/fixtures/object-spread/side-effect/exec.js index efb73e7a98ca..15144e7782ec 100644 --- a/packages/babel-plugin-proposal-object-rest-spread/test/fixtures/object-spread/side-effect/exec.js +++ b/packages/babel-plugin-proposal-object-rest-spread/test/fixtures/object-spread/side-effect/exec.js @@ -1,4 +1,8 @@ var k = { a: 1, b: 2 }; var o = { a: 3, ...k, b: k.a++ }; - expect(o).toEqual({a: 1, b: 1}); + +var k = { a: 1, get b() { l = { z: 9 }; return 2; } }; +var l = { c: 3 }; +var o = { ...k, ...l }; +expect(o).toEqual({ a: 1, b: 2, z: 9 }); diff --git a/packages/babel-plugin-proposal-object-rest-spread/test/fixtures/object-spread/side-effect/output.js b/packages/babel-plugin-proposal-object-rest-spread/test/fixtures/object-spread/side-effect/output.js index 19ec3bc08cbe..a06b7aceb423 100644 --- a/packages/babel-plugin-proposal-object-rest-spread/test/fixtures/object-spread/side-effect/output.js +++ b/packages/babel-plugin-proposal-object-rest-spread/test/fixtures/object-spread/side-effect/output.js @@ -25,15 +25,15 @@ function impureFunc() { console.log('hello'); } -var output = _objectSpread(_objectSpread(_objectSpread({}, pureA), {}, { +var output = _objectSpread(_objectSpread(_objectSpread(_objectSpread(_objectSpread(_objectSpread({}, pureA), {}, { get foo() {}, get bar() {} -}, pureB, {}, pureC, {}), impureFunc(), {}, pureD, { +}, pureB), pureC), impureFunc()), pureD), {}, { pureD }); -var simpleOutput = _objectSpread({}, pureA, { +var simpleOutput = _objectSpread(_objectSpread({}, pureA), {}, { test: '1' }, pureB);