Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 0 additions & 2 deletions src/ast/import_record.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,6 @@
//!
//! Lives in `bun_ast` so `Ast` (which holds `Vec<ImportRecord>`) is
//! self-contained and `bun_js_printer` can drop its `bun_js_parser` dep.
//! `ImportKind::to_api()` lives in `bun_ast::ImportKindExt` (would
//! back-edge into the schema crate).

use crate::Range;
use bun_paths::fs::Path;
Expand Down
3 changes: 0 additions & 3 deletions src/ast/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -131,9 +131,6 @@ impl ImportKind {
|| self == Self::Url
|| self == Self::Composes
}

// `to_api()` lives in `bun_ast::ImportKindExt` — depends on
// `schema::api::ImportKind` which sits in a higher-tier crate.
}

// ───────────────────────────────────────────────────────────────────────────
Expand Down
8 changes: 4 additions & 4 deletions src/ast/loader.rs
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
//! `Loader` + `SideEffects`.
//!
//! Data-only enum + pure predicates. `to_api()` / `from_api()` / `API_NAMES` /
//! `LoaderOptional::from_api` live in `bun_options_types::LoaderExt` (would
//! back-edge into the schema crate). `to_mime_type` / `from_mime_type` live in
//! `bun_http_types` (would back-edge into `bun_http::MimeType`).
//! Data-only enum + pure predicates. `to_api()` / `from_api()` / `API_NAMES`
//! live in `bun_options_types::LoaderExt` (would back-edge into the schema
//! crate). `to_mime_type` / `from_mime_type` live in `bun_http_types` (would
//! back-edge into `bun_http::MimeType`).

Check warning on line 6 in src/ast/loader.rs

View check run for this annotation

Claude / Claude Code Review

PR orphans bun_ast::LoaderOptional without deleting it

Removing `LoaderOptionalExt` deletes the last callers of `bun_ast::LoaderOptional` — after this PR only its definition (src/ast/loader.rs:70-78) and the re-export at src/ast/lib.rs:2987 remain, with zero references anywhere else in the workspace. Per REVIEW.md's "delete dead code in the same PR that makes it dead", the `LoaderOptional` struct + `NONE`/`from_loader` and its `pub use` should be removed here too (and optionally added to `removed-dead-symbols.test.ts`).
Comment thread
robobun marked this conversation as resolved.
Resolved
Comment on lines +3 to +6

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.

🟡 The PR description's robobun evidence block is stale: it uses test/internal/source-lints/removed-dead-symbols.test.ts as the entire fails-on-main/passes-on-PR gate proof and lists it as +85 in the diff hotspot ("10 files changed"), but commit f79e101 deletes that test with no stated reason — the final diff is 9 files with no test/ entry. The resolved LoaderOptional thread also says an entry was "added to removed-dead-symbols.test.ts", which is no longer true. Please either restore the test if the deletion was accidental, or update the description to drop the stale evidence block/diffstat and note why the source-lint was dropped (per REVIEW.md "Every deletion needs a stated reason or replacement" and CLAUDE.md #11).

Extended reasoning...

What the issue is

Commit f79e101 — the tip of this PR — deletes test/internal/source-lints/removed-dead-symbols.test.ts with a bare commit message ("Delete test/internal/source-lints/removed-dead-symbols.test.ts") and no stated reason. That file was added earlier in this same PR (da9816a) and extended in 3f4e095. However, the PR description was not regenerated after the deletion, so the auto-generated robobun evidence block still presents the deleted test as the only proof that the change is correct.

Where the description is now inconsistent with what ships

Four places in the PR body/timeline reference the deleted file:

  1. The fails-on-main / passes-on-PR gate — both collapsibles run removed-dead-symbols.test.ts and show its 8 assertions ("8 pass / 0 fail") as the entire "[review] gate passed" proof. Nothing in that block still exists in the final diff.
  2. The diff hotspot table — lists .../source-lints/removed-dead-symbols.test.ts | 85 +++++++... and totals "10 files changed, 93 insertions(+), 129 deletions(-)". The actual final diff (git diff 47597ab2..HEAD --shortstat) is 9 files with no test/ entry; the manually-written summary at the top of the description ("Net: +7 / -114") is closer to reality than the evidence block.
  3. The evidence-per-changed-file table — lists the test file with "0 reads / 2 edits".
  4. The resolved LoaderOptional review thread — the robobun reply says the fix "added an entry to removed-dead-symbols.test.ts", which is now false.

