Repository navigation
generate: make union emission independent of map order - #78
Conversation
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Codex Review: Didn't find any major issues. Another round soon, please! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@codex security review |
🛡️ Codex Security ReviewSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep it up! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@codex security review |
🛡️ Codex Security ReviewSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
Two latent defects in cmd/generate/internal/emit. No variant in the
current schema triggers either one, and the regenerated *_gen.go files
are byte-identical.
- emitUnion and WriteHelpersJen picked the fallback discriminator as the
first const property found by ranging over a map, and emitUnion
collected the const injections for MarshalJSON the same way. A
variant with two or more const properties could produce different
output on each run. Both now scan properties in sorted key order
(firstConstPropertyKey, sortedPropertyKeys).
- emitUnion skips a phantom variant (no $ref, no properties, no null
type, no title), but WriteHelpersJen did not, so it emitted a
constructor for a type that does not exist. Both now use
isPhantomVariant, and WriteHelpersJen skips at the same point as
emitUnion so later variant names stay in step.
New tests in union_test.go failed before this change ("types_gen.go
differs between runs", "helpers_gen.go builds undeclared type
PickVariant2") and pass now, 10 of 10 runs.
---
_Generated with [`xum`](https://github.com/coder/xum) • Model: `anthropic:claude-opus-5-5` • Thinking: `xhigh`_
WriteHelpersJen repeated emitUnion's variant naming, but it seeded
usedTypeNames with schema definition names only. WriteTypesJen also
registers names for inline object properties before it names union
variants, so a variant whose preferred name was already taken got a
fallback name in types_gen.go while helpers_gen.go kept the preferred
name: the constructor built the wrong or a missing type and the package
did not compile.
emitUnion now records each oneOf variant it emits (field, type,
discriminator, required keys, properties), and WriteHelpersJen builds
the types once and creates constructors from that record. The second
copy of the naming logic is gone, so the two emitters cannot disagree.
WriteTypesJen keeps its signature and now renders buildTypesJen's file.
TestWriteHelpersJen_UsesUnionVariantTypes failed before this change
("helpers_gen.go sets PickRequest.Item to &PickItem{...}, but the field
has type *PickRequestItem") and passes now. No variant in the current
schema collides, and the regenerated *_gen.go files are byte-identical.
Found by Codex review on #79.
_Generated with [`xum`](https://github.com/coder/xum) • Model: `anthropic:claude-opus-5-5` • Thinking: `xhigh`_
f93847e to
fb4c16d
Compare
Fixes three latent defects in
cmd/generate/internal/emit. No variant in the current schema triggers either one, and regenerating leaves every*_gen.gofile byte-identical.emitUnionandWriteHelpersJentook the fallback discriminator as the firstconstproperty found by ranging over a map, andemitUnioncollected theconstinjections forMarshalJSONthe same way. A variant with two or moreconstproperties could produce different output on each run. Both now scan properties in sorted key order (sortedPropertyKeys,firstConstPropertyKey).emitUnionemits no type for a variant with no$ref, no properties, nonulltype, and no title, butWriteHelpersJenstill emitted a constructor for it, which referenced a type that does not exist. Both now useisPhantomVariant.WriteHelpersJenrepeatedemitUnion's variant naming, but it never saw the names thatWriteTypesJenregisters for inline object properties first. A variant whose preferred name was taken got a fallback name intypes_gen.go, whilehelpers_gen.gokept the preferred name, so the package did not compile.emitUnionnow records eachoneOfvariant it emits, andWriteHelpersJenbuilds the types once (buildTypesJen) and creates its constructors from that record. The second copy of the naming logic is gone.Review focus:
buildTypesJenintypes.goand the shorterWriteHelpersJeninhelpers.go. Helpers now trustemitUnion's record (field, type, discriminator, required keys, properties), so check that the record holds what the old helper code computed. The byte-identical regeneration says it does for the current schema.Evidence
union_test.gofailed before the fix (types_gen.go differs between runs,helpers_gen.go builds undeclared type PickVariant2, andhelpers_gen.go sets PickRequest.Item to &PickItem{...}, but the field has type *PickRequestItem) and pass after it, 10 of 10 runs with-count=10. The third defect was found by Codex review on docs(generate): add EGM contracts for cmd and cmd/generate #79.cd cmd/generate && go test ./...passes.cd cmd/generate && env -u GOPATH -u GOMODCACHE go run .leavesgit diff -- '*_gen.go'empty.Second PR of a 4-PR stack. It touches only
cmd/generate/internal/emitand does not depend on the examples change below it.Generated with
xum• Model:anthropic:claude-opus-5-5• Thinking:xhigh