fix(es/minifier): Consider side effects while removing an object spread (#4788)

.github/workflows/cargo.yml

@@ -390,7 +390,7 @@ jobs:
             .swc-exec-cache
           key: swc-exec-cache-${{ runner.os }}-${{ matrix.settings.crate }}-${{ hashFiles('**/Cargo.lock') }}
           restore-keys: |
-            swc-exec-cache-${{ runner.os }}-${{ matrix.settings.crate }}
+            swc-exec-cache-${{ matrix.settings.crate }}-${{ runner.os }}
 
       - name: Run cargo test
         run: |

crates/swc/tests/exec/issues-4xxx/4788/define-properties/1/exec.js

@@ -0,0 +1,29 @@
+let id = 0;
+
+const obj = {};
+Object.defineProperty(obj, "foo", {
+    get: () => console.log("foo", id++),
+});
+
+obj.foo;
+
+const obj2 = {
+    ...obj,
+};
+
+obj2.foo;
+
+const obj3 = {
+    ...obj,
+    foo: 1,
+};
+
+obj3.foo;
+
+const obj4 = {
+    foo: 2,
+    ...obj,
+    foo: 1,
+};
+
+obj4.foo;

crates/swc/tests/exec/issues-4xxx/4788/define-properties/2/exec.js

@@ -0,0 +1,30 @@
+let id = 0;
+
+const obj = {};
+Object.defineProperty(obj, "foo", {
+    get: () => console.log("foo", id++),
+    enumerable: false,
+});
+
+obj.foo;
+
+const obj2 = {
+    ...obj,
+};
+
+obj2.foo;
+
+const obj3 = {
+    ...obj,
+    foo: 1,
+};
+
+obj3.foo;
+
+const obj4 = {
+    foo: 2,
+    ...obj,
+    foo: 1,
+};
+
+obj4.foo;

crates/swc/tests/exec/issues-4xxx/4788/define-properties/3/exec.js

@@ -0,0 +1,30 @@
+let id = 0;
+
+const obj = {};
+Object.defineProperty(obj, "foo", {
+    get: () => console.log("foo", id++),
+    configurable: false,
+});
+
+obj.foo;
+
+const obj2 = {
+    ...obj,
+};
+
+obj2.foo;
+
+const obj3 = {
+    ...obj,
+    foo: 1,
+};
+
+obj3.foo;
+
+const obj4 = {
+    foo: 2,
+    ...obj,
+    foo: 1,
+};
+
+obj4.foo;

crates/swc/tests/exec/issues-4xxx/4788/define-properties/4/exec.js

@@ -0,0 +1,31 @@
+let id = 0;
+
+const obj = {};
+Object.defineProperty(obj, "foo", {
+    get: () => console.log("foo", id++),
+    enumerable: false,
+    configurable: false,
+});
+
+obj.foo;
+
+const obj2 = {
+    ...obj,
+};
+
+obj2.foo;
+
+const obj3 = {
+    ...obj,
+    foo: 1,
+};
+
+obj3.foo;
+
+const obj4 = {
+    foo: 2,
+    ...obj,
+    foo: 1,
+};
+
+obj4.foo;

crates/swc/tests/exec/issues-4xxx/4788/inline/1/exec.js

@@ -0,0 +1,30 @@
+let id = 0;
+
+const obj = {
+    get foo() {
+        console.log("foo", id++);
+    },
+};
+
+obj.foo;
+
+const obj2 = {
+    ...obj,
+};
+
+obj2.foo;
+
+const obj3 = {
+    ...obj,
+    foo: 1,
+};
+
+obj3.foo;
+
+const obj4 = {
+    foo: 2,
+    ...obj,
+    foo: 1,
+};
+
+obj4.foo;

crates/swc/tests/exec/issues-4xxx/4788/inline/2/exec.js

@@ -0,0 +1,20 @@
+let id = 0;
+
+const obj = {
+    foo: 1,
+    bar: 2,
+    baz: 3,
+};
+
+console.log({
+    ...obj,
+    ...obj,
+    foo: 10,
+});
+
+console.log({
+    ...obj,
+    foo: 10,
+    ...obj,
+    bar: 120,
+});

crates/swc/tests/exec/issues-4xxx/4788/inline/3/exec.js

@@ -0,0 +1,4 @@
+let { x, y, ...z } = { x: 1, y: 2, a: 3, b: 4 };
+console.log(x);
+console.log(y);
+console.log(z);

crates/swc/tests/exec/issues-4xxx/4788/inline/4/exec.js

@@ -0,0 +1,12 @@
+let id = 0;
+
+const obj = {
+    get x() {
+        console.log("x", id++);
+    },
+};
+
+let { x, y, ...z } = { ...obj, x: 1, y: 2, a: 3, b: 4 };
+console.log(x);
+console.log(y);
+console.log(z);

crates/swc/tests/exec/issues-4xxx/4788/inline/5/exec.js

@@ -0,0 +1,12 @@
+let id = 0;
+
+const obj = {
+    get x() {
+        console.log("x", id++);
+    },
+};
+
+let { x, y, ...z } = { x: 1, y: 2, a: 3, b: 4, ...obj };
+console.log(x);
+console.log(y);
+console.log(z);

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

@@ -42,12 +42,12 @@ mod conditionals;
 mod dead_code;
 mod evaluate;
 mod fns;
-mod hoist_props;
 mod if_return;
 mod iife;
 mod inline;
 mod loops;
 mod ops;
+mod props;
 mod sequences;
 mod strings;
 mod switches;
@@ -1742,6 +1742,8 @@ where
 
         self.inline(e);
 
+        self.handle_property_access(e);
+
         if let Expr::Bin(bin) = e {
             let expr = self.optimize_lit_cmp(bin);
             if let Some(expr) = expr {

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

@@ -1,9 +1,9 @@
 use swc_common::DUMMY_SP;
 use swc_ecma_ast::*;
-use swc_ecma_utils::contains_this_expr;
+use swc_ecma_utils::{contains_this_expr, prop_name_eq, ExprExt};
 
-use super::Optimizer;
-use crate::mode::Mode;
+use super::{unused::PropertyAccessOpts, Optimizer};
+use crate::{mode::Mode, util::deeply_contains_this_expr};
 
 /// Methods related to the option `hoist_props`.
 impl<M> Optimizer<'_, M>
@@ -219,3 +219,110 @@ where
         }
     }
 }
