Port object rest and spread transform #1529
Conversation
…ut tests still target es5, so get it right
…e collection to skip the actual rest element
…ittle different from Strada in how they are calculated
There was a problem hiding this comment.
Pull Request Overview
This PR implements object rest and spread transforms for downleveling ES2018 features to earlier targets. The primary purpose is to enable compilation of modern object destructuring and spread syntax (const a = {...b} and const {...x} = {a:1}) for targets that don't natively support ES2018 features.
Key changes include:
- Porting the object rest/spread transform from the existing codebase
- Integrating destructuring transform logic inline for simplicity
- Unifying transform context passing by having all JS transforms accept a proper
Contextparameter
testdata/baselines/reference/submodule/conformance/trailingCommasInBindingPatterns.js
Show resolved
Hide resolved
| @@ -5,9 +5,7 @@ | |||
|
|
|||
| //// [main.cjs] | |||
| -const tslib_1 = require("tslib"); | |||
| +import tslib_1 = require("tslib"); | |||
There was a problem hiding this comment.
IMO, this is obviously not right - even in verbatimModuleSyntax, you shouldn't have TS syntax in the output - but it's not the fault of this PR. module: preserve seems to be broken with importHelpers right now, and adding the helper to this test just triggers the broken behavior (but, tbf, I'm not sure if it should output a const or an import * as there in that scenario without peeking at other exports and guessing the format!).
| @@ -22,11 +22,22 @@ function test<T extends object>(value: T): Test { | |||
|
|
|||
|
|
|||
| //// [intersectionPropertyCheck.js] | |||
| var __assign = (this && this.__assign) || function () { | |||
There was a problem hiding this comment.
Aside: If our min target is truly supposed to be es2015, this helper should never be emitted (Object.assign is used directly instead), but our tests don't default to that yet, so I err'd on keeping the helper output to minimize diffs.
There was a problem hiding this comment.
I'm confused because I removed the es2015 transforms from the submodule; is there more code that needs to be removed that I forgot about, maybe?
There was a problem hiding this comment.
(I had various different versions of that change so I may have only done a subset and removed the transform, but not other mentions of es2015 in other transforms)
There was a problem hiding this comment.
Tests still run with ScriptTargetES5 - In this case, some helper emit logic in the core printer is conditioned on it.
| // https://github.com/microsoft/TypeScript/issues/43400 | ||
| var x, y; | ||
| -[_a] = [{ abc: 1 }], x = __rest(_a, []); | ||
| -for (let _c of [[{ abc: 1 }]]) { | ||
| - [_b] = _c, y = __rest(_b, []); | ||
| +[{ ...x }] = [{ abc: 1 }]; | ||
| +for ([{ ...y }] of [[{ abc: 1 }]]) | ||
| +_a = [{ abc: 1 }], [_b] = _a, x = __rest(_b, []); |
There was a problem hiding this comment.
In strada, there's a bunch of logic to allow sometimes inlining the temporary used as the first argument of the __rest call if it's not used outside the rest expression and doesn't contain anything sensitive to execution order... but it's a fair chunk of logic that doesn't map cleanly to the new, more structured visitor/transformer shape (in strada there's actually a separate visitor for value-unused expressions which enables this simplification) for only a handful of tests cleaner output. I could work on adding that in if it's important, but it hardly seemed worth it to me.
| func getModuleTransformer(emitContext *printer.EmitContext, options *core.CompilerOptions, resolver binder.ReferenceResolver, getEmitModuleFormatOfFile func(file ast.HasFileName) core.ModuleKind) *transformers.Transformer { | ||
| switch options.GetEmitModuleKind() { | ||
| func getModuleTransformer(ctx context.Context, resolver binder.ReferenceResolver, getEmitModuleFormatOfFile func(file ast.HasFileName) core.ModuleKind) *transformers.Transformer { | ||
| switch transformers.GetCompilerOptionsFromContext(ctx).GetEmitModuleKind() { |
There was a problem hiding this comment.
I feel kinda weird about context.Context being a grab back for stuff that was previously nice and static, but I suppose it's fine...
| case core.ModuleKindPreserve: | ||
| // `ESModuleTransformer` contains logic for preserving CJS input syntax in `--module preserve` | ||
| return moduletransforms.NewESModuleTransformer(emitContext, options, resolver, getEmitModuleFormatOfFile) | ||
| return moduletransforms.NewESModuleTransformer(ctx, resolver, getEmitModuleFormatOfFile) |
There was a problem hiding this comment.
Reading further, I'm surprised to see what is basically a constructor function taking in a context; that usually implies that something is being closed over out of the context and then returned (outside the implied lifetime of the context as a function scoped var), which makes me feel a bit unsure.
There was a problem hiding this comment.
The other option would be having a struct like TransformOptions that just has a grabbag of stuff some transforms use (because we'd like them to have the same constructor shape so they dynamically compose); but the Context itself is good for the EmitResolver users in particular (declarations, const enum, decorator metadata), since it'll allow us to properly thread the context into those checker API calls without storing it in the host. Not that I've done that yet.
There was a problem hiding this comment.
thread the context into those checker API calls without storing it in the host
Is that something we'll need? IIRC we don't need the context in the checker except for cancellation, which isn't checked outside of plain top-down checking.
|
I feel pretty uneasy about the context change; is that required for the other stuff in this PR? |
|
If we're going to need it anyway to grab out a |
For reference, this enables downleveling for:
and
to targets before its' native support in es2018.
This is a good chunk of what is the es2018 transform in strada (most of the rest being async generators). This required porting the destructuring transform strada uses in many places - for simplicity's sake, it's inlined into the rest/spread transform for now, until such time as it's needed elsewhere.
Additionally, to unify how disparate options are passed to transforms, all the js transforms now take a proper
Context(vs an*EmitContextand sometimes*CompilerOptions), which is expected to contain any set options the transform may use. I should probably broaden this to include the declaration emit and module transforms as well, so that all transform factories conform to the samefunc(ctx context.Context) *transforms.Transformershape, but that cleanup can come later.