Support open generics in polymorphic serialization in System.Text.Json#127318
Support open generics in polymorphic serialization in System.Text.Json#127318
Conversation
- Add TryResolveOpenGenericDerivedType with type unification to PolymorphicTypeResolver - Resolve open generic derived types in PolymorphicTypeResolver constructor - Add new error message for unresolvable open generic derived types - Add SYSLIB1227 source generator diagnostic for invalid open generics - Update existing test to expect success for now-valid generic scenario Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/f39c123c-a576-4d8d-9da4-b02cd81b6577 Co-authored-by: eiriktsarpalis <2813363+eiriktsarpalis@users.noreply.github.com>
Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/f39c123c-a576-4d8d-9da4-b02cd81b6577 Co-authored-by: eiriktsarpalis <2813363+eiriktsarpalis@users.noreply.github.com>
…ay "open generic type definitions" instead of "generic types" Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/f39c123c-a576-4d8d-9da4-b02cd81b6577 Co-authored-by: eiriktsarpalis <2813363+eiriktsarpalis@users.noreply.github.com>
| PolymorphicGenericClass<int> value = new PolymorphicGenericClass<int>.DerivedClass(); | ||
| await Assert.ThrowsAsync<InvalidOperationException>(() => Serializer.SerializeWrapper(value)); | ||
| string json = await Serializer.SerializeWrapper(value); | ||
| Assert.Equal("{}", json); |
There was a problem hiding this comment.
Please add deserialization validation as well. Also include a few properties in both layers for good measure.
There was a problem hiding this comment.
Addressed in 3c4d937. Added BaseValue and DerivedValue properties to both layers. Deserialization validation is included in the tests that have type discriminators (e.g., OpenGenericDerivedType_WithStringDiscriminator_DeserializationWorks, OpenGenericDerivedType_ProgrammaticApi_Works). The no-discriminator PolymorphicGenericClass test only validates serialization since without a discriminator, deserialization can't distinguish the derived type.
| | __`SYSLIB1225`__ | Type includes ref like property, field or constructor parameter. | | ||
| | __`SYSLIB1226`__ | 'JsonIgnoreCondition.Always' is not valid on type-level 'JsonIgnoreAttribute' annotations. | | ||
| | __`SYSLIB1227`__ | _`SYSLIB1220`-`SYSLIB1229` reserved for System.Text.Json.SourceGeneration._ | | ||
| | __`SYSLIB1227`__ | Open generic derived type is not supported on a non-generic base type. | |
There was a problem hiding this comment.
The diagnostic description is slightly more narrow than what it is actually doing, i.e. the base type may be generic but the derived type generics are not resolvable to it.
There was a problem hiding this comment.
Updated in 3c4d937. The SYSLIB1227 diagnostic now reads: "Open generic derived type is not resolvable for the polymorphic base type" / "The type arguments of the open generic derived type must be resolvable from the base type's generic arguments." This covers both non-generic base and unresolvable type argument cases.
| var derivedType = (ITypeSymbol)attributeData.ConstructorArguments[0].Value!; | ||
| EnqueueType(derivedType, typeToGenerate.Mode); | ||
|
|
||
| // Open generic derived types (e.g. typeof(Derived<>)) are resolved at runtime |
There was a problem hiding this comment.
We should NEVER do any open generic resolution at runtime, since that is typically not supported in native AOT. It is the source generator's job to do this, and continue type graph traversal in the reconstituted specialization.
There was a problem hiding this comment.
Addressed in 3c4d937. The source generator now resolves open generics at compile time using Roslyn's type system and emits the resolved closed types directly. The parser uses TryResolveOpenGenericDerivedType (Roslyn-based type unification) and the emitter generates PolymorphismOptions.DerivedTypes.Add(new JsonDerivedType(typeof(Resolved<int>), "disc")) after CreateObjectInfo. No runtime resolution occurs in the source-gen path.
…dd source generator compile-time resolution - Revert PolymorphicTypeResolver to original (no open generic handling) - Move open generic resolution to DefaultJsonTypeInfoResolver.Helpers (reflection-only) - Add TryResolveOpenGenericDerivedType with Roslyn type unification in source generator Parser - Source generator emits resolved derived types directly into PolymorphismOptions - Add PolymorphicDerivedTypeSpec model record for source generator - CreateFromAttributeDeclarations skips open generic derived types (null-safe) - Update diagnostic description to cover broader unresolvable cases - Fix tests: remove deserialization from no-discriminator test, use closed types in programmatic API test Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/ad0de94b-b94b-48dd-8df5-4159bc77a3c0 Co-authored-by: eiriktsarpalis <2813363+eiriktsarpalis@users.noreply.github.com>
| } | ||
| } | ||
|
|
||
| [UnconditionalSuppressMessage("ReflectionAnalysis", "IL2055:UnrecognizedReflectionPattern", |
There was a problem hiding this comment.
@copilot These suppressions are invalid. Propagate the requirement using RequiresUnreferencedCode and RequiresDynamicCode attributes instead
There was a problem hiding this comment.
Addressed in ce67a92. Removed the three UnconditionalSuppressMessage attributes and added [RequiresUnreferencedCode] and [RequiresDynamicCode] to both TryResolveOpenGenericDerivedType and its caller ResolveOpenGenericDerivedTypes. The caller CreateTypeInfoCore already has both attributes so the chain is fully propagated.
There was a problem hiding this comment.
@copilot no annotations should be necessary, the containing type is already annotated RequiresUnreferencedCode and RequiresDynamicCode.
There was a problem hiding this comment.
DefaultJsonTypeInfoResolver doesn't have type-level [RequiresUnreferencedCode]/[RequiresDynamicCode] — only individual methods do. Removing the annotations from these methods causes build failures (IL2055 for MakeGenericType, IL2070 for GetInterfaces, IL3050 for AOT). The current approach matches the existing pattern in this file where every private method that uses reflection has both attributes (e.g., CreateTypeInfoCore, AddMembersDeclaredBySuperType, DetermineCreateObjectDelegate, etc.).
There was a problem hiding this comment.
@eiriktsarpalis The type annotations do not apply to static methods. I expect that copilot is going to find they are necessary.
…quiresDynamicCode propagation Remove invalid suppressions on TryResolveOpenGenericDerivedType and propagate the requirements using RequiresUnreferencedCode and RequiresDynamicCode on both ResolveOpenGenericDerivedTypes and TryResolveOpenGenericDerivedType. The caller CreateTypeInfoCore already has both attributes. Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/d2df80f8-8ef2-4e50-8949-b80c4ab36d89 Co-authored-by: jkotas <6668460+jkotas@users.noreply.github.com>
eiriktsarpalis
left a comment
There was a problem hiding this comment.
@copilot please find the location that handles source generated JsonTypeInfo and currently resolved derived type metadata using attribute resolution. This is no longer necessary given that the source generator does this for us, however in order to preserve binary compatibility with source generated code from earlier versions of STJ, we need to conditionally preserve the attribute logic. This can be triggered if we detect that no polymorphism metadata is already present on the JsonTypeInfo.
| int i => $", {i}", | ||
| _ => "", | ||
| }; | ||
| writer.WriteLine($"({JsonTypeInfoLocalVariableName}.{PolymorphismOptionsPropName} ??= new()).DerivedTypes.Add(new global::System.Text.Json.Serialization.Metadata.JsonDerivedType(typeof({derivedTypeSpec.DerivedType.FullyQualifiedName}){discriminatorArg}));"); |
There was a problem hiding this comment.
Doing ??= new() shouldn't be necessary for every entry. Prefer something along the lines of:
PolymorphismOptions options = new();
options.DerivedTypes.Add(new JsonDerivedType(...));
options.DerivedTypes.Add(new JsonDerivedType(...));
options.DerivedTypes.Add(new JsonDerivedType(...));
jsonTypeInfo.PolymorphismOptions = options;There was a problem hiding this comment.
Fixed in 4712c26. The emitter now creates a single polymorphismOptions variable, adds all derived types, then assigns to jsonTypeInfo.PolymorphismOptions.
| ClassType = classType, | ||
| PrimitiveTypeKind = primitiveTypeKind, | ||
| IsPolymorphic = isPolymorphic, | ||
| OpenGenericDerivedTypes = openGenericDerivedTypes?.ToImmutableEquatableArray(), |
There was a problem hiding this comment.
@copilot ensure the derived types are sorted by some key for deterministic compilation.
There was a problem hiding this comment.
Added in 4712c26. Derived types are sorted by DerivedType.FullyQualifiedName via OrderBy before creating the immutable array.
| var derivedType = (ITypeSymbol)attributeData.ConstructorArguments[0].Value!; | ||
| EnqueueType(derivedType, typeToGenerate.Mode); | ||
|
|
||
| // Open generic derived types (e.g. typeof(Derived<>)) are resolved at compile time |
There was a problem hiding this comment.
Move logic to a helper method that optionally returns a closed type to enqueue, or null to emit a warning diagnostic.
There was a problem hiding this comment.
Refactored in 4712c26. TryResolveOpenGenericDerivedType now accepts ITypeSymbol baseType and does the INamedTypeSymbol { IsGenericType: true } check internally, returning false for non-generic base types. The caller is simplified to TryResolveOpenGenericDerivedType(unboundDerived, typeToGenerate.Type, out ...).
| /// for the reflection-based resolver path (not source generator / AOT). | ||
| /// </summary> | ||
| [RequiresUnreferencedCode(JsonSerializer.SerializationUnreferencedCodeMessage)] | ||
| [RequiresDynamicCode(JsonSerializer.SerializationRequiresDynamicCodeMessage)] |
…actor parser, remove doc comment, fix null guard, conditional PopulatePolymorphismMetadata Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/e620b332-1edf-45ad-9d85-bc907429e008 Co-authored-by: eiriktsarpalis <6668460+eiriktsarpalis@users.noreply.github.com>
Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/e620b332-1edf-45ad-9d85-bc907429e008 Co-authored-by: eiriktsarpalis <6668460+eiriktsarpalis@users.noreply.github.com>
Addressed in 4712c26. |
Description
Polymorphic serialization had no mechanism for open generic derived type declarations. This adds support for resolving open generic types in
[JsonDerivedType]attributes against the constructed base type's type arguments.Also supports complex forms like
Derived<T> : Base<List<T>>, interfaces, and nested types.Changes
DefaultJsonTypeInfoResolver.Helpers(reflection-only path): AddedResolveOpenGenericDerivedTypeswithTryResolveOpenGenericDerivedTypeusingType.MakeGenericType. This is called only from the reflection-based resolver, keepingMakeGenericTypeout of shared code paths that run in Native AOT. Both methods propagate[RequiresUnreferencedCode]and[RequiresDynamicCode]from the callerCreateTypeInfoCore. NullDerivedTypevalues are handled safely viais { IsGenericTypeDefinition: true }pattern matching.JsonPolymorphismOptions.CreateFromAttributeDeclarations: Skips open generic derived types (IsGenericTypeDefinition). They are resolved separately by the reflection resolver or source generator.JsonTypeInfo.PopulatePolymorphismMetadata: Now conditionally skips attribute-based resolution whenPolymorphismOptionshas already been set by the source generator. This preserves binary compatibility with source-generated code from earlier versions of STJ — older source-gen code that doesn't set polymorphism options will still fall back to attribute-based resolution.PolymorphicTypeResolver: Unchanged — no open generic handling here. Open generics are resolved before they reach the resolver.TryResolveOpenGenericDerivedTypehelper method using Roslyn's type system (INamedTypeSymbol.Construct()) with full type parameter unification. AcceptsITypeSymbol baseTypeand handles the generic type check internally. Resolves open generics at compile time, enqueues the closed types for generation, and extracts discriminator values from the attribute. Resolved derived types are sorted byFullyQualifiedNamefor deterministic compilation.JsonPolymorphismOptionsinstance, adds all resolved derived types, then assigns tojsonTypeInfo.PolymorphismOptions. Uses proper type ref constants (JsonPolymorphismOptionsTypeRef,JsonDerivedTypeTypeRef). No runtimeMakeGenericTypein the source-gen path.PolymorphicDerivedTypeSpecrecord to carry resolved open generic derived type data through the incremental pipeline.Polymorphism_OpenGenericDerivedTypeNotSupportedruntime message for unresolvable open generics (non-generic base, unresolvable type params, constraint violations).SYSLIB1227source generator diagnostic for open generics that cannot be resolved from the base type's generic arguments.Derived<T> : Base<List<T>>), programmatic API with closed types, and invalid pattern rejection. UpdatedDefaultResolver_ReportsCorrectPolymorphismMetadatato handle open generic derived types that are resolved to closed types.Architecture
The key architectural decision (per reviewer feedback) is that
Type.MakeGenericTypeis never called from shared code paths — only fromDefaultJsonTypeInfoResolver.Helpers(reflection-only). The reflection-only methods properly propagate[RequiresUnreferencedCode]and[RequiresDynamicCode]attributes through the call chain (CreateTypeInfoCore→ResolveOpenGenericDerivedTypes→TryResolveOpenGenericDerivedType) rather than using suppressions. The source generator resolves open generics at compile time using Roslyn's type system and emits the concrete types directly, making this fully Native AOT compatible.For binary compatibility,
PopulatePolymorphismMetadata()(called byJsonMetadataServicesfor source-gen path) conditionally preserves attribute-based resolution: it only reads[JsonDerivedType]attributes ifPolymorphismOptionshasn't already been set by the source generator. This ensures older source-generated code (which didn't populate polymorphism options directly) continues to work.Rejected patterns (throw
InvalidOperationException)