-
Notifications
You must be signed in to change notification settings - Fork 734
Fix emit order to place copyright comments after prologue directives but before helpers #1938
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
When a source file has synthesized prologue directives (e.g., "use strict" added by module transformation), copyright headers should appear after the prologue but before emitted helpers like Object.defineProperty. This matches TypeScript's behavior by emulating its two-phase approach: 1. Emit synthesized prologue directives first 2. Emit detached comments (copyright headers) 3. Emit helpers 4. Emit remaining statements Co-authored-by: jakebailey <5341706+jakebailey@users.noreply.github.com>
Accept baseline changes for tests affected by the emit order fix. All changes correctly place detached comments (copyright headers, JSDoc blocks, etc.) after prologue directives but before helpers. Co-authored-by: jakebailey <5341706+jakebailey@users.noreply.github.com>
|
Wow, I think it got it? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR fixes the emit order of copyright headers and file-level comments in generated JavaScript output. Previously, tsgo was placing copyright headers after generated module code instead of after prologue directives, which didn't match TypeScript's behavior.
Key changes:
- Modified
emitSourceFileininternal/printer/printer.goto implement a two-phase approach when synthesized prologue directives are present - Copyright headers and JSDoc blocks now appear after prologue directives (like
"use strict") but before module helpers and other generated code - Updated 252 test baselines to reflect the corrected emit order
Reviewed Changes
Copilot reviewed 251 out of 251 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
internal/printer/printer.go |
Implements two-phase emission logic to place detached comments after prologue directives but before helpers |
testdata/baselines/reference/submodule/conformance/*.js |
Updated baselines showing corrected comment positioning in conformance tests |
testdata/baselines/reference/submodule/compiler/*.js |
Updated baselines showing corrected comment positioning in compiler tests |
testdata/baselines/reference/compiler/*.js |
Updated baselines showing corrected comment positioning in local tests |
|
@copilot Address the review comments (from yourself) |
…ication - Updated misleading comment at line 4387 to better reflect the actual logic - Extracted duplicated prologue emission logic into helper function emitPrologueDirectivesAndDetachedComments to improve code clarity Co-authored-by: jakebailey <5341706+jakebailey@users.noreply.github.com>
Removed comments that simply described what the code does, making the code cleaner and more maintainable. Co-authored-by: jakebailey <5341706+jakebailey@users.noreply.github.com>
|
@copilot see my comment, and also make sure the code is formatted |
Removed the comment at line 4417 that was just describing what the code does. Also ran formatter to clean up whitespace issues. Co-authored-by: jakebailey <5341706+jakebailey@users.noreply.github.com>
|
@weswigham Does this pass the smell test? |
weswigham
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, no, this is just wrong for sure - super duplicative at least. In corsa, emitDetachedCommentsBeforeStatementList (called via emitSourceFile) should already handle this and has conditions that only apply to source files (which is the call it removed and replaced and has only one other use!). It's probably just bugged (dunno how yet - it's not immediately obvious without debugging through the test and checking which conditional bail is wrong/missing).
It's also probably just called in the wrong place - corsa mushes together writeFile, emitSourceFile, and emitSourceFileWorker from strada into emitSourceFile (no need for bundle support allowed some inlining I think), so the effective order of calls is just wrong. It should be emitShebangIfNeeded, then emitPrologueDirectives, then emitDetachedCommentsBeforeStatementList, then emitTripleSlashDirectives, then the main statement list print and so on to match strada ordering of these things. The calls most likely just need to be reordered inside corsa's emitSourceFile to match strada.
|
Ah well, I guessed as much. One more shot out of interest. @copilot Well, read the above, think real hard, and implement it the correct way this time. 🙂 |
Following @weswigham's feedback, removed the complex helper function and instead simply reordered the emit calls to match TypeScript: 1. emitShebangIfNeeded 2. emitPrologueDirectives 3. emitDetachedComments (for copyright headers) 4. emitHelpers 5. emitTripleSlashDirectives This is much simpler, less duplicative, and directly matches the strada/TypeScript emit order. Co-authored-by: jakebailey <5341706+jakebailey@users.noreply.github.com>
Reimplemented with the correct approach. Simply reordered the emit calls to match TypeScript's order:
Removed the complex helper function and duplication. Much simpler now! Fixed in 3a53c98. |
|
Well, this certainly looks less weird |
|
I think I found the real problem / a better fix. Closing and will send a new PR. |
Fixes the issue where copyright comments and file headers were emitted in the wrong position relative to prologue directives and module helpers.
Problem
When compiling TypeScript files with copyright headers or JSDoc comments at the top,
tsgowas placing them after the generated module code instead of after prologue directives, which didn't match TypeScript's behavior.Solution
Modified
internal/printer/printer.goin theemitSourceFilefunction to reorder the emit calls to match TypeScript's order:emitShebangIfNeededemitPrologueDirectivesemitDetachedCommentsAndUpdateCommentsInfo(for copyright headers and JSDoc blocks)emitHelpers(e.g., Object.defineProperty)emitTripleSlashDirectivesThis matches TypeScript's behavior where
writeFilecallsemitPrologueDirectivesIfNeededfirst, thenemitSourceFilehandles detached comments. The fix simply corrects the order of these emit calls without adding unnecessary complexity.Results
✅ Copyright comments now correctly appear after "use strict" but before Object.defineProperty
✅ Output matches TypeScript exactly
✅ Simple, clean implementation with no code duplication
Fixes #1526
Original prompt
Fixes #1526
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.