Skip to content

Commit

Permalink
fix(es/minifier): Prevent removing side effects from accessing getter (
Browse files Browse the repository at this point in the history
…#9530)

Closes #9500

Caused by
https://github.com/swc-project/swc/blob/c7fdd6b69b129a11465125d4e11a898326b7e884/crates/swc_ecma_minifier/src/compress/pure/misc.rs#L1547.
When the object with getters pass to `self.ignore_return_value`,

https://github.com/swc-project/swc/blob/c7fdd6b69b129a11465125d4e11a898326b7e884/crates/swc_ecma_minifier/src/compress/pure/misc.rs#L966
converts the object to `0` because the object is side-effect-free
according to

https://github.com/swc-project/swc/blob/c7fdd6b69b129a11465125d4e11a898326b7e884/crates/swc_ecma_utils/src/lib.rs#L1496

We should skip this process to fix the issue.

As is known only accessing getters and setters may cause side effect, we
can safely do the transformation when none of them appears in the
object. More precision is possible if comparing the lit prop names. I
also collect computed keys of getters and setters in the object, is
there any bad case?

The reason why only numeric (string) key removes the statement is that
string key (`Computed`) is converted to `Ident` in other phases, e.g.
`{}['a']` => `{}.a`, which does not matching the pattern.
  • Loading branch information
CPunisher committed Sep 4, 2024
1 parent 84b0043 commit 8513816
Show file tree
Hide file tree
Showing 6 changed files with 95 additions and 48 deletions.
6 changes: 6 additions & 0 deletions .changeset/stale-poets-cover.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
swc_core: patch
swc_ecma_minifier: patch
---

fix(es/minifier): prevent removing side effects from accessing getter with numeric string key
116 changes: 68 additions & 48 deletions crates/swc_ecma_minifier/src/compress/pure/misc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,45 @@ fn can_compress_new_regexp(args: Option<&[ExprOrSpread]>) -> bool {
}
}

fn collect_exprs_from_object(obj: &mut ObjectLit) -> Vec<Box<Expr>> {
let mut exprs = Vec::new();

for prop in obj.props.take() {
if let PropOrSpread::Prop(p) = prop {
match *p {
Prop::Shorthand(p) => {
exprs.push(p.into());
}
Prop::KeyValue(p) => {
if let PropName::Computed(e) = p.key {
exprs.push(e.expr);
}

exprs.push(p.value);
}
Prop::Getter(p) => {
if let PropName::Computed(e) = p.key {
exprs.push(e.expr);
}
}
Prop::Setter(p) => {
if let PropName::Computed(e) = p.key {
exprs.push(e.expr);
}
}
Prop::Method(p) => {
if let PropName::Computed(e) = p.key {
exprs.push(e.expr);
}
}
_ => {}
}
}
}

exprs
}

impl Pure<'_> {
/// `foo(...[1, 2])`` => `foo(1, 2)`
pub(super) fn eval_spread_array(&mut self, args: &mut Vec<ExprOrSpread>) {
Expand Down Expand Up @@ -1417,39 +1456,8 @@ impl Pure<'_> {
}

Expr::Object(obj) => {
if obj.props.iter().all(|p| match p {
PropOrSpread::Spread(_) => false,
PropOrSpread::Prop(p) => matches!(
&**p,
Prop::Shorthand(_) | Prop::KeyValue(_) | Prop::Method(..)
),
}) {
let mut exprs = Vec::new();

for prop in obj.props.take() {
if let PropOrSpread::Prop(p) = prop {
match *p {
Prop::Shorthand(p) => {
exprs.push(p.into());
}
Prop::KeyValue(p) => {
if let PropName::Computed(e) = p.key {
exprs.push(e.expr);
}

exprs.push(p.value);
}
Prop::Method(p) => {
if let PropName::Computed(e) = p.key {
exprs.push(e.expr);
}
}

_ => unreachable!(),
}
}
}

if obj.props.iter().all(|prop| !prop.is_spread()) {
let exprs = collect_exprs_from_object(obj);
*e = self
.make_ignored_expr(obj.span, exprs.into_iter())
.unwrap_or(Invalid { span: DUMMY_SP }.into());
Expand Down Expand Up @@ -1542,22 +1550,34 @@ impl Pure<'_> {
obj,
prop: MemberProp::Computed(prop),
..
}) => match &**obj {
Expr::Object(..) | Expr::Array(..) => {
}) => match obj.as_mut() {
Expr::Object(object) => {
// Accessing getters and setters may cause side effect
// More precision is possible if comparing the lit prop names
if object.props.iter().all(|p| match p {
PropOrSpread::Spread(..) => false,
PropOrSpread::Prop(p) => match &**p {
Prop::Getter(..) | Prop::Setter(..) => false,
_ => true,
},
}) {
let mut exprs = collect_exprs_from_object(object);
exprs.push(prop.expr.take());
*e = self
.make_ignored_expr(*span, exprs.into_iter())
.unwrap_or(Invalid { span: DUMMY_SP }.into());
return;
}
}
Expr::Array(..) => {
self.ignore_return_value(obj, opts);

match &**obj {
Expr::Object(..) => {}
_ => {
*e = self
.make_ignored_expr(
*span,
vec![obj.take(), prop.expr.take()].into_iter(),
)
.unwrap_or(Invalid { span: DUMMY_SP }.into());
return;
}
};
*e = self
.make_ignored_expr(
*span,
vec![obj.take(), prop.expr.take()].into_iter(),
)
.unwrap_or(Invalid { span: DUMMY_SP }.into());
return;
}
_ => {}
},
Expand Down
10 changes: 10 additions & 0 deletions crates/swc_ecma_minifier/tests/fixture/issues/9500/input.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
let foo = 1;
const obj = {
get 1() {
// same with get "1"()
foo = 2;
return 40;
},
};
obj["1"]; // same with obj[1]
console.log(foo);
7 changes: 7 additions & 0 deletions crates/swc_ecma_minifier/tests/fixture/issues/9500/output.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
let foo = 1;
({
get 1 () {
return(// same with get "1"()
foo = 2, 40);
}
})["1"], console.log(foo);
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
({
a: 1,
})[undetermined()];
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
undetermined();

0 comments on commit 8513816

Please sign in to comment.