Fix surrogate pairs splitting in toText()/fromText() #1
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Resolves google#69 for JavaScript Sometimes we can find a common prefix that runs into the middle of a surrogate pair and we split that pair when building our diff groups. This is fine as long as we are operating on UTF-16 code units. It becomes problematic when we start trying to treat those substrings as valid Unicode (or UTF-8) sequences. When we pass these split groups into `toDelta()` we do just that and the library crashes. In this patch we're post-processing the diff groups before encoding them to make sure that we un-split the surrogate pairs. The post-processed diffs should produce the same output when applying the diffs. The diff string itself will be different but should change that much - only by a single character at surrogate boundaries.
I'm not sure that I made the right assumptions about Python3's Unicode handling when I made the first patch to it. By constructing the specific `diffs` output I created a sequence of code units that `diff_main` in Python3 would _not_ have made because it's operating on Unicode code points natively when finding the common prefix. Therefore I do not think that the Python3 library experienced this problem as the others did. Nonetheless it _has_ been reporting the diff length differently than in other languages and I have left that change in there. Of note, it doesn't look like we have true harmony between the languages despite the appearance of such. The `lua` wiki page makes this clear, but at least with Python we have the ability to harmonize the meaning of the lengths and I have done that in this change.
```bash java \ -jar path/to/closure-compiler-v20191027.jar \ --js_output_file=diff_match_patch.js \ diff_match_patch_uncompressed.js ```
In the previous iteration of this patch we were only properly handling cases where a new surrogate pair was inserted in between two existing pairs whose high surrogates all matched. Unfortunately when swapping characters or performing any edits where we delete a surrogate pair the patch failed because it only carried the trailing high surrogate over to the next group instead of distributing it to any insert _and_ delete groups following an equality group. In this patch I've updated the JavaScript library to properly distribute the trailing high surrogate.
…lves Because sometimes we get a patch that was built in Python or another library that will happily URI-encode half of a surrogate pair.
There's no need to validate the entire range of input integers. Using `digit16()` will be faster and give us more precise validation.
Python can be compiled in "narrow" mode or "wide" mode which determines how wide the internal string units are. In narrow mode we have UCS-2 code units and higher-order Unicode code points will be stored as surrogate pairs. In wide mode we have UCS-4 code units and so all Unicode code points will be stored as a single item in the string. This patch incorporate a decision based on the internal string width to run the native-looking `toDelta` or the _encoded_ version. In the _encoded_ version we explicitly encode the string to `utf-16be` for consistency sake with the other libraries. We _could_ always run the encoded version but I suspect that the native version will be faster and many deployments will be running narrow mode.
In testing with `speedtest.py` I found no significant performance impact for running the "native" narrow-mode `toDelta()` compared to running the fully-encoding version that operates on bytes. For the sake of simplicity I'm removing the narrow version.
Stop breaking surrogate pairs in toDelta()/fromDelta()
michal-kurz
force-pushed
the
fix-surrogate-pairs-in-text-fns
branch
from
254688b to
cab92fc
Compare
dmsnell
reviewed
Oct 4, 2022
| overwrittenDiffsCounter ++; | ||
| } | ||
|
|
||
| return diffs.splice(0, overwrittenDiffsCounter) |
There was a problem hiding this comment.
can you help me understand what's going on here with overwrittenDiffsCounter? at a glance it looks like we're overwriting the diff operations starting at the first one and then removing those all at the end.
in my head this should be scanning through the array and potentially modifying the current and previous diff objects, but not removing the front of the list of them.
There was a problem hiding this comment.
if it's not easy to do this in-place then I suppose bringing back the array cloning, as you initially proposed, might be a practical solution 🤷♂️
michal-kurz
force-pushed
the
fix-surrogate-pairs-in-text-fns
branch
from
cab92fc to
08e5792
Compare
Fix surrogate pairs splitting in toText()/fromText()
Make js version usable as npm pkg
michal-kurz
force-pushed
the
fix-surrogate-pairs-in-text-fns
branch
from
d9948ae to
7751a1c
Compare
michal-kurz
force-pushed
the
fix-surrogate-pairs-in-text-fns
branch
from
b441d1b to
0e8f62a
Compare
|
@michal-kurz I left some feedback in one of your merged PRs about checking for string input on |
dmsnell
force-pushed
the
issues/69-broken-surrogate-pairs
branch
from
d681ef6 to
50f1542
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.