Skip to content

Remove dead code from bun_glob and bun_css - #46196

Open
robobun wants to merge 1 commit into
mainfrom
robobun/082c115e/dead-code-sweep
Open

robobun wants to merge 1 commit into
mainfrom
robobun/082c115e/dead-code-sweep

Conversation

@robobun

@robobun robobun commented Oct 20, 2026

Copy link
Copy Markdown
Collaborator

Behaviour change: none

Problem

Fix

  • Delete the field with its two writes, and the macro arm: 4 lines in 2 files.
  • Correct because nothing reads the field. With #[deprecated] on it, rustc lists two uses, and both are writes. The local variable has_choice stays and still feeds has_group and is_decided_at_once.
  • Verified: bun bd, bun run rust:check-all (12 targets), test/js/bun/glob/match.test.ts, test/js/bun/css/color.test.ts (Notes).
  • Self-reviewed: 29 concerns raised, 29 addressed, most by the cut from 188 files to these 2 (Notes).

Background

  • A write-only field is state that code stores and no code loads.
  • define_colorspace! generates a color type. Each @gamut arm generates the ColorGamut impl for one kind of gamut.
  • No other design was weighed: a deletion has one place.

Downsides

  • None found. Checked: the lines that 5,848 pull request heads add or delete. Checked also: a trial merge with each of the 39 heads that touch these two files.
Notes

Why this run is small

  • The run looked for code with no reference. It covered the C++ bindings, uSockets, uWS, the Rust crates, the built-in TypeScript and the build scripts.
  • Almost all that is certainly dead is already in an open pull request. One of the 61 open dead-code pull requests deletes it, or another pull request writes it. These 4 lines are what no other pull request has.
  • The first draft of this branch had 188 files. A self-review cut it. 844 of its 856 lines were preprocessor guards, not unreferenced code. They gave open pull requests new conflicts (see "Held").

Found dead in this run, and left alone

Held: the ENABLE(WEB_CRYPTO) guards

Verification

  • On main at ea6a2c7 plus this commit.
  • bun bd (debug, ASAN) builds and links.
  • bun run rust:check-all: 12 ok, 0 failed, 0 skipped.
  • cargo fmt --all --check: clean.
  • Tests with the debug build: test/js/bun/glob/match.test.ts (574 pass), test/js/bun/glob/proto.test.ts (4 pass, 1 skip), test/cli/lint/glob.test.ts (63 pass), test/js/bun/css/color.test.ts (1022 pass, 1 skip). No failure.
  • test/js/bun/glob/scan.test.ts: 200 pass, 6 fail. The six tests compare a scan of the whole checkout with the fast-glob package. Each has a fixed limit of 30 s, which the debug build does not meet on this machine. The release build 1.4.3 needs 2 to 6 s for each. The change deletes a field that nothing reads.
  • rustc -W unused_macro_rules reports the macro arm on main and nothing in color.rs after the change.

Open pull requests

  • The scan covers 5,848 heads. 5,763 pull requests were open on 2026-10-19, and 85 opened since.
  • 39 touch matcher.rs or color.rs. 25 of them merge cleanly with main, and the same 25 merge cleanly with this branch. No head gets a new conflict file.
  • No head adds a use of has_choice or of gamut = none, and no head deletes one of these 4 lines.

How the candidates were found

  • Linker: the debug objects were linked a second time with --gc-sections --print-gc-sections. The linker drops 376 first-party C and C++ functions and 3,342 Rust functions. Open pull requests already delete 179 of the C++ functions. The other 197 are template instances, code for Windows or macOS, or macro output, except getWebSocketConstructor.
  • clang: -Wunused-function -Wunused-member-function -Wunused-template -Wunused-macros over 608 files and the 71 unified sources.
  • Rust fields: in a scratch tree each of 19,043 named struct fields got #[deprecated]. One cargo check --workspace then lists every use (183,184). This found has_choice.
  • rustc -W unused_macro_rules found the macro arm.
  • Also run: rustc -W unused_crate_dependencies, tsc --noUnusedLocals on the 15 TypeScript projects, and a name index over all tracked sources. Two more searches looked for files that nothing names and for prototypes with no definition.

Self-review

  • The review raised 29 concerns about the 188-file draft. The main ones:
  • This PR follows its recommendation for each. It ships only the lines no other pull request has. It records the rest here. It tests conflicts against every open head.
  • Two follow-ups are not in this PR: the guard removal above, and a full re-run of the field probe as its own sweep.

Delete the field Parsed.has_choice of the glob matcher, which is
assigned and never read, and the @gamut none arm of define_colorspace!,
which no invocation matches.
@robobun

