Support supplementary Unicode and emoji sequences

[ccoVeille]

Treat valid 4-byte UTF-8 scalars as printable so supplementary Unicode
characters and emoji sequences parse correctly.

Fixes #354

Supersedes #356

[ccoVeille]

@TomWright I feel like this might be enough for everyone, without a need for a plugin.

I'm curious what you think about my implementation compared to yours in TomWright/dasel#549

cc @colinjlacy @ingydotnet

[Copilot]

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

[TomWright]

I've tested these changes in dasel locally using the tests in my PR TomWright/dasel#549 and it's all green.
I've also ran the entire suit that has various YAML read/writes and didn't find any breakage.
I've obviously only tested as a consumer, but it looks good to me.

My only comment would mirror the AI review on the bounds checks to make sure we don't get panics, but since I haven't seen them happen before I'm guessing there's some safety checks somewhere up the call stack?

[ccoVeille]

I've tested these changes in dasel locally using the tests in my PR TomWright/dasel#549 and it's all green.

👍

I've also ran the entire suit that has various YAML read/writes and didn't find any breakage.
I've obviously only tested as a consumer, but it looks good to me.

🎉

My only comment would mirror the AI review on the bounds checks to make sure we don't get panics, but since I haven't seen them happen before I'm guessing there's some safety checks somewhere up the call stack?

I will assume you are talking about this

• Support supplementary Unicode and emoji sequences #357 (comment)

isPrintable is used at two places:

• when scanning
• when parsing

About the scanner, I think we are safe because libyaml always make sure to fetch a few more character. But scanning a YAML ending with an invalid UTF-8 character could panic.

Then about emitter, I'm unsure it's possible, but to write something invalid. I will give a try.

That said, if you take a look at isPrintable, the line above are also requesting i+1 and i+2. So if the potential bug already exists.

I think this should be addressed in another issue once we will validate it can occur.

I would like to keep the PR simple, but I can give a try and see how much work it could be.

[ccoVeille]

For records, I have checked:

• the scanner cannot fail with a panic

YAML somehow prevents this. And the scanner is built to avoid issues.

I also tried with a YAML !!binary tag to pass an invalid UTF-8 char, but the scanner prevented the issue by returning an error.

• emmiter can fail when a list of YAML events is forged with byte directly.

The fix for the emitter is easy.

I plan to add tests for both. But as I said it would be in another PR.

For now, I created #358 to keep a track of the issue

• Potential panic in emitter with malformed UTF-8 #358

[TomWright]

OK great. All sounds good to me, I'm happy for you to go ahead. Appreciate the speedy response!

[colinjlacy]

LGTM 👍

[colinjlacy]

Just address the copilot comment regarding the name of the test @ccoVeille

[Copilot]

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.