Fix leading source file comment emit bugs #1945
Conversation
jakebailey
requested review from
Copilot and
weswigham
and removed request for
Copilot
| containerEnd := p.containerEnd | ||
| declarationListContainerEnd := p.declarationListContainerEnd | ||
| skipLeadingComments := emitFlags&EFNoLeadingComments == 0 && !ast.PositionIsSynthesized(detachedRange.Pos()) | ||
| skipLeadingComments := ast.PositionIsSynthesized(detachedRange.Pos()) || emitFlags&EFNoLeadingComments != 0 |
There was a problem hiding this comment.
This was a porting bug.
There was a problem hiding this comment.
the ol' "let's invert this condition" and then fail to invert the usage/name.
| if !p.writer.IsAtStartOfLine() { | ||
| p.writeLine() | ||
| } |
There was a problem hiding this comment.
This is required. I am not sure where Strada did this particular newline.
There was a problem hiding this comment.
The first line of emitSourceFile is an unconditional writeLine, which is after writeFile's emitShebangIfNeeded and emitPrologueDirectivesIfNeeded calls. Which puts this in exactly the right place.
There was a problem hiding this comment.
Pull Request Overview
This PR fixes bugs in the emission of leading source file comments, particularly addressing issues with comment placement when emitting JavaScript files. The fix ensures that leading comments (such as triple-slash references, JSDoc comments, and pinned comments) are emitted in the correct position relative to module boilerplate code.
Key changes:
- Corrected the logic for when to emit detached comments in source files
- Fixed the placement of comments to appear before
Object.definePropertyexports statements rather than after - Ensured proper line breaks before emitting detached comments after prologue directives
Reviewed Changes
Copilot reviewed 300 out of 857 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| internal/printer/printer.go | Fixed comment emission timing and logic in emitSourceFile to handle non-JSON files correctly, and corrected the conditional logic in emitDetachedCommentsBeforeStatementList |
| testdata/baselines/reference/submodule/compiler/*.js | Updated baseline test outputs showing leading comments now appear before module boilerplate |
| testdata/baselines/reference/submodule/compiler/*.js.diff | Removed diff entries as the output now matches TypeScript's expected behavior |
| testdata/baselines/reference/compiler/*.js | Updated baseline for local test files with corrected comment placement |
| if node.IsDeclarationFile { | ||
| p.emitTripleSlashDirectives(node) | ||
| } | ||
| } else { |
There was a problem hiding this comment.
I don't think a JSON document will ever have detached leading comments (since it only ever has one statement and no prologues), so this could probably be a hardcoded initial empty comment emit state, but it's probably fine.
There was a problem hiding this comment.
Yeah, no test change when I added this back (nil is fine), but since the original code did this unconditionally (both codebases) I thought I would add it back. I bet we just don't have any tests which reprint a JSON file without downleveling it...
| containerEnd := p.containerEnd | ||
| declarationListContainerEnd := p.declarationListContainerEnd | ||
| skipLeadingComments := emitFlags&EFNoLeadingComments == 0 && !ast.PositionIsSynthesized(detachedRange.Pos()) | ||
| skipLeadingComments := ast.PositionIsSynthesized(detachedRange.Pos()) || emitFlags&EFNoLeadingComments != 0 |
There was a problem hiding this comment.
the ol' "let's invert this condition" and then fail to invert the usage/name.
Fixes #1526
#1938 sort of found the right area but I found some misports that fix way more baselines still.