Transcode non-ASCII-compatible strings before dumping - #820
Open
Halvanhelv wants to merge 1 commit into
Open
Conversation
`visit_String` matches the string against US-ASCII regexp literals to decide the scalar style. A string whose encoding is not ASCII-compatible cannot be matched against an ASCII regexp, so `Psych.dump` raised `Encoding::CompatibilityError` for any UTF-16/UTF-32 string, empty ones included. The `binary?` guard above those checks only covers ASCII_8BIT, so these encodings fell through to the regexp. Transcode to UTF-8 up front when the encoding is not ASCII-compatible, so every check below operates on a matchable string. Such strings now dump exactly as their UTF-8 equivalents do, matching how the other non-UTF-8 encodings already behave, and matching `Psych.load`, which already accepts UTF-16 input.
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
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.
Fixes #819.
Psych.dumpraisesEncoding::CompatibilityErrorfor any string in a non-ASCII-compatible encoding (UTF-16LE/BE, UTF-32LE/BE). An empty string is enough:This is asymmetric with the load side, which already accepts UTF-16 and has tests for it (
test_transcode_utf16le/test_transcode_utf16be).Cause
visit_Stringpicks the scalar style by matching against US-ASCII regexp literals —yaml_tree.rb:303,:313and:316. A string whose encoding is not ASCII-compatible cannot be matched against an ASCII regexp at all, so the first of them raises regardless of the string's content. Thebinary?guard above them only coversASCII_8BIT, so UTF-16/32 falls through.The crashing set maps exactly onto
Encoding#ascii_compatible?— every other non-UTF-8 encoding (ISO-8859-1, EUC-JP, Windows-1252, Shift_JIS) already transcodes and round-trips fine, so this looks like an unhandled case rather than a deliberate restriction.Fix
Transcode to UTF-8 at the top of
visit_Stringwhen the encoding is not ASCII-compatible, so every check below operates on a matchable string.Placing it before
binary?rather than patching the individual regexps means all three checks are covered at once. It is safe there:binary?tests forASCII_8BIT, which neither the original nor the transcoded string is, so that branch is unaffected; reassigningois the pattern the method already uses (the binary branch doeso = [o].pack('m0')); and alias/anchor registration happens inacceptagainst the original object, so object identity tracking is untouched. UTF-8 input skipsencodeentirely.Strings with invalid byte sequences still raise from
encode, which matches the existing behaviour for ascii-compatible encodings — psych already raisesArgumentError: invalid byte sequencefor those. Using:invalid => :replacewould silently corrupt data.Result
UTF-16/32 strings now produce byte-identical output to their UTF-8 equivalents, and are not tagged
!binary:"multi\nline"--- |-\n multi\n line\n"<<"--- !!str '<<'\n"yes"--- 'yes'\n"0123"--- '0123'\nThose exercise lines 303, 313 and 316 respectively, so the style logic is genuinely reached rather than bypassed.
Tests
Added
test_dump_non_ascii_compatible_encoding, asserting that all four encodings dump identically to UTF-8 across seven content shapes (empty, plain, embedded newline, non-ASCII, and theyes/<</0123special-cased forms). It errors without the fix and passes with it.Full suite before:
678 tests, 1706 assertions, 0 failures, 0 errors.Full suite after:
679 tests, 1734 assertions, 0 failures, 0 errors.