Provide trim/AOT-safe mechanism to (de)serialize enums from strings with JSON source-gen

[layomia]

See #79311 (comment) for an updated API proposal.

Original Proposal

Background and Motivation

The JsonStringEnumConverter class is a built-in JsonConverterFactory that is not compatible with NativeAOT, since it relies on Type.MakeGenericType() to work. We need to update the source generator so that it intercepts [JsonConverter(typeof(JsonStringEnumConverter))] annotations and replaces them with a NativeAOT-safe factory method invocation.

Should be addressed in conjunction with #81833.

API Proposal

namespace System.Text.Json.Serialization.Metadata;

public static class JsonMetadataServices
{
    public static JsonConverter<T> GetEnumConverter<T>(JsonSerializerOptions options) where T : struct, Enum
+   public static JsonConverter<T> GetStringEnumConverter<T>(JsonStringEnumConverter converterFactory, JsonSerializerOptions options) where T : struct, Enum
}

Alternative Design

We could add the generic factory method on the converter factory itself:

public class JsonStringEnumConverter
{
    public JsonConverter CreateConverter(Type typeToConvert, JsonSerializerOptions options);
+   public JsonConverter<T> CreateConverter<T>(JsonSerializerOptions options) where T : struct, Enum
}

However I would prefer if this call were hidden behind the EditorBrowsable.Never JsonMetadataServices class.

Edited by @eiriktsarpalis

Original Proposal

Forking from https://github.com//issues/73124 which goes over a friendly, general pattern for JsonConverterFactory usage with source-gen.

