From 1c48a8c8f4ede63661c663cb548345a417c63cf6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Donny/=EA=B0=95=EB=8F=99=EC=9C=A4?= Date: Thu, 26 May 2022 16:37:34 +0900 Subject: [PATCH] fix(es/minifier): Consider side effects while removing an object spread (#4788) --- .github/workflows/cargo.yml | 2 +- .../4788/define-properties/1/exec.js | 29 +++++ .../4788/define-properties/2/exec.js | 30 +++++ .../4788/define-properties/3/exec.js | 30 +++++ .../4788/define-properties/4/exec.js | 31 +++++ .../exec/issues-4xxx/4788/inline/1/exec.js | 30 +++++ .../exec/issues-4xxx/4788/inline/2/exec.js | 20 ++++ .../exec/issues-4xxx/4788/inline/3/exec.js | 4 + .../exec/issues-4xxx/4788/inline/4/exec.js | 12 ++ .../exec/issues-4xxx/4788/inline/5/exec.js | 12 ++ .../src/compress/optimize/mod.rs | 4 +- .../optimize/{hoist_props.rs => props.rs} | 113 +++++++++++++++++- .../src/compress/pure/ctx.rs | 1 + .../src/compress/pure/mod.rs | 2 - .../src/compress/pure/properties.rs | 98 +-------------- crates/swc_ecma_minifier/tests/TODO.txt | 1 - crates/swc_ecma_minifier/tests/exec.rs | 30 +++++ crates/swc_ecma_minifier/tests/golden.txt | 1 + 18 files changed, 345 insertions(+), 105 deletions(-) create mode 100644 crates/swc/tests/exec/issues-4xxx/4788/define-properties/1/exec.js create mode 100644 crates/swc/tests/exec/issues-4xxx/4788/define-properties/2/exec.js create mode 100644 crates/swc/tests/exec/issues-4xxx/4788/define-properties/3/exec.js create mode 100644 crates/swc/tests/exec/issues-4xxx/4788/define-properties/4/exec.js create mode 100644 crates/swc/tests/exec/issues-4xxx/4788/inline/1/exec.js create mode 100644 crates/swc/tests/exec/issues-4xxx/4788/inline/2/exec.js create mode 100644 crates/swc/tests/exec/issues-4xxx/4788/inline/3/exec.js create mode 100644 crates/swc/tests/exec/issues-4xxx/4788/inline/4/exec.js create mode 100644 crates/swc/tests/exec/issues-4xxx/4788/inline/5/exec.js rename crates/swc_ecma_minifier/src/compress/optimize/{hoist_props.rs => props.rs} (67%) diff --git a/.github/workflows/cargo.yml b/.github/workflows/cargo.yml index aeba212726e..85f31131239 100644 --- a/.github/workflows/cargo.yml +++ b/.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: | diff --git a/crates/swc/tests/exec/issues-4xxx/4788/define-properties/1/exec.js b/crates/swc/tests/exec/issues-4xxx/4788/define-properties/1/exec.js new file mode 100644 index 00000000000..879819008d4 --- /dev/null +++ b/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; diff --git a/crates/swc/tests/exec/issues-4xxx/4788/define-properties/2/exec.js b/crates/swc/tests/exec/issues-4xxx/4788/define-properties/2/exec.js new file mode 100644 index 00000000000..0ff44685dcf --- /dev/null +++ b/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; diff --git a/crates/swc/tests/exec/issues-4xxx/4788/define-properties/3/exec.js b/crates/swc/tests/exec/issues-4xxx/4788/define-properties/3/exec.js new file mode 100644 index 00000000000..a904d856706 --- /dev/null +++ b/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; diff --git a/crates/swc/tests/exec/issues-4xxx/4788/define-properties/4/exec.js b/crates/swc/tests/exec/issues-4xxx/4788/define-properties/4/exec.js new file mode 100644 index 00000000000..7a3bc82f57b --- /dev/null +++ b/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; diff --git a/crates/swc/tests/exec/issues-4xxx/4788/inline/1/exec.js b/crates/swc/tests/exec/issues-4xxx/4788/inline/1/exec.js new file mode 100644 index 00000000000..bbad09b4c4c --- /dev/null +++ b/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; diff --git a/crates/swc/tests/exec/issues-4xxx/4788/inline/2/exec.js b/crates/swc/tests/exec/issues-4xxx/4788/inline/2/exec.js new file mode 100644 index 00000000000..52ffb12c830 --- /dev/null +++ b/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, +}); diff --git a/crates/swc/tests/exec/issues-4xxx/4788/inline/3/exec.js b/crates/swc/tests/exec/issues-4xxx/4788/inline/3/exec.js new file mode 100644 index 00000000000..53a5ef2dbfb --- /dev/null +++ b/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); diff --git a/crates/swc/tests/exec/issues-4xxx/4788/inline/4/exec.js b/crates/swc/tests/exec/issues-4xxx/4788/inline/4/exec.js new file mode 100644 index 00000000000..65bc734bb99 --- /dev/null +++ b/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); diff --git a/crates/swc/tests/exec/issues-4xxx/4788/inline/5/exec.js b/crates/swc/tests/exec/issues-4xxx/4788/inline/5/exec.js new file mode 100644 index 00000000000..3d98922498a --- /dev/null +++ b/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); diff --git a/crates/swc_ecma_minifier/src/compress/optimize/mod.rs b/crates/swc_ecma_minifier/src/compress/optimize/mod.rs index 7bcaa200529..862893bb600 100644 --- a/crates/swc_ecma_minifier/src/compress/optimize/mod.rs +++ b/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 { diff --git a/crates/swc_ecma_minifier/src/compress/optimize/hoist_props.rs b/crates/swc_ecma_minifier/src/compress/optimize/props.rs similarity index 67% rename from crates/swc_ecma_minifier/src/compress/optimize/hoist_props.rs rename to crates/swc_ecma_minifier/src/compress/optimize/props.rs index 042013705ca..b5a8a8ab35a 100644 --- a/crates/swc_ecma_minifier/src/compress/optimize/hoist_props.rs +++ b/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 Optimizer<'_, M> @@ -219,3 +219,110 @@ where } } } + +impl 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(_) => {} + }, + } + } + } +} diff --git a/crates/swc_ecma_minifier/src/compress/pure/ctx.rs b/crates/swc_ecma_minifier/src/compress/pure/ctx.rs index 1e83408b570..4b97c027cdd 100644 --- a/crates/swc_ecma_minifier/src/compress/pure/ctx.rs +++ b/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, diff --git a/crates/swc_ecma_minifier/src/compress/pure/mod.rs b/crates/swc_ecma_minifier/src/compress/pure/mod.rs index 19ae8f987b9..c70ab33344b 100644 --- a/crates/swc_ecma_minifier/src/compress/pure/mod.rs +++ b/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); diff --git a/crates/swc_ecma_minifier/src/compress/pure/properties.rs b/crates/swc_ecma_minifier/src/compress/pure/properties.rs index 498c4eaae66..67f6b386d73 100644 --- a/crates/swc_ecma_minifier/src/compress/pure/properties.rs +++ b/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, diff --git a/crates/swc_ecma_minifier/tests/TODO.txt b/crates/swc_ecma_minifier/tests/TODO.txt index 8974118fbf6..8d24a562c42 100644 --- a/crates/swc_ecma_minifier/tests/TODO.txt +++ b/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 diff --git a/crates/swc_ecma_minifier/tests/exec.rs b/crates/swc_ecma_minifier/tests/exec.rs index a4d4fc43a6d..6da0d6fe027 100644 --- a/crates/swc_ecma_minifier/tests/exec.rs +++ b/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); +} diff --git a/crates/swc_ecma_minifier/tests/golden.txt b/crates/swc_ecma_minifier/tests/golden.txt index edec6eb7216..98b10f0165a 100644 --- a/crates/swc_ecma_minifier/tests/golden.txt +++ b/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