Skip to content

Remove the Zig-emitting unicode table generator from misctools - #41473

Open
robobun wants to merge 5 commits into
mainfrom
robobun/6a891167/dead-code-sweep
Open

robobun wants to merge 5 commits into
mainfrom
robobun/6a891167/dead-code-sweep

Conversation

@robobun

@robobun robobun commented Sep 6, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

  • misctools/gen-unicode-table.ts and misctools/unicode-generator.ts (246 lines) emit Zig source (pub fn name(cp: u21) bool, [_]u16{...}). The repository has no Zig sources. The identifier tables they produced live in src/bun_core/string/identifier.rs as Rust. Nothing references either script.
  • misctools/package.json exists only for their @unicode/* dependencies. The two remaining misctools scripts import node builtins only.

Fix

  • Delete the two generator scripts and misctools/package.json. That is the whole diff: three files, 260 lines, nothing under src/.
  • The marker comment in identifier.rs ("re-run the identifier-table generator with .rs output") stays as it is. No .rs output mode ever existed, so the comment already points at work not yet done. A reword belongs with a port of the generator, if one happens.
  • Verified: bun bd builds. bun bd test passes on test/bundler/transpiler/transpiler.test.js, test/bundler/transpiler/template-literal.test.ts, test/js/web/console/console-log.test.ts, and test/internal/source-lints/dead-code-escapes.test.ts.

Background

  • This sweep is small on purpose. Twenty open robobun dead-code PRs already claim nearly every unreferenced symbol the scans find. Details and the items left for a human decision are in the notes.
  • No test is added. Every removed line is unreferenced, so there is no behavior a test can observe. test/internal/source-lints/CLAUDE.md asks for no lint tests for dead symbols.
Notes

History of this PR: the first revision also removed the writer entry from src/js/builtins/BunBuiltinNames.h and reworded the identifier.rs comment. The writer removal was wrong. ConsoleObject.ts reaches the private symbol by string, $getByIdDirectPrivate(this, "writer"), so console.write crashed in CI (build 110635, ASSERTION FAILED: symbol in emit_intrinsic_getByIdDirectPrivate). Both src/ edits were reverted. A private name can be live with no $name or namePrivateName() mention anywhere.

Scans run against HEAD f42e980255, with every file listed in an open robobun dead-code PR excluded:

  • Rust: the workspace denies dead_code and unreachable_pub, so intra-crate dead code cannot exist. A cross-crate scan of 14,624 pub items, enum variants, and pub struct fields (rg on the name, comment lines excluded) found 0 unclaimed dead items. Every hit was a proc-macro entry point, an FFI struct field, a derive-parsed CSS keyword variant, or a symbol an open PR already deletes.
  • Built-in JS (src/js): 3,608 top-level declarations, 0 dead outside internal-for-testing.ts. All 45 export functions in src/js/builtins are referenced. tsc --noUnusedLocals reports only intentional _-prefixed locals. kServerSocket in net.ts is write-only by design (a GC edge).
  • C++: 12,121 function definitions plus class-body methods in .h files. Every zero-reference hit is a macro-pasted name (jsWebStreamsHandler_*, Bun__*JobCtx__*), an N-API export, sqlite, or already claimed.
  • Unreferenced headers: only v8config.h, already claimed. Unused Cargo dependencies: only bytemuck in bun_paths, already claimed. .rs files outside any mod tree: none.
  • BunBuiltinNames.h: every entry is live. mockedFunction is aliased by BunCommonStrings.h. writer is reached through $getByIdDirectPrivate(this, "writer"). Both escape a plain $name search.

Unreferenced but deliberately kept, left for a human decision (not in this diff):

  • packages/h3blast/ (1,465 lines): a standalone HTTP/3 load tool added in Bun.serve: HTTP/3 (QUIC) support via h3: true #29768. Nothing builds, tests, or documents it.
  • packages/bun-plugin-yaml/ (220 lines): the published package source. Tests install it from npm. Bun has a native YAML loader now.
  • packages/bun-debug-adapter-protocol/src/protocol/protocol.json (3,779 lines): the generator writes it, nothing reads it. index.d.ts is the consumed output.
  • scripts/lldb-inline-tool.cpp + lldb-inline.sh, scripts/gamble.ts, scripts/debug-coredump.ts, scripts/github-metrics.ts, scripts/trace.sh: hand-run developer tools with no caller.
  • misctools/lldb/bun_pretty_printer.py: loaded from .lldbinit, but its printers match Zig type names (ZigString, baby_list.BabyList) that no longer exist.
  • scripts/packer/build-image.pkr.hcl: machine.mjs only ever selects the azure-arm sources. The docker.debian and amazon-ebs.debian sources are never built.

no test proof · iteration 3 · no src or test change; test-proof not applicable

misctools/gen-unicode-table.ts and misctools/unicode-generator.ts emit Zig
source for identifier tables. The repository has no Zig sources and the
tables live in src/bun_core/string/identifier.rs as Rust. Nothing
references the two scripts. misctools/package.json only held their
@unicode dependencies, and the remaining misctools scripts import only
node builtins.

BunBuiltinNames.h: the writer private identifier has no user in the JS
builtins, the C++ bindings, or the generated code.
@robobun

robobun commented Sep 6, 2026 •

Copy link
Copy Markdown
Collaborator Author
Updated 11:38 PM PT - Sep 5th, 2026

❌ @robobun, your commit 46729e5 has 1 failures in Build #110712 (All Failures):


🧪   To try this PR locally:

bunx bun-pr 41473

That installs a local version of the PR into your bun-41473 executable, so you can run:

bun-41473 --bun

@robobun

robobun commented Sep 6, 2026

Copy link
Copy Markdown
Collaborator Author

No test is added on purpose. Every removed line is unreferenced, so there is no behavior a test can observe before and after the change. test/internal/source-lints/CLAUDE.md asks for no lint tests for dead symbols, and the earlier sweeps of this kind (#39795, #40610, #40881) landed without one.

The code paths next to this change are already covered and pass on the debug build of this branch: test/bundler/transpiler/transpiler.test.js exercises the identifier tables in identifier.rs (the only change there is a comment), test/js/bun/util/filesink.test.ts and test/js/web/console/console-log.test.ts exercise Bun.stdout.writer() and the Console object that once used the $writer private name, and test/js/bun/test/mock-fn.test.js exercises the mockedFunction common string that stays. The rebuilt BunBuiltinNames+extras.h does not add writer back, which confirms no JS builtin references it.

@coderabbitai

coderabbitai Bot commented Sep 6, 2026 •

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Essentials

Run ID: cbc37b17-b7fe-4dc5-8f64-4ba69e019373

📥 Commits

Reviewing files that changed from the base of the PR and between f42e980 and 895f26b.

📒 Files selected for processing (5)
  • misctools/gen-unicode-table.ts
  • misctools/package.json
  • misctools/unicode-generator.ts
  • src/bun_core/string/identifier.rs
  • src/js/builtins/BunBuiltinNames.h
💤 Files with no reviewable changes (4)
  • misctools/package.json
  • misctools/unicode-generator.ts
  • misctools/gen-unicode-table.ts
  • src/js/builtins/BunBuiltinNames.h

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


Walkthrough

The PR deletes Unicode table generator tooling, updates the generated Unicode table comment, and removes writer from the common private builtin-name declarations.

Changes

Unicode generator cleanup

Layer / File(s) Summary
Remove Unicode table generation tooling
misctools/gen-unicode-table.ts, misctools/unicode-generator.ts, misctools/package.json, src/bun_core/string/identifier.rs
The Unicode generator scripts and package metadata are deleted. The generated identifier table comment now identifies Unicode 15.1 ID_Start and ID_Continue data.

Builtin name cleanup

Layer / File(s) Summary
Remove the writer builtin name
src/js/builtins/BunBuiltinNames.h
The writer entry is removed from the common private builtin-name list, along with its generated enum value and accessors.

Merge Risk: ⚪ Minimal · up to 05555

This cleanup removes unused tooling and a stale builtin identifier while only clarifying Unicode table metadata. No current merge-blocking risk is identified.

🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description check ✅ Passed The description clearly explains the problem, the fix, verification steps, test results, and scope. It does not use the exact template headings, but it provides the required information in equivalent …
Title check ✅ Passed The title clearly identifies the primary change: removing the Zig-emitting Unicode table generator from misctools. It is concise and directly related to the changeset.

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

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.

Comment thread src/js/builtins/BunBuiltinNames.h Resolved
ConsoleObject.ts reaches it by string through $getByIdDirectPrivate(this, "writer"), so the console.write builtin needs the private symbol registered.
Comment thread src/bun_core/string/identifier.rs Resolved
@robobun robobun changed the title Remove the Zig-emitting unicode table generator and a stale builtin name Remove the Zig-emitting unicode table generator from misctools Sep 6, 2026

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM — with the src/ edits reverted, this is now a pure deletion of three unreferenced Zig-era codegen files.

What was reviewed:

  • Grepped the tree for gen-unicode-table, unicode-generator, and misctools/package.json — zero hits; identifier.rs does not name the script.
  • Confirmed the two remaining misctools scripts import only node builtins and bun, so dropping misctools/package.json leaves nothing unresolved.
  • Verified the net diff against base touches only the three deleted files; the earlier BunBuiltinNames.h and identifier.rs edits are fully backed out.
Extended reasoning...

Overview

The PR now deletes exactly three files under misctools/: gen-unicode-table.ts (108 lines), unicode-generator.ts (138 lines), and package.json (14 lines). The two scripts emitted Zig source (pub fn ... (cp: u21) bool, [_]u16{...}) for identifier lookup tables — output the Rust codebase cannot consume. The package.json existed solely to declare their @ unicode/* dependencies. Since my earlier review, the author reverted the BunBuiltinNames.h macro(writer) removal (which CI showed was live via $getByIdDirectPrivate(this, "writer")) and the identifier.rs comment reword, so the diff no longer touches src/ at all.

Security risks

None. This is a pure deletion of local developer tooling that is not built, shipped, or invoked by any script, workflow, or source file. No runtime code, dependency manifest that ships, or CI configuration is affected.

Level of scrutiny

Low. The only obligation for a deletion-only PR under REVIEW.md is proving nothing still references what is removed. A repo-wide grep for each filename and for misctools/package.json returns zero hits; src/bun_core/string/identifier.rs does not reference the script by name. The two surviving misctools scripts (generate-add-completions.ts, generate-cli-completions.ts) import only fs/path/bun, so removing the manifest cannot break them. No CODEOWNERS rule covers misctools/.

Other factors

My prior inline comment concerned the since-reverted BunBuiltinNames.h change and is now moot — that file is no longer in the diff. There are no outstanding third-party CHANGES_REQUESTED reviews. The bug hunt exited on dry_streak with no findings. Given the diff is now a 260-line pure deletion of demonstrably dead Zig-targeting codegen with no remaining references, approval is appropriate.

@robobun

robobun commented Sep 6, 2026

Copy link
Copy Markdown
Collaborator Author

CI status: the diff deletes three files under misctools/ and touches nothing under src/ or test/. The remaining red on build 110712 is unrelated flake: bun-install-registry, cpu-prof, no-orphans, require-cache, and in-process-cron each passed on retry, and the napi test_object/do.test.ts stall is pre-existing on main. Build 110651 failed on a darwin-aarch64 runner that lost its build artifact mid-run, also unrelated. Ready for a maintainer.

This branch has not been deployed

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants