fix(go): disambiguate union discriminant field when it collides with a member name#16320
Conversation
…a member name When a discriminated union's discriminant property name matches one of its member discriminant values (e.g. discriminant 'event' with a member also keyed 'event'), v1 emitted duplicate struct fields and getters that would not compile. The discriminant field is now given a unique collision-free name (e.g. EventType) while member fields and their public accessors are preserved unchanged.
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| taken := make(map[string]struct{}, len(union.Types)+len(union.BaseProperties)) | ||
| for _, unionType := range union.Types { | ||
| taken[goExportedFieldName(unionType.DiscriminantValue.Name.PascalCase.UnsafeName)] = struct{}{} | ||
| } | ||
| for _, property := range union.BaseProperties { | ||
| taken[goExportedFieldName(property.Name.Name.PascalCase.UnsafeName)] = struct{}{} | ||
| } |
There was a problem hiding this comment.
🟡 unionDiscriminantFieldName omits union.Extends properties from collision check
The unionDiscriminantFieldName function builds its taken set from union.Types and union.BaseProperties (lines 395-400), but it does not include properties inherited via union.Extends. In VisitUnion, visitObjectProperties is called for each extended type (model.go:423-431), and it writes fields directly to the struct via t.writer.P(fieldName, " ", goType) at model.go:1353. If the discriminant's Go exported name matches an extended property's exported name, the collision goes undetected, and duplicate struct fields are emitted — the exact same compilation error this PR aims to fix.
Example scenario and relevant code paths
If a union extends an object that has a property named e.g. "event", and the discriminant is also "event", the taken map won't contain the extended property's name, so collides is false and the base name is returned unchanged. The struct then gets two Event fields. The fix should collect extended property names into the taken map, recursively resolving nested extends via t.writer.types[extend.TypeId].Shape.Object.
The getTypeFieldsForUnion function correctly processes extends at model.go:1497-1499, confirming these fields are part of the union's struct.
Prompt for agents
The unionDiscriminantFieldName function at generators/go/internal/generator/model.go:392-415 needs to also collect field names from union.Extends into the taken map. The union.Extends field contains DeclaredTypeName references whose TypeId can be used to look up the object type declaration. However, this function is a standalone function (not a method on typeVisitor), so it doesn't have access to the types map. You need to either:
1. Add a types parameter (map[common.TypeId]*ir.TypeDeclaration) to unionDiscriminantFieldName, then recursively walk each extended object's properties (and their extends) collecting goExportedFieldName values into taken. This mirrors the recursive approach used in visitObjectProperties at model.go:1321-1326.
2. Or convert unionDiscriminantFieldName to a method on typeVisitor so it can access t.writer.types directly.
The recursive collection should mirror what visitObjectProperties does: for each extend, resolve the object via types[extend.TypeId].Shape.Object, iterate its Properties collecting goExportedFieldName(property.Name.Name.PascalCase.UnsafeName), and recurse into that object's own Extends.
All call sites (model.go:419 and model.go:1490) would need to be updated to pass the types map if using approach 1.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Good catch — fixed in f857d6e. unionDiscriminantFieldName is now a typeVisitor method and recursively collects exported field names from union.Extends (and nested extends) into the taken set, with nil/cycle guards, so a discriminant colliding with an inherited property is now disambiguated too. The server-sent-event-examples fixture output is unchanged (no extends collision there), confirming this is purely additive coverage.
…heck Make unionDiscriminantFieldName a typeVisitor method so it can also account for properties inherited via union.Extends (recursively, with nil/cycle guards) when disambiguating the discriminant field name. Previously only member and base-property names were checked, so a discriminant colliding with an extended property could still emit duplicate struct fields.
SDK Generation Benchmark ResultsComparing PR branch against median of 5 nightly run(s) on Full benchmark table (click to expand)
main (generator): generator-only time via --skip-scripts (includes Docker image build, container startup, IR parsing, and code generation — this is the same Docker-based flow customers use via |
…ed.yml allowedFailures conflict)
2 participants
Description
Fixes the
server-sent-event-examples:with-wire-testsgo-sdk seed fixture by resolving a duplicate identifier bug in v1's discriminated union generation.Changes Made
unionDiscriminantFieldName()helper ingenerators/go/internal/generator/model.gothat detects when the discriminant's exported field name collides with a union member field name and returns a collision-free name (e.g.EventTypeinstead ofEvent)VisitUnionstruct emission and thegetTypeFieldsForUniongetter generationserver-sent-event-examples:with-wire-testsfromallowedFailuresinseed/go-sdk/seed.ymlRoot cause:
StreamEventContextProtocolhas discriminantvalue: eventand a union member also keyedevent(→EventEvent). Both map to Go field nameEvent, producing duplicate struct fields and duplicate getter methods with conflicting return types.Fix: The discriminant field is renamed to
EventType(with a loop for further disambiguation if needed). Member fields and their public accessors (GetEvent(),NewStreamEventContextProtocolFromEvent()) are preserved. The JSON wire tag still uses the original wire value"event".Testing
pnpm seed test --generator go-sdk --fixture server-sent-event-examples --local --skip-scriptspasses 1/1go build ./...passes on v1completions.gocompiles with no duplicate identifiersEventType string, getterGetEventType() stringfor discriminant;Event *EventEvent, getterGetEvent() *EventEventfor member)Link to Devin session: https://app.devin.ai/sessions/6ba63e4405694aa8ab8875f0dbfd9afc
Requested by: @iamnamananand996