Skip to content

ast: shrink Expr/Stmt/Binding 24→16 bytes#32507

Merged
Jarred-Sumner merged 8 commits into
mainfrom
claude/ast-shrink-expr-16
Jun 19, 2026
Merged

ast: shrink Expr/Stmt/Binding 24→16 bytes#32507
Jarred-Sumner merged 8 commits into
mainfrom
claude/ast-shrink-expr-16

Conversation

@Jarred-Sumner

@Jarred-Sumner Jarred-Sumner commented Jun 19, 2026

Copy link
Copy Markdown
Collaborator

Packs the align-8 AST primitives down to align 4 via #[repr(C, packed(4))] so Expr/Stmt/Binding drop to 16 bytes (−33%), and boxes the rarely-populated Symbol.namespace_alias / Part.import_symbol_property_uses so the linker hot data shrinks too.

What changed

primitive before after how
Ref / StoreRef<T> 8 B align 8 8 B align 4 #[repr(C, packed(4))] — same single-scalar codegen, niche, const fn, provenance
StoreSlice<T> / StoreStr 16 B align 8 12 B align 4 #[repr(C, packed(4))] + len: usizeu32
E::Number 8 B align 8 8 B align 4 #[repr(C, packed(4))]; .value.value()/::new()
Option<Loc> / Option<Ref> storage 8/16 B 4/8 B use existing Loc::EMPTY / Ref::NONE sentinels
Symbol.namespace_alias ~32 B inline 8 B Option<Box<NamespaceAlias>> (None for >95% of symbols)
Part.import_symbol_property_uses ~80 B inline 8 B Option<AstBox<…>> (only set for import * as ns; ns.foo)
Symbol 6× bool 6 B 1 B SymbolFlags: u8 bitflags

Cascade

before after
Expr / Stmt / Binding 24 16
Symbol 80 48
Part ~272 200
LocRef 24 12
E::Binary 56 36
E::Arrow 48 32
G::Fn / G::Class 112 88

Bench (vs main 3b24ca9, same LTO, GOMAXPROCS=2)

main this branch Δ
transformSync 11.2 MB 136 ms 120 ms −12%
RSS per parse 174 MB 152 MB −13%
bundler three10x 327 ms 309 ms −6%
bundler 10k 528 ms 484 ms −8%
bundler 100k 3.87 s 3.79 s −2%

Pack the five primitives that force align-8 into the AST down to align 4:

- StoreRef<T>: NonNull<T> → {NonZeroU32 hi, u16 lo} (48-bit ptr split at
  bit 16; hi is non-zero for any address ≥ 64 KiB so Option<StoreRef<T>>
  keeps its niche). 8 B align 8 → 8 B align 4.
- StoreSlice<T>, StoreStr: NonNull + len → u32 + u16 + u32 (48-bit ptr +
  u32 len). 16 B align 8 → 12 B align 4.
- Ref: u64 → {u32 lo, u32 hi}. 8 B align 8 → 8 B align 4.
- E::Number: f64 → [u32; 2] via to_bits/from_bits (.value field becomes
  .value()/::new()). 8 B align 8 → 8 B align 4.

With every inline Data payload now align-4, expr::Data / stmt::Data / B
drop 16→12 and Expr/Stmt/Binding drop 24→16. The cascade through every
heap node that embeds an Expr/Stmt/StoreStr is large: E::Binary 56→36,
E::Dot 48→36, E::If 72→48, S::SExpr 32→20, G::Property 128→88, G::Fn
112→88, G::Class 112→88.

Also: Option<Loc> fields → Loc with the existing Loc::EMPTY sentinel
(comma_after_spread, star_name_loc, NamedImport.alias_loc); Option<Ref>
fields → Ref with the existing Ref::NONE sentinel (LocRef.ref_ 24→12,
Fn.arguments_ref, Scope.label_ref, NamedImport.namespace_ref, all 25
runtime Imports.__* slots).

User-space addresses are ≤48 bits on every supported target — same
invariant TaggedPtr already relies on. ptr→int splitting forbids
const-eval, so EString::from_static is no longer const; the 5 static
EString singletons move to LazyLock and the dead EString::CLASS/TRUE/
FALSE/NULL/UNDEFINED consts are removed.
@robobun

robobun commented Jun 19, 2026

Copy link
Copy Markdown
Collaborator
Updated 2:20 AM PT - Jun 19th, 2026

@Jarred-Sumner, your commit c8753a6 has 1 failures in Build #63484 (All Failures):


🧪   To try this PR locally:

bunx bun-pr 32507

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

bun-32507 --bun

Comment thread src/ast/b.rs
@coderabbitai

coderabbitai Bot commented Jun 19, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

Walkthrough

The PR restructures three core AST data types to reduce in-memory size and eliminate defensive Option wrappers. E::Number adopts repr(C, packed(4)) with a private value field and public new/value accessors; StoreRef, StoreStr, and StoreSlice move to packed 4-byte-aligned layouts with u32 lengths; Ref gains alignment assertions and a to_nullable() bridge method. Downstream, a dozen AST fields (LocRef.ref_, NamedImport.alias_loc/namespace_ref, Scope.label_ref, Fn.arguments_ref, Import.star_name_loc, Array/Object.comma_after_spread, Runtime.Imports slots, Symbol boolean flags) drop their Option wrappers in favor of Ref::NONE/Loc::EMPTY sentinels or bitflags. All call sites across the parser, visitor, bundler, linker, printer, and consumer APIs are updated to work with the new sentinel-based representations, removing hundreds of infallible expect() unwraps and is_some()/Some()/None pattern-matching sequences.

Changes

AST Layout Compaction and Sentinel Propagation

