http_types: delete unused can_open_in_editor and URLPath::is_root/path_without_asset_prefix#31552
Conversation
…s_root/path_without_asset_prefix
|
Warning Review limit reached
More reviews will be available in 11 minutes. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughPR removes MimeType's ChangesHTTP types API refactoring
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
|
Updated 1:05 AM PT - May 29th, 2026
❌ @dylan-conway, your commit f730821 has 2 failures in
🧪 To try this PR locally: bunx bun-pr 31552That installs a local version of the PR into your bun-31552 --bun |
…tor); update CLAUDE.md example
There was a problem hiding this comment.
LGTM — thanks for folding in the is_code() cascade; the remaining is_source_map field nit is non-blocking.
Extended reasoning...
Overview
Pure dead-code sweep in bun_http_types: removes MimeType::can_open_in_editor(), Category::is_code() (added in the follow-up commit after my earlier note), URLPath::is_root(), and URLPath::path_without_asset_prefix(), plus a one-line doc example swap in src/CLAUDE.md. Net change is ~50 lines deleted, 1 line modified. I verified via grep that no Rust callers remain for any of the removed items.
Security risks
None. This is deletion-only; no new code paths, inputs, or behavior. The deleted functions were unreachable helpers with no auth/crypto/IO surface.
Level of scrutiny
Low. Removing provably-uncalled pub methods from an internal crate carries effectively zero runtime risk — cargo check --workspace passing is the right verification bar, and the PR description documents the Zig-side dead-code analysis for each item. The CodeRabbit walkthrough mistakenly describes take_decoded_storage() as new, but the diff shows it as unchanged context only.
Other factors
My previous inline comment about the is_code() cascade was addressed in commit f730821. The new inline nit (the now-write-only URLPath::is_source_map struct field) is the same one-hop cascade pattern on the URLPath side — harmless to leave, fine to fold in if you want a complete sweep. The musl LTO build failure reported by robobun on the earlier commit ("Linking two modules of different data layouts … '' vs …") is an llvm-link/bitcode infrastructure issue unrelated to deleting these Rust methods.
| @@ -29,36 +29,6 @@ pub struct URLPath { | |||
| } | |||
|
|
|||
| impl URLPath { | |||
There was a problem hiding this comment.
🟡 🟡 Same cascade pattern on the URLPath side: deleting path_without_asset_prefix() orphans the pub is_source_map: bool struct field (line 18) — its sole reader was the deleted if self.is_source_map && out.ends_with(b".map"), and the Zig field's only reader is inside pathWithoutAssetPrefix (documented dead-in-Zig in this PR's table). The field decl and its struct-literal write at line ~176 could go in the same sweep. Note: the local is_source_map in parse() (lines 146/148/171) is still live — only the struct field is dead.
Extended reasoning...
What this is
Not a correctness bug — an incomplete sweep, the same one-hop-downstream cascade already addressed for Category::is_code() in commit f730821. Deleting URLPath::path_without_asset_prefix() removes the only reader of the pub is_source_map: bool struct field, so the field now meets the identical "0 Rust readers / dead in Zig too" criterion this PR uses for the other deletions.
Step-by-step proof that the struct field is now dead
- Rust side, before this PR: the only read of
self.is_source_mapwas insidepath_without_asset_prefix()at the line being deleted:if self.is_source_map && out.ends_with(b".map") { return &out[0..out.len() - 4]; }
- Rust side, after this PR: grepping all
*.rsforis_source_mapyields onlyURLPath.rshits — the field declaration at:18, the local variable inparse()at:146/:148/:171, and the struct-literal write at:176. None of the six Rust files that referenceURLPath(fetch.rs,filesystem_router.rs,router/lib.rs,http_types/lib.rs,http/lib.rs) touch the field. Zero field readers remain. - Zig side: the field is declared at
URLPath.zig:11and written at:135. Its only read isURLPath.zig:28, insidepathWithoutAssetPrefix— which the PR description's own table documents as having 0 callers (its only callerisRootis itself dead). So the field is transitively dead in Zig too. - Conclusion:
URLPath::is_source_mapmeets the same "0 Rust readers / dead in Zig too" bar that justified deletingis_root()/path_without_asset_prefix()/can_open_in_editor(), and is the exact analogue of theis_code()cascade already accepted and fixed.
What is not dead
The local let is_source_map = extname == b"map"; at parse():146 is still live — it drives the extname/path recomputation at :148 and :171. Only the struct field (line 18) and its assignment in the struct literal (line 176, plus the doc comment at 16–17) are dead.
Why nothing else flags this
The field is pub, so rustc's dead-code lint won't fire. There's no test or external consumer reading it. It survives only because it's one hop downstream of the function being deleted, and the sweep stopped at the first level — same as is_code() was before the follow-up commit.
Impact
None on runtime behavior — purely about completing the cleanup. Leaving the field in place is harmless; a later sweep would catch it.
Suggested fix
Either:
- Extend this PR to also delete
pub is_source_map: bool(URLPath.rs:16-18) and theis_source_map,line in the struct literal (:176) — the local variable stays, or - Intentionally keep it as parser output for Zig-port parity — in which case feel free to disregard.
Dead-code sweep of
bun_http_types.cargo check --workspacepasses.Why this code exists
MimeType::can_open_in_editorsrc/http_types/MimeType.zig:64(canOpenInEditor)URLPath::is_rootsrc/http_types/URLPath.zig:13(isRoot)URLPath::path_without_asset_prefixURLPath.zig:21(pathWithoutAssetPrefix)isRootat:14(same file), itself dead