robobun commented Oct 20, 2026 •

Copy link
Copy Markdown
Collaborator Author
Updated 11:23 AM PT - Oct 20th, 2026

✅ @robobun, your commit 6c32571f56fae4c80b3729d3dc2c2701abe87127 passed in Build #129528! 🎉


🧪   To try this PR locally:

bunx bun-pr 46196

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

bun-46196 --bun

@coderabbitai

coderabbitai Bot commented Oct 20, 2026

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository: oven-sh/bun/.coderabbit.yaml
  • Review profile: ASSERTIVE
  • Plan: Advanced
  • Run ID: 6faa2b07-8b52-4334-8da4-93b02d773138
📥 Commits

Reviewing files that changed from the base of the PR and between ea6a2c7 and 6c32571.

📒 Files selected for processing: 2
  • src/css/values/color.rs
  • src/glob/matcher.rs
💤 Files with no reviewable changes: 2
  • src/css/values/color.rs
  • src/glob/matcher.rs

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 3 remain after this review.

Walkthrough

The changes remove a no-op colorspace macro arm and the private glob parser field has_choice. Choice detection remains incorporated into has_group during parsing.

Changes

Colorspace macro

Layer / File(s) Summary
Remove no-op gamut arm
src/css/values/color.rs
Removes the @gamut none arm from the define_colorspace! macro.

Glob parser metadata

Layer / File(s) Summary
Remove redundant choice metadata
src/glob/matcher.rs
Removes the private Parsed::has_choice field, its initialization, and its standalone assignment. Choice detection remains part of has_group during parsing.

Suggested reviewers: jarred-sumner

Priority: ⬇️ Low

Merge Risk: ⚪ Minimal · up to 6c325

The cleanup is mergeable after normal checks; no behavior change or outstanding issue was identified.

Pre-merge checks: Passed 4
Check name Status Explanation
Title check Passed The title clearly identifies the removal of dead code in the two affected areas, bun_glob and bun_css.
Description check Passed The description explains the problem, the exact fixes, the lack of behavior changes, scope decisions, and verification results. It does not use the template headings exactly, but it provides the requi…
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.
Full details: Description check

Explanation

The description explains the problem, the exact fixes, the lack of behavior changes, scope decisions, and verification results. It does not use the template headings exactly, but it provides the required information in equivalent sections.

✨ Finishing Touches: 💡 1
🛠️ Fix failing CI checks: 💡
  • Commit to this branch
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

❤️ Share

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.

LGTM — a four-line dead-code deletion with no behavior change.

What was reviewed:

  • Parsed::has_choice in src/glob/matcher.rs: grepped the repo for .has_choice reads — none; the local has_choice still feeds has_group and is_decided_at_once on the same lines, so matching is unchanged.
  • The @ gamut none arm of define_colorspace! in src/css/values/color.rs: the macro is file-local and all 14 invocations use unbounded, bounded or hsl_hwb, so the arm was unreachable; a future gamut = none would now be a compile error rather than a silent no-op impl, which is the safer failure.
  • Checked for size/layout assertions on Parsed (embedded in Once<Parsed> and Option<Parsed>) — none exist, and the struct is crate-private.
Extended reasoning...

The change deletes a write-only bool field from the private glob Parsed struct (declaration, initializer, final store) and an unreachable arm of the file-local define_colorspace! macro; no lines are added. It touches no security-sensitive surface. Approval rests on the grep showing no reader of the field and no gamut = none invocation anywhere in the repo, and on the diff being purely subtractive with the existing glob and css test suites covering the affected code paths. Neither file is covered by CODEOWNERS and there are no third-party objections on the PR.

@robobun

robobun commented Oct 20, 2026 •

Copy link
Copy Markdown
Collaborator Author

Status: ready for review. CI is green: Buildkite build 129528 passed, 181 of 181 jobs.

How to check each deletion on main:

  • git grep -n has_choice src/glob/matcher.rs shows the declaration, the false in Parsed::new, one assignment, and the reads of the local variable of the same name. Nothing reads the field.
  • RUSTFLAGS='--cap-lints warn -W unused_macro_rules' cargo check -p bun_css reports rule #5 of macro define_colorspace is never used at src/css/values/color.rs:1613. With this change it reports nothing for that file.

Verified on ea6a2c7 plus this commit: bun bd, bun run rust:check-all (12 ok, 0 failed), and the glob and CSS color tests named in the description.

has_choice came with #45043 one week ago. If it is the start of later work, say so and I drop that hunk.

The one red check, claude-find-issues, is a review job and not a build or a test. It failed 39 seconds after the PR opened. The code review on this PR reports no issue.

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