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

[API Proposal]: Add System.Text.Json built-in support for more numeric types #87994

Closed
eiriktsarpalis opened this issue Jun 24, 2023 · 6 comments · Fixed by #88962
Closed

[API Proposal]: Add System.Text.Json built-in support for more numeric types #87994

eiriktsarpalis opened this issue Jun 24, 2023 · 6 comments · Fixed by #88962
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-System.Text.Json
Milestone

Comments

@eiriktsarpalis
Copy link
Member

eiriktsarpalis commented Jun 24, 2023

Background and motivation

System.Text.Json does not currently have built-in support for Int128/UInt128 and Half.

API Proposal

namespace System.Text.Json.Serialization.Metadata;

public partial class JsonMetadataServices
{
    public static JsonConverter<Int128> Int128Converter { get; }
    public static JsonConverter<UInt128> UInt128Converter { get; }
    public static JsonConverter<Half> HalfConverter { get; }
}

API Usage

The JsonMetadataServices APIs are marked EditorBrowsable.Never and are only intended for use by the source generator.

Alternative Designs

Note that this proposes adding numeric type support on the converter level only and not on the Utf8JsonWriter/Utf8JsonReader types or the DOM types. Since the underlying types do not have support for large numbers representable in Int128 or BigInteger, we need to make use of other primitives. Examples of this can be found in this PR. Deserialization without incurring allocation also necessitates making this change.

Risks

Adding built-in support for new types risks regressing code size for apps that use both reflection and trimming such as Blazor. If folks think this is unacceptable, we might consider making these opt-in specifically for reflection, requiring users to manually touch the relevant converters:

var options = new JsonSerializerOptions {  Converters = { JsonMetadataServices.BigIntegerConverter } };

Although it goes without saying this would introduce inconsistent behavior between the two serializers.

Related to #86442

@eiriktsarpalis eiriktsarpalis added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Jun 24, 2023
@ghost ghost added the untriaged New issue has not been triaged by the area owner label Jun 24, 2023
@eiriktsarpalis eiriktsarpalis self-assigned this Jun 24, 2023
@eiriktsarpalis eiriktsarpalis added api-ready-for-review API is ready for review, it is NOT ready for implementation and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Text.Json untriaged New issue has not been triaged by the area owner labels Jun 24, 2023
@ghost
Copy link

ghost commented Jun 24, 2023

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

System.Text.Json does not currently have built-in support for Int128/UInt128, Half and BigInteger.

API Proposal

namespace System.Text.Json.Serialization.Metadata;

public partial class JsonMetadataServices
{
    public static JsonConverter<Int128> Int128Converter { get; }
    public static JsonConverter<UInt128> UInt128Converter { get; }
    public static JsonConverter<Half> HalfConverter { get; }
    public static JsonConverter<BigInteger> BigIntegerConverter { get; }
}

API Usage

The JsonMetadataServices APIs are marked EditorBrowsable.Never and are only intended for use by the source generator.

Alternative Designs

No response

Risks

Adding built-in support for new types risks regressing code size for apps that use both reflection and trimming such as Blazor. If folks think this is unacceptable, we might consider making these opt-in specifically for reflection, requiring users to manually touch the relevant converters:

var options = new JsonSerializerOptions {  Converters = { JsonMetadataServices.BigIntegerConverter } };

Although it goes without saying this would introduce inconsistent behavior between the two serializers.

Author: eiriktsarpalis
Assignees: eiriktsarpalis
Labels:

api-suggestion, area-System.Text.Json, untriaged

Milestone: -

@eiriktsarpalis eiriktsarpalis added this to the 8.0.0 milestone Jun 24, 2023
@eiriktsarpalis eiriktsarpalis added blocking Marks issues that we want to fast track in order to unblock other important work area-System.Text.Json labels Jun 24, 2023
@ghost
Copy link

ghost commented Jun 24, 2023

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

System.Text.Json does not currently have built-in support for Int128/UInt128, Half and BigInteger.

API Proposal

namespace System.Text.Json.Serialization.Metadata;

public partial class JsonMetadataServices
{
    public static JsonConverter<Int128> Int128Converter { get; }
    public static JsonConverter<UInt128> UInt128Converter { get; }
    public static JsonConverter<Half> HalfConverter { get; }
    public static JsonConverter<BigInteger> BigIntegerConverter { get; }
}

API Usage

The JsonMetadataServices APIs are marked EditorBrowsable.Never and are only intended for use by the source generator.

Alternative Designs

No response

Risks

Adding built-in support for new types risks regressing code size for apps that use both reflection and trimming such as Blazor. If folks think this is unacceptable, we might consider making these opt-in specifically for reflection, requiring users to manually touch the relevant converters:

var options = new JsonSerializerOptions {  Converters = { JsonMetadataServices.BigIntegerConverter } };

Although it goes without saying this would introduce inconsistent behavior between the two serializers.

Author: eiriktsarpalis
Assignees: eiriktsarpalis
Labels:

area-System.Text.Json, blocking, api-ready-for-review

Milestone: 8.0.0

@Sergio0694
Copy link
Contributor

More of a side question, but out of curiosity:

"The JsonMetadataServices APIs are marked EditorBrowsable.Never and are only intended for use by the source generator."

Why is that the case? I get that they're more advanced and specific APIs (in fact they're in a different namespace too), but why make them entirely not visible? I've found myself using them in a few cases, namely when writing custom converters. It's pretty handy to be able to get some specific built-in converters in those cases and then build your own custom converter on top of them, using composition. Would it make sense to consider making that type not hidden? 🤔

@eiriktsarpalis
Copy link
Member Author

The short answer is that these served as proxy APIs back when contract customization wasn't available. The plan is to eventually have the source generator retarget contract customization APIs only and obsolete JsonMetadataServices (although it's a missing a few pieces before that becomes possible).

@eiriktsarpalis
Copy link
Member Author

Punting BigInteger from the proposal since we'd probably need to implement a max digit cutoff in the API surface as well.

@bartonjs
Copy link
Member

bartonjs commented Jul 6, 2023

Video

Looks good as proposed. There was discussion to inspire a future change of accepting things based on the new numeric interfaces, but that's not for this issue. There was also a question of whether nint/nuint should be included, and the answer was 'no'.

namespace System.Text.Json.Serialization.Metadata;

public partial class JsonMetadataServices
{
    public static JsonConverter<Int128> Int128Converter { get; }
    public static JsonConverter<UInt128> UInt128Converter { get; }
    public static JsonConverter<Half> HalfConverter { get; }
}

@bartonjs bartonjs added api-approved API was approved in API review, it can be implemented and removed blocking Marks issues that we want to fast track in order to unblock other important work api-ready-for-review API is ready for review, it is NOT ready for implementation labels Jul 6, 2023
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Jul 16, 2023
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Jul 18, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Aug 18, 2023
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.

4 participants