Reject non-uniform JsonDerivedType registrations against a generic base (JsonPolymorphism follow-up)

[eiriktsarpalis]

Note

The code in this PR was AI/Copilot-generated. The PR author reviewed the changes before opening this PR.

Follow-up to #127318

While stress-testing the open-generics polymorphism support added in #127318, a handful of pathological [JsonDerivedType] registrations surfaced where the source generator either crashed, silently produced ill-formed metadata, or emitted a confusing SYSLIB1229. The shared root cause: a derived registration that does not apply uniformly to every specialization of the open generic base it is attached to — for example, a closed derived attached to an open generic base, or an open derived whose base specification pins one of the base's type arguments to a concrete type.

This PR formalises a uniform-applicability rule: a [JsonDerivedType] attribute on an open generic base is only well-formed if it admits exactly one canonical mapping from each derived type parameter to a base type parameter that simultaneously satisfies every matching ancestor of the derived type, with derived constraints exactly matching the corresponding base parameter constraints. Anything that does not meet this bar is rejected at compile time with SYSLIB1229 (source generator) and at Configure() time with InvalidOperationException (reflection resolver), instead of being silently filtered, silently miscompiled, or only surfacing on the first serialization call.

Motivating cases

Each of the cases below is from the original triage list. Before this PR they either crashed, emitted a confusing diagnostic, or silently produced wrong metadata. After this PR they each emit SYSLIB1229 (source generator) and throw InvalidOperationException (reflection) with a precise reason string:

1. Closed derived on an open generic base

[JsonDerivedType(typeof(Cat))]
[JsonDerivedType(typeof(Dog))]
class Animal<T>;
class Cat : Animal<int>;
class Dog : Animal<string>;

The attribute lives on the open base definition Animal<T> and is therefore shared across every closure of Animal. Cat only applies to Animal<int>; Dog only applies to Animal<string>; neither applies to (e.g.) Animal<bool>. There is no single uniform answer the resolver can give.

→ SYSLIB1229 with reason Polymorphism_OpenGeneric_Reason_ClosedDerivedOnGenericBase:

a closed derived type cannot serve a generic base type uniformly; the registration pins a specific specialization of the base.

2. Open derived with a constraint that the base's open form does not guarantee

[JsonDerivedType(typeof(Derived<>))]
class Base<T>;
class Derived<T> : Base<T> where T : IEnumerable<int>;

Derived<T> is only constructible for T : IEnumerable<int>, but Base<T> has no such constraint, so the registration would silently work for Base<List<int>> and break for Base<string>.

→ SYSLIB1229 with reason Polymorphism_OpenGeneric_Reason_ConstraintMismatch:

the derived type's type parameter 'T' declares constraints that differ from the constraints on the corresponding base type parameter 'T'

3. Multiple derived registrations with different arities

[JsonDerivedType(typeof(Cat<>))]
[JsonDerivedType(typeof(Cat<,>))]
class Animal<T>;
class Cat<T> : Animal<T>;
class Cat<T, U> : Animal<T>;

The Cat<T, U> registration has an unbound parameter U the base cannot pin down.

→ SYSLIB1229 with reason Polymorphism_OpenGeneric_Reason_UnboundParameter:

the type parameter 'U' of the derived type is not bound by the base type's arguments

4. Closed derived registered against a non-generic base that the derived does not actually inherit from

class Base;
[JsonDerivedType(typeof(int))]
class Foo : Base; // typo: should be typeof(FooImpl)
class FooImpl : Foo;

Previously this passed through the source generator silently and only failed at first serialization (inside PolymorphicTypeResolver.IsSupportedDerivedType). Now the source generator catches it at metadata-resolution time.

→ SYSLIB1229 with reason Polymorphism_OpenGeneric_Reason_NotAssignable:

the derived type is not assignable to the base type

What's actually in this PR