+
+impl<M> Optimizer<'_, M>
+where
+    M: Mode,
+{
+    /// Converts `{ a: 1 }.a` into `1`.
+    pub(super) fn handle_property_access(&mut self, e: &mut Expr) {
+        if !self.options.props {
+            return;
+        }
+
+        if self.ctx.is_update_arg {
+            return;
+        }
+
+        if self.ctx.is_callee {
+            return;
+        }
+
+        let me = match e {
+            Expr::Member(m) => m,
+            _ => return,
+        };
+
+        let key = match &me.prop {
+            MemberProp::Ident(prop) => prop,
+            _ => return,
+        };
+
+        let obj = match &mut *me.obj {
+            Expr::Object(o) => o,
+            _ => return,
+        };
+
+        let duplicate_prop = obj
+            .props
+            .iter()
+            .filter(|prop| match prop {
+                PropOrSpread::Spread(_) => false,
+                PropOrSpread::Prop(p) => match &**p {
+                    Prop::Shorthand(p) => p.sym == key.sym,
+                    Prop::KeyValue(p) => prop_name_eq(&p.key, &key.sym),
+                    Prop::Assign(p) => p.key.sym == key.sym,
+                    Prop::Getter(p) => prop_name_eq(&p.key, &key.sym),
+                    Prop::Setter(p) => prop_name_eq(&p.key, &key.sym),
+                    Prop::Method(p) => prop_name_eq(&p.key, &key.sym),
+                },
+            })
+            .count()
+            != 1;
+        if duplicate_prop {
+            return;
+        }
+
+        if obj.props.iter().any(|prop| match prop {
+            PropOrSpread::Spread(s) => self.should_preserve_property_access(
+                &s.expr,
+                PropertyAccessOpts {
+                    allow_getter: false,
+                    only_ident: false,
+                },
+            ),
+            PropOrSpread::Prop(p) => match &**p {
+                Prop::Shorthand(..) => false,
+                Prop::KeyValue(p) => {
+                    p.key.is_computed()
+                        || p.value.may_have_side_effects(&self.expr_ctx)
+                        || deeply_contains_this_expr(&p.value)
+                }
+                Prop::Assign(p) => {
+                    p.value.may_have_side_effects(&self.expr_ctx)
+                        || deeply_contains_this_expr(&p.value)
+                }
+                Prop::Getter(p) => p.key.is_computed(),
+                Prop::Setter(p) => p.key.is_computed(),
+                Prop::Method(p) => p.key.is_computed(),
+            },
+        }) {
+            log_abort!("Property accesses should not be inlined to preserve side effects");
+            return;
+        }
+
+        for prop in &obj.props {
+            match prop {
+                PropOrSpread::Spread(_) => {}
+                PropOrSpread::Prop(p) => match &**p {
+                    Prop::Shorthand(_) => {}
+                    Prop::KeyValue(p) => {
+                        if prop_name_eq(&p.key, &key.sym) {
+                            report_change!(
+                                "properties: Inlining a key-value property `{}`",
+                                key.sym
+                            );
+                            self.changed = true;
+                            *e = *p.value.clone();
+                            return;
+                        }
+                    }
+                    Prop::Assign(_) => {}
+                    Prop::Getter(_) => {}
+                    Prop::Setter(_) => {}
+                    Prop::Method(_) => {}
+                },
+            }
+        }
+    }
+}

