Filter closed-derived registrations that don't match the base specialization (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 few pathological registration shapes surfaced where the source generator threw InvalidOperationException (or emitted SYSLIB1229) on perfectly legitimate user code. The common pattern: the user attaches closed [JsonDerivedType] attributes to an open generic base, intending each attribute to apply to a specific specialization of that base.

Example that motivated this PR

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

Before this change:

• Resolving Animal<int> would crash because Dog (assignable only to Animal<string>) failed the IsAssignableFrom check inside PolymorphicTypeResolver.
• Resolving Animal<bool> would crash because neither closed derived was assignable to it.

After this change:

• Animal<int> resolves with { Cat } as derived types.
• Animal<string> resolves with { Dog } as derived types.
• Animal<bool> silently demotes to non-polymorphic (no [JsonPolymorphic] was declared, so emitting empty polymorphism options would itself throw).

The same fix is mirrored in both the reflection resolver (DefaultJsonTypeInfoResolver.Helpers.ResolveOpenGenericDerivedTypes) and the source-gen parser (JsonSourceGenerator.Parser.ProcessTypeCustomAttributes).

What is NOT changed

All open-derived failure modes from #127318 keep their existing behavior — NotAssignable, UnboundParameter, UnificationFailed, ConstraintViolation, and AmbiguousMatch still throw at reflection time and still emit SYSLIB1229 at source-gen time. The rationale: an open [JsonDerivedType(typeof(Foo<>))] declares intent to participate at this generic position, so failure to do so is a real misregistration the user should learn about. A closed derived attached to an open base, by contrast, structurally cannot apply to every specialization — silent filtering is the only sensible behavior.

The behavioral change is therefore confined to:

Symbol	Before	After
Closed derived on a generic base, not assignable to the current specialization	Throws InvalidOperationException / emits SYSLIB1229	Silently dropped
All JsonDerivedType attributes filtered + no explicit [JsonPolymorphic]	Empty polymorphism options → throws downstream	Demoted to non-polymorphic
Closed derived on a non-generic base, not assignable	Throws (unchanged — true misregistration)	Throws (unchanged)
Any open-derived failure mode	Throws / SYSLIB1229 (unchanged)	Throws / SYSLIB1229 (unchanged)

Implementation notes

• Reflection side iterates derivedTypes backwards so RemoveAt(i) is safe.
• Reflection side captures the pre-filter count and, if filtering empties the list and [JsonPolymorphic] was not explicitly declared, nulls out the options before calling SetPolymorphismOptions. When the user did write [JsonPolymorphic], intent is preserved and the downstream "no derived types specified" error is left to surface.
• Source-gen side uses Compilation.HasImplicitConversion to perform the same assignability check at compile time.

Tests

• Reflection — added SpecAnimal_ResolvesPerSpecialization, ConstrainedBase_*, ExtraParam_*, and Mixed_OpenAndClosedDerived_* cases to PolymorphicTests.CustomTypeHierarchies.cs covering all the patterns from the original issue.
• Source-gen runtime — mirrored cases in System.Text.Json.SourceGeneration.Tests/Serialization/PolymorphismTests.cs. #pragma-suppressed SYSLIB1229 for the contexts that intentionally exercise the open-derived failure paths to keep the fixture compileable.
• All affected suites pass locally: PolymorphicTests* 6136/6136, JsonPolymorphismOptionsTests 35/35, source-gen runtime PolymorphismTests 16/16, JsonSourceGeneratorDiagnosticsTests 86/86 (Roslyn 4.4) + 81/81 (Roslyn 3.11).

Branch name

Note: 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 contents are completely disentangled from #127318 — it sits directly on top of origin/main with a single commit.

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).

[github-actions]

Note

This review was generated by Copilot.

🤖 Copilot Code Review — PR #129294

Holistic Assessment

Motivation: Justified. PR #127318 introduced open-generics polymorphism support but left a gap: closed [JsonDerivedType] attributes on open generic bases crash for specializations that structurally cannot match. Since attributes are declared on the type definition and cannot conditionally apply per specialization, silent filtering is the only sensible semantic.

