Skip to content

Allow unescaped U+2028 and U+2029 in strings - #118

Open
youdie006 wants to merge 1 commit into
dpranke:mainfrom
youdie006:allow-line-separators-in-strings
Open

youdie006 wants to merge 1 commit into
dpranke:mainfrom
youdie006:allow-line-separators-in-strings

Conversation

@youdie006

Copy link
Copy Markdown

json5.loads rejects strings containing an unescaped U+2028 or U+2029, such as the output of JavaScript's JSON.stringify("a
b"), which json.loads accepts. The JSON5 spec lists both code points as valid string characters ("Like JSON, JSON5 allows the Unicode code points U+2028 and U+2029 to appear unescaped in strings"), and the reference json5 parser accepts them.

The string rules excluded every eol character; this adds the two code points as alternatives in json5.g and regenerates parser.py. The current glop doesn't emit the hand-added pos argument in Parser.__init__, so I kept that hunk as it was.

The tests cover both quote styles, a backslash before U+2028 still being a line continuation, and a raw CR still failing. Compared with json5 2.2.3 on json5-tests and about 33k generated strings, the only results that change are these cases, and all of them now match the reference. python run tests and python run check pass.

Written with AI assistance (Claude); I have reviewed the change.

The string rules excluded every eol character, so loads() rejected the
two separators that JSON and the JSON5 spec allow in strings, e.g. the
output of JSON.stringify for such a string.
@dpranke

dpranke commented Oct 2, 2026

Copy link
Copy Markdown
Owner

Interesting bug! I'm curious, how did you find this?

I think the fix looks correct, but this isn't the way I'd prefer it be written.

I'd argue that the problem is that the line in the grammar ~bslash ~squote ~eol anything:c is actually wrong (or at least misleading); it says that as long as something isn't a backslash, a single quote, or one of the EOL characters, it should be accepted, i.e., this is equivalent to ~(bslash | squote | eol) anything:c. And, while \u2028 is one of the EOL characters, it should be allowed in this context. So, the rule plus your fix is correct, but the underlying check really isn't; it's excluding too much and doesn't really reflect the structure of the grammar that well.

So, what you probably really want is

  • not a backslash
  • not a single quote
  • not (strict==True and 0x00-0x1f)

and then the rule would be

sqchar = bslash esc_char:c                                          -> c
       | bslash eol                                                 -> ''
       | ~(bslash | squote | ?(_strict) '0x00'..'\x1f') anything:c  -> c

(and you'd change the dqchar rule the same way).

Although, from a performance point of view, given the current implementation, it'd probably be slightly faster to have the line be:

sqchar = ~(bslash | squote | ?(_strict) '0x00'..'\x1f') anything:c  -> c
       | bslash esc_char:c                                          -> c
       | bslash eol                                                 -> ''

Since the common case is actually that rule, and not a backslash followed by something.

From your comment, it sounds like you actually figured out how to use glop, too, which is pretty above-and-beyond, kudos.

Given that, do you want to try to rewrite the fix the way I'm suggesting? I haven't actually coded this myself, so I'm not 100% sure I'm right, and I'd want to code it and run the tests to see :).

Separately, in the tests, please don't embed \u2028 or other non-printable characters directly in the test file, use the Python unicode escapes instead. The reason is that you can't tell by just looking at the file that the character is \u2028 and not just \u0020. The difference is critically important to the test, so it should be visible and obvious.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants