Skip to content

fix(mangler): assign correct slot to shadowed function-expression names#21535

Merged
graphite-app[bot] merged 1 commit intomainfrom
fix/mangler-shadowed-fn-expr-name
Apr 20, 2026
Merged

fix(mangler): assign correct slot to shadowed function-expression names#21535
graphite-app[bot] merged 1 commit intomainfrom
fix/mangler-shadowed-fn-expr-name

Conversation

@Dunqing
Copy link
Copy Markdown
Member

@Dunqing Dunqing commented Apr 17, 2026

Summary

https://github.com/oxc-project/monitor-oxc/actions/runs/24643474969/job/72051723921

Fix a mangler bug where a named function expression whose name is shadowed by a same-named declaration in its body collides with an unrelated outer-scope variable in the mangled output.

The bug

A named function expression binds its name into the function's own scope. If anything inside the body redeclares that name — var, parameter, let, const, a nested function, or a nested class — the scope binding map entry for the fn-expr is overwritten and the fn-expr symbol is orphaned (it still exists, but scoping.iter_bindings() never yields it).

Previously the slot array was initialised with 0, so orphans silently kept slot 0 and ended up sharing the mangled name of whichever unrelated symbol actually owned slot 0.

function _() {
  var x;
  var f = function foo() { var foo = x; };
}

Before this fix, function foo and the outer _ collapsed onto the same identifier in the output.

Fix

  • Initialise slots to a SLOT_UNASSIGNED sentinel (Slot::MAX).
  • Inside the per-scope slot-assignment loop, after slotting a function scope's bindings, check whether the scope's owning AST node is a named function with its id symbol still unassigned (the orphan signature — function declarations have their name slotted in the parent scope and so fail this check). If so, look up the shadower in the scope's binding map and copy its slot. Both render with the same mangled name — safe, since every body reference resolves to the shadower, not the orphan. Folded into the existing scope walk so there's no second pass over symbols, and confined to function scopes (the only place this orphaning can happen).
  • In tally_slot_frequencies, skip symbols still at SLOT_UNASSIGNED so unrepaired orphans (e.g. when the shadower is keep_names-preserved) retain their original names.

Test plan

  • New function_expression_name_shadowed test covers var, parameter, and hoisted-var-in-else-block shadow kinds with concrete mangled-output assertions.
  • New shadowed_fn_expr_mangle_is_idempotent test confirms mangling the output a second time is a fixed point (sanity + multi-shadowed-fn-expr coverage).
  • Existing mangler and semantic suites pass unchanged.

Copy link
Copy Markdown
Member Author

Dunqing commented Apr 17, 2026


How to use the Graphite Merge Queue

Add either label to this PR to merge it via the merge queue:

  • 0-merge - adds this PR to the back of the merge queue
  • hotfix - for urgent changes, fast-track this PR to the front of the merge queue

You must have a Graphite account in order to use the merge queue. Sign up using this link.

An organization admin has enabled the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

This stack of pull requests is managed by Graphite. Learn more about stacking.

@github-actions github-actions Bot added A-minifier Area - Minifier A-ast Area - AST C-bug Category - Bug labels Apr 17, 2026
@codspeed-hq
Copy link
Copy Markdown

codspeed-hq Bot commented Apr 17, 2026

Merging this PR will not alter performance

✅ 44 untouched benchmarks
⏩ 7 skipped benchmarks1


Comparing fix/mangler-shadowed-fn-expr-name (23c546c) with main (065ce47)

Open in CodSpeed

Footnotes

  1. 7 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports.

@Dunqing Dunqing force-pushed the fix/mangler-shadowed-fn-expr-name branch 3 times, most recently from 9f515db to ce1489a Compare April 20, 2026 03:09
@Dunqing
Copy link
Copy Markdown
Member Author

Dunqing commented Apr 20, 2026

This is a workaround in the minifier for post-handling shadowed function-expression names created in Semantic. The real fix should implement oxc-project/backlog#176. However, this would affect every crate and lead to numerous regressions.

@Dunqing Dunqing marked this pull request as ready for review April 20, 2026 03:22
@Dunqing Dunqing force-pushed the fix/mangler-shadowed-fn-expr-name branch from ce1489a to 7e91afa Compare April 20, 2026 03:26
@Dunqing Dunqing requested review from Boshen and sapphi-red April 20, 2026 03:26
Comment thread crates/oxc_mangler/src/lib.rs
@Dunqing Dunqing force-pushed the fix/mangler-shadowed-fn-expr-name branch from 7db3439 to 23c546c Compare April 20, 2026 05:20
@sapphi-red sapphi-red added the 0-merge Merge with Graphite Merge Queue label Apr 20, 2026
Copy link
Copy Markdown
Member

