Repository navigation
Copy synthetic comments in SetOriginal and emit them once in TS trasforms - #64736
Trevor Florence (trevorade) wants to merge 4 commits into
Conversation
…forms
In the TypeScript 6 compiler, `setOriginalNode` merges the original node's emit node into the new node (`mergeEmitNode`), including its synthetic leading and trailing comments. In tsgo, `emitNode.copyFrom` copies comment and source map ranges, emit flags, and helpers, but not synthetic comments. So a synthetic comment attached to a declaration is dropped as soon as a transform replaces that declaration. For example, with a synthetic `/*leading*/` comment on `enum E { A }`, the TS transforms emit `var E; (function (E) { ... })(E || (E = {}));` with no comment at all.
This PR:
- copies synthetic comments in `copyFrom`, prepending them to any the destination already has, as `mergeEmitNode` does;
- clears them on the `VariableDeclaration` that `addVarForDeclaration` creates for an enum or namespace, as Strada does (this was the `// !!! synthetic comments` TODO), so they are emitted once, on the `var` statement;
- clears them on the class expression, its variable declaration, and the `__decorate` statement in the legacy decorators transform, so they are emitted once, on the `let` statement.
The last item goes beyond a port. Strada calls `setOriginalNode` on all of those nodes without clearing anything, and TypeScript 6.0.3 prints a class's synthetic comment four times under `experimentalDecorators`:
```js
/*synthetic*/ let /*synthetic*/ C = /*synthetic*/ class C {
};
/*synthetic*/ C = __decorate([
dec
], C);
```
That is microsoft#54742 (labeled Bug and Help Wanted). Its fix, microsoft#54750, was closed unmerged in the post-6.0 freeze. Copying synthetic comments without also clearing them here would carry that bug into tsgo.
One difference from Strada remains: a trailing synthetic comment on an enum or namespace is emitted after the closing `})(E || (E = {}));`, not after `var E;`. tsgo's printer treats `EFNoTrailingComments` on the `var` statement as also suppressing synthetic trailing comments. The new position matches where source trailing comments for these declarations already go.
The new test `TestSyntheticCommentsAreEmittedOnce` covers an enum, a namespace, and a decorated class, with a leading and a trailing synthetic comment on each. Without this change, all three cases emit no comment.
…is added Strada clears them on the enum and namespace statements when it emits a leading `var` (ts.ts, `visitEnumDeclaration` and `visitModuleDeclaration`), so the comments are emitted once, on the `var` statement. tsgo relied on `EFNoLeadingComments` suppressing synthesized comments instead, which microsoft#64453 reports as a divergence from Strada and microsoft#64488 would remove.
There was a problem hiding this comment.
🟡 Changes recommended
The class-fields static-block transform can duplicate synthetic comments after #64488.
1 open finding
What changed in this PR
Brings synthetic-comment handling in tsgo transforms closer to the TypeScript compiler while preventing duplicate emission.
Changes:
- Copies synthetic comments during
SetOriginal. - Clears duplicate comments from enum, namespace, and legacy-decorator outputs.
- Adds regression coverage for single emission.
| File | Description |
|---|---|
syntheticcomments_test.go |
Tests enum, namespace, and decorated-class emission. |
runtimesyntax.go |
Prevents duplicate comments in runtime syntax transforms. |
legacydecorators.go |
Prevents decorated-class comment duplication. |
emitcontext.go |
Copies and merges synthetic comments. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
When a static property initializer becomes `static { this.x = ...; }`,
the property's comments are emitted on the static block. Strada clears
them on the initializer statement (classFields.ts,
`transformPropertyOrClassStaticBlock` caller in `visitPropertyDeclaration`).
tsgo only set `EFNoComments`, which stops suppressing synthetic comments
once microsoft#64488 lands, so the comments would be printed twice.
| @@ -0,0 +1,103 @@ | |||
| package tstransforms_test | |||
There was a problem hiding this comment.
I find this file a little weird. We typically don't do unit tests for these, and this duplicates a helper string, and tests multiple packages ad-hoc.
There was a problem hiding this comment.
Fair points. I looked at doing this as a compiler baseline test instead, but I don't think one can see it: synthetic comments only reach these transforms when an API user adds them (that's how we hit it, via a custom transformer). In a normal emit, the only synthetic comments in JS output come from the const enum inliner, which runs after these transforms, so a baseline passes with or without the change.
I trimmed the test down instead in 2c4d14a. It now just checks that each comment is emitted once rather than matching the whole output, so the duplicated __decorate helper is gone. It's also split by package: the SetOriginal copy is tested next to copyFrom in printer, the enum/namespace/decorator cases stay in tstransforms, and the static field case moved to estransforms.
There was a problem hiding this comment.
I guess we have to do something annoying here because this is all Go API related. Just not mega satisfied with these ad hoc tests. Perhaps it's fine.
…ents Split the synthetic comment tests by package instead of testing several packages from tstransforms: - printer: SetOriginal copying synthetic comments is tested next to the other emit context tests. - tstransforms: the enum, namespace and legacy decorator cases. - estransforms: the static property moved into a static block. The transform tests now count how often each comment is emitted instead of matching the whole output, which removes the duplicated __decorate helper string. emittestutil gains an Emit helper that returns the printed output so tests can make such assertions. A compiler baseline cannot observe this change: in a normal emit, the only synthetic comments in JS output come from the const enum inliner, which runs after these transforms.
Fixes #54742 for tsgo.
In the TypeScript 6 compiler,
setOriginalNodemerges the original node's emit node into the new node (mergeEmitNode), including its synthetic leading and trailing comments. In tsgo,emitNode.copyFromcopies comment and source map ranges, emit flags, and helpers, but not synthetic comments. So a synthetic comment attached to a declaration is dropped as soon as a transform replaces that declaration. For example, with a synthetic/*leading*/comment onenum E { A }, the TS transforms emitvar E; (function (E) { ... })(E || (E = {}));with no comment at all.This PR:
copyFrom, prepending them to any the destination already has, asmergeEmitNodedoes;var, clears them on theVariableDeclaration(this was the// !!! synthetic commentsTODO) and on the(function (E) { ... })(...)statement, as Strada does, so they are emitted once, on thevarstatement;__decoratestatement in the legacy decorators transform, so they are emitted once, on theletstatement.The last item goes beyond a port. Strada calls
setOriginalNodeon all of those nodes without clearing anything, and TypeScript 6.0.3 prints a class's synthetic comment four times underexperimentalDecorators:That is #54742 (Bug, Help Wanted, Backlog). Its fix, #54750, was closed unmerged in the post-6.0 freeze. Copying synthetic comments without also clearing them here would carry that bug into tsgo.
Relationship to #64453 / #64488: tsgo's printer currently treats
EFNoLeadingComments/EFNoTrailingCommentsas also suppressing synthetic comments, which Strada does not, and #64488 removes that. This PR does not depend on either order. Because it ports Strada's explicit clears rather than relying on those flags, nothing is emitted twice before or after #64488. The one visible effect of #64453 here is that a trailing synthetic comment on an enum or namespace is currently not emitted, because thevarstatement hasEFNoTrailingComments. After #64488 it is emitted aftervar E;, as in TypeScript 6. The test comments on this.The new test
TestSyntheticCommentsAreEmittedOncecovers an enum, a namespace, and a decorated class, with a leading and a trailing synthetic comment on each. Without this change, none of the three emits any comment.Verification:
go test ./...passes exceptinternal/astnav'sTestGetTouchingPropertyName, which shells out to Node and needsnode_modules/typescript(it fails the same way onmainwithoutnpm ci). golangci-lint with the repo's custom plugin reports 0 issues on the changed packages, and gofumpt with the repo's dprint settings reports no changes.npx hereby test,lint, andcheck:formatwere not run directly.AI assistance: I used Jetski, Google's internal AI coding agent, to investigate this and to write the patch, the test, and this description. I reviewed the change and will handle review feedback and revisions directly.