quinnj / JSON3.jl

Other
214 stars 47 forks source link

Generated types not working for floats #198

Open dchansen opened 2 years ago

dchansen commented 2 years ago

JSON3 casts all floating points to integers, as long as it can be done without loss of precision. I.e.

test = "{ \"A\": 0.0 }"
x = JSON3.read(test)

JSON3.Object{Base.CodeUnits{UInt8, String}, Vector{UInt64}} with 1 entry:
  :A => 0

However this behaviour causes generated types to not work correctly:

JSON3.@generatetypes test
JSON3.read(test,JSONTypes.Root)

ERROR: ArgumentError: invalid JSON at byte position 9 while parsing type Main.JSONTypes.Root: ExpectedComma
{ "A": 0.0 }
mcmcgrath13 commented 2 years ago

@quinnj It looks like the generated struct looks like

mutable struct Root
  A::Int64
end

which seems reasonable to me, is this an error with the reader?

dchansen commented 2 years ago

I think the reader is right, and the struct should have had Float64 field. At least, that would have been the desired behavior in my cases, and I think it's reasonable not assume the presence of a decimal point to indicate a floating point number.

mcmcgrath13 commented 2 years ago

Because the reader says this is an Int64, generate types gets the type information from that. If the examples are all safely castable to Int, that's what generate types will see and propose as the type for the struct.

quinnj commented 2 years ago

This has been brought up a few times; I think probably the best way forward is to do one of two things, or maybe a combination of both:

mcmcgrath13 commented 2 years ago

I think I like option 1.

In this case we know that the number can be read as an Int, but when we try to read into an Int field on the struct, it errors. Any theories?

mo8it commented 2 years ago

I do think that types have to be preserved. That is why I support the first option, too.

visr commented 2 years ago

if we parse any number with "float-specific" characters (., e, etc.), we should not try to cast to integer

To JSON 1 and 1.0 is exactly the same number. From a Julia perspective this might look like a good idea, but a JSON minifier will happily cut off the .0, and thereby change the way it is parsed in JSON3, which might not be intended.

The current approach of trying to cast to integer is giving some related but different issues. For GeoJSON.jl we now lazily parse using JSON3.read(source). We don't use the struct API, because it is fast, and can be used as a starting point to convert geometries, which are nested JSON3.Array with a length 2 array for coordinates at the lowest level, to more functional geometry types from other packages. However now whenever a longitude falls on a whole number, interoperability is reduced since other packages cannot handle mixed-type coordinates (link). So the proposed option of numbertype=Float64 would be much more useful for this case. I'd even argue it should be the default, but that would be quite a breaking change, so perhaps would be better to avoid.