1. Uniform-applicability resolver (mirrored in source-gen RoslynExtensions.TryResolveOpenGenericDerivedType and reflection DefaultJsonTypeInfoResolver.Helpers.TryResolveOpenGenericDerivedType). Per-ancestor unifications are computed independently and then verified to coincide — this is what catches asymmetric multi-interface implementations like D<U1, U2> : IBase<U1, U2>, IBase<U2, U1> where each ancestor admits a unifier on its own but no single canonical answer covers both.
2. Exact constraint-match check between each derived type parameter and the matched base type parameter, applied recursively to F-bounded constraints (so where T : IComparable<T> on the derived matches where U : IComparable<U> on the base only after the renaming substitution has been applied to both occurrences). The compile-time-only notnull constraint is intentionally ignored.
3. Closed-derived-on-generic-base early-out that rejects the pattern in case 1 above without going through the unification machinery (which would have rejected it anyway, but with a less actionable message).
4. Post-condition assignability check in the source-generator's TryResolveDerivedType dispatcher: every successful arm — including the common closed-derived / non-generic-base arm — now flows through a single baseType.IsAssignableFrom(resolvedDerivedType) gate. This is the change that catches case 4 above.
5. Diagnostic split: the previous catch-all NonUniversalPinning reason has been renamed to NonUniformPinning (no unifier exists for the ancestor-base pair) and an additional NonUniformUnification reason has been carved out for the case where a unifier exists but maps a derived parameter to something other than a base parameter (a pure renaming check). The new reason is essentially unreachable today; it exists so any future relaxation of TryUnifyWith surfaces with a precise diagnostic.
6. Terminology refresh: SR strings, doc comments, code comments, and test method names have been updated from universal / universally to uniform / uniformly throughout the polymorphism subsystem. "Uniform" is a better fit for the property being checked (the resolver works the same way for every specialization of the base) and matches the uniformity terminology already used in System.Linq.Expressions and the runtime's generic dictionary code.

Patterns that remain admitted

Everything that was already admitted by #127318 continues to work. The complete admitted set is:

Pattern	Worked example
Matching arity, identity binding	[JsonDerivedType(typeof(D<>))] class B<T>; class D<T> : B<T>;
Reordered parameters	[JsonDerivedType(typeof(D<,>))] class B<T1,T2>; class D<U,V> : B<V,U>;
Wrapped / nested type arguments	[JsonDerivedType(typeof(D<>))] class B<T>; class D<T> : B<List<T>>;
Generic interface base	[JsonDerivedType(typeof(D<>))] interface IB<T>; class D<T> : IB<T>;
Type parameters from enclosing generic types	[JsonDerivedType(typeof(Outer<>.Leaf<>))] class B<T>; class Outer<T> { class Leaf<U> : B<(T,U)>; }
Identical constraints on identified parameters	[JsonDerivedType(typeof(D<>))] class B<T> where T : struct; class D<T> : B<T> where T : struct;
F-bounded constraints, identical after renaming	[JsonDerivedType(typeof(D<>))] class B<U> where U : IComparable<U>; class D<T> : B<T> where T : IComparable<T>;
[JsonDerivedType(typeof(Cat))] class Animal; class Cat : Animal; (non-generic base, closed derived — the common case)	unchanged

Diagnostic table

Reason ID	Message
Polymorphism_OpenGeneric_Reason_NotAssignable	the derived type is not assignable to the base type
Polymorphism_OpenGeneric_Reason_UnificationFailed	the base type's arguments do not match the derived type's base specification
Polymorphism_OpenGeneric_Reason_NonUniformPinning	the derived type does not apply uniformly to every specialization of the base type because it pins one or more base type arguments
Polymorphism_OpenGeneric_Reason_NonUniformUnification (new)	the unification of the derived type with the base type binds a derived type parameter to a target other than a base type parameter
Polymorphism_OpenGeneric_Reason_UnboundParameter	the type parameter '{0}' of the derived type is not bound by the base type's arguments
Polymorphism_OpenGeneric_Reason_AmbiguousMatch	the derived type matches the base type through multiple ancestors
Polymorphism_OpenGeneric_Reason_ConstraintMismatch	the derived type's type parameter '{0}' declares constraints that differ from the constraints on the corresponding base type parameter '{1}'
Polymorphism_OpenGeneric_Reason_ClosedDerivedOnGenericBase	a closed derived type cannot serve a generic base type uniformly; the registration pins a specific specialization of the base

