fix(native-preview): preserve lone surrogate string literals - #3518
Conversation
d573d96 to
5168a98
Compare
Andrew Ghostuhin (TorinAsakura)
changed the title
Andrew Ghostuhin (TorinAsakura)
changed the title
5168a98 to
fcce776
Compare
This comment was marked as outdated.
This comment was marked as outdated.
There was a problem hiding this comment.
Pull request overview
This PR addresses lossy handling of JS string literals containing lone UTF-16 surrogate escapes by encoding such strings as WTF-8 in the binary AST protocol, updating native-preview decoding accordingly, and bumping the protocol version.
Changes:
- Add Go-side reconstruction of literal text from raw source and encode lone surrogates as WTF-8 bytes when emitting binary AST strings.
- Update native-preview to decode protocol/msgpack strings with a WTF-8-aware decoder and bump protocol version from 5 to 6.
- Add regression tests in both Go and native-preview for surrogate pairs and lone surrogates.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/api/encoder/literal_text.go | Adds escape decoding + WTF-8 emission for surrogate code units based on raw literal text. |
| internal/api/encoder/encoder.go | Bumps protocol version to 6, documents WTF-8, and uses new literal-text encoding for string/template literals. |
| internal/api/encoder/encoder_test.go | Adds a Go regression test asserting WTF-8 bytes are preserved in encoded string literal text. |
| _packages/native-preview/src/api/node/wtf8.ts | Introduces a WTF-8-capable decoder to preserve lone surrogates when decoding bytes to JS strings. |
| _packages/native-preview/src/api/sync/api.ts | Switches RemoteSourceFile decoding from TextDecoder to Wtf8Decoder. |
| _packages/native-preview/src/api/async/api.ts | Switches RemoteSourceFile decoding from TextDecoder to Wtf8Decoder. |
| _packages/native-preview/src/api/node/node.ts | Uses Wtf8Decoder when decoding node payloads into RemoteSourceFile. |
| _packages/native-preview/src/api/node/msgpack.ts | Uses Wtf8Decoder for msgpack string decoding. |
| _packages/native-preview/src/api/node/protocol.ts | Updates native-preview protocol version constant to 6. |
| _packages/native-preview/test/wtf8.test.ts | Adds direct unit coverage for WTF-8 decoding behavior. |
| _packages/native-preview/test/sync/api.test.ts | Adds sync API regression coverage for a string literal containing lone surrogate escapes. |
| _packages/native-preview/test/async/api.test.ts | Adds async API regression coverage for a string literal containing lone surrogate escapes. |
| _packages/native-preview/test/encoder.test.ts | Updates tests to expect protocol version 6. |
Comments suppressed due to low confidence (1)
_packages/native-preview/src/api/node/msgpack.ts:114
- MsgpackReader now decodes strings with Wtf8Decoder, but MsgpackWriter still encodes strings with TextEncoder, which replaces lone surrogates with U+FFFD. If any msgpack string payloads can contain lone surrogates (or if callers round-trip data that now preserves them), this will be lossy. Consider introducing a WTF-8 encoder (paired with Wtf8Decoder) and using it in MsgpackWriter.writeString (and other protocol string encoders like the AST StringTable) to keep the protocol symmetric.
const encoder = new TextEncoder();
const decoder = new Wtf8Decoder();
export class MsgpackWriter {
private buf: Uint8Array;
private view: DataView;
private pos: number;
constructor(initialSize = 256) {
this.buf = new Uint8Array(initialSize);
this.view = new DataView(this.buf.buffer);
this.pos = 0;
}
private ensure(n: number): void {
if (this.pos + n > this.buf.length) {
let newSize = this.buf.length * 2;
while (newSize < this.pos + n) newSize *= 2;
const next = new Uint8Array(newSize);
next.set(this.buf);
this.buf = next;
this.view = new DataView(this.buf.buffer);
}
}
writeArrayHeader(length: number): void {
if (length <= 0x0f) {
this.ensure(1);
this.buf[this.pos++] = 0x90 | length;
}
else if (length <= 0xffff) {
this.ensure(3);
this.buf[this.pos++] = 0xdc;
this.view.setUint16(this.pos, length, false);
this.pos += 2;
}
else {
this.ensure(5);
this.buf[this.pos++] = 0xdd;
this.view.setUint32(this.pos, length, false);
this.pos += 4;
}
}
writeUint(value: number): void {
if (value <= 0x7f) {
this.ensure(1);
this.buf[this.pos++] = value;
}
else if (value <= 0xff) {
this.ensure(2);
this.buf[this.pos++] = 0xcc;
this.buf[this.pos++] = value;
}
else if (value <= 0xffff) {
this.ensure(3);
this.buf[this.pos++] = 0xcd;
this.view.setUint16(this.pos, value, false);
this.pos += 2;
}
else {
this.ensure(5);
this.buf[this.pos++] = 0xce;
this.view.setUint32(this.pos, value, false);
this.pos += 4;
}
}
writeString(str: string): void {
const encoded = encoder.encode(str);
const len = encoded.length;
if (len <= 0x1f) {
e7cbc0d to
adf8f61
Compare
…terals # Conflicts: # internal/api/encoder/encoder_test.go
|
Just so I confirm I'm reading this correctly... all that is changing is what text decoder is used? I don't see any Go side changes, which implies we are already in Go doing the right thing, and also base64 encoding. In which case, I don't think we even need a version bump, as this is arguably just a client bugfix? A perf comparison would be helpful, since you are theoretically replacing the native text decoder with one written in JS, which means it could be slower to transfer data. |
You’re reading it correctly. After #4181, the Go side is already producing the bytes we need, and async JSON-RPC wraps the binary AST response as base64, so this PR is now just a native-preview client decode fix. I removed the protocol version bump and kept protocol version 5. I also added a fast path so buffers without the WTF-8 surrogate lead byte stay on native TextDecoder. Local Node v25.9.0 microbenchmark, 80 decode iterations per case:
So the common path remains effectively native TextDecoder speed. The slower path is limited to buffers containing 0xED, where we either need WTF-8 handling or need to validate that the byte sequence is not a surrogate triplet. Given that, I think this PR is correctly scoped as a client-side native-preview bugfix rather than a protocol change. |
|
I do wonder a bit if that can be sped up by basically splitting the file whenever these characters are seen and just concatting as needed, but that does sound pretty annoying. Andrew Branch (@andrewbranch) Do you have any concerns about this? I think I had thought this was intractable, but did not realize that binary data was already sent over as non-text. |
|
This looks right to me, but Copilot points out that |
|
We already use |
|
I think it would be as easy as checking the range of the byte after |
|
Yes, later would be good, both at the same time |
Fixes #1701
Close torin-asakura/workspace#118.
Summary
Tests
npx hereby tsgo:buildgo test ./internal/api/encodernpm run -w @typescript/native-preview node -- --test test/wtf8.test.ts test/encoder.test.tsnpm run -w @typescript/native-preview node -- --test --test-name-pattern "unicode escapes|template unicode escapes" test/async/api.test.ts test/sync/api.test.tsnpx hereby lintTSGO_HEREBY_NOEMBED=true npx hereby lintDecoder benchmark
Local Node v25.9.0 microbenchmark, 80 decode iterations per case:
The first two rows cover the common path: when there is no surrogate lead byte, the decoder falls back to native TextDecoder. The latter rows cover the uncommon path where the buffer contains 0xED bytes and requires JS-side WTF-8 handling or validation.
Verification logs