Approach: Sound and minimal. The fix correctly splits behavior between closed-derived (silent filter for generic bases) and open-derived (loud failure preserved) at both the reflection and source-gen layers. The backward-iteration pattern, demotion logic, and ClassifyConversion-based assignability check are all well-implemented.

Summary: ✅ LGTM. The code is correct, the reflection/source-gen parity is maintained, previous review feedback has been addressed (ClassifyConversion instead of HasImplicitConversion, test renames, explicit [JsonPolymorphic] test), and test coverage is comprehensive. One minor observation below (non-blocking).

---

Detailed Findings

✅ Correctness — Core filtering logic is sound

Reflection side (ResolveOpenGenericDerivedTypes):

• Backward iteration with RemoveAt(i) is safe — removing at index i doesn't disturb indices below i.
• The split of the old combined if (entry.DerivedType is null || !entry.DerivedType.IsGenericTypeDefinition) continue; into two separate blocks correctly separates the null-check from the new closed-derived filtering logic.
• Closed derived types on non-generic bases pass through the filter (no RemoveAt) and hit continue, correctly deferring validation to PolymorphicTypeResolver.

Source-gen side (ProcessTypeCustomAttributes):

• IsClosedDerivedAssignableToBase uses ClassifyConversion with IsIdentity || (IsImplicit && IsReference) which precisely mirrors Type.IsAssignableFrom semantics — excludes user-defined implicit operators that HasImplicitConversion would incorrectly admit.
• The else if condition only fires when the base is generic (INamedTypeSymbol { IsGenericType: true }) and the derived is not an unbound generic (first if already caught that). This is the correct filter scope.
• When all derived types are filtered via continue, the existing condition hasPolymorphicAttribute || derivedTypes is { Count: > 0 } naturally handles demotion — no special post-processing needed.

Demotion logic (PopulatePolymorphismMetadata):

• originalDerivedTypeCount > 0 guard ensures only actively-filtered lists (not originally-empty ones) trigger demotion.
• polymorphicAttribute is null correctly preserves user intent when [JsonPolymorphic] is explicit.

✅ Tests — Comprehensive scenario coverage

The test matrix covers:

• Per-specialization filtering (int/string match, bool filters all)
• Demotion to non-polymorphic when all filtered + no [JsonPolymorphic]
• Explicit [JsonPolymorphic] + all-filtered → throws (the SpecAnimalExplicit<T> test)
• Open-derived constraint failure → throws (still surfaces error)
• Open-derived unbound parameter → throws
• Mixed closed + open registrations → only applicable survive
• Round-trip serialization/deserialization

Source-gen tests appropriately use narrowly-scoped #pragma suppression for SYSLIB1229.

💡 Minor observation — source-gen test gap for [JsonPolymorphic]-explicit path (non-blocking, follow-up)

The reflection tests include ClosedDerivedTypes_AllFilteredButPolymorphicAttributeExplicit_Throws which validates that an explicit [JsonPolymorphic] preserves intent even when filtering empties the list. On the source-gen side, this scenario is implicitly handled (the hasPolymorphicAttribute flag keeps polymorphismOptions populated), but there's no explicit test for it. This is not a blocker — the source-gen code path naturally produces the correct outcome, and the reflection test validates the runtime behavior — but a future follow-up could add it for completeness.

✅ Previous review feedback addressed

All four resolved review threads from the first review pass are addressed in commit c5e3059:

• HasImplicitConversion → ClassifyConversion with identity/reference check ✓
• Dropped INamedTypeSymbol pattern on derivedType (now handles all ITypeSymbol shapes) ✓
• Test renames to match SpecAnimal<T> fixture ✓
• Removed unnecessary System.Collections.Generic qualifications ✓
• Added explicit [JsonPolymorphic] test ✓

✅ No public API changes

No ref/ assembly files modified. The behavioral change is internal to the polymorphism resolution pipeline.

Note

🔒 Integrity filter blocked 2 items

The following items were blocked because they don't meet the GitHub integrity level.

• f59cad6 list_commits: has lower integrity than agent requires. The agent cannot read data with integrity below "approved".
• 85de42c list_commits: 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 · ● 6.2M · ◷