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

Enable std.json.parseJSON to parse nan/inf. #3141

Merged
merged 5 commits into from Apr 14, 2015

Conversation

rcorre
Copy link
Contributor

@rcorre rcorre commented Apr 3, 2015

JSONValue.toString outputs 'nan', 'inf', or '-inf', when it has type
JSON_TYPE.FLOAT and value double.nan/double.infinity/-double.infinity.

However, parseJSON would fail on encountering these values in a string.
This patch enables parseJSON to read special float values.

I submitted this as issue #14399 a bit ago and then decided to take a shot at fixing it.

To be clear: this does not change the output of std.json.toString, which was already outputting special values for nan/inf. It simply allows std.json to read the special values that it outputs.

JSONValue.toString will output 'nan', 'inf', or '-inf', when it has type
JSON_TYPE.FLOAT and value double.nan/double.infinity/-double.infinity.
However, parseJSON would fail on encountering these values in a string.
This patch enables parseJSON to read special float values.
@aG0aep6G
Copy link
Contributor

aG0aep6G commented Apr 3, 2015

nan and inf don't seem to be valid JSON values. I think we shouldn't make our own JSON dialect by accepting them. Instead, I'd suggest to refuse the special values when constructing a JSONValue.

@rcorre
Copy link
Contributor Author

rcorre commented Apr 3, 2015

Fair enough, they aren't part of the official standard. However, I could at least see a use case for serializing inf, if not nan. Would it be more acceptable to serialize into the strings "nan", "inf", and "-inf"?

@aG0aep6G
Copy link
Contributor

aG0aep6G commented Apr 3, 2015

Would it be more acceptable to serialize into the strings "nan", "inf", and "-inf"?

I think JSONValue should be kept in sync with its JSON output. That is, let's not have JSON_TYPE.FLOAT values that serialize to strings.

We could let JSONValue(float.nan) etc construct string values. I.e. let them be the same as JSONValue("nan") etc.

I'd lean towards letting them throw, though. I think it's less surprising than getting strings back.

@rcorre
Copy link
Contributor Author

rcorre commented Apr 3, 2015

Someone on the bugzilla issue linked this go discussion that mentions a part of the json spec where it specifies that NaN and Inf should be represented as null, but the link to the spec they quote is dead.
Plus using null would lose information about whether it is NaN, Inf, or -Inf.

@rcorre
Copy link
Contributor Author

rcorre commented Apr 3, 2015

If we decide to throw, I have implemented that at (see rcorre@d2c1665).
However, I am in favor of finding some way to encode NaN/Inf even if it is not strictly 'standard'.

@aG0aep6G
Copy link
Contributor

aG0aep6G commented Apr 3, 2015

Someone on the bugzilla issue linked this go discussion that mentions a part of the json spec where it specifies that NaN and Inf should be represented as null, but the link to the spec they quote is dead.

Here's the (dead-)linked specification: http://www.ecma-international.org/ecma-262/5.1/

The quotes are from section 15.12.3 which describes a stringify function which returns a JSON fragment. So this is not about how JSON is defined, but about what JSON is produced by ECMAScript.

So we'd be in the good company of ECMAScript itself if we'd map nan and inf to null.

However, I am in favor of finding some way to encode NaN/Inf even if it is not strictly 'standard'.

I'm against adding new keywords to the JSON output (like foo: nan), because it wouldn't be proper JSON anymore.

Generating JSON strings (like foo: "nan") seems ok-ish to me. This would mean that a processor has to be aware of the convention and handle the cases accordingly. We can't let std.json map "nan" to double.nan, because the value may be meant to be the string "nan".

I'd still prefer to reject the special values, though. When I have to be aware of nan -> "nan" on the processing end, then the convenience on the generating end isn't worth much to me. Better to fail early, in my opinion.

@rcorre
Copy link
Contributor Author

rcorre commented Apr 5, 2015

I'd still prefer to reject the special values, though. When I have to be aware of nan -> "nan" on the processing end, then the convenience on the generating end isn't worth much to me.

Yeah, I agree here.
I'm not sure I'd like to encode as null either, as that loses the information of whether it was NaN or Inf.
I'll put in a PR for making them throw.

@MartinNowak
Copy link
Member

It's a separate option to enable this for the std.data.json proposal.
http://s-ludwig.github.io/std_data_json/stdx/data/json/lexer/lex_options.special_float_literals.html

@MartinNowak
Copy link
Member

  • Ruby × (has allow_nan option)
  • Python ✓
  • JS ✓

It think the best option here would be to make it configurable.

Adds the SpecialFloats flag to JSONValue.toString, JSONValue.toPrettyString,
and JSONValue.toJSON.
Given SpecialFloats.yes, the special float values NaN, Infinity, and -Infinity
will be encoded as the strings "NaN", "Infinite", "-Infinite".
Given SpecialFloats.no, attempting to stringify a JSONValue containing a
special float value will throw.
This overrides the previous behavior of encoding special float values as
non-string literals, which resulted in non-standard JSON strings.
Passing JsonSpecialFloats.yes to parseJSON prompts the parser to interpret
"NaN", "Infinite", and "-Infinite" as the special floating-point values they
represent.

Adds the overload parseJSON(JSONValue, SpecialFloats) that takes a JSONValue and
the JsonSpecialFloats flag so users do not have to pass maxDepth in order to
specify special float handling behavior.
@rcorre
Copy link
Contributor Author

rcorre commented Apr 13, 2015

I've updated the PR with optional encoding/decoding of special float values (defaults to 'no').
However, if std.data.json is planned to replace std.json soon, there may not be much point to this.

@MartinNowak
Copy link
Member

However, if std.data.json is planned to replace std.json soon, there may not be much point to this.

We hardly found a review manager yet. Better to fix things now than prolonging it in favor of something else to come.

/**
Flag that enables encoding special float values (NaN/Inf) as strings.
*/
alias JsonSpecialFloats = Flag!"JsonSpecialFloats";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The convention is alias JsonSpecialFloats = Flag!"jsonSpecialFloats";, not the small j, so that you can write JsonSpecialFloats.yes or Yes.jsonSpecialFloats.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's better if we start with an orable enum here, because it easily allowed to add more options without changing the API.

enum JsonOptions
{
    none,
    specialFloatLiterals = 0x1,
}

@MartinNowak
Copy link
Member

Looking good, thanks.

Replace Flag!"JsonSpecialFloats" with a flagset enum to allow future
extensibility.
Add more documentation to modified functions.
@rcorre
Copy link
Contributor Author

rcorre commented Apr 14, 2015

Updated docs and replaced Flag with a flagset enum JsonOptions.

@MartinNowak
Copy link
Member

Auto-merge toggled on

MartinNowak added a commit that referenced this pull request Apr 14, 2015
Enable std.json.parseJSON to parse nan/inf.
@MartinNowak MartinNowak merged commit c9122fc into dlang:master Apr 14, 2015
@MartinNowak
Copy link
Member

Thx

@rcorre rcorre deleted the json-nan-inf branch April 14, 2015 11:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants