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

escaped characters not handled correctly in strings #6

Closed
Conduitry opened this issue Apr 2, 2018 · 12 comments · Fixed by #7
Closed

escaped characters not handled correctly in strings #6

Conduitry opened this issue Apr 2, 2018 · 12 comments · Fixed by #7

Comments

@Conduitry
Copy link
Collaborator

Conduitry commented Apr 2, 2018

If some json5 contains a string containing an escaped newline, in the parsed or evaluated version, that newline will be lost.

fleece.evaluate('"\\n"')

This returns an empty string. This also happens with strings containing \r or \t or \\.

Strings containing \x## or \u#### escape characters are also not handled correctly. The \x or \u is dropped and in the evaluated string the ## or #### is left as-is.

@Conduitry Conduitry changed the title Cannot parse strings containing newline escaped characters not handled correctly in strings Apr 2, 2018
@Conduitry
Copy link
Collaborator Author

It looks like the issue is that other escaped characters are not properly handled here.

@Conduitry
Copy link
Collaborator Author

Patching of strings so that they contain newlines is also problematic, as the newlines are simply copied into the patched string without escaping them.

@Rich-Harris
Copy link
Owner

Thanks — it looks like the bug is in fleece.parse — working on a fix

Rich-Harris added a commit that referenced this issue Apr 2, 2018
Rich-Harris added a commit that referenced this issue Apr 2, 2018
handle newlines correctly
@Conduitry
Copy link
Collaborator Author

It looks like that fix won't handle the \x## and \u#### cases - and it also doesn't look like it would handle the issue in patch.

@Conduitry
Copy link
Collaborator Author

Also I'm not 100% clear on what should be done with unknown escape sequences. With this change, golden-fleece will completely drop things like \q from strings. The normal js parser would treat that as simply q and the json parser would throw an error. I don't know what the json5 specs say about this.

@Rich-Harris Rich-Harris reopened this Apr 2, 2018
@Rich-Harris
Copy link
Owner

Ah yep, \x and \u still need handling. Shouldn't have closed this. \q is handled though — if it's not one of the escapeable characters (\t, \b, \n and so on) it just adds the character to the result.

@Conduitry
Copy link
Collaborator Author

Oh whoops I missed the value += escapeable[char] || char;.

I'm a bit more concerned about the patch/stringify issue though actually. In sveltejs/v2.svelte.dev#262 if you make any edits to the text area, it corrupts the json5 data by having actual newlines in the string.

@Rich-Harris
Copy link
Owner

Opened #12. Those characters won't get escaped upon stringification, but maybe that's okay?

@Conduitry
Copy link
Collaborator Author

I think that's good enough for now. If we run into issues or if it surfaces that this isn't json5-compliant, we can revisit - but for now that's definitely better than what there was before.

@Conduitry
Copy link
Collaborator Author

Actually the list of characters that json5 officially escapes was pretty easy to find. It might make sense to augment the list I added in #10 to include the couple of extra ones.

@Rich-Harris
Copy link
Owner

I think all the various cases are handled now (thanks!) — you think we're good to close this?

@Conduitry
Copy link
Collaborator Author

Yup I think everything should be covered now.

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 a pull request may close this issue.

2 participants