godotengine / godot

Godot Engine – Multi-platform 2D and 3D game engine
https://godotengine.org
MIT License
91.3k stars 21.23k forks source link

JSON.stringify (and JSON.print in 3.x) produces invalid JSON for INF/NaN floats #89777

Open wareya opened 8 months ago

wareya commented 8 months ago

Tested versions

Godot Engine v4.2.2.rc1.official.c7fb0645a

Also seen in an otherwise-unspecified 3.5 build by someone I know

System information

Windows 10, amd rx 480 gpu

Issue description

The JSON.stringify function (JSON.print in 3.x) produces illegal JSON if the given data contains INF or NaN floats.

JSON numbers must be finite real numbers, not arbitrary floating point numbers. This means that the JSON stringifier can produce text that the JSON parser cannot parse.

firefox_2024-03-22_06-12-16

The JSON stringifier should either produce an error, or use best-approximation values (e.g. DBL_MAX or +1e9999 or similar for INF, 0.0 or null for NaN, or strings saying "INF/-INF/NaN") when fed numbers that can't be stored as finite real numbers.

Steps to reproduce

Run the following code in the _ready() function of a new scene in the editor and hit the "run current scene" button:

    prints("json: ", JSON.stringify([1.0/0.0]))
    prints("json: ", JSON.stringify([sqrt(-1.0)]))

Observe this output in the debug console output:

json:  [inf]
json:  [nan]

Minimal reproduction project (MRP)

N/A

AThousandShips commented 8 months ago

It's partially clarified that the standard isn't entirely followed in the docs, but more details would be needed

This change would be complicated and would arguably be a compatibility breakage if we changed to conform closer to the standard

What we could do is add an optional argument for additional conformance

wareya commented 8 months ago

At the moment, affected outputs don't parse, so working projects won't get any real compatibility breakage. For example, the following code:

    var temp := JSON.stringify([sqrt(-1.0)])
    var out = JSON.parse_string(temp)

prints a runtime error and out contains null.

I think adding INF and NaN checks to the top of the Variant::FLOAT case in JSON::_stringify should paper over the issue without affecting any working projects. Code that only stringifies Vector2s etc. won't be affected because those types are JSONified as strings rather than numbers.

AThousandShips commented 8 months ago

Indeed there's no handling for those, so that would be possible

The discussion would be how to solve it, what direction to go

Malcolmnixon commented 8 months ago

Searching online shows this is a common complaint with JSON. The solutions seem to be:

  1. Just document the issue
  2. Convert any non-finite value to null
  3. Find numeric alternatives (-INF= -1e9999, INF = 1e9999, NAN = ??? no good option)
  4. Add new JSON keywords to encode NAN, INF, and -INF

The NAN, INF, and -INF values contain useful information, and my preference would be to allow round-tripping them through JSON. If we went down this path we would just have to decide what keyword we would use for this encoding.

According to https://evanhahn.com/pythons-nonstandard-json-encoding/ Python has the following non-standard extension which is supported by a few other parsers. I would propose we adopt this standard so we're at least compatible with something:

import json
import math

json.dumps([math.inf, -math.inf, math.nan])
# => "[Infinity, -Infinity, NaN]"

We may also decide to leave the choice to the user and make the non-finite handling an optional parameter to the JSON stringify method:

wareya commented 8 months ago

I think always using the numeric alternative for INF (1e9999 etc) and having an option for how to convert NaN (error? null? literal NaN keyword like Python?) would be a good compromise.