This issue focuses on a plan to support [JsonStringEnumConverter] (or an alternate approach), the primary scenario needed in 8.0 for the ASP.NET effort for AOT friendlinessfriendliness (JSON items tracked by #79122).

[ghost]

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

Issue Details

---

Forking from #73124 which goes over a friendly, general pattern for JsonConverterFactory usage with source-gen.

This issue focuses on a plan to support [JsonStringEnumConverter] (or an alternate approach), the primary scenario needed in 8.0 for the ASP.NET effort for AOT friendliness.

Author:	layomia
Assignees:	-
Labels:	area-System.Text.Json, trimming-for-aot, source-generator, partner-impact
Milestone:	8.0.0

[Sergio0694]

Curious about this since we're also deserializing a bunch of enums from strings in the Store. The way we're currently doing this (to keep backwards compatibility with Newtonsoft.Json) is by having a custom ResilientStringEnumConverter<T> that implements a Newtonsoft-compatible (ie. case invariant and also supporting reading from numbers) converter for enums. Then we use it like so:

[JsonConverter(typeof(ResilientStringEnumConverter<SomeEnum>))]
enum SomeEnum
{
    A,
    B,
    C
}

This works just fine including with trimming (we're using .NET Native and it works with no issues as far as we can tell), and by inspecting the generated code we can see that in fact the generator is creating an instance of each specialized converter for each property of one of our enum types.

So just to understand this better, is the goal of this issue to add support for this in a way that it also works when eg. passed via a non-generic context (eg. via the options) or when annotating an enum type with the non-generic converter type? As in, it seems like this is otherwise already supported and working fine with trimming on AOT as long as you annotate each enum type with the correct generic converter already? 🤔

Just trying to get a better understanding on this 🙂
Thank you!

[layomia]

The goal is to have usage of [JsonStringEnumConverter] work well with AOT. The issue is that converter factories often create converter instances using reflection (problem for AOT). This is most commonly due to needing to fill a generic T (the type to (de)serialize, in this case enums) dynamically at runtime.

[Sergio0694]

"The goal is to have usage of [JsonStringEnumConverter] work well with AOT."

Just to elaborate on what I meant - it seems to me there's two different issues with respect to enum serialization, which don't necessarily need the same solution, as it doesn't have the same complexity. What I meant is that:

• If you want to support just adding JsonStringEnumConverter to the JSON options and have it "just work" at runtime on any enum type while still being trimmable and needing no reflection, then yes some new factory API is needed for this.
• If you just want to support this over specific enum types or properties, I don't think anything else is needed other than just making JsonStirngEnumConverter<T> public. That's already fully trimmable and with no reflection needed. In fact you can already do this today by just writing your own enum converter that works the same way (we're doing this extensively in the Microsoft Store, for instance).

For point 2, that is, like I mentioned in my previous example, if you have:

[JsonConverter(typeof(JsonStringEnumConverter<MyEnum>))]
public enum MyEnum { A, B }

Then the source generator will just generate code to construct a new JsonStringEnumConverter<MyEnum> converter at runtime, which is already fully trimmable since the generic type is already statically constructed. Eg. in the Store we have our own CaseInvariantEnumConverter<T> and ResilientCaseInvariantEnumConverter<T> types which we added as annotations over all our enum types as needed, and that works just fine with trimming and AOT.

To clarify - I'm not saying that a general factory pattern isn't needed, but just that in case it might be useful to break this work down into smaller units, the second part can be made to work without the need for any new APIs/patterns, is all 🙂

[eiriktsarpalis]

I've updated the OP to include the new API necessary to make string enum serialization work.

[eiriktsarpalis]

To clarify - I'm not saying that a general factory pattern isn't needed, but just that in case it might be useful to break this work down into smaller units, the second part can be made to work without the need for any new APIs/patterns, is al

I don't think we could solve this without any new APIs. The underlying converter implementation, EnumConverter<T> is currently internal and is used both by the built-in enum converter factory and JsonStringEnumConverter. In order to make migration to NativeAOT easier, the proposed fix is to have the source generator identify and intercept [JsonConverter(typeof(JsonStringEnumConverter))] annotations replacing them with calls to a generic factory method.

[Sergio0694]

Yup! To clarify, I didn't mean "no public API changes at all", I meant that making the existing EnumConverter<T> public would've also fixed the issue (as in, by just changing visibility of that type, but without needing to implement new functionality per se. I like the new proposal of just exposing a generic hook for the generator to identify [JsonStringEnumConverter] a lot, as it's pretty much the same (ie. the generator conceptually emits a strongly-typed new SomeConverter<TheEnumType>() call, but it's even easier to use as you don't need to type the type argument yourself when annotating the enum type. Love it! 😄

[terrajobst]

• Looks good as proposed

namespace System.Text.Json.Serialization.Metadata;

public static class JsonMetadataServices
{
    // Existing:
    // public static JsonConverter<T> GetEnumConverter<T>(JsonSerializerOptions options) where T : struct, Enum;

    public static JsonConverter<T> GetStringEnumConverter<T>(JsonStringEnumConverter converterFactory, JsonSerializerOptions options) where T : struct, Enum
}

[eerhardt]

FYI - I ran into this issue trying to make a piece of YARP AOT-compatible: microsoft/reverse-proxy#2145.

[eiriktsarpalis]

Per discussion in #87149 (comment) it looks like using the existing JsonStringEnumConverter factory in AOT is not viable since it will make the compiler warn even in cases where the unsafe factory method is not exercised. It seems we need to pivot to a separate converter design:

API proposal

namespace System.Text.Json.Serialization;

[RequiresDynamicCode]
public partial class JsonStringEnumConverter : JsonConverterFactory 
{ 
    public JsonStringEnumConverter();
    public JsonStringEnumConverter(JsonNamingPolicy? namingPolicy = null, bool allowIntegerValues = true);

    public override bool CanConvert(Type type) => type.IsEnum;
}

+public class JsonStringEnumConverter<TEnum> : JsonConverterFactory
+    where TEnum : struct, Enum
+{
+    public JsonStringEnumConverter();
+    public JsonStringEnumConverter(JsonNamingPolicy? namingPolicy = null, bool allowIntegerValues = true);
+
+    public override bool CanConvert(Type type) => type == typeof(TEnum);
+}

API Usage

public class MyPoco
{
    [JsonConverter(typeof(JsonStringEnumConverter<BindingFlags>))]
    public BindingFlags Flags { get; set; }
}

We should also include a diagnostic in the source generator that prompts users to replace instances of the old JsonStringEnumConverter with the generic variant.

[terrajobst]

Video

• Looks good as proposed
• While we can register the convert globally, there is no good way to ensure all enums are registered

namespace System.Text.Json.Serialization;

// Existing:
//
// [RequiresDynamicCode]
// public partial class JsonStringEnumConverter : JsonConverterFactory 
// { 
//     public JsonStringEnumConverter();
//     public JsonStringEnumConverter(JsonNamingPolicy? namingPolicy = null, bool allowIntegerValues = true);
// 
//     public override bool CanConvert(Type type) => type.IsEnum;
// }

public class JsonStringEnumConverter<TEnum> : JsonConverterFactory
    where TEnum : struct, Enum
{
    public JsonStringEnumConverter();
    public JsonStringEnumConverter(JsonNamingPolicy? namingPolicy = null, bool allowIntegerValues = true);

    public override bool CanConvert(Type type) => type == typeof(TEnum);
}