Repository navigation
Conversation
|
Status: diff is green on all lanes that touch the bundler/parser/codegen; the remaining CI reds on build #87081 are unrelated to this change. Ready for review. CI on #87081:
All eight Reproduced with: printf 'class UserService {}\nfunction makeThing(){return 1}\nconst arrow=()=>1;\nconsole.log(UserService.name, makeThing.name, arrow.name, (function named(){}).name);\n' > /tmp/e.js
bun build /tmp/e.js --minify --keep-names | bun -
# before: e o s n
# after: UserService makeThing arrow namedBuiltin codegen: |
|
Found 3 issues this PR may fix:
🤖 Generated with Claude Code |
|
Linked #12304 and #25332 (both are the bundler #4958 is a different path: it's the runtime transpiler ( |
WalkthroughChangesName preservation
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/js_parser/visit/visit_stmt.rs (2)
935-958: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
--keep-namesdoesn't preserve function names for TS namespace-exported functions.The namespace-export branch here is a separate
iffrom theelse if !mark_as_dead { ... }branch that callsp.keep_stmt_symbol_name(...)(see Line 998). Sonamespace Foo { export function bar() {} }never gets its name preserved when minifying with--keep-names, unlike the equivalent class case ins_class(Line 1092-1102), where the namespace-export path is folded into the same block that includes thekeep_stmt_symbol_namecall.🩹 Proposed fix
stmts.push(Stmt::assign( p.new_expr( E::Dot { target: Expr::init_identifier(enclosing_namespace_arg_ref, stmt.loc), name: original_name.into(), name_loc: func_name.loc, ..Default::default() }, stmt.loc, ), Expr::init_identifier(func_name.ref_, func_name.loc), )); + if let Some(keep) = p.keep_stmt_symbol_name(stmt.loc, name_ref, original_name) { + stmts.push(keep); + }🤖 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/visit/visit_stmt.rs` around lines 935 - 958, Update the namespace-export branch in the statement visitor to invoke p.keep_stmt_symbol_name(...) for exported functions when --keep-names is enabled, matching the existing behavior in the subsequent !mark_as_dead branch and the analogous namespace-export handling in s_class. Preserve the current namespace assignment generation while ensuring the function’s original name is retained.
1078-1102: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winHoist the custom static-name check before
lower_class.lower_classmutatesdata.class.propertiesand can move decorated fields out of the class body, so checkingclass_has_custom_static_name(&data.class)afterward can miss a user-definedstatic nameand append akeep_stmt_symbol_name(...)that clobbers it.src/js_parser/visit/visit_stmt.rs:1087-1099🤖 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/visit/visit_stmt.rs` around lines 1078 - 1102, Compute and store the result of class_has_custom_static_name(&data.class) before calling lower_class in the class statement handling flow. Use this stored result after lowering to decide whether keep_stmt_symbol_name should be appended, preserving the existing class-name and export conditions.
🤖 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/js_parser/visit/visit_stmt.rs`:
- Around line 935-958: Update the namespace-export branch in the statement
visitor to invoke p.keep_stmt_symbol_name(...) for exported functions when
--keep-names is enabled, matching the existing behavior in the subsequent
!mark_as_dead branch and the analogous namespace-export handling in s_class.
Preserve the current namespace assignment generation while ensuring the
function’s original name is retained.
- Around line 1078-1102: Compute and store the result of
class_has_custom_static_name(&data.class) before calling lower_class in the
class statement handling flow. Use this stored result after lowering to decide
whether keep_stmt_symbol_name should be appended, preserving the existing
class-name and export conditions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6a017367-2fa3-4470-8cd7-823f51a08a11
📒 Files selected for processing (5)
src/js_parser/p.rssrc/js_parser/visit/visit_expr.rssrc/js_parser/visit/visit_stmt.rstest/bundler/bundler_minify.test.tstest/bundler/esbuild/extra.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🟡
src/js_parser/visit/visit_stmt.rs:874-884— For an anonymousexport default class {}with decorators (or a bareaccessorfield), the decorator-name-injection at line 830 has already setclass.class.class_name = Some(data.default_name)by the timekeep_nameis computed here, soload_name_from_refreturns the bundler-internal{source}_defaultsymbol name (e.g."d_default") instead of hitting theDEFAULT_ALIASfallback. The siblingSFunctionarm at lines 584-589 avoids this by computingnamebefore injectingdata.default_name; capturingclass_namebefore line 824 (or falling through toDEFAULT_ALIASwhenn.ref_ == data.default_name.ref_) would fix it.Extended reasoning...
What the bug is
The
export default classkeep-names block computeskeep_namefromclass.class.class_nameat line 875, but by that point the pre-existing decorator-name-injection at lines 824-832 may have already overwritten it withdata.default_name:// lines 824-832 (pre-existing) if class.class.has_decorators || class.class.should_lower_standard_decorators { if class.class.class_name.is_none() || ...ref_.is_empty() { class.class.class_name = Some(data.default_name); } } // ... // line 835 — follow-up commit 72e69dfc captures has_custom_static_name here, // but class_name has ALREADY been injected above // ... // lines 875-877 (new in this PR) let keep_name: &[u8] = match class.class.class_name { Some(n) if !n.ref_.is_empty() => p.load_name_from_ref(n.ref_), _ => js_ast::ClauseItem::DEFAULT_ALIAS, };
data.default_name.ref_is a real symbol created bycreate_default_name()(p.rs:3995) whoseoriginal_nameis the bundler-internal identifier"{source_path_ident}_default"— not the string"default". So the match takes theSome(n) if !n.ref_.is_empty()arm,load_name_from_ref(p.rs:756-763) returnssymbols[...].original_name.slice(), and theDEFAULT_ALIASfallback is never reached.Code path that triggers it
Any anonymous
export default class {}whosehas_decoratorsorshould_lower_standard_decoratorsis set. Per the comment at lines 820-823, that includes classes with only an auto-accessor field and no decorators at all:// d.ts, --keep-names --minify-identifiers export default class { accessor x = 1 } // or with experimentalDecorators: // @dec export default class {}
Why existing code doesn't prevent it
The follow-up commit 72e69df moved the
has_custom_static_namecapture to line 835 (beforelower_class), but that is still after the injection at line 830 and does not capture the pre-injectionclass_name. The earlier inline comment on this PR aboutlower_class()/mem::take()mutations covers different mutation sites (lines 841 and 868); the injection at 830 is a third, earlier one it does not address.Contrast with the sibling
SFunctionarm at lines 584-589, which computesnamefromfunc.func.namein the same expression that injectsdata.default_name, so an anonymousexport default function() {}correctly getsDEFAULT_ALIAS. TheSClassarm is asymmetric.Step-by-step proof
- Input:
/d.ts=export default class { accessor x = 1 }with--keep-names --minify-identifiers. parse_stmt.rscreatesdata.default_nameviacreate_default_name(), whose symboloriginal_nameis"d_default"(from the source path).visit_stmt.rs:824:should_lower_standard_decoratorsis true (auto-accessor lowering),class_nameisNone.- Line 830:
class.class.class_name = Some(data.default_name)— nowclass_namepoints at thed_defaultsymbol. - Line 875:
match class.class.class_nameseesSome(n);n.ref_is a real symbol so!n.ref_.is_empty()is true. p.load_name_from_ref(n.ref_)returnssymbols[inner_index].original_name.slice()=b"d_default".- Line 880 emits
__name(default_ref, "d_default"). - At runtime, the imported class's
.name === "d_default"— a leaked internal bundler identifier — instead of the spec value"default"(which theKeepNamesExportDefaulttest asserts for the non-decorated/d.jscase).
Impact
Narrow: opt-in flag × anonymous
export default class× decorators/accessor. Not a regression relative to before this PR (where.namewas already wrong under--minify-identifiers), but it's newly-added logic emitting the wrong string for a case whose sibling arm handles it correctly — REVIEW.md's "Fix the whole class — parallel switch arms".Fix
Mirror the
SFunctionarm: capturekeep_namefromclass.class.class_namebefore line 824. Alternatively, in the match at 875, fall through toDEFAULT_ALIASwhenn.ref_ == data.default_name.ref_. Optionally add a decorated/accessor variant toKeepNamesExportDefault. - Input:
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🟡
src/js_parser/visit/visit_stmt.rs:838-841— Thekeep_namecapture runs after the decorator-name-injection block at lines 824-832, so for an anonymousexport default @dec class {}(orexport default class { accessor x }) it reads the synthesizeddata.default_nameref and emits__name(ref, "<src>_default")instead of__name(ref, "default")— leaking an internal identifier as the runtime.namewhere the spec (§16.2.3.7) mandates"default". Move this capture above line 824 (or addif n.ref_ != data.default_name.ref_to theSome(n)guard) so the anonymous case falls through toDEFAULT_ALIAS.Extended reasoning...
What the bug is
Commit 05f92bc fixed the earlier
lower_class/mem::takeordering issue by capturingkeep_nameandhas_custom_static_namebeforelower_class()runs. But it placed that capture at lines 838-841, which is still after the pre-existing decorator-name-injection block at lines 824-832. That block gives an anonymousexport default classa synthetic name whenever decorator lowering needs one:// visit_stmt.rs:824-832 if class.class.has_decorators || class.class.should_lower_standard_decorators { if class.class.class_name.is_none() || class.class.class_name.unwrap().ref_.is_empty() { class.class.class_name = Some(data.default_name); } }
So by the time the
keep_namematch runs,class.class.class_nameisSome(data.default_name)for an anonymous decorated class, and theSome(n) if !n.ref_.is_empty()arm is taken instead of the intended_ => DEFAULT_ALIASfallback.Code path
data.default_namefor an anonymousexport default classis created viacreate_default_name(p.rs:3995-4009), whose symboloriginal_nameisformat!("{}_default", source.path.name().fmt_identifier()).load_name_from_refon aRefTag::Symbolref (p.rs:761-763) returns thatoriginal_nameslice. So for a source filed.js,keep_namebecomesb"d_default", and line 881 emits__name(default_ref, "d_default").Per ES §16.2.3.7 (NamedEvaluation of
ClassDeclaration : class ClassTail), an anonymousexport default class {}gets.name === "default". The_ => js_ast::ClauseItem::DEFAULT_ALIASarm (b"default") is clearly what was intended for this case — it just never fires when decorator lowering has already injected a name.Why existing tests don't catch it
KeepNamesExportDefault's/d.jsisexport default class {}with no decorators or auto-accessors, sohas_decoratorsandshould_lower_standard_decoratorsare both false, line 830 doesn't fire,class_namestaysNone, and theDEFAULT_ALIASarm is correctly taken. Adding a decorator or anaccessorfield to/d.jswould make the test assert"default"and observe"d_default".Step-by-step proof
- Input:
export default @dec class {}(orexport default class { accessor x = 1 }) with--keep-names. - Parse:
class_name = None;has_decorators = true(orshould_lower_standard_decorators = true);data.default_name = create_default_name(...)withoriginal_name = "<src>_default". Line 816-818 ensuresdata.default_name.ref_is a real symbol. - visit_stmt.rs:824-832: condition true,
class_name.is_none()→class.class.class_name = Some(data.default_name). - visit_stmt.rs:838-841:
match class.class.class_name { Some(n) if !n.ref_.is_empty() => p.load_name_from_ref(n.ref_) }→"<src>_default". - visit_stmt.rs:881:
keep_stmt_symbol_name(stmt.loc, data.default_name.ref_, "<src>_default")→ emits__name(ref, "<src>_default"). - Runtime:
.name === "d_default"(or"entry_default", etc.); expected"default".
Not a duplicate
- The resolved
lower_class/mem::takethread is what this capture was added to fix; this bug was introduced by that fix's placement. - The open comment at line 1112 (decorator-suffix ordering) is about when
__nameruns relative to decorator callbacks, not what string it carries. - The open
e_class/should_lower_standard_decoratorscomment is about the class-expression path invisit_expr.rs.
Impact & fix
Narrow — requires anonymous
export default class+ decorators/auto-accessors + opt-in--keep-names— and not a regression (the flag was a no-op before this PR). But it leaks an internal synthesized identifier into user-visible.namefrom newly-added logic whose intent (theDEFAULT_ALIASarm) is defeated. One-line fix: hoist thekeep_name/has_custom_static_namecapture above line 824, or addif n.ref_ != data.default_name.ref_to theSome(n)guard so the injected default-name ref falls through toDEFAULT_ALIAS. - Input:
|
Updated 3:47 AM PT - Aug 1st, 2026
⏳ @robobun, your commit 4a2aa48 is still building in
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/codegen/bundle-functions.ts (1)
329-349: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
__namehelper reinjection is skipped entirely for sloppy-mode captures, even when needed.
usesKeepNamesis computed unconditionally oncaptured, but the tamper-safevar __name=...reinjection only happens insidecaptured.replace(...), which is skipped wheneverfn.directives.sloppyis true (finalReplacement = capturedunchanged). If a sloppy builtin's captured body contains a keep-names-generated__name(...)call (plausible sincekeepNames: trueis passed unconditionally to every builtin compile at Line 317), the emitted code references an undefined__nameand will throw aReferenceErrorat runtime — this would be a boot-time crash for that builtin, not just a lint nit.🐛 Proposed fix: inject the helper regardless of sloppy mode
const finalReplacement = - (fn.directives.sloppy - ? captured + (fn.directives.sloppy && !usesKeepNames + ? captured : captured.replace( /function\s*\(.*?\)\s*{/, - '$&"use strict";' + + "$&" + + (fn.directives.sloppy ? "" : '"use strict";') + (usesKeepNames ? 'var __name=(t,n)=>(__intrinsic__Object.__intrinsic__defineProperty(t,"name",{value:n,configurable:!0}),t);' : "") + (usesDebug ? createLogClientJS("BUILTINS", fn.name) : "") + (usesAssert ? createAssertClientJS(fn.name) : ""), ) )🤖 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/codegen/bundle-functions.ts` around lines 329 - 349, Update the finalReplacement construction so keep-names helper reinjection occurs for sloppy captures as well as strict captures. When usesKeepNames detects __name( in captured, prepend the existing tamper-safe __name helper to the emitted body regardless of fn.directives.sloppy, while preserving the current strict-mode directive and debug/assert injections.src/js_parser/visit/visit_stmt.rs (1)
905-1066: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winServer-components wrapped named function exports don't get
--keep-namesapplied.When
p.options.features.server_components.wraps_exports()is true and the function is exported (lines 980-1006), the export is rewritten intoexport var <name> = registerClientReference(<func>, ...)and pushed directly viap.s(S::Local {...}), bypassing bothkeep_stmt_symbol_name(called in the siblingelsebranch at Line 1009) and the genericvisit_decl/maybe_keep_expr_symbol_namepath. The function's original name is not preserved for this specific combination of--keep-names+ server components client-reference wrapping.🤖 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/visit/visit_stmt.rs` around lines 905 - 1066, Apply keep-names handling in the server-components wrapping branch of s_function before or alongside pushing the generated Local declaration. Ensure the rewritten client-reference export preserves the original function name using the existing keep_stmt_symbol_name path, without changing behavior when keep-names is disabled.src/js_parser/p.rs (1)
5412-5435: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftAnonymous decorated class expressions can clobber a custom
static nameunder--keep-names.
keep_expr_symbol_name()re-checksExprData::EClassafter decorator lowering has already wrapped the class, so theclass_has_custom_static_name()guard no longer applies and__name()can overwrite the user-defined name. Snapshot that flag before lowering and thread it through here instead of re-deriving it from the lowered expression.🤖 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/p.rs` around lines 5412 - 5435, Preserve whether the original class has a custom static name before decorator lowering, then pass that flag into keep_expr_symbol_name instead of re-checking ExprData::EClass on the lowered expression. Use the captured flag to bypass __name() for anonymous decorated classes with custom static name, while preserving existing behavior for other expressions.
🤖 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/codegen/bundle-functions.ts`:
- Around line 329-349: Update the finalReplacement construction so keep-names
helper reinjection occurs for sloppy captures as well as strict captures. When
usesKeepNames detects __name( in captured, prepend the existing tamper-safe
__name helper to the emitted body regardless of fn.directives.sloppy, while
preserving the current strict-mode directive and debug/assert injections.
In `@src/js_parser/p.rs`:
- Around line 5412-5435: Preserve whether the original class has a custom static
name before decorator lowering, then pass that flag into keep_expr_symbol_name
instead of re-checking ExprData::EClass on the lowered expression. Use the
captured flag to bypass __name() for anonymous decorated classes with custom
static name, while preserving existing behavior for other expressions.
In `@src/js_parser/visit/visit_stmt.rs`:
- Around line 905-1066: Apply keep-names handling in the server-components
wrapping branch of s_function before or alongside pushing the generated Local
declaration. Ensure the rewritten client-reference export preserves the original
function name using the existing keep_stmt_symbol_name path, without changing
behavior when keep-names is disabled.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: b4d660f5-a4c8-43ed-bc2f-059e9a74f5f1
📒 Files selected for processing (7)
src/codegen/bundle-functions.tssrc/js_parser/p.rssrc/js_parser/visit/mod.rssrc/js_parser/visit/visit_binary.rssrc/js_parser/visit/visit_expr.rssrc/js_parser/visit/visit_stmt.rstest/bundler/bundler_minify.test.ts
|
On the three outside-diff findings (reviewed against 963df73, before 017c8d2):
|
There was a problem hiding this comment.
No new issues found in this revision — the earlier rounds' feedback (tree-shaking flag, decorator-lowering ordering, simple parameter defaults, bundle-functions keepNames) is all addressed. I'm deferring rather than approving because this is a ~400-line feature implementation across six parser/visitor files with restructured control flow in e_class, and it activates the previously-inert --keep-names in bundle-modules.ts (release-build internal modules now get __name() calls) — worth a human sanity check on the size/perf side.
What was reviewed:
visit_binary.rs:&&=added to the logical-assign arm; verified the{}.field ??= vHMR fold stays gated off for&&=(correct —{}.x &&= visundefined, notv).e_classrestructure: early-return removed so the__name()wrap runs on the lowered-decorator result too;has_custom_static_nameandoriginal_class_nameare captured pre-lowering.s_class/s_function/export-default:__nameinsertion order relative to lowered decorator suffix and namespace assignments.bundle-functions.tsnow dropskeepNamesentirely (no injected helper, no sloppy-branch hole).
Extended reasoning...
Overview
This PR turns the --keep-names bundler flag from a near-no-op into a real feature by implementing keep_expr_symbol_name / keep_stmt_symbol_name to emit __name(value, "originalName") calls, mirroring esbuild. It touches p.rs (helpers + class_has_custom_static_name), four visitor files (mod.rs for parameter defaults, visit_binary.rs for ??=/||=/&&=, visit_expr.rs for class expressions with restructured decorator-lowering flow, visit_stmt.rs for function/class declarations and export-default), drops keepNames from bundle-functions.ts, and adds/rewrites ~230 lines of bundler tests that run the output and assert .name values.
Security risks
None identified. The feature is opt-in (--keep-names) and only affects bundler output. The one indirect effect — release-build internal modules now emit __name() calls via the pre-existing --keep-names in bundle-modules.ts — routes through the same userland Object.defineProperty that __export/__toESM/etc. already use, so it is not a new tamper surface (as the author noted in-thread). Builtin functions (bundle-functions.ts) now drop keepNames entirely, so the tamper concern I raised there is resolved.
Level of scrutiny
Medium-high. This is a real feature implementation, not a mechanical fix: ~400 lines across the parser/visitor, restructured control flow in e_class (the standard-decorator early-return was removed and the unused-name-stripping block moved into an else), and insertion-order logic in s_class that splices __name() between the class stmt and its lowered decorator suffix. Everything is gated on minify_keep_names && allow_runtime, so the without-flag output is unchanged (covered by KeepNamesDisabledByDefault). The change also affects release-build internal-module codegen size/startup (every nested function/class/arrow gets a defineProperty call at load), which the author acknowledged as a possible follow-up.
Other factors
The PR went through several review rounds; all nine inline findings from prior runs are resolved (fixes in 05f92bc, 746e810, d3cfb29, 017c8d2) or explicitly deferred with empirical justification (the anonymous-decorated-class + static name case matches released-Bun behavior; the server-components wraps_exports branch is intentionally skipped). Test coverage is solid — the new tests execute bundles and assert runtime .name, cover tree-shaking, TS namespaces, decorators, static-name skipping, and the negative case. Given the scope and the codegen side-effect on internal modules, this is past the auto-approval bar; a human should take a look.
--keep-names was documented as preserving function/class names under minification but the core helper keep_expr_symbol_name was a no-op stub that returned its input unchanged. Every declared function/class and every arrow/function expression bound to an identifier still lost its .name to the identifier renamer, breaking name-keyed DI registries and serializers in minified bundles. This implements keep_expr_symbol_name to wrap expressions in __name(value, "originalName") (the runtime helper already existed in runtime.js), adds keep_stmt_symbol_name for function/class declarations and export default, and plumbs both through the statement/expression visitors. Classes that define their own static name member are skipped so the user's value is not overwritten. The existing keep-names bundler tests only asserted output shape with minifyIdentifiers disabled; they now run the bundled output and assert the actual .name values with identifiers minified. Two previously todo'd esbuild FunctionHoistingKeepNames tests are also enabled.
…before lower_class Emit __name for functions exported from a TypeScript namespace (the namespace-export branch in s_function was a separate if arm that bypassed the keep_stmt_symbol_name call). Capture class_has_custom_static_name before lower_class runs in both s_class and the export-default class arm, since lowering can hoist static members out of the property list.
…_name before mutation class_has_custom_static_name skipped IsComputed properties, missing static ["name"] = ... whose key is an EString with IsComputed set. The inner EString match already filters non-constant computed keys, so drop the flag guard. In the export-default class arm, compute keep_name from class.class before lower_class / the server-components mem::take so a named default class still sees its real name.
…ssign, fix ordering/tree-shaking edges
- Mark the trailing __name() S::SExpr with does_not_affect_tree_shaking so
an unused top-level declaration is not pinned by its own keep-names call.
- s_class: insert __name() immediately after the class inside lower_class
output (before decorator suffix statements) so decorator callbacks observe
the preserved name; same in the export-default class arm.
- s_export_default(class): capture keep_name/has_custom_static_name before
decorator-name injection so an anonymous default class with
accessors/decorators still gets "default", not the internal
{source}_default symbol.
- visit_binary: extend the BinAssign keep-names handling to ??=, ||=, &&=
(all perform NamedEvaluation with the LHS name per ES 13.15.2).
- e_class: hoist the static-name check before decorator lowering and wrap
the lowered result for named class expressions that take the
should_lower_standard_decorators early return.
…s from builtin codegen
visit_args wraps a simple BIdentifier parameter default the same way the
destructuring-binding default paths already do, so
function f(cb = () => {}) {} keeps cb.name under --minify-identifiers.
bundle-functions.ts / bundle-modules.ts passed keepNames for its old
"don't strip unused expression names" side effect. Now that the flag
emits __name() calls, the helper lands outside the captured function
body / builtin wrapper and is undefined at runtime, so drop it. Those
scripts do not minify identifiers, so .name inference from bindings is
unaffected.
bundle-functions.ts captures only the source between $$capture_start$$
and $$capture_end$$, so the bundler's __name runtime helper (emitted
at the top of the output) is discarded and the captured body references
an undefined __name. Detect __name( in the captured body and inject a
local definition after "use strict";.
bundle-modules.ts wraps the full per-file bundler output in
(function(){...}), which already includes the __name helper, so restore
--keep-names there.
…ed __name helper Route the keep-names helper through the JSC intrinsic so builtin function bodies stay tamper-safe, matching the existing $Object.$defineProperty usage elsewhere in src/js/builtins.
bundle-functions.ts does not minify identifiers, and the only named
function expressions in src/js/builtins are bound to a matching
identifier (var getStringWidth = function getStringWidth(...)), so
binding-inferred .name is already correct. Dropping keepNames avoids
the __name() wrapper calls entirely, which removes the need for the
capture-window helper injection and its tamper-safety / sloppy-branch
edge cases.
bundle-modules.ts keeps --keep-names: internal modules have named
function expressions whose .name existing tests depend on
(crypto.createHash, EventEmitter.prototype.emit, streams _write), and
the (function(){<full output>}) wrapper already includes the bundler's
__name helper at module scope.
…oose/mongodb shape, #15552)
1328250 to
4a2aa48
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
No new issues found in this pass, but this is a ~400-line parser/visitor change implementing a user-facing bundler feature with knock-on effects on internal-module codegen, so it warrants a human sign-off.
What was reviewed:
keep_expr_symbol_name/keep_stmt_symbol_nameemitters and all call sites (declarators, assignments incl.&&=/||=/??=, param defaults,export default, TS namespaces,s_classdecorator ordering).e_classcontrol-flow restructure (earlyreturn→else) and theclass_has_custom_static_nameskip.bundle-functions.tsdroppingkeepNames— the siblingbundle-modules.tsstill passes--keep-names, so release-build internal modules now run__name()(userlandObject.defineProperty) at load; the author flagged this as a possible follow-up.- comment-cop has 7 open inline notes on the new code comments.
Extended reasoning...
Overview
Implements --keep-names for bun build by making keep_expr_symbol_name / keep_stmt_symbol_name emit __name(value, "originalName") calls, and wiring them into every relevant visitor site: variable declarators, plain and logical assignment (= ??= ||= &&=), simple and destructuring parameter defaults, class field initializers, export default function/class, TS namespace export function/class, function/class declarations, and named class expressions (including the standard-decorator early-return path in e_class). Adds a class_has_custom_static_name guard so a user-written static name is not clobbered. Drops keepNames from bundle-functions.ts codegen. Rewrites the two existing keep-names tests to run the bundle and adds seven new itBundled cases plus un-todos two esbuild extra tests.
Security risks
None direct. The one tamper-safety concern raised earlier — __name reading userland Object.defineProperty inside JSC builtin functions — was resolved by dropping keepNames from bundle-functions.ts. bundle-modules.ts (release-build internal modules) still passes --keep-names, so those modules now execute the runtime.js __name helper at load; the author's position is that this is the same surface as the existing __export/__toESM helpers. That is a reasonable argument but is a design/perf tradeoff a maintainer should confirm.
Level of scrutiny
High. This touches the core JS parser visit pass across five files, restructures control flow in e_class (removes an early return), inserts synthesized statements into the middle of lower_class output, and changes what every release-build internal module executes at load time. It went through several review iterations with non-trivial fixes (decorator ordering, mem::take capture, simple parameter defaults, sloppy-mode injection). This is a feature implementation, not a mechanical fix.
Other factors
- Test coverage is good: the new tests run the minified bundle and assert exact
.namevalues across the variant matrix, plus a negative test that no__name/name strings appear without the flag, a tree-shaking test, and a CJS-shape regression test for #15552. - All prior inline findings from the bug-hunting system are marked resolved.
github-actionscomment-cop has 7 unresolved inline comments (dated today) flagging the new multi-line code comments under the repo's "paragraph-long comment justifying a workaround" rule; the author has not responded to those yet.- The author explicitly left two edges as follow-ups (anonymous decorated class expr with
static name; server-componentswraps_exportsbranch) — both were verified to match pre-existing behaviour rather than regress it, but a maintainer should agree those are acceptable to defer.
|
Closing in favor of #40503, which implements the same feature with the same esbuild-style I left a comment on #40503 with the cases from this branch that it does not cover yet: the #12304 and #15552 should close when #40503 lands. #25332 is already closed as a duplicate of #12304. |
What does this PR do?
bun build --helpdocuments--keep-namesas "Preserve original function and class names when minifying", but the flag's only visible effect was to stop the syntax pass from stripping the inline name off an unused function/class expression. Every declared function/class, and every arrow or function expression assigned to a binding, still lost its.nameto the identifier renamer. Name-keyed DI registries and serializers break in minified bundles.Cause
keep_expr_symbol_nameinsrc/js_parser/p.rswas a no-op stub that returned its input unchanged. All the visitor call sites that feed it (variable declarators, assignments, class field initializers, default parameter values,export default) were already plumbed; the helper just never produced anything. The__nameruntime helper insrc/runtime.jsand the__nameentry inast/runtime.rsexisted but had no emitter.Fix
keep_expr_symbol_nameto wrap expressions in__name(value, "originalName"), markedcan_be_unwrapped_if_unused: IfUnusedso tree-shaking still removes unused bindings.keep_stmt_symbol_nameand emit__name(ref, "originalName")after function/class declarations (includingexport default function/class).e_class(anonymous class expressions were already covered viamaybe_keep_expr_symbol_name).static namemember (field/method/getter/setter) so__name()never clobbers a user-supplied value.All of this is gated on
options.features.minify_keep_names && allow_runtime, so output without--keep-namesis byte-identical.After:
The JS API path (
Bun.build({ minify: { keepNames: true } })) already read the option; it now has the same effect as the CLI flag.Tests
bundler_minify.test.ts: the two existing keep-names tests are rewritten to run the bundle and assert.namevalues withminifyIdentifiers: true; addedKeepNamesExportDefault,KeepNamesSkipsStaticNameMember, and a negativeKeepNamesDisabledByDefaultthat asserts no__name/name strings are emitted without the flag.esbuild/extra.test.ts: un-todoFunctionHoistingKeepNames1/2(block-scopedif (1) function f() {}hoisting), which now pass.Fixes #12304
Fixes #25332
Fixes #15552
[review] gate passed · iteration 1 · 8 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 5 passed · 0 rejected · iteration 1
evidence per changed file