dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
15.38k stars 4.75k forks source link

Make the return type of JsonSerializerOptions.GetTypeInfo nullable. #81269

Closed eiriktsarpalis closed 1 year ago

eiriktsarpalis commented 1 year ago

Background and motivation

The STJ source generator's implementation of the JsonSerializerContext.GetTypeInfo and IJsonTypeInfoResolver.GetTypeInfo methods are currently duplicated:

public override global::System.Text.Json.Serialization.Metadata.JsonTypeInfo? GetTypeInfo(global::System.Type type)
{
    if (type == typeof(global::TestModel))
    {
        return this.TestModel;
    }

    if (type == typeof(global::System.Int32))
    {
        return this.Int32;
    }

    return null;
}

global::System.Text.Json.Serialization.Metadata.JsonTypeInfo? global::System.Text.Json.Serialization.Metadata.IJsonTypeInfoResolver.GetTypeInfo(global::System.Type type, global::System.Text.Json.JsonSerializerOptions options)
{
    if (type == typeof(global::TestModel))
    {
        return Create_TestModel(options, makeReadOnly: false);
    }

    if (type == typeof(global::System.Int32))
    {
        return Create_Int32(options, makeReadOnly: false);
    }

    return null;
}

The size of these methods increases with the number of generated types, so in the interest of size reduction we would at least want to eliminate the duplication. The first GetTypeInfo method could be implemented by replacing it with

public override global::System.Text.Json.Serialization.Metadata.JsonTypeInfo? GetTypeInfo(global::System.Type type)
    => Options.GetTypeInfo(type);

There is a however a snag, since JsonSerializerContext.GetTypeInfo needs to return null for unsupported types, whereas JsonSerializerOptions.GetTypeInfo is non-nullable and returns NotSupportedException for unsupported types.

API Proposal

namespace System.Text.Json;

public partial class JsonSerializerOptions
{
    public JsonTypeInfo GetTypeInfo(Type type);
+    public bool TryGetTypeInfo(Type type, [NotNullWhen(true)] out JsonTypeInfo? typeInfo);
}

API Usage

The source generator could then implement the first GetTypeInfo as follows:

public override global::System.Text.Json.Serialization.Metadata.JsonTypeInfo? GetTypeInfo(global::System.Type type)
{
    Options.TryGetTypeInfo(type, out JsonTypeInfo? result);
    return result;
}

Testing against our own source gen test projects, the change contributes to ~2.2% size reduction to generated source files and just over 1.2% to the test assemblies (which includes test code beyond just the source generated contexts). A minor yet measurable improvement.

Alternative Designs

  1. Do not ship the a new API and instead source generate

    public override global::System.Text.Json.Serialization.Metadata.JsonTypeInfo? GetTypeInfo(global::System.Type type)
    {
        try
        {
            return Options.GetTypeInfo(type);
        }
        catch (NotSupportedException)
        {
             return null;
        }
    }
  2. Make the existing GetTypeInfo method nullable:

     public partial class JsonSerializerOptions
    {
    -        public JsonTypeInfo GetTypeInfo(Type type);
    +        public JsonTypeInfo? GetTypeInfo(Type type);
    }   

    It would return null where the method currently throws, so technically this is not a breaking change but it would trigger nullability warnings in places where it previous didn't apply.

Risks

No response

ghost commented 1 year ago

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
### Background and motivation The STJ source generator's implementation of the `JsonSerializerContext.GetTypeInfo` and `IJsonTypeInfoResolver.GetTypeInfo` methods are currently duplicated: ```C# public override global::System.Text.Json.Serialization.Metadata.JsonTypeInfo? GetTypeInfo(global::System.Type type) { if (type == typeof(global::TestModel)) { return this.TestModel; } if (type == typeof(global::System.Int32)) { return this.Int32; } return null; } global::System.Text.Json.Serialization.Metadata.JsonTypeInfo? global::System.Text.Json.Serialization.Metadata.IJsonTypeInfoResolver.GetTypeInfo(global::System.Type type, global::System.Text.Json.JsonSerializerOptions options) { if (type == typeof(global::TestModel)) { return Create_TestModel(options, makeReadOnly: false); } if (type == typeof(global::System.Int32)) { return Create_Int32(options, makeReadOnly: false); } return null; } ``` The size of these methods increases with the number of generated types, so in the interest of size reduction we would at least want to eliminate the duplication. The first `GetTypeInfo` method could be implemented by replacing it with ```C# public override global::System.Text.Json.Serialization.Metadata.JsonTypeInfo? GetTypeInfo(global::System.Type type) => Options.GetTypeInfo(type); ``` There is a however a snag, since `JsonSerializerContext.GetTypeInfo` needs to return `null` for unsupported types, whereas `JsonSerializerOptions.GetTypeInfo` is non-nullable and returns `NotSupportedException` for unsupported types. ### API Proposal ```diff namespace System.Text.Json; public partial class JsonSerializerOptions { public JsonTypeInfo GetTypeInfo(Type type); + public bool TryGetTypeInfo(Type type, [NotNullWhen(true)] out JsonTypeInfo? typeInfo); } ``` ### API Usage The source generator could then implement the first `GetTypeInfo` as follows: ```C# public override global::System.Text.Json.Serialization.Metadata.JsonTypeInfo? GetTypeInfo(global::System.Type type) { Options.GetTypeInfo(type, out JsonTypeInfo? result); return result; } ``` Testing against our own source gen test projects, the change contributes to ~2.2% size reduction to generated source files and just over 1.2% to the test assemblies (which includes test code beyond just the source generated contexts). A minor yet measurable improvement. ### Alternative Designs _No response_ ### Risks _No response_
Author: eiriktsarpalis
Assignees: -
Labels: `api-suggestion`, `area-System.Text.Json`
Milestone: -
ghost commented 1 year ago

