Support generic converters on generic types with JsonConverterAttribute#123209
Support generic converters on generic types with JsonConverterAttribute#123209eiriktsarpalis merged 16 commits intomainfrom
Conversation
|
@copilot there appear to be build failures related to the source generator. Please fix those. |
Fixed in e5944c5. The issue was that the code tried to assign to |
eiriktsarpalis
left a comment
There was a problem hiding this comment.
@copilot you should add a few tests that check what happens if there is a mismatch in type parameters or type constraints between the two types, or if a the generic instantiation implements a converter not matching the original type. Add tests both at runtime and source generator unit tests checking for relevant diagnostic messages.
...stem.Text.Json/tests/System.Text.Json.SourceGeneration.Tests/TestClasses.CustomConverters.cs
Show resolved
Hide resolved
Added tests in 92d1a10:
|
|
@stephentoub @PranavSenthilnathan before we get this merged, I would appreciate feedback on the merits of this new convention. |
There was a problem hiding this comment.
Pull request overview
This PR extends JsonConverterAttribute to accept open generic converter types when applied to generic target types with matching arity, supporting both the reflection-based resolver and the source generator (including nested generic converter scenarios).
Changes:
- Runtime: construct a closed converter type from an open generic converter type when the target type is a closed generic with matching arity.
- Source generator: resolve
JsonConverterAttributefor open generic (including nested) converter types by constructing the appropriate closed converter type. - Tests: add reflection and source-generation test coverage for supported scenarios and key failure modes (e.g., arity mismatch diagnostics).
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/DefaultJsonTypeInfoResolver.Converters.cs | Constructs closed generic converter types at runtime when attribute specifies an open generic converter with matching arity. |
| src/libraries/System.Text.Json/gen/JsonSourceGenerator.Parser.cs | Adds logic to close unbound generic (including nested) converter types during generator parsing. |
| src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/CustomConverterTests/CustomConverterTests.Attribute.cs | Adds reflection-based tests covering open generic converters, nested converters, constraints, and mismatch scenarios. |
| src/libraries/System.Text.Json/tests/System.Text.Json.SourceGeneration.Unit.Tests/JsonSourceGeneratorDiagnosticsTests.cs | Adds generator diagnostic tests for arity mismatch and verifies generator behavior for other scenarios. |
| src/libraries/System.Text.Json/tests/System.Text.Json.SourceGeneration.Unit.Tests/CompilationHelper.cs | Adds compilation helpers to generate test sources for the new diagnostic and nested/constrained scenarios. |
| src/libraries/System.Text.Json/tests/System.Text.Json.SourceGeneration.Tests/TestClasses.CustomConverters.cs | Adds source-gen test model types/converters for open generic and nested generic converter cases. |
| src/libraries/System.Text.Json/tests/System.Text.Json.SourceGeneration.Tests/JsonSerializerContextTests.cs | Adds integration tests validating source-gen contexts can serialize/deserialize with open generic converters. |
@eiriktsarpalis what's the downside? |
None that I'm aware of. With the exception of debugger proxies I'm not aware of any serializers/source generators applying this convention, so it might be difficult for users to discover. |
|
It seems fine to me. Hard to imagine what else it could mean. |
|
@copilot please file a PR against https://github.com/dotnet/docs extending the https://github.com/dotnet/docs/blob/main/docs/standard/serialization/system-text-json/converters-how-to.md article with instructions and examples of how to use this new convention. Explain how in certain cases it provides an AOT compatible alternative JsonConverterFactory. |
# Conflicts: # src/libraries/System.Text.Json/tests/System.Text.Json.SourceGeneration.Unit.Tests/JsonSourceGeneratorDiagnosticsTests.cs
tarekgh
left a comment
There was a problem hiding this comment.
LGTM. Would it make sense to add a negative test to the following cases?
// Open generic converter on a non-generic type
[JsonConverter(typeof(MyConverter<>))] // open generic converter
public class PlainClass // NOT generic - no <T>
{
public string Name { get; set; }
} // Swapped parameter order
[JsonConverter(typeof(SwappedConverter<,>))]
public class MyType<T1, T2>
{
public T1 First { get; set; }
public T2 Second { get; set; }
}
public class SwappedConverter<A, B> : JsonConverter<MyType<B, A>> // note: B,A not A,B
{
// ...
}… swapped parameter order Co-authored-by: eiriktsarpalis <2813363+eiriktsarpalis@users.noreply.github.com>
Head branch was pushed to by a user without write access
Added both tests in 3e2fbae:
|
|
@copilot we should not be surfacing the |
…ric converter on incompatible type Co-authored-by: eiriktsarpalis <2813363+eiriktsarpalis@users.noreply.github.com>
Done in 5cffdfe. The open generic converter on non-generic type and arity mismatch cases now both throw |
|
/ba-g test failures unrelated |
…tem.Text.Json (#124645) ## Description System.Text.Json only supports naming policies globally via `JsonSerializerOptions.PropertyNamingPolicy`. This adds `JsonNamingPolicyAttribute` to allow granular naming policy control at the type, property, and field level. ### API ```csharp [AttributeUsage(AttributeTargets.Class | AttributeTargets.Struct | AttributeTargets.Interface | AttributeTargets.Property | AttributeTargets.Field, AllowMultiple = false)] public class JsonNamingPolicyAttribute : JsonAttribute { public JsonNamingPolicyAttribute(JsonKnownNamingPolicy namingPolicy); protected JsonNamingPolicyAttribute(JsonNamingPolicy namingPolicy); public JsonNamingPolicy NamingPolicy { get; } } ``` ### Usage ```csharp var options = new JsonSerializerOptions { PropertyNamingPolicy = JsonNamingPolicy.SnakeCaseLower }; JsonSerializer.Serialize(new MyPoco(), options); // {"myFirstProperty":"first","my-second-property":"second"} [JsonNamingPolicyAttribute(JsonKnownNamingPolicy.CamelCase)] public class MyPoco { public string MyFirstProperty { get; set; } = "first"; [JsonNamingPolicyAttribute(JsonKnownNamingPolicy.KebabCaseLower)] public string MySecondProperty { get; set; } = "second"; } ``` ### Priority order `JsonPropertyName` > member-level `JsonNamingPolicyAttribute` > type-level `JsonNamingPolicyAttribute` > `JsonSerializerOptions.PropertyNamingPolicy` > original name. ### Changes - **`JsonNamingPolicyAttribute.cs`**: New attribute with public ctor for `JsonKnownNamingPolicy` and protected ctor for custom `JsonNamingPolicy` subclasses - **`DefaultJsonTypeInfoResolver.Helpers.cs`**: Type-level `JsonNamingPolicyAttribute` is resolved once per type in `PopulateProperties` and threaded through `AddMembersDeclaredBySuperType` → `AddMember` → `CreatePropertyInfo`. `DeterminePropertyName` checks member-level then type-level attribute before falling back to global options. Integrated alongside the type-level `JsonIgnoreAttribute` resolution from #123209. - **Source generator** (`Parser.cs`, `KnownTypeSymbols.cs`): Parses the attribute at type and member level using `IsAssignableFrom` (handles subclasses); derived attributes with custom policies that can't be resolved at compile time fall back to `JsonKnownNamingPolicy.Unspecified` — this flows through `DetermineEffectiveJsonPropertyName` where `Unspecified` maps to null (returning the original CLR name), and the non-null `metadataJsonPropertyName` is then emitted to prevent the runtime global `PropertyNamingPolicy` from incorrectly overriding. Both `typeNamingPolicy` and `typeIgnoreCondition` parameters are threaded through `ProcessTypeCustomAttributes` → `ParsePropertyGenerationSpecs` → `ParsePropertyGenerationSpec`. - **Ref API**: Added `JsonNamingPolicyAttribute` to public API surface - **Tests**: Serialization/deserialization coverage for type-level, member-level, mixed, and precedence scenarios across both reflection and source-gen paths. Tests use `Assert.Equal` with triple-quote strings on full JSON output. Includes tests for custom derived `JsonNamingPolicyAttribute` subclasses using the protected ctor — reflection tests verify the custom policy is applied correctly, and source gen tests verify both graceful fallback to CLR property names and that the global `PropertyNamingPolicy` is prevented from applying when a custom derived naming-policy attribute is present. <!-- START COPILOT ORIGINAL PROMPT --> <details> <summary>Original prompt</summary> ---- *This section details on the original issue you should resolve* <issue_title>STJ should allow setting naming policies on the member level.</issue_title> <issue_description>## Motivation System.Text.Json supports user-defined naming policies for properties/fields which can be specified via the `JsonSerializerOptions.PropertyNamingPolicy`. Today such policies can only be specified globally which removes the ability to apply granular policies on the individual type/property/field level. ## API Proposal ```csharp namespace System.Text.Json.Serialization; [AttributeUsage(AttributeTargets.Class | AttributeTargets.Struct | AttributeTargets.Interface | AttributeTargets.AttributeTargets.Property | AttributeTargets.Field, AllowMultiple = false)] public class JsonNamingPolicyAttribute : JsonAttribute { public JsonNamingPolicy(JsonKnownNamingPolicy namingPolicy); protected JsonNamingPolicy(JsonNamingPolicy namingPolicy); // protected ctor for user-defined extensibility public JsonNamingPolicy NamingPolicy { get; } } ``` ## API Usage ```C# JsonSerializerOptions options = new() { PropertyNamingPolicy = JsonNamingPolicy.SnakeCaseLower }; JsonSerializer.Serialize(new MyPoco(), options); // { "myFirstProperty" : "first", "my-second-property": "second" } [JsonNamingPolicy(JsonKnownNamingPolicy.CamelCase)] public class MyPoco { public string MyFirstProperty { get; set; } = "first"; [JsonNamingPolicy(JsonKnownNamingPolicy.KebabCaseLower)] public string MySecondProperty { get; set; } = "second"; } ``` cc @stephentoub</issue_description> ## Comments on the Issue (you are @copilot in this section) <comments> <comment_new><author>@eiriktsarpalis</author><body> FYI @jeffhandley @PranavSenthilnathan something to consider for .NET 10.</body></comment_new> <comment_new><author>@eiriktsarpalis</author><body> I personally prefer the explicit approach, ultimately it's a trade-off between writing less boilerplate and having an explicit contract.</body></comment_new> <comment_new><author>@stephentoub</author><body> If I have a class with 20 members, I'd rather put one attribute on the class then one on each of the 20 properties. As Eric says, it's a tradeoff between verbosity and explicitness.</body></comment_new> <comment_new><author>@stephentoub</author><body> > @stephentoub I think you make be missing my point about explicitness. I think using [JsonPropertyName()] is more explicit and intentional than applying a policy to an individual property. My point wasn't about global vs local. In #108232 (comment), you asked about the advantage of: ```C# [JsonNamingPolicy(JsonKnownNamingPolicy.KebabCaseLower)] public string MySecondProperty { get; set; } ``` vs ```C# [JsonPropertyName("my-second-property")] public string MySecondProperty { get; set; } ``` That's not the comparison that's interesting. The comparison that's interesting is: ```C# [JsonNamingPolicy(JsonKnownNamingPolicy.KebabCaseLower)] class MyClass { public string MyPropA { get; set; } public string MyPropB { get; set; } public string MyPropC { get; set; } public string MyPropD { get; set; } public string MyPropE { get; set; } public string MyPropF { get; set; } public string MyPropG { get; set; } public string MyPropH { get; set; } } ``` vs ```C# class MyClass { [JsonPropertyName("my-prop-a")] public string MyPropA { get; set; } [JsonPropertyName("my-prop-b")] public string MyPropB { get; set; } [JsonPropertyName("my-prop-c")] public string MyPropC { get; set; } [JsonPropertyName("my-prop-d")] public string MyPropD { get; set; } [JsonPropertyName("my-prop-e")] public string MyPropE { get; set; } [JsonPropertyName("my-prop-f")] public string MyPropF { get; set; } [JsonPropertyName("my-prop-g")] public string MyPropG { get; set; } [JsonPropertyName("my-prop-h")] public string MyPropH { get; set; } } ``` And there there's an obvious advantage of the former, that of brevity, maintenance, etc. That advantage is then weighed against the slightly more explicit nature of JsonPropertyName. I say slightly because both are explicit; it's just a question of whether a constant is provided or whether a constant is provided with a formula over it, e.g. "6" vs "x == 3 and the formula is 2 * x". JsonPropertyName is arguably a bit more explicit, which you see as an advantage. Hence, "it's a tradeoff between verbosity and explicitness". I don't believe I'm missing the point.</body></comment_new> <comment_new><author>@eiriktsarpalis</author><body> > Happy to share the PR. Feel free to create an implementation PR following the [approved API shape](https://github.com/dotnet/runtime/issues/108232#issuecomment-2386591306).</body></comment_new> <comment_new><author>@eiriktsarpalis</author><body> > The example you posted of comparing a single policy attribute at th... </details> <!-- START COPILOT CODING AGENT SUFFIX --> - Fixes #108232 <!-- START COPILOT CODING AGENT TIPS --> --- 🔒 GitHub Advanced Security automatically protects Copilot coding agent pull requests. You can protect all pull requests by enabling Advanced Security for your repositories. [Learn more about Advanced Security.](https://gh.io/cca-advanced-security) --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: eiriktsarpalis <2813363+eiriktsarpalis@users.noreply.github.com> Co-authored-by: Eirik Tsarpalis <eirik.tsarpalis@gmail.com> Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Description
JsonConverterAttributenow supports open generic converter types on generic types when the type parameter arity matches. This works for both the reflection-based provider and source generator.Example Usage
Changes
DefaultJsonTypeInfoResolver.Converters.cs: When converter type is an open generic with matching arity to
typeToConvert, automatically construct the closed generic converter type usingMakeGenericType. The reflection APIs (Type.GetGenericArguments()andType.MakeGenericType()) handle nested generic types implicitly. When an open generic converter is used on an incompatible type (e.g., non-generic type or arity mismatch), anInvalidOperationExceptionwith a contextual error message is thrown proactively rather than surfacing internalArgumentExceptionfromActivator.CreateInstance.ThrowHelper.Serialization.cs: Added
ThrowInvalidOperationException_IncompatibleOpenGenericConverterTypehelper for the new contextual error message.Strings.resx: Added
IncompatibleOpenGenericConverterTyperesource string: "The open generic converter type '{0}' is not compatible with type '{1}'. Ensure that the total number of generic type parameters on the converter matches the number on the target type."JsonSourceGenerator.Parser.cs:
GetTotalTypeParameterCount()to count all type parameters including those from containing types for nested genericsConstructNestedGenericType()to properly construct closed generic types for nested generic converters (e.g.,Container<>.NestedConverter<>)typeToConvertthrough converter attribute resolution to enable open generic detectionINamedTypeSymbolonly exposesTypeParametersfor the immediate type, unlike reflection'sType.GetGenericArguments()which returns all type parameters across the hierarchyTests: Comprehensive coverage across reflection, source generator integration, and source generator unit tests including:
Option<T>withOptionConverter<T>)Result<TValue, TError>withResultConverter<TValue, TError>)Container<>.NestedConverter<>)OuterGeneric<>.MiddleNonGeneric.InnerConverter<>)NonGenericOuter.SingleLevelConverter<>)Level1<,,>.Level2<>.Level3Converter<>with 5 params split 3+1+1)where T : class)[Theory]with[InlineData]where applicable per codebase conventionsSupported Scenarios
[JsonConverter(typeof(OptionConverter<>))]onOption<T>[JsonConverter(typeof(ResultConverter<,>))]onResult<TValue, TError>[JsonConverter(typeof(Container<>.NestedConverter<>))]onTypeWithNestedConverter<T1, T2>— type parameters matched in order of definition regardless of nestedness[JsonConverter(typeof(OuterGeneric<>.MiddleNonGeneric.InnerConverter<>))]— generic within non-generic within generic[JsonConverter(typeof(Level1<,,>.Level2<>.Level3Converter<>))]— each nesting level consumes a different number of type parameters (3+1+1)[JsonConverter(typeof(NonGenericOuter.SingleLevelConverter<>))]ConverterWithClassConstraint<T> where T : classError Handling
InvalidOperationExceptionwith a contextual error message explaining the incompatibility between the open generic converter and the target typeInvalidOperationExceptionwith contextual error message at runtime; source generator emits warning diagnosticArgumentExceptionat runtime (fromMakeGenericType)InvalidOperationExceptionat runtime (converter doesn't convert the target type)InvalidOperationExceptionat runtime because the constructed converter (e.g.,SwappedConverter<int, string>convertingMyType<string, int>) doesn't match the expected converter type for the targetOriginal prompt
JsonConverterAttributeshould support generic converters on generic types #123208💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.