Eric Rogstad (Jul 11 2026 at 07:54): Eric Rogstad (Jul 11 2026 at 07:54):Hi All,
I was playing around with using Roc for a Zulip client of all things (for this project), when I stumbled upon a bug parsing JSON strings with escapes in them. I saw that there was already an issue for the bug, and that @Luke Boswell had tagged it good first issue, so I decided to take a crack at it myself (with the help of Claude).
Here's a PR that I believe correctly implements JSON escape parsing, and addresses all the desiderata in the issue Luke filed: https://github.com/roc-lang/roc/pull/10045.
But this is my first time attempting to contribute to roc-lang, so any feedback / pointers / tips are very much appreciated!
Cheers,
Eric
Eric Rogstad (Jul 11 2026 at 08:02):A few notes worth calling out (also mentioned in the PR):
Json.split_json_string_tail caused document parsing to be accidentally quadratic, due to a call to Str.find_first(tail, "\\") that would scan to the end of the document for every string field (in the case where no backslashes were found). Our implementation avoids ever scanning past the next (unescaped) quote character, and so stays linear (validated empirically). Eric Rogstad (Jul 14 2026 at 00:20):Hey @Luke Boswell, thanks for the review (and adding that extra check and merging the PR)!
I've got a follow-up to make a similar fix for parsing scalars: https://github.com/roc-lang/roc/pull/10145
This one's a little simpler, but let me know if you see any issues!
Luke Boswell (Jul 14 2026 at 02:21):Unicode whitespace set is wider than RFC 8259
What options do we have here? I vaguely remember going deep on this topic when I was building roc-json but I've since evicted all of that from memory... I would assume we want to align with the JSON standard here.
Luke Boswell (Jul 14 2026 at 02:23):Also I really appreciate the care you are taking with this, particularly with tracking the allocations and performance.
Eric Rogstad (Jul 14 2026 at 05:53):Yeah, I think aligning with the standard makes sense. In which case we can't let Str.trim_start silently drop some characters that should have caused us to reject the document.
One approach would be to replace calls to Str.trim_start with a json_trim_start function that only drops allowed whitespace, and then count on the calling code to fail when it tries to match disallowed whitespace against whatever it was expecting.
I've got a (Claude-generated) draft PR in my Roc fork that takes that approach: https://github.com/ESRogs/roc/pull/4.
(And I'm currently in the process of auditing that the calling code does in fact fail the disallowed whitespace.)
Does that approach sound reasonable? If so, I can promote it to a PR against the main repo.
Eric Rogstad (Jul 15 2026 at 19:56):@Luke Boswell does that approach of trimming only allowed whitespace and then counting on the rest of the parsing logic to fail to match on any remaining disallowed whitespace makes sense?
I had Claude audit all of the trim calls and it looks like it should work in every case.
If that makes sense to you, then I can just fold my https://github.com/ESRogs/roc/pull/4 PR into https://github.com/roc-lang/roc/pull/10145, and then that'll resolve the one open issue I'd flagged.
Luke Boswell (Jul 16 2026 at 00:03):Looks good to me... thank you for including the explanation
Luke Boswell (Jul 16 2026 at 00:04):Did you need me to merge that into your other branch so we keep that PR description or explanation around for reference later
Luke Boswell (Jul 16 2026 at 00:04):oh nvm, this is in your fork :smile:
Eric Rogstad (Jul 16 2026 at 01:12):Okay, I've folded that commit that was just into my fork into https://github.com/roc-lang/roc/pull/10145 and updated the PR description. I think it's ready for review / merge now.
Luke Boswell (Jul 16 2026 at 01:28):We can eliminate a List allocation for small strings if we implement a few of the planned builtins on Str.
I'm going to piggy back on top of your work here and add those as this will be a good test to demonstrate zero allocations for small and large strings.
Luke Boswell (Jul 16 2026 at 01:28):Do you mind if I push the commits to your branch, or would you prefer I make a PR back into your fork?
Luke Boswell (Jul 16 2026 at 01:40):Or even better... I might just merge your work if it's passing in CI and make a follow up PR
Luke Boswell (Jul 16 2026 at 02:26):Here's the stacked PR https://github.com/roc-lang/roc/pull/10173
Eric Rogstad (Jul 16 2026 at 02:35):Oh nice, I also had this stacked PR on my fork trying to achieve a similar goal. Haven't looked through yours to tell if mine is redundant yet though.
https://github.com/ESRogs/roc/pull/1
Eric Rogstad (Jul 16 2026 at 07:25):Okay, yeah, looks like that work is partly redundant now.
Here's a new draft PR that builds on yours and rewrites the string parsing (complementing your rewrite of the scalar parsing):
https://github.com/ESRogs/roc/pull/5
Eric Rogstad (Jul 17 2026 at 20:28):@Luke Boswell this one finally passed all its CI checks (Windows minici had kept timing out): https://github.com/roc-lang/roc/pull/10145
Okay to go ahead and merge it?
Eric Rogstad (Jul 17 2026 at 20:30):Then we can rebase our various other stacked PRs on top.
I'd suggest:
Last updated: Jul 23 2026 at 13:15 UTC