Fix two union serialization defects: nullable case overload merging (#128688) and default(struct union) crash (#128834)#128900
Open
eiriktsarpalis wants to merge 4 commits into
Conversation
When a union declares both `Foo(T)` and `Foo(T?)` constructor overloads for a value type `T`, the metadata layer previously merged them into a single nullable case. This caused the deconstructor's switch arms to collapse to one `Type? => caseType` arm, which then incorrectly returned `typeof(T?)` when the value was a non-null `T`, and produced `UnionDoesNotAcceptNull` at runtime when serializing a non-nullable union case constructed with null. Keep both overloads as separate `UnionCaseSpec` entries throughout the parser and model layers. Introduce `IsSwitchArm` to mark exactly one entry per `(CaseType)` group as the canonical switch arm so the emitter doesn't generate duplicate `case typeof(T)` labels (which would be a CS0152 error). The reflection deconstructor uses the same most-derived-first dispatch. Add a value-based null fast-path in both the reflection deconstructor and the source-generated emitter: when the union's `Value` is null, dispatch through the nullable case if one exists, otherwise the caller decides (the deconstructor returns `(null, null)` and the converter writes JSON null). Fixes dotnet#128688 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
A `default(struct union)` has no selected case and its `Value` returns null. Previously the deconstructor threw `UnionDoesNotAcceptNull` in that state, so `JsonSerializer.Serialize(new Container())` crashed at the union property even when the container was newly constructed and the union was untouched. The C# compiler's CS8655 already treats this null state as part of every union's domain — a `value switch` on any union requires a `null` arm. Align serialization with that contract: * The reflection deconstructor returns `(null, null)` when `Value` is null and no nullable case exists (and dispatches through the nullable case when one is present). * The source-generated emitter always emits a `null =>` switch arm: a typed arm when a nullable case exists, or `null => ((global::System.Type?)null, (object?)null)` otherwise. * `JsonUnionConverter` already writes JSON null when the deconstructor reports `(null, null)`. For symmetry on the read side, `JsonUnionConverter.OnTryRead` now produces `default(TUnion)` instead of throwing when JSON null arrives for a union with no nullable case. This makes `default(union) → "null" → default(union)` a full round-trip. The constructor-delegate contract is unchanged — callers that invoke the delegate directly with `null` on a non-nullable union still see `UnionDoesNotAcceptNull`. Add round-trip tests for `NoNullableCaseUnion`, its container, and a new `IntOrBool` (`union IntOrBool(int, bool);`) that mirrors the exact repro shape from the issue. Fixes dotnet#128834 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
eiriktsarpalis
requested review from
DeagleGross,
PranavSenthilnathan and
jeffhandley
Contributor
|
Tagging subscribers to this area: @dotnet/area-system-text-json |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates System.Text.Json union serialization to (1) preserve distinct union cases for Foo(T) vs Foo(T?) constructor overloads, and (2) make default(struct union) / null payload states serialize as JSON null and round-trip without throwing.
Changes:
- Runtime metadata: stop merging
T+Nullable<T>cases; adjust union deconstructor/constructor null handling; treat JSONnullasdefault(TUnion)when no nullable case exists. - Source generator: retain both declared cases, introduce
IsSwitchArmto avoid duplicate/illegal pattern arms, and always emit anull =>arm in generated deconstructors. - Tests: expand union test coverage for default-union serialization, null routing, and distinct-case metadata expectations; register new test unions/containers in the source-gen contexts.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/libraries/System.Text.Json/tests/System.Text.Json.SourceGeneration.Tests/Serialization/UnionTests.cs | Registers newly added union/container types in source-gen contexts to exercise the updated behaviors. |
| src/libraries/System.Text.Json/tests/Common/UnionTests.cs | Adds/updates regression tests for default(union) serialization, null routing, and distinct T vs T? case metadata. |
| src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/DefaultJsonTypeInfoResolver.Union.cs | Updates reflection-based union metadata discovery and deconstructor/constructor null semantics. |
| src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Union/JsonUnionConverter.cs | Changes JSON null read behavior to return default(TUnion) when no nullable union case exists. |
| src/libraries/System.Text.Json/gen/Model/UnionCaseSpec.cs | Extends the generator’s union case model with IsSwitchArm to represent canonical pattern arms. |
| src/libraries/System.Text.Json/gen/JsonSourceGenerator.Parser.cs | Stops normalizing Nullable<T> to T for union cases; computes canonical arm roles for pattern collisions. |
| src/libraries/System.Text.Json/gen/JsonSourceGenerator.Emitter.cs | Updates generated union constructor/deconstructor emission to avoid duplicate/illegal pattern arms and always include null handling. |
eiriktsarpalis
force-pushed
the
unions/no-merge-nullable-ctor-overloads
branch
from
2b705fa to
2ea38bb
Compare
Replaces the emitter-side `GetPatternTypeFullyQualifiedName` helper, which stripped a trailing `?` from the fully-qualified name to recover the underlying type of `Nullable<T>` union cases. That manipulation assumed a specific spelling of the FQN and was unsafe in principle. The parser now records a `PatternType` `TypeRef` on `UnionCaseSpec` computed directly from the symbol: for `Nullable<T>` we use the type argument's symbol; otherwise we reuse `CaseType`. The emitter just reads `caseSpec.PatternType.FullyQualifiedName` at both former call sites. Snapshot output is byte-identical, all union tests (reflection + source-gen) pass with unchanged counts. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The reflection resolver captured `nullableCase` by scanning the topologically sorted (most-derived-first) `orderedCases` list, but `JsonTypeInfo.UnionNullableCaseType` is cached by scanning the public `UnionCases` list in declaration order. When a union has multiple nullable cases in an inheritance relationship, those two scans diverge: `JsonUnionConverter` would advertise the declaration-first nullable case as the canonical null case, while the captured `nullableCase` (used by the constructor delegate's null fast-path and the deconstructor's null arm) would silently dispatch JSON null through the most-derived nullable case instead. Scan `_caseEntries` instead so the delegate dispatch and the public metadata agree. Most-derived dispatch for non-null values is unaffected and continues to use `orderedCases`. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
DeagleGross
approved these changes
Jun 2, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Note
This PR was prepared with assistance from GitHub Copilot.
What this PR fixes
Two related defects in
System.Text.Jsonunion serialization. Each is in its own commit.Commit 1 — Fixes #128688
When a union declares both
Foo(T)andFoo(T?)constructor overloads for a value typeT, the metadata layer used to merge them into a single nullable case. The deconstructor's switch arms collapsed to oneType? => caseTypearm and, as a result:new Foo((int)42)deconstructed totypeof(int?)instead oftypeof(int);new Foo((int?)null)threwUnionDoesNotAcceptNulleven though the nullable overload was clearly the right case.The fix keeps both overloads as separate
UnionCaseSpecentries through the parser and model layers, and introducesIsSwitchArmto mark exactly one entry perCaseTypegroup as the canonical switch arm (so the emitter doesn't generate duplicatecase typeof(T)labels — CS0152). A value-based null fast-path was added in both the reflection deconstructor and the source-generated emitter so null payloads dispatch through the nullable case when one exists.This supersedes #128689; the supplementary PR-by-comment review there pointed at this root cause.
Commit 2 — Fixes #128834
default(struct union)has no selected case andValuereturns null. Previously the deconstructor threwUnionDoesNotAcceptNullin that state, so\\\csharp public union BoolOrInt(bool, int); public class Container { public BoolOrInt Foo { get; init; } } JsonSerializer.Serialize(new Container()); // crash\\crashed at the union property even when the container was newly constructed. The C# compiler's CS8655 already treats this null state as part of every union's domain (a
value switchon any union requires anullarm), so the deconstructor now matches that contract:(null, null)whenValueis null and no nullable case exists.null =>arm (typed when a nullable case exists, untyped((Type?)null, (object?)null)otherwise).JsonUnionConverter.OnTryReadproducesdefault(TUnion)for JSON null when no nullable case exists, makingdefault(union) → "null" → default(union)a complete round-trip.The constructor-delegate contract is unchanged — calling
typeInfo.UnionConstructor!(typeof(int), null)on a non-nullable union still throwsUnionDoesNotAcceptNull(pinned byConstructor_NullValue_NoNullableCase_Throws).Tests
UnionTests.cs— addedNoNullableCaseUnionContainer,IntOrBool(matching the exact shape from the STJ: union vs. class asymmetry — missing JSON silently produces default(union) that crashes on serialize #128834 repro), andIntOrBoolContainer, plus 8 new tests covering deconstructor, serialize, and full round-trip for both shapes.Deconstructor_UnionWithNullValue_NoNullableCase_Throwstest to assert the new_ReturnsNullPaircontract.UnionTestsContext_MetadataandUnionTestsContext_Default).All four
System.Text.Jsonsuites pass locally:System.Text.Json.Tests)The snapshot baseline (
StringOrIntUnion) already had anull =>arm because itsstringcase is nullable, so the always-null-arm logic doesn't alter any existing baseline — no regen needed.