JSON fixes #5511

CyberShadow · 2017-06-26T10:02:40Z

Please see the individual commits for details.

dlang-bot · 2017-06-26T10:02:41Z

Thanks for your pull request, @CyberShadow!

Bugzilla references

Fix	Bugzilla	Description
✓	5904	std.json parseString doesn't handle chars outside the BMP
✓	17553	std.json should not do UTF decoding when encoding JSON
✓	17555	[REG2.070.0] Control characters in JSON data are invalid and should cause an exception
✓	17556	std.json encodes non-BMP characters incorrectly with JSONOptions.escapeNonAsciiChars
✓	17557	std.json should not do UTF decoding when parsing

CyberShadow · 2017-06-26T10:19:22Z

@wilzbach Is CircleCI broken on stable?

wilzbach · 2017-06-26T11:03:14Z

Is CircleCI broken on stable?

Looks like it:

make: *** No rule to make target `../generated/linux/release/64/VERSION', needed by `../generated/linux/release/64/dmd'.  Stop.
make: *** Waiting for unfinished jobs....

aG0aep6G

Last commit (fix for issue 17557) looks bad. Rest looks good.

aG0aep6G · 2017-06-26T10:51:29Z

std/json.d

+{
+    import std.exception : assertThrown;
+
+    assertThrown(parseJSON("\"a\nb\""));


Could specify the type of the expected exception.

aG0aep6G · 2017-06-26T12:00:30Z

std/json.d

@@ -707,22 +707,36 @@ if (isInputRange!T && !isInfinite!T && isSomeChar!(ElementEncodingType!T))
    JSONValue root;
    root.type_tag = JSON_TYPE.NULL;

+    // UTF decoding is unnecessary when parsing JSON.
+    static if (is(T : const(char)[]))
+        alias Char = char;


This breaks stuff:

parseJSON("\"\U0001D11E\""); /* std.json.JSONException@std/json.d(1374): Illegal control character. (Line 1:3) */

The thing is that the rest of the code assumes dchar. While char converts to dchar, it doesn't always keep the same meaning. So stuff gets misinterpreted.

In this specific case, isControl is given a char that becomes a control character when converted to dchar. So it rings the alarm even though the encoded code point in the string is not a control character. Could also go the other way: When a control character is encoded with chars that don't look like control characters, it slips through (haven't checked if this can actually happen).

Good catch, fixed.

…valid and should cause an exception

Since the previous commit, it was only called from one place.

…h JSONOptions.escapeNonAsciiChars

…e BMP

CyberShadow · 2017-06-26T12:13:25Z

Could also go the other way: When a control character is encoded with chars that don't look like control characters, it slips through (haven't checked if this can actually happen).

Hmm, it's one thing to force decoding a D string just to avoid putting Unicode control characters in the JSON text (as that would result in non-conforming JSON)... but to force decoding JSON just so we detect and throw on Unicode control characters?

CyberShadow · 2017-06-26T12:28:24Z

Done.

CyberShadow · 2017-06-26T12:31:26Z

Done.

Said done thing (posted it on the wrong PR):

OK, well, I checked the latest JSON RFC (7159) and it explicitly states that we don't care about the Unicode control characters:

and the control characters (U+0000 through U+001F).

so, I'm going to amend this and look into disabling auto-decoding for encoding JSON as well.

aG0aep6G

The isControl thing is fixed. As far as I see, there are no other such issues. So, LGTM.

But it's rather fragile. Future code might wrongly assume that it's always dealing with dchars.

aG0aep6G · 2017-06-26T13:03:43Z

std/json.d

