Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(es/minifier): Abort sequential inliner on reassigned identifier #6048

Closed
wants to merge 5 commits into from

Conversation

kdy1
Copy link
Member

@kdy1 kdy1 commented Oct 5, 2022

Description:

cc @Austaras
I think we should fix this by improving infection analysis, as regression is way too much

Related issue:

@kdy1 kdy1 added this to the Planned milestone Oct 5, 2022
@kdy1 kdy1 self-assigned this Oct 5, 2022
@Austaras
Copy link
Member

Austaras commented Oct 5, 2022

Seems like many are caused by unresolved idents, maybe we could fix #6049 first?

@kdy1
Copy link
Member Author

kdy1 commented Oct 5, 2022

I'll try

@kdy1
Copy link
Member Author

kdy1 commented Oct 5, 2022

There are still lots of regressions

@Austaras
Copy link
Member

Austaras commented Oct 5, 2022

Oh, I mean we should stop at where callee is reassigned

@kdy1
Copy link
Member Author

kdy1 commented Oct 5, 2022

Ah, thank you!
I did something wrong

@kdy1
Copy link
Member Author

kdy1 commented Oct 5, 2022

It didn't work, and actually we were not skipping callee anyway

@kdy1 kdy1 changed the title fix(es/minifier): Fix sequential inliner fix(es/minifier): Abort sequential inliner on reassigned identifier Oct 5, 2022
@Austaras
Copy link
Member

Austaras commented Oct 5, 2022

Well basically I thought it should be

// sequences.rs#1530
 Expr::Call(CallExpr {
                callee: Callee::Expr(b_callee),
                args: b_args,
                ..
            }) => {
                let is_this_undefined = b_callee.is_ident();
                trace_op!("seq: Try callee of call");

                if let Expr::Ident(i) = &**b_callee {
                    if let Some(usage) = self.data.vars.get(&i.to_id()) {
                        if usage.reassigned() {
                            return Ok(false);
                        }
                    }
                }

But that would cause lots of regressions too.

@kdy1 kdy1 closed this Oct 6, 2022
@kdy1 kdy1 deleted the next branch October 6, 2022 01:36
@kdy1 kdy1 modified the milestones: Planned, v1.3.5 Oct 6, 2022
@swc-project swc-project locked as resolved and limited conversation to collaborators Nov 5, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

minifier should not skip callee in more cases
2 participants