Pure GAP fallback for JSON parser - #39
Conversation
gap/utf8.gi and gap/parse.gi transcribe the picojson parser in src/picojson/picojson.h, gap_val::from_str in src/picojson/gap-traits.h, and getUTF8Char/outputUnicodeChar/FuncJSON_ESCAPE_STRING in src/json.cc, so that the two implementations accept and reject the same texts, produce the same values, and report the same errors. That includes picojson's relaxed reading of numbers and the C code's idiosyncratic UTF-8 decoding, both reproduced deliberately: behaviour should not depend on whether the kernel extension happened to compile. Speed comes from scanning with a TranslateString'd copy of the input and the kernel's Position, so that runs of ordinary bytes are copied in one slice rather than a character at a time, and from reading streams in chunks and seeking back afterwards. Two differences from the C code are on purpose. Input nested deeper than _JSON_MAX_DEPTH is rejected with an ordinary error instead of overflowing the C stack and taking GAP with it. And reading past the end of a string yields 0 rather than whatever follows the buffer, so escaping a string ending in a truncated UTF-8 sequence no longer crashes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
tst/testall.g now runs the whole test directory once per available implementation, so the GAP code is exercised even where the kernel extension is present. All the existing tests pass unchanged under both, including the ones asserting on exact error messages. tst/JSONTestSuite holds the test_parsing corpus of https://github.com/nst/JSONTestSuite (MIT). Of its 318 files, two nest 100000 levels deep and crash GAP through the kernel parser, so they are left out; the other 316 are checked against the expectations in tst/jsontestsuite.g, which also record the eleven number formats we accept although RFC 8259 does not. Since testall.g runs the file under each implementation against the same expectations, and the file also compares the implementations against each other directly, agreement between them is checked rather than assumed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #39 +/- ##
==========================================
+ Coverage 96.42% 97.59% +1.16%
==========================================
Files 6 10 +4
Lines 140 791 +651
==========================================
+ Hits 135 772 +637
- Misses 5 19 +14 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Of course this also interacts with PR #36 -- depending on which is merged first, we'll have to rebase. I also want to add some benchmark numbers to give a better feeling how much slower this is. But for many of my use cases, the difference is irrelevant, and having a slow-yet-good-enough JSON parser beat not having a fast JSON parser (because it wasn't compiled by the user, with the user having no idea how to even do that) |
|
If I understand this proposal right then the ideas are
Concerning the latter statement, some comments in the new code give the impression that picojson has some bugs and limitations (?), and that the new GAP code on purpose "emulates" these problems. From a technical point of view, once the JSON package need not be compiled anymore I will turn it into a needed package of AtlasRep and CTblLib; then I can remove my JSON parser (which is in fact older than the JSON package) from AtlasRep. My idea for AtlasRep and CTblLib had been that I did not want a dependency on a package that needs compilation. |
|
@ThomasBreuer thanks for the comments. Regarding the bug-by-bug compatibility: I wanted to avoid the problem that people get different behaviour depending on which parser is active, that leads to a lot of pain. I'd be happy to look into fixing some of those, though, but then ideally in both the GAP and C++ version. One "bug" is important for us, though: JSON does not have big integers, but our parsers both do support them -- and there is code out there relying on this. So we should keep this, and make sure we document this as an intentional feature people can rely on. BTW I forgot to tag @ChrisJefferson of course I also want to hear from him if he thinks this PR, and also the stuff we are discussing now, are moving in a direction he is happy with. |
|
From my memory the things not supported are big integers (we should truncate them to floats, that would obviously be stupid in GAP), and some very, very unusual stuff (like BOMs, which I suppose we could support). |
|
Concerning big integers, I would say that supporting arbitrary large integers is a feature not a bug. Well, there would be a problem if we promise that the JSON format files which we create for GAP can be interpreted "correctly" by any JSON parser. |
3 participants
This is a pure GAP JSON parser backend, which is used if the kernel extension is not loaded.
Marked as a draft as I did not yet have a chance to review the code (which Claude Opus 5 created). Also, it would make sense to also analyze and fix the crashes in the C++ code, but I am out of tokens for the next couple hours 😂
And perhaps the test suite should be imported separately, and first... (incidentally, this resolves #4)
Part of the ArtifactManager effort.
CC @ThomasBreuer @lgoettgens