fix(es/minifier): take ident for ObjectPatProp::Assign with `value: Some(_)` #9465

wjw99830 · 2024-08-20T07:50:52Z

Description:
In order to remove unused variables declared by object destructuring with default value, call take_ident_of_pat_if_unused for ObjectPatProp::Assign whose value match Some(_)

Related issue:

Closes Cannot remove unused variables declared by object destructuring with default value #9460

changeset-bot · 2024-08-20T07:50:57Z

⚠️ No Changeset found

Latest commit: 717bec5

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

CLAassistant · 2024-08-20T07:50:59Z

All committers have signed the CLA.

magic-akari · 2024-08-20T08:05:48Z

crates/swc_ecma_minifier/tests/fixture/issues/9460/input.js

@@ -0,0 +1,3 @@
+const a = 1;
+const { b = "b", c, d } = {};


Even with pure_getters, the computation of the default value for assignment may still have side effects.
For example:

const { b = console.log("b"), c, d } = {};

@Austaras What do you think?

ignore_return_value will check if the expr is side-effect-free.

ignore_return_value will check if the expr is side-effect-free.

It seems that take_ident_of_pat_if_unused will return before calling ignore_return_value, so that the expression will always be removed if unused.

ignore_return_value will check if the expr is side-effect-free.

No. There're multiple places that would call take_pat_if_unused like drop_unused_arrow_params

Maybe just check that assign value is side effect free using may_have_side_effects?

may_have_side_effects

I tried it. But expressions with pure annotation cannot be recognized so that there're still side-effect-free expressions in output code.

Input code

const { a = /*#__PURE__*/ sideEffectFree(), b = "b" } = {};

Output code

const { a = /*#__PURE__*/ sideEffectFree() } = {};

See the latest commit, expected output is

const { a = sideEffect(), d } = {}; console.log(d);

but got

const { a = /*#__PURE__*/ sideEffectFree(sideEffect()), d } = {}; console.log(d);

is_pure_callee doesn't consume the pure annotation.

It's intentional, and if we want to drop it we can invoke ignore_return_value from the method.

It seems that ignore_return_value doesn't consume pure either. Its return expr is same as input expr.

…estructuring with default value

codspeed-hq · 2024-08-20T09:36:09Z

CodSpeed Performance Report

Merging #9465 will not alter performance

_{Comparing wjw99830:hotfix/issue_9460 (717bec5) with main (b0b5e36)}

Summary

✅ 178 untouched benchmarks

… side-effect-free

kdy1

Can we extract all side effects to somewhere, and drop useless bindings instead of checking for side-effect-free from here?

It's how other passes work.

kdy1 · 2024-08-21T06:43:07Z

I made #9478

wjw99830 · 2024-08-21T07:40:34Z

drop useless bindings

Assign of object pattern only run when the binding's value is undefined. If we drop the binding, the expr will always run.

wjw99830 · 2024-08-21T08:20:05Z

Maybe we could invoke has_mark from ignore_return_value?
It's sure that the return value of expr is unused when calling ignore_return_value, and we need the assignment indeed.

kdy1 · 2024-08-21T08:39:06Z

Yeap it would be fine.

…ix/issue_9460

wjw99830 · 2024-08-21T09:53:50Z

Should we apply the optimize of take_pat_if_unused for unpredictable expression when pure_getters is true?

export function Component(props) {
  const { a, b } = props;
}

pure_getters means that all unpredictable objects are pure.

kdy1 · 2024-08-21T10:34:24Z

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

@@ -164,6 +164,53 @@ impl Optimizer<'_> {
        params.retain(|p| !p.is_invalid());
    }

