fix(runtime): RegExp \w/\W/\b use ASCII word semantics and . excludes all four LineTerminators - #9263
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
Included review availability: Your plan provides up to 8 included reviews per hour; 4 remain after this review. 📝 WalkthroughWalkthroughChangesThe regex engine now translates RegExp semantics
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: ⚪ Minimal · up to This PR corrects RegExp word-character and line-terminator matching to align with ECMAScript behavior, with focused regression coverage and no actionable merge-blocking risk remaining after normal checks and review. Sequence Diagram(s)sequenceDiagram
participant DifferentialRunner
participant NodeJS
participant PerryBinary
DifferentialRunner->>NodeJS: Run generated TypeScript regex probe
NodeJS-->>DifferentialRunner: Serialize pattern results
DifferentialRunner->>PerryBinary: Run the same probe
PerryBinary-->>DifferentialRunner: Serialize pattern results
DifferentialRunner->>DifferentialRunner: Compare records and report divergences
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Description checkExplanation The description provides a clear summary, detailed changes, related issue references, verification commands and results, regression coverage, and scope notes. It does not reproduce every template heading or checklist item, but the required information is mostly complete. Full details: Linked Issues checkExplanation The implementation satisfies issue Full details: Out of Scope Changes checkExplanation The supplied linked issue covers the Full details: Docstring CoverageExplanation Docstring coverage is 65.22% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 23 functions across 5 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
9f9fe62 to
cb23960
Compare
|
Merged. Validated on a shared branch with #9228, #9257, #9263, #9271, #9272, #9274, #9277, #9279 and #9280 — one build, one validation pass, then split back out and merged individually. Results across the batch:
One probe ( |
…erryTS#9305 fallout) The transport fix unmasked this second regression: js_regex_to_rust spells ECMAScript's ASCII \b/\B as (?-iu:\b) (PerryTS#9263), which the regex crate accepts but fancy-regex rejects (NonUnicodeUnsupported). Any lookaround/backreference pattern with a word boundary was a SyntaxError — cli.js's marked html-block regex among them; its throw inside a microtask was the longjmp that the miscompiled runner turned into the --help SIGSEGV. build_fancy_regex now rewrites the translator's marker (unambiguous — '(?-iu:' cannot survive from user input) into the one-char-lookaround boundary spelling the i+u path already uses. Claude-Session: https://claude.ai/code/session_014knX724SYDogwzsXybCGxp
…ault fixed, parity gate back online (#9305) (#9323) * runtime: route every longjmp-target setjmp through a C trampoline (#9305) rustc cannot express returns_twice, so a raw setjmp in a Rust frame is compiled under LLVM's one-return assumption — stack slots live only into the longjmp path get colored into unrelated normal-path temporaries. run_microtasks crashed exactly this way (cached TLS base spill reused by the task-record copy loop). No Rust frame is a longjmp target anymore: perry_sjlj_try (C, compiled with real setjmp semantics) is the only twice-returning frame, and Rust callers use exception::arm_trap_and_run / catch_js_throw. The one remaining raw setjmp (gc/roots.rs register snapshot) never longjmps and is documented as such. Claude-Session: https://claude.ai/code/session_014knX724SYDogwzsXybCGxp * tests: #9305 regression fixture (throw-in-microtask) + transport unit tests Claude-Session: https://claude.ai/code/session_014knX724SYDogwzsXybCGxp * fmt + changelog fragment (#9305) Claude-Session: https://claude.ai/code/session_014knX724SYDogwzsXybCGxp * docs: js_try_push / ffi::setjmp contract notes (#9305) Claude-Session: https://claude.ai/code/session_014knX724SYDogwzsXybCGxp * stdlib+ext-fastify: convert remaining raw setjmp trap sites to the C trampoline (#9305) Same hazard class as the runtime sites: raw setjmp in a Rust frame is compiled without returns_twice. perry-stdlib goes through exception::catch_js_throw; perry-ext-fastify (no Cargo dep on perry-runtime by design) declares the perry_sjlj_try C symbol directly and mirrors arm_trap_and_run locally. Claude-Session: https://claude.ai/code/session_014knX724SYDogwzsXybCGxp * runtime(regex): fancy engine accepts the ASCII word-boundary marker (#9305 fallout) The transport fix unmasked this second regression: js_regex_to_rust spells ECMAScript's ASCII \b/\B as (?-iu:\b) (#9263), which the regex crate accepts but fancy-regex rejects (NonUnicodeUnsupported). Any lookaround/backreference pattern with a word boundary was a SyntaxError — cli.js's marked html-block regex among them; its throw inside a microtask was the longjmp that the miscompiled runner turned into the --help SIGSEGV. build_fancy_regex now rewrites the translator's marker (unambiguous — '(?-iu:' cannot survive from user input) into the one-char-lookaround boundary spelling the i+u path already uses. Claude-Session: https://claude.ai/code/session_014knX724SYDogwzsXybCGxp --------- Co-authored-by: Ralph Küpper <ralph@skelpo.com>
1 participant
Fixes #9217 and #9218 — two silent wrong answers in the JS→Rust regex translation, both of which made patterns match more than the program asked for.
#9217 —
\w,\W,\b,\Bwere UnicodePerry emitted Rust's Unicode word-character semantics. ECMAScript defines the word set as ASCII-only
[A-Za-z0-9_], and theuflag does not widen it — the only widening isi+uadding U+212A KELVIN SIGN and U+017F LATIN SMALL LETTER LONG S (§22.2.2.9.3 WordCharacters). So/:?[\w-]+/matchedéΩ中, and\binherited the divergence because word boundaries are defined in terms of\w.Mixed and negated character classes now preserve JS case-insensitive semantics without letting Rust case-fold the word arm — that separation is what makes
[\w\W]and[^\w\W]come out right underi.#9218 —
.excluded only\nECMAScript's
.matches any character except a LineTerminator:\n,\r, U+2028, U+2029 (§22.2.2.7 CompileAtom, §12.3). Perry excluded only\n, so"\t\r\n".match(/.{2}/g)returned["\t\r"]where node returnsnull—.-patterns ran straight across CRLF boundaries. DotAll (s) is unchanged.#9216's rewrite is preserved
The
[^] → (?s:.)and[] → [a&&b]translations that avoid the catastrophic case-fold ([\s\S]underiwalked 1,114,112 code points) remain intact, with no full-range class reintroduced. A regression test additionally pins that quantifiers apply atomically to the rewritten negated classes.Verification
scripts/regex_9217_9218_differential.mjs) was run against unchangedmainfirst and confirmed to fail on the cases fixed here./.{2}/gCRLF case.test_gap_9217_9218_regexp_word_dot.ts: node and perry produce identical 106-line output (sha256b989905fdf66e0ed…).cargo test -p perry-runtime --lib -- --test-threads=1: 2,870 passed, 0 failed, 4 ignored. Existing regex tests 66/66.A correction to the issue reports
The 3,402-pattern corpus differential that produced #9217 and #9218 reported 64 diverging records on
mainand attributed all of them to these two bugs. That attribution was not quite right. Record 484 —/[ \t]+$/gmagainst"\t\r\n"— is an independent multiline-$/CRLF divergence, and it is deliberately left unchanged here.The other residual divergences after this fix are likewise pre-existing and unrelated: non-
uUTF-16 code-unit matching for astral characters (emoji), and multiline anchors around CRLF. The fixture usesufor its astral case specifically so it does not conflate those with this change.Summary by CodeRabbit
Bug Fixes
.so it excludes all line terminators unless dotAll mode is enabled.Tests