Skip to content

Commit 18aeac6

Browse files
authored
fix(es/minifier): don't eliminate class expr if some side effects contain this (#11003)
**Description:** Manual reasoning: > Um...There are some options to handle this problem: > 1. Prefer size. Check if there's any `this` in the statement with side effect. This could be inefficient and I'm not sure whether there's any other corner case. > 2. Prefer performance. Just like what oxc does, keep entire class declarations if some static property contains side effect initializer. > > Since it's anti-pattern code, I'm not sure above options are worthy just for this case...Maybe a better one is emitting warnings for all those anti-pattern code. However, swc minifier doesn't diagnostic system for minifier, and I don't know how many code in swc minifier is used for handling such cases (so it could be redundant to introduce it now). After some consideration I finally decide to directly add `contains_this_expr` check for it. **Related issue:** - Closes #10981
1 parent 0088ab8 commit 18aeac6

File tree

5 files changed

+86
-38
lines changed

5 files changed

+86
-38
lines changed

crates/swc_ecma_minifier/src/compress/optimize/mod.rs

Lines changed: 13 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -668,27 +668,32 @@ impl Optimizer<'_> {
668668
return Some(cls.take().into());
669669
}
670670

671+
let Some(side_effects) =
672+
extract_class_side_effect(self.ctx.expr_ctx, &mut cls.class)
673+
else {
674+
return Some(cls.take().into());
675+
};
676+
671677
report_change!(
672678
"ignore_return_value: Dropping unused class expr as it does not have any side \
673679
effect"
674680
);
675681
self.changed = true;
676682

677-
let exprs: Vec<Box<Expr>> =
678-
extract_class_side_effect(self.ctx.expr_ctx, *cls.class.take())
679-
.into_iter()
680-
.filter_map(|mut e| self.ignore_return_value(&mut e))
681-
.map(Box::new)
682-
.collect();
683+
let side_effects: Vec<Box<Expr>> = side_effects
684+
.into_iter()
685+
.filter_map(|e| self.ignore_return_value(e))
686+
.map(Box::new)
687+
.collect();
683688

