feat(es/minifier): Fine grained effect analysis of class #11814
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,16 +1 @@ | ||
| //// [assignParameterPropertyToPropertyDeclarationES2022.ts] | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,16 +1 @@ | ||
| //// [assignParameterPropertyToPropertyDeclarationESNext.ts] | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,3 +1,2 @@ | ||
| //// [classStaticBlock15.ts] | ||
| console.log(void 0); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,3 +1,2 @@ | ||
| //// [classStaticBlock15.ts] | ||
| console.log(void 0); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,12 +1,2 @@ | ||
| //// [superSymbolIndexedAccess3.ts] | ||
| Symbol.for('myThing'); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,8 +10,8 @@ use swc_ecma_ast::*; | |
| use swc_ecma_transforms_base::rename::contains_eval; | ||
| use swc_ecma_transforms_optimization::debug_assert_valid; | ||
| use swc_ecma_utils::{ | ||
| prepend_stmts, prop_name_from_ident, ExprCtx, ExprExt, ExprFactory, IsEmpty, ModuleItemLike, | ||
| StmtLike, Type, Value, | ||
| }; | ||
| use swc_ecma_visit::{noop_visit_mut_type, VisitMut, VisitMutWith, VisitWith}; | ||
| #[cfg(feature = "debug")] | ||
|
|
@@ -677,26 +677,11 @@ impl Optimizer<'_> { | |
| } | ||
|
|
||
| Expr::Class(cls) => { | ||
| let Some(side_effects) = extract_class_side_effect( | ||
| self.ctx.expr_ctx, | ||
| cls.ident.as_ref(), | ||
| &mut cls.class, | ||
| ) else { | ||
| return Some(cls.take().into()); | ||
| }; | ||
|
|
||
|
|
@@ -1193,7 +1178,11 @@ impl Optimizer<'_> { | |
| }) => { | ||
| report_change!("ignore_return_value: Reducing binary ({})", *op); | ||
|
|
||
| let left = if let Expr::PrivateName(_) = &**left { | ||
| None | ||
| } else { | ||
|
Comment on lines
+1181
to
+1183
There was a problem hiding this comment.
Special-casing Useful? React with 👍 / 👎.
Member
Author
There was a problem hiding this comment. I'd argue this aligns with current behaviour although it is a bug. See terser/terser#1297 |
||
| self.ignore_return_value(left).map(Box::new) | ||
| }; | ||
|
Comment on lines
+1181
to
+1185
|
||
| let right = self.ignore_return_value(right).map(Box::new); | ||
|
|
||
| let mut seq = SeqExpr { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,6 @@ | ||
| use std::{ | ||
| borrow::Borrow, | ||
| mem::{self, take}, | ||
| ops::{Deref, DerefMut}, | ||
| }; | ||
|
|
||
|
|
@@ -10,9 +10,11 @@ use swc_common::{util::take::Take, Mark, SyntaxContext, DUMMY_SP}; | |
| use swc_ecma_ast::*; | ||
| use swc_ecma_transforms_base::perf::{Parallel, ParallelExt}; | ||
| use swc_ecma_utils::{ | ||
| collect_decls, prop_name_from_ident, ExprCtx, ExprExt, IdentUsageFinder, Remapper, | ||
| }; | ||
| use swc_ecma_visit::{ | ||
| noop_visit_mut_type, noop_visit_type, Visit, VisitMut, VisitMutWith, VisitWith, | ||
| }; | ||
| use tracing::debug; | ||
|
|
||
| use super::{Ctx, Optimizer}; | ||
|
|
@@ -164,17 +166,32 @@ impl Drop for WithCtx<'_, '_> { | |
| } | ||
| } | ||
|
|
||
| pub(crate) fn extract_class_side_effect<'a>( | ||
| expr_ctx: ExprCtx, | ||
| ident: Option<&'a Ident>, | ||
| c: &'a mut Class, | ||
| ) -> Option<Vec<&'a mut Box<Expr>>> { | ||
| 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); | ||
| } | ||
| } | ||
|
|
||
| let mut visitor = ClassEffectVisitor { | ||
| found: false, | ||
| private_ident: FxHashSet::default(), | ||
| }; | ||
|
|
||
| for m in &mut c.body { | ||
| if let ClassMember::PrivateProp(PrivateProp { key, .. }) | ||
| | ClassMember::PrivateMethod(PrivateMethod { key, .. }) = m | ||
| { | ||
| visitor.private_ident.insert(key.name.clone()); | ||
| } | ||
| } | ||
|
Comment on lines
174
to
193
There was a problem hiding this comment.
Comment on lines
176
to
193
There was a problem hiding this comment. For |
||
|
|
||
| for m in &mut c.body { | ||
| match m { | ||
| ClassMember::Method(ClassMethod { | ||
|
|
@@ -195,10 +212,18 @@ pub(crate) fn extract_class_side_effect( | |
|
|
||
| if let Some(v) = &mut p.value { | ||
| if p.is_static && v.may_have_side_effects(expr_ctx) { | ||
| v.visit_with(&mut visitor); | ||
| if visitor.found { | ||
|
Austaras marked this conversation as resolved.
|
||
| return None; | ||
| } | ||
|
|
||
| if let Some(id) = ident { | ||
| if IdentUsageFinder::find(id, v) { | ||
| return None; | ||
| } | ||
| } | ||
|
|
||
| value.push(v); | ||
|
Comment on lines
+220
to
+226
There was a problem hiding this comment. The new self-reference guard ( |
||
| } | ||
| } | ||
| } | ||
|
|
@@ -208,20 +233,134 @@ pub(crate) fn extract_class_side_effect( | |
| .. | ||
| }) => { | ||
| if v.may_have_side_effects(expr_ctx) { | ||
| v.visit_with(&mut visitor); | ||
| if visitor.found { | ||
| return None; | ||
| } | ||
|
|
||
| if let Some(id) = ident { | ||
| if IdentUsageFinder::find(id, v) { | ||
| return None; | ||
| } | ||
| } | ||
|
|
||
| value.push(v); | ||
| } | ||
| } | ||
| ClassMember::StaticBlock(s) => { | ||
| if s.body.stmts.len() > 1 { | ||
| return None; | ||
| } | ||
|
|
||
| let first = if let Some(stmt) = s.body.stmts.get_mut(0) { | ||
| &mut stmt.as_mut_expr()?.expr | ||
| } else { | ||
| continue; | ||
| }; | ||
|
|
||
| if first.may_have_side_effects(expr_ctx) { | ||
| first.visit_with(&mut visitor); | ||
| if visitor.found { | ||
| return None; | ||
| } | ||
|
Comment on lines
+261
to
265
There was a problem hiding this comment.
This hoisting path can move static-block expressions out of the class body after only Useful? React with 👍 / 👎. |
||
|
|
||
| if let Some(id) = ident { | ||
| if IdentUsageFinder::find(id, first) { | ||
| return None; | ||
| } | ||
| } | ||
|
|
||
| value.push(first); | ||
|
There was a problem hiding this comment.
Useful? React with 👍 / 👎. |
||
| } | ||
| } | ||
|
|
||
| _ => {} | ||
| } | ||
| } | ||
|
|
||
| res.append(&mut value); | ||
|
|
||
| Some(res) | ||
| } | ||
|
|
||
| struct ClassEffectVisitor { | ||
|
Member
There was a problem hiding this comment. Do we really need a new visitor? Asking because it's too bad for the binary size
Member
Author
There was a problem hiding this comment. We don't have a private name reference visitor yet, so I write one. Maybe there's one in compact, I would check later. |
||
| found: bool, | ||
| private_ident: FxHashSet<Atom>, | ||
| } | ||
|
|
||
| impl Visit for ClassEffectVisitor { | ||
| noop_visit_type!(); | ||
|
|
||
| /// Don't recurse into constructor | ||
| fn visit_constructor(&mut self, _: &Constructor) {} | ||
|
|
||
| /// Don't recurse into fn | ||
| fn visit_fn_decl(&mut self, _: &FnDecl) {} | ||
|
|
||
| /// Don't recurse into fn | ||
| fn visit_fn_expr(&mut self, _: &FnExpr) {} | ||
|
|
||
| /// Don't recurse into fn | ||
| fn visit_function(&mut self, _: &Function) {} | ||
|
|
||
| /// Don't recurse into fn | ||
| fn visit_getter_prop(&mut self, n: &GetterProp) { | ||
| n.key.visit_with(self); | ||
| } | ||
|
|
||
| /// Don't recurse into fn | ||
| fn visit_method_prop(&mut self, n: &MethodProp) { | ||
| n.key.visit_with(self); | ||
| n.function.visit_with(self); | ||
| } | ||
|
|
||
| /// Don't recurse into fn | ||
| fn visit_setter_prop(&mut self, n: &SetterProp) { | ||
| n.key.visit_with(self); | ||
| n.param.visit_with(self); | ||
|
Austaras marked this conversation as resolved.
|
||
| } | ||
|
|
||
| fn visit_this_expr(&mut self, _: &ThisExpr) { | ||
| self.found = true; | ||
| } | ||
|
|
||
| fn visit_prop(&mut self, n: &Prop) { | ||
| n.visit_children_with(self); | ||
|
|
||
| if let Prop::Shorthand(Ident { sym, .. }) = n { | ||
| if &**sym == "arguments" { | ||
| self.found = true; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| fn visit_super(&mut self, _: &Super) { | ||
| self.found = true; | ||
| } | ||
|
|
||
| fn visit_private_name(&mut self, n: &PrivateName) { | ||
| if self.private_ident.contains(&n.name) { | ||
| self.found = true | ||
| } | ||
| } | ||
|
|
||
| fn visit_class(&mut self, n: &Class) { | ||
| let mut new_set = FxHashSet::default(); | ||
|
|
||
| for m in &n.body { | ||
| if let ClassMember::PrivateProp(PrivateProp { key, .. }) | ||
| | ClassMember::PrivateMethod(PrivateMethod { key, .. }) = m | ||
| { | ||
| new_set.insert(key.name.clone()); | ||
| } | ||
| } | ||
|
|
||
| let old_set = mem::replace(&mut self.private_ident, new_set); | ||
| n.visit_children_with(self); | ||
| self.private_ident = old_set; | ||
| } | ||
| } | ||
|
|
||
| pub(crate) fn is_valid_for_lhs(e: &Expr) -> bool { | ||
| !matches!(e, Expr::Lit(..) | Expr::Unary(..)) | ||
| } | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ignore_return_valuenow always delegates class expressions toextract_class_side_effectwithout first guarding self-references from the class name. This makes inputs like(class C { [C] = 1 });or(class C extends C {});droppable, even though evaluating either expression throws a TDZReferenceError. In this path the class name only appears in computed keys/heritage, which are not rejected by the current guard set, so dead-code elimination can silently remove required runtime errors.Useful? React with 👍 / 👎.