ES5:Emit parameter initialiser before object rest destructuring#14090
Conversation
Fix #14026, where ES5 emit for a parameter with 1. a default value initialiser 2. an object binding pattern containing an object rest incorrectly emitted the destructuring for the object rest before the default value initialisation. This happened because, during emit, the ES next transform runs first, transforming object rest destructuring and marking it as part of the function prologue. Then the ES5 transform runs and transforms the default initialiser, also marking it as part of the prologue. Then the prologue is emitted in the order the statements were added. The fix is to not mark the object rest destructuring as part of the prologue. I'm not 100% sure that this is the right fix, but it fixes the bug as it stands today. Here's an example: ```ts function foobar({ bar={}, ...opts }: any = {}) { } ``` which should have the ES5 emit: ```js function foobar(_a) { if (_a === void 0) { _a = {}; } var _b = _a.bar, bar = _b === void 0 ? {} : _b, opts = __rest(_a, ["bar"]); } ```
|
From in-person discussion with @rbuckton, I'm going to try the alternate fix of |
|
@rbuckton can you take another look at this? |
| if (constructor) { | ||
| addDefaultValueAssignmentsIfNeeded(statements, constructor); | ||
| addRestParameterIfNeeded(statements, constructor, hasSynthesizedSuper); | ||
| if (!hasSynthesizedSuper) { |
There was a problem hiding this comment.
We need to always emit real prologue directives first (e.g. "use strict"), then our custom prologue statements.
There was a problem hiding this comment.
Maybe we need to split addPrologueDirectives into two functions, addPrologueDirectives (which only handles real prologue directives) and addCustomPrologueDirectives (which only handles our custom leading statements).
There was a problem hiding this comment.
How do I tell the difference? Is "use strict" the only real prologue directive right now?
There was a problem hiding this comment.
Ah, it just looks like it's any ExpressionStatement at the beginning of a file/function that is just a string literal.
| if (isBlock(body)) { | ||
| // ensureUseStrict is false because no new prologue-directive should be added. | ||
| // addPrologueDirectives will simply put already-existing directives at the beginning of the target statement-array | ||
| const statementOffset = addPrologueDirectives(statements, body.statements, /*ensureUseStrict*/ false, visitor); |
There was a problem hiding this comment.
This also would result in a "use strict" appearing after executable statements.
There was a problem hiding this comment.
I'm surprised we either don't have a test for this or that an existing test isn't failing.
There was a problem hiding this comment.
I'm trying to create a test and realised I don't know how to get "use strict" to be generated anywhere except at the beginning of a file. How can I get it to emit at the beginning of a function?
There was a problem hiding this comment.
You write it yourself. We respect existing directives.
The original function is now named addPrologue. There are then three places than need to call the two split functions separately.
sandersn
left a comment
There was a problem hiding this comment.
I split the functions as you suggested. It turns out that transformFunctionBody in esnext was not calling addPrologue, so I added it there as well.
| if (constructor) { | ||
| addDefaultValueAssignmentsIfNeeded(statements, constructor); | ||
| addRestParameterIfNeeded(statements, constructor, hasSynthesizedSuper); | ||
| if (!hasSynthesizedSuper) { |
There was a problem hiding this comment.
Ah, it just looks like it's any ExpressionStatement at the beginning of a file/function that is just a string literal.
rbuckton
left a comment
There was a problem hiding this comment.
Minor nit regarding naming, otherwise it looks good.
There was a problem hiding this comment.
To differentiate, perhaps its name should be addStandardPrologue?
Fix #14026, where ES5 emit for a parameter with
incorrectly emitted the destructuring for the object rest before the default value initialisation.
This happened because, during emit, the ES next transform runs first, transforming object rest destructuring and marking it as part of the function prologue. Then the ES5 transform runs and transforms the default initialiser, also marking it as part of the prologue. Then the prologue is emitted in the order the statements were added.
The fix is to not mark the object rest destructuring as part of the prologue. I'm not 100% sure that this is the right fix, but it fixes the bug as it stands today.
Here's an example:
which should have the ES5 emit: