Bump WebKit (oven-sh/WebKit#456 preview): non-unicode /i folds the case pairs added in Unicode 16 and 17 - #39381
Bump WebKit (oven-sh/WebKit#456 preview): non-unicode /i folds the case pairs added in Unicode 16 and 17#39381robobun wants to merge 2 commits into
Conversation
|
Warning Review limit reached
Next review available in: 4 minutes Limit details: You’ve used all 5 included reviews currently available under your plan. Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Comment |
|
Status
|
|
The review comment on the class range cases refers to e02f77f; 9c3198c already tightens both entries (U+A7DB against |
There was a problem hiding this comment.
Thanks — commit 9c3198c1 addresses the earlier inline note: all five class-range entries now test a code unit outside the literal range that can only match via its new case partner. This run found no further issues in the test file. Not auto-approving since WEBKIT_VERSION is still on the preview tag (as the PR notes) and a WebKit engine bump warrants a human sign-off once it's re-pinned to the merged oven-sh/WebKit commit.
Checked: the matches() helper covers atom/class/negated-class/backref/replace in both directions under /i and /iu; the neighbouring-pair and "does not fold" tables use distinct code units from the new pairs; the interpreter subprocess drains stdout/stderr/exited concurrently and spreads bunEnv.
Extended reasoning...
Overview
Two-file change: WEBKIT_VERSION in scripts/build/deps/webkit.ts moves from commit c6cfe90c60 to the preview tag autobuild-preview-pr-456-bd2b42cd, and a new 132-line test file test/js/bun/jsc/regexp-ignore-case.test.ts covers the eight Unicode 16/17 case pairs that the regenerated Yarr /i canonicalize table now folds. The engine change itself lives in oven-sh/WebKit#456; nothing in Bun's own source is touched.
Prior feedback resolved
My earlier inline comment on line 88 flagged two vacuous entries in the "/i class ranges pick up the new partners" test where the input was already a literal member of the range. Commit 9c3198c1 tightened both: entry 1 is now /[\ua7cc-\ua7da]/i.test("\ua7db") (U+A7DB is one past the range end, matches via partner U+A7DA) and entry 5 is now /[\u1c80-\u1c89]/i.test("\u1c8a") (U+1C8A one past the end, matches via U+1C89). Each of the five entries now depends on the new folding, matching the comment above them. The robobun status comment confirms the same tightening was applied to the WebKit-side stress test.
Security risks
None identified. The version-string change flows into a GitHub release download URL and a cache-dir name via existing prebuiltUrl/prebuiltDestDir logic that already handles the autobuild- prefix. The test file constructs regexes from string-literal code units only.
Level of scrutiny
High — a WebKit version pin selects the JavaScript engine binary for every platform. The PR description says the WebKit branch is the previous pin plus one regenerated table file, and the BMP-wide sweep against ES Canonicalize reports zero differences, but I can't verify the WebKit-side diff from this repo. The pin is also explicitly temporary: the PR and the robobun status both state it must be re-pinned to the merged oven-sh/WebKit commit before this lands. Auto-approving a value that is planned to change is not appropriate.
Other factors
The test file follows harness conventions: bunExe()/bunEnv with {...bunEnv, BUN_JSC_useRegExpJIT: "false"}, await using on the spawned process, concurrent Promise.all drain of stdout/stderr/exited, exit-code asserted last. The describe.each/test.each matrix covers both flag modes and both directions per pair, and the negative test ("does not fold the code units around the new pairs") guards against over-folding. No toUpperCase assertions are made, which the file's header comment correctly notes would be platform-dependent on macOS.
|
Noted on the review: waiting for the human sign-off after the re-pin is the intended flow here. Current state: the Buildkite build for |
|
Rebased locally onto main (the conflict was the |
…and 17 Picks up oven-sh/WebKit#456, which regenerates JavaScriptCore's committed /i canonicalization table (yarr/YarrCanonicalizeUCS2.cpp) under ICU 78. The /iu table is built from CaseFolding.txt 17.0.0 at build time, so the eight BMP case pairs added in Unicode 16 and 17 folded under /iu but not under /i. Pinned to the preview build of that PR for now. The test covers the eight pairs in both directions under /i and /iu, the neighbouring pairs whose table runs were merged or split, the code units around them, class ranges, and the Yarr interpreter.
The tested character now lies outside each range, so these only pass when the range's partner is canonicalized into the class.
9c3198c to
21cdf4d
Compare
|
Rebased on main and pushed as |
Bumps
WEBKIT_VERSIONto pick up oven-sh/WebKit#456 and adds thebun:testcoverage for it.Pinned to the preview build of oven-sh/WebKit#456 so CI exercises the change; to be re-pinned to the merged oven-sh/WebKit commit before this merges.
Problem
Non-unicode
/idoes not fold the eight BMP case pairs added in Unicode 16 and 17, while/iudoes (and V8 folds them under both flags). Found by comparing RegExp results against V8:Pairs: U+019B/U+A7DC, U+0264/U+A7CB, U+A7CC/U+A7CD, U+A7DA/U+A7DB, U+1C89/U+1C8A (Unicode 16) and U+A7CE/U+A7CF, U+A7D2/U+A7D3, U+A7D4/U+A7D5 (Unicode 17). Every other BMP case pair already folds the same way as in V8; these are the only differences.
Cause: JavaScriptCore builds the
/iutable fromucd/CaseFolding.txt(17.0.0) at build time, but the/itable,yarr/YarrCanonicalizeUCS2.cpp, is a committed file generated by running a script in a JS shell, and it was last regenerated from pre-Unicode-16 case data. Bun 1.4 also moved its bundled ICU to 78 (Unicode 17), so on Linux and Windows"\u019b".toUpperCase() === "\ua7dc"while/\u019b/idoes not match it; on macOStoUpperCasefollows the system ICU, the regexp tables are the same everywhere.No Bun source is involved; the fix is the engine's table.
Fix
YarrCanonicalizeUCS2.cppunder ICU 78. The diff there is the sixteen code units above; the rest of the table, the range count and the Latin-1 table are unchanged, and the whole regenerated table was checked against the ESCanonicalizedefinition for all 65536 code units and against V8. That branch is directly on top ofeeab04040f, the commit main pins today (it was rebased there after Upgrade WebKit to 47f7250137c6 #39371 moved the pin), so the new pin is exactly the current engine plus that one file.test/js/bun/jsc/regexp-ignore-case.test.ts. For each of the eight pairs, in both directions, under/iand/iu: atom, anchored atom, atom inside a longer pattern, class, negated class, backreference andreplace. It also checks the neighbouring pairs whose table runs were merged or split by the regeneration (U+019A/U+023D, U+0263/U+0194, U+0265/U+A78D, U+A7C9/A, U+A7D0/1, U+A7D6/7, U+A7D8/9, U+A7F5/6, U+1C88 with U+A64A/B), that the unassigned or unrelated code units next to the new entries still do not fold, that case-insensitive class ranges pick up the new partners, and that the Yarr interpreter (BUN_JSC_useRegExpJIT=false, in a child process) agrees with the JIT.eeab04040f, and thec6cfe90c60before it) the file fails 10 of its 39 tests (the eight/ipairs, the class ranges and the interpreter child; every/iform fails for every pair, and the child reports all sixteen directions) and the/iuand neighbouring-pair tests pass, so those describe behaviour the bump keeps. Against this pin all 39 pass (bun bd test, debug ASAN build) and the stress test from [JSC] Yarr: regenerate the non-unicode /i canonicalization table with the Unicode 16 and 17 case pairs WebKit#456 passes with the Yarr JIT on and off. A BMP-wide sweep of/iagainst the ESCanonicalizedefinition under ICU 78, run against the first preview build (same table file byte for byte), reports 0 differences, against main's pin 16.Background
Canonicalizewithout theuflag compares code units after mapping each throughtoUpperCase(leaving it alone when the result is not a single code unit, or when a non-ASCII character would map into ASCII); withuit uses simple case folding. Yarr precompiles both relations into range tables over the BMP and binary-searches them at compile time (JIT) or match time (interpreter), so both tiers read the same data.WEBKIT_VERSIONinscripts/build/deps/webkit.tsis the only place the engine version lives; CI andbun bddownload the prebuiltautobuild-<version>release for it from oven-sh/WebKit. Preview builds of a WebKit PR are published asautobuild-preview-pr-<n>-<sha>and can be pinned the same way.[decide:webkit] gate passed · iteration 0 · 2 files touched
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 0 rejected · iteration 0
evidence per changed file