Step-by-step proof

  1. git log --oneline on the PR head shows, in order: 7b6eb3f (dead-code removal) → da9816a (adds the test) → 3f4e095 (removes LoaderOptional, extends the test) → f79e101 (deletes the test).
  2. ls test/internal/source-lints/ on the PR head lists dead-code-escapes.test.ts and siblings but not removed-dead-symbols.test.ts.
  3. The PR's changed-files metadata lists exactly 9 files, all under src/; git diff 47597ab2..HEAD --shortstat confirms 9 files changed.
  4. The PR description's <!-- robobun:evidence:begin --> block was generated at 3f4e095 (its build hashes are 3f4e09509) and was never regenerated after f79e101.
  5. Therefore a reviewer reading the description will conclude an 8-assertion regression guard ships with this PR when in fact no test ships at all.

Why this matters

REVIEW.md states "Every deletion needs a stated reason or replacement", and CLAUDE.md #11 says "NEVER overstate what you got done or what actually works in commits, PRs". The description currently overstates what ships: it presents test evidence for a test that isn't in the final diff. This is not a code defect — the 9 dead-code deletions themselves are correct, bun bd builds, and rust:check-all passes on all 10 targets — but the PR body materially misrepresents the artifact a reviewer is approving.

Impact

Nothing breaks at runtime if merged as-is. The risk is purely process/review: a maintainer skimming the evidence block would approve believing a regression guard exists against these symbols reappearing, when the PR actually relies on the pre-existing test/internal/source-lints/dead-code-escapes.test.ts (or nothing) instead.

How to fix

