Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add ReadOnlySpan<char> overloads to JsonSerializer.Deserialize #1235

Closed
ahsonkhan opened this issue Jan 1, 2020 · 1 comment · Fixed by #41957
Closed

Add ReadOnlySpan<char> overloads to JsonSerializer.Deserialize #1235

ahsonkhan opened this issue Jan 1, 2020 · 1 comment · Fixed by #41957
Labels
api-approved API was approved in API review, it can be implemented area-System.Text.Json
Milestone

Comments

@ahsonkhan
Copy link
Member

We already have string-based overloads along with UTF-8 byte span overloads. We should add ReadOnlySpan<char> based overloads too to avoid forcing folks who already have string span slices to unnecessary call ToString().

On such use case is in https://stackoverflow.com/questions/58845880/how-to-deserialize-readonlyspanchar-to-object-using-system-text-json

Add:

public static partial class JsonSerializer
{
    public static object? Deserialize(ReadOnlySpan<char> json, Type returnType, JsonSerializerOptions? options = null);
    [return: MaybeNull]
    public static TValue Deserialize<TValue>(ReadOnlySpan<char> json, JsonSerializerOptions? options = null);
}

Existing:

public static partial class JsonSerializer
{
    public static object? Deserialize(ReadOnlySpan<byte> utf8Json, Type returnType, JsonSerializerOptions? options = null);
    public static object? Deserialize(string json, Type returnType, JsonSerializerOptions? options = null);
    public static object? Deserialize(ref Utf8JsonReader reader, Type returnType, JsonSerializerOptions? options = null);
    public static ValueTask<object?> DeserializeAsync(Stream utf8Json, Type returnType, JsonSerializerOptions? options = null, CancellationToken cancellationToken = default);
    public static ValueTask<TValue> DeserializeAsync<TValue>(Stream utf8Json, JsonSerializerOptions? options = null, CancellationToken cancellationToken = default);
    [return: MaybeNull]
    public static TValue Deserialize<TValue>(ReadOnlySpan<byte> utf8Json, JsonSerializerOptions? options = null);
    [return: MaybeNull]
    public static TValue Deserialize<TValue>(string json, JsonSerializerOptions? options = null);
    [return: MaybeNull]
    public static TValue Deserialize<TValue>(ref Utf8JsonReader reader, JsonSerializerOptions? options = null);
}

It is arguable that the string overload was mainly added for usability and folks who care about performance should really be using the UTF-8 overloads instead of us adding ReadOnlySpan<char> overloads (essentially, wherever they got the string/UTF-16 span from, they should replace try to get the UTF-8 bytes or stream instead). For example, instead of doing File.ReadAllText, pass in the FileStream directly.

However, we have added such overloads to APIs that accept string elsewhere, and there is still value for scenarios where the input JSON cannot be changed to something other than string.

@ahsonkhan ahsonkhan added this to the 5.0 milestone Jan 1, 2020
@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added the untriaged New issue has not been triaged by the area owner label Jan 1, 2020
@ahsonkhan ahsonkhan removed the untriaged New issue has not been triaged by the area owner label Jan 1, 2020
@layomia layomia modified the milestones: 5.0.0, Future Jun 20, 2020
@carlossanlop carlossanlop added api-ready-for-review API is ready for review, it is NOT ready for implementation and removed api-ready-for-review labels Jul 6, 2020
@terrajobst terrajobst added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for review, it is NOT ready for implementation labels Jul 24, 2020
@terrajobst
Copy link
Member

terrajobst commented Jul 24, 2020

Video

  • Looks good as proposed
public static partial class JsonSerializer
{
    public static object? Deserialize(ReadOnlySpan<char> json, Type returnType, JsonSerializerOptions? options = null);
    [return: MaybeNull]
    public static TValue Deserialize<TValue>(ReadOnlySpan<char> json, JsonSerializerOptions? options = null);
}

@ghost ghost locked as resolved and limited conversation to collaborators Dec 11, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.Text.Json
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants