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): Mark delete as a property mutation #6063

Merged
merged 6 commits into from Oct 6, 2022

Conversation

kdy1
Copy link
Member

@kdy1 kdy1 commented Oct 6, 2022

@kdy1 kdy1 added this to the Planned milestone Oct 6, 2022
@kdy1 kdy1 self-assigned this Oct 6, 2022
@kdy1 kdy1 requested a review from jridgewell October 6, 2022 02:39
@kdy1 kdy1 marked this pull request as ready for review October 6, 2022 02:40
kodiakhq[bot]
kodiakhq bot previously approved these changes Oct 6, 2022
Copy link
Member Author

@kdy1 kdy1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

swc-bump:

  • swc_ecma_minifier

kodiakhq[bot]
kodiakhq bot previously approved these changes Oct 6, 2022
@@ -1040,7 +1061,7 @@ where
v.mark_indexed_with_dynamic_key();
}

if self.ctx.in_assign_lhs {
if self.ctx.in_assign_lhs || self.ctx.is_delete_arg {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is only catching delete foo.bar, but fails to capture delete foo.bar.baz? Based on the Expr::Ident(obj) = e.obj we're inside of. Come to think of it, there are a few others like foo().bar that won't be caught?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For delete foo.bar.baz, foo will be marked as property mutated because we don't unset is_delete_arg for the obj of MemberExpr. (

{
let ctx = Ctx {
is_exact_arg: false,
is_exact_reassignment: false,
is_callee: false,
..self.ctx
};
e.obj.visit_with(&mut *self.with_ctx(ctx));
}
)
And currently, we do not inline properties. But nice catch, we should ensure it when we add partial, per-property inlining.

Copy link
Contributor

@jridgewell jridgewell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, we're tracking if a variable reference has any mutations, and this is now detecting property mutations as a mutation of the variable.

@kdy1 kdy1 enabled auto-merge (squash) October 6, 2022 04:07
Copy link
Collaborator

@swc-bot swc-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated review comment generated by auto-rebase script

@kdy1 kdy1 merged commit 38df597 into swc-project:main Oct 6, 2022
@kdy1 kdy1 deleted the next2 branch October 6, 2022 04:48
@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.

Compress is using deleted properties with "sourceType": "module"
3 participants