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.

Sign up for GitHub

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

Jump to bottom

fix(es/minifier): Mark delete as a property mutation #6063

Merged
merged 6 commits into from
Oct 6, 2022
Merged

fix(es/minifier): Mark delete as a property mutation #6063

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
0b6f2d5
Add a test dor #6004
kdy1 Oct 6, 2022
d192726
Mark delete as property mutation
kdy1 Oct 6, 2022
bcca282
Update test refs
kdy1 Oct 6, 2022
e40b6b1
Revert "Add a test dor #6004"
kdy1 Oct 6, 2022
114bc97
Add a test
kdy1 Oct 6, 2022
88438b9
Merge branch 'main' into next2
swc-bot Oct 6, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions crates/swc_ecma_minifier/src/analyzer/ctx.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,8 @@ pub(crate) struct Ctx {

pub(super) in_await_arg: bool,

pub(super) is_delete_arg: bool,

pub(super) in_left_of_for_loop: bool,

pub(super) executed_multiple_time: bool,
Expand Down
46 changes: 44 additions & 2 deletions crates/swc_ecma_minifier/src/analyzer/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -409,6 +409,7 @@ where
{
let ctx = Ctx {
in_pat_of_param: true,
is_delete_arg: false,
..child.ctx
};
n.params.visit_with(&mut *child.with_ctx(ctx));
Expand All @@ -431,6 +432,7 @@ where
{
let ctx = Ctx {
in_pat_of_param: true,
is_delete_arg: false,
..child.ctx
};
n.params.visit_with(&mut *child.with_ctx(ctx));
Expand All @@ -449,13 +451,15 @@ where
in_assign_lhs: true,
is_exact_reassignment: true,
is_op_assign: n.op != op!("="),
is_delete_arg: false,
..self.ctx
};
n.left.visit_with(&mut *self.with_ctx(ctx));

let ctx = Ctx {
in_assign_lhs: false,
is_exact_reassignment: false,
is_delete_arg: false,
..self.ctx
};
n.right.visit_with(&mut *self.with_ctx(ctx));
Expand Down Expand Up @@ -493,6 +497,7 @@ where
{
let ctx = Ctx {
in_pat_of_param: false,
is_delete_arg: false,
var_decl_kind_of_pat: None,
..self.ctx
};
Expand All @@ -504,6 +509,7 @@ where
fn visit_await_expr(&mut self, n: &AwaitExpr) {
let ctx = Ctx {
in_await_arg: true,
is_delete_arg: false,
..self.ctx
};
n.visit_children_with(&mut *self.with_ctx(ctx));
Expand Down Expand Up @@ -585,6 +591,7 @@ where
let ctx = Ctx {
inline_prevented,
in_call_arg: true,
is_delete_arg: false,
is_exact_arg: true,
is_exact_reassignment: false,
is_callee: false,
Expand Down Expand Up @@ -614,6 +621,7 @@ where
{
let ctx = Ctx {
in_cond: true,
is_delete_arg: false,
in_catch_param: true,
..self.ctx
};
Expand All @@ -623,6 +631,7 @@ where
{
let ctx = Ctx {
in_cond: true,
is_delete_arg: false,
..self.ctx
};
self.with_ctx(ctx).visit_in_cond(&n.body);
Expand All @@ -636,6 +645,7 @@ where
{
let ctx = Ctx {
inline_prevented: true,
is_delete_arg: false,
..self.ctx
};
n.super_class.visit_with(&mut *self.with_ctx(ctx));
Expand Down Expand Up @@ -695,10 +705,12 @@ where
#[cfg_attr(feature = "debug", tracing::instrument(skip(self, n)))]
fn visit_do_while_stmt(&mut self, n: &DoWhileStmt) {
n.body.visit_with(&mut *self.with_ctx(Ctx {
is_delete_arg: false,
executed_multiple_time: true,
..self.ctx
}));
n.test.visit_with(&mut *self.with_ctx(Ctx {
is_delete_arg: false,
executed_multiple_time: true,
..self.ctx
}));
Expand Down Expand Up @@ -799,6 +811,7 @@ where
e.left.visit_with(self);
let ctx = Ctx {
in_cond: true,
is_delete_arg: false,
..self.ctx
};
self.with_ctx(ctx).visit_in_cond(&e.right);
Expand Down Expand Up @@ -827,6 +840,7 @@ where
fn visit_fn_decl(&mut self, n: &FnDecl) {
let ctx = Ctx {
in_decl_with_no_side_effect_for_member_access: true,
is_delete_arg: false,
..self.ctx
};
self.with_ctx(ctx).declare_decl(&n.ident, true, None, true);
Expand Down Expand Up @@ -893,15 +907,17 @@ where

self.with_child(n.span.ctxt, ScopeKind::Block, |child| {
let ctx = Ctx {
in_left_of_for_loop: true,
is_exact_reassignment: true,
is_delete_arg: false,
in_left_of_for_loop: true,
..child.ctx
};
n.left.visit_with(&mut *child.with_ctx(ctx));

n.right.visit_with(child);

let ctx = Ctx {
is_delete_arg: false,
executed_multiple_time: true,
in_cond: true,
..child.ctx
Expand All @@ -919,13 +935,15 @@ where
let ctx = Ctx {
in_left_of_for_loop: true,
is_exact_reassignment: true,
is_delete_arg: false,
..child.ctx
};
n.left.visit_with(&mut *child.with_ctx(ctx));

let ctx = Ctx {
executed_multiple_time: true,
in_cond: true,
is_delete_arg: false,
..child.ctx
};
child.with_ctx(ctx).visit_in_cond(&n.body);
Expand All @@ -939,6 +957,7 @@ where
let ctx = Ctx {
executed_multiple_time: true,
in_cond: true,
is_delete_arg: false,
..self.ctx
};

Expand Down Expand Up @@ -987,6 +1006,7 @@ where
fn visit_if_stmt(&mut self, n: &IfStmt) {
let ctx = Ctx {
in_cond: true,
is_delete_arg: false,
..self.ctx
};
n.test.visit_with(self);
Expand Down Expand Up @@ -1024,6 +1044,7 @@ where
is_exact_arg: false,
is_exact_reassignment: false,
is_callee: false,
is_delete_arg: false,
..self.ctx
};
c.visit_with(&mut *self.with_ctx(ctx));
Expand All @@ -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. (

swc/crates/swc_ecma_minifier/src/analyzer/mod.rs

Lines 1032 to 1040 in 114bc97

{
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.

v.mark_has_property_mutation();
}

Expand Down Expand Up @@ -1240,6 +1261,7 @@ where
in_call_arg: false,
in_assign_lhs: false,
in_await_arg: false,
is_delete_arg: false,
..self.ctx
};
n.visit_children_with(&mut *self.with_ctx(ctx));
Expand All @@ -1251,6 +1273,7 @@ where
for stmt in stmts {
let ctx = Ctx {
in_cond: self.ctx.in_cond || had_cond,
is_delete_arg: false,
..self.ctx
};

Expand All @@ -1266,6 +1289,7 @@ where
let ctx = Ctx {
is_exact_arg: false,
is_exact_reassignment: false,
is_delete_arg: false,
..self.ctx
};
c.visit_with(&mut *self.with_ctx(ctx));
Expand All @@ -1280,6 +1304,7 @@ where

for case in n.cases.iter() {
let ctx = Ctx {
is_delete_arg: false,
in_cond: true,
..self.ctx
};
Expand All @@ -1297,6 +1322,7 @@ where
fn visit_try_stmt(&mut self, n: &TryStmt) {
let ctx = Ctx {
in_cond: true,
is_delete_arg: false,
..self.ctx
};

Expand All @@ -1308,6 +1334,18 @@ where
let ctx = Ctx {
in_update_arg: true,
is_exact_reassignment: true,
is_delete_arg: false,
..self.ctx
};
n.visit_children_with(&mut *self.with_ctx(ctx));
}

#[cfg_attr(feature = "debug", tracing::instrument(skip(self, n)))]
fn visit_unary_expr(&mut self, n: &UnaryExpr) {
let ctx = Ctx {
in_update_arg: false,
is_exact_reassignment: false,
is_delete_arg: n.op == op!("delete"),
..self.ctx
};
n.visit_children_with(&mut *self.with_ctx(ctx));
Expand All @@ -1321,6 +1359,7 @@ where
in_call_arg: false,
in_assign_lhs: false,
in_await_arg: false,
is_delete_arg: false,
..self.ctx
};
n.visit_children_with(&mut *self.with_ctx(ctx));
Expand Down Expand Up @@ -1365,6 +1404,7 @@ where
.as_deref()
.map(is_safe_to_access_prop)
.unwrap_or(false),
is_delete_arg: false,
..self.ctx
};
e.name.visit_with(&mut *self.with_ctx(ctx));
Expand Down Expand Up @@ -1407,11 +1447,13 @@ where
fn visit_while_stmt(&mut self, n: &WhileStmt) {
n.test.visit_with(&mut *self.with_ctx(Ctx {
executed_multiple_time: true,
is_delete_arg: false,
..self.ctx
}));
let ctx = Ctx {
executed_multiple_time: true,
in_cond: true,
is_delete_arg: false,
..self.ctx
};

Expand Down
4 changes: 2 additions & 2 deletions crates/swc_ecma_minifier/tests/benches-full/jquery.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2102,7 +2102,7 @@
},
prefilters: [
function(elem, props, opts) {
var prop, value, toggle, hooks, oldfire, propTween, restoreDisplay, display, anim = this, orig = {}, style = elem.style, hidden = elem.nodeType && isHiddenWithinTree(elem), dataShow = dataPriv.get(elem, "fxshow");
var prop, value, toggle, hooks, oldfire, propTween, restoreDisplay, display, isBox = "width" in props || "height" in props, anim = this, orig = {}, style = elem.style, hidden = elem.nodeType && isHiddenWithinTree(elem), dataShow = dataPriv.get(elem, "fxshow");
for(prop in opts.queue || (null == (hooks = jQuery._queueHooks(elem, "fx")).unqueued && (hooks.unqueued = 0, oldfire = hooks.empty.fire, hooks.empty.fire = function() {
hooks.unqueued || oldfire();
}), hooks.unqueued++, anim.always(function() {
Expand All @@ -2116,7 +2116,7 @@
}
orig[prop] = dataShow && dataShow[prop] || jQuery.style(elem, prop);
}
if (!(!(propTween = !jQuery.isEmptyObject(props)) && jQuery.isEmptyObject(orig))) for(prop in ("width" in props || "height" in props) && 1 === elem.nodeType && (opts.overflow = [
if (!(!(propTween = !jQuery.isEmptyObject(props)) && jQuery.isEmptyObject(orig))) for(prop in isBox && 1 === elem.nodeType && (opts.overflow = [
style.overflow,
style.overflowX,
style.overflowY
Expand Down
2 changes: 1 addition & 1 deletion crates/swc_ecma_minifier/tests/benches-full/victory.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18116,7 +18116,7 @@
target: target
}, "state"), mutatedProps = eventReturn.mutation(lodash_assign__WEBPACK_IMPORTED_MODULE_8___default()({}, mutationTargetProps, mutationTargetState), baseProps), childState = baseState[childName] || {}, updateState = function(state) {
var state1, state2;
return mutatedProps ? "parent" === target ? lodash_assign__WEBPACK_IMPORTED_MODULE_8___default()(state, _defineProperty({}, key, lodash_assign__WEBPACK_IMPORTED_MODULE_8___default()(state[key], mutatedProps))) : lodash_assign__WEBPACK_IMPORTED_MODULE_8___default()(state, _defineProperty({}, key, lodash_assign__WEBPACK_IMPORTED_MODULE_8___default()(state[key], _defineProperty({}, target, mutatedProps)))) : (state[key] && state[key][target] && delete state[key][target], state[key] && !lodash_keys__WEBPACK_IMPORTED_MODULE_0___default()(state[key]).length && delete state[key], state);
return mutatedProps ? "parent" === target ? lodash_assign__WEBPACK_IMPORTED_MODULE_8___default()(state, _defineProperty({}, key, lodash_assign__WEBPACK_IMPORTED_MODULE_8___default()(state[key], mutatedProps))) : lodash_assign__WEBPACK_IMPORTED_MODULE_8___default()(state, _defineProperty({}, key, lodash_assign__WEBPACK_IMPORTED_MODULE_8___default()(state[key], _defineProperty({}, target, mutatedProps)))) : ((state2 = state)[key] && state2[key][target] && delete state2[key][target], state2[key] && !lodash_keys__WEBPACK_IMPORTED_MODULE_0___default()(state2[key]).length && delete state2[key], state2);
};
return null != childName ? lodash_assign__WEBPACK_IMPORTED_MODULE_8___default()(baseState, _defineProperty({}, childName, updateState(childState))) : updateState(baseState);
}, getReturnByChild = function(childName) {
Expand Down
14 changes: 14 additions & 0 deletions crates/swc_ecma_minifier/tests/exec.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10131,6 +10131,20 @@ fn feedback_regex_range() {
run_default_exec_test(src);
}

#[test]
fn issue_6004() {
run_default_exec_test(
r###"
const props = {'a': 1, 'b': 2};
const isBox = 'a' in props || 'b' in props;
for (const p in props) {
delete props[p];
}
console.log(isBox);
"###,
);
}

#[test]
fn issue_6047_1() {
run_default_exec_test(
Expand Down
8 changes: 4 additions & 4 deletions crates/swc_ecma_minifier/tests/fixture/issues/firebase-core/1/output.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1704,7 +1704,7 @@
}();
}
exports.default = function(_param) {
var src, arr, sizerSvg, src1 = _param.src, sizes = _param.sizes, _unoptimized = _param.unoptimized, unoptimized = void 0 !== _unoptimized && _unoptimized, _priority = _param.priority, priority = void 0 !== _priority && _priority, loading = _param.loading, _lazyBoundary = _param.lazyBoundary, className = _param.className, quality = _param.quality, width = _param.width, height = _param.height, objectFit = _param.objectFit, objectPosition = _param.objectPosition, onLoadingComplete = _param.onLoadingComplete, _loader = _param.loader, loader = void 0 === _loader ? defaultImageLoader : _loader, _placeholder = _param.placeholder, placeholder = void 0 === _placeholder ? "empty" : _placeholder, blurDataURL = _param.blurDataURL, all = function(source, excluded) {
var src, arr, sizerSvg, src1 = _param.src, sizes = _param.sizes, _unoptimized = _param.unoptimized, unoptimized = void 0 !== _unoptimized && _unoptimized, _priority = _param.priority, priority = void 0 !== _priority && _priority, loading = _param.loading, _lazyBoundary = _param.lazyBoundary, className = _param.className, quality = _param.quality, width = _param.width, height = _param.height, objectFit = _param.objectFit, objectPosition = _param.objectPosition, onLoadingComplete = _param.onLoadingComplete, _loader = _param.loader, loader = void 0 === _loader ? defaultImageLoader : _loader, _placeholder = _param.placeholder, placeholder = void 0 === _placeholder ? "empty" : _placeholder, blurDataURL = _param.blurDataURL, rest = function(source, excluded) {
if (null == source) return {};
var key, i, target = function(source, excluded) {
if (null == source) return {};
Expand Down Expand Up @@ -1735,7 +1735,7 @@
"placeholder",
"blurDataURL"
]), layout = sizes ? "responsive" : "intrinsic";
"layout" in all && (all.layout && (layout = all.layout), delete all.layout);
"layout" in rest && (rest.layout && (layout = rest.layout), delete rest.layout);
var staticSrc = "";
if ("object" == typeof (src = src1) && (isStaticRequire(src) || void 0 !== src.src)) {
var staticImageData = isStaticRequire(src1) ? src1.default : src1;
Expand Down Expand Up @@ -1851,7 +1851,7 @@
alt: "",
"aria-hidden": !0,
src: "data:image/svg+xml;base64,".concat(_toBase64.toBase64(sizerSvg))
}) : null) : null, _react.default.createElement("img", Object.assign({}, all, imgAttributes, {
}) : null) : null, _react.default.createElement("img", Object.assign({}, rest, imgAttributes, {
decoding: "async",
"data-nimg": layout,
className: className,
Expand All @@ -1874,7 +1874,7 @@
}(img, srcString, layout, placeholder, onLoadingComplete);
},
style: _objectSpread({}, imgStyle, blurStyle)
})), _react.default.createElement("noscript", null, _react.default.createElement("img", Object.assign({}, all, generateImgAttrs({
})), _react.default.createElement("noscript", null, _react.default.createElement("img", Object.assign({}, rest, generateImgAttrs({
src: src1,
unoptimized: unoptimized,
layout: layout,
Expand Down
8 changes: 4 additions & 4 deletions crates/swc_ecma_minifier/tests/fixture/issues/react-countup/1/output.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@
}();
}
exports.default = function(_param) {
var src, arr, sizerSvg, src1 = _param.src, sizes = _param.sizes, _unoptimized = _param.unoptimized, unoptimized = void 0 !== _unoptimized && _unoptimized, _priority = _param.priority, priority = void 0 !== _priority && _priority, loading = _param.loading, _lazyBoundary = _param.lazyBoundary, className = _param.className, quality = _param.quality, width = _param.width, height = _param.height, objectFit = _param.objectFit, objectPosition = _param.objectPosition, onLoadingComplete = _param.onLoadingComplete, _loader = _param.loader, loader = void 0 === _loader ? defaultImageLoader : _loader, _placeholder = _param.placeholder, placeholder = void 0 === _placeholder ? "empty" : _placeholder, blurDataURL = _param.blurDataURL, all = function(source, excluded) {
var src, arr, sizerSvg, src1 = _param.src, sizes = _param.sizes, _unoptimized = _param.unoptimized, unoptimized = void 0 !== _unoptimized && _unoptimized, _priority = _param.priority, priority = void 0 !== _priority && _priority, loading = _param.loading, _lazyBoundary = _param.lazyBoundary, className = _param.className, quality = _param.quality, width = _param.width, height = _param.height, objectFit = _param.objectFit, objectPosition = _param.objectPosition, onLoadingComplete = _param.onLoadingComplete, _loader = _param.loader, loader = void 0 === _loader ? defaultImageLoader : _loader, _placeholder = _param.placeholder, placeholder = void 0 === _placeholder ? "empty" : _placeholder, blurDataURL = _param.blurDataURL, rest = function(source, excluded) {
if (null == source) return {};
var key, i, target = function(source, excluded) {
if (null == source) return {};
Expand Down Expand Up @@ -126,7 +126,7 @@
"placeholder",
"blurDataURL"
]), layout = sizes ? "responsive" : "intrinsic";
"layout" in all && (all.layout && (layout = all.layout), delete all.layout);
"layout" in rest && (rest.layout && (layout = rest.layout), delete rest.layout);
var staticSrc = "";
if ("object" == typeof (src = src1) && (isStaticRequire(src) || void 0 !== src.src)) {
var staticImageData = isStaticRequire(src1) ? src1.default : src1;
Expand Down Expand Up @@ -242,7 +242,7 @@
alt: "",
"aria-hidden": !0,
src: "data:image/svg+xml;base64,".concat(_toBase64.toBase64(sizerSvg))
}) : null) : null, _react.default.createElement("img", Object.assign({}, all, imgAttributes, {
}) : null) : null, _react.default.createElement("img", Object.assign({}, rest, imgAttributes, {
decoding: "async",
"data-nimg": layout,
className: className,
Expand All @@ -265,7 +265,7 @@
}(img, srcString, layout, placeholder, onLoadingComplete);
},
style: _objectSpread({}, imgStyle, blurStyle)
})), _react.default.createElement("noscript", null, _react.default.createElement("img", Object.assign({}, all, generateImgAttrs({
})), _react.default.createElement("noscript", null, _react.default.createElement("img", Object.assign({}, rest, generateImgAttrs({
src: src1,
unoptimized: unoptimized,
layout: layout,
Expand Down