System.Text.Json: transitive [JsonDerivedType] resolution + cross-assembly boundary semantics#124506
System.Text.Json: transitive [JsonDerivedType] resolution + cross-assembly boundary semantics#124506
[JsonDerivedType] resolution + cross-assembly boundary semantics#124506Conversation
|
Tagging subscribers to this area: @dotnet/runtime-infrastructure |
Co-authored-by: stephentoub <2642209+stephentoub@users.noreply.github.com>
Co-authored-by: stephentoub <2642209+stephentoub@users.noreply.github.com>
Co-authored-by: stephentoub <2642209+stephentoub@users.noreply.github.com>
[JsonDerivedType] resolution + cross-assembly boundary semantics
|
@copilot invoke the code-review skill on opus 4.6 and post the analysis/results as a comment on this pr |
I invoked the code-review skill and attempted to run it on Opus 4.6, but this environment rejected that model as unsupported. I then ran the same review with the available model and here are the results for commit 🤖 Copilot Code Review — PR #124506Holistic Assessment
Detailed Findings
Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
| /// Walks the DerivedTypes list (which may grow during iteration) and appends | ||
| /// any additional derived types found via transitive [JsonDerivedType] declarations | ||
| /// on already-known derived types. Types with [JsonPolymorphic] are treated as | ||
| /// boundary nodes and are not traversed. Uses a set to prevent duplicates and cycles. |
There was a problem hiding this comment.
Types with [JsonPolymorphic] are treated as boundary nodes and are not traversed
@copilot why?
There was a problem hiding this comment.
Because [JsonPolymorphic] on the child means that child is declaring its own polymorphic contract (including its own discriminator space). If we kept traversing through it, the parent would implicitly absorb the child’s derived-type declarations, which can mix independent contracts and produce surprising parent-level behavior/discriminator coupling. Treating it as a boundary keeps transitive expansion to a single unbroken [JsonDerivedType] chain unless a new polymorphic root is explicitly declared.
|
|
||
| // Do not walk through types that declare their own [JsonPolymorphic] configuration; | ||
| // those define an independent polymorphic scheme and their [JsonDerivedType] entries | ||
| // belong to that scheme, not to the root type's. |
There was a problem hiding this comment.
@copilot what does it mean for those to be an independent scheme? Is that because [JsonPolymorphic] brings with it configuration that would then only apply to this type and its derivations (like discriminator)? Does that actually affect the semantics of the [JsonDerivedType] though?
There was a problem hiding this comment.
Yes — that’s exactly the rationale. [JsonPolymorphic] marks a type as a polymorphic root with its own options (TypeDiscriminatorPropertyName, UnknownDerivedTypeHandling, etc.), so its [JsonDerivedType] declarations are interpreted in that root’s contract.
If we transitively hoist those declarations into an ancestor root, we effectively reinterpret them under the ancestor’s contract (possibly different discriminator property/handling), and we can also create discriminator-space coupling/conflicts between two roots that were authored independently.
So while [JsonDerivedType] still means “this base recognizes this derived type,” the question is which base’s scheme it belongs to. The boundary rule keeps it scoped to the nearest root that explicitly declares polymorphism.
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.