Implement ES decorator transform (ESNext -> ES2022)#2926
Implement ES decorator transform (ESNext -> ES2022)#2926AlCalzone wants to merge 4 commits intomicrosoft:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Implements the ES decorator downlevel transform needed for --target es2022 (from esnext) in the TypeScript-Go emitter pipeline, along with the helper emit support and updated golden baselines to reflect the new output.
Changes:
- Adds ES decorator runtime helpers (
__esDecorate,__runInitializers) to the printer helper set. - Ensures the external helpers import declaration is passed through the module transformer visitor so it’s properly transformed for the output module kind.
- Fixes a named-evaluation check in
getAssignedNameOfPropertyNameand updates many conformance/compiler baselines to match the new decorator transform output.
Reviewed changes
Copilot reviewed 154 out of 273 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| internal/printer/helpers.go | Adds new emit helpers for ES decorators and initializer execution. |
| internal/transformers/moduletransforms/esmodule.go | Visits the external helpers import declaration so it can be transformed appropriately (e.g., import = require → const require). |
| internal/transformers/estransforms/namedevaluation.go | Fixes a computed-property-name check to validate the correct node. |
| testdata/baselines/reference/submodule/compiler/importHelpersVerbatimModuleSyntax.js | Baseline updated for the corrected external helpers import emission. |
| testdata/baselines/reference/submodule/compiler/importHelpersVerbatimModuleSyntax.js.diff | Baseline diff updated accordingly. |
| testdata/baselines/reference/submodule/conformance/* | Extensive baseline updates reflecting ES decorator downlevel transform output across many scenarios (classes, fields, accessors, private elements, metadata, named evaluation, using, etc.). |
| statements := slices.Clone(prologue) | ||
| if externalHelpersImportDeclaration != nil { | ||
| statements = append(statements, externalHelpersImportDeclaration) | ||
| statements = append(statements, tx.Visitor().VisitNode(externalHelpersImportDeclaration)) |
There was a problem hiding this comment.
Strada does have a visit here, so I can see why a tool would add it, but surely it's unrelated to the desired change. And probably not requried, since in strada it was likely used to map es imports to amd/umd/cjs ones, meanwhile in corsa it gets to just stay as the es import it's generated as, since cjs is handled by an entirely different transform.
| } | ||
|
|
||
| if !ast.IsComputedPropertyName(expression) { | ||
| if !ast.IsComputedPropertyName(name) { |
There was a problem hiding this comment.
This looks like a correct driveby fix of this helper, but we should go further - since we have it, this should be a debug.Assert now, instead of a raw panic.
| tx.classThis = tx.top.classThisData | ||
| tx.classSuper = tx.top.classSuperData | ||
| case lexicalEntryKindName: | ||
| // name -> class-element -> class -> ??? |
There was a problem hiding this comment.
This comment is ??? - not in the original source, doesn't explain anything. Should be removed.
| } | ||
|
|
||
| func (tx *esDecoratorTransformer) exitClass() { | ||
| tx.pendingExpressions = tx.top.savedPendingExpressions |
There was a problem hiding this comment.
This function is missing a debug.Assert call that asserts that the kind of tx.top is lexicalEntryKindClass.
| } | ||
|
|
||
| func (tx *esDecoratorTransformer) enterClassElement(node *ast.Node) { | ||
| entry := &lexicalEntry{ |
There was a problem hiding this comment.
Likewise, an assert is missing here, too.
| kind: lexicalEntryKindClassElement, | ||
| next: tx.top, | ||
| } | ||
| if ast.IsClassStaticBlockDeclaration(node) || (ast.IsPropertyDeclaration(node) && ast.HasStaticModifier(node)) { |
There was a problem hiding this comment.
What's with the parens around the later half of the condition here? Seems unintentional. Strada doesn't have them, for sure, I don't see why go'd need them.
| if tx.top != nil && tx.top.classInfoData != nil { | ||
| entry.classThisData = tx.top.classInfoData.classThis | ||
| entry.classSuperData = tx.top.classInfoData.classSuper | ||
| } |
There was a problem hiding this comment.
This should have an else to nil these two fields in the entry to match strada behavior, but it might not matter? I wouldn't want the context to leak accidentally though.
| } | ||
|
|
||
| func (tx *esDecoratorTransformer) exitClassElement() { | ||
| tx.top = tx.top.next |
There was a problem hiding this comment.
Once more, this is missing assertions from strada.
| } | ||
|
|
||
| func (tx *esDecoratorTransformer) enterName() { | ||
| tx.top = &lexicalEntry{ |
| } | ||
|
|
||
| func (tx *esDecoratorTransformer) exitName() { | ||
| tx.top = tx.top.next |
|
|
||
| func (tx *esDecoratorTransformer) enterOther() { | ||
| if tx.top != nil && tx.top.kind == lexicalEntryKindOther { | ||
| tx.top.depth++ |
There was a problem hiding this comment.
This is missing a debug.Assert(len(tx.pendingExpressions) == 0)
| } | ||
|
|
||
| func (tx *esDecoratorTransformer) exitOther() { | ||
| if tx.top.depth > 0 { |
There was a problem hiding this comment.
Likewise, another few missing asserts.
| } | ||
| } | ||
|
|
||
| // --- Visitor dispatch --- |
There was a problem hiding this comment.
These "logically grouping" comments weren't in strada and we don't need them here, since it's not like the order is hard and fast anyway - they're very.... C# folding-range-y.
| } | ||
| switch node.Kind { | ||
| case ast.KindDecorator: | ||
| return nil |
There was a problem hiding this comment.
This should have its missing debug.Fail added so it's not a quiet failure.
| if !tx.shouldVisitNode(node) { | ||
| return node | ||
| } | ||
| switch node.Kind { |
There was a problem hiding this comment.
This switch is missing Fails on Constructors, PropertyDeclarations, and ClassStaticBlockDeclarations.
| ast.ChildIsDecorated(false, node, nil) | ||
| } | ||
|
|
||
| func moveRangePastDecorators(node *ast.Node) core.TextRange { |
There was a problem hiding this comment.
I'm confident this is a duplicate of a helper elsewhere already in the go codebase. Likely in the non-standard decorator transform. Extract it to a common util location and reuse it, don't reimplement it.
| return node.Loc | ||
| } | ||
|
|
||
| func moveRangePastModifiers(node *ast.Node) core.TextRange { |
There was a problem hiding this comment.
Likewise with this, this helper is already in the codebase.
|
|
||
| func getHelperVariableName(node *ast.Node) string { | ||
| declarationName := "" | ||
| if node.Name() != nil && ast.IsIdentifier(node.Name()) { |
There was a problem hiding this comment.
This condition is missing an exception for generated identifiers.
| declarationName := "" | ||
| if node.Name() != nil && ast.IsIdentifier(node.Name()) { | ||
| declarationName = node.Name().Text() | ||
| } else if node.Name() != nil && ast.IsPrivateIdentifier(node.Name()) { |
| } | ||
|
|
||
| // getAllDecoratorsOfClass returns the decorators for a class-like declaration (ES decorators). | ||
| func getAllDecoratorsOfClass(node *ast.Node) []*ast.Node { |
There was a problem hiding this comment.
Just delete this helper. Use node.Decorators() instead.
| } | ||
|
|
||
| // getAllDecoratorsOfClassElement returns the decorators for a class element (ES decorators). | ||
| func getAllDecoratorsOfClassElement(member *ast.Node, parent *ast.Node) []*ast.Node { |
There was a problem hiding this comment.
Hold up, this is already in the legacydecorators transform - and more faithfully to strada, too - extract it and reuse it.
| return expression | ||
| } | ||
|
|
||
| // createCallBinding is a simplified version of the factory's createCallBinding. |
There was a problem hiding this comment.
This comment says nothing in the context of corsa (where there is no factory createCallBinding to "simplify") and doesn't refer to strada. Just remove it.
| if callee.Kind == ast.KindSuperKeyword { | ||
| return callee, tx.Factory().NewThisExpression() | ||
| } | ||
| if ast.IsPropertyAccessExpression(callee) { |
There was a problem hiding this comment.
This is missing the EFHelperName branch from strada.
There was a problem hiding this comment.
(Which is important because even though it matches the fallthrough case, its' earlier priority blocks tslib.__helperName references from triggering the property access case unnecessarily)
| } | ||
|
|
||
| func (tx *esDecoratorTransformer) shouldBeCapturedInTempVariable(node *ast.Expression) bool { | ||
| // Capture identifiers that are not simple (i.e., not just a name or "this") |
There was a problem hiding this comment.
In Strada, this function is.... honestly, entirely different. What in the world is going on here?
| } | ||
|
|
||
| // If the class itself is decorated, create a _classThis binding | ||
| if ast.HasDecorators(node) && ast.NodeCanBeDecorated(false, node, nil, nil) { |
There was a problem hiding this comment.
Just use ast.nodeIsDecorated - you'd have to capitalize it to make it public, since it wasn't needed outside other util functions until now.
| return tx.Factory().UpdateConstructorDeclaration(ctor, modifiers, nil, parameters, nil, nil, body) | ||
| } | ||
|
|
||
| func (tx *esDecoratorTransformer) transformConstructorBodyWorker(statementsOut *[]*ast.Statement, statementsIn []*ast.Statement, statementOffset int, superPath []int, superPathDepth int, initializerStatements []*ast.Statement) { |
There was a problem hiding this comment.
It's a bit more idiomatic, imo, to swap this to returning a statement list, rather than modifying the input list like strada did, given how go slices behave.
| superStatementIndex := superPath[superPathDepth] | ||
| // Visit statements before super | ||
| if superStatementIndex > statementOffset { | ||
| stmtVisitor := tx.EmitContext().NewNodeVisitor(tx.visit) |
There was a problem hiding this comment.
...wat. Why is this making a new visitor clone and not just using tx.Visitor(). Below, too. Seems like it got stuck on a very off pattern here.
|
|
||
| func (tx *esDecoratorTransformer) finishClassElement(updated *ast.Node, original *ast.Node) *ast.Node { | ||
| if updated != original { | ||
| tx.EmitContext().AssignCommentRange(updated, original) |
There was a problem hiding this comment.
The comment in the original code is likely worth preserving, so as to alleviate any concerns as to why the two ranges do not match.
| ec := tx.EmitContext() | ||
|
|
||
| if ci == nil { | ||
| modVisitor := ec.NewNodeVisitor(tx.modifierVisitor) |
There was a problem hiding this comment.
Making this on every class element is wasteful - a single visitor in the transform root used everywhere needed should suffice. This pattern is repeated, too - all these extra sub-visitor creations on node visit aughta get removed and replaced with references to either the main tx.Visitor() or a tx.modififersVisitor (renaming the impl to tx.modifiersVisitorFunc) and the like for the other small visitors. (fallbackVisitor, classElementVisitor, etc)
This pervasive anitpattern makes me wanna toss an assert into NewNodeVisitor that throws if it's ever called during an ongoing transformation, but it's generally not a mistake a human makes...
|
|
||
| // Determine decorator kind | ||
| kind := "" | ||
| switch { |
There was a problem hiding this comment.
This switch is missing a debug.Fail on unknown kinds.
| result.thisArg = ci.classThis | ||
| } | ||
|
|
||
| ctorArg := (*ast.Node)(nil) |
There was a problem hiding this comment.
This is non-idiomatic afaik (at least from what I've seen of our codebase) - it should be var ctorArg *ast.Node instead. I'm surprised the linter doesn't express a preference.
| tx.pendingExpressions = append(tx.pendingExpressions, memberDecoratorsAssignment) | ||
|
|
||
| // Determine which decoration statement bucket to use | ||
| var statements *[]*ast.Statement |
There was a problem hiding this comment.
Hm. I'm not terribly comfortable with using pointers to handle field selection like this, but I suppose we do actually want to mutate the member - minimally, we need a safety debug.Fail when we don't set statements, though (as in strada), so we don't try to deference nil later on on the offchance something changes and we fall into that case. I'd almost prefer moving the target list selection to the end, though, and doing non-pointer-based ci.staticNonFieldDecorationStatements = append(ci.staticNonFieldDecorationStatements, esDecorateStatement) and the like instead. Wouldn't be too verbose if extracted to a helper.
| tx.enterClassElement(node) | ||
| f := tx.Factory() | ||
| ec := tx.EmitContext() | ||
|
|
There was a problem hiding this comment.
Like elsewhere, this is missing a debug.Assert - in this case, an assert that this transform doesn't support declare'd props.
| result = tx.Visitor().VisitEachChild(node) | ||
| // Transfer AssignedName metadata to the new node so isClassNamedEvaluationHelperBlock | ||
| // can still find it after visiting (visiting may create a new node when this->_classThis) | ||
| if assignedName := tx.EmitContext().AssignedName(node); assignedName != nil && result != node { |
There was a problem hiding this comment.
This reeks of being an unneeded diff from strada that's covering up another mistake elsewhere. Strada didn't need to manually copy this metadata, especially in what's basically a no-op branch.
| if tx.classInfoStack != nil { | ||
| tx.classInfoStack.hasStaticInitializers = true | ||
| if len(tx.classInfoStack.pendingStaticInitializers) > 0 { | ||
| stmts := []*ast.Statement{} |
There was a problem hiding this comment.
| stmts := []*ast.Statement{} | |
| stmts := []*ast.Node{} |
and just .AsNode() the factory call, rather than converting the slice for no reason. This is kinda nonsensical as-is.
| call := node.AsCallExpression() | ||
| if ast.IsSuperProperty(call.Expression) && tx.classThis != nil { | ||
| expression := tx.Visitor().VisitNode(call.Expression) | ||
| argVisitor := tx.EmitContext().NewNodeVisitor(tx.visit) |
There was a problem hiding this comment.
I'm going to stop calling out these mistakenly immediately constructed visitors at this point. They're all wrong and should use a premade object.
| if isNamedEvaluation(tx.EmitContext(), node) && isAnonymousClassNeedingAssignedName(node.Initializer()) { | ||
| node = transformNamedEvaluation(tx.EmitContext(), node, false, "") | ||
| } | ||
| return tx.Visitor().VisitEachChild(node) |
There was a problem hiding this comment.
This is pretty different from strada, which manually updates/visits the parameter, moves a bunch of spans on the result, and seemingly intentionally elides all parameter modifiers (including decorators). They are technically an error, but it'd be bad to leak them into the output.
| // --- Simple pass-through visitors that handle NamedEvaluation --- | ||
|
|
||
| func (tx *esDecoratorTransformer) visitParameterDeclaration(node *ast.Node) *ast.Node { | ||
| if isNamedEvaluation(tx.EmitContext(), node) && isAnonymousClassNeedingAssignedName(node.Initializer()) { |
There was a problem hiding this comment.
This is not strictly the same as the strada logic - by passing isAnonymousClassNeedingAssignedName as a callback to isNamedEvaluation, it's also calling skipOuterExpressions on the initializer in strada. That aughta be replicated here.
| } | ||
|
|
||
| func (tx *esDecoratorTransformer) visitPropertyAssignment(node *ast.Node) *ast.Node { | ||
| if isNamedEvaluation(tx.EmitContext(), node) && isAnonymousClassNeedingAssignedName(node.Initializer()) { |
There was a problem hiding this comment.
See my previous comment on the isNamedEvaluation call and skipOuterExpressions for here and the following functions, too.
|
|
||
| func (tx *esDecoratorTransformer) visitPropertyAssignment(node *ast.Node) *ast.Node { | ||
| if isNamedEvaluation(tx.EmitContext(), node) && isAnonymousClassNeedingAssignedName(node.Initializer()) { | ||
| node = transformNamedEvaluation(tx.EmitContext(), node, false, "") |
There was a problem hiding this comment.
pretty sure this false should be a call to canIgnoreEmptyStringLiteralInAssignedName, likewise elsewhere.
| if ast.IsPropertyNameLiteral(node) || ast.IsPrivateIdentifier(node) { | ||
| referencedName = tx.Factory().NewStringLiteralFromNode(node) | ||
| name = tx.Visitor().VisitNode(node) | ||
| return |
There was a problem hiding this comment.
Personally, I am not a fan of using named returns like this when a direct return will suffice - obfuscates things for no reason.
|
|
||
| // --- Pending expressions injection --- | ||
|
|
||
| func (tx *esDecoratorTransformer) injectPendingExpressions(expression *ast.Expression) *ast.Expression { |
There was a problem hiding this comment.
This and the following function, injectPendingInitializers, shared a common core helper function for their mostly shared functionality (of chaining expressions) - that common helper aughta be restored, because this repetition feels bad.
| if len(staticMods) == 0 { | ||
| return nil | ||
| } | ||
| return tx.Factory().NewModifierList(staticMods) |
There was a problem hiding this comment.
To fully replicate the functionality of the visitor this replaces, this need a if len(staticMods) == len(modifiers.Nodes) { return modifiers } so it's not overwriting lists it doesn't need to.
Or it should be reimplemented as a visitor.
|
|
||
| // --- findSuperStatementIndexPath (duplicated from tstransforms) --- | ||
|
|
||
| func findSuperStatementIndexPath(statements []*ast.Statement, start int) []int { |
There was a problem hiding this comment.
This is already in runtimesyntax.go - extract and reuse it.
|
|
||
| // --- Slice conversion helpers --- | ||
|
|
||
| func expressionsToNodes(exprs []*ast.Expression) []*ast.Node { |
There was a problem hiding this comment.
All of these slice-casting helpers should be removed and different casting should be used at their use sites instead, they're a waste of memory. There's no situation where we need a slice of Statements, other than local iteration, where we can just do .AsStatement() on the underlying Nodes instead.
| // --- Utility: createAssignmentTargetWrapper --- | ||
| // Creates: ({ set value(_p) { Reflect.set(target, key, _p, receiver) } }).value | ||
|
|
||
| func createAssignmentTargetWrapper(f *printer.NodeFactory, target *ast.Expression, propertyKey *ast.Expression, receiver *ast.Expression) *ast.Expression { |
There was a problem hiding this comment.
This name is misleading since it also builds in the Reflect.set call that was previously done by callers. It's more like createReflectedAssignmentTargetWrapper instead.
|
|
||
| // --- Utility: isCompoundAssignment --- | ||
|
|
||
| func isCompoundAssignment(kind ast.Kind) bool { |
There was a problem hiding this comment.
This duplicates a function in checker - extract it to ast and capitalize it so it's usable in both places.
| } else { | ||
| operator = node.AsPostfixUnaryExpression().Operator | ||
| } | ||
|
|
There was a problem hiding this comment.
This is missing a debug assert on non-++ or -- ops from strada.
weswigham
left a comment
weswigham
left a comment
There was a problem hiding this comment.
There's a pervasive issue with missing assertions and immediately-constructed-visitors that needs fixing throughout, but aside from that there's not too much obviously wrong here.
There's also a lot of useful spec text comments that got dropped in translation - those aughta get copied over, too - they're good reference material.
|
Thanks for the thorough review - I'll see what I can do about it. |
This PR implements the decorator transform portion of #2354
This is the only thing blocking me from adopting
tsgo, so I thought I'd help out by porting the original PR.Disclaimer: This is an entire day worth of Claude Opus tokens. I don't claim I understand how it works, but I did my best to direct Claude to minimize the baseline diffs, of which it removed quite a few.