Skip to content
Merged
Show file tree
Hide file tree
Changes from all 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
5 changes: 1 addition & 4 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 Expand Up @@ -2987,7 +2984,7 @@ pub use ast_result::{
TopLevelSymbolToParts, TsEnumsMap,
};
pub use import_record::{Flags as ImportRecordFlags, ImportRecord, Tag as ImportRecordTag};
pub use loader::{Loader, LoaderHashTable, LoaderOptional, SideEffects};
pub use loader::{Loader, LoaderHashTable, SideEffects};
pub use target::Target;
pub mod transpiler_cache;
// Glob re-export: `link_interface!` emits `#[doc(hidden)]` type aliases that
Expand Down
22 changes: 4 additions & 18 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 description references deleted test file; evidence block is stale

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 f79e1018 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
Comment thread
robobun marked this conversation as 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 Expand Up @@ -64,20 +64,6 @@
Text = 13, Bunsh = 14, Sqlite = 15, SqliteEmbedded = 16, Html = 17,
);

/// `Loader.Optional` — `enum(u8) { none = 254, _ }` niche-packed optional.
#[repr(transparent)]
#[derive(Copy, Clone, Eq, PartialEq, Debug)]
pub struct LoaderOptional(pub(crate) u8);

impl LoaderOptional {
pub const NONE: LoaderOptional = LoaderOptional(254);

#[inline]
pub const fn from_loader(l: Loader) -> LoaderOptional {
LoaderOptional(l as u8)
}
}

// E0658: inherent assoc types are nightly-only; lifted to module scope.
pub type LoaderHashTable = bun_collections::StringArrayHashMap<Loader>;

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
Loading