fix(rust): preserve inherited fields in union variants - #17297
fix(rust): preserve inherited fields in union variants#17297fern-api[bot] wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
AI Review Summary
Small, well-targeted fix: object types declaring extends are excluded from union-variant inlining, so inherited fields stop vanishing. The guard, fixture, snapshots, and seed regen all line up. Only minor nits around test assertions and changelog framing.
- 🔵 2 suggestion(s)
| const plantEvent = files.find((file) => file.fileContents.includes("pub enum PlantEvent")); | ||
| expect(plantEvent?.fileContents).toContain("data: SproutedEvent,"); | ||
| expect(plantEvent?.fileContents).toContain("data: WateredEvent,"); | ||
|
|
||
| // The wrapper structs must still be generated, with their inherited fields. | ||
| const sproutedEvent = files.find((file) => file.fileContents.includes("pub struct SproutedEvent")); | ||
| expect(sproutedEvent?.fileContents).toContain("plant_event_base_fields: PlantEventBase"); |
There was a problem hiding this comment.
🔵 suggestion
files.find(...) can return undefined, and the ?. then hands undefined to toContain, producing a confusing matcher error rather than "file not generated". Assert the files exist first, and while here consider asserting the inherited field on WateredEvent too (that's the variant most likely to regress silently since it has an own property).
| const plantEvent = files.find((file) => file.fileContents.includes("pub enum PlantEvent")); | |
| expect(plantEvent?.fileContents).toContain("data: SproutedEvent,"); | |
| expect(plantEvent?.fileContents).toContain("data: WateredEvent,"); | |
| // The wrapper structs must still be generated, with their inherited fields. | |
| const sproutedEvent = files.find((file) => file.fileContents.includes("pub struct SproutedEvent")); | |
| expect(sproutedEvent?.fileContents).toContain("plant_event_base_fields: PlantEventBase"); | |
| const plantEvent = files.find((file) => file.fileContents.includes("pub enum PlantEvent")); | |
| expect(plantEvent).toBeDefined(); | |
| expect(plantEvent?.fileContents).toContain("data: SproutedEvent,"); | |
| expect(plantEvent?.fileContents).toContain("data: WateredEvent,"); | |
| // The wrapper structs must still be generated, with their inherited fields. | |
| const sproutedEvent = files.find((file) => file.fileContents.includes("pub struct SproutedEvent")); | |
| expect(sproutedEvent).toBeDefined(); | |
| expect(sproutedEvent?.fileContents).toContain("plant_event_base_fields: PlantEventBase"); | |
| const wateredEvent = files.find((file) => file.fileContents.includes("pub struct WateredEvent")); | |
| expect(wateredEvent).toBeDefined(); | |
| expect(wateredEvent?.fileContents).toContain("plant_event_base_fields: PlantEventBase"); |
| referenced object type got all of its properties from `extends`, the variant was inlined to | ||
| an empty struct, so the payload deserialized successfully but every field was discarded. | ||
| Such types are no longer inlined and keep their `#[serde(flatten)]` wrapper. | ||
| type: fix |
There was a problem hiding this comment.
🔵 suggestion
Two things:
- The seed diff shows this changes generated public API for affected unions (
FooExtended { age }→FooExtended { data: FooExtended }, andfoo_extended(age: i64)→foo_extended(data: FooExtended)). Users on 0.24.x withextends-based variants will get compile errors on upgrade — worth calling out explicitly in the summary even if the type staysfix. - The behavior change originates in
generators/rust/model. If that generator has its ownchanges/unreleaseddirectory and is published separately, it needs an entry there as well.
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 |
Description
Rust discriminated union variants silently dropped every field a variant's type inherited via
extends.Since rust
0.24.0(#13849), asamePropertiesAsObjectvariant whose referenced type is used nowhere else gets inlined into the enum variant. The inlining copiesreferencedType.shape.propertiesonly —shape.extendsis never resolved. A wrapper type whose fields all come fromextendstherefore inlines to nothing:Because the enum is internally tagged, this is worse than a compile error: the events deserialize successfully and the data disappears. Python and TypeScript generate the full field set for the same IR, so Rust was the outlier. Reported by a customer whose SSE stream produced empty
Heartbeat/Entityvariants.Fix: exclude object types that declare
extendsfrominlinedUnionVariantTypeIds. Those variants keep the pre-0.24.0 wrapper form, which preserves the whole shape:Variants whose types have no
extendsstill inline, so the improved union ergonomics from #13849 are unchanged for everything else.Changes Made
generators/rust/model/src/generateModels.ts: requireshape.extends.length === 0before treating asamePropertiesAsObjecttype as inlinable.union-typesmodel fixture with aPlantEventunion whose variants inheritoccurred_atfrom a base type (one variant with no own properties, one with an extra field), plus snapshots.seed/rust-sdk/unionsandseed/rust-sdk/unions-with-local-date:FooExtendedis no longer inlined and its wrapper struct is emitted again.fix).Testing
generators/rust/model: 17/17 pass. Reverting the one-line guard fails the new test with exactly the reported symptom:Sprouted {}andWatered { liters }(the inheritedoccurred_atis missing from both).rust-sdkseed run: 142/142 fixtures pass; the only source diffs are the twounions*fixtures above.{"event":"sprouted","occurred_at":"..."}deserializes toSprouted(field lost, re-serializes to{"event":"sprouted"}) on the old shape, and preservesoccurred_aton the new one.Note:
cargo testinsideseed/rust-sdk/unionsdoes not compile onmainfor an unrelated pre-existing reason (types_union_with_duplicative_discriminants.rshas a variant field namedtypethat collides with the serde tag), so the serde round-trip was verified in a standalone crate rather than the fixture.Independent of the OAuth header fix in #17296.