684-
if exprs.is_empty() {
689+
if side_effects.is_empty() {
685690
return None;
686691
}
687692

688693
return Some(
689694
SeqExpr {
690695
span: cls.class.span,
691-
exprs,
696+
exprs: side_effects,
692697
}
693698
.into(),
694699
);

crates/swc_ecma_minifier/src/compress/optimize/unused.rs

Lines changed: 30 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -521,34 +521,43 @@ impl Optimizer<'_> {
521521
.map(|v| v.usage_count == 0 && v.property_mutation_count == 0)
522522
.unwrap_or(false)
523523
{
524+
let class = decl.as_mut_class().unwrap();
525+
let Some(side_effects) =
526+
extract_class_side_effect(self.ctx.expr_ctx, &mut class.class)
527+
else {
528+
return;
529+
};
530+
524531
self.changed = true;
525532
report_change!(
526533
"unused: Dropping a decl '{}{:?}' because it is not used",
527534
ident.sym,
528535
ident.ctxt
529536
);
530-
// This will remove the declaration.
531-
let class = decl.take().class().unwrap();
532-
let mut side_effects =
533-
extract_class_side_effect(self.ctx.expr_ctx, *class.class);
534-
535-
if !side_effects.is_empty() {
536-
self.prepend_stmts.push(
537-
ExprStmt {
538-
span: DUMMY_SP,
539-
expr: if side_effects.len() > 1 {
540-
SeqExpr {
541-
span: DUMMY_SP,
542-
exprs: side_effects,
543-
}
544-
.into()
545-
} else {
546-
side_effects.remove(0)
547-
},
548-
}
549-
.into(),
550-
)
537+
538+
let mut side_effects: Vec<_> =
539+
side_effects.into_iter().map(|expr| expr.take()).collect();
540+
decl.take();
541+
542+
if side_effects.is_empty() {
543+
return;
551544
}
545+
546+
self.prepend_stmts.push(
547+
ExprStmt {
548+
span: DUMMY_SP,
549+
expr: if side_effects.len() > 1 {
550+
SeqExpr {
551+
span: DUMMY_SP,
552+
exprs: side_effects,
553+
}
554+
.into()
555+
} else {
556+
side_effects.remove(0)
557+
},
558+
}
559+
.into(),
560+
);
552561
}
553562
}
554563
Decl::Fn(FnDecl { ident, .. }) => {

crates/swc_ecma_minifier/src/compress/optimize/util.rs

Lines changed: 18 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@ use swc_atoms::Atom;
88
use swc_common::{util::take::Take, Mark, SyntaxContext, DUMMY_SP};
99
use swc_ecma_ast::*;
1010
use swc_ecma_transforms_base::perf::{Parallel, ParallelExt};
11-
use swc_ecma_utils::{collect_decls, ExprCtx, ExprExt, Remapper};
11+
use swc_ecma_utils::{collect_decls, contains_this_expr, ExprCtx, ExprExt, Remapper};
1212
use swc_ecma_visit::{noop_visit_mut_type, VisitMut, VisitMutWith};
1313
use tracing::debug;
1414

@@ -161,34 +161,40 @@ impl Drop for WithCtx<'_, '_> {
161161
}
162162
}
163163

164-
pub(crate) fn extract_class_side_effect(expr_ctx: ExprCtx, c: Class) -> Vec<Box<Expr>> {
164+
pub(crate) fn extract_class_side_effect(
165+
expr_ctx: ExprCtx,
166+
c: &mut Class,
167+
) -> Option<Vec<&mut Box<Expr>>> {
165168
let mut res = Vec::new();
166-
if let Some(e) = c.super_class {
169+
if let Some(e) = &mut c.super_class {
167170
if e.may_have_side_effects(expr_ctx) {
168171
res.push(e);
169172
}
170173
}
171174

172-
for m in c.body {
175+
for m in &mut c.body {
173176
match m {
174177
ClassMember::Method(ClassMethod {
175178
key: PropName::Computed(key),
176179
..
177180
}) => {
178181
if key.expr.may_have_side_effects(expr_ctx) {
179-
res.push(key.expr);
182+
res.push(&mut key.expr);
180183
}
181184
}
182185

183186
ClassMember::ClassProp(p) => {
184-
if let PropName::Computed(key) = p.key {
187+
if let PropName::Computed(key) = &mut p.key {
185188
if key.expr.may_have_side_effects(expr_ctx) {
186-
res.push(key.expr);
189+
res.push(&mut key.expr);
187190
}
188191
}
189192

190-
if let Some(v) = p.value {
193+
if let Some(v) = &mut p.value {
191194
if p.is_static && v.may_have_side_effects(expr_ctx) {
195+
if contains_this_expr(v) {
196+
return None;
197+
}
192198
res.push(v);
193199
}
194200
}
@@ -199,6 +205,9 @@ pub(crate) fn extract_class_side_effect(expr_ctx: ExprCtx, c: Class) -> Vec<Box<
199205
..
200206
}) => {
201207
if v.may_have_side_effects(expr_ctx) {
208+
if contains_this_expr(v) {
209+
return None;
210+
}
202211
res.push(v);
203212
}
204213
}
@@ -207,7 +216,7 @@ pub(crate) fn extract_class_side_effect(expr_ctx: ExprCtx, c: Class) -> Vec<Box<
207216
}
208217
}
209218

210-
res
219+
Some(res)
211220
}
212221

213222
pub(crate) fn is_valid_for_lhs(e: &Expr) -> bool {
Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
1+
class C {
2+
static foo = bar;
3+
}
4+
5+
(class C {
6+
static foo = bar;
7+
});
8+
9+
class D {
10+
static #_ = (this.FOO = {});
11+
}
12+
13+
(class D {
14+
static #_ = (this.FOO = {});
15+
});
Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
class C {
2+
static foo = bar;
3+
}
4+
bar;
5+
class D {
6+
static #_ = this.FOO = {};
7+
}
8+
(class {
9+
static #_ = this.FOO = {};
10+
});

0 commit comments

Comments
 (0)