Fix surrogate pairs splitting in toText()/fromText()#1
Uh oh!
There was an error while loading. Please reload this page.
Conversation
This file contains hidden or 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
Resolvesgoogle#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-kurzforce-pushed the fix-surrogate-pairs-in-text-fns branch from 254688b to cab92fcCompare
dmsnell reviewed Oct 4, 2022
| overwrittenDiffsCounter++; | ||
| } | ||
| returndiffs.splice(0,overwrittenDiffsCounter) |
Owner
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.
Owner
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-kurzforce-pushed the fix-surrogate-pairs-in-text-fns branch from cab92fc to 08e5792CompareFix surrogate pairs splitting in toText()/fromText()
Make js version usable as npm pkg
michal-kurzforce-pushed the fix-surrogate-pairs-in-text-fns branch from d9948ae to 7751a1cComparemichal-kurzforce-pushed the fix-surrogate-pairs-in-text-fns branch from b441d1b to 0e8f62aCompare
Owner
dmsnell commented Oct 10, 2022
@michal-kurz I left some feedback in one of your merged PRs about checking for string input on |
dmsnellforce-pushed the issues/69-broken-surrogate-pairs branch from d681ef6 to 50f1542CompareSign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment
2 participants
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.