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 BigInteger support to System.Text.Json #60780

Open
eiriktsarpalis opened this issue Oct 22, 2021 · 19 comments
Open

[API Proposal]: Add BigInteger support to System.Text.Json #60780

eiriktsarpalis opened this issue Oct 22, 2021 · 19 comments
Assignees
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Text.Json wishlist Issue we would like to prioritize, but we can't commit we will get to it yet
Milestone

Comments

@eiriktsarpalis
Copy link
Member

eiriktsarpalis commented Oct 22, 2021

Background and motivation

Currently System.Text.Json has no support for serializing BigInteger values. This is a proposal for serializing BigInteger values as arbitrarily sized JSON numbers.

API Proposal

namespace System.Text.Json
{
    public ref partial struct Utf8JsonReader
    {
+        public BigInteger GetBigInteger();
+        public bool TryGetBigInteger(out BigInteger value);
    }

    public sealed partial class Utf8JsonWriter
    {
+        public void WriteNumber(JsonEncodedText propertyName, BigInteger value);
+        public void WriteNumber(string propertyName, BigInteger value);
+        public void WriteNumber(ReadOnlySpan<char> propertyName, BigInteger value);
+        public void WriteNumber(ReadOnlySpan<byte> utf8PropertyName, BigInteger value);
+        public void WriteNumberValue(BigInteger value);
    }

    public readonly partial struct JsonElement
    {
+        public BigInteger GetBigInteger();
+        public bool TryGetBigInteger(out BigInteger value)
    }
}

namespace System.Text.Json.Node
{
    public abstract partial class JsonNode
    {
+        public static explicit operator BigInteger(JsonNode value);
+        public static explicit operator BigInteger?(JsonNode? value);
+        public static implicit operator JsonNode(BigInteger value);
+        public static implicit operator JsonNode?(BigInteger? value);
    }
}

namespace System.Text.Json.Serialization.Metadata
{
    public static partial class JsonMetadataServices
    {
+        public static JsonConverter<BigInteger> BigIntegerConverter { get; }
    }
}

API Usage

JsonSerializer.Serialize((BigInteger)42);

// Current JSON: {"IsPowerOfTwo":false,"IsZero":false,"IsOne":false,"IsEven":true,"Sign":1}
// New JSON: 42

Alternative Designs

No response

Risks

No response

@eiriktsarpalis eiriktsarpalis added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Oct 22, 2021
@dotnet-issue-labeler dotnet-issue-labeler bot added area-System.Text.Json untriaged New issue has not been triaged by the area owner labels Oct 22, 2021
@ghost
Copy link

ghost commented Oct 22, 2021

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

Issue Details

Background and motivation

Currently System.Text.Json has no support for serializing BigInteger values. This is a proposal for serializing BigInteger values as arbitrarily sized JSON numbers.

API Proposal

namespace System.Text.Json
{
    public ref partial struct Utf8JsonReader
    {
+        public BigInteger GetBigInteger();
+        public bool TryGetBigInteger(out BigInteger value);
    }

    public sealed partial class Utf8JsonWriter
    {
+        public void WriteNumber(JsonEncodedText propertyName, BigInteger value);
+        public void WriteNumber(string propertyName, BigInteger value);
+        public void WriteNumber(ReadOnlySpan<char> propertyName, BigInteger value);
+        public void WriteNumber(ReadOnlySpan<byte> utf8PropertyName, BigInteger value);
+        public void WriteNumberValue(BigInteger value);
    }

    public readonly partial struct JsonElement
    {
+        public BigInteger GetBigInteger();
+        public bool TryGetBigInteger(out BigInteger value)
    }
}

namespace System.Text.Json.Node
{
    public abstract partial class JsonNode
    {
+        public static explicit operator BigInteger(JsonNode value);
+        public static explicit operator BigInteger?(JsonNode? value);
+        public static implicit operator JsonNode(BigInteger value);
+        public static implicit operator JsonNode?(BigInteger? value);
    }
}

namespace System.Text.Json.Serialization.Metadata
{
    public static partial class JsonMetadataServices
    {
+        public static JsonConverter<BigInteger> BigIntegerConverter { get; }
    }
}

API Usage

JsonSerializer.Serialize((BigInteger)42);

// Current JSON: {"IsPowerOfTwo":false,"IsZero":false,"IsOne":false,"IsEven":true,"Sign":1}
// New JSON: 42

Alternative Designs

No response

Risks

No response

Author: eiriktsarpalis
Assignees: -
Labels:

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

Milestone: -

@eiriktsarpalis eiriktsarpalis removed the untriaged New issue has not been triaged by the area owner label Oct 22, 2021
@eiriktsarpalis eiriktsarpalis added this to the 7.0.0 milestone Oct 22, 2021
@eiriktsarpalis
Copy link
Member Author

From @Tornhoof in #60763 (comment)

Fwiw, the recommendation for Numbers in JSON is usually ~ 54 bit. See https://datatracker.ietf.org/doc/html/rfc8259#section-6 Most Implementations work fine with 64bit, everything above is lottery.

That being said, Biginteger implements ISpanFormattable, which might be the better target for Implementation.

@eiriktsarpalis
Copy link
Member Author

From @tannergooding in #60763 (comment)

Fwiw, the recommendation for Numbers in JSON is usually ~ 54 bit.

Just noting that its 53-bits. Whole integers up to 2^53 (9007199254740992) can be accurately represented by a double-precision floating-point value. After that, you can only represent multiples of 2 (even numbers), up until the next power of two. This means that for values between 2^53 (9007199254740992) and 2^54 (18014398509481984) you cannot represent odd numbers and so you cannot represent 9,007,199,254,740,993 for example (it rounds down). This continues doubling every power of 2, so at 2^54 you can only represent multiples of 4, then 8 at 2^55, etc; all the way up to multiples of 2048 at 2^63; and then continuing up to double.MaxValue in the same pattern.

Sure, but I would interpret the wording in that section as a recommendation for improved interoperability rather than something mandated by the spec

I'd agree. While this is best for interoperability, its not required and as far as the spec is concerned larger numbers are valid, so there should be no "issue" in printing larger values from BigInteger. Handling "portability" recommendations would likely involve telling users to actually track a type and exclusively use strings for representing numbers (which also covers things like nan, infinity, negative zero, etc).

@eiriktsarpalis eiriktsarpalis self-assigned this Oct 22, 2021
@Tornhoof
Copy link
Contributor

I looked at other JSON implementations, apparently most simply write it as a number.
There is even https://www.npmjs.com/package/json-bigint, which bypasses the bigint in javascript problem.
So i withdraw my objections.

@jkotas
Copy link
Member

jkotas commented Oct 22, 2021

Is this going to make BigInteger rooted for trimming in any use of System.Text.Json?

@eiriktsarpalis
Copy link
Member Author

Is this going to make BigInteger rooted for trimming in any use of System.Text.Json?

Yes.

@jkotas
Copy link
Member

jkotas commented Oct 23, 2021

It means that this feature would introduce noticeable size regression for minimal Blazor apps where we care about size a lot. I doubt it would be acceptable. Features like this one have to be designed as opt-in.

cc @marek-safar

@eiriktsarpalis
Copy link
Member Author

eiriktsarpalis commented Oct 25, 2021

Definitely, and this is really a concern with any type we propose to support OOTB (e.g. #53539). I don't think this should be preventing us from adding OOTB support for new types altogether, but we should definitely be weighing in the value of each proposal on a case-by-case basis.

Features like this one have to be designed as opt-in.

I'm slightly concerned about the UX of needing to opt in to a valid serialization of a type we claim to support OOTB. Not supporting it at all is likely preferable (though I would need to check if a custom converter workaround is currently possible: it's not clear to me if Utf8JsonReader currently supports reading arbitrarily large JSON numbers).

@stephentoub
Copy link
Member

I don't think this should be preventing us from adding OOTB support for new types altogether, but we should definitely be weighing in the value of each proposal on a case-by-case basis.

I agree we need to come up with an alternate approach that enables us to add in such support without rooting everything. The current path isn't sustainable.

@jkotas
Copy link
Member

jkotas commented Oct 25, 2021

Another point to consider is that supporting new types like BigInteger is behavior breaking change. You can argue that nobody should be depending on the current non-sensical behavior and I accept that, but it just makes it more acceptable breaking change.

alternate approach that enables us to add in such support without rooting everything

Add support for new types for source generators only?

@layomia
Copy link
Contributor

layomia commented Oct 25, 2021

Add support for new types for source generators only?

Seems like a hard stance to take for right now as the feature stabilizes. However, this is the latest thinking based on 6.0 discussions: that the reflection serializer would be characterized as unfriendly for trimming by-design, and that folks that care about size should use the generator. cc @eerhardt


What's the motivation for the API proposal for .NET 7.0? Custom user converters can handle BigInteger. If best perf is required, we could pursue #54410 (non-allocating view on string props when reading) and leverage the new Utf8JsonWriter.WriteRawValue methods.

@eiriktsarpalis
Copy link
Member Author

eiriktsarpalis commented Oct 25, 2021

What's the motivation for the API proposal for .NET 7.0?

No particular motivation. I added it to the 7.0.0 milestone so that it can be considered during our planning. I should add this is merely transcribing the ask in #60763 into an API proposal.

@eiriktsarpalis
Copy link
Member Author

If best perf is required, we could pursue #54410 (non-allocating view on string props when reading)

Note that the particular use case would require extending the feature to also include JSON numbers.

@jkotas
Copy link
Member

jkotas commented Oct 25, 2021

Add support for new types for source generators only?

Seems like a hard stance to take for right now as the feature stabilizes

I do not think that the reflection-based serializers are a sustainable choice for the core platform. Reflection-based serializers come with too many issues (compatibility and performance in particular). Case in point: All older reflection-based serializers in the core platform are in sustained engineering mode, they are impossible to evolve.

@eiriktsarpalis
Copy link
Member Author

It means that this feature would introduce noticeable size regression for minimal Blazor apps where we care about size a lot. I doubt it would be acceptable. Features like this one have to be designed as opt-in.

cc @marek-safar

I'd like to revisit this concern. Nowadays, any trimmed application using the source generator sees the bulk of built-in converters being trimmed and components are kept on a pay-for-play basis. I don't think this should be a blocker for us introducing built-in support for types, asssuming they are sufficiently popular.

cc @eerhardt

@eerhardt
Copy link
Member

any trimmed application using the source generator sees the bulk of built-in converters being trimmed and components are kept on a pay-for-play basis

Note this is true for apps the only support the source generator and disable/disallow reflection fallback for Json serialization. However, Blazor WASM and Maui apps don't fall into that category. Those apps only trim the base libraries, but not the user code. Both app models still use reflection based serialization.

@eiriktsarpalis
Copy link
Member Author

Both app models still use reflection based serialization.

Interesting, thanks.

@eiriktsarpalis
Copy link
Member Author

Closing in favor of #87994

@eiriktsarpalis
Copy link
Member Author

Reopening since BigInteger was cut from #87994. When implementing we might want to consider implementing a max digit threshold when deserializing. Related to #55121, which however only applies to BigInteger implementations post .NET 7.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Text.Json wishlist Issue we would like to prioritize, but we can't commit we will get to it yet
Projects
None yet
Development

No branches or pull requests

6 participants