Skip to content

fix(go): dedupe union base properties that collide with extended properties#16276

Open
iamnamananand996 wants to merge 2 commits into
mainfrom
devin/1780667523-go-property-access-union-dedup
Open

fix(go): dedupe union base properties that collide with extended properties#16276
iamnamananand996 wants to merge 2 commits into
mainfrom
devin/1780667523-go-property-access-union-dedup

Conversation

@iamnamananand996
Copy link
Copy Markdown
Contributor

Description

Second in the one-by-one series to clear seed/go-sdk/seed.yml allowedFailures. Fixes the property-access fixture.

A discriminated union that both extends an object and declares base-properties with an overlapping name produced duplicate Go fields. In the fixture, UserOrAdminDiscriminated does:

UserOrAdminDiscriminated:
  base-properties:
    normal: string   # collides with Foo.normal below
    foo: Foo
  extends: Foo        # Foo has: normal, read, write
  union: { user: User, admin: Admin, empty: {} }

Both the extended Foo.normal and the base-property normal mapped to a Normal field, so the v1 model generator emitted Normal twice → types.go:488: Normal redeclared, duplicate GetNormal, and duplicate struct-literal keys. The generated SDK did not compile.

Changes Made

  • generators/go/internal/generator/model.go: added unionExtendedPropertyNames() helper that collects (transitively) the exported field names contributed by a union's extends. In VisitUnion (struct def, UnmarshalJSON, MarshalJSON) and getTypeFieldsForUnion (getters), base properties whose exported name is already contributed by an extended object are skipped — the extended property already covers them.
  • seed/go-sdk/seed.yml: removed property-access from allowedFailures.
  • Regenerated seed/go-sdk/property-access/ (types.go, types_test.go): single Normal field/getter now.
  • Added changelog generators/go/sdk/changes/unreleased/fix-union-base-property-collision.yml (type: fix).
  • Updated README.md generator (if applicable) — n/a

Testing

  • pnpm seed test --generator go-sdk --fixture property-access --local now passes (was a build failure: Normal redeclared).
  • Full local go-sdk seed suite: only property-access output changed; no other fixture's generated code changed (verified via git status). Remaining failures are the still-listed allowedFailures plus known flaky WireMock runtime tests that generate byte-identical code to main.
  • v1 generator go build ./..., go vet, go test ./internal/generator/..., and gofmt all clean.

Link to Devin session: https://app.devin.ai/sessions/6ba63e4405694aa8ab8875f0dbfd9afc
Requested by: @iamnamananand996

…erties

A discriminated union that both extends an object and declares base-properties with an overlapping name (e.g. property-access fixture: extends Foo and base-property normal, both yielding a Normal field) emitted duplicate struct fields, getters, and marshaler entries, so the generated Go did not compile. Skip base properties whose exported name is already contributed by an extended object.
@devin-ai-integration
Copy link
Copy Markdown
Contributor

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

  • Address comments on this PR. Add '(aside)' to your comment to have me ignore it.
  • Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

  • Disable automatic comment, CI, and merge conflict monitoring

@iamnamananand996 iamnamananand996 marked this pull request as ready for review June 5, 2026 17:18
Copy link
Copy Markdown

@claude claude Bot left a comment

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor

@devin-ai-integration devin-ai-integration Bot left a comment

Choose a reason for hiding this comment

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

✅ Devin Review: No Issues Found

Devin Review analyzed this PR and found no potential bugs to report.

View in Devin Review to see 3 additional findings.

Open in Devin Review

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Jun 5, 2026

SDK Generation Benchmark Results

Comparing PR branch against median of 5 nightly run(s) on main (latest: 2026-06-05T05:30:09Z).

Full benchmark table (click to expand)
Generator Spec main (generator) main (E2E) PR (generator) Delta
go-sdk square 139s (n=5) 283s (n=5) 128s -11s (-7.9%)

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 fern generate). main (E2E): full customer-observable time including build/test scripts (nightly baseline, informational). Delta is computed against generator-only baseline.
⚠️ = generation exited with a non-zero exit code (timing may not reflect a successful run).
Baseline from nightly runs on main (latest: 2026-06-05T05:30:09Z). Trigger benchmark-baseline to refresh.
Last updated: 2026-06-05 20:26 UTC

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

1 participant