fix(golang): always emit consolidated imports regardless of leadingComments#2988
Merged
Conversation
…mments (#2670) Passing leadingComments (even []) to the quicktype-core API skipped GolangRenderer's whole single-file header block, including the consolidated top-of-file import collection, so a lazily-emitted `import "time"` for date-time fields landed mid-file, producing invalid Go. leadingComments now only replaces the default header comment, matching the pattern used by other language renderers, while imports are always consolidated at the top of the file. Co-Authored-By: gpt-5.6-sol via pi <noreply@openai.com>
Co-Authored-By: Claude <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Bug
Passing
leadingComments(even[]) to thequicktype-coreAPI'squicktype()function for Go output produced invalid, non-compiling Go code. For example, a schema with adate-timefield would emitimport "time"in the middle of the file, between two struct definitions — Go requires allimportdeclarations to precede any other top-level declarations.Root cause
In
GolangRenderer.ts,emitSourceStructure(single-file mode) andemitTopLevel(multi-file mode) both gated their entire header block — including the consolidated top-of-file import collection (emitPackageDefinitons(false, collectAllImports())) — behindthis.leadingComments === undefined. Passing anyleadingCommentsvalue (including[]) skipped that whole block, so the consolidated import block was never emitted. A later, per-class emission ofimport "time"(fortime.Timefields) is normally deduplicated against that header block viaemitLineOnce, but with no header block to dedupe against, the bareimport "time"landed wherever the class happened to be emitted in the file — i.e. mid-file.Other language renderers (
CSharpRenderer,JavaRenderer,SwiftRenderer,RustRenderer,KotlinRenderer, etc.) use a different, correct pattern:leadingComments, when set, only replaces the default generated-file header comment; it does not suppress unrelated output like import blocks.GolangRendererwas the outlier that conflated the two.Fix
emitTopLevel(multi-file) andemitSourceStructure(single-file) now always emit the import/package block;leadingComments, when provided, replaces the default header comment text instead of skipping the whole block, matching the pattern used elsewhere in the codebase.emitTopLevelnow collects and emits the actual imports needed by the top-level type (viacollectClassImports/collectUnionImports) instead of emitting no imports at all for the top-level file's own type.Test coverage
Added
LeadingCommentsGoFixture(fixture idschema-golang-leading-comments) intest/fixtures.ts, which is registered inallFixtures. SinceleadingCommentsis aquicktype-coreAPI option not exposed via the CLI, this fixture drivesquicktype-core'squicktype()/quicktypeMultiFile()directly (rather than through the CLI-based fixture path) usingtest/inputs/schema/date-time.schema, withleadingComments: []set, and verifies both single-file and multi-file Go output actually compiles and runs correctly (go test/go runvia the existing Go fixture driver).This fixture reproduces the bug on unfixed code (
imports must appear before other declarations) and passes after the fix.Verification performed locally
npm run buildpasses.npm run test:unit— 163 tests passed.FIXTURE=schema-golang-leading-comments QUICKTEST=true script/test— passes (new fixture, compiles and runs generated Go for both single- and multi-file output).FIXTURE=schema-golang QUICKTEST=true script/test— passes (no regression in existing Go schema fixtures).FIXTURE=golang QUICKTEST=true script/test— passes for the vast majority of samples; a few samples (bitcoin-block.json,getting-started.json,uuids.json) hit an unrelated, environment-level flake (go: cannot determine current directory: getwd: no such file or directory) from running manygo runprocesses across 16 parallel workers in this sandbox. Re-running each of those three samples individually withgo runsucceeded cleanly (exit 0, correct output), confirming this is sandbox/concurrency flakiness, not a code regression. CI will validate the full fixture matrix.quicktype-corepackage before and after the fix, confirming theimport "time"now lands in the consolidated top-of-file import block instead of mid-file.Fixes #2670
🤖 Generated with Claude Code