Tagging subscribers to 'size-reduction': @eerhardt, @SamMonoRT, @marek-safar See info in area-owners.md if you want to be subscribed.

Issue Details
### Background and motivation The STJ source generator's implementation of the `JsonSerializerContext.GetTypeInfo` and `IJsonTypeInfoResolver.GetTypeInfo` methods are currently duplicated: ```C# public override global::System.Text.Json.Serialization.Metadata.JsonTypeInfo? GetTypeInfo(global::System.Type type) { if (type == typeof(global::TestModel)) { return this.TestModel; } if (type == typeof(global::System.Int32)) { return this.Int32; } return null; } global::System.Text.Json.Serialization.Metadata.JsonTypeInfo? global::System.Text.Json.Serialization.Metadata.IJsonTypeInfoResolver.GetTypeInfo(global::System.Type type, global::System.Text.Json.JsonSerializerOptions options) { if (type == typeof(global::TestModel)) { return Create_TestModel(options, makeReadOnly: false); } if (type == typeof(global::System.Int32)) { return Create_Int32(options, makeReadOnly: false); } return null; } ``` The size of these methods increases with the number of generated types, so in the interest of size reduction we would at least want to eliminate the duplication. The first `GetTypeInfo` method could be implemented by replacing it with ```C# public override global::System.Text.Json.Serialization.Metadata.JsonTypeInfo? GetTypeInfo(global::System.Type type) => Options.GetTypeInfo(type); ``` There is a however a snag, since `JsonSerializerContext.GetTypeInfo` needs to return `null` for unsupported types, whereas `JsonSerializerOptions.GetTypeInfo` is non-nullable and returns `NotSupportedException` for unsupported types. ### API Proposal ```diff namespace System.Text.Json; public partial class JsonSerializerOptions { public JsonTypeInfo GetTypeInfo(Type type); + public bool TryGetTypeInfo(Type type, [NotNullWhen(true)] out JsonTypeInfo? typeInfo); } ``` ### API Usage The source generator could then implement the first `GetTypeInfo` as follows: ```C# public override global::System.Text.Json.Serialization.Metadata.JsonTypeInfo? GetTypeInfo(global::System.Type type) { Options.GetTypeInfo(type, out JsonTypeInfo? result); return result; } ``` Testing against our own source gen test projects, the change contributes to ~2.2% size reduction to generated source files and just over 1.2% to the test assemblies (which includes test code beyond just the source generated contexts). A minor yet measurable improvement. ### Alternative Designs _No response_ ### Risks _No response_
Author: eiriktsarpalis
Assignees: eiriktsarpalis
Labels: `area-System.Text.Json`, `api-ready-for-review`, `size-reduction`
Milestone: 8.0.0
eiriktsarpalis commented 1 year ago

Related to #77897

Sergio0694 commented 1 year ago

Love It! We do end up having quite a few types in these switches (last I checked it was well over a hundred), so this does sound like it should make a noticeable difference, happy to share numbers/deltas from our end once this is available in a preview package and we can try this out! 😄

bartonjs commented 1 year ago

Video

Based on our expectation of there being a low number of callers, and the method having only been added in the most recent version, we've recommended changing the existing method to allow a null return.

 public partial class JsonSerializerOptions
{
-        public JsonTypeInfo GetTypeInfo(Type type);
+        public JsonTypeInfo? GetTypeInfo(Type type);
} 
eiriktsarpalis commented 1 year ago

FYI @davidfowl @eerhardt ☝️

This will introduce nullability warnings in the aspnetcore components that do call into GetTypeInfo.

eiriktsarpalis commented 1 year ago

After experimentation, we have concluded that making the existing GetTypeInfo nullable is not desirable -- it is often the case that users need an accelerator method that guarantees a result or fails with a standardized error message. I've notified FXDC that we wish to go back to the original proposal:

namespace System.Text.Json;

public partial class JsonSerializerOptions
{
    public JsonTypeInfo GetTypeInfo(Type type);
+    public bool TryGetTypeInfo(Type type, [NotNullWhen(true)] out JsonTypeInfo? typeInfo);
}