Skip to content

Fix leading source file comment emit bugs #1945

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.

Sign up for GitHub

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

Merged
merged 4 commits into from
Oct 24, 2025
Merged

Fix leading source file comment emit bugs #1945

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5572376
Fix leading source file comment emit bugs
jakebailey Oct 23, 2025
bf9be16
newline thing
jakebailey Oct 23, 2025
ee2b62b
Accept baselines
jakebailey Oct 23, 2025
18d6d3c
Ensure state is always set
jakebailey Oct 23, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
The table of contents is too big for display.
Diff view
Diff view
  •  
  •  
  •  
10 changes: 8 additions & 2 deletions internal/printer/printer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4384,18 +4384,24 @@ func (p *Printer) emitSourceFile(node *ast.SourceFile) {

p.writeLine()

state := p.emitDetachedCommentsBeforeStatementList(node.AsNode(), node.Statements.Loc)
p.pushNameGenerationScope(node.AsNode())
p.generateAllNames(node.Statements)

index := 0
var state *commentState
if node.ScriptKind != core.ScriptKindJSON {
p.emitShebangIfNeeded(node)
index = p.emitPrologueDirectives(node.Statements)
if !p.writer.IsAtStartOfLine() {
p.writeLine()
}
Comment on lines +4395 to +4397
Copy link
Member Author

@jakebailey jakebailey Oct 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is required. I am not sure where Strada did this particular newline.

Copy link
Member

@weswigham weswigham Oct 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

state = p.emitDetachedCommentsBeforeStatementList(node.AsNode(), node.Statements.Loc)
p.emitHelpers(node.AsNode())
if node.IsDeclarationFile {
p.emitTripleSlashDirectives(node)
}
} else {
Copy link
Member

@weswigham weswigham Oct 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@jakebailey jakebailey Oct 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

state = p.emitDetachedCommentsBeforeStatementList(node.AsNode(), node.Statements.Loc)
}

// !!! Emit triple-slash directives
Expand Down Expand Up @@ -5083,7 +5089,7 @@ func (p *Printer) emitDetachedCommentsBeforeStatementList(node *ast.Node, detach
containerPos := p.containerPos
containerEnd := p.containerEnd
declarationListContainerEnd := p.declarationListContainerEnd
skipLeadingComments := emitFlags&EFNoLeadingComments == 0 && !ast.PositionIsSynthesized(detachedRange.Pos())
skipLeadingComments := ast.PositionIsSynthesized(detachedRange.Pos()) || emitFlags&EFNoLeadingComments != 0
Copy link
Member Author

@jakebailey jakebailey Oct 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was a porting bug.

Copy link
Member

@weswigham weswigham Oct 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the ol' "let's invert this condition" and then fail to invert the usage/name.


if !skipLeadingComments {
p.emitDetachedCommentsAndUpdateCommentsInfo(detachedRange)
Expand Down
4 changes: 2 additions & 2 deletions testdata/baselines/reference/compiler/declarationEmitNoCrashOnCrossFileNode.js
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion testdata/baselines/reference/compiler/emitEndOfFileJSDocComments2.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,8 @@ function test(param) {

//// [emitEndOfFileJSDocComments2.js]
"use strict";
Object.defineProperty(exports, "__esModule", { value: true });
/** @typedef {number} A */
Object.defineProperty(exports, "__esModule", { value: true });
/**
* JSDoc comment for function
* @param {string} param - A string parameter
Expand Down
5 changes: 5 additions & 0 deletions testdata/baselines/reference/submodule/compiler/APISample_Watch.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,11 @@ watchMain();

//// [APISample_Watch.js]
"use strict";
/*
* Note: This test is a public API sample. The sample sources can be found
* at: https://github.com/Microsoft/TypeScript-wiki/blob/master/Using-the-Compiler-API.md#writing-an-incremental-program-watcher
* Please log a "breaking change" issue for any API breaking change affecting this issue
*/
Object.defineProperty(exports, "__esModule", { value: true });
const ts = require("typescript");
const formatHost = {
Expand Down
12 changes: 3 additions & 9 deletions testdata/baselines/reference/submodule/compiler/APISample_Watch.js.diff
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,8 @@
--- old.APISample_Watch.js
+++ new.APISample_Watch.js
@@= skipped -84, +84 lines =@@

//// [APISample_Watch.js]
"use strict";
-/*
- * Note: This test is a public API sample. The sample sources can be found
- * at: https://github.com/Microsoft/TypeScript-wiki/blob/master/Using-the-Compiler-API.md#writing-an-incremental-program-watcher
- * Please log a "breaking change" issue for any API breaking change affecting this issue
- */
@@= skipped -90, +90 lines =@@
* Please log a "breaking change" issue for any API breaking change affecting this issue
*/
Object.defineProperty(exports, "__esModule", { value: true });
-var ts = require("typescript");
+const ts = require("typescript");
Expand Down
4 changes: 4 additions & 0 deletions testdata/baselines/reference/submodule/compiler/APISample_WatchWithDefaults.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,10 @@ watchMain();

//// [APISample_WatchWithDefaults.js]
"use strict";
/*
* Note: This test is a public API sample. This uses default sys interface without having to pass anything
* Please log a "breaking change" issue for any API breaking change affecting this issue
*/
Object.defineProperty(exports, "__esModule", { value: true });
const ts = require("typescript");
function watchMain() {
Expand Down
11 changes: 3 additions & 8 deletions testdata/baselines/reference/submodule/compiler/APISample_WatchWithDefaults.js.diff
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,8 @@
--- old.APISample_WatchWithDefaults.js
+++ new.APISample_WatchWithDefaults.js
@@= skipped -57, +57 lines =@@

//// [APISample_WatchWithDefaults.js]
"use strict";
-/*
- * Note: This test is a public API sample. This uses default sys interface without having to pass anything
- * Please log a "breaking change" issue for any API breaking change affecting this issue
- */
@@= skipped -62, +62 lines =@@
* Please log a "breaking change" issue for any API breaking change affecting this issue
*/
Object.defineProperty(exports, "__esModule", { value: true });
-var ts = require("typescript");
+const ts = require("typescript");
Expand Down
4 changes: 4 additions & 0 deletions testdata/baselines/reference/submodule/compiler/APISample_WatchWithOwnWatchHost.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,10 @@ watchMain();

//// [APISample_WatchWithOwnWatchHost.js]
"use strict";
/*
* Note: This test is a public API sample. This sample verifies creating abstract builder to watch list of root files
* Please log a "breaking change" issue for any API breaking change affecting this issue
*/
Object.defineProperty(exports, "__esModule", { value: true });
const ts = require("typescript");
function watchMain() {
Expand Down
11 changes: 3 additions & 8 deletions testdata/baselines/reference/submodule/compiler/APISample_WatchWithOwnWatchHost.js.diff
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,8 @@
--- old.APISample_WatchWithOwnWatchHost.js
+++ new.APISample_WatchWithOwnWatchHost.js
@@= skipped -64, +64 lines =@@

//// [APISample_WatchWithOwnWatchHost.js]
"use strict";
-/*
- * Note: This test is a public API sample. This sample verifies creating abstract builder to watch list of root files
- * Please log a "breaking change" issue for any API breaking change affecting this issue
- */
@@= skipped -69, +69 lines =@@
* Please log a "breaking change" issue for any API breaking change affecting this issue
*/
Object.defineProperty(exports, "__esModule", { value: true });
-var ts = require("typescript");
+const ts = require("typescript");
Expand Down
5 changes: 5 additions & 0 deletions testdata/baselines/reference/submodule/compiler/APISample_compile.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,11 @@ compile(process.argv.slice(2), {

//// [APISample_compile.js]
"use strict";
/*
* Note: This test is a public API sample. The sample sources can be found
* at: https://github.com/Microsoft/TypeScript/wiki/Using-the-Compiler-API#a-minimal-compiler
* Please log a "breaking change" issue for any API breaking change affecting this issue
*/
Object.defineProperty(exports, "__esModule", { value: true });
exports.compile = compile;
const ts = require("typescript");
Expand Down
11 changes: 2 additions & 9 deletions testdata/baselines/reference/submodule/compiler/APISample_compile.js.diff
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,7 @@
--- old.APISample_compile.js
+++ new.APISample_compile.js
@@= skipped -47, +47 lines =@@

//// [APISample_compile.js]
"use strict";
-/*
- * Note: This test is a public API sample. The sample sources can be found
- * at: https://github.com/Microsoft/TypeScript/wiki/Using-the-Compiler-API#a-minimal-compiler
- * Please log a "breaking change" issue for any API breaking change affecting this issue
- */
@@= skipped -54, +54 lines =@@
*/
Object.defineProperty(exports, "__esModule", { value: true });
exports.compile = compile;
-var ts = require("typescript");
Expand Down
6 changes: 6 additions & 0 deletions testdata/baselines/reference/submodule/compiler/APISample_jsdoc.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,12 @@ function getSomeOtherTags(node: ts.Node) {

//// [APISample_jsdoc.js]
"use strict";
/*
* Note: This test is a public API sample. The original sources can be found
* at: https://github.com/YousefED/typescript-json-schema
* https://github.com/vega/ts-json-schema-generator
* Please log a "breaking change" issue for any API breaking change affecting this issue
*/
Object.defineProperty(exports, "__esModule", { value: true });
const ts = require("typescript");
// excerpted from https://github.com/YousefED/typescript-json-schema
Expand Down
13 changes: 3 additions & 10 deletions testdata/baselines/reference/submodule/compiler/APISample_jsdoc.js.diff
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,15 +1,8 @@
--- old.APISample_jsdoc.js
+++ new.APISample_jsdoc.js
@@= skipped -122, +122 lines =@@

//// [APISample_jsdoc.js]
"use strict";
-/*
- * Note: This test is a public API sample. The original sources can be found
- * at: https://github.com/YousefED/typescript-json-schema
- * https://github.com/vega/ts-json-schema-generator
- * Please log a "breaking change" issue for any API breaking change affecting this issue
- */
@@= skipped -129, +129 lines =@@
* Please log a "breaking change" issue for any API breaking change affecting this issue
*/
Object.defineProperty(exports, "__esModule", { value: true });
-var ts = require("typescript");
+const ts = require("typescript");
Expand Down
5 changes: 5 additions & 0 deletions testdata/baselines/reference/submodule/compiler/APISample_linter.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,11 @@ fileNames.forEach(fileName => {

//// [APISample_linter.js]
"use strict";
/*
* Note: This test is a public API sample. The sample sources can be found
* at: https://github.com/Microsoft/TypeScript/wiki/Using-the-Compiler-API#traversing-the-ast-with-a-little-linter
* Please log a "breaking change" issue for any API breaking change affecting this issue
*/
Object.defineProperty(exports, "__esModule", { value: true });
exports.delint = delint;
const ts = require("typescript");
Expand Down
11 changes: 2 additions & 9 deletions testdata/baselines/reference/submodule/compiler/APISample_linter.js.diff
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,7 @@
--- old.APISample_linter.js
+++ new.APISample_linter.js
@@= skipped -73, +73 lines =@@

//// [APISample_linter.js]
"use strict";
-/*
- * Note: This test is a public API sample. The sample sources can be found
- * at: https://github.com/Microsoft/TypeScript/wiki/Using-the-Compiler-API#traversing-the-ast-with-a-little-linter
- * Please log a "breaking change" issue for any API breaking change affecting this issue
- */
@@= skipped -80, +80 lines =@@
*/
Object.defineProperty(exports, "__esModule", { value: true });
exports.delint = delint;
-var ts = require("typescript");
Expand Down
5 changes: 5 additions & 0 deletions testdata/baselines/reference/submodule/compiler/APISample_parseConfig.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,11 @@ export function createProgram(rootFiles: string[], compilerOptionsJson: string):

//// [APISample_parseConfig.js]
"use strict";
/*
* Note: This test is a public API sample. The sample sources can be found
* at: https://github.com/Microsoft/TypeScript/wiki/Using-the-Compiler-API#a-minimal-compiler
* Please log a "breaking change" issue for any API breaking change affecting this issue
*/
Object.defineProperty(exports, "__esModule", { value: true });
exports.createProgram = createProgram;
const ts = require("typescript");
Expand Down
11 changes: 2 additions & 9 deletions testdata/baselines/reference/submodule/compiler/APISample_parseConfig.js.diff
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,7 @@
--- old.APISample_parseConfig.js
+++ new.APISample_parseConfig.js
@@= skipped -45, +45 lines =@@

//// [APISample_parseConfig.js]
"use strict";
-/*
- * Note: This test is a public API sample. The sample sources can be found
- * at: https://github.com/Microsoft/TypeScript/wiki/Using-the-Compiler-API#a-minimal-compiler
- * Please log a "breaking change" issue for any API breaking change affecting this issue
- */
@@= skipped -52, +52 lines =@@
*/
Object.defineProperty(exports, "__esModule", { value: true });
exports.createProgram = createProgram;
-var ts = require("typescript");
Expand Down
5 changes: 5 additions & 0 deletions testdata/baselines/reference/submodule/compiler/APISample_transform.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,11 @@ console.log(JSON.stringify(result));

//// [APISample_transform.js]
"use strict";
/*
* Note: This test is a public API sample. The sample sources can be found
* at: https://github.com/Microsoft/TypeScript/wiki/Using-the-Compiler-API#a-simple-transform-function
* Please log a "breaking change" issue for any API breaking change affecting this issue
*/
Object.defineProperty(exports, "__esModule", { value: true });
const ts = require("typescript");
const source = "let x: string = 'string'";
Expand Down
12 changes: 3 additions & 9 deletions testdata/baselines/reference/submodule/compiler/APISample_transform.js.diff
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,8 @@
--- old.APISample_transform.js
+++ new.APISample_transform.js
@@= skipped -25, +25 lines =@@

//// [APISample_transform.js]
"use strict";
-/*
- * Note: This test is a public API sample. The sample sources can be found
- * at: https://github.com/Microsoft/TypeScript/wiki/Using-the-Compiler-API#a-simple-transform-function
- * Please log a "breaking change" issue for any API breaking change affecting this issue
- */
@@= skipped -31, +31 lines =@@
* Please log a "breaking change" issue for any API breaking change affecting this issue
*/
Object.defineProperty(exports, "__esModule", { value: true });
-var ts = require("typescript");
+const ts = require("typescript");
Expand Down
5 changes: 5 additions & 0 deletions testdata/baselines/reference/submodule/compiler/APISample_watcher.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,11 @@ watch(currentDirectoryFiles, { module: ts.ModuleKind.CommonJS });

//// [APISample_watcher.js]
"use strict";
/*
* Note: This test is a public API sample. The sample sources can be found
* at: https://github.com/Microsoft/TypeScript/wiki/Using-the-Compiler-API#incremental-build-support-using-the-language-services
* Please log a "breaking change" issue for any API breaking change affecting this issue
*/
Object.defineProperty(exports, "__esModule", { value: true });
const ts = require("typescript");
function watch(rootFileNames, options) {
Expand Down
12 changes: 3 additions & 9 deletions testdata/baselines/reference/submodule/compiler/APISample_watcher.js.diff
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,8 @@
--- old.APISample_watcher.js
+++ new.APISample_watcher.js
@@= skipped -120, +120 lines =@@

//// [APISample_watcher.js]
"use strict";
-/*
- * Note: This test is a public API sample. The sample sources can be found
- * at: https://github.com/Microsoft/TypeScript/wiki/Using-the-Compiler-API#incremental-build-support-using-the-language-services
- * Please log a "breaking change" issue for any API breaking change affecting this issue
- */
@@= skipped -126, +126 lines =@@
* Please log a "breaking change" issue for any API breaking change affecting this issue
*/
Object.defineProperty(exports, "__esModule", { value: true });
-var ts = require("typescript");
+const ts = require("typescript");
Expand Down
1 change: 1 addition & 0 deletions testdata/baselines/reference/submodule/compiler/accessorInAmbientContextES5.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ class RegularClass {
}

//// [accessorInAmbientContextES5.js]
// Should allow accessor in ambient contexts even when targeting ES5
// Regular class should still error when targeting ES5
class RegularClass {
accessor shouldError; // Should still error
Expand Down
5 changes: 2 additions & 3 deletions testdata/baselines/reference/submodule/compiler/accessorInAmbientContextES5.js.diff
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,9 @@
--- old.accessorInAmbientContextES5.js
+++ new.accessorInAmbientContextES5.js
@@= skipped -28, +28 lines =@@
}
@@= skipped -29, +29 lines =@@

//// [accessorInAmbientContextES5.js]
-// Should allow accessor in ambient contexts even when targeting ES5
// Should allow accessor in ambient contexts even when targeting ES5
-var __classPrivateFieldGet = (this && this.__classPrivateFieldGet) || function (receiver, state, kind, f) {
- if (kind === "a" && !f) throw new TypeError("Private accessor was defined without a getter");
- if (typeof state === "function" ? receiver !== state || !f : !state.has(receiver)) throw new TypeError("Cannot read private member from an object whose class did not declare it");
Expand Down
1 change: 1 addition & 0 deletions ...ines/reference/submodule/compiler/aliasOfGenericFunctionWithRestBehavedSameAsUnaliased.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ let check3: test3 = "y";


//// [aliasOfGenericFunctionWithRestBehavedSameAsUnaliased.js]
// the type printback for every `test` below should be "y"
let check = "y";
let check1 = "y";
let check2 = "y";
Expand Down
5 changes: 2 additions & 3 deletions ...reference/submodule/compiler/aliasOfGenericFunctionWithRestBehavedSameAsUnaliased.js.diff
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@

//// [aliasOfGenericFunctionWithRestBehavedSameAsUnaliased.js]
-"use strict";
-// the type printback for every `test` below should be "y"
// the type printback for every `test` below should be "y"
let check = "y";
let check1 = "y";
let check2 = "y";
let check1 = "y";
2 changes: 1 addition & 1 deletion testdata/baselines/reference/submodule/compiler/amdDependencyComment1.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ m1.f();

//// [amdDependencyComment1.js]
"use strict";
Object.defineProperty(exports, "__esModule", { value: true });
///<amd-dependency path='bar'/>
Object.defineProperty(exports, "__esModule", { value: true });
const m1 = require("m2");
m1.f();
7 changes: 2 additions & 5 deletions testdata/baselines/reference/submodule/compiler/amdDependencyComment1.js.diff
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,9 @@
--- old.amdDependencyComment1.js
+++ new.amdDependencyComment1.js
@@= skipped -7, +7 lines =@@

//// [amdDependencyComment1.js]
@@= skipped -9, +9 lines =@@
"use strict";
-///<amd-dependency path='bar'/>
///<amd-dependency path='bar'/>
Object.defineProperty(exports, "__esModule", { value: true });
-var m1 = require("m2");
+///<amd-dependency path='bar'/>
+const m1 = require("m2");
m1.f();
2 changes: 1 addition & 1 deletion testdata/baselines/reference/submodule/compiler/amdDependencyCommentName1.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ m1.f();

//// [amdDependencyCommentName1.js]
"use strict";
Object.defineProperty(exports, "__esModule", { value: true });
///<amd-dependency path='bar' name='b'/>
Object.defineProperty(exports, "__esModule", { value: true });
const m1 = require("m2");
m1.f();
Loading
Loading