All reasons are surfaced through SYSLIB1229 (source generator) and InvalidOperationException from ThrowHelper.ThrowInvalidOperationException_OpenGenericDerivedTypeCouldNotBeResolved (reflection). SYSLIB1229 is #pragma-suppressible; suppressing it causes the offending registration to be dropped from the generated metadata.

Tests

Added coverage across both worlds:

• Reflection (PolymorphicTests.CustomTypeHierarchies.cs) — uniform vs non-uniform variants of every pattern above, plus a position-pinning matrix, parameter collapse / reorder cases, transitive inheritance, self-referential constraints, and a multi-interface-construction case (D<U1,U2> : IBase<U1,U2>, IBase<U2,U1>) that exercises the per-ancestor coincidence check.
• Source-gen runtime (System.Text.Json.SourceGeneration.Tests/Serialization/PolymorphismTests.cs) — mirrors the reflection fixture, with #pragma warning disable SYSLIB1229 annotations on the contexts that intentionally exercise the failure path so the fixture compiles.
• Source-gen diagnostics (JsonSourceGeneratorDiagnosticsTests.cs) — one OpenGenericDerivedType_*_WarnsWithSYSLIB1229 test per reason, plus five new ClosedDerivedType_* tests covering the post-condition assignability check (closed derived not assignable to a non-generic class base / interface base, value type registered against a reference type base, plus regression guards for the well-formed happy paths so the post-condition isn't accidentally over-eager).

Local test runs on main:

Suite	Result
System.Text.Json.Tests (net11.0 Release)	53012/0/0
System.Text.Json.Tests (net481 Release)	52774/0/0
System.Text.Json.SourceGeneration.Roslyn4.4.Tests (net11.0 Release)	8337/0/0
System.Text.Json.SourceGeneration.Roslyn4.4.Tests (net481 Release)	8250/0/0
System.Text.Json.SourceGeneration.Roslyn3.11.Tests (net481 Release)	465/0/0
System.Text.Json.SourceGeneration.Roslyn4.4.Unit.Tests (net11.0 Release)	250/0/0

Drive-by test fix

tests/Common/JsonCreationHandlingTests.Object.cs had two [JsonDerivedType] registrations on BaseClassRecursive that referenced types from an unrelated hierarchy (BaseClassWithPolymorphism and DerivedClass_DerivingFrom_BaseClassWithPolymorphism) — what appears to be a copy-paste from the neighbouring BaseClassWithPolymorphism class. The test's intent is to verify that the JsonObjectCreationHandling.Populate plus polymorphism combination throws InvalidOperationException at runtime; with the registrations corrected to a real subtype the conflict still throws and the test passes on both reflection and source-generator paths.

Branch name

The local branch name still carries the pr/127318/copilot/... prefix because the session's PR association couldn't be reset prior to opening this PR. The branch is otherwise disentangled from #127318.

cc @eiriktsarpalis

[dotnet-policy-service]

Tagging subscribers to this area: @dotnet/area-system-text-json
See info in area-owners.md if you want to be subscribed.

[Copilot]

Pull request overview

This PR adjusts System.Text.Json polymorphism metadata generation for generic base specializations so that closed [JsonDerivedType] registrations that don’t match the current constructed generic base are silently filtered (instead of causing exceptions / diagnostics). It also ensures that when filtering leaves no derived types and there was no explicit [JsonPolymorphic], the specialization is treated as non-polymorphic to avoid downstream “empty options” failures.

Changes:

• Reflection resolver: filter out mismatched closed derived registrations for generic base specializations and demote to non-polymorphic when filtering empties the list (unless [JsonPolymorphic] was explicitly declared).
• Source generator parser: mirror the closed-derived filtering for generic base specializations.
• Add reflection + source-gen runtime tests covering specialization-specific closed-derived registrations, constraint/unification failure behaviors, and mixed open/closed scenarios.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.

File	Description
src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/DefaultJsonTypeInfoResolver.Helpers.cs	Filters mismatched closed derived types for generic base specializations and demotes to non-polymorphic when filtering empties derived types without explicit [JsonPolymorphic].
src/libraries/System.Text.Json/gen/JsonSourceGenerator.Parser.cs	Adds source-gen-side filtering of mismatched closed derived registrations for generic base specializations (mirrors reflection behavior).
src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/PolymorphicTests.CustomTypeHierarchies.cs	Adds reflection-based test coverage for specialization-specific filtering and existing open-derived failure modes.
src/libraries/System.Text.Json/tests/System.Text.Json.SourceGeneration.Tests/Serialization/PolymorphismTests.cs	Adds source-gen runtime tests validating emitted metadata behavior (filtered derived types and non-polymorphic demotion).

[Copilot]

Pull request overview

Copilot reviewed 22 out of 22 changed files in this pull request and generated 4 comments.

[Copilot]

Pull request overview

Copilot reviewed 22 out of 22 changed files in this pull request and generated 3 comments.

[Copilot]

Pull request overview

Copilot reviewed 23 out of 23 changed files in this pull request and generated 4 comments.

[github-actions]

🤖 Copilot Code Review — PR #129294

Note

This review was generated by GitHub Copilot.

Holistic Assessment

Motivation: Well-justified. The PR addresses real failure modes discovered through stress-testing of #127318's open-generics polymorphism: crash, silent miscompilation, and confusing diagnostics for pathological [JsonDerivedType] registrations. The uniform-applicability rule is a principled design decision that makes the system more predictable and prevents silent per-closure failures.

Approach: Sound. The implementation mirrors the resolver logic across source-gen (Roslyn symbols) and reflection (System.Type), with comprehensive documentation about intentional asymmetries. Extracting the core unification into RoslynExtensions.TryResolveOpenGenericDerivedType and adding a top-level TryResolveDerivedType dispatcher that handles all four cases (open/closed × generic/non-generic base) is a good separation of concerns.

Summary: ⚠️ Needs Human Review. The code is well-structured, thoroughly tested, and the design rationale is clearly documented. However, this is a deliberate behavioral breaking change from patterns that were accepted in #127318 (e.g. Derived<T> : Base<List<T>>, Derived<T> : Base<T, int>). While these patterns were added in the same release cycle and the change is well-motivated, a human reviewer should confirm the breaking-change stance is acceptable at this stage and whether any documentation/announcement is needed beyond the PR description. Additionally, there are two minor dead-resource-string items noted below.

---

Detailed Findings

✅ Correctness — Core algorithm is sound

The per-ancestor independent unification with subsequent coincidence check is a correct implementation of the uniform-applicability concept. Key properties verified:

• Each ancestor's unification is computed independently via TryUnifyWith, which performs structural rigid-target matching.
• The canonical substitution consistency check (SubstitutionsEqual) catches asymmetric multi-interface implementations like D<U1, U2> : IBase<U1, U2>, IBase<U2, U1> where each ancestor admits a unifier but no single answer covers both.
• The constraint-equivalence check (AreConstraintsEquivalent) handles F-bounded constraints correctly by applying substitution before comparison, and intentionally ignores notnull (compile-time only).
• The post-condition baseType.IsAssignableFrom(resolvedDerivedType) catches invalid closed-derived registrations at compile time that previously only surfaced at first serialization.

✅ Mirror implementations are consistent

The source-gen (RoslynExtensions.TryResolveOpenGenericDerivedType + AreConstraintsEquivalent) and reflection (DefaultJsonTypeInfoResolver.Helpers.TryResolveOpenGenericDerivedType + ReflectionExtensions.AreConstraintsEquivalent) implementations follow the same algorithm structure. The documented asymmetry around unmanaged constraints (reflection can't observe the modreq) is properly handled with a MakeGenericType fallback.

✅ Diagnostic dedup is well-designed

The DiagnosedOpenDerivedRegistrations set (keyed by (BaseDefinition, DerivedDefinition)) prevents diagnostic spam when the same open base is referenced via multiple [JsonSerializable] closures. Using field ??= new(...) for lazy initialization is clean.

✅ Drive-by test fix is correct

The fix to JsonCreationHandlingTests.Object.cs (changing typeof(BaseClassWithPolymorphism) → typeof(BaseClassRecursive) and typeof(DerivedClass_DerivingFrom_BaseClassWithPolymorphism) → typeof(DerivedClass_DerivingFrom_BaseClassRecursive)) is a legitimate copy-paste error correction. Verified that DerivedClass_DerivingFrom_BaseClassRecursive actually inherits from BaseClassRecursive (line 561).

✅ Test coverage is comprehensive

The test additions cover:

• All diagnostic reason codes with dedicated source-gen unit tests
• Positive and negative variants for the reflection runtime tests (pattern catalog A–K)
• Source-gen runtime tests verifying that suppressed SYSLIB1229 drops registrations from metadata
• Multi-interface ancestor scenarios (D1–D4)
• Constraint matrix (C1–C8)
• Nested enclosing types (E1–E2), transitive inheritance (F1–F2), and programmatic API parity (K)

💡 Dead resource strings in gen project

Polymorphism_OpenGeneric_Reason_ConstraintViolation and Polymorphism_OpenGeneric_Reason_UnificationFailed are defined in gen/Resources/Strings.resx (and all xlf files) but are no longer referenced by any C# code in the source generator project. The old code that used them was removed in this PR. They are still referenced in the src/ project (reflection side: ConstraintViolation in the MakeGenericType catch block), so they should remain there but could be cleaned from the gen resources. This is a minor follow-up item, not blocking.

⚠️ Breaking change scope — requires maintainer confirmation

This PR intentionally rejects patterns that were accepted by #127318:

• Wrapped type args: Derived<T> : Base<List<T>> (previously resolved Base<List<string>> → Derived<string>)
• Partial concretization: Derived<T> : Base<T, int> (previously resolved Base<string, int> → Derived<string>)
• Array shape pinning: Derived<T> : Base<T[]> (previously resolved Base<int[]> → Derived<int>)
• Closed derived on generic base: [JsonDerivedType(typeof(Cat))] class Animal<T>

Since #127318 was merged for .NET 10 (still in preview), this is the right time to tighten the rules. The design rationale (preventing silent failures when a different base closure is constructed) is sound. However, the test changes show that several previously-passing tests now assert ThrowsAsync<InvalidOperationException>, which confirms the behavioral shift. A maintainer should confirm:

1. This breaking-change scope is acceptable at the current release stage.
2. Whether a breaking-change doc should be filed (even for preview-to-preview changes, per the repo's conventions).

💡 Minor: NestedGeneric_TypeParameterInEnclosing test intent shift

The test NestedGeneric_TypeParameterInEnclosing_ThrowsForNonUniformPinning (previously _Resolves) was originally added to verify that the source-gen correctly handles type parameters in enclosing types. Under the new rules this pattern (NestedDerivedParamInEnclosing<T> : NestedBaseB<NestedOuterB<T>.NestedBoxB<int>>) is correctly rejected because it pins TInner to int. The test name and comment update accurately reflect the new semantic. No action needed — just noting for reviewer awareness.

Note

🔒 Integrity filter blocked 1 item

The following item was blocked because it doesn't meet the GitHub integrity level.

• Reject non-uniform JsonDerivedType registrations against a generic base (JsonPolymorphism follow-up) #129294 pull_request_read: has lower integrity than agent requires. The agent cannot read data with integrity below "approved".

To allow these resources, lower min-integrity in your GitHub frontmatter:

tools:
  github:
    min-integrity: approved  # merged | approved | unapproved | none

Generated by Code Review for issue #129294 · ● 22.2M · ◷