Skip to content

fix: count UTF-16 units for inline string length - #10228

Closed
proggeramlug wants to merge 2 commits into
mainfrom
codex/10191-sso-utf16-length
Closed

proggeramlug wants to merge 2 commits into
mainfrom
codex/10191-sso-utf16-length

Conversation

@proggeramlug

@proggeramlug proggeramlug commented Sep 13, 2026

Copy link
Copy Markdown
Contributor

Summary

JSON.parse('{"a":"é"}').a.length returned 2 because inline strings store their UTF-8 byte length. It now returns 1, matching JavaScript's UTF-16 code-unit semantics across runtime and generated property reads.

Changes

  • Keep the storage byte count separate from a new allocation-free UTF-16 length accessor, used by runtime property reads and suffix cursors.
  • Share inline UTF-16 counting between typed reads, generic reads, and element-shape loop clones. ASCII retains its direct byte-count path, non-ASCII strings remain eligible for inline storage, and clones stay call-free.
  • Add runtime regression tests and a Node parity fixture covering JSON strings, literals, BMP/astral text, lone surrogates, computed keys, boxed strings, slicing, and repeated field reads. Codegen tests verify the clone includes the Unicode path without calls.

Related issue

Fixes #10191

Test plan

  • New runtime regressions failed before the fix; RUST_TEST_THREADS=1 cargo test -p perry-runtime --lib value:: now passes (54 tests).
  • cargo test -p perry-codegen --lib element_shape passes (70 tests).
  • cargo test -p perry-codegen --lib length passes (38 tests).
  • Built the compiler and both matching static archives together with cargo build -p perry -p perry-runtime-static -p perry-stdlib-static in the default dev profile; verified artifact timestamps after the source changes.
  • Compiled test-files/test_gap_10191_sso_utf16_length.ts with those archives and --no-auto-optimize; native stdout matches Node 26.5.1 byte-for-byte. The specialized clone is covered by the codegen tests; this source fixture's trace uses the ordinary generated length paths.
  • python3 scripts/check_node_version_consistency.py, formatting, and git diff --check pass.
  • scripts/pre-tag-check.sh --quick passes except the existing public benchmark evidence freshness failure. Its checker, committed artifact, and all fingerprint inputs are unchanged from base 6000a00dfe.

Checklist

  • No workspace version bump or changes to Cargo.lock, CLAUDE.md, or CHANGELOG.md.
  • Added a changelog fragment and registered the new Rust test module.
  • Commits use the repository's conventional prefixes.
  • Read the contribution guide and Code of Conduct.

Summary by CodeRabbit

  • Bug Fixes

    • Fixed .length for short strings containing non-ASCII characters to count UTF-16 code units, matching JavaScript behavior.
    • Corrected length results across property access, typed and generic reads, suffix operations, JSON-parsed values, and optimized loops.
    • Preserved correct handling for malformed sequences, surrogate halves, combining characters, and astral symbols.
  • Tests

    • Added regression coverage for inline-string length behavior across supported access patterns.

CI follow-up

CI is not green. The runtime suite fails in native_stack::tests::stack_top_respects_custom_thread_stack_sizes (bound must belong to this worker); the warnings gate reports unused WebAssembly helpers; and the checker self-test rejects a raw thread_local! in regex/perex_owner.rs. Those files are unchanged by this PR. Lint also reports the benchmark artifact freshness failure noted above.

@coderabbitai

coderabbitai Bot commented Sep 13, 2026

Copy link
Copy Markdown

Review Change StackReview Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: f758d3a2-f8ca-41c1-8d7a-3567af170fb1

📥 Commits

Reviewing files that changed from the base of the PR and between 6000a00 and ca78777.