+    #[cfg_attr(feature = "debug", tracing::instrument(skip_all))]
+    fn take_ident_of_obj_pat_if_unused_and_side_effect_free(


I think this is unnecessarily complex.

See https://github.com/swc-project/swc/pull/9478/files#diff-0ff76c3ab1246166f0a7132a4da5346082038c9dd821ab81d95343aa31510aa0R399-R408

OK.
This pr could be closed and then follow #9478.

**Related issue:** - Closes #9459 - Closes #9460 - Closes #9465

wjw99830 requested a review from a team as a code owner August 20, 2024 07:50

kdy1 requested review from magic-akari and Austaras August 20, 2024 07:57

magic-akari reviewed Aug 20, 2024

View reviewed changes

fix(es/minifier): Cannot remove unused variables declared by object d…

9711739

…estructuring with default value

wjw99830 force-pushed the hotfix/issue_9460 branch from c1c126c to 9711739 Compare August 20, 2024 08:07

kdy1 added this to the Planned milestone Aug 20, 2024

wjw99830 and others added 2 commits August 20, 2024 18:45

feat(es/minifier): use may_have_side_effects to check assign value is…

e1f8d62

… side-effect-free

Merge branch 'main' into hotfix/issue_9460

c74ef0d

kdy1 requested changes Aug 21, 2024

View reviewed changes

kdy1 mentioned this pull request Aug 21, 2024

feat(es/minifier): Drop more patterns with PURE marker #9478

Merged

wjw99830 added 2 commits August 21, 2024 17:14

feat(es/minifier): use ignore_return_value instead

a92c84d

Merge branch 'hotfix/issue_9460' of github.com:wjw99830/swc into hotf…

717bec5

…ix/issue_9460

wjw99830 requested a review from kdy1 August 21, 2024 10:04

kdy1 reviewed Aug 21, 2024

View reviewed changes

wjw99830 closed this Aug 21, 2024

kdy1 added a commit that referenced this pull request Aug 22, 2024

feat(es/minifier): Drop more patterns with PURE marker (#9478)

ede1a52

**Related issue:** - Closes #9459 - Closes #9460 - Closes #9465

kdy1 modified the milestones: Planned, v1.7.17 Aug 23, 2024

swc-project locked as resolved and limited conversation to collaborators Sep 23, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(es/minifier): take ident for ObjectPatProp::Assign with `value: Some(_)` #9465

fix(es/minifier): take ident for ObjectPatProp::Assign with `value: Some(_)` #9465

wjw99830 commented Aug 20, 2024 •

edited by kdy1

Loading

changeset-bot bot commented Aug 20, 2024 •

edited

Loading

CLAassistant commented Aug 20, 2024 •

edited

Loading

magic-akari Aug 20, 2024

wjw99830 Aug 20, 2024

wjw99830 Aug 20, 2024

Austaras Aug 20, 2024 •

edited

Loading

Austaras Aug 20, 2024 •

edited

Loading

wjw99830 Aug 20, 2024

wjw99830 Aug 20, 2024

kdy1 Aug 20, 2024

wjw99830 Aug 21, 2024 •

edited

Loading

codspeed-hq bot commented Aug 20, 2024 •

edited

Loading

kdy1 left a comment

kdy1 commented Aug 21, 2024

wjw99830 commented Aug 21, 2024

wjw99830 commented Aug 21, 2024

kdy1 commented Aug 21, 2024

wjw99830 commented Aug 21, 2024 •

edited

Loading

kdy1 Aug 21, 2024

wjw99830 Aug 21, 2024

fix(es/minifier): take ident for ObjectPatProp::Assign with value: Some(_) #9465

fix(es/minifier): take ident for ObjectPatProp::Assign with value: Some(_) #9465

Conversation

wjw99830 commented Aug 20, 2024 • edited by kdy1 Loading

changeset-bot bot commented Aug 20, 2024 • edited Loading

⚠️ No Changeset found

CLAassistant commented Aug 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Austaras Aug 20, 2024 • edited Loading

Choose a reason for hiding this comment

Austaras Aug 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wjw99830 Aug 21, 2024 • edited Loading

Choose a reason for hiding this comment

codspeed-hq bot commented Aug 20, 2024 • edited Loading

Merging #9465 will not alter performance

Summary

kdy1 left a comment

Choose a reason for hiding this comment

kdy1 commented Aug 21, 2024

wjw99830 commented Aug 21, 2024

wjw99830 commented Aug 21, 2024

kdy1 commented Aug 21, 2024

wjw99830 commented Aug 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fix(es/minifier): take ident for ObjectPatProp::Assign with `value: Some(_)` #9465

fix(es/minifier): take ident for ObjectPatProp::Assign with `value: Some(_)` #9465

wjw99830 commented Aug 20, 2024 •

edited by kdy1

Loading

changeset-bot bot commented Aug 20, 2024 •

edited

Loading

CLAassistant commented Aug 20, 2024 •

edited

Loading

Austaras Aug 20, 2024 •

edited

Loading

Austaras Aug 20, 2024 •

edited

Loading

wjw99830 Aug 21, 2024 •

edited

Loading

codspeed-hq bot commented Aug 20, 2024 •

edited

Loading

wjw99830 commented Aug 21, 2024 •

edited

Loading