crates/swc_ecma_minifier/src/compress/pure/ctx.rs

@@ -11,6 +11,7 @@ pub(super) struct Ctx {
     /// `true` if we are in `arg` of `++arg` or `--arg`.
     pub is_update_arg: bool,
 
+    #[allow(unused)]
     pub is_callee: bool,
 
     pub _in_try_block: bool,

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

@@ -333,8 +333,6 @@ impl VisitMut for Pure<'_> {
 
         self.swap_bin_operands(e);
 
-        self.handle_property_access(e);
-
         self.optimize_bools(e);
 
         self.drop_logical_operands(e);

crates/swc_ecma_minifier/src/compress/pure/properties.rs

@@ -1,10 +1,9 @@
 use swc_atoms::js_word;
 use swc_common::SyntaxContext;
 use swc_ecma_ast::*;
-use swc_ecma_utils::{prop_name_eq, ExprExt};
 
 use super::Pure;
-use crate::{compress::util::is_valid_identifier, util::deeply_contains_this_expr};
+use crate::compress::util::is_valid_identifier;
 
 impl Pure<'_> {
     pub(super) fn optimize_property_of_member_expr(
@@ -97,101 +96,6 @@ impl Pure<'_> {
         }
     }
 
-    /// Converts `{ a: 1 }.a` into `1`.
-    pub(super) fn handle_property_access(&mut self, e: &mut Expr) {
-        if !self.options.props {
-            return;
-        }
-
-        if self.ctx.is_update_arg {
-            return;
-        }
-
-        if self.ctx.is_callee {
-            return;
-        }
-
-        let me = match e {
-            Expr::Member(m) => m,
-            _ => return,
-        };
-
-        let key = match &me.prop {
-            MemberProp::Ident(prop) => prop,
-            _ => return,
-        };
-
-        let obj = match &mut *me.obj {
-            Expr::Object(o) => o,
-            _ => return,
-        };
-
-        let duplicate_prop = obj
-            .props
-            .iter()
-            .filter(|prop| match prop {
-                PropOrSpread::Spread(_) => false,
-                PropOrSpread::Prop(p) => match &**p {
-                    Prop::Shorthand(p) => p.sym == key.sym,
-                    Prop::KeyValue(p) => prop_name_eq(&p.key, &key.sym),
-                    Prop::Assign(p) => p.key.sym == key.sym,
-                    Prop::Getter(p) => prop_name_eq(&p.key, &key.sym),
-                    Prop::Setter(p) => prop_name_eq(&p.key, &key.sym),
-                    Prop::Method(p) => prop_name_eq(&p.key, &key.sym),
-                },
-            })
-            .count()
-            != 1;
-        if duplicate_prop {
-            return;
-        }
-
-        if obj.props.iter().any(|prop| match prop {
-            PropOrSpread::Spread(_) => false,
-            PropOrSpread::Prop(p) => match &**p {
-                Prop::Shorthand(..) => false,
-                Prop::KeyValue(p) => {
-                    p.key.is_computed()
-                        || p.value.may_have_side_effects(&self.expr_ctx)
-                        || deeply_contains_this_expr(&p.value)
-                }
-                Prop::Assign(p) => {
-                    p.value.may_have_side_effects(&self.expr_ctx)
-                        || deeply_contains_this_expr(&p.value)
-                }
-                Prop::Getter(p) => p.key.is_computed(),
-                Prop::Setter(p) => p.key.is_computed(),
-                Prop::Method(p) => p.key.is_computed(),
-            },
-        }) {
-            return;
-        }
-
-        for prop in &obj.props {
-            match prop {
-                PropOrSpread::Spread(_) => {}
-                PropOrSpread::Prop(p) => match &**p {
-                    Prop::Shorthand(_) => {}
-                    Prop::KeyValue(p) => {
-                        if prop_name_eq(&p.key, &key.sym) {
-                            report_change!(
-                                "properties: Inlining a key-value property `{}`",
-                                key.sym
-                            );
-                            self.changed = true;
-                            *e = *p.value.clone();
-                            return;
-                        }
-                    }
-                    Prop::Assign(_) => {}
-                    Prop::Getter(_) => {}
-                    Prop::Setter(_) => {}
-                    Prop::Method(_) => {}
-                },
-            }
-        }
-    }
-
     pub(super) fn handle_known_computed_member_expr(
         &mut self,
         c: &mut ComputedPropName,

crates/swc_ecma_minifier/tests/TODO.txt

@@ -5,7 +5,6 @@ drop_unused/keep_assign/input.js
 drop_unused/reassign_const/input.js
 evaluate/issue_2535_1/input.js
 harmony/array_literal_with_spread_4a/input.js
-logical_assignment/logical_assignment_not_always_happens/input.js
 nullish/conditional_to_nullish_coalescing_2/input.js
 properties/dot_properties/input.js
 properties/issue_2256/input.js

crates/swc_ecma_minifier/tests/exec.rs

@@ -9923,3 +9923,33 @@ fn terser_insane_2() {
 
     run_exec_test(src, config, false);
 }
+
+#[test]
+fn issue_4788_1() {
+    let src = r###"
+    let id = 0;
+
+    const obj = {
+        get foo() {
+            console.log("foo", id++);
+        },
+    };
+
+    obj.foo;
+
+    const obj2 = {
+        ...obj,
+    };
+
+    obj2.foo;
+
+    const obj3 = {
+        ...obj,
+        foo: 1,
+    };
+
+    obj3.foo;
+    "###;
+
+    run_default_exec_test(src);
+}

crates/swc_ecma_minifier/tests/golden.txt

@@ -875,6 +875,7 @@ labels/labels_7/input.js
 labels/labels_8/input.js
 labels/labels_9/input.js
 logical_assignment/assign_in_conditional_part_reused/input.js
+logical_assignment/logical_assignment_not_always_happens/input.js
 logical_assignment/prematurely_evaluate_assignment/input.js
 logical_assignment/prematurely_evaluate_assignment_inv/input.js
 loops/dead_code_condition/input.js