sapphi-red commented Apr 20, 2026

Merge activity

…es (#21535)

## Summary

https://github.com/oxc-project/monitor-oxc/actions/runs/24643474969/job/72051723921

Fix a mangler bug where a named function expression whose name is shadowed by a same-named declaration in its body collides with an unrelated outer-scope variable in the mangled output.

## The bug

A named function expression binds its name into the function's own scope. If anything inside the body redeclares that name — `var`, parameter, `let`, `const`, a nested `function`, or a nested `class` — the scope binding map entry for the fn-expr is overwritten and the fn-expr symbol is orphaned (it still exists, but `scoping.iter_bindings()` never yields it).

Previously the slot array was initialised with `0`, so orphans silently kept slot `0` and ended up sharing the mangled name of whichever unrelated symbol actually owned slot `0`.

```js
function _() {
  var x;
  var f = function foo() { var foo = x; };
}
```

Before this fix, `function foo` and the outer `_` collapsed onto the same identifier in the output.

## Fix

- Initialise `slots` to a `SLOT_UNASSIGNED` sentinel (`Slot::MAX`).
- Inside the per-scope slot-assignment loop, after slotting a function scope's bindings, check whether the scope's owning AST node is a named function with its `id` symbol still unassigned (the orphan signature — function declarations have their name slotted in the parent scope and so fail this check). If so, look up the shadower in the scope's binding map and copy its slot. Both render with the same mangled name — safe, since every body reference resolves to the shadower, not the orphan. Folded into the existing scope walk so there's no second pass over symbols, and confined to function scopes (the only place this orphaning can happen).
- In `tally_slot_frequencies`, skip symbols still at `SLOT_UNASSIGNED` so unrepaired orphans (e.g. when the shadower is `keep_names`-preserved) retain their original names.

## Test plan

- [x] New `function_expression_name_shadowed` test covers `var`, parameter, and hoisted-var-in-else-block shadow kinds with concrete mangled-output assertions.
- [x] New `shadowed_fn_expr_mangle_is_idempotent` test confirms mangling the output a second time is a fixed point (sanity + multi-shadowed-fn-expr coverage).
- [x] Existing mangler and semantic suites pass unchanged.
@graphite-app graphite-app Bot force-pushed the fix/mangler-shadowed-fn-expr-name branch from 23c546c to 50e9d26 Compare April 20, 2026 05:43
@graphite-app graphite-app Bot merged commit 50e9d26 into main Apr 20, 2026
27 checks passed
@graphite-app graphite-app Bot removed the 0-merge Merge with Graphite Merge Queue label Apr 20, 2026
@graphite-app graphite-app Bot deleted the fix/mangler-shadowed-fn-expr-name branch April 20, 2026 06:01
camc314 added a commit that referenced this pull request Apr 20, 2026
### 🐛 Bug Fixes

- 48967e8 isolated_declarations: Drop required type check for private
parameter properties on private constructors (#21515) (Dunqing)
- 91e5bde transformer/typescript: Preserve computed-key static block
when class has an empty constructor (#21562) (Dunqing)
- 50e9d26 mangler: Assign correct slot to shadowed function-expression
names (#21535) (Dunqing)
- 065ce47 isolated_declarations: Collect types from private accessors
for paired inference (#21516) (Dunqing)
- 00fc136 codegen: Preserve coverage comments before object properties
(#21312) (bab)
- d676e0c minifier: Mark LHS of `??=` as read when converting from `==
null &&` (#21546) (Gunnlaugur Thor Briem)

### ⚡ Performance

- e45efc5 parser: Reduce `try_parse` usage in favour of `lookahead`
(#21532) (Boshen)
- ddb1bf8 parser: Avoid redundant `IdentifierReference` clone in
shorthand property (#21511) (Boshen)
- be2b392 allocator: Store pointers directly in `Arena` (#21483)
(overlookmotel)

Co-authored-by: Boshen <1430279+Boshen@users.noreply.github.com>
Co-authored-by: Cameron <cameron.clark@hey.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-ast Area - AST A-minifier Area - Minifier C-bug Category - Bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants