Skip to content

feat(stjm): support .NET 11 unions, numeric sources and a net11.0 target - #199

Merged
egil merged 60 commits into
mainfrom
claude/stjm-dotnet-11-upgrade-ac6918
Sep 21, 2026
Merged

egil merged 60 commits into
mainfrom
claude/stjm-dotnet-11-upgrade-ac6918

Conversation

@egil

@egil egil commented Sep 17, 2026 •

Copy link
Copy Markdown
Owner

Summary

Builds on #182 (STJM assessment of .NET 11 RC1) and turns it into shipped changes. Verified against the RC1 runtime source (v11.0.0-rc.1.26425.128); the key correction to the issue is that the new JsonTypeClassifier does not lift the [JsonPolymorphic] limitation (STJ still checks CanHaveMetadata on the classifier-resolved derived converter), whereas C# unions have no such gate.

What changed

  • net10.0;net11.0 multi-target for package, tests, samples and benchmarks; CI installs the 10.x band plus the pinned RC1 SDK and packs both assets. Global.json allows the prerelease SDK until GA (2026-11-10).
  • Numeric non-object sources (fix): a JSON number now matches the numeric types with built-in STJ converters — the TypeCode set plus an explicit allowlist of Half, Int128, UInt128 and the .NET 11 BFloat16/Decimal32/64/128 (types such as BigInteger that need a custom converter are excluded); Nullable<T> is unwrapped. Strings route to string, char, DateTime, DateTimeOffset, DateOnly, TimeOnly, TimeSpan, Guid, Uri, Version and byte[]; quoted numbers reach numeric sources under AllowReadingFromString/AllowNamedFloatingPointLiterals when no string source exists; sources with a converter override are never shape-matched. Classified once per migrator; no reflection on the read path. Ambiguity rules unchanged.
  • Clear diagnostic for non-object targets (fix): [JsonMigratable] on a union/collection/dictionary throws a guided NotSupportedException instead of STJ's "invalid operation for kind".
  • Unions of [JsonMigratable] cases on .NET 11 (feat): JsonMigratableUnionTypeClassifier routes current, migrated (static + external migrators) and UndiscriminatedSourceType payloads by discriminator with pre-encoded UTF-8 comparisons and no allocations on the hit path. AddJsonMigrationSupport() registers it in options.TypeClassifiers; source-generated contexts annotate the union with [JsonUnion(TypeClassifier = typeof(JsonMigratableUnionTypeClassifier))] (the generator otherwise rejects unions whose cases share a JSON value type). 100% branch coverage on the new code.
  • Benchmarks: new union-dispatch category; the migration classifier is faster than STJ's structural classifier at every payload size (0.74–1.02 ratio) and allocates 4× less on small payloads.
  • Tests for PascalCase, per-member [JsonNamingPolicy], type-level [JsonIgnore] on both resolvers.
  • Docs: polymorphism/union recipe, NDJSON batch recipe (SerializeAsyncEnumerable), corrected AOT recipe, README perf refresh.

Reviewer notes

  • NativeAOT is not supported and the docs now say so: a smoke app published fine but 5/9 scenarios failed because trimming removes the IMigrateFrom<,> interface implementations and parameterless constructors discovery relies on. Annotations alone cannot fix it; a trim-safe explicit registration API is the follow-up.
  • Perf tables were regenerated on WSL2 and are noisier than the previous Windows numbers (small happy-path ratio 1.63 vs 1.11 before, RatioSD 0.22); the code on that path is unchanged. Consider re-running on the usual machine before release.
  • At .NET 11 GA: switch the CI pin to 11.0.x and allowPrerelease back to false.

Verification

  • dotnet test Egil.SystemTextJson.Migration.slnx -c Release: net10 211 + 44 samples, net11 265 + 49 samples, all passing, warning-free.
  • dotnet pack produces lib/net10.0 and lib/net11.0.
  • dotnet mdsnippets + git diff --exit-code -- '*.md' clean.

Closes #182.

🤖 Generated with Claude Code

