fix(es/minifier): avoid moved binding collisions#11981
Conversation
|
![kodiakhq[bot]](https://avatars.githubusercontent.com/in/29196?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3aa4da8e55
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| fn visit_class_expr(&mut self, node: &ClassExpr) { | ||
| let old_analyzer = self.analyzer.enter_block_scope(); | ||
| let old_analyzer = self.analyzer.enter_block_scope(Some(node.class.ctxt)); |
There was a problem hiding this comment.
With direct eval inside a named class expression, e.g. const C = class Foo { static m() { return eval("Foo") } }, this uses Class::ctxt as the source-scope context, but the resolver leaves Class::ctxt empty and marks the class-expression name in the child scope in visit_mut_class_expr. That makes id.1 == ctxt false for Foo, so the name is queued for mangling even though eval still resolves the string "Foo", regressing the eval-preservation behavior this code is trying to maintain.
Useful? React with 👍 / 👎.
|
|
||
| fn visit_constructor(&mut self, node: &Constructor) { | ||
| let old_analyzer = self.analyzer.enter_fn_scope(); | ||
| let old_analyzer = self.analyzer.enter_fn_scope(node.ctxt); |
There was a problem hiding this comment.
For class constructors with direct eval, such as class C { constructor(longName) { eval("longName") } }, Constructor::ctxt is never populated by the resolver; visit_mut_constructor creates a child function scope, marks parameters with that child mark, and only marks body.ctxt. Passing the empty node.ctxt here means constructor parameters and locals no longer match scope_ctxt, so they can be mangled while eval still looks up their original names.
Useful? React with 👍 / 👎.
| self.decl_collector.is_pat_decl = true; | ||
|
|
||
| let old_analyzer = self.analyzer.enter_fn_scope(); | ||
| let old_analyzer = self.analyzer.enter_fn_scope(node.ctxt); |
There was a problem hiding this comment.
For arrows with direct eval, e.g. const f = (longName) => eval("longName"), the resolver creates a child function scope for the parameters but never marks ArrowExpr::ctxt; only a block body gets marked, and expression-bodied arrows have no marked body to use. Passing this empty context makes eval preservation compare the parameter context against SyntaxContext::empty(), so mangle can rename longName while eval still looks up the original string.
Useful? React with 👍 / 👎.
| for id in &self.data.preserved { | ||
| reverse.push_entry(id.0.clone(), id.clone()); |
There was a problem hiding this comment.
This only inserts eval-preserved names from the current scope into the reverse map; preserved names in child scopes are not reserved until after the parent queue has already been renamed. When an ancestor has a queued moved binding and an eval-containing child preserves a short local such as a, the ancestor can be mangled to a, and references to the ancestor from inside that child will bind to the child's local once contexts are erased.
Useful? React with 👍 / 👎.
Merging this PR will degrade performance by 2.35%
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ❌ | Simulation | es/hygiene/typescript |
431.9 ms | 442.3 ms | -2.35% |
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing kdy1/fix-11977-minifier-binding (3aa4da8) with main (26d8a28)2
Footnotes
-
61 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. ↩
-
No successful run was found on
main(9c8e6f3) during the generation of this report, so 26d8a28 was used instead as the comparison base. There might be some changes unrelated to this pull request in this report. ↩
|
I believe this PR still has its own merit. Maybe in the future we could enable inline/unused pass even in presence of |
|
I'll keep this as a draft 👍 |
2 participants
Problem
IIFE parameter extraction can move bindings from an inner scope into an outer scope that contains direct
eval. The rename analysis preserved every declaration whose context descended from the resolver top-level mark whenevalwas present, so moved/inlined bindings could keep a printed name that collided with an existing same-scope user declaration aftercompress + mangle.Fixes #11977.
Changes
swc_ecma_transforms_base::renameanalysis.g.foo = function() { ... }case.Testing
git submodule update --init --recursivecargo test -p swc_ecma_minifier --test exec issue_11977cargo test -p swc_ecma_minifier --test exec issue_11294cargo test -p swc_ecma_minifiercargo fmt --allcargo clippy --all --all-targets -- -D warnings