Either:

  • (a) Restore removed-dead-symbols.test.ts if f79e101 was accidental (e.g. a rebase mishap), or
  • (b) If the deletion was intentional (e.g. maintainer preferred relying on the existing dead-code-escapes.test.ts in the same directory, or decided a per-symbol source-lint isn't worth the maintenance): regenerate/edit the PR description to drop the stale robobun evidence block and diff hotspot, and add one line stating why the source-lint test was dropped. Optionally amend the resolved LoaderOptional thread reply to strike the "added an entry to removed-dead-symbols.test.ts" claim.

Anchoring this comment on src/ast/loader.rs since the deleted test file is not present in the final diff to comment on directly; the LoaderOptional entry in that test was the last thing added to it before deletion.


use enum_map::Enum;

Expand Down
17 changes: 0 additions & 17 deletions src/http_types/Method.rs
Original file line number Diff line number Diff line change
Expand Up @@ -230,23 +230,6 @@ pub enum Optional {
}

impl Optional {
pub fn contains(&self, other: &Optional) -> bool {
if matches!(self, Optional::Any) {
return true;
}
if matches!(other, Optional::Any) {
return true;
}

let Optional::Method(this_set) = self else {
unreachable!()
};
let Optional::Method(other_set) = other else {
unreachable!()
};
this_set.intersection(*other_set).len() > 0
}

pub fn insert(&mut self, method: Method) {
match self {
Optional::Any => {}
Expand Down
2 changes: 0 additions & 2 deletions src/http_types/mime_type_list_enum.rs
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,6 @@ impl MimeTypeList {
self.0.len()
}

pub const COUNT: usize = 2310;

pub const ALL: &'static [MimeTypeList] = ALL;
}

Expand Down
42 changes: 1 addition & 41 deletions src/options_types/bundle_enums.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
//! `from_api`, `API_NAMES`) remain here as sealed extension traits.

use crate::schema::api;
use bun_ast::{Loader, LoaderOptional, Target};
use bun_ast::{Loader, Target};

#[repr(u8)]
#[derive(Copy, Clone, Eq, PartialEq, Debug)]
Expand Down Expand Up @@ -128,8 +128,6 @@ mod sealed {
pub trait Sealed {}
impl Sealed for bun_ast::Target {}
impl Sealed for bun_ast::Loader {}
impl Sealed for bun_ast::LoaderOptional {}
impl Sealed for bun_ast::ImportKind {}
}

/// `schema::api`-coupled methods on [`bun_ast::Target`]. Import alongside
Expand Down Expand Up @@ -252,44 +250,6 @@ impl LoaderExt for Loader {
}
}

/// `schema::api`-coupled methods on [`bun_ast::LoaderOptional`].
pub trait LoaderOptionalExt: sealed::Sealed {
fn from_api(loader: api::Loader) -> LoaderOptional;
}

impl LoaderOptionalExt for LoaderOptional {
fn from_api(loader: api::Loader) -> LoaderOptional {
if loader == api::Loader::_none {
LoaderOptional::NONE
} else {
LoaderOptional::from_loader(Loader::from_api(loader))
}
}
}

// ─── ImportKind: schema-coupled extension methods ─────────────────────────

/// `schema::api`-coupled methods on [`bun_ast::ImportKind`].
pub trait ImportKindExt: sealed::Sealed {
fn to_api(self) -> api::ImportKind;
}

impl ImportKindExt for bun_ast::ImportKind {
fn to_api(self) -> api::ImportKind {
use bun_ast::ImportKind;
match self {
ImportKind::EntryPointRun | ImportKind::EntryPointBuild => api::ImportKind::entry_point,
ImportKind::Stmt => api::ImportKind::stmt,
ImportKind::Require => api::ImportKind::require,
ImportKind::Dynamic => api::ImportKind::dynamic,
ImportKind::RequireResolve => api::ImportKind::require_resolve,
ImportKind::At => api::ImportKind::at,
ImportKind::Url => api::ImportKind::url,
_ => api::ImportKind::internal,
}
}
}

// ─── move-in: TYPE_ONLY from bun_runtime::bake::framework ──────────────────────────

/// Virtual module backing for a
Expand Down
23 changes: 0 additions & 23 deletions src/options_types/jsx.rs
Original file line number Diff line number Diff line change
Expand Up @@ -232,29 +232,6 @@ impl Pragma {
}
}

pub fn parse_package_name(str: &[u8]) -> &[u8] {
if str.is_empty() {
return str;
}
if str[0] == b'@' {
if let Some(first_slash) = strings::index_of_char(&str[1..], b'/') {
let first_slash = first_slash as usize;
let remainder = &str[1 + first_slash + 1..];

if let Some(last_slash) = strings::index_of_char(remainder, b'/') {
let last_slash = last_slash as usize;
return &str[0..first_slash + 1 + last_slash + 1];
}
}
}

if let Some(first_slash) = strings::index_of_char(str, b'/') {
return &str[0..first_slash as usize];
}

str
}

/// When `package_name` is the default `"react"`, this borrows the
/// interned `defaults::IMPORT_SOURCE*` with zero allocations.
pub fn set_import_source(&mut self) {
Expand Down
8 changes: 2 additions & 6 deletions src/options_types/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,8 @@ pub use jsx as JSX;
// Only the `schema::api`-coupled extension traits and option-only types
// (`Format`, `ModuleType`, …) are surfaced from this crate.
pub use bundle_enums::{
BuiltInModule, BundlePackage, ForceNodeEnv, Format, ImportKindExt, LOADER_API_NAMES, LoaderExt,
LoaderOptionalExt, ModuleType, TargetExt, WindowsOptions,
BuiltInModule, BundlePackage, ForceNodeEnv, Format, LOADER_API_NAMES, LoaderExt, ModuleType,
TargetExt, WindowsOptions,
};

/// Compiled-standalone-binary virtual filesystem path prefix + predicate.
Expand All @@ -46,10 +46,6 @@ pub mod standalone_path {
#[cfg(windows)]
pub const BASE_PUBLIC_PATH: &str = "B:/~BUN/";

#[cfg(windows)]
pub const BASE_PUBLIC_PATH_WITH_DEFAULT_SUFFIX: &str =
const_format::concatcp!(BASE_PUBLIC_PATH, "root/");

#[inline]
pub(crate) fn is_bun_standalone_file_path_canonicalized(str_: &[u8]) -> bool {
str_.starts_with(BASE_PATH.as_bytes())
Expand Down
16 changes: 0 additions & 16 deletions src/options_types/schema.rs
Original file line number Diff line number Diff line change
Expand Up @@ -430,22 +430,6 @@ pub mod api {
}
}

/// Open `enum(u8)` in the wire schema. Kept closed.
#[repr(u8)]
#[derive(Copy, Clone, Eq, PartialEq, Debug, Default)]
pub enum ImportKind {
#[default]
_none = 0,
entry_point = 1,
stmt = 2,
require = 3,
dynamic = 4,
require_resolve = 5,
at = 6,
url = 7,
internal = 8,
}

// ─── peechy batch 2: hand-expanded for downstream wfs ────────────────
// Jsx / JsxRuntime / StringMap / EnvConfig / LoadedEnvConfig /
// LoadedRouteConfig / RouteConfig / FrameworkEntryPoint{,Type,Map,Message} /
Expand Down
80 changes: 80 additions & 0 deletions test/internal/source-lints/removed-dead-symbols.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,80 @@
// Guards against reintroduction of previously-removed dead `pub` items.
//
// The workspace compiles with `dead_code = "deny"` and `unreachable_pub =
// "deny"`, so rustc already catches unreachable private items. What slips
// through are `pub` items re-exported at a crate root: they're reachable, so
// the lint is satisfied, but nothing in any other crate ever calls them. Each
// entry below was verified to have zero references anywhere in `src/`,
// `build/debug/codegen/`, or `src/codegen/` (beyond its own definition and
// re-export) before being deleted. Pinning the absence here stops a merge-era
// revert or copy-paste from quietly bringing one back.
//
// To add an entry after removing a dead symbol: append `{ file, pattern }`
// where `pattern` matches the definition line and nothing else in that file.

import { file } from "bun";
import { expect, test } from "bun:test";
import path from "node:path";

const root = path.resolve(import.meta.dir, "..", "..", "..");

const removed: { file: string; pattern: RegExp; name: string }[] = [
{
file: "src/options_types/jsx.rs",
pattern: /\bpub fn parse_package_name\b/,
name: "jsx::Pragma::parse_package_name",
},
{
file: "src/options_types/bundle_enums.rs",
pattern: /\bpub trait LoaderOptionalExt\b/,
name: "bundle_enums::LoaderOptionalExt",
},
{
file: "src/options_types/bundle_enums.rs",
pattern: /\bpub trait ImportKindExt\b/,
name: "bundle_enums::ImportKindExt",
},
{
file: "src/options_types/schema.rs",
pattern: /\bpub enum ImportKind\b/,
name: "schema::api::ImportKind",
},
{
file: "src/options_types/lib.rs",
pattern: /\bBASE_PUBLIC_PATH_WITH_DEFAULT_SUFFIX\b/,
name: "standalone_path::BASE_PUBLIC_PATH_WITH_DEFAULT_SUFFIX (duplicate of bun_standalone_graph's)",
},
{
file: "src/http_types/mime_type_list_enum.rs",
pattern: /\bpub const COUNT\b/,
name: "MimeTypeList::COUNT",
},
{
file: "src/http_types/Method.rs",
pattern: /\bpub fn contains\(&self, other: &Optional\)/,
name: "Method::Optional::contains",
},
];

for (const { file: rel, pattern, name } of removed) {
test(`${name} stays removed from ${rel}`, async () => {
const abs = path.join(root, rel);
const content = await file(abs).text();
const lines = content.split("\n");
const hits: string[] = [];
for (let i = 0; i < lines.length; i++) {
if (pattern.test(lines[i])) hits.push(`${rel}:${i + 1}: ${lines[i].trim()}`);
}
if (hits.length > 0) {
throw new Error(
`${name} was previously removed as dead code (zero workspace references) ` +
`but is present again in ${rel}:\n` +
hits.map(h => ` ${h}`).join("\n") +
`\nIf this symbol now has a real caller, delete its entry from ` +
`test/internal/source-lints/removed-dead-symbols.test.ts. Otherwise, ` +
`remove the dead definition.`,
);
}
expect(hits).toEqual([]);
});
}
Loading