feat(es/minifier): Fine grained effect analysis of class#11814
feat(es/minifier): Fine grained effect analysis of class#11814Austaras wants to merge 4 commits intoswc-project:mainfrom
Conversation
| @@ -1,3 +0,0 @@ | |||
| class c { | |||
| [c] = 42; | |||
| } | |||
There was a problem hiding this comment.
This is actually fine. Refer class itself in computed key is a TDZ violation like let a = a so it's fine to remove it. I wasn't aware of this when I introduce used_recursively.
![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: 108f868dea
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| let Some(side_effects) = | ||
| extract_class_side_effect(self.ctx.expr_ctx, Some(ident), class) | ||
| else { |
There was a problem hiding this comment.
After the broad self-reference guard was removed, this path now relies on extract_class_side_effect to decide whether an unused class declaration can be erased, but that helper does not treat class-name references inside computed keys as effectful. Cases like class C { [C] = 1 } (or set [C](v){}) can therefore be dropped under toplevel+unused, even though evaluating the original class throws a TDZ ReferenceError, so this changes runtime semantics.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
Fixes SWC minifier dead-code elimination for classes that self-reference inside their body (notably when extending an external/unknown superclass), addressing #11730.
Changes:
- Treat class identifiers as “recursive usage” while visiting class bodies to avoid counting self-references as external usage.
- Refine class side-effect extraction (superclass expr, static field initializers, and simple static blocks) to enable dropping more unused classes while preserving necessary effects.
- Update/add fixtures and snapshot outputs reflecting the new DCE behavior across terser tests, tsc-reference golden files, and bundle-size snapshot.
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| crates/swc_ecma_minifier/src/usage_analyzer/analyzer/mod.rs | Marks class ids as recursively-used while analyzing class bodies to avoid keeping unused self-referential classes. |
| crates/swc_ecma_minifier/src/compress/optimize/util.rs | Extends extract_class_side_effect to consider class ident, super, and static blocks during side-effect extraction. |
| crates/swc_ecma_minifier/src/compress/optimize/unused.rs | Uses the refined class side-effect extraction when dropping unused class declarations. |
| crates/swc_ecma_minifier/src/compress/optimize/mod.rs | Routes unused class-expression elimination through the updated side-effect extraction logic. |
| crates/swc_ecma_minifier/tests/fixture/issues/11730/input.js | Adds a regression input for #11730. |
| crates/swc_ecma_minifier/tests/fixture/issues/11730/output.js | Adds expected output showing the class eliminated with Unknown preserved. |
| crates/swc_ecma_minifier/tests/terser/compress/classes/class_recursive_refs/output.js | Updates expected output reflecting more aggressive removal of unused classes. |
| crates/swc_ecma_minifier/tests/terser/compress/classes/class_duplication/output.js | Updates expected output (export default class handling) under new class-DCE behavior. |
| crates/swc_ecma_minifier/tests/terser/compress/classes/class_duplication_2/output.js | Updates expected output for class-in-call contexts under new class-DCE behavior. |
| crates/swc_ecma_minifier/tests/fixture/issues/10981/output.js | Updates expected output based on revised class/static side-effect handling. |
| crates/swc_ecma_minifier/tests/benches-full/d3.js | Updates benchmark golden output showing different class retention/extraction decisions. |
| crates/swc_ecma_minifier/tests/libs-size.snapshot.md | Updates gzip-size snapshot impacted by new minification output. |
| crates/swc/tests/tsc-references/uniqueSymbols.2.minified.js | Updates golden output reflecting new class/static-block DCE behavior. |
| crates/swc/tests/tsc-references/uniqueSymbolsDeclarations.2.minified.js | Updates golden output reflecting new class/static-block DCE behavior. |
| crates/swc/tests/tsc-references/superSymbolIndexedAccess3.2.minified.js | Updates golden output reflecting class dropping when only side effects remain. |
| crates/swc/tests/tsc-references/classStaticBlock15(target=esnext).2.minified.js | Updates golden output for static-block related transformations. |
| crates/swc/tests/tsc-references/classStaticBlock15(target=es2022).2.minified.js | Updates golden output for static-block related transformations. |
| crates/swc/tests/tsc-references/assignParameterPropertyToPropertyDeclarationESNext.2.minified.js | Updates golden output reflecting removal of unused classes. |
| crates/swc/tests/tsc-references/assignParameterPropertyToPropertyDeclarationES2022.2.minified.js | Updates golden output reflecting removal of unused classes. |
Comments suppressed due to low confidence (1)
crates/swc_ecma_minifier/src/compress/optimize/util.rs:187
PropName::Computedkeys are evaluated during class definition. We currently extractkey.exprside effects without guarding against references that are only valid/meaningful inside class evaluation (e.g.super) or against self-references to the class identifier. This can produce invalid output when the class is dropped (e.g. extractingleak(Dead)from[leak(Dead)]()after removingclass Dead). Consider applying the samecontains_super/IdentUsageFinderchecks used for static initializers (and also considercontains_this_exprto preserve strict-modethissemantics) before extracting computed key expressions (both for methods and for computed class props).
ClassMember::Method(ClassMethod {
key: PropName::Computed(key),
..
}) => {
if key.expr.may_have_side_effects(expr_ctx) {
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Merging this PR will not alter performance
Comparing 
Footnotes
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 213fa63c1d
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| self.used_recursively | ||
| .insert(id.clone(), RecursiveUsage::FnOrClass); | ||
| n.visit_children_with(self); |
There was a problem hiding this comment.
extends as live class usage
Marking class declarations as used_recursively before visiting children suppresses normal usage counting for references inside the class heritage. With this change, class C extends C {} can satisfy the usage_count == 0 drop condition, and the class may be erased even though evaluating the original heritage throws ReferenceError (TDZ). Heritage self-references should prevent dead-class removal.
Useful? React with 👍 / 👎.
Binary Sizes
Commit: a5ed7e1 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ff7a50d9b3
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 19 out of 19 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ClassMember::Method(ClassMethod { | ||
| key: PropName::Computed(key), | ||
| .. | ||
| }) => { | ||
| if key.expr.may_have_side_effects(expr_ctx) { | ||
| res.push(&mut key.expr); | ||
| key_res.push(&mut key.expr); | ||
| } | ||
| } |
There was a problem hiding this comment.
For computed method names, key.expr can reference the class name binding (e.g. class C { [C.foo()](){} }). If this key expression is extracted while removing the class, it will run without the class binding and can change behavior (ReferenceError / different lookup). extract_class_side_effect checks IdentUsageFinder for static field values and static blocks but not for computed keys; consider applying the same ident self-reference check to key.expr before collecting it (or returning None if referenced).
| ClassMember::ClassProp(p) => { | ||
| if let PropName::Computed(key) = &mut p.key { | ||
| if key.expr.may_have_side_effects(expr_ctx) { | ||
| res.push(&mut key.expr); | ||
| key_res.push(&mut key.expr); | ||
| } | ||
| } |
There was a problem hiding this comment.
For computed class field names, key.expr can reference the class name binding. When the class is dropped and side effects are extracted, evaluating such a key expression outside the class body can become invalid. extract_class_side_effect should also guard PropName::Computed(key) with the ident self-reference check (similar to the checks done for p.value / static blocks), otherwise the minifier may emit extracted expressions that reference a now-removed class binding.
|
Well it's my first time to find out that we run babel tests with compress. Maybe just ignore those tests? |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ded02a2d2c
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| } | ||
| } | ||
|
|
||
| value.push(first); |
There was a problem hiding this comment.
extract_class_side_effect now lifts single-expression static blocks into a top-level sequence, but this branch only rejects this, super, and class-name references. A static block like static { #x in sideEffect() } (or obj().#x) can therefore be extracted when the class declaration is dropped, emitting a private-name expression outside the class body, which is invalid JavaScript syntax and changes a valid program into a parse error.
Useful? React with 👍 / 👎.
|
I added some test cases to ensure that this change does not break any real app |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 27 out of 27 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let mut res = Vec::new(); | ||
| let mut value = Vec::new(); | ||
| if let Some(e) = &mut c.super_class { | ||
| if e.may_have_side_effects(expr_ctx) { | ||
| res.push(e); | ||
| value.push(e); | ||
| } | ||
| } |
There was a problem hiding this comment.
extract_class_side_effect collects super_class into value and then appends value after computed-key effects (res.append(&mut value)). This reorders evaluation so the extends expression would run after computed property keys, but in JS class definition evaluation the heritage (extends) must be evaluated first. Consider keeping a dedicated prefix list (heritage first), then computed keys, then static initializers/blocks, or otherwise inserting super_class effects at the front of the returned sequence to preserve spec order.
| if let Some(id) = ident { | ||
| if IdentUsageFinder::find(id, v) { | ||
| return None; | ||
| } | ||
| } | ||
|
|
||
| value.push(v); |
There was a problem hiding this comment.
The new self-reference guard (IdentUsageFinder::find(id, v)) only runs for static field values. Similar self-reference checks are needed for other class-evaluation-time expressions that can throw/behave differently when the class is removed, especially the extends (heritage) expression and computed member keys (which are evaluated during class definition). Without that, cases like class X extends X {} or [X](){} can become droppable even though they throw due to TDZ / depend on the class binding.
| if let Some(e) = &mut c.super_class { | ||
| if e.may_have_side_effects(expr_ctx) { | ||
| res.push(e); | ||
| value.push(e); | ||
| } | ||
| } |
There was a problem hiding this comment.
For super_class, extraction is currently gated only by may_have_side_effects. But extends <class-name> (or other self-references) can throw at class evaluation time even when may_have_side_effects returns false (e.g. extends X where X is the class binding in TDZ). Consider returning None when super_class references ident (or contains super) to prevent incorrectly dropping classes that must be preserved for runtime error behavior.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9c1dc79f23
ℹ️ 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".
|
I have deleted the test |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 25 out of 25 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
crates/swc_ecma_minifier/src/compress/optimize/util.rs:9
- This file already uses
rustc_hash::{FxHashMap, FxHashSet}, but the newClassEffectVisitorusesstd::collections::HashSetforprivate_ident. For consistency (and to avoid a potentially slower hash implementation in a hot path), consider usingFxHashSet<Atom>here as well.
use std::{
borrow::Borrow,
collections::HashSet,
mem::{self, take},
ops::{Deref, DerefMut},
};
use rustc_hash::{FxHashMap, FxHashSet};
use swc_atoms::{Atom, Wtf8Atom};
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 75c975d90e
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if first.may_have_side_effects(expr_ctx) { | ||
| first.visit_with(&mut visitor); | ||
| if visitor.found { | ||
| return None; | ||
| } |
There was a problem hiding this comment.
new.target static effects in class context
This hoisting path can move static-block expressions out of the class body after only visitor.found checks, but ClassEffectVisitor never flags new.target usage. As a result, an unused class like class C { static { sideEffect(new.target); } } can be rewritten to sideEffect(new.target), where new.target is no longer in a valid class/function context. That turns valid input into a syntax error and changes observable behavior.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 05e4d41f9c
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| let Some(side_effects) = | ||
| extract_class_side_effect(self.ctx.expr_ctx, Some(ident), class) | ||
| else { |
There was a problem hiding this comment.
This new drop_unused_decl path hoists class static initializer/block expressions out of the class body into a plain statement, but class bodies are always strict while the surrounding scope may be sloppy. In sloppy contexts this changes observable behavior (for example, class C { static x = (undeclared = 1) } throws ReferenceError during class evaluation, while hoisted undeclared = 1; creates/updates a global without throwing). Because this commit now applies extraction to unused class declarations with static effects, it can silently change runtime semantics.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This is a very rare use case. Maybe in another PR.
| let left = if let Expr::PrivateName(_) = &**left { | ||
| None | ||
| } else { |
There was a problem hiding this comment.
in checks when reducing binary expressions
Special-casing Expr::PrivateName on the left side to None drops the left operand entirely, which is not semantics-preserving for private-brand checks. For #x in rhs, the original code performs the in operation (including throwing when rhs is not an object), but after this change it can collapse to just evaluating rhs. When class side effects are extracted, this turns runtime TypeError cases into non-throwing behavior.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
I'd argue this aligns with current behaviour although it is a bug. See terser/terser#1297
Description:
BREAKING CHANGE:
Related issue (if exists):
Closes #11730