Layer / File(s) Summary
E::Number, Ref repr, and EString constants: core data type layout changes
src/ast/e.rs, src/ast/lib.rs
E::Number adopts repr(C, packed(4)) with private value field and public new()/value() accessors; From<f64>, to_string(), and to<T>() route through the accessor. Ref gains repr(C, packed(4)) and size/alignment assertions; new to_nullable(self) -> Option<Ref> method converts sentinel to optional form. EString::EMPTY redefined as struct literal; CLASS, TRUE, FALSE, NULL, UNDEFINED pub-const exports removed. Array and Object comma_after_spread change from Option<Loc> to direct Loc with Loc::EMPTY defaults.
StoreRef / StoreStr / StoreSlice: packed arena pointer layouts
src/ast/nodes.rs
StoreRef moves to repr(C, packed(4)) with Deref/DerefMut using as_ptr() and PartialEq comparing copied NonNull fields to avoid unaligned access. StoreStr adopts repr(C, packed(4)) with u32 length (debug-assert fit), NonNull::new_unchecked, and usize casts in slice reconstruction. StoreSlice similarly packed with u32 length in both new() and new_mut().
AST node field de-optionalization: LocRef, NamedImport, Scope, Fn, Import, Runtime.Imports
src/ast/nodes.rs, src/ast/scope.rs, src/ast/g.rs, src/ast/s.rs, src/ast/runtime.rs
LocRef.ref_, NamedImport.alias_loc/namespace_ref, Scope.label_ref, Fn.arguments_ref, and Import.star_name_loc all drop Option wrappers and use Ref::NONE/Loc::EMPTY sentinels. Runtime.Imports slots change from Option<Ref> to Ref; field() uses to_nullable() to materialize optional form, put() assigns directly. Part.import_symbol_property_uses becomes Option<AstBox<...>>.
Compile-time layout size/alignment assertions
src/ast/b.rs, src/ast/expr.rs, src/ast/stmt.rs
const assert blocks updated: B size 16→12 bytes, align 4; Data size 16→12 bytes, align 4; Stmt size ≤24→=16 bytes. Additional inline-payload ceiling assertions added in expr.rs.
E::Number accessor propagation in Expr methods and known-global optimizer
src/ast/expr.rs, src/ast/known_global.rs
Expr::as_number, maybe_simplify_not, Data::write_to_hasher, to_number, to_finite_number, extract_numeric_value, and Data::eql all switch from e.value field to e.value() method call. KnownGlobal::Array optimizer updated identically.
JS parser: parse entry, fn, import/export, prefix, stmt, TypeScript, and property parsing
src/js_parser/parse/mod.rs, src/js_parser/parse/parse_entry.rs, src/js_parser/parse/parse_fn.rs, src/js_parser/parse/parse_import_export.rs, src/js_parser/parse/parse_prefix.rs, src/js_parser/parse/parse_property.rs, src/js_parser/parse/parse_stmt.rs, src/js_parser/parse/parse_typescript.rs
All parse_* functions remove Some() wrapping for LocRef.ref_ construction and initialize to Ref::NONE. star_name_loc assignments drop Some() and use Loc::EMPTY. parse_fn initializes arguments_ref to Ref::NONE and stores reserved "arguments" symbol directly. Numeric property keys use E::Number::new(). parse_prefix assigns comma_after_spread directly. TypeScript namespace/enum LocRef init uses Ref::NONE directly.
JS parser: visitor pass, scan_imports, fold, lower passes, repl_transforms, lib scope slot
src/js_parser/visit/mod.rs, src/js_parser/scan/scan_imports.rs, src/js_parser/fold.rs, src/js_parser/lower/lower_esm_exports_hmr.rs, src/js_parser/repl_transforms.rs, src/js_parser/lib.rs
visit_func, visit_class, visit_stmts, visit_decls remove expect() and use ref_ directly or via to_nullable(). scan_imports replaces is_some/is_none with is_empty/to_nullable() for star_name_loc and ref fields, updating NamedImport construction to assign alias_loc/namespace_ref directly. fold.rs removes Option wrapping from LocRef and changes maybe_comma_spread_error parameter to plain Loc. lower_esm_exports_hmr removes expect() patterns and changes deduplicated_import parameter from Option<Loc> to Loc. lib.rs label-slot assignment uses to_nullable().
JS parser p.rs: import generation, require wiring, label/class/enum, numeric construction, decorator lowering
src/js_parser/p.rs, src/js_parser/parser.rs
Removes Option wrapping for namespace_ref, star_name_loc, default_name, label_ref throughout generate_import_stmt*, process_import_statement, ensure_require_symbol, to_ast, find_label_symbol, jsx_import, lower_class, and LowerUsingDeclarationsContext. E::Number::new() replaces struct literals in all numeric construction sites including decorator params and using-declaration helpers. JSXImportSymbols helpers remove infallible expect(). prefill::string::children() replaces the CHILDREN constant.
JS parser visit: binary constant folding, expression visitor, and side-effect simplification
src/js_parser/visit/visit_binary.rs, src/js_parser/visit/visit_expr.rs, src/js_parser/scan/scan_side_effects.rs
visit_binary.rs rewrites all arithmetic/bitwise constant-fold branches to E::Number::new(). visit_expr.rs updates e_index value() accessor usage, unary/ternary fold sites, charCodeAt fold, and function/class name eligibility checks. scan_side_effects.rs to_boolean and zero-constant construction updated.
Bundler: AstBuilder scope, import, export, and code generation
src/bundler/AstBuilder.rs, src/bundler/transpiler.rs, src/bundler/defines.rs, src/bundler/ServerComponentParseTask.rs, src/bundler/linker_context/*
AstBuilder::init/push_scope initialize label_ref to Ref::NONE; add_import_stmt boxes NamespaceAlias and sets ClauseItem.ref_ directly; to_bundled_ast removes expect() on default_name.ref_. Transpiler export-default/clause constructs remove Some() from ref_ assignments. convertStmts variants update star_name_loc/default_name.ref_. generateCode/postProcessJSChunk/renameSymbols remove expect() and Some() wrapping. defines.rs uses E::Number::new(). Cross-chunk deps and CSS pre-processing drop module_id field. ParseTask::css_symbols_to_parser_symbols assigns flags directly. doStep5 handles optional import_symbol_property_uses.
Bundler linker: LinkerContext import matching and related sub-tasks
src/bundler/LinkerContext.rs, src/bundler/HTMLScanner.rs, src/bundler/ParseTask.rs
LinkerContext::match_import_with_export uses is_valid() instead of is_some/expect for namespace_ref and direct alias_loc. match_imports_with_exports_for_file boxes NamespaceAlias. HTMLScanner import creation and CSS prepare drop module_id field.
JS printer: ref unpacking, star_name_loc emptiness, number printing, symbol marking
src/js_printer/lib.rs, src/js_printer/renamer.rs
Removes all infallible expect() unwraps from ref_ fields throughout import/export/label/function/class printing. is_some checks on star_name_loc become is_empty. Number printing calls e.value(). Export-clause and export-from name lookups use item.name.ref_ directly. minify_identifiers uses set_must_not_be_renamed(). renamer.rs uses generated accessor methods for must_not_be_renamed() and must_start_with_capital_letter_for_jsx().
Parsers and runtime APIs: JSON, JSON5, TOML, YAML, install, audit, upgrade, bunfig, ini, sourcemap
src/parsers/json.rs, src/parsers/json5.rs, src/parsers/toml.rs, src/parsers/yaml.rs, src/runtime/api/JSON5Object.rs, src/runtime/api/JSTranspiler.rs, src/runtime/api/YAMLObject.rs, src/runtime/cli/*, src/install/*, src/bunfig/bunfig.rs, src/ini/lib.rs, src/sourcemap/lib.rs, src/js_parser/defines_table.rs, src/js_parser_jsc/*
All parsers switch E::Number construction to E::Number::new(). All consumer value reads switch from n.value field to n.value() method across ConfigVersion, lockfile parsers (bun.lock, npm, pnpm), audit, pm_pkg, upgrade. Runtime APIs and JSC similarly updated. Bunfig, ini, sourcemap, and defines_table use new forms.
AST symbol flags refactor and AstBox allocation helpers
src/ast/symbol.rs, src/bun_alloc/ast_alloc.rs, src/bun_alloc/lib.rs
Symbol refactored to use packed SymbolFlags bitfield instead of individual boolean fields; generated accessor methods replace direct field access. namespace_alias now Option<Box<NamespaceAlias>>. src/bun_alloc adds AstBox type alias and ast_box() helper; re-exported in lib.rs.

Possibly related PRs

  • oven-sh/bun#30901: Both PRs touch the TypeScript namespace-resolution logic in src/js_parser/scan/scan_symbols.rs's find_symbol_with_record_usage (main PR changes the NamespaceAlias boxing / must-not-be-renamed update, while the retrieved PR gates that probe behind IS_TYPESCRIPT_ENABLED).

Suggested reviewers

  • alii
🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes the main optimization: reducing AST node sizes from 24 to 16 bytes, which is the central focus of this PR.
Description check ✅ Passed The description is thorough and well-structured, covering what changed, the cascade effects, and performance benchmarks. However, it deviates from the template structure by using a detailed technical format instead of following the required 'What does this PR do?' and 'How did you verify your code works?' sections.
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.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


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

@coderabbitai coderabbitai 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.

Actionable comments posted: 7

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/ast/nodes.rs`:
- Around line 58-80: Replace the debug_assert! calls with assert! calls in
StoreRef::pack, StoreStr::new, and StoreSlice constructors (from_raw, new,
new_mut) to ensure pointer and length bounds validation occurs in release
builds, not just debug builds. Additionally, replace the unsafe
NonZeroU32::new_unchecked call in StoreRef::pack with
NonZeroU32::new().expect(...) to safely handle the case where the pointer falls
in the low 64 KiB range, eliminating the possibility of undefined behavior from
truncation or invalid pointer packing.

In `@src/bundler/AstBuilder.rs`:
- Around line 524-526: In the non-HMR import scan path around the alias_loc
assignment, replace the value `item.name.loc` with `item.alias_loc` to make the
error location handling consistent with the HMR branch. This ensures that the
alias_loc field correctly points to the alias token regardless of the execution
path, preventing mode-dependent error locations.

In `@src/js_parser/parse/parse_entry.rs`:
- Around line 1201-1203: The code at the usage sites of
`deferred_import.namespace.ref_` (in the blocks around lines 1201-1213 and
1446-1453) directly consumes this field without validation that it is not
`Ref::NONE`, even though the default `LocRef` initializes `ref_` to `Ref::NONE`.
Add an assertion (such as `assert_ne!(deferred_import.namespace.ref_,
Ref::NONE)`) at both usage sites before consuming `namespace.ref_`, or
alternatively add a clear documentation comment on the `DeferredImportNamespace`
type definition explicitly stating that `namespace.ref_` is guaranteed to be
valid and must never be default-initialized.

In `@src/js_parser/parse/parse_fn.rs`:
- Line 62: The code contains redundant field name assignments that trigger the
clippy lint. In the struct initialization where `ref_: ref_,` appears (at line
62 in parse_fn and also at line 447), remove the redundant `ref_:` prefix and
use the shorthand syntax by changing `ref_: ref_,` to just `ref_,`. Apply this
same fix to both occurrences to resolve the clippy::redundant_field_names lint
failure.

In `@src/js_parser/parser.rs`:
- Around line 671-695: The getter methods in this code that return Option<Ref>
are using map to extract the ref_ field directly, which can return
Some(Ref::NONE) instead of properly returning None. In both the method with the
series of if statements checking byte string names (checking for b"jsx",
b"jsxDEV", b"jsxs", b"Fragment", b"createElement") and the get_with_tag method
with the match expression over JSXImport enum, replace the map patterns that
extract ref_ with a proper nullable conversion (such as calling to_nullable() on
the ref_ value) to ensure that sentinel Ref::NONE values are converted to None
before being returned through the Option<Ref> API. Check the corresponding .zig
implementation files to confirm the correct nullability semantics that should be
preserved.

In `@src/js_parser/scan/scan_imports.rs`:
- Line 400: Replace redundant field initializers with Rust's field init
shorthand syntax to satisfy clippy. In the struct initialization, replace
`namespace_ref: namespace_ref` with just `namespace_ref`. Apply this same
shorthand pattern to all similar redundant field initializers throughout the
file where a field name is being initialized with a variable of the same name.

In `@src/js_parser/visit/visit_stmt.rs`:
- Line 1436: The field initializer at line 1436 uses the redundant form ref_:
ref_, where the field name matches the variable name. Replace this with the Rust
shorthand form ref_, which is more concise and satisfies clippy conventions.
Simply remove the "ref_: " part before the variable name, leaving only ref_, to
match the field-init shorthand pattern.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 06067fdc-ed25-420c-844c-5c47c751e81e

📥 Commits

Reviewing files that changed from the base of the PR and between dae2e87 and 930396f.

📒 Files selected for processing (67)
  • src/ast/b.rs
  • src/ast/e.rs
  • src/ast/expr.rs
  • src/ast/g.rs
  • src/ast/known_global.rs
  • src/ast/lib.rs
  • src/ast/nodes.rs
  • src/ast/runtime.rs
  • src/ast/s.rs
  • src/ast/scope.rs
  • src/ast/stmt.rs
  • src/bundler/AstBuilder.rs
  • src/bundler/LinkerContext.rs
  • src/bundler/ServerComponentParseTask.rs
  • src/bundler/defines.rs
  • src/bundler/linker_context/computeCrossChunkDependencies.rs
  • src/bundler/linker_context/convertStmtsForChunk.rs
  • src/bundler/linker_context/convertStmtsForChunkForDevServer.rs
  • src/bundler/linker_context/generateCodeForFileInChunkJS.rs
  • src/bundler/linker_context/generateCodeForLazyExport.rs
  • src/bundler/linker_context/postProcessJSChunk.rs
  • src/bundler/linker_context/renameSymbolsInChunk.rs
  • src/bundler/transpiler.rs
  • src/bunfig/bunfig.rs
  • src/ini/lib.rs
  • src/install/ConfigVersion.rs
  • src/install/lockfile/bun.lock.rs
  • src/install/migration.rs
  • src/install/npm.rs
  • src/install/pnpm.rs
  • src/js_parser/defines_table.rs
  • src/js_parser/fold.rs
  • src/js_parser/lib.rs
  • src/js_parser/lower/lower_decorators.rs
  • src/js_parser/lower/lower_esm_exports_hmr.rs
  • src/js_parser/p.rs
  • src/js_parser/parse/mod.rs
  • src/js_parser/parse/parse_entry.rs
  • src/js_parser/parse/parse_fn.rs
  • src/js_parser/parse/parse_import_export.rs
  • src/js_parser/parse/parse_prefix.rs
  • src/js_parser/parse/parse_property.rs
  • src/js_parser/parse/parse_stmt.rs
  • src/js_parser/parse/parse_typescript.rs
  • src/js_parser/parser.rs
  • src/js_parser/repl_transforms.rs
  • src/js_parser/scan/scan_imports.rs
  • src/js_parser/scan/scan_side_effects.rs
  • src/js_parser/visit/mod.rs
  • src/js_parser/visit/visit_binary.rs
  • src/js_parser/visit/visit_expr.rs
  • src/js_parser/visit/visit_stmt.rs
  • src/js_parser_jsc/Macro.rs
  • src/js_parser_jsc/expr_jsc.rs
  • src/js_printer/lib.rs
  • src/parsers/json.rs
  • src/parsers/json5.rs
  • src/parsers/toml.rs
  • src/parsers/yaml.rs
  • src/runtime/api/JSON5Object.rs
  • src/runtime/api/JSTranspiler.rs
  • src/runtime/api/YAMLObject.rs
  • src/runtime/bake/bake_body.rs
  • src/runtime/cli/audit_command.rs
  • src/runtime/cli/pm_pkg_command.rs
  • src/runtime/cli/upgrade_command.rs
  • src/sourcemap/lib.rs

Comment thread src/ast/nodes.rs Outdated
Comment thread src/bundler/AstBuilder.rs
Comment on lines 524 to +526
alias: Some(item.alias),
alias_loc: Some(item.name.loc),
namespace_ref: Some(st.namespace_ref),
alias_loc: item.name.loc,
namespace_ref: st.namespace_ref,

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.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Use item.alias_loc in the non-HMR import scan path.

This branch writes alias_loc: item.name.loc while the HMR branch uses item.alias_loc, which makes error locations mode-dependent and can point at the wrong token.

Suggested fix
-                                    alias_loc: item.name.loc,
+                                    alias_loc: item.alias_loc,
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
alias: Some(item.alias),
alias_loc: Some(item.name.loc),
namespace_ref: Some(st.namespace_ref),
alias_loc: item.name.loc,
namespace_ref: st.namespace_ref,
alias: Some(item.alias),
alias_loc: item.alias_loc,
namespace_ref: st.namespace_ref,
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/bundler/AstBuilder.rs` around lines 524 - 526, In the non-HMR import scan
path around the alias_loc assignment, replace the value `item.name.loc` with
`item.alias_loc` to make the error location handling consistent with the HMR
branch. This ensures that the alias_loc field correctly points to the alias
token regardless of the execution path, preventing mode-dependent error
locations.

Comment on lines +1201 to 1203
deferred_import.namespace.ref_,
deferred_import.namespace.loc,
deferred_import.import_record_id,

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.

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Inspect where imports_to_convert_from_require entries are created/updated
rg -n -C3 'imports_to_convert_from_require|namespace\.ref_|namespace_ref' src/js_parser

# Inspect the deferred import/namespace type definitions and defaults
rg -n -C3 'struct .*Deferred|namespace:|LocRef|Ref::NONE' src/js_parser src/ast

Repository: oven-sh/bun

Length of output: 50368


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Get the specific lines mentioned in the review
echo "=== Lines 1201-1203 in parse_entry.rs ==="
sed -n '1195,1210p' src/js_parser/parse/parse_entry.rs

echo ""
echo "=== Lines 1446-1450 in parse_entry.rs ==="
sed -n '1440,1460p' src/js_parser/parse/parse_entry.rs

echo ""
echo "=== Structure definition for deferred imports ==="
rg -n "struct.*Deferred|pub.*deferred" src/js_parser/parse/parse_entry.rs | head -20

echo ""
echo "=== Where deferred_import is pushed/added to imports_to_convert_from_require ==="
rg -n "imports_to_convert_from_require.*push\|imports_to_convert_from_require.*insert\|Deferred.*{" src/js_parser/parse/parse_entry.rs | head -30

Repository: oven-sh/bun

Length of output: 2335


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Try to find struct definitions for deferred imports
echo "=== Searching for Deferred struct/type definition ==="
rg -n "struct Deferred|Deferred {" src/js_parser/parse/ --type rust

echo ""
echo "=== Type of namespace field ==="
rg -n "namespace\s*:" src/js_parser/parse/parse_entry.rs | head -5

echo ""
echo "=== Check if ns_ref is used as VecExt::append arg ==="
sed -n '1213,1220p' src/js_parser/parse/parse_entry.rs

echo ""
echo "=== Check namespace initialization in code ==="
rg -n "namespace\s*=" src/js_parser/parse/parse_entry.rs | head -20

Repository: oven-sh/bun

Length of output: 243


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Try to find struct definitions for deferred imports
echo "=== Searching for Deferred struct/type definition ==="
rg -n "struct Deferred" src/js_parser/parse/ --type rust

echo ""
echo "=== Check imports_to_convert_from_require type ==="
rg -n "imports_to_convert_from_require\s*:" src/js_parser/parse/parse_entry.rs

echo ""
echo "=== Check where imports_to_convert_from_require entries are added ==="
rg -n "imports_to_convert_from_require.push\|imports_to_convert_from_require.append" src/js_parser/parse/parse_entry.rs | head -20

echo ""
echo "=== Check the context around line 1213 where ns_ref is used ==="
sed -n '1210,1230p' src/js_parser/parse/parse_entry.rs

echo ""
echo "=== Look for Namespace struct definition ==="
rg -n "pub struct Namespace" src/js_parser/ -A 5

Repository: oven-sh/bun

Length of output: 109


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Get more context around the actual usage lines
echo "=== Full context around 1201-1203 ==="
sed -n '1180,1230p' src/js_parser/parse/parse_entry.rs

echo ""
echo "=== Full context around 1446-1450 ==="
sed -n '1430,1480p' src/js_parser/parse/parse_entry.rs

Repository: oven-sh/bun

Length of output: 5985


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Find where imports_to_convert_from_require is populated
echo "=== Where imports_to_convert_from_require is modified ==="
rg -n "imports_to_convert_from_require\[|imports_to_convert_from_require.push" src/js_parser/parse/parse_entry.rs | head -20

echo ""
echo "=== Look at visit/mod.rs lines around 344-347 where namespace.ref_ is assigned ==="
sed -n '340,355p' src/js_parser/visit/mod.rs

echo ""
echo "=== Check what type namespace is and what ref_ is ==="
rg -n "namespace.*Ref|struct.*Namespace" src/js_parser/ -A 3 | head -40

Repository: oven-sh/bun

Length of output: 3431


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Find LocRef definition
echo "=== LocRef struct definition ==="
rg -n "pub struct LocRef|struct LocRef" src/js_parser/ -A 3

echo ""
echo "=== How are imports_to_convert_from_require entries initially created? ==="
rg -n "DeferredImportNamespace\s*{" src/js_parser/parse/parse_entry.rs -B 5 -A 5

echo ""
echo "=== Check where imports_to_convert_from_require is first populated ==="
rg -n "imports_to_convert_from_require.push\|\.push.*DeferredImportNamespace" src/js_parser/ | head -20

echo ""
echo "=== Look for initialization of imports_to_convert_from_require ==="
rg -n "imports_to_convert_from_require\s*=" src/js_parser/parser.rs | head -10

Repository: oven-sh/bun

Length of output: 88


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Try different pattern for LocRef
echo "=== Search for LocRef definition ==="
rg -n "LocRef" src/js_parser/ -A 2 | head -30

echo ""
echo "=== Where are DeferredImportNamespace entries added ==="
rg -n "DeferredImportNamespace" src/js_parser/ --type rust | head -30

echo ""
echo "=== Check parse_entry.rs to find where imports_to_convert_from_require is populated ==="
rg -n "push\|\.append" src/js_parser/parse/parse_entry.rs | grep "import\|deferred" | head -20

Repository: oven-sh/bun

Length of output: 2393


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Look at where DeferredImportNamespace is pushed
echo "=== Context around line 1288 in p.rs where DeferredImportNamespace is pushed ==="
sed -n '1280,1310p' src/js_parser/p.rs

echo ""
echo "=== LocRef definition in js_ast ==="
rg -n "pub struct LocRef\|pub.*LocRef.*Ref" src/js_ast/ -A 2

echo ""
echo "=== Ref::NONE definition ==="
rg -n "pub const NONE\|const NONE" src/js_ast/ | head -10

Repository: oven-sh/bun

Length of output: 1576


🏁 Script executed:

#!/bin/bash
set -euoripefail

# Find LocRef in the AST files
echo "=== Finding LocRef definition ==="
fd -e rs -e zig | xargs grep -l "struct LocRef" | head -5

echo ""
echo "=== LocRef in js_ast files ==="
find src -name "*.rs" -o -name "*.zig" | xargs grep -l "LocRef" | head -10

echo ""
echo "=== Looking in ast directory ==="
ls -la src/ast/ 2>/dev/null | head -20 || echo "ast not found"

echo ""
echo "=== Check js_parser/ast if it exists ==="
ls -la src/js_parser/ast/ 2>/dev/null | head -20 || echo "ast not found in js_parser"

echo ""
echo "=== Searching for LocRef definition more broadly ==="
rg "LocRef\s*\{" src/ -A 3 | head -40

Repository: oven-sh/bun

Length of output: 731


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Find LocRef in the AST files
echo "=== Finding LocRef definition ==="
find src -name "*.rs" | xargs grep -l "struct LocRef" | head -5

echo ""
echo "=== Looking for LocRef with ref_ field ==="
rg "LocRef.*ref_|pub struct LocRef" src/ -B 1 -A 3 | head -50

Repository: oven-sh/bun

Length of output: 2016


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Now look at Ref::NONE
echo "=== Ref::NONE definition ==="
rg -n "pub const NONE|const NONE|NONE.*Ref" src/ast/ src/js_ast/ -A 1 | head -20

echo ""
echo "=== Check if ref_ can be Ref::NONE by looking at initialization flow ==="
echo "Initial creation at p.rs line 1288 context:"
sed -n '1275,1305p' src/js_parser/p.rs

echo ""
echo "=== Check the reassignment in visit/mod.rs - is there any guard? ==="
sed -n '335,360p' src/js_parser/visit/mod.rs

echo ""
echo "=== Look at the actual usage at line 1213 in parse_entry.rs ==="
sed -n '1210,1220p' src/js_parser/parse/parse_entry.rs

Repository: oven-sh/bun

Length of output: 558


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Check if LocRef has a Default impl and what it defaults to
echo "=== LocRef Default implementation ==="
rg -n "impl.*Default.*LocRef|LocRef.*Default" src/ast/ -A 5

echo ""
echo "=== Check if Ref::NONE is used in any LocRef initialization ==="
rg -n "LocRef.*Ref::NONE\|Ref::NONE.*LocRef" src/js_parser/ | head -20

echo ""
echo "=== Now check the actual code flow - where is deferred_import first created ==="
echo "Looking at visit/mod.rs context more carefully:"
sed -n '330,360p' src/js_parser/visit/mod.rs

echo ""
echo "=== Check if there's a guard before using namespace.ref_ at line 1213 ==="
sed -n '1195,1225p' src/js_parser/parse/parse_entry.rs

Repository: oven-sh/bun

Length of output: 427


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Key finding: LocRef default is Ref::NONE. Let's verify the initialization flow
echo "=== Full initialization at p.rs 1288 ==="
sed -n '1275,1310p' src/js_parser/p.rs

echo ""
echo "=== Full visit/mod.rs context where ref_ is reassigned ==="
sed -n '320,365p' src/js_parser/visit/mod.rs

echo ""
echo "=== Check: are there any code paths where a DeferredImportNamespace entry is created but not filled? ==="
rg -n "DeferredImportNamespace\s*{" src/js_parser/ -B 5 -A 10

echo ""
echo "=== Verify: at usage site parse_entry.rs, what guards exist? ==="
sed -n '1180,1215p' src/js_parser/parse/parse_entry.rs

Repository: oven-sh/bun

Length of output: 4463


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Fix the regex
echo "=== Where DeferredImportNamespace is created ==="
rg -n "DeferredImportNamespace" src/js_parser/ --type rust -B 3 -A 3 | head -60

echo ""
echo "=== In visit/mod.rs, what is id.r#ref and can it be NONE? ==="
sed -n '338,355p' src/js_parser/visit/mod.rs

echo ""
echo "=== Check BIdentifier ref_ assignment in parser ==="
rg -n "BIdentifier.*ref_|r#ref\s*=" src/js_parser/parse/parse_stmt.rs | head -20

Repository: oven-sh/bun

Length of output: 3263


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Let's check if there was prior validation that was removed
echo "=== Check git history/blame for the problematic lines ==="
git log -p --all -S "deferred_import.namespace.ref_" -- src/js_parser/parse/parse_entry.rs | head -100

echo ""
echo "=== Check if there were guards that checked Ref validity before ==="
rg -n "Ref::NONE\|if.*ref.*==" src/js_parser/parse/parse_entry.rs | head -20

echo ""
echo "=== Look for similar patterns - how are other ref_ fields validated ==="
rg -n "VecExt::append.*ref|assert.*ref|ref_.*NONE" src/js_parser/parse/parse_entry.rs | head -15

Repository: oven-sh/bun

Length of output: 182


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Let's understand the complete flow - when is an entry added to imports_to_convert_from_require?
echo "=== All places where .push is called on imports_to_convert_from_require ==="
rg -n "imports_to_convert_from_require" src/js_parser/ --type rust | grep -i "push\|insert" 

echo ""
echo "=== Check if any entry could have a stale/default LocRef ==="
rg -n "imports_to_convert_from_require\[" src/js_parser/ --type rust -B 2 -A 2 | head -40

echo ""
echo "=== Verify: are ALL entries created with a valid ref at initialization? ==="
echo "Looking at just p.rs line 1288:"
sed -n '1285,1302p' src/js_parser/p.rs

Repository: oven-sh/bun

Length of output: 131


Add explicit assertions that namespace.ref_ is valid before consuming it, or document the invariant guaranteeing it cannot be Ref::NONE.

Based on code inspection, DeferredImportNamespace entries are created with a valid namespace.ref_ from new_symbol() in p.rs:1288. However, LocRef's Default implementation sets ref_ to Ref::NONE (ast/nodes.rs:634–639), and both usage sites at lines 1201–1213 and 1446–1453 consume namespace.ref_ directly without validating it is not Ref::NONE. If any code path instantiates an entry with a default LocRef rather than via the explicit initialization, or if the invariant is subtle, add an assertion at the point of use to prevent silent failures. Alternatively, document clearly in the DeferredImportNamespace type definition that namespace.ref_ is guaranteed to be non-NONE and must not be default-initialized.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/js_parser/parse/parse_entry.rs` around lines 1201 - 1203, The code at the
usage sites of `deferred_import.namespace.ref_` (in the blocks around lines
1201-1213 and 1446-1453) directly consumes this field without validation that it
is not `Ref::NONE`, even though the default `LocRef` initializes `ref_` to
`Ref::NONE`. Add an assertion (such as
`assert_ne!(deferred_import.namespace.ref_, Ref::NONE)`) at both usage sites
before consuming `namespace.ref_`, or alternatively add a clear documentation
comment on the `DeferredImportNamespace` type definition explicitly stating that
`namespace.ref_` is guaranteed to be valid and must never be
default-initialized.

Comment thread src/js_parser/parse/parse_fn.rs Outdated
Comment thread src/js_parser/parser.rs
Comment on lines +671 to 695
return self.jsx.map(|jsx| jsx.ref_);
}
if name == b"jsxDEV" {
return self
.jsx_dev
.map(|jsx| jsx.ref_.expect("infallible: ref bound"));
return self.jsx_dev.map(|jsx| jsx.ref_);
}
if name == b"jsxs" {
return self
.jsxs
.map(|jsxs| jsxs.ref_.expect("infallible: ref bound"));
return self.jsxs.map(|jsxs| jsxs.ref_);
}
if name == b"Fragment" {
return self
.fragment
.map(|f| f.ref_.expect("infallible: ref bound"));
return self.fragment.map(|f| f.ref_);
}
if name == b"createElement" {
return self
.create_element
.map(|c| c.ref_.expect("infallible: ref bound"));
return self.create_element.map(|c| c.ref_);
}
None
}

pub(crate) fn get_with_tag(&self, tag: JSXImport) -> Option<Ref> {
match tag {
JSXImport::Jsx => self.jsx.map(|jsx| jsx.ref_.expect("infallible: ref bound")),
JSXImport::JsxDEV => self
.jsx_dev
.map(|jsx| jsx.ref_.expect("infallible: ref bound")),
JSXImport::Jsxs => self
.jsxs
.map(|jsxs| jsxs.ref_.expect("infallible: ref bound")),
JSXImport::Fragment => self
.fragment
.map(|f| f.ref_.expect("infallible: ref bound")),
JSXImport::CreateElement => self
.create_element
.map(|c| c.ref_.expect("infallible: ref bound")),
JSXImport::Jsx => self.jsx.map(|jsx| jsx.ref_),
JSXImport::JsxDEV => self.jsx_dev.map(|jsx| jsx.ref_),
JSXImport::Jsxs => self.jsxs.map(|jsxs| jsxs.ref_),
JSXImport::Fragment => self.fragment.map(|f| f.ref_),
JSXImport::CreateElement => self.create_element.map(|c| c.ref_),
}

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.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Preserve nullable semantics when projecting sentinel-backed refs

map(|...| ...ref_) can return Some(Ref::NONE), which leaks an invalid ref through an Option<Ref> API. For these getters, convert sentinel refs to None (e.g. via to_nullable()) to preserve the prior nullability contract.

Suggested fix
 pub(crate) fn get(&self, name: &[u8]) -> Option<Ref> {
     if name == b"jsx" {
-        return self.jsx.map(|jsx| jsx.ref_);
+        return self.jsx.and_then(|jsx| jsx.ref_.to_nullable());
     }
     if name == b"jsxDEV" {
-        return self.jsx_dev.map(|jsx| jsx.ref_);
+        return self.jsx_dev.and_then(|jsx| jsx.ref_.to_nullable());
     }
     if name == b"jsxs" {
-        return self.jsxs.map(|jsxs| jsxs.ref_);
+        return self.jsxs.and_then(|jsxs| jsxs.ref_.to_nullable());
     }
     if name == b"Fragment" {
-        return self.fragment.map(|f| f.ref_);
+        return self.fragment.and_then(|f| f.ref_.to_nullable());
     }
     if name == b"createElement" {
-        return self.create_element.map(|c| c.ref_);
+        return self.create_element.and_then(|c| c.ref_.to_nullable());
     }
     None
 }

 pub(crate) fn get_with_tag(&self, tag: JSXImport) -> Option<Ref> {
     match tag {
-        JSXImport::Jsx => self.jsx.map(|jsx| jsx.ref_),
-        JSXImport::JsxDEV => self.jsx_dev.map(|jsx| jsx.ref_),
-        JSXImport::Jsxs => self.jsxs.map(|jsxs| jsxs.ref_),
-        JSXImport::Fragment => self.fragment.map(|f| f.ref_),
-        JSXImport::CreateElement => self.create_element.map(|c| c.ref_),
+        JSXImport::Jsx => self.jsx.and_then(|jsx| jsx.ref_.to_nullable()),
+        JSXImport::JsxDEV => self.jsx_dev.and_then(|jsx| jsx.ref_.to_nullable()),
+        JSXImport::Jsxs => self.jsxs.and_then(|jsxs| jsxs.ref_.to_nullable()),
+        JSXImport::Fragment => self.fragment.and_then(|f| f.ref_.to_nullable()),
+        JSXImport::CreateElement => self.create_element.and_then(|c| c.ref_.to_nullable()),
     }
 }

As per coding guidelines, when porting/fixing behavior, .zig siblings should be treated as semantic source-of-truth for nullability behavior.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/js_parser/parser.rs` around lines 671 - 695, The getter methods in this
code that return Option<Ref> are using map to extract the ref_ field directly,
which can return Some(Ref::NONE) instead of properly returning None. In both the
method with the series of if statements checking byte string names (checking for
b"jsx", b"jsxDEV", b"jsxs", b"Fragment", b"createElement") and the get_with_tag
method with the match expression over JSXImport enum, replace the map patterns
that extract ref_ with a proper nullable conversion (such as calling
to_nullable() on the ref_ value) to ensure that sentinel Ref::NONE values are
converted to None before being returned through the Option<Ref> API. Check the
corresponding .zig implementation files to confirm the correct nullability
semantics that should be preserved.

Source: Coding guidelines

Comment thread src/js_parser/scan/scan_imports.rs Outdated
Comment thread src/js_parser/visit/visit_stmt.rs Outdated
…t fixes

Ref::bits() and StoreRef::as_ptr() were doing two u32 loads + shift|or
to reconstruct the 64-bit value, which showed as a ~5% bundler regression
in the minify-renamer path (millions of HashMap<Ref> eq/hash). Both
structs are repr(C) {u32, u32} with no padding, so transmute<_, u64> is
sound and compiles to one mov — same cost as the old NonNull<T>/u64
representation. StoreRef drops its NonZeroU32 niche (Option<StoreRef<T>>
grows 8→12 at the five storage sites that use it; the Data/Expr niche
comes from spare discriminant values and is unaffected).

Also: clippy redundant-field-names autofix; refresh the stale layout
derivation comments above the b.rs/expr.rs/stmt.rs size asserts.

@coderabbitai coderabbitai 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.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)
src/ast/lib.rs (1)

249-258: ⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

Enforce Ref width limits in release builds.

pack() masks inner after only a debug_assert!, so a release build silently aliases refs once inner_index exceeds 28 bits. This is reachable from source-derived offsets/symbol counts per the comment, so validate before packing or return a recoverable error at the layer that creates the oversized ref. As per coding guidelines, size/index arithmetic on external data must be release-validated because debug assertions compile out.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/ast/lib.rs` around lines 249 - 258, The pack function relies on
debug_assert to validate that inner does not exceed 28 bits, but this assertion
is stripped in release builds leaving the code to silently alias refs via
masking. Replace the debug_assert with a runtime validation that works in both
debug and release builds. Either convert the pack function to return a Result
type and propagate errors to the caller, or use a regular assert to panic on
invalid input. Ensure the validation occurs before the bitwise operations on
inner to prevent silent data corruption when inner_index exceeds the INNER_BITS
limit in production.

Source: Coding guidelines

src/ast/nodes.rs (1)

102-107: ⚠️ Potential issue | 🟠 Major

Make from_static() unsafe to encode the immutable-static mutation contract in the type system.

StoreRef::from_static() is a safe constructor that returns a StoreRef<T> capable of mutable dereference to immutable static storage. This violates memory safety: the comment documents that DerefMut on the result is UB, but the type system does not enforce this. Convert to unsafe fn with a # Safety section stating that the returned StoreRef must never be mutably dereferenced, or return a read-only wrapper type that omits the DerefMut impl.

-    pub fn from_static(r: &'static T) -> Self {
+    /// # Safety
+    ///
+    /// The returned `StoreRef` must never be used for mutation via `DerefMut`.
+    pub unsafe fn from_static(r: &'static T) -> Self {
         // Provenance is shared/read-only: the pointee is *never* written
         // through — `DerefMut` on a `StoreRef` produced here is UB and callers
         // must not do so (audited: only `Deref`/`get()` reads occur).
         Self::pack(core::ptr::from_ref(r).cast_mut())
     }
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/ast/nodes.rs` around lines 102 - 107, The `from_static()` method in the
`StoreRef` implementation currently encodes an unsafe contract (that the
returned value must never be mutably dereferenced) only in comments, but does
not enforce this through the type system. Convert the `from_static` function
from a safe function to an `unsafe fn`, and add a `# Safety` documentation
section that explicitly states the safety requirement: callers must guarantee
that the returned `StoreRef` is never mutably dereferenced, as doing so on
immutable static storage would constitute undefined behavior.

Source: Coding guidelines

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Outside diff comments:
In `@src/ast/lib.rs`:
- Around line 249-258: The pack function relies on debug_assert to validate that
inner does not exceed 28 bits, but this assertion is stripped in release builds
leaving the code to silently alias refs via masking. Replace the debug_assert
with a runtime validation that works in both debug and release builds. Either
convert the pack function to return a Result type and propagate errors to the
caller, or use a regular assert to panic on invalid input. Ensure the validation
occurs before the bitwise operations on inner to prevent silent data corruption
when inner_index exceeds the INNER_BITS limit in production.

In `@src/ast/nodes.rs`:
- Around line 102-107: The `from_static()` method in the `StoreRef`
implementation currently encodes an unsafe contract (that the returned value
must never be mutably dereferenced) only in comments, but does not enforce this
through the type system. Convert the `from_static` function from a safe function
to an `unsafe fn`, and add a `# Safety` documentation section that explicitly
states the safety requirement: callers must guarantee that the returned
`StoreRef` is never mutably dereferenced, as doing so on immutable static
storage would constitute undefined behavior.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: b120d960-9a95-4ab3-adb7-8e7a4280ac86

📥 Commits

Reviewing files that changed from the base of the PR and between 930396f and 208ffe3.

📒 Files selected for processing (13)
  • src/ast/b.rs
  • src/ast/expr.rs
  • src/ast/lib.rs
  • src/ast/nodes.rs
  • src/ast/stmt.rs
  • src/bundler/AstBuilder.rs
  • src/bundler/ServerComponentParseTask.rs
  • src/bundler/linker_context/computeCrossChunkDependencies.rs
  • src/bundler/transpiler.rs
  • src/js_parser/p.rs
  • src/js_parser/parse/parse_fn.rs
  • src/js_parser/scan/scan_imports.rs
  • src/js_parser/visit/visit_stmt.rs

@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.

Thanks for the comment + clippy fixes in 208ffe3 — the bug-hunter pass on the new head came back clean. Deferring to human review given the scope: unsafe pointer-packing in StoreRef/StoreSlice/Ref plus the Option→sentinel migration across ~67 files is the kind of layout/invariant change that warrants a maintainer's eyes.

Extended reasoning...

Overview

This PR repacks the core AST primitives to align-4 so Expr/Stmt/Binding shrink 24→16 bytes: Ref becomes {u32,u32} with transmute-based bits(), StoreRef<T> stores the address as [u32;2], StoreStr/StoreSlice split a 48-bit pointer + u32 len into 12 bytes, and E::Number stores f64 as [u32;2]. It then propagates two consequences across 67 files: (a) E::Number { value }E::Number::new()/.value() everywhere, and (b) Option<Ref>/Option<Loc> storage on LocRef, NamedImport, Scope.label_ref, Fn.arguments_ref, Import.star_name_loc, comma_after_spread, and runtime::Imports is replaced with Ref::NONE/Loc::EMPTY sentinels, with .to_nullable() bridging back to Option at read sites.

A second commit (208ffe3) addressed my earlier nit about stale layout-derivation comments in b.rs/expr.rs/stmt.rs and fixed the redundant_field_names clippy failures CodeRabbit flagged — the diff now shows updated derivations and field-init shorthand throughout.

Security risks

No direct attack-surface change (no auth/crypto/network). The memory-safety angle is the new unsafe in nodes.rs/lib.rs: transmute<[u32;2], usize> for pointer reconstruction and transmute<Ref, u64> for the hot hash key, both gated by a const cfg!(target_endian = "little") assert and the existing 48-bit-VA invariant TaggedPtr already relies on. The debug_assert!(bits >> 48 == 0) and len <= u32::MAX guards in StoreStr/StoreSlice are debug-only, which CodeRabbit called out; whether those should be release assert!s on a hot constructor is a perf/safety trade-off the maintainers should weigh.

Level of scrutiny

High. This is hot-path transpiler/bundler infrastructure — the parser, visitor, printer, linker, and every config/lockfile parser flow through these types. The Option→sentinel migration is mechanically simple per-site but spans ~100+ call sites where a missed .to_nullable() or an is_some()!is_empty() inversion would silently change behavior (e.g. star_name_loc, comma_after_spread). The const size/align asserts catch layout regressions but not semantic ones.

Other factors

The bug-hunting system found nothing on this revision. The remaining CodeRabbit inline comments are either already fixed (clippy shorthand), pre-existing behavior unrelated to this PR (alias_loc: item.name.loc in AstBuilder), or design suggestions (release-mode asserts, to_nullable() in JSX getters where the prior code also expect-unwrapped). None block, but the overall scale and the unsafe layout work put this well outside auto-approval territory.

The {u32, u32} representation for Ref/StoreRef/StoreSlice/StoreStr was
classified as ScalarPair by rustc and passed in two registers, forcing
shl|or recombination at every call site (visible as +20% in
symbol::Map::follow and +30-60% in the lexer string-literal path).

#[repr(C, packed(4))] on the original single-scalar layouts (Ref(u64),
StoreRef(NonNull<T>), StoreSlice/Str{NonNull, u32}, Number{f64}) lowers
alignment to 4 with the same struct sizes but keeps single-register
passing, the NonNull niche, const-fn constructors, and pointer
provenance. Codegen is identical to the pre-shrink layout except for
4-aligned 8-byte loads (free on x64/arm64).

Bench vs main-base (same LTO, GOMAXPROCS=2):
  transformSync 11.2 MB    136 -> 125 ms  (-8%)
  RSS per parse            174 -> 148 MB  (-15%)
  bundler three10x         328 -> 316 ms  (-4%)
  bundler 10k              523 -> 509 ms  (-3%)
  bundler 100k            3.87 -> 3.79 s  (-2%)

Also reverts the LazyLock workaround for static EString (StoreStr::new
is const again).
Both fields are inline on every instance but populated rarely:
- Symbol.namespace_alias (~28 B) is only set for import-clause names;
  >95% of symbols are locals/temps where it stays None.
- Part.import_symbol_property_uses (~80 B ArrayHashMap) is only
  non-empty for `import * as ns; ns.foo` access patterns.

Boxing them takes Symbol 68->48 (-40% vs main 80) and Part 280->200.
Also removes the long-dead ImportRecord.module_id field.

Bench vs main 3b24ca9 (same LTO, GOMAXPROCS=2):
  transformSync 11.2 MB    136 -> 120 ms  (-12%)
  bundler three10x         327 -> 309 ms  (-6%)
  bundler 10k              528 -> 484 ms  (-8%)
  bundler 100k            3.87 -> 3.79 s  (-2%)
Comment thread src/js_parser/parser.rs Outdated
Comment thread src/ast/nodes.rs
The 6 Symbol bools collapse into a u8 SymbolFlags. Symbol stays at 48 B
(the saved bytes were padding under the align-8 Box pointer), but
merge_contents_with becomes one OR instead of six branches.

Part.import_symbol_property_uses moves from Box<_> to AstBox<_>
(= Box<_, AstAlloc>) so its 80 B header lives in the same spill heap as
the rest of Part and is reclaimed on heap reset rather than relying on
Drop. Adds bun_alloc::AstBox/ast_box() alongside AstVec.

@coderabbitai coderabbitai 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.

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
src/bundler/AstBuilder.rs (1)

385-410: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Push the rewritten local statement after clearing is_export.

Line 391 mutates the copied st, but Line 410 pushes *stmt, so the original export remains in the HMR wrapper path.

🐛 Proposed fix
-                        hmr_stmts.push(*stmt);
+                        hmr_stmts.push(Stmt::alloc(st, stmt.loc));
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/bundler/AstBuilder.rs` around lines 385 - 410, The code modifies the
copied `st` struct by setting `st.is_export = false` but then pushes the
original `*stmt` to `hmr_stmts` instead of using the modified version. This
means the HMR wrapper path will still have the export flag set. After mutating
`st` to clear the `is_export` flag, reconstruct the statement with the modified
`st` data and push that reconstructed statement to `hmr_stmts` instead of
pushing `*stmt` directly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/bun_alloc/ast_alloc.rs`:
- Around line 331-339: The public type `AstBox<T>` and function `ast_box<T>`
lack documentation about the drop-safety contract they enforce. Add doc comments
to both `AstBox<T>` and `ast_box<T>` that explicitly document the requirement
that the generic type parameter `T` must be AST-lifetime-safe and must not rely
on `Drop` for correctness, since `AstAlloc::deallocate` is intentionally a
no-op. The documentation should clarify that any required cleanup (dropping heap
allocations, refcounts, or file descriptors) must happen before the arena
resets, or the value must be explicitly dropped before that point. Reference the
coding guideline about arena-backed values in `bun_alloc::MimallocArena` to
reinforce this contract.

---

Outside diff comments:
In `@src/bundler/AstBuilder.rs`:
- Around line 385-410: The code modifies the copied `st` struct by setting
`st.is_export = false` but then pushes the original `*stmt` to `hmr_stmts`
instead of using the modified version. This means the HMR wrapper path will
still have the export flag set. After mutating `st` to clear the `is_export`
flag, reconstruct the statement with the modified `st` data and push that
reconstructed statement to `hmr_stmts` instead of pushing `*stmt` directly.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 6113935d-08f2-4567-a76c-f55d6028f2bf

📥 Commits

Reviewing files that changed from the base of the PR and between eb8abc2 and 69560c0.

📒 Files selected for processing (23)
  • src/ast/import_record.rs
  • src/ast/nodes.rs
  • src/ast/symbol.rs
  • src/bun_alloc/ast_alloc.rs
  • src/bun_alloc/lib.rs
  • src/bundler/AstBuilder.rs
  • src/bundler/HTMLScanner.rs
  • src/bundler/LinkerContext.rs
  • src/bundler/ParseTask.rs
  • src/bundler/linker_context/doStep5.rs
  • src/bundler/linker_context/postProcessJSChunk.rs
  • src/bundler/linker_context/prepareCssAstsForChunk.rs
  • src/css/css_parser.rs
  • src/js_parser/lower/lower_esm_exports_hmr.rs
  • src/js_parser/p.rs
  • src/js_parser/parse/parse_fn.rs
  • src/js_parser/scan/scan_imports.rs
  • src/js_parser/scan/scan_symbols.rs
  • src/js_parser/visit/mod.rs
  • src/js_parser/visit/visit_expr.rs
  • src/js_parser/visit/visit_stmt.rs
  • src/js_printer/lib.rs
  • src/js_printer/renamer.rs
💤 Files with no reviewable changes (5)
  • src/bundler/linker_context/prepareCssAstsForChunk.rs
  • src/ast/import_record.rs
  • src/bundler/HTMLScanner.rs
  • src/css/css_parser.rs
  • src/bundler/linker_context/postProcessJSChunk.rs

Comment thread src/bun_alloc/ast_alloc.rs
Symbol lives in ArenaVec<'a, Symbol> alongside Part, so Drop is not
guaranteed to run when the arena resets. Move the box header into the
AstAlloc spill heap so it's reclaimed on reset like the rest of the
per-file AST data. NamespaceAlias is already arena-lifetime (its
alias: StoreStr points into the parse arena), so AstAlloc is the
correct allocator.
Comment thread src/ast/symbol.rs Outdated
Comment thread src/ast/symbol.rs Outdated
@Jarred-Sumner Jarred-Sumner merged commit 25e32c1 into main Jun 19, 2026
78 of 79 checks passed
@Jarred-Sumner Jarred-Sumner deleted the claude/ast-shrink-expr-16 branch June 19, 2026 22:38
Jarred-Sumner added a commit that referenced this pull request Jun 19, 2026
repr(C, align(8)) on EString and prefer_template moved after rope_len. The
StoreStr packed(4) change in #32507 let repr(Rust) place data.ptr at offset
20, so static EString::from_static initializers (5 sites in
runtime/bake/bake_body.rs and bundler/defines.rs) emit ARM64_RELOC_UNSIGNED
at a non-8-aligned offset, which arm64 ld rejects. repr(C) pins data (the
only field needing a static relocation) at offset 0. EString is
StoreRef-boxed so Expr stays 16; EString stays 40.

Also: Arrow size assert 48→32 (StoreSlice shrink); two
or_insert_with→or_default clippy.

Claude-Session: https://claude.ai/code/session_01FShraojuNHfmfS8q2DLDZb
Jarred-Sumner added a commit that referenced this pull request Jun 19, 2026
Mechanical: Some(x)→x, .and_then(|n| n.ref_)→.map(|n| n.ref_).filter(is_valid),
star_name_loc None→Loc::EMPTY, E::Number{value}→E::Number::new(x), n.value→.value().

Claude-Session: https://claude.ai/code/session_01FShraojuNHfmfS8q2DLDZb
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants