From 8513816139c6ceef12a906b03c1bcf9471ce0b07 Mon Sep 17 00:00:00 2001 From: CPunisher <1343316114@qq.com> Date: Wed, 4 Sep 2024 20:28:46 +0800 Subject: [PATCH] fix(es/minifier): Prevent removing side effects from accessing getter (#9530) Closes https://github.com/swc-project/swc/issues/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. --- .changeset/stale-poets-cover.md | 6 + .../src/compress/pure/misc.rs | 116 ++++++++++-------- .../tests/fixture/issues/9500/input.js | 10 ++ .../tests/fixture/issues/9500/output.js | 7 ++ .../member_expr/undetermined_prop/input.js | 3 + .../member_expr/undetermined_prop/output.js | 1 + 6 files changed, 95 insertions(+), 48 deletions(-) create mode 100644 .changeset/stale-poets-cover.md create mode 100644 crates/swc_ecma_minifier/tests/fixture/issues/9500/input.js create mode 100644 crates/swc_ecma_minifier/tests/fixture/issues/9500/output.js create mode 100644 crates/swc_ecma_minifier/tests/fixture/member_expr/undetermined_prop/input.js create mode 100644 crates/swc_ecma_minifier/tests/fixture/member_expr/undetermined_prop/output.js diff --git a/.changeset/stale-poets-cover.md b/.changeset/stale-poets-cover.md new file mode 100644 index 000000000000..7b43a6c0ee68 --- /dev/null +++ b/.changeset/stale-poets-cover.md @@ -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 diff --git a/crates/swc_ecma_minifier/src/compress/pure/misc.rs b/crates/swc_ecma_minifier/src/compress/pure/misc.rs index 939727eb83fe..52464831f707 100644 --- a/crates/swc_ecma_minifier/src/compress/pure/misc.rs +++ b/crates/swc_ecma_minifier/src/compress/pure/misc.rs @@ -56,6 +56,45 @@ fn can_compress_new_regexp(args: Option<&[ExprOrSpread]>) -> bool { } } +fn collect_exprs_from_object(obj: &mut ObjectLit) -> Vec> { + 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) { @@ -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()); @@ -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; } _ => {} }, diff --git a/crates/swc_ecma_minifier/tests/fixture/issues/9500/input.js b/crates/swc_ecma_minifier/tests/fixture/issues/9500/input.js new file mode 100644 index 000000000000..dab3112ce7fb --- /dev/null +++ b/crates/swc_ecma_minifier/tests/fixture/issues/9500/input.js @@ -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); diff --git a/crates/swc_ecma_minifier/tests/fixture/issues/9500/output.js b/crates/swc_ecma_minifier/tests/fixture/issues/9500/output.js new file mode 100644 index 000000000000..de9bf726e8cb --- /dev/null +++ b/crates/swc_ecma_minifier/tests/fixture/issues/9500/output.js @@ -0,0 +1,7 @@ +let foo = 1; +({ + get 1 () { + return(// same with get "1"() + foo = 2, 40); + } +})["1"], console.log(foo); diff --git a/crates/swc_ecma_minifier/tests/fixture/member_expr/undetermined_prop/input.js b/crates/swc_ecma_minifier/tests/fixture/member_expr/undetermined_prop/input.js new file mode 100644 index 000000000000..4272f39a61e1 --- /dev/null +++ b/crates/swc_ecma_minifier/tests/fixture/member_expr/undetermined_prop/input.js @@ -0,0 +1,3 @@ +({ + a: 1, +})[undetermined()]; diff --git a/crates/swc_ecma_minifier/tests/fixture/member_expr/undetermined_prop/output.js b/crates/swc_ecma_minifier/tests/fixture/member_expr/undetermined_prop/output.js new file mode 100644 index 000000000000..4b66a145cfc4 --- /dev/null +++ b/crates/swc_ecma_minifier/tests/fixture/member_expr/undetermined_prop/output.js @@ -0,0 +1 @@ +undetermined();