Skip to content

fix(go): preserve embedded struct and shadow fields in HandleExplicitFields - #17618

Merged
adidavid014 merged 2 commits into
mainfrom
devin/1788365802-go-explicit-fields-shadow
Sep 4, 2026
Merged

fix(go): preserve embedded struct and shadow fields in HandleExplicitFields#17618
adidavid014 merged 2 commits into
mainfrom
devin/1788365802-go-explicit-fields-shadow

Conversation

@devin-ai-integration

@devin-ai-integration devin-ai-integration Bot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Description

Linear ticket: N/A

Calling any generated SetXxx setter on a Go SDK type that also has date/date-time fields silently dropped most of the JSON body (and made offset-based auto-pagination loop forever).

Generated MarshalJSON for such types builds a wrapper struct { embed; StartDate *internal.DateTime ...; EndDate ... }. HandleExplicitFields only recognized the embed pattern when NumField() == 1, so multi-field wrappers fell into the "regular struct" branch:

  1. the explicit-field bits (indexed by the embedded type's field order) were applied to the wrapper's field order (SetCount -> bit 2 -> wrapper EndDate -> "end_date":null), and
  2. the lowercase embed field was skipped as unexported, so the whole embedded payload vanished.

Fix: HandleExplicitFields now detects the embed by Field(0).Anonymous regardless of field count, indexes bits against the embedded type, and builds a flat struct where each wrapper shadow field (matched by JSON name) replaces the embedded field it shadows. omitempty is stripped from whichever field actually serializes (the shadow for date fields, the embedded field otherwise). Non-shadowing wrapper fields are appended. Fast path (explicitFields == nil || Sign() == 0) is unchanged.

before                                    after
req.SetCount(&n)      -> {"end_date":null}   {"count":7}
req.SetOffset(&n)     -> {}                  {"offset":7}
req.SetStartDate(nil) -> {}                  {"start_date":null}

Changes Made

  • generators/go/internal/generator/sdk/internal/explicit_fields.go (embedded runtime template) — fix above; extracted jsonFieldName / removeOmitEmpty helpers
  • generators/go/internal/generator/sdk/internal/explicit_fields_test.go — new TestHandleExplicitFieldsShadowedWrapper: multi-field wrapper with non-date field set ({"count":7}), date field explicitly nil ("start_date":null), bit index beyond wrapper field count (SetOffset), mixed shadowed/non-shadowed. Existing 1-field-wrapper tests act as the regression guard.
  • generators/go/internal/fern/ir/internal/explicit_fields.go — same file (kept in sync)
  • seed/go-sdk/**/internal/explicit_fields{,_test}.go — snapshots updated (183 fixtures)
  • generators/go/sdk/changes/unreleased/fix-explicit-fields-date-wrapper.yml — changelog entry
  • Updated README.md generator (if applicable)

Testing

  • Unit tests added/updated — go test ./... in generators/go passes; go test ./internal/ in seed/go-sdk/allof passes
  • Manual testing completed — dependency-free repro from the report run against old vs new file (output above)

Link to Devin session: https://app.devin.ai/sessions/c429338faac64d5ba771101848dd1fbf
Open in Devin Desktop: https://app.devin.ai/desktop/session/c429338faac64d5ba771101848dd1fbf?variant=devin


Devin Review

…Fields

Co-Authored-By: bot_apk <apk@cognition.ai>
@devin-ai-integration

Copy link
Copy Markdown
Contributor Author

🤖 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

@nitpickybot nitpickybot Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

AI Review Summary

The fix correctly generalizes the embed-pattern detection and indexes explicit-field bits against the embedded type. Main concerns: the explicit-field bit index no longer matches the embedded struct's field ordinal once unexported/explicitFields fields appear before other fields (pre-existing but now more visible), and the shadow lookup is O(n·m) with a closure that could be a map. Also worth noting the -/json:"-" name collision in jsonFieldName could cause an unintended shadow match.

  • 🟡 2 warning(s)
  • 🔵 1 suggestion(s)

To request another review, comment /ai-review on this pull request.

Comment thread generators/go/internal/generator/sdk/internal/explicit_fields.go
Comment thread generators/go/internal/generator/sdk/internal/explicit_fields.go Outdated
Comment thread generators/go/internal/generator/sdk/internal/explicit_fields.go Outdated
@github-actions

github-actions Bot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

SDK Generation Benchmark Results

Comparing PR branch against median of 5 nightly run(s) on main (latest: 2026-09-04T04:06:24Z).

Full benchmark table (click to expand)
Generator Spec main (generator) main (E2E) PR (generator) Delta
go-sdk square 149s (n=5) 311s (n=5) 127s -22s (-14.8%)

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-09-04T04:06:24Z). Trigger benchmark-baseline to refresh.
Last updated: 2026-09-04 19:13 UTC

…:"-" shadows; map-based shadow lookup

Co-Authored-By: bot_apk <apk@cognition.ai>
@adidavid014
adidavid014 merged commit 97cc02a into main Sep 4, 2026
77 checks passed
@adidavid014
adidavid014 deleted the devin/1788365802-go-explicit-fields-shadow branch September 4, 2026 19:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants