fix: align scanner semantics with vscode-oniguruma (tie-breaking, CAPTURE_GROUP, UTF-16 offsets) - #244
Conversation
Three divergences from the reference vscode-oniguruma implementation
caused incorrect token scopes / colors in real-world grammars:
1. Tie-breaking across patterns used match length. The TextMate
contract is: leftmost match wins, ties are won by the lowest
pattern index (rule order = rule priority). Tie-breaking by
length lets later rules steal matches, producing inconsistent
scopes for identical constructs (e.g. some object keys colored,
others not). Also adds the reference early-exit when a match
starts exactly at startPosition.
2. Patterns were compiled with ONIG_OPTION_DEFAULT. Oniguruma then
disables numbered captures in any pattern containing named groups,
silently breaking TextMate `captures: {1: ..., 2: ...}` scope
assignment. vscode-oniguruma compiles with
ONIG_OPTION_CAPTURE_GROUP by default.
3. findNextMatchSync received UTF-16 code unit offsets from
vscode-textmate but passed them to oniguruma as UTF-8 byte
offsets, and returned byte offsets back unconverted. Any
multi-byte character (CJK, emoji) shifted every following token
boundary on the line. vscode-oniguruma converts in both
directions (UtfString / convertUtf8OffsetToUtf16); this adds the
equivalent conversion in the JSI bridge.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis PR fixes three divergences between the scanner implementation and the vscode-oniguruma reference: match-selection now uses pattern-index tie-breaking instead of match length, capture groups are preserved via ChangesScanner semantics alignment with vscode-oniguruma
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Infer (1.2.0)packages/react-native-shiki-engine/cpp/NativeShikiEngineModule.cppIn file included from packages/react-native-shiki-engine/cpp/NativeShikiEngineModule.cpp:1: ... [truncated 2200 characters] ... ors.ml", line 48, characters 6-141 packages/react-native-shiki-engine/cpp/onig_regex.cppIn file included from packages/react-native-shiki-engine/cpp/onig_regex.cpp:1: ... [truncated 2200 characters] ... /cFrontend_decl.ml" (inlined), line 54, characters 4-52 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/react-native-shiki-engine/cpp/NativeShikiEngineModule.cpp (1)
53-65: 💤 Low valueConsider binary search for better performance on long lines.
The linear scan is O(n) where n is the string length. Since the table is monotonically non-decreasing, binary search would reduce this to O(log n). While typical line lengths are short, lines with many multi-byte characters could benefit from this optimization.
♻️ Optional binary search implementation
static int utf16ToByteOffset(const std::vector<int>& table, int utf16Offset) { if (utf16Offset <= 0) { return 0; } - // table is monotonically non-decreasing; find first byte whose utf16 >= target const int n = static_cast<int>(table.size()) - 1; - for (int b = 0; b <= n; b++) { - if (table[b] >= utf16Offset) { - return b; - } - } - return n; + // Binary search for first byte whose utf16 offset >= target + int lo = 0, hi = n; + while (lo < hi) { + int mid = lo + (hi - lo) / 2; + if (table[mid] < utf16Offset) { + lo = mid + 1; + } else { + hi = mid; + } + } + return lo; }🤖 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 `@packages/react-native-shiki-engine/cpp/NativeShikiEngineModule.cpp` around lines 53 - 65, The utf16ToByteOffset function currently does a linear scan over table to find the first byte index where table[b] >= utf16Offset; replace that O(n) scan with a binary search (e.g., classic lower_bound style) over the monotonic vector<int> table to return the first index b meeting table[b] >= utf16Offset, still handling utf16Offset <= 0 (return 0) and the case where no entry meets the condition (return last index n). Update utf16ToByteOffset to use binary search logic while preserving existing return semantics and types.
🤖 Prompt for all review comments with 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.
Nitpick comments:
In `@packages/react-native-shiki-engine/cpp/NativeShikiEngineModule.cpp`:
- Around line 53-65: The utf16ToByteOffset function currently does a linear scan
over table to find the first byte index where table[b] >= utf16Offset; replace
that O(n) scan with a binary search (e.g., classic lower_bound style) over the
monotonic vector<int> table to return the first index b meeting table[b] >=
utf16Offset, still handling utf16Offset <= 0 (return 0) and the case where no
entry meets the condition (return last index n). Update utf16ToByteOffset to use
binary search logic while preserving existing return semantics and types.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d27eab63-fa71-4ef5-8885-fb546d1b39aa
📒 Files selected for processing (2)
packages/react-native-shiki-engine/cpp/NativeShikiEngineModule.cpppackages/react-native-shiki-engine/cpp/onig_regex.cpp
2 participants
Fixes #243
Aligns
findNextMatchSyncsemantics with the reference vscode-oniguruma implementation. Three divergences caused visibly wrong token scopes/colors with real-world grammars (details, screenshots and reference-source quotes in #243):1. Tie-breaking: lowest pattern index, not longest match (
onig_regex.cpp)TextMate contract: leftmost match wins; ties at the same position are won by the lowest pattern index (rule order = rule priority). The previous length-based tie-break let later rules steal matches from higher-priority rules, flipping scopes depending on token length. Also adds the reference early-exit once a match starts exactly at
startPosition(no later pattern can do better, ties keep the earlier pattern) — same as theif (location == position) break;in vscode-oniguruma'sonig.cc.2. Compile patterns with
ONIG_OPTION_CAPTURE_GROUP(onig_regex.cpp)With
ONIG_OPTION_DEFAULT, oniguruma disables numbered captures in any pattern that also contains named groups, silently breaking TextMatecaptures: {1: ..., 2: ...}scope assignment. vscode-oniguruma defaults toFindOption.CaptureGroup.3. UTF-16 ⇄ UTF-8 offset conversion in the JSI bridge (
NativeShikiEngineModule.cpp)vscode-textmate passes/expects offsets in UTF-16 code units; oniguruma scans the UTF-8 buffer and reports byte offsets. The bridge now:
startPositionfrom UTF-16 code units to a byte offset, andstart/endbyte offsets back to UTF-16 (negative offsets of unmatched optional groups pass through).Surrogate pairs (4-byte UTF-8 sequences → 2 UTF-16 code units) are handled. The byte→UTF-16 table is built per call, O(line length); mirrors
UtfString/convertUtf8OffsetToUtf16in vscode-oniguruma.Testing
pnpm patchon iOS (RN 0.83.6, new arch, @shikijs/core 3.23.0,html/javascriptgrammars,one-light/one-dark-prothemes) against the chat transcripts that exposed the bug; the semantics of all three changes are taken 1:1 from vscode-oniguruma (onig.ccscanner loop,FindOption.CaptureGroupdefault,UtfStringoffset conversion).codeToTokensBaseoutput between this engine and@shikijs/engine-oniguruma(WASM) over the bundled language samples — happy to add one if you can point me at the preferred test setup.🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
Bug Fixes