egil and others added 8 commits September 17, 2026 14:15
The package, tests, samples and benchmarks now multi-target net10.0 and
net11.0 so every scenario runs on both runtimes. CI installs the 10.x SDK
band plus the pinned .NET 11 RC1 SDK and packs both assets; Global.json
allows the prerelease SDK until .NET 11 GA.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Non-object payload matching previously classified a JSON number source by
a fixed TypeCode list, so migrators whose source type is Half, Int128,
UInt128, a Nullable<T> numeric, or one of the .NET 11 numerics
(BFloat16, Decimal32, Decimal64, Decimal128) were never selected and the
raw number fell through to target deserialization, which failed. Sources
are now classified once per migrator by the JSON token family STJ reads
them from: the existing TypeCode set plus any type implementing
INumberBase<TSelf>, with Nullable<T> unwrapped and char treated as a
string. Collection element disambiguation uses the same classification,
and the read path no longer calls Type.GetTypeCode per candidate.
Ambiguity rules are unchanged: two numeric sources for one target still
throw a JsonException.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Applying [JsonMigratable] to a type whose JSON contract is not an object
(a collection, a dictionary, or a C# union on .NET 11) used to fail deep
inside System.Text.Json with "Invalid JsonTypeInfo operation for
JsonTypeInfoKind", because the discriminator property can only be added
to object contracts. The converter factory now throws a
NotSupportedException that names the type and its contract kind and
explains where to put the attribute instead.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
C# unions (.NET 11) whose cases are [JsonMigratable] types are now
classified by migration type discriminators. Current payloads, payloads
of every source type that migrates into a case (static IMigrateFrom
contracts and registered external migrators), and payloads of a case's
UndiscriminatedSourceType are routed to that case, whose converter then
performs the migration as usual; the case's own discriminator is what is
written on serialization. Objects without a leading discriminator go to
the single non-migratable object case, or to a lone migratable case
under legacy-payload semantics; arrays, strings, numbers and booleans go
to the single case of that JSON shape. Ambiguous or unmatched payloads
throw a JsonException that lists the known discriminators.

AddJsonMigrationSupport() registers the classifier in
JsonSerializerOptions.TypeClassifiers, so reflection-based serialization
needs no extra configuration. Source-generated contexts must annotate
the union with
[JsonUnion(TypeClassifier = typeof(JsonMigratableUnionTypeClassifier))]
because the generator rejects unions whose cases share a JSON value
type unless a classifier is declared; the same public type serves both
routes. [JsonPolymorphic] hierarchies remain unsupported because
System.Text.Json still requires derived converters to support its
metadata protocol.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
… [skip notes]

Adds a UnionDispatch category (net11.0 only) comparing System.Text.Json's
structural union classifier against the migration classifier for current
and migrated payloads, and refreshes the generated benchmark reports from
a .NET 11 RC1 run.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Pins that the injected discriminator, round trips and nested migration
survive the PascalCase naming policy, per-member [JsonNamingPolicy]
overrides, explicit [JsonPropertyName] precedence and type-level
[JsonIgnore] defaults with both the reflection and source-generated
resolvers.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Shows how to migrate newline-delimited JSON streams on .NET 11 with
DeserializeAsyncEnumerable and the new SerializeAsyncEnumerable, so a
batch is migrated record by record and written back in the current
format without buffering, including how failures and cancellation
surface.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…d AOT

The polymorphism recipe now explains that the .NET 11 JsonTypeClassifier
does not lift the [JsonPolymorphic] limitation (derived converters still
need System.Text.Json's internal metadata support) and documents the
supported alternative: a C# union of [JsonMigratable] cases, with the
classification rules and a source-generation example. The AOT recipe
replaces the inaccurate "no reflection" note with what discovery does at
runtime and states that trimmed/NativeAOT publishing is not supported
yet. The README performance table is refreshed from a .NET 11 RC1 run
and gains the union dispatch scenario.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Copilot AI lite review requested due to automatic review settings September 17, 2026 15:06

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

The critical union-routing fallback issue remains unresolved.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Adds .NET 11 support to STJM, including migration-aware unions, expanded numeric source matching, diagnostics, tests, documentation, benchmarks, and CI updates.

Changes:

  • Multi-targets packages, tests, samples, and benchmarks for .NET 10/.NET 11.
  • Adds numeric migration support and union classification.
  • Updates samples, recipes, performance reports, and AOT guidance.
  • Adds targeted diagnostics and expanded test coverage.

Review findings:

  • Critical: UnionCaseRouting.cs can misroute discriminator-less nested-union objects and silently drop data.
  • Nit: AOT documentation should qualify its claim that read/write paths do not use reflection.
File summaries
File Reviewed change
Egil.SystemTextJson.Migration/test/Egil.SystemTextJson.Migration.Tests/UnsupportedTargetKindTests.cs Unsupported target diagnostics tests
Egil.SystemTextJson.Migration/test/Egil.SystemTextJson.Migration.Tests/UnsupportedTargetKindNet11Tests.cs .NET 11 union target diagnostic tests
Egil.SystemTextJson.Migration/test/Egil.SystemTextJson.Migration.Tests/UnionMigrationNet11Tests.cs .NET 11 union behavior tests
Egil.SystemTextJson.Migration/test/Egil.SystemTextJson.Migration.Tests/NumericSourceMigrationTests.cs Numeric migration tests
Egil.SystemTextJson.Migration/test/Egil.SystemTextJson.Migration.Tests/NumericSourceMigrationNet11Tests.cs .NET 11 numeric migration tests
Egil.SystemTextJson.Migration/test/Egil.SystemTextJson.Migration.Tests/JsonSerializerOptionsCombinationsTests.cs Shared serializer options tests
Egil.SystemTextJson.Migration/test/Egil.SystemTextJson.Migration.Tests/JsonSerializerOptionsCombinationsNet11Tests.cs .NET 11 serializer options tests
Egil.SystemTextJson.Migration/test/Egil.SystemTextJson.Migration.Tests/Egil.SystemTextJson.Migration.Tests.csproj Test multi-targeting
Egil.SystemTextJson.Migration/src/Egil.SystemTextJson.Migration/Migrations/UnionCaseRouting.cs Union routing implementation
Egil.SystemTextJson.Migration/src/Egil.SystemTextJson.Migration/Migrations/StaticMigratorContracts.cs Static migrator contract discovery
Egil.SystemTextJson.Migration/src/Egil.SystemTextJson.Migration/Migrations/SourceValueShape.cs Source token-shape classification
Egil.SystemTextJson.Migration/src/Egil.SystemTextJson.Migration/Migrations/MigratorReference.cs Cached source metadata
Egil.SystemTextJson.Migration/src/Egil.SystemTextJson.Migration/Migrations/JsonMigratableTypes.cs Migratable type helpers
Egil.SystemTextJson.Migration/src/Egil.SystemTextJson.Migration/Migrations/JsonMigratableTargetKindNotSupportedException.cs Guided target diagnostics
Egil.SystemTextJson.Migration/src/Egil.SystemTextJson.Migration/Migrations/JsonMigratableConverterFactory.cs Target validation and migrator discovery
Egil.SystemTextJson.Migration/src/Egil.SystemTextJson.Migration/Migrations/JsonMigratableConverter.NonObjectMatching.cs Non-object matching integration
Egil.SystemTextJson.Migration/src/Egil.SystemTextJson.Migration/JsonMigrationSerializerOptionsExtensions.cs Union classifier registration
Egil.SystemTextJson.Migration/src/Egil.SystemTextJson.Migration/JsonMigratableUnionTypeClassifier.cs Public union classifier
Egil.SystemTextJson.Migration/src/Egil.SystemTextJson.Migration/JsonMigratableAttribute.cs Updated API documentation
Egil.SystemTextJson.Migration/src/Egil.SystemTextJson.Migration/Egil.SystemTextJson.Migration.csproj Library multi-targeting
Egil.SystemTextJson.Migration/scripts/update-perf-docs.ps1 Performance report generation
Egil.SystemTextJson.Migration/samples/Egil.SystemTextJson.Migration.Samples/UnionMigrationSample.cs Union migration sample
Egil.SystemTextJson.Migration/samples/Egil.SystemTextJson.Migration.Samples/NdjsonBatchSample.cs NDJSON batch sample
Egil.SystemTextJson.Migration/samples/Egil.SystemTextJson.Migration.Samples/Egil.SystemTextJson.Migration.Samples.csproj Sample multi-targeting
Egil.SystemTextJson.Migration/README.md Feature and performance documentation
Egil.SystemTextJson.Migration/perf/Egil.SystemTextJson.Migration.PerfTests/MigrationScenarioBenchmarks.Union.cs Union benchmarks
Egil.SystemTextJson.Migration/perf/Egil.SystemTextJson.Migration.PerfTests/MigrationScenarioBenchmarks.cs Benchmark setup
Egil.SystemTextJson.Migration/perf/Egil.SystemTextJson.Migration.PerfTests/Egil.SystemTextJson.Migration.PerfTests.csproj Benchmark multi-targeting
Egil.SystemTextJson.Migration/Global.json Prerelease SDK configuration
Egil.SystemTextJson.Migration/docs/recipes/README.md Recipe index
Egil.SystemTextJson.Migration/docs/recipes/polymorphism.md Union and polymorphism guidance
Egil.SystemTextJson.Migration/docs/recipes/migration-tracking.md NDJSON guidance
Egil.SystemTextJson.Migration/docs/recipes/migration-authoring.md Numeric migration guidance
Egil.SystemTextJson.Migration/docs/recipes/aot-source-gen.md AOT and source-generation guidance
Egil.SystemTextJson.Migration/docs/perf/source-gen-benchmarks.md Source-generation benchmark updates
Egil.SystemTextJson.Migration/docs/perf/reflection-benchmarks.md Reflection benchmark updates
Egil.SystemTextJson.Migration/AGENTS.md Targeting guidance
.github/workflows/egil-systemtextjson-migration-ci.yml .NET 10/.NET 11 CI and packaging
Review details

Suppressed comments (1)

Egil.SystemTextJson.Migration/docs/recipes/aot-source-gen.md:35

  • This says the JSON read and write paths do not use reflection, but the existing collection/dictionary fallback still calls TryGetMigratableMetadata, which uses GetCustomAttribute during read-time element-discriminator matching. Please qualify this as applying to the normal/cached path (or explicitly mention fallback discovery) so the NativeAOT guidance does not make a false claim.
Migration itself is driven by `static abstract` interface methods and the type metadata your `JsonSerializerContext` provides; the JSON read and write paths do not use reflection.
  • Files reviewed: 38/38 changed files
  • Comments generated: 1
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

egil and others added 2 commits September 17, 2026 15:28
A union case that is itself a union or uses a custom converter can
accept any JSON shape, so routing a payload without a leading
discriminator to another case by shape could silently drop data the
nested case would have read. Such unions now classify discriminated
payloads only; discriminator-less objects and primitives throw a
JsonException naming the case that prevents shape-based routing.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…otes]

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings September 17, 2026 15:28

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

Unresolved critical and moderate union-routing and numeric-classification issues remain.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (3)

Egil.SystemTextJson.Migration/AGENTS.md:27

  • This file now documents a multi-targeted project, but the benchmark command above still runs dotnet run without selecting a target framework. With both net10.0 and net11.0 present, the command does not reliably select the .NET 11 benchmark target that contains the union category; update the documented command (and its README duplicate) to pass --framework net11.0.
- Language/runtime: C# on `net10.0` and `net11.0` (multi-targeted), nullable enabled. .NET 11 specific code lives under `#if NET11_0_OR_GREATER`.

Egil.SystemTextJson.Migration/docs/recipes/aot-source-gen.md:35

  • TryGetMigratableMetadata in JsonMigratableConverter.NonObjectMatching still calls GetCustomAttribute and TypeMetadata.FromType while disambiguating collection/dictionary elements, so this new claim that the library's read/write paths use no reflection after converter creation is false for that path. Narrow the statement to the paths that avoid reflection or update the implementation.
Migration itself is driven by `static abstract` interface methods and the type metadata your `JsonSerializerContext` provides; once a type's converter has been created, the library's own read and write paths do not use reflection (System.Text.Json's converter and metadata resolution behaves as it does without the library).

Egil.SystemTextJson.Migration/src/Egil.SystemTextJson.Migration/Migrations/SourceValueShape.cs:58

  • INumberBase<TSelf> is broader than “types STJ reads from a JSON number”: it also covers numeric-like types such as BigInteger/Complex that do not have the corresponding built-in number converter (and custom converters can choose a different wire shape). This helper is also used before JsonTypeInfo.Kind is checked in UnionCaseRouting, so a union containing one of those cases can route a JSON number to a converter that expects an object and fail. Base the classification on the resolved STJ converter/actual supported numeric set, and add a regression for a non-number INumberBase case.
        // Every other numeric STJ supports (Half, Int128, UInt128, BFloat16, Decimal32/64/128,
        // and future additions) has TypeCode.Object but implements INumberBase<TSelf>, so
        // the interface check keeps this list-free across runtime versions. char also
        // implements INumberBase<char> but is handled as a string above.
        return ImplementsNumberBase(type) ? SourceValueShape.Number : SourceValueShape.Unknown;
  • Files reviewed: 38/38 changed files
  • Comments generated: 2
  • Review effort level: Lite

egil and others added 3 commits September 17, 2026 15:38
…nion fallback

Non-object matching and union classification now treat DateTime,
DateTimeOffset, DateOnly, TimeOnly, TimeSpan, Guid, Uri, Version, char
and byte[] as string-shaped sources, so a JSON string routes to such a
case or migrator instead of being reported as an unknown shape. In a
union that also contains a nested union or custom-converter case, an
object without a leading discriminator is now rejected before the
UndiscriminatedSourceType fallback is applied, so the nested case can
never lose data to the configured migrator.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Disambiguating collection and dictionary migrators by the first
element's discriminator looked up the element type's [JsonMigratable]
attribute and encoded the discriminator to UTF-8 on every read. Both are
now computed when the migrator is registered, so that path performs no
reflection and allocates nothing.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…ip notes]

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings September 17, 2026 15:38

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

A critical numeric classification issue and two moderate routing and coverage issues remain unresolved.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (2)

Previously missed (1) — in code that hasn't changed since the last review.

Egil.SystemTextJson.Migration/test/Egil.SystemTextJson.Migration.Tests/NumericSourceMigrationNet11Tests.cs:18

  • The new numeric classification is intended to cover all four .NET 11 numeric primitives, but the tests exercise only Decimal64 and BFloat16 as scalar sources; Decimal32 has no coverage and Decimal128 is tested only as a collection element. Add direct scalar migration cases for both types so a runtime-specific converter or INumberBase mismatch cannot ship unnoticed.

Egil.SystemTextJson.Migration/src/Egil.SystemTextJson.Migration/Migrations/UnionCaseRouting.cs:116

  • This shape check runs before the effective STJ converter is resolved, so a scalar case with a converter override can be routed to the wrong token family. For example, an enum case configured with JsonStringEnumConverter is classified as Number; a valid string payload has no string route, while a number is sent to a converter that expects a string. Either classify from the resolved JsonTypeInfo/converter or reject shape-based fallback for overridden cases instead of accepting a configuration that cannot deserialize valid payloads.
            switch (SourceValueShapes.Classify(caseType))
            {
                case SourceValueShape.String:
                    stringCases.Add(caseType);
                    continue;
                case SourceValueShape.Number:
                    numberCases.Add(caseType);
                    continue;
                case SourceValueShape.Boolean:
                    booleanCases.Add(caseType);
                    continue;
            }
  • Files reviewed: 38/38 changed files
  • Comments generated: 1
  • Review effort level: Lite

…rters

Numeric non-object sources are now an explicit allowlist of the types
System.Text.Json reads from a JSON number out of the box (the TypeCode
numerics and enums, Half, Int128, UInt128, and on .NET 11 BFloat16,
Decimal32, Decimal64 and Decimal128). The earlier INumberBase<TSelf>
check also matched BigInteger and Complex, which have no built-in
converter, so a migrator with such a source could be selected for a
number payload and then fail inside the serializer.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings September 17, 2026 15:48
egil and others added 2 commits September 17, 2026 15:49
A union case whose type carries [JsonConverter] or matches a converter in
JsonSerializerOptions.Converters (for example an enum read with
JsonStringEnumConverter) can read a different token family than its CLR
shape suggests, so shape-based fallback could send a valid payload to a
case that rejects it. Such cases are now treated like nested unions:
payloads without a leading discriminator throw a JsonException naming
the case instead of being misrouted.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

A critical union-routing gap and moderate diagnostic and test-coverage issues remain unresolved.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (4)

Egil.SystemTextJson.Migration/docs/recipes/polymorphism.md:144

  • The documented order is incomplete: the implementation rejects all shape-based fallback first when any nested union or custom-converter case is present (unknownShapeCase), including before UndiscriminatedSourceType. This can otherwise mislead readers into expecting the documented fallback to work for those unions; describe that guard before listing the fallback order.
1. **Object payloads** are routed by their **first property**. A known discriminator selects the case it belongs to: each `[JsonMigratable]` case claims its own discriminator plus the discriminators of every source type that migrates into it (static `IMigrateFrom<,>` contracts and registered external migrators). The case's converter then performs the migration as usual, so failure handling, tracking and nested migration all apply.
2. Without a recognized leading discriminator the payload goes to, in order: the single case that declares `UndiscriminatedSourceType`; the single non-migratable case whose contract is a JSON object or dictionary; the single migratable case (legacy-payload semantics).
3. **Arrays, strings, numbers and booleans** go to the single case with that JSON shape. Numbers use the same rules as [non-object payload migration](migration-authoring.md#migrating-from-non-object-json-payloads); string-shaped cases are `string`, `char`, `DateTime`, `DateTimeOffset`, `DateOnly`, `TimeOnly`, `TimeSpan`, `Guid`, `Uri`, `Version` and `byte[]`. A case with a converter override (`[JsonConverter]` on the type or a matching entry in `options.Converters`, such as `JsonStringEnumConverter`) may read any token family, so it is treated like a nested union: only discriminated payloads are classified. If any case is a nested union or uses a custom converter, it may accept any shape, so no shape-based fallback is attempted and only discriminated payloads are classified.

Egil.SystemTextJson.Migration/src/Egil.SystemTextJson.Migration/JsonMigratableUnionTypeClassifier.cs:75

  • This diagnostic is raised while deserializing a union classifier (as exercised by the test immediately above), but it tells users to configure migration support only before serializing. That wording omits the failing operation and can send users to the wrong place; say "before using this union" or explicitly mention both serialization and deserialization.
                $"'{context.DeclaringType.FullName}' uses {nameof(JsonMigratableUnionTypeClassifier)}, but the serializer options have no migration support. Call options.AddJsonMigrationSupport() before serializing.");

Egil.SystemTextJson.Migration/src/Egil.SystemTextJson.Migration/Migrations/SourceValueShape.cs:75

  • The allowlist adds independent .NET 11 cases for BFloat16, Decimal32, Decimal64, and Decimal128, but the new tests exercise only BFloat16 and Decimal64 (and do not exercise UInt128). A converter/type-name mistake for either omitted decimal type would therefore pass the suite while the documentation promises support; add net11 migration coverage for each remaining allowlisted numeric type, preferably using a small theory.
            || type == typeof(System.Numerics.BFloat16)
            || type == typeof(System.Numerics.Decimal32)
            || type == typeof(System.Numerics.Decimal64)
            || type == typeof(System.Numerics.Decimal128)

Egil.SystemTextJson.Migration/src/Egil.SystemTextJson.Migration/Migrations/UnionCaseRouting.cs:212

  • This comment describes the old fallback order, but unknownShapeCase is checked before undiscriminatedCase, so a nested-union/custom-converter case causes an exception before either fallback is attempted. Please update the comment to document the guard that makes shape-based fallback unsafe.
                    if (reader.ValueTextEquals(entry.DiscriminatorUtf8))
                    {
                        return entry.CaseType;
  • Files reviewed: 38/38 changed files
  • Comments generated: 1
  • Review effort level: Lite

Copilot AI review requested due to automatic review settings September 17, 2026 15:54
Copilot AI review requested due to automatic review settings September 17, 2026 19:27

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

A critical converter-override matching issue remains unresolved, and the .NET 11 compatibility documentation needs updating.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (1)

Egil.SystemTextJson.Migration/src/Egil.SystemTextJson.Migration/Egil.SystemTextJson.Migration.csproj:4

  • Now that this project ships a net11.0 asset, Migrations/StjInternals.cs still documents ReadAsObject as targeting only .NET 10. Update that compatibility note to cover the .NET 11 runtime contract as well, so the new target is not supported by an undocumented private API assumption.
    <TargetFrameworks>net10.0;net11.0</TargetFrameworks>
  • Files reviewed: 41/41 changed files
  • Comments generated: 1
  • Review effort level: Lite

…1 [skip notes]

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings September 17, 2026 19:36

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

Unresolved critical and moderate routing issues must be addressed before approval.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (3)

Egil.SystemTextJson.Migration/src/Egil.SystemTextJson.Migration/Migrations/JsonMigratableTargetKindNotSupportedException.cs:15

  • The collection guidance says to “wrap the elements,” which suggests changing each element type; the unsupported target is the collection itself, so the actionable workaround is to wrap the collection in an object target (and the same applies to dictionaries). Please make the diagnostic unambiguous.
        "Migration targets must serialize as JSON objects because the type discriminator is written as a property. " +
        "Apply the attribute to the object types instead: for unions on .NET 11 annotate the case types and let AddJsonMigrationSupport() classify the union; " +
        "for collections wrap the elements in an object type. See https://github.com/egil/framework/blob/main/Egil.SystemTextJson.Migration/docs/recipes/polymorphism.md.")

Egil.SystemTextJson.Migration/src/Egil.SystemTextJson.Migration/Migrations/MigratorReference.cs:90

  • ElementMayAcceptAnyShape only guards explicit converter overrides and unions, but built-in object/JsonElement element converters can consume every JSON token too. With IMigrateFrom<List<object>, T> alongside IMigrateFrom<List<int>, T>, [1] is accepted by both yet MatchByPrimitiveElementType skips the List<object> route and selects List<int>, silently losing a valid migrator; mark these any-shape element types as guarded (or classify them recursively) before shape selection.
    private static bool ElementMayAcceptAnyShape(Type elementType, System.Text.Json.JsonSerializerOptions options)
    {
        if (JsonMigratableTypes.HasConverterOverride(elementType, options))
        {
            return true;
        }

#if NET11_0_OR_GREATER
        return options.GetTypeInfo(elementType).Kind is JsonTypeInfoKind.Union;

Egil.SystemTextJson.Migration/src/Egil.SystemTextJson.Migration/Migrations/UnionCaseRouting.cs:130

  • A non-migratable union case with a converter override is marked as discriminator-only here, but no discriminator entry is ever added for the case. Consequently a custom converter case can never be selected by this classifier (for example, a converter that reads { "$type": "<case full name>", ... } will be rejected as unknown/guarded). Add the case's default/registered TypeMetadata discriminator route, or reject this configuration instead of leaving the case unreachable.
  • Files reviewed: 42/42 changed files
  • Comments generated: 1
  • Review effort level: Lite

… and guard any-shape elements

Override detection now starts from the converter the resolved
JsonTypeInfo actually carries: the migration converter means no
override, any converter type outside System.Text.Json is an override,
and only for built-in converter types are options.Converters and
[JsonConverter] consulted in System.Text.Json precedence. A custom
resolver that bypasses options.Converters is therefore detected, and a
migratable collection element it serves no longer contributes a
discriminator route that would bypass migration.

Collections of object, JsonElement, JsonDocument or JsonNode elements
accept every JSON token and now take part in element disambiguation
like overridden-element collections. In union classification a
non-migratable case with a converter override is reachable through its
default discriminator when its type can carry one. The target-kind
diagnostic now tells collection users to wrap the collection in an
object type.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings September 17, 2026 19:47

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔵 Needs a closer look

Nested union discriminators are not propagated, so valid nested-union payloads can be rejected.

Review details

Suppressed comments (1)

Egil.SystemTextJson.Migration/src/Egil.SystemTextJson.Migration/Migrations/UnionCaseRouting.cs:223

  • Nested union cases are marked as unknownShapeCase here, but their classifier's discriminator table is never merged or consulted. Thus an outer union such as Outer(Inner, Plain) rejects {"$type":"inner-v1",...} when inner-v1 belongs only to Inner, even though the payload is discriminated and Inner could classify it; the comment's claim that discriminated payloads still route is false for nested-union discriminators. Recursively delegate/propagate nested routes, or explicitly reject nested unions instead of advertising this behavior.
  • Files reviewed: 42/42 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

A union case that is itself a union was reachable only by the outer
classifier's fallback, which refuses shape-based routing for such
cases, so a payload discriminated for one of the nested union's cases was
rejected. The outer classifier now registers the discriminators of the
nested union's migratable cases and their object sources (recursively)
as routes to the nested union case, after the direct cases so a direct
case keeps a discriminator both claim. The classifier also volunteers
for unions whose only migratable cases sit inside a nested union.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔵 Needs a closer look

Two unresolved critical union-routing defects remain in a broad .NET 11 change set.

Review details
  • Files reviewed: 42/42 changed files
  • Comments generated: 2
  • Review effort level: Lite

…ring strict

Discriminators forwarded from nested unions now yield only to direct
cases of the outer union; two nested unions claiming the same
discriminator fail at configuration instead of the second silently
routing to the first. Sources of nested migratable cases are forwarded
with the same rules as direct sources, so a scalar source read through
a converter override no longer advertises a discriminator the nested
classifier cannot honour.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

Unresolved critical and moderate findings remain in union-source routing and null-first collection/dictionary matching.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (1)

Egil.SystemTextJson.Migration/src/Egil.SystemTextJson.Migration/Migrations/JsonMigratableConverter.NonObjectMatching.cs:122

  • When multiple collection/dictionary candidates have a first value of null, neither the primitive nor complex shape matcher can identify a candidate, but this fallback returns the first registered migrator. That makes a valid payload such as [null] or {"a":null} registration-order dependent and can either select the wrong source or fail even when another candidate accepts null. Treat an unresolved null element as ambiguous rather than falling back to singleCandidate.
        return match ?? singleCandidate;
  • Files reviewed: 42/42 changed files
  • Comments generated: 1
  • Review effort level: Lite

…ll-first collection picks

A migrator whose source type is a union cannot be selected by the
migration converter, because a union payload never carries the source
type's discriminator; the union classifier therefore no longer forwards
such a source's case discriminators to the target case and only refuses
shape-based fallback for it. Collection and dictionary migrators whose
first value is null are now reported as ambiguous when several
candidates exist instead of falling back to registration order.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔵 Needs a closer look

The changes span .NET 11 runtime internals, union routing, and multi-targeted packaging; final human review is warranted.

Review details
  • Files reviewed: 42/42 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

@egil egil left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

I recommend addressing the four inline findings before merging, particularly the nullable collection regression that silently selects the wrong migrator.

Reviewed head 8a1eba228b440c2c04f843a12fd512c0d4806a0f against base e9bdf7b48eb7714dbacc66e41ce91b51bf9b1fa6. All 660 existing Release tests passed locally: .NET 10.0.12 had 243 library + 44 sample tests; .NET 11 RC1 had 324 library + 49 sample tests. The SDK was the CI-pinned 11.0.100-rc.1.26425.128.

The findings were reproduced separately against the PR's built Release assemblies. The collection regression was also compared with the base revision on .NET 10: the base throws for ambiguity, while this head silently selects the competing collection migrator. The other three findings reproduce on .NET 11 RC1.

egil and others added 2 commits September 21, 2026 09:37
…nions and nested overridden cases

- Nullable<T> elements and union cases of a migratable struct take T's
  discriminator, migrators and contract kind, so a collection of them is
  matched by its element discriminator instead of a competing source, and
  a union with a nullable migratable case round-trips through its
  discriminator.
- A union configured inside the converter factory's cloned options accepts
  the excluded case's plain converter as the recursion guard it is, so
  recursive models (Branch(Node[] Children) with union Node(Branch, Leaf))
  serialize and migrate nested payloads of other types.
- Nested unions forward the discriminator routes of overridden object-like
  cases, matching the direct-case rule.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…yload caveat [skip notes]

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🟡 Changes recommended

Union routing has unresolved custom scalar-converter and nullable migratable-source handling issues.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1 High severity · 1 Medium severity

Open (2)

…minator and keep numeric structs discriminator-less

- TypeMetadata.FromType resolves the attribute, discriminator and resolver
  input from the underlying type of a Nullable<T> source, so the migration
  converter and the union classifier both match such a source by T's
  discriminator.
- Union routing treats INumberBase<TSelf> types (BigInteger, Complex) as
  scalars, so an overridden numeric source never advertises a synthetic
  object discriminator.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🔵 Needs a closer look

Three unresolved moderate findings remain in the sample metadata registration and nullable-source union routing.

Review effort: Lite
Findings: None

Resolved since last review (2)

@egil egil left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Follow-up review of 6a71f2f859e74067c814269a14473f64061fd676, including the commit pushed during verification. I still recommend changes.

Verified fixes: nullable collection/dictionary element routing, nullable union-case discriminator routing, nested custom-converter discriminator forwarding, and the new nullable-source routing. The original recursive configuration exception is also gone.

Three issues remain:

  • P1: The recursive fix still silently skips a nested migration. I reopened the existing thread with a serializer-produced, uniformly old-version tree reproduction; the root migrates and the child's renamed field becomes null.
  • P2: A converter registered directly for a nullable case bypasses the migration-converter validation, so a recognized old payload can skip its migrator.
  • P2: The newest INumberBase<TSelf> exclusion removes a working discriminator route for a custom Complex converter that writes JSON objects; the same reproduction succeeds on d39e309 and fails on this head.

Validation: all 671 existing Release tests passed on this exact head (.NET 10: 246 library + 44 samples; .NET 11 RC1: 332 library + 49 samples). The additional probes reference the built Release assemblies. The current package CI is also green. These failures are outside the existing tests.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🔵 Needs a closer look

Broad .NET 11 runtime, union-routing, and multi-targeting changes merit final human review.

Review effort: Lite
Findings: None

…lable case converters, keep numeric struct routes

- A union case configured inside its own migration (the factory's cloned
  options) keeps its current-version route but maps its source
  discriminators to a refusing entry, so a nested old payload throws a
  JsonException naming the discriminator instead of being read as the
  current version. Modelling the old version with old types migrates
  every level; both are covered by tests and the recipe.
- The declared nullable case type must resolve to STJ's nullable wrapper;
  a converter registered for T? is rejected at configuration.
- Overridden numeric structs keep their discriminator route: the custom
  converter decides the JSON shape (reverts the INumberBase exclusion).

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🔵 Needs a closer look

The broad runtime-routing changes and unresolved source-generated union metadata issue require human review.

Review effort: Lite
Findings: None

@egil egil left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Follow-up review of 821cd11: all three findings from my previous round are addressed. No new actionable findings in this fix.

  • The original serializer-produced recursive tree now throws JsonException naming tree-v1 and TreeBranch, instead of silently dropping the child's data. The revised documentation accurately distinguishes this unsupported model from migration using old types throughout the old tree; the new test verifies the supported model migrates every level.
  • The converter registered specifically for the nullable union case is now rejected during configuration, preventing migration bypass. Normal nullable cases and sources continue to work.
  • The object-writing Complex converter's serialized output again migrates correctly through the union, matching direct target deserialization.

Verification: the unchanged independent reproduction probes against this commit produce the expected results, including the earlier nullable collection and nested custom-converter regressions. Release solution tests pass on net10.0 and net11.0: 674 passed, 0 failed, 0 skipped. The package CI run is also green. All review threads are currently resolved.

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.

Investigate STJM compatibility and improvement opportunities in .NET 11 RC1

2 participants