Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 51 additions & 2 deletions crates/perry-runtime/src/string/append.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,22 @@

use super::*;

/// True if the final WTF-8 unit of `bytes` is a lone HIGH surrogate
/// (`ED A0..AF ..`), matching the encoding `canonicalize_surrogate_pairs` uses.
/// `0xED` is always a 3-byte lead (never a continuation), so `bytes[n-3] == 0xED`
/// means the last code unit starts there.
#[inline]
fn ends_with_lone_high_surrogate(bytes: &[u8]) -> bool {
let n = bytes.len();
n >= 3 && bytes[n - 3] == 0xED && (0xA0..=0xAF).contains(&bytes[n - 2])
}

/// True if the first WTF-8 unit of `bytes` is a lone LOW surrogate (`ED B0..BF ..`).
#[inline]
fn starts_with_lone_low_surrogate(bytes: &[u8]) -> bool {
bytes.len() >= 3 && bytes[0] == 0xED && (0xB0..=0xBF).contains(&bytes[1])
}

/// Append a string to another string in-place if possible.
/// Returns the (possibly new) string pointer.
///
Expand Down Expand Up @@ -32,6 +48,9 @@ pub extern "C" fn js_string_append(
ptr::copy_nonoverlapping(src_data, new_data, src_blen as usize);
(*new_ptr).byte_len = src_blen;
(*new_ptr).utf16_len = (*src).utf16_len;
// Preserve the lone-surrogate flag on the duplicate so later
// concats/appends still canonicalize correctly. (#6728)
(*new_ptr).flags |= (*src).flags & STRING_FLAG_HAS_LONE_SURROGATES;
}
}
return new_ptr;
Expand Down Expand Up @@ -61,6 +80,24 @@ pub extern "C" fn js_string_append(

let new_blen = dest_blen + src_blen;

// A high→low surrogate pair can only newly form at the dest|src join
// (both operands are already canonical), so detect it in O(1) from the
// boundary bytes: ordinary appends never pay for a whole-string scan;
// only an actual straddling pair triggers canonicalization below. The
// `+=` path previously skipped this entirely, so `s += hi; s += lo`
// kept two lone 3-byte WTF-8 surrogates instead of the astral char's
// 4-byte UTF-8 (unlike expression `hi + lo`, which canonicalizes). That
// corrupted every emoji built up code-unit-by-code-unit. (#6728)
let flag_bits = ((*dest).flags | (*src).flags) & STRING_FLAG_HAS_LONE_SURROGATES;
let boundary_pair = {
let d = std::slice::from_raw_parts(
(dest as *const u8).add(std::mem::size_of::<StringHeader>()),
dest_blen as usize,
);
let s = std::slice::from_raw_parts(string_data(src), src_blen as usize);
ends_with_lone_high_surrogate(d) && starts_with_lone_low_surrogate(s)
};

// In-place append optimization: if dest is uniquely owned (refcount==1)
// and has enough capacity, append directly without allocation.
// This turns O(n^2) string building loops into amortized O(n).
Expand All @@ -74,7 +111,14 @@ pub extern "C" fn js_string_append(
);
(*dest).byte_len = new_blen;
(*dest).utf16_len += (*src).utf16_len;
return dest; // Same pointer, no allocation!
(*dest).flags |= flag_bits;
return if boundary_pair {
// Merge the straddling pair (usually returns a new, smaller
// string; rare, so the in-place win still holds in general).
super::concat::canonicalize_surrogate_pairs(dest)
} else {
dest // Same pointer, no allocation!
};
}

// Allocate fresh with 2x capacity for future in-place appends.
Expand Down Expand Up @@ -103,11 +147,16 @@ pub extern "C" fn js_string_append(
);
(*new_ptr).byte_len = new_blen;
(*new_ptr).utf16_len = (*dest).utf16_len + (*src).utf16_len;
(*new_ptr).flags |= flag_bits;

// Mark as uniquely owned β€” the caller (codegen) is about to assign
// this pointer to a single variable, so in-place append is safe next time.
(*new_ptr).refcount = 1;

new_ptr
if boundary_pair {
super::concat::canonicalize_surrogate_pairs(new_ptr)
} else {
new_ptr
}
}
}
86 changes: 86 additions & 0 deletions tests/test_string_append_surrogate_repair.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,86 @@
#!/bin/bash
# Regression: `+=` string append must re-pair a split UTF-16 surrogate the same
# way expression concat and Node do. Found while testing pi #6728: pi's
# `visibleWidth` strips ANSI by rebuilding a string one code unit at a time
# (`stripped += clean[i]`), which splits an emoji's surrogate pair across two
# `+=` appends. perry's append path used to copy the two lone 3-byte WTF-8
# surrogates verbatim instead of coalescing them into the astral char's 4-byte
# UTF-8 β€” so `[...s].length`/`codePointAt` disagreed with Node, and pi's TUI
# width invariant aborted on any emoji in a colored line.
#
# This is a differential test: perry's output must be byte-identical to Node's.

set -e

SCRIPT_DIR="$(cd "$(dirname "$0")" && pwd)"
PERRY="$SCRIPT_DIR/../target/release/perry"
[ ! -f "$PERRY" ] && PERRY="$SCRIPT_DIR/../target/debug/perry"
if [ ! -f "$PERRY" ]; then
echo "SKIP: perry binary not found (build with cargo build --release)"
exit 0
fi
if ! command -v node >/dev/null 2>&1; then
echo "SKIP: node not found (differential test needs node)"
exit 0
fi

TMPDIR=$(mktemp -d)
trap "rm -rf $TMPDIR" EXIT

COMPILE_ENV=()
if [ -f "$SCRIPT_DIR/../target/debug/libperry_runtime.a" ] || [ -f "$SCRIPT_DIR/../target/release/libperry_runtime.a" ]; then
COMPILE_ENV=(env PERRY_NO_AUTO_OPTIMIZE=1)
fi

cat > "$TMPDIR/main.ts" << 'EOF'
function show(label: string, s: string): void {
console.log(label + " len=" + s.length + " cp=" + [...s].length + " cp0=" + s.codePointAt(0) + " eq=");
}
const e = "aπŸ‘‹b"; // real astral char (UTF-8 in source)
const hi = e[1], lo = e[2]; // lone high + low surrogate from indexing

// 1. The bug: incremental += of the two halves must re-pair.
let Y = ""; Y += hi; Y += lo;
show("pluseq", Y);
console.log("pluseq-eq", Y === "πŸ‘‹", (hi + lo) === Y);

// 2. pi's ANSI-strip pattern: rebuild a string one code unit at a time.
let out = "";
for (let i = 0; i < e.length; i++) out += e[i];
console.log("rebuild", out === e, [...out].length);

// 3. Multiple emoji, some with a leading ASCII run (exercises both branches).
const t = "xπŸ˜€yπŸ‘‹zπŸŽ‰w";
let r = "";
for (let i = 0; i < t.length; i++) r += t[i];
console.log("multi", r === t, [...r].length, r.codePointAt(1));
Comment on lines +52 to +56

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟑 Minor | ⚑ Quick win

Cover surrogate repair in the fresh-allocation path.

multi repairs pairs while capacity remains available; bulk reallocates but has no surrogate boundary. Add a boundary pair that forces allocation, or regressions in lines 156-159 will pass unnoticed.

Proposed regression case
 // 3. Multiple emoji, some with a leading ASCII run (exercises both branches).
 const t = "xπŸ˜€yπŸ‘‹zπŸŽ‰w";
 let r = "";
 for (let i = 0; i < t.length; i++) r += t[i];
 console.log("multi", r === t, [...r].length, r.codePointAt(1));
 
+// 3b. Fill the 32-byte append buffer, then form a pair across a fresh allocation.
+let fresh = "";
+for (let i = 0; i < 29; i++) fresh += "a";
+fresh += hi; // 29 ASCII bytes + 3-byte WTF-8 high surrogate.
+fresh += lo; // Forces allocation and must re-pair at the boundary.
+console.log("fresh", fresh.length, [...fresh].length, fresh.codePointAt(29));
+
 // 4. A genuinely lone surrogate must STAY lone (no false merge).
πŸ“ Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
// 3. Multiple emoji, some with a leading ASCII run (exercises both branches).
const t = "xπŸ˜€yπŸ‘‹zπŸŽ‰w";
let r = "";
for (let i = 0; i < t.length; i++) r += t[i];
console.log("multi", r === t, [...r].length, r.codePointAt(1));
// 3. Multiple emoji, some with a leading ASCII run (exercises both branches).
const t = "xπŸ˜€yπŸ‘‹zπŸŽ‰w";
let r = "";
for (let i = 0; i < t.length; i++) r += t[i];
console.log("multi", r === t, [...r].length, r.codePointAt(1));
// 3b. Fill the 32-byte append buffer, then form a pair across a fresh allocation.
let fresh = "";
for (let i = 0; i < 29; i++) fresh += "a";
fresh += hi; // 29 ASCII bytes + 3-byte WTF-8 high surrogate.
fresh += lo; // Forces allocation and must re-pair at the boundary.
console.log("fresh", fresh.length, [...fresh].length, fresh.codePointAt(29));
πŸ€– Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/test_string_append_surrogate_repair.sh` around lines 52 - 56, Extend
the surrogate-repair test around the multi-character append case to include a
surrogate pair whose completion forces fresh allocation, exercising the
allocation path in the append implementation near the referenced boundary
handling. Assert that the resulting string preserves the pair and expected
code-point count, while retaining the existing checks for the capacity-available
path.


// 4. A genuinely lone surrogate must STAY lone (no false merge).
let L = ""; L += hi; L += "Z";
console.log("lone", [...L].length, L.codePointAt(0), L.length);

// 5. ASCII fast path is unaffected (and stays correct at scale).
let A = "";
for (let i = 0; i < 500; i++) A += "aΓ©"; // 1 ascii + 1 two-byte utf8
console.log("bulk", A.length, [...A].length);

// 6. Emoji halves split by an unrelated append in between (no false pair).
let M = ""; M += hi; M += "-"; M += lo;
console.log("split", [...M].length, M.codePointAt(0));
EOF

cd "$TMPDIR"
"${COMPILE_ENV[@]}" "$PERRY" compile main.ts --output test_bin --no-cache >/dev/null 2>&1
PERRY_OUT=$(./test_bin 2>&1)
NODE_OUT=$(node main.ts 2>&1)

if [ "$PERRY_OUT" = "$NODE_OUT" ]; then
echo "PASS"
exit 0
fi

echo "FAIL: perry output diverged from node (surrogate re-pairing on +=)"
echo "--- node ---"; echo "$NODE_OUT"
echo "--- perry ---"; echo "$PERRY_OUT"
diff <(echo "$NODE_OUT") <(echo "$PERRY_OUT") || true
exit 1
Loading