Repository navigation
feat(unicode): migrate grapheme breaking to uucode with GB9c support - #26376
Merged
Merged
Conversation
Replace Bun's outdated grapheme breaking code with Ghostty's approach using
the uucode library. This adds proper GB9c (Indic Conjunct Break) support
and modernizes the grapheme cluster algorithm.
## Changes
### New: Vendored uucode library (src/deps/uucode/)
- MIT-licensed Unicode property library by Jacob Sandlund
- Includes Unicode Character Database (UCD) for table generation
- Used only at build time for table generation; no runtime dependency
### New: Build-time integration (src/unicode/uucode/)
- uucode_config.zig: Configures which Unicode properties to generate
- grapheme_gen.zig: Generator binary that queries uucode and outputs tables
- lut.zig: 3-level lookup table generator (from Ghostty)
- CLAUDE.md: Maintenance documentation for future contributors
### Rewritten: grapheme.zig
- GraphemeBreakNoControl enum (u5, 17 values) replaces GraphemeBoundaryClass (u4, 12 values)
- BreakState enum (u3, 5 states) replaces packed struct (u2, 2 bools)
- Adds GB9c (Indic Conjunct Break) - Devanagari conjuncts now cluster correctly
- Full algorithm runs at comptime to build 8KB decision table
- Runtime is two table lookups with zero branching
### Generated: grapheme_tables.zig
- Pre-generated 3-level lookup table mapping codepoints to GraphemeBreakNoControl
- Only stores the grapheme break enum (not width or emoji properties)
- stage2 uses u8 (max value 16) instead of u16, saving ~27KB in binary
- Only 17 unique stage3 entries (one per enum value)
- Regenerate with: zig build generate-grapheme-tables
### Modified: visible.zig
- Two lines: BreakState{} → .default
### New: scripts/update-uucode.sh
- Shell script to update vendored uucode and regenerate tables
### New: test cases
- Devanagari conjunct tests (GB9c) in stringWidth.test.ts
## Binary size impact
- Tables: ~43KB (stage1 16KB + stage2 27KB + stage3 17 bytes)
- Precomputed decisions: 8KB
- Previous approach was ~70KB+ with wider types
Collaborator
|
Updated 5:20 PM PT - Jan 22nd, 2026
❌ @Jarred-Sumner, your commit c1a7730 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 26376That installs a local version of the PR into your bun-26376 --bun |
Contributor
WalkthroughThis PR vendors the uucode Unicode library into Bun and integrates it into the build system. Additions include build-time grapheme table generation, comprehensive Unicode property support, and grapheme cluster breaking functionality with Unicode data resources. Changes
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 11
🤖 Fix all issues with AI agents
In `@scripts/update-uucode.sh`:
- Around line 56-67: The script currently looks only for Zig's cache at
"$HOME/.cache/zig/p"; update the fallback in scripts/update-uucode.sh to support
macOS and XDG overrides by checking additional cache locations (e.g.,
"$HOME/Library/Caches/zig" and "${XDG_CACHE_HOME:-$HOME/.cache}/zig/p") and pick
the newest matching "uucode-*" directory before calling update_from_dir; ensure
you search both paths (or multiple) with find, combine results, sort -V, then
select tail -1 so update_from_dir receives the correct cross-platform cache
directory.
- Around line 33-43: In update_from_url(), the trap currently expands $tmp
immediately; change the trap to use single quotes so the temporary directory
variable is expanded at EXIT, e.g. update the trap invocation to trap 'rm -rf
"$tmp"' EXIT; ensure tmp is still created with tmp=$(mktemp -d) and left
unchanged, and verify cleanup behavior after update_from_dir is called.
In `@src/deps/uucode/build.zig`:
- Around line 446-452: Update the inline comment adjacent to the .use_llvm =
true setting for build_tables_exe (the executable defined as build_tables_exe
with .root_module = build_tables_mod) to document the LLVM backend requirement
by adding a brief note and a link to the relevant Zig issue or bug tracker
entry; keep the existing explanation about the x86 backend segfaulting, append
the URL to the issue for future reference, and ensure the comment stays concise
and directly above the .use_llvm field so maintainers see it when inspecting
build_tables_exe.
In `@src/deps/uucode/README.md`:
- Line 59: Typo: the identifier "uccode" is misspelled and should be "uucode".
Update the README code snippet so the call uses uucode.utf8.Iterator instead of
uccode.utf8.Iterator (i.e., fix the expression where
uucode.grapheme.Iterator(...) is invoked) so the identifiers match
uucode.grapheme.Iterator and uucode.utf8.Iterator.
In `@src/deps/uucode/src/ascii.zig`:
- Around line 76-85: The toLower implementation is asymmetric with toUpper
(toUpper uses XOR '^' while toLower uses OR '|'); update toLower (function
toLower) to compute the same mask as now but apply it with XOR (c ^ mask)
instead of OR so both case conversions use the same bit-toggling operation and
remain correct; keep the mask generation using `@intFromBool`(isUpper(c)) << 5 and
only change the final operator to '^' to match toUpper.
In `@src/deps/uucode/src/config.zig`:
- Around line 513-523: The guard that prevents overrides is mis-parenthesized so
that std.mem.eql checks for "min_value" and "max_value" are evaluated outside
the !is_updating_ucd condition; update the condition in the inline for over
`@typeInfo`(`@TypeOf`(overrides)).@"struct".fields so that the !is_updating_ucd
applies to all field-name checks (including "min_value" and "max_value") by
grouping the entire series of std.mem.eql(...) OR expressions inside the same
parentheses with !is_updating_ucd; locate the conditional using is_updating_ucd
and f.name and adjust parentheses so all field comparisons are governed by the
same !is_updating_ucd guard.
In `@src/deps/uucode/src/get.zig`:
- Around line 81-108: The code computes fields_len - 1 for FieldEnum.tag_type
which underflows at comptime when fields_len == 0; add a comptime guard around
the construction of the enum in FieldEnum (referencing FieldEnum and fields_len)
that checks if (fields_len == 0) and handles that case explicitly (either return
an enum with a safe tag_type range using 0..0 via std.math.IntFittingRange(0, 0)
or otherwise produce a clear comptime error), otherwise use the existing
std.math.IntFittingRange(0, fields_len - 1); ensure the guard is applied where
the break :blk `@Type`(...) is created so no subtraction occurs on zero.
In `@src/deps/uucode/src/types.zig`:
- Around line 853-856: The inline import `@import`("./get.zig") used to compute
hardcoded_backing should be moved out of the struct body and placed at the
bottom of the struct (or file) and replaced inside the struct with a reference
to the previously bound symbol; specifically, separate the import (e.g., import
get = `@import`("./get.zig"); or equivalent) and the backingFor lookup so that
hardcoded_backing within the struct uses get.backingFor(c.name) (or the chosen
alias), and ensure both the import binding and any non-struct-scope constants
are declared after the struct definition to conform to Zig’s guideline.
In `@src/deps/uucode/src/x/grapheme.zig`:
- Around line 658-661: The code opens "ucd/auxiliary/GraphemeBreakTest.txt"
using a relative path via std.fs.cwd().openFile(); change this to resolve and
open an absolute path (use std.fs.cwd().realPath or build an absolute path from
std.fs.path.join if needed) and call openFileAbsolute or openFileAbsoluteZ
instead of openFile — update the symbol usage around file_path and the open call
so the code uses the absolute path variable and
openFileAbsolute/openFileAbsoluteZ on the same file handle creation (ensure you
still defer file.close()).
In `@src/unicode/uucode/CLAUDE.md`:
- Around line 11-23: The fenced code block in CLAUDE.md is missing a language
tag, causing markdownlint MD040; update the opening fence to include a language
(e.g., change the triple backtick line to ```text) so the block becomes ```text
... ```; edit the fenced block in src/unicode/uucode/CLAUDE.md (the diagram/code
block at the top of the file) to add the language tag.
In `@src/unicode/uucode/grapheme_gen.zig`:
- Around line 9-28: Add a compile-time assertion to ensure this
GraphemeBreakNoControl enum stays identical to the GraphemeBreakNoControl
definition used in grapheme.zig: at comptime import or reference the other
file's GraphemeBreakNoControl and assert the variant count and ordering match
(e.g., compare `@typeInfo`(...).Enum.fields or compare `@enumToInt` of each named
variant) so a mismatch fails compilation; place the comptime assert next to the
enum definition and reference the GraphemeBreakNoControl symbol from both
modules.
Comment on lines
+33
to
+43
| update_from_url() { | ||
| local url="$1" | ||
| local tmp | ||
| tmp=$(mktemp -d) | ||
| trap "rm -rf $tmp" EXIT | ||
|
|
||
| echo "Downloading uucode from: $url" | ||
| curl -fsSL "$url" | tar -xz -C "$tmp" --strip-components=1 | ||
|
|
||
| update_from_dir "$tmp" | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Fix trap to use single quotes per shellcheck recommendation.
The trap on line 37 expands $tmp immediately. While this works correctly here since $tmp is set just before, using single quotes is the recommended practice to avoid subtle bugs if the code is refactored.
♻️ Suggested fix
update_from_url() {
local url="$1"
local tmp
tmp=$(mktemp -d)
- trap "rm -rf $tmp" EXIT
+ trap 'rm -rf "$tmp"' EXIT
echo "Downloading uucode from: $url"
curl -fsSL "$url" | tar -xz -C "$tmp" --strip-components=1📝 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
| update_from_url() { | |
| local url="$1" | |
| local tmp | |
| tmp=$(mktemp -d) | |
| trap "rm -rf $tmp" EXIT | |
| echo "Downloading uucode from: $url" | |
| curl -fsSL "$url" | tar -xz -C "$tmp" --strip-components=1 | |
| update_from_dir "$tmp" | |
| } | |
| update_from_url() { | |
| local url="$1" | |
| local tmp | |
| tmp=$(mktemp -d) | |
| trap 'rm -rf "$tmp"' EXIT | |
| echo "Downloading uucode from: $url" | |
| curl -fsSL "$url" | tar -xz -C "$tmp" --strip-components=1 | |
| update_from_dir "$tmp" | |
| } |
🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 37-37: Use single quotes, otherwise this expands now rather than when signalled.
(SC2064)
🤖 Prompt for AI Agents
In `@scripts/update-uucode.sh` around lines 33 - 43, In update_from_url(), the
trap currently expands $tmp immediately; change the trap to use single quotes so
the temporary directory variable is expanded at EXIT, e.g. update the trap
invocation to trap 'rm -rf "$tmp"' EXIT; ensure tmp is still created with
tmp=$(mktemp -d) and left unchanged, and verify cleanup behavior after
update_from_dir is called.
Comment on lines
+56
to
+67
| else | ||
| # Default: use the zig global cache if available | ||
| CACHED=$(find "$HOME/.cache/zig/p" -maxdepth 1 -name "uucode-*" -type d 2>/dev/null | sort -V | tail -1) | ||
| if [ -n "$CACHED" ]; then | ||
| update_from_dir "$CACHED" | ||
| else | ||
| echo "error: no uucode source specified and none found in zig cache" | ||
| echo "" | ||
| echo "usage: $0 <path-to-uucode-dir-or-url>" | ||
| exit 1 | ||
| fi | ||
| fi |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider cross-platform Zig cache location.
The default cache path $HOME/.cache/zig/p is Linux-specific. On macOS, the Zig cache is typically at $HOME/Library/Caches/zig. Consider supporting both locations for better portability:
♻️ Suggested fix for cross-platform support
else
# Default: use the zig global cache if available
- CACHED=$(find "$HOME/.cache/zig/p" -maxdepth 1 -name "uucode-*" -type d 2>/dev/null | sort -V | tail -1)
+ # Try Linux cache first, then macOS
+ CACHED=$(find "$HOME/.cache/zig/p" "$HOME/Library/Caches/zig/p" -maxdepth 1 -name "uucode-*" -type d 2>/dev/null | sort -V | tail -1)
if [ -n "$CACHED" ]; then
update_from_dir "$CACHED"
else📝 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
| else | |
| # Default: use the zig global cache if available | |
| CACHED=$(find "$HOME/.cache/zig/p" -maxdepth 1 -name "uucode-*" -type d 2>/dev/null | sort -V | tail -1) | |
| if [ -n "$CACHED" ]; then | |
| update_from_dir "$CACHED" | |
| else | |
| echo "error: no uucode source specified and none found in zig cache" | |
| echo "" | |
| echo "usage: $0 <path-to-uucode-dir-or-url>" | |
| exit 1 | |
| fi | |
| fi | |
| else | |
| # Default: use the zig global cache if available | |
| # Try Linux cache first, then macOS | |
| CACHED=$(find "$HOME/.cache/zig/p" "$HOME/Library/Caches/zig/p" -maxdepth 1 -name "uucode-*" -type d 2>/dev/null | sort -V | tail -1) | |
| if [ -n "$CACHED" ]; then | |
| update_from_dir "$CACHED" | |
| else | |
| echo "error: no uucode source specified and none found in zig cache" | |
| echo "" | |
| echo "usage: $0 <path-to-uucode-dir-or-url>" | |
| exit 1 | |
| fi | |
| fi |
🤖 Prompt for AI Agents
In `@scripts/update-uucode.sh` around lines 56 - 67, The script currently looks
only for Zig's cache at "$HOME/.cache/zig/p"; update the fallback in
scripts/update-uucode.sh to support macOS and XDG overrides by checking
additional cache locations (e.g., "$HOME/Library/Caches/zig" and
"${XDG_CACHE_HOME:-$HOME/.cache}/zig/p") and pick the newest matching "uucode-*"
directory before calling update_from_dir; ensure you search both paths (or
multiple) with find, combine results, sort -V, then select tail -1 so
update_from_dir receives the correct cross-platform cache directory.
Comment on lines
+446
to
+452
| const build_tables_exe = b.addExecutable(.{ | ||
| .name = "uucode_build_tables", | ||
| .root_module = build_tables_mod, | ||
|
|
||
| // Zig's x86 backend is segfaulting, so we choose the LLVM backend always. | ||
| .use_llvm = true, | ||
| }); |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Document the LLVM backend requirement.
The comment explains that Zig's x86 backend is segfaulting, requiring LLVM. Consider adding a link to the relevant Zig issue for future reference.
📝 Proposed documentation improvement
const build_tables_exe = b.addExecutable(.{
.name = "uucode_build_tables",
.root_module = build_tables_mod,
- // Zig's x86 backend is segfaulting, so we choose the LLVM backend always.
+ // Zig's x86 backend segfaults during table generation.
+ // See: https://github.com/ziglang/zig/issues/XXXXX (replace with actual issue)
.use_llvm = true,
});🤖 Prompt for AI Agents
In `@src/deps/uucode/build.zig` around lines 446 - 452, Update the inline comment
adjacent to the .use_llvm = true setting for build_tables_exe (the executable
defined as build_tables_exe with .root_module = build_tables_mod) to document
the LLVM backend requirement by adding a brief note and a link to the relevant
Zig issue or bug tracker entry; keep the existing explanation about the x86
backend segfaulting, append the URL to the issue for future reference, and
ensure the comment stays concise and directly above the .use_llvm field so
maintainers see it when inspecting build_tables_exe.
| var it = uucode.grapheme.utf8Iterator("👩🏽🚀🇨🇭👨🏻🍼") | ||
|
|
||
| // (which is equivalent to:) | ||
| var it = uucode.grapheme.Iterator(uccode.utf8.Iterator).init(.init("👩🏽🚀🇨🇭👨🏻🍼")); |
Contributor
There was a problem hiding this comment.
Typo: uccode should be uucode.
-var it = uucode.grapheme.Iterator(uccode.utf8.Iterator).init(.init("👩🏽🚀🇨🇭👨🏻🍼"));
+var it = uucode.grapheme.Iterator(uucode.utf8.Iterator).init(.init("👩🏽🚀🇨🇭👨🏻🍼"));📝 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
| var it = uucode.grapheme.Iterator(uccode.utf8.Iterator).init(.init("👩🏽🚀🇨🇭👨🏻🍼")); | |
| var it = uucode.grapheme.Iterator(uucode.utf8.Iterator).init(.init("👩🏽🚀🇨🇭👨🏻🍼")); |
🤖 Prompt for AI Agents
In `@src/deps/uucode/README.md` at line 59, Typo: the identifier "uccode" is
misspelled and should be "uucode". Update the README code snippet so the call
uses uucode.utf8.Iterator instead of uccode.utf8.Iterator (i.e., fix the
expression where uucode.grapheme.Iterator(...) is invoked) so the identifiers
match uucode.grapheme.Iterator and uucode.utf8.Iterator.
Comment on lines
+76
to
+85
| pub fn toUpper(c: u21) u21 { | ||
| const mask = @as(u21, @intFromBool(isLower(c))) << 5; | ||
| return c ^ mask; | ||
| } | ||
|
|
||
| /// Lowercases the code point and returns it as-is if already lowercase or not a letter. | ||
| pub fn toLower(c: u21) u21 { | ||
| const mask = @as(u21, @intFromBool(isUpper(c))) << 5; | ||
| return c | mask; | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Inconsistent case conversion implementation.
toUpper uses XOR (^) while toLower uses OR (|). Both should work correctly for their intended purposes, but the asymmetry is subtle:
toUpper: XOR clears bit 5 whenisLoweris true (correct: 'a'^0x20='A')toLower: OR sets bit 5 whenisUpperis true (correct: 'A'|0x20='a')
The logic is correct but consider using the same operation for consistency:
♻️ Optional: Use consistent XOR for both conversions
/// Lowercases the code point and returns it as-is if already lowercase or not a letter.
pub fn toLower(c: u21) u21 {
const mask = `@as`(u21, `@intFromBool`(isUpper(c))) << 5;
- return c | mask;
+ return c ^ mask;
}Both XOR and OR work here because bit 5 is guaranteed to be 0 for uppercase letters, but XOR makes the symmetry with toUpper explicit.
📝 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
| pub fn toUpper(c: u21) u21 { | |
| const mask = @as(u21, @intFromBool(isLower(c))) << 5; | |
| return c ^ mask; | |
| } | |
| /// Lowercases the code point and returns it as-is if already lowercase or not a letter. | |
| pub fn toLower(c: u21) u21 { | |
| const mask = @as(u21, @intFromBool(isUpper(c))) << 5; | |
| return c | mask; | |
| } | |
| pub fn toUpper(c: u21) u21 { | |
| const mask = `@as`(u21, `@intFromBool`(isLower(c))) << 5; | |
| return c ^ mask; | |
| } | |
| /// Lowercases the code point and returns it as-is if already lowercase or not a letter. | |
| pub fn toLower(c: u21) u21 { | |
| const mask = `@as`(u21, `@intFromBool`(isUpper(c))) << 5; | |
| return c ^ mask; | |
| } |
🤖 Prompt for AI Agents
In `@src/deps/uucode/src/ascii.zig` around lines 76 - 85, The toLower
implementation is asymmetric with toUpper (toUpper uses XOR '^' while toLower
uses OR '|'); update toLower (function toLower) to compute the same mask as now
but apply it with XOR (c ^ mask) instead of OR so both case conversions use the
same bit-toggling operation and remain correct; keep the mask generation using
`@intFromBool`(isUpper(c)) << 5 and only change the final operator to '^' to match
toUpper.
Comment on lines
+81
to
+108
| pub const FieldEnum = blk: { | ||
| var fields_len: usize = 0; | ||
| for (@typeInfo(@TypeOf(tables)).@"struct".fields) |tableInfo| { | ||
| fields_len += @typeInfo(TableData(tableInfo.type)).@"struct".fields.len; | ||
| } | ||
|
|
||
| var fields: [fields_len]std.builtin.Type.EnumField = undefined; | ||
| var i: usize = 0; | ||
|
|
||
| for (@typeInfo(@TypeOf(tables)).@"struct".fields) |tableInfo| { | ||
| for (@typeInfo(TableData(tableInfo.type)).@"struct".fields) |f| { | ||
| fields[i] = .{ | ||
| .name = f.name, | ||
| .value = i, | ||
| }; | ||
| i += 1; | ||
| } | ||
| } | ||
|
|
||
| break :blk @Type(.{ | ||
| .@"enum" = .{ | ||
| .tag_type = std.math.IntFittingRange(0, fields_len - 1), | ||
| .fields = &fields, | ||
| .decls = &[_]std.builtin.Type.Declaration{}, | ||
| .is_exhaustive = true, | ||
| }, | ||
| }); | ||
| }; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd -t f "get.zig" | head -20Repository: oven-sh/bun
Length of output: 105
🏁 Script executed:
cat -n src/deps/uucode/src/get.zig | sed -n '81,108p'Repository: oven-sh/bun
Length of output: 1098
🏁 Script executed:
git log --oneline -n 5 -- src/deps/uucode/src/get.zig 2>/dev/null || echo "Not a git repo or file not in git"Repository: oven-sh/bun
Length of output: 88
🏁 Script executed:
rg -B 5 "pub const FieldEnum = blk:" src/deps/uucode/src/get.zig | head -30Repository: oven-sh/bun
Length of output: 197
🏁 Script executed:
rg "const tables" src/deps/uucode/src/get.zigRepository: oven-sh/bun
Length of output: 95
🏁 Script executed:
rg -A 3 "fn TableData|const TableData" src/deps/uucode/src/get.zig | head -50Repository: oven-sh/bun
Length of output: 198
🏁 Script executed:
cat -n src/deps/uucode/src/get.zig | sed -n '1,110p'Repository: oven-sh/bun
Length of output: 4409
🏁 Script executed:
find . -name "tables.zig" -o -name "tables" -type d | grep uucodeRepository: oven-sh/bun
Length of output: 37
🏁 Script executed:
rg "\.tables\s*=" src/deps/uucode/ -A 5 | head -40Repository: oven-sh/bun
Length of output: 310
🏁 Script executed:
rg "const tables" src/deps/uucode/ -A 2 -B 2Repository: oven-sh/bun
Length of output: 2316
🏁 Script executed:
find src/deps/uucode -name "*.zig" -exec grep -l "tables" {} \; | head -10Repository: oven-sh/bun
Length of output: 138
🏁 Script executed:
rg "comptime.*IntFittingRange" --type zig | head -20Repository: oven-sh/bun
Length of output: 37
🏁 Script executed:
rg "IntFittingRange" src/ -B 2 -A 2 | head -50Repository: oven-sh/bun
Length of output: 3064
Guard against empty FieldEnum input to avoid comptime underflow.
Line 102 computes fields_len - 1 for the enum tag range; if a custom config yields zero fields, this underflows and produces a confusing compile error. Add an explicit comptime guard for fields_len == 0.
🛠️ Suggested fix
var fields_len: usize = 0;
for (`@typeInfo`(`@TypeOf`(tables)).@"struct".fields) |tableInfo| {
fields_len += `@typeInfo`(TableData(tableInfo.type)).@"struct".fields.len;
}
+ if (fields_len == 0) {
+ `@compileError`("FieldEnum requires at least one field across tables");
+ }
+
var fields: [fields_len]std.builtin.Type.EnumField = undefined;📝 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
| pub const FieldEnum = blk: { | |
| var fields_len: usize = 0; | |
| for (@typeInfo(@TypeOf(tables)).@"struct".fields) |tableInfo| { | |
| fields_len += @typeInfo(TableData(tableInfo.type)).@"struct".fields.len; | |
| } | |
| var fields: [fields_len]std.builtin.Type.EnumField = undefined; | |
| var i: usize = 0; | |
| for (@typeInfo(@TypeOf(tables)).@"struct".fields) |tableInfo| { | |
| for (@typeInfo(TableData(tableInfo.type)).@"struct".fields) |f| { | |
| fields[i] = .{ | |
| .name = f.name, | |
| .value = i, | |
| }; | |
| i += 1; | |
| } | |
| } | |
| break :blk @Type(.{ | |
| .@"enum" = .{ | |
| .tag_type = std.math.IntFittingRange(0, fields_len - 1), | |
| .fields = &fields, | |
| .decls = &[_]std.builtin.Type.Declaration{}, | |
| .is_exhaustive = true, | |
| }, | |
| }); | |
| }; | |
| pub const FieldEnum = blk: { | |
| var fields_len: usize = 0; | |
| for (`@typeInfo`(`@TypeOf`(tables)).@"struct".fields) |tableInfo| { | |
| fields_len += `@typeInfo`(TableData(tableInfo.type)).@"struct".fields.len; | |
| } | |
| if (fields_len == 0) { | |
| `@compileError`("FieldEnum requires at least one field across tables"); | |
| } | |
| var fields: [fields_len]std.builtin.Type.EnumField = undefined; | |
| var i: usize = 0; | |
| for (`@typeInfo`(`@TypeOf`(tables)).@"struct".fields) |tableInfo| { | |
| for (`@typeInfo`(TableData(tableInfo.type)).@"struct".fields) |f| { | |
| fields[i] = .{ | |
| .name = f.name, | |
| .value = i, | |
| }; | |
| i += 1; | |
| } | |
| } | |
| break :blk `@Type`(.{ | |
| .@"enum" = .{ | |
| .tag_type = std.math.IntFittingRange(0, fields_len - 1), | |
| .fields = &fields, | |
| .decls = &[_]std.builtin.Type.Declaration{}, | |
| .is_exhaustive = true, | |
| }, | |
| }); | |
| }; |
🤖 Prompt for AI Agents
In `@src/deps/uucode/src/get.zig` around lines 81 - 108, The code computes
fields_len - 1 for FieldEnum.tag_type which underflows at comptime when
fields_len == 0; add a comptime guard around the construction of the enum in
FieldEnum (referencing FieldEnum and fields_len) that checks if (fields_len ==
0) and handles that case explicitly (either return an enum with a safe tag_type
range using 0..0 via std.math.IntFittingRange(0, 0) or otherwise produce a clear
comptime error), otherwise use the existing std.math.IntFittingRange(0,
fields_len - 1); ensure the guard is applied where the break :blk `@Type`(...) is
created so no subtraction occurs on zero.
Comment on lines
+853
to
+856
| // Note: while it would be better for modularity to pass `backing` | ||
| // in, this makes for a nicer API without having to wrap Slice. | ||
| const hardcoded_backing = @import("./get.zig").backingFor(c.name); | ||
|
|
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check file size and read around line 855
wc -l src/deps/uucode/src/types.zigRepository: oven-sh/bun
Length of output: 166
🏁 Script executed:
# Read context around line 855 (lines 840-870 to see the structure)
sed -n '840,870p' src/deps/uucode/src/types.zig | cat -nRepository: oven-sh/bun
Length of output: 1279
🏁 Script executed:
# Get more context to understand the struct definition
sed -n '800,870p' src/deps/uucode/src/types.zig | cat -nRepository: oven-sh/bun
Length of output: 2951
Inline @import should be moved to the bottom of the struct, not just separated.
Line 855 contains an inline @import("./get.zig") within the struct body. Per Zig guidelines, imports must be placed at the bottom of the struct or file, not inline. Separate the import from its usage and relocate both to the end of the struct for consistency.
The suggested refactor shown earlier separates the statements but does not move them to the bottom—it should do both.
🤖 Prompt for AI Agents
In `@src/deps/uucode/src/types.zig` around lines 853 - 856, The inline import
`@import`("./get.zig") used to compute hardcoded_backing should be moved out of
the struct body and placed at the bottom of the struct (or file) and replaced
inside the struct with a reference to the previously bound symbol; specifically,
separate the import (e.g., import get = `@import`("./get.zig"); or equivalent) and
the backingFor lookup so that hardcoded_backing within the struct uses
get.backingFor(c.name) (or the chosen alias), and ensure both the import binding
and any non-struct-scope constants are declared after the struct definition to
conform to Zig’s guideline.
Comment on lines
+658
to
+661
| const file_path = "ucd/auxiliary/GraphemeBreakTest.txt"; | ||
|
|
||
| const file = try std.fs.cwd().openFile(file_path, .{}); | ||
| defer file.close(); |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
# First, let's locate and read the file
find . -name "grapheme.zig" -type f 2>/dev/null | head -20Repository: oven-sh/bun
Length of output: 207
🏁 Script executed:
# Check the file content around lines 658-661
if [ -f "src/deps/uucode/src/x/grapheme.zig" ]; then
wc -l "src/deps/uucode/src/x/grapheme.zig"
echo "=== Lines 650-670 ==="
sed -n '650,670p' "src/deps/uucode/src/x/grapheme.zig"
fiRepository: oven-sh/bun
Length of output: 758
🏁 Script executed:
# Search for similar path opening patterns in the Bun codebase to understand best practices
rg "openFileAbsolute|realpathAlloc" --type zig -A 2 -B 2 | head -100Repository: oven-sh/bun
Length of output: 4779
🏁 Script executed:
# Check if bun.path utilities are used in the codebase for similar operations
rg "bun\.path\." --type zig -A 1 -B 1 | head -100Repository: oven-sh/bun
Length of output: 6122
🏁 Script executed:
# Let's check the exact signature and usage of openFileAbsolute vs openFileAbsoluteZ in std
rg "fn openFileAbsolute" --type zig -A 3Repository: oven-sh/bun
Length of output: 37
🏁 Script executed:
# Check if there's a different pattern commonly used for test file paths in bun tests
rg "testGraphemeBreak|test.*ucd" --type zig -B 2 -A 5 | head -50Repository: oven-sh/bun
Length of output: 3313
Use an absolute path for the GraphemeBreakTest.txt file.
The relative path passed to std.fs.cwd().openFile() at line 660 violates the coding guideline requiring absolute paths in file operations for src/**/*.zig files. Resolve the relative path to an absolute path and use openFileAbsolute or openFileAbsoluteZ.
🛠️ Suggested fix
const allocator = std.testing.allocator;
const file_path = "ucd/auxiliary/GraphemeBreakTest.txt";
- const file = try std.fs.cwd().openFile(file_path, .{});
+ const abs_path = try std.fs.cwd().realpathAlloc(allocator, file_path);
+ defer allocator.free(abs_path);
+
+ const file = try std.fs.openFileAbsolute(abs_path, .{});📝 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
| const file_path = "ucd/auxiliary/GraphemeBreakTest.txt"; | |
| const file = try std.fs.cwd().openFile(file_path, .{}); | |
| defer file.close(); | |
| const file_path = "ucd/auxiliary/GraphemeBreakTest.txt"; | |
| const abs_path = try std.fs.cwd().realpathAlloc(allocator, file_path); | |
| defer allocator.free(abs_path); | |
| const file = try std.fs.openFileAbsolute(abs_path, .{}); | |
| defer file.close(); |
🤖 Prompt for AI Agents
In `@src/deps/uucode/src/x/grapheme.zig` around lines 658 - 661, The code opens
"ucd/auxiliary/GraphemeBreakTest.txt" using a relative path via
std.fs.cwd().openFile(); change this to resolve and open an absolute path (use
std.fs.cwd().realPath or build an absolute path from std.fs.path.join if needed)
and call openFileAbsolute or openFileAbsoluteZ instead of openFile — update the
symbol usage around file_path and the open call so the code uses the absolute
path variable and openFileAbsolute/openFileAbsoluteZ on the same file handle
creation (ensure you still defer file.close()).
Comment on lines
+11
to
+23
| ``` | ||
| src/deps/uucode/ ← Vendored uucode library (MIT, don't modify) | ||
| src/unicode/uucode/ ← THIS DIRECTORY: build-time integration | ||
| ├── uucode_config.zig ← Configures which uucode fields to generate | ||
| ├── grapheme_gen.zig ← Generator binary: queries uucode → writes tables | ||
| ├── lut.zig ← 3-level lookup table generator | ||
| └── CLAUDE.md ← You are here | ||
|
|
||
| src/string/immutable/ ← Runtime code (no uucode dependency) | ||
| ├── grapheme.zig ← Grapheme break API + precomputed decision table | ||
| ├── grapheme_tables.zig ← PRE-GENERATED property tables (committed) | ||
| └── visible.zig ← String width calculation (uses grapheme.zig) | ||
| ``` |
Contributor
There was a problem hiding this comment.
Add a language tag to the fenced code block.
markdownlint MD040 requires a language; this will fail lint if enforced.
🔧 Suggested fix
-```
+```text
src/deps/uucode/ ← Vendored uucode library (MIT, don't modify)
src/unicode/uucode/ ← THIS DIRECTORY: build-time integration
├── uucode_config.zig ← Configures which uucode fields to generate
├── grapheme_gen.zig ← Generator binary: queries uucode → writes tables
├── lut.zig ← 3-level lookup table generator
└── CLAUDE.md ← You are here
@@
src/string/immutable/ ← Runtime code (no uucode dependency)
├── grapheme.zig ← Grapheme break API + precomputed decision table
├── grapheme_tables.zig ← PRE-GENERATED property tables (committed)
└── visible.zig ← String width calculation (uses grapheme.zig)
-```
+```📝 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
| ``` | |
| src/deps/uucode/ ← Vendored uucode library (MIT, don't modify) | |
| src/unicode/uucode/ ← THIS DIRECTORY: build-time integration | |
| ├── uucode_config.zig ← Configures which uucode fields to generate | |
| ├── grapheme_gen.zig ← Generator binary: queries uucode → writes tables | |
| ├── lut.zig ← 3-level lookup table generator | |
| └── CLAUDE.md ← You are here | |
| src/string/immutable/ ← Runtime code (no uucode dependency) | |
| ├── grapheme.zig ← Grapheme break API + precomputed decision table | |
| ├── grapheme_tables.zig ← PRE-GENERATED property tables (committed) | |
| └── visible.zig ← String width calculation (uses grapheme.zig) | |
| ``` |
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
11-11: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
In `@src/unicode/uucode/CLAUDE.md` around lines 11 - 23, The fenced code block in
CLAUDE.md is missing a language tag, causing markdownlint MD040; update the
opening fence to include a language (e.g., change the triple backtick line to
```text) so the block becomes ```text ... ```; edit the fenced block in
src/unicode/uucode/CLAUDE.md (the diagram/code block at the top of the file) to
add the language tag.
Comment on lines
+9
to
+28
| /// Must be kept in sync with grapheme.zig's GraphemeBreakNoControl. | ||
| const GraphemeBreakNoControl = enum(u5) { | ||
| other, | ||
| prepend, | ||
| regional_indicator, | ||
| spacing_mark, | ||
| l, | ||
| v, | ||
| t, | ||
| lv, | ||
| lvt, | ||
| zwj, | ||
| zwnj, | ||
| extended_pictographic, | ||
| emoji_modifier_base, | ||
| emoji_modifier, | ||
| indic_conjunct_break_extend, | ||
| indic_conjunct_break_linker, | ||
| indic_conjunct_break_consonant, | ||
| }; |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding a compile-time check for enum synchronization.
The comment notes this enum must stay in sync with grapheme.zig's GraphemeBreakNoControl. To catch drift at compile time rather than relying on comments:
♻️ Proposed compile-time sync check
/// Must be kept in sync with grapheme.zig's GraphemeBreakNoControl.
const GraphemeBreakNoControl = enum(u5) {
other,
prepend,
regional_indicator,
// ... rest of variants
};
+
+comptime {
+ // Verify enum count matches at compile time
+ const local_count = `@typeInfo`(GraphemeBreakNoControl).@"enum".fields.len;
+ const uucode_count = `@typeInfo`(`@TypeOf`(uucode.get(.grapheme_break_no_control, 0))).@"enum".fields.len;
+ if (local_count != uucode_count) {
+ `@compileError`("GraphemeBreakNoControl enum out of sync with uucode");
+ }
+}🤖 Prompt for AI Agents
In `@src/unicode/uucode/grapheme_gen.zig` around lines 9 - 28, Add a compile-time
assertion to ensure this GraphemeBreakNoControl enum stays identical to the
GraphemeBreakNoControl definition used in grapheme.zig: at comptime import or
reference the other file's GraphemeBreakNoControl and assert the variant count
and ordering match (e.g., compare `@typeInfo`(...).Enum.fields or compare
`@enumToInt` of each named variant) so a mismatch fails compilation; place the
comptime assert next to the enum definition and reference the
GraphemeBreakNoControl symbol from both modules.
xhjkl
pushed a commit
to xhjkl/bun
that referenced
this pull request
May 14, 2026
…ven-sh#26376) ## Summary Replace Bun's outdated grapheme breaking implementation with [Ghostty's approach](https://github.com/ghostty-org/ghostty/tree/main/src/unicode) using the [uucode](https://github.com/jacobsandlund/uucode) library. This adds proper **GB9c (Indic Conjunct Break)** support — Devanagari and other Indic script conjuncts now correctly form single grapheme clusters. ## Motivation The previous implementation used a `GraphemeBoundaryClass` enum with only 12 values and a 2-bit `BreakState` (just `extended_pictographic` and `regional_indicator` flags). It had no support for Unicode's GB9c rule, meaning Indic conjunct sequences (consonant + virama + consonant) were incorrectly split into multiple grapheme clusters. ## Architecture ### Runtime (zero uucode dependency, two table lookups) ``` codepoint → [3-level LUT] → GraphemeBreakNoControl enum (u5, 17 values) (state, gb1, gb2) → [8KB precomputed array] → (break_result, new_state) ``` The full grapheme break algorithm (GB6-GB13, GB9c, GB11, GB999) runs only at **comptime** to populate the 8KB decision array. At runtime it's pure table lookups. ### File Layout ``` src/deps/uucode/ ← Vendored library (MIT, build-time only) src/unicode/uucode/ ← Build-time integration ├── uucode_config.zig ← What Unicode properties to generate ├── grapheme_gen.zig ← Generator: queries uucode → writes tables ├── lut.zig ← 3-level lookup table generator └── CLAUDE.md ← Maintenance docs src/string/immutable/ ← Runtime (no uucode dependency) ├── grapheme.zig ← Grapheme break API + comptime decisions ├── grapheme_tables.zig ← Pre-generated tables (committed, ~91KB source) └── visible.zig ← Width calculation (2 lines changed) scripts/update-uucode.sh ← Update vendored uucode + regenerate ``` ### Key Types | Type | Size | Values | |------|------|--------| | `GraphemeBreakNoControl` | u5 | 17 (adds `indic_conjunct_break_{consonant,linker,extend}`, `emoji_modifier_base`, `zwnj`, etc.) | | `BreakState` | u3 | 5 (`default`, `regional_indicator`, `extended_pictographic`, `indic_conjunct_break_consonant`, `indic_conjunct_break_linker`) | ### Binary Size The tables store only the `GraphemeBreakNoControl` enum per codepoint (not width or emoji properties, which visible.zig handles separately): - stage1: 8192 × u16 = **16KB** (maps high byte → stage2 offset) - stage2: 27392 × u8 = **27KB** (maps to stage3 index; max value is 16) - stage3: 17 × u5 = **~17 bytes** (one per enum value) - Precomputed decisions: **8KB** - **Total: ~51KB** (vs previous ~70KB+) ## How to Regenerate Tables ```bash # After updating src/deps/uucode/: ./scripts/update-uucode.sh # Or manually: vendor/zig/zig build generate-grapheme-tables ``` Normal builds never run the generator — they use the committed `grapheme_tables.zig`. ## Testing ```bash bun bd test test/js/bun/util/stringWidth.test.ts ``` New test cases verify Devanagari conjuncts (GB9c): - `क्ष` (Ka+Virama+Ssa) → single cluster, width 2 - `क्ष` (Ka+Virama+ZWJ+Ssa) → single cluster, width 2 - `क्क्क` (Ka+Virama+Ka+Virama+Ka) → single cluster, width 3 --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Replace Bun's outdated grapheme breaking implementation with Ghostty's approach using the uucode library. This adds proper GB9c (Indic Conjunct Break) support — Devanagari and other Indic script conjuncts now correctly form single grapheme clusters.
Motivation
The previous implementation used a
GraphemeBoundaryClassenum with only 12 values and a 2-bitBreakState(justextended_pictographicandregional_indicatorflags). It had no support for Unicode's GB9c rule, meaning Indic conjunct sequences (consonant + virama + consonant) were incorrectly split into multiple grapheme clusters.Architecture
Runtime (zero uucode dependency, two table lookups)
The full grapheme break algorithm (GB6-GB13, GB9c, GB11, GB999) runs only at comptime to populate the 8KB decision array. At runtime it's pure table lookups.
File Layout
Key Types
GraphemeBreakNoControlindic_conjunct_break_{consonant,linker,extend},emoji_modifier_base,zwnj, etc.)BreakStatedefault,regional_indicator,extended_pictographic,indic_conjunct_break_consonant,indic_conjunct_break_linker)Binary Size
The tables store only the
GraphemeBreakNoControlenum per codepoint (not width or emoji properties, which visible.zig handles separately):How to Regenerate Tables
Normal builds never run the generator — they use the committed
grapheme_tables.zig.Testing
bun bd test test/js/bun/util/stringWidth.test.tsNew test cases verify Devanagari conjuncts (GB9c):
क्ष(Ka+Virama+Ssa) → single cluster, width 2क्ष(Ka+Virama+ZWJ+Ssa) → single cluster, width 2क्क्क(Ka+Virama+Ka+Virama+Ka) → single cluster, width 3