Repository navigation
fix/json unicode offsets - #2659
jurgenvinju wants to merge 28 commits into
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2659 +/- ##
=======================================
Coverage 45% 45%
- Complexity 6807 6812 +5
=======================================
Files 844 844
Lines 68823 68865 +42
Branches 10038 10053 +15
=======================================
+ Hits 31415 31457 +42
Misses 35005 35005
Partials 2403 2403 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
I think there's some uncommitted file? |
|
Yes, there was an uncommitted file. Thanks |
|
DavyLandman
left a comment
DavyLandman
left a comment
There was a problem hiding this comment.
I think this looks good, but I have the feeling it's duplicating code we already have in the ColumnOffset code. I suspect we'll be able to reduce the size of this PR (and thus new code to maintain) if we can reuse that.
| // For every high surrogate we assume a low surrogate will follow, | ||
| // and we count only one of them for the character offset by increasing `shift` |
There was a problem hiding this comment.
this would mean that a broken pair causes offsets? is that fine?
There was a problem hiding this comment.
probably fine, but it is possible to make this code more robust against some common cases.
| * See the body of {@link JsonReader#fillBuffer(int minimum)} for the contract that we must satisfy and | ||
| * the preconditions we are given at every call to {@link #read(char[], int, int)}. | ||
| */ | ||
| public static class OriginTrackingReader extends FilterReader { |
There was a problem hiding this comment.
a lot of this code reminds me of the ColumnMaps and LineColumnOffsetMap (and its implementation ArrayLineOffsetMap). They've been migrated to rascal a while back, so maybe we can reuse that code here?
Perhaps the LineColumnOffsetMap needs to learn a few new features, but it has been used for quite a few years in rascal-lsp for a very similar job.
There was a problem hiding this comment.
The current solution is tightly integrated into the streaming/buffering semantics of the parser that we are re-using. We use internal/private counters and offsets from this implementation, and depend highly on the state and changes of state (when the buffer is being re-filled).
The point of all this is to keep a streaming semantics and not fill a buffer the size of the file, and try to be as fast as Gson can be. I don't understand yet what that would mean for using the LineColumnOffsetMap, and the mapping of lines and columns is not the challenge that this code solves.
The main challenge that is solved here is to find the start and end character offsets (UTF32) of every JSON node and field while the reused parser/reader is unwilling to give up this information. So we recover the information from its internal buffer on-the-go and synchronizing with buffer resets.
4aaad45 to
5dfa802
Compare
|
|
Fixes unicode offsets for the JSON parser/validator:
srckeyword fieldslocsemantics of vallang and Rascal (offset, length, line, column)This makes JSON parsers ready for use in an editor/UI context. From that perspective, this was a bug. From the "we had a reasonable JSON parser" perspective, this was an enhancement.
Instruments the OriginTrackingReader embedded in JSONValueReader to accurately deal with the presence of unicode surrogate pairs in the char buffer of the reader.
Note that unicode characters in comments are equally responsible for shifts in the offsets as unicode characters in string constants and field names.
posindex into buffer compensated for surrogate pairsThe current solution still streams quickly and scales freely to very long JSON content, very long lines in JSON content, and very long comments or strings in JSON content.