@@ -1160,7 +1161,7 @@ string toJSON(const ref JSONValue root, in bool pretty = false, in JSONOptions o
                case '\t':      json.put("\\t");        break;
                default:
                {
-                    import std.uni : isControl;
+                    import std.ascii : isControl;
                    import std.utf : encode;



I'd like to assert here that escapeNonAsciiChars is not set together with Char = char. If that could happen, we'd output a \u sequence for each char in a multibyte sequence (which would be completely wrong).

Done. Also discovered and fixed another issue (see last commit).

wilzbach · 2017-06-26T19:20:09Z

Hmm, then why is #5511 failing?

Bad karma? No it's due to dlang/dmd#6935

Fix: dlang/dmd#6941

std.json: Remove unused error parameter to appendJSONChar

145f279

dlang-bot added the Enhancement label Jun 26, 2017

wilzbach mentioned this pull request Jun 26, 2017

Fix #65 - the Dlang-Bot should use the 'Enhancement' label dlang/dlang-bot#84

Merged

aG0aep6G suggested changes Jun 26, 2017

View reviewed changes

CyberShadow added 5 commits June 26, 2017 12:05

Fix Issue 17555 - [REG2.070.0] Control characters in JSON data are in…

55aa34e

…valid and should cause an exception

std.json: Inline appendJSONChar

e44666f

Since the previous commit, it was only called from one place.

Fix Issue 17556 - std.json encodes non-BMP characters incorrectly wit…

941e293

…h JSONOptions.escapeNonAsciiChars

std.json: Refactor parsing Unicode character escapes into a new function

b339911

Fix Issue 5904 - std.json parseString doesn't handle chars outside th…

b23e7a4

…e BMP

CyberShadow force-pushed the pull-20170626-074035 branch from 2e900a9 to ca788f1 Compare June 26, 2017 12:10

dlang-bot added the Enhancement label Jun 26, 2017

Fix Issue 17557 - std.json should not do UTF decoding when parsing

226f8e0

CyberShadow force-pushed the pull-20170626-074035 branch from ca788f1 to 226f8e0 Compare June 26, 2017 12:21

dlang-bot added the Enhancement label Jun 26, 2017

JackStouffer added Bug Fix Needs Review labels Jun 26, 2017

aG0aep6G approved these changes Jun 26, 2017

View reviewed changes

CyberShadow mentioned this pull request Jun 26, 2017

Re-add has_public_example to circleci.sh for legacy compatibility #5514

Merged

CyberShadow added 2 commits June 26, 2017 18:52

Fix Issue 17553 - std.json should not do UTF decoding when encoding JSON

5031ff1

std.json: Fix handling ranges of non-dchars

71875c0

CyberShadow force-pushed the pull-20170626-074035 branch from 68149a3 to 71875c0 Compare June 26, 2017 18:53

dlang-bot added the Enhancement label Jun 26, 2017

wilzbach mentioned this pull request Jun 26, 2017

Don't re-add labels if they are already existent dlang/dlang-bot#97

Closed

MartinNowak approved these changes Jul 3, 2017

View reviewed changes

MartinNowak merged commit 8f85aaa into dlang:stable Jul 3, 2017

wilzbach mentioned this pull request Jul 6, 2017

Fix style (space between a .. b) in std/json.d #5560

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

JSON fixes #5511

JSON fixes #5511

CyberShadow commented Jun 26, 2017

dlang-bot commented Jun 26, 2017 •

edited

CyberShadow commented Jun 26, 2017

wilzbach commented Jun 26, 2017

aG0aep6G left a comment

aG0aep6G Jun 26, 2017

CyberShadow Jun 26, 2017

aG0aep6G Jun 26, 2017

CyberShadow Jun 26, 2017

CyberShadow commented Jun 26, 2017

CyberShadow commented Jun 26, 2017

CyberShadow commented Jun 26, 2017

aG0aep6G left a comment •

edited

aG0aep6G Jun 26, 2017

CyberShadow Jun 26, 2017

wilzbach commented Jun 26, 2017

JSON fixes #5511

JSON fixes #5511

Conversation

CyberShadow commented Jun 26, 2017

dlang-bot commented Jun 26, 2017 • edited

Bugzilla references

CyberShadow commented Jun 26, 2017

wilzbach commented Jun 26, 2017

aG0aep6G left a comment

Choose a reason for hiding this comment

aG0aep6G Jun 26, 2017

Choose a reason for hiding this comment

CyberShadow Jun 26, 2017

Choose a reason for hiding this comment

aG0aep6G Jun 26, 2017

Choose a reason for hiding this comment

CyberShadow Jun 26, 2017

Choose a reason for hiding this comment

CyberShadow commented Jun 26, 2017

CyberShadow commented Jun 26, 2017

CyberShadow commented Jun 26, 2017

aG0aep6G left a comment • edited

Choose a reason for hiding this comment

aG0aep6G Jun 26, 2017

Choose a reason for hiding this comment

CyberShadow Jun 26, 2017

Choose a reason for hiding this comment

wilzbach commented Jun 26, 2017

dlang-bot commented Jun 26, 2017 •

edited

aG0aep6G left a comment •

edited