📒 Files selected for processing (17)
  • changelog.d/10228-sso-utf16-length.md
  • crates/perry-codegen/src/expr/element_shape_reads.rs
  • crates/perry-codegen/src/expr/property_get/generic_dispatch.rs
  • crates/perry-codegen/src/expr/property_get/tests.rs
  • crates/perry-codegen/src/expr/string_length.rs
  • crates/perry-codegen/src/stmt/element_shape_fields_random_tests.rs
  • crates/perry-codegen/src/stmt/element_shape_loop_tests.rs
  • crates/perry-runtime/src/string/alloc.rs
  • crates/perry-runtime/src/string/char_ops.rs
  • crates/perry-runtime/src/string/concat.rs
  • crates/perry-runtime/src/string/mod.rs
  • crates/perry-runtime/src/string/suffix_cursor.rs
  • crates/perry-runtime/src/value/dynamic_object.rs
  • crates/perry-runtime/src/value/jsvalue.rs
  • crates/perry-runtime/src/value/mod.rs
  • crates/perry-runtime/src/value/sso_length_tests.rs
  • test-files/test_gap_10191_sso_utf16_length.ts

Included review availability: Your plan provides up to 8 included reviews per hour; 5 remain after this review.


📝 Walkthrough

Walkthrough

Short inline strings now report JavaScript UTF-16 lengths across runtime, generated-code, property, suffix-cursor, and element-shape paths. A shared runtime helper and call-free codegen lowering handle non-ASCII and malformed WTF-8 sequences. Regression tests cover JSON parsing and multiple access forms.

Changes

SSO UTF-16 length handling

Layer / File(s) Summary
Runtime UTF-16 length paths
crates/perry-runtime/src/value/jsvalue.rs, crates/perry-runtime/src/value/dynamic_object.rs, crates/perry-runtime/src/string/*, crates/perry-runtime/src/value/sso_length_tests.rs
Adds short_string_utf16_len() and uses it for runtime property, dynamic-object, and suffix-cursor length reads. Tests cover Unicode and malformed sequences.
Generated UTF-16 length lowering
crates/perry-codegen/src/expr/string_length.rs, crates/perry-codegen/src/expr/property_get/*, crates/perry-codegen/src/expr/element_shape_reads.rs, crates/perry-codegen/src/stmt/*tests.rs
Uses shared SSO lowering that preserves the ASCII fast path and counts non-ASCII payloads as UTF-16 units without runtime calls. Element-shape clone tests include the new blocks.
End-to-end regression coverage
test-files/test_gap_10191_sso_utf16_length.ts, changelog.d/10228-sso-utf16-length.md
Tests JSON-parsed short strings across typed, generic, computed, object, suffix, and element-shape access paths. Documents the fix.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~25 minutes

Change: Bug fix · Severity of issue fixed: Medium

Merge Risk: ⚪ Minimal · up to ca787

The short inline-string length fix is covered across runtime and generated paths, including non-ASCII and malformed inputs. No merge-blocking risk remains.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Linked Issues check ✅ Passed The PR meets the coding requirements in issue #10191. Runtime length reads use short_string_utf16_len() for short strings. Suffix cursors, generic property reads, typed reads, and element-shape clon…
Out of Scope Changes check ✅ Passed The changes stay within issue #10191. Runtime and codegen changes implement consistent SSO .length handling. Tests verify the required runtime and generated paths. Changelog and documentation update…
Docstring Coverage ✅ Passed Docstring coverage is 80.77% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 26 functions across 16 files. (1 skipped: 1…
Title check ✅ Passed The title clearly and concisely describes the main change: correcting inline string length to count UTF-16 code units.
Description check ✅ Passed The description includes all required sections, explains the fix, lists concrete changes, references issue #10191, documents the test plan, and records known CI failures unrelated to the changed files…
✨ Finishing Touches 💡 1
🛠️ Fix failing CI checks 💡
  • Create stacked PR
  • Commit on current branch
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch codex/10191-sso-utf16-length

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@proggeramlug

Copy link
Copy Markdown
Contributor Author

Landed via merge train 185r (#10242) at 9fda98d on main.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

SSO short strings report .length in bytes, not UTF-16 code units

1 participant