bundler: make --drop match bound identifiers (imports/locals), not just free globals#34985
bundler: make --drop match bound identifiers (imports/locals), not just free globals#34985robobun wants to merge 3 commits into
Conversation
…st free globals
`--drop=<name>` piggybacks on the define table and inherited its
unbound-only gate, so calls to an imported or locally-declared function
survived while the same name as a free global was dropped:
import { devlog } from './log';
devlog('secret'); // kept
gfn('free'); // dropped (unbound)
- e_identifier: consult the drop flag for bound symbols too (define
substitution stays unbound-only)
- e_call: accept EImportIdentifier as a droppable target and ignore the
target's recorded usage so the import can tree-shake
- is_dot_define_match: take allow_bound_root so --drop=a.b matches when
a is a local/imported binding
|
Warning Review limit reached
Next review available in: 24 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
Comment |
|
Status: diff is green. CI: two consecutive builds each tripped a single unrelated test on one lane, and neither recurred across builds.
Both test failures are in subsystems this PR does not touch ( |
|
Updated 2:58 PM PT - Jul 21st, 2026
❌ @robobun, your commit de9efca has 1 failures in 🧪 To try this PR locally: bunx bun-pr 34985That installs a local version of the PR into your bun-34985 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I didn't find bugs, but this is a user-facing semantic change to what --drop strips from bundles (now matching bound imports/locals in addition to free globals), threaded through core parser paths (is_dot_define_match signature, e_identifier, e_dot, e_call). Worth a human look to confirm the intended scope.
What was reviewed:
- All
is_dot_define_matchcall sites updated;import.metapath kept atallow_bound_root=false. - New bound-identifier branch in
e_identifieris gated on the call-target flag +assign_target == None+!is_delete_target, and only reads the drop flag —--definevalue substitution stays unbound-only (covered byDefineDoesNotReplaceBoundIdentifier). - New
ignore_usage_of_identifier_in_dot_chainat the drop site balances the target usage recorded during visit;ignore_usageusessaturating_subso pre-existing unbound-root drops (console.*) aren't at risk of underflow.
Extended reasoning...
Overview
The PR extends --drop=<name> and --drop=<root>.<prop> to match calls whose target resolves to a bound symbol (imports, local function/const, namespace imports), where previously only unbound (free global) roots matched. It touches src/js_parser/p.rs (adds allow_bound_root param to is_dot_define_match; adds EImportIdentifier arm to ignore_usage_of_identifier_in_dot_chain) and src/js_parser/visit/visit_expr.rs (new bound branch in e_identifier; per-entry allow_bound_root in the e_dot define loop; EImportIdentifier accepted as a droppable target in e_call; ignore_usage_of_identifier_in_dot_chain on the target when a call is dropped). Eight new itBundled cases exercise imported/default/local/namespace/dot roots plus two negative guards.
Security risks
None. This is bundler AST transformation logic; no untrusted-input parsing surface, syscalls, or network paths are involved.
Level of scrutiny
High. This changes what code the bundler strips from user output. Even though it aligns with the documented "anyIdentifier" behavior, it silently changes bundle output for anyone who has a bound local/import shadowing a name they pass to --drop. The is_dot_define_match signature change and the new ignore_usage_of_identifier_in_dot_chain call at visit_expr.rs:2030 also apply to the pre-existing unbound drop paths, so all existing --drop behavior flows through modified code. A maintainer should confirm this scope is intended.
Other factors
The implementation is careful: the new e_identifier branch fires only when property_access_for_method_call_maybe_should_replace_with_undefined is set (i.e., the identifier is a call target) and only consults the drop flag, so --define substitution semantics for bound identifiers are unchanged. In e_dot, allow_bound_root is derived from the define entry's drop flag before matching, but the actual replacement still requires the call-target flag at line 1391-1395, so non-call reads of a bound logger.debug won't be replaced by the drop flag (they may match, but valueless() on drop entries prevents value substitution). Negative tests cover the bare-identifier non-call case; there is no explicit negative test for the dotted non-call case (const ref = logger.debug with --drop=logger.debug), which a maintainer might want. The PR reports the broader bundler suites (edgecase, esbuild/dce, minify, regressions, transpiler) as green.
|
Added On scope: the new bound-identifier branch only consults the |
There was a problem hiding this comment.
No bugs found. Deferring to a human because this is a deliberate semantic change to --drop (now matches imports/locals, not just unbound globals) — the implementation and negative guards look right, but a maintainer should confirm the product intent.
What was reviewed:
- New bound-identifier branch in
e_identifieris gated on the call-target flag, so--definesubstitution and non-call reads stay unbound-only (covered byDefineDoesNotReplaceBoundIdentifier/BoundIdentifierNonCallNotReplaced). is_dot_define_match(.., allow_bound_root): both call sites updated;e_dotonly enables it for drop entries, and the flag it sets is still guarded byproperty_access_for_method_call_maybe_should_replace_with_undefined, soconst ref = logger.debugis unaffected.ignore_usage_of_identifier_in_dot_chainat the replacement site runs afteris_control_flow_deadis restored, so the decrement balances the target-visit'srecord_usage; for the pre-existing unbound path it's a saturating no-op.
Extended reasoning...
Overview
Extends --drop=<name> / --drop=<root>.<prop> to match calls through bound identifiers (imports, local function/const, namespace imports), where previously only unbound globals matched. Touches src/js_parser/p.rs (is_dot_define_match gains allow_bound_root; ignore_usage_of_identifier_in_dot_chain handles EImportIdentifier), src/js_parser/visit/visit_expr.rs (new bound branch in e_identifier, allow_bound_root wiring in e_dot, EImportIdentifier accepted as droppable target in e_call, and ignore_usage on the target chain when a call is dropped), and adds 9 bundler tests.
Security risks
None. No untrusted-input parsing changes; this only affects which call expressions the bundler elides when the user explicitly opts in via --drop.
Level of scrutiny
Moderate-to-high. The diff is small (~45 lines of Rust) and well-scoped, but it changes bundler output semantics for anyone using --drop against a name that happens to be bound in scope. The PR frames this as aligning with the documented drop: ["anyIdentifier.or.propertyAccess"] contract, which is a reasonable read, but it is still a user-visible behavior change that a maintainer should sign off on rather than a pure bug fix.
Other factors
The negative guards are the important part and they check out: the new bound-identifier branch in e_identifier (visit_expr.rs:324) is behind property_access_for_method_call_maybe_should_replace_with_undefined, which is only set from e_call's target visit and propagated through e_dot, so bare reads and --define value substitution are untouched. In e_dot, allow_bound_root is derived per-entry from method_call_must_be_replaced_with_undefined(), so a plain --define=a.b=... entry still uses the old unbound-only root check; the only observable side effect for a bound-root non-call match is copying can_be_removed_if_unused onto the dot, which is consistent with the unbound case and arguably desirable under an explicit --drop. The added ignore_usage_of_identifier_in_dot_chain at the replacement site is placed after is_control_flow_dead is restored to old_is_control_flow_dead, so it correctly balances the usage recorded when the target was visited (and is a saturating no-op for unbound symbols with no declaration). All call sites of is_dot_define_match were updated. Test coverage is thorough (imported named/default/namespace, local function/const, dotted bound root, plus three negative guards) and the author reports edgecase/dce/minify/regressions/transpiler suites green.
There was a problem hiding this comment.
No bugs found, but this broadens --drop to match bound identifiers (imports, local const/function) — a user-facing semantic change routed through core visitor paths (e_identifier, e_dot, e_call, use-count accounting). Worth a human confirming the intended semantics.
Checked: the new bound-identifier branch is gated on call-target context only (property_access_for_method_call_maybe_should_replace_with_undefined), so --define and non-call reads are unchanged; drop entries are always valueless per DefineData::parse/merge, so allow_bound_root in e_dot never triggers value substitution; the new ignore_usage_of_identifier_in_dot_chain call runs after is_control_flow_dead is restored, so it balances the target's prior record_usage and no-ops on EUndefined (import.meta.hot path).
Extended reasoning...
Overview
Extends --drop=<name> to match calls through bound identifiers (imports, local declarations, namespace imports) instead of only unbound globals. Touches src/js_parser/p.rs (is_dot_define_match gains an allow_bound_root param; ignore_usage_of_identifier_in_dot_chain handles EImportIdentifier) and src/js_parser/visit/visit_expr.rs (e_identifier adds a bound-symbol drop-flag branch; e_dot sets allow_bound_root per-entry from the drop flag; e_call accepts EImportIdentifier targets and now calls ignore_usage_of_identifier_in_dot_chain on the dropped target). Nine new itBundled tests including three negative guards.
Security risks
None. Bundler transform behavior; no untrusted-input parsing, auth, crypto, or filesystem changes.
Level of scrutiny
Medium-high. The diff is small and internally consistent, but it lives in the parser's expression visitor and adjusts symbol use-count accounting — an area where off-by-one balancing errors show up as tree-shaking regressions far from the change. It also intentionally changes user-visible semantics: --drop=log will now remove calls to a local function log(), which is what the docs describe but is a broadening a maintainer should sign off on.
Other factors
- I traced the
allow_bound_rootpath ine_dot: drop entries are constructed withvalue_is_undefined=true(defines.rs:576) andmergeforcesvaluelesswhen either side has the drop flag, so a bound-root match never falls into!valueless()value substitution.can_be_removed_if_unusedwill now be set on bound-rootEDotmatches, which is benign. ignore_usage_of_identifier_in_dot_chain(e_.target)is called afterp.is_control_flow_deadis restored to its pre-visit value, soignore_usage's dead-flow guard behaves correctly relative to the target visit (which happened before the flag was flipped). For pre-existing unbound targets this is a newly-addedsaturating_sub(1)that balances the earlierrecord_usage; forEUndefined(import.meta.hot) it hits the_ => returnarm.- Negative guards cover non-call bound identifier, non-call bound dot, and
--defineon a bound identifier — the obvious over-reach hazards. - Gate evidence shows fail-before/pass-after on ASAN and release;
bundler_edgecase,esbuild/dce,bundler_minify, andtranspiler.test.jsare reported green.
Deferring because this is a deliberate semantic change to a user-facing bundler option flowing through hot visitor code; a maintainer should confirm the behavior is desired.
2 participants
Reproduction
The docs describe
drop: ["anyIdentifier.or.propertyAccess"], but only calls through an unbound identifier were removed. Calls through an import, a localfunction/const, or a namespace import (ns.method(...)withdrop: ["ns"]ordrop: ["ns.method"]) all survived without warning.Cause
--dropentries are stored in the define table withmethod_call_must_be_replaced_with_undefinedset.e_identifieronly consults that table when the symbol's kind isUnbound:and
is_dot_define_matchapplies the sameUnbound-only check to the root of a dotted define. Imported bindings areImport, local function declarations areHoistedFunction,constisConstant, so the drop flag is never reached for them. When the target was an import,handle_identifieralso rewrites it toEImportIdentifier, which thee_calldrop path did not recognize as a removable target.Fix
e_identifier: for bound symbols that are being visited as a call target, consult the drop flag on the matching define entry. Value substitution for--definestays unbound-only.is_dot_define_match: addallow_bound_root, set totrueonly for drop entries so--drop=a.bmatches whenais a local or imported binding.e_call: acceptEImportIdentifieras a droppable target, andignore_usageon the target chain when a call is dropped so the now-unused import/local can tree-shake.After the fix:
Verification
Also green:
bundler_edgecase(116),esbuild/dce(73),bundler_minify(48),bundler_regressions,transpiler/transpiler.test.js(182).[review] gate passed · iteration 2 · 3 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 3 passed · 0 rejected · iteration 2
evidence per changed file