[Thoth.Json.Core] Negative integers are loosing their sign when encoding them

Extracted from #175

@njlr

Perhaps this is a good time to make a breaking change?

#r "nuget: System.Text.Json, 8.0.3"

open System.Text.Json

let json = JsonSerializer.Serialize(-7s)

printfn $"%s{json}"

// -7

I don't think users would expect overflow for Thoth.Json.System.Text.Json (needs a better name! 😄 )

@MangelMaxime

Oh you mean, I made a mistakes by converting a int16 into an unsigned int which don't accept negative values?

Depends on what you mean by breaking changes, the JSON representation cannot be changed compared to Thoth.Json because I know some people who stored the JSON databases meaning we need to be backward compatible at least for the JSON representation. API wise, it is possible to have breaking changes indeed Thoth.Json.Core is released as 0.x which is a beta version.

@njlr

Oh you mean, I made a mistakes by converting a int16 into an unsigned int which don't accept negative values? Depends on what you mean by breaking changes, the JSON representation cannot be changed compared to Thoth.Json because I know some people who stored the JSON databases meaning we need to be backward compatible at least for the JSON representation. API wise, it is possible to have breaking changes indeed Thoth.Json.Core is released as 0.x which is a beta version.

On this type:

[<RequireQualifiedAccess; NoComparison>]
type Json =
    | String of string
    | Char of char
    | DecimalNumber of float
    | Null
    | Boolean of bool
    | Object of (string * Json) seq
    | Array of Json[]
    // Thoth.Json as an abritrary limit to the size of numbers
    | IntegralNumber of uint32
    | Unit

I wonder where a negative int32 e.g. -7 should live?

IntegralNumber loses the sign
DecimalNumber loses the fact it is a whole number

It's interesting to see how other libraries handle this.

yojson allows for an int32 or a float: ocaml-community/yojson@4c1d4b5/lib/type.ml#L3-L33

FSharp.Data uses decimal or float: fsprojects/FSharp.Data@674cacf/src/FSharp.Data.Json.Core/JsonValue.fs#L37-L46

Newtonsoft has JTokenType.Integer, which is used to represent int32, int64, ulong, ... JamesNK/Newtonsoft.Json@55a7204/Src/Newtonsoft.Json/Linq/JValue.cs#L105-L113

The JSON spec leaves it up to the implementation 🤷

The reason for | IntegralNumber of uint32 is because Thoth.Json (legacy) use

let inline sbyte (value: sbyte) : JsonValue = box value

let inline byte (value: byte) : JsonValue = box value

let inline int16 (value: int16) : JsonValue = box value

let inline uint16 (value: uint16) : JsonValue = box value

let inline int (value: int) : JsonValue = box value

let inline uint32 (value: uint32) : JsonValue = box value

let int64 (value: int64) : JsonValue =
    box (value.ToString(CultureInfo.InvariantCulture))

let uint64 (value: uint64) : JsonValue =
    box (value.ToString(CultureInfo.InvariantCulture))

And I made the mistake of think that I could enforce the arbitrary limit of numbers by using the bigger number representation here uint32 but I forgot that this type is unsigned which cause issues when casting a negative number to it.

We could decide to change | IntegralNumber of uint32 to | IntegralNumber of int64 which would solve the problem but could cause trouble because then the max size up to which we output a JSON number and not a string is not enforced anymore.

Or we could decide to split it into 2 cases:

| SignedIntegralNumber of int16
| UnsignedIntegralNumber of uint32

I believe the second option is aligned with the legacy behaviour and also have the benefit of enforcing it in the domain directly.

thoth-org / Thoth.Json

[Thoth.Json.Core] Negative integers are loosing their sign when encoding them #187