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

WIP: fix: don't drop side effects of default param assignments #1148

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
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
44 changes: 43 additions & 1 deletion lib/compress/index.js
Expand Up @@ -2463,7 +2463,49 @@ def_optimize(AST_Call, function(self, compressor) {
}).optimize(compressor);
}

const can_drop_this_call = is_regular_func && compressor.option("side_effects") && fn.body.every(is_empty);
function parameter_defaults_have_side_effects() {
return fn.argnames.some((param, ind) => {
if (!(param instanceof AST_DefaultAssign)) {
return false;
}

const arg = self.args[ind];
const default_param_assignment_wont_be_performed =
arg
&& (
// If the argument evaluates to `undefined`, default assignment is also performed.
// This checks whether the argument is guaranteed to NOT evaluate to `undefined`.

// TODO this works, but it the conditions are too narrow. Need to consider things like
// objects, functions, and all the other stuff.

// Not sure if this doesn't give false positives.
// // Keep in mind that the fact that `is_undefined()` alone returned `false` doesn't mean that
// // the argument can't evaluate to `undefined` at runtime, so we also need `is_constant()`
// // to make it so.
// arg.is_constant()
// && !is_undefined(arg)

arg.is_boolean()
|| arg.is_number()
|| arg.is_string()
);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it's best to create a is_not_undefined in compress/inference.js and use that. This way there's no implicit dependency between is_constant and !is_undefined that needs to be maintained and remembered.

if (default_param_assignment_wont_be_performed) {
return false;
}

// Keep in mind that this could be a destructuring assignment, so
// `!param.right.has_side_effects(compressor)` is not enough.
if (!param.has_side_effects(compressor)) {
return false;
}

return true;
});
}

const can_drop_this_call = is_regular_func && compressor.option("side_effects") && fn.body.every(is_empty)
&& !parameter_defaults_have_side_effects();
if (can_drop_this_call) {
var args = self.args.concat(make_node(AST_Undefined, self));
return make_sequence(self, args).optimize(compressor);
Expand Down
214 changes: 214 additions & 0 deletions test/compress/test.js
@@ -0,0 +1,214 @@
// dsa: {
// options = {
// toplevel: true,
// unused: true,
// reduce_vars: true,
// side_effects: true,

// inline: true,
// }
// input: {
// // function cause_side_effect(a = console.log("side effect")) {}
// // cause_side_effect();

// function cause_side_effect(
// // {a} = {a:console.log("side effect 1")},
// // p2 = console.log("side effect 2"),
// // p3 = console.log("side effect 3"),

// p1 = 2,
// p2 = (()=>{})(),
// ) {}
// cause_side_effect(undefined, 2);

// // function dummy() {}
// // dummy(cause_side_effect());
// }
// expect: {
// console.log("side effect 1");
// console.log("side effect 2");
// console.log("side effect 3");
// }
// }
// asd: {
// options = {
// toplevel: true,
// unused: true,
// reduce_vars: true,
// side_effects: true,

// inline: true,
// // passes: 10,

// // collapse_vars: true,
// // conditionals: true,
// // evaluate: true,
// // inline: true,
// // passes: 3,
// // reduce_funcs: true,
// // reduce_vars: true,
// // sequences: true,
// // unused: true,
// }
// input: {
// // function cause_side_effect(a = console.log("side effect")) {}
// // cause_side_effect();

// function cause_side_effect(
// p1 = console.log("side effect 1"),
// p2 = console.log("side effect 2"),
// p3 = console.log("side effect 3"),
// ) {}
// cause_side_effect(undefined, 2);

// // function dummy() {}
// // dummy(cause_side_effect());
// }
// expect: {
// console.log("side effect 1");
// console.log("side effect 2");
// console.log("side effect 3");
// }
// }
simple: {
options = {
toplevel: true,
unused: true,
reduce_vars: true,
side_effects: true,
// passes: 5
}
input: {
function cause_side_effect(p = console.log("side effect")) {}

cause_side_effect();
// If the argument evaluates to `undefined`, default assignment is also performed.
cause_side_effect(undefined);
// If it is unknown what the argument evaluates to, keep.
// cause_side_effect(Math.random() > 0.5 ? undefined : true);
cause_side_effect(id());
cause_side_effect(id);
}
expect: {
function cause_side_effect(p = console.log("side effect")) {}

cause_side_effect();
cause_side_effect(void 0);
cause_side_effect(id());
cause_side_effect(id);
}
// TODO when I uncomment this it starts saying "failed running reminified input" and producing incorrect output.
// expect_stdout: [
// "side effect",
// "side effect",
// "side effect",
// ]
}
drop_if_assignment_not_used: {
options = {
toplevel: true,
unused: true,
reduce_vars: true,
side_effects: true,
}
input: {
function cause_side_effect(p = console.log("side effect")) {}

cause_side_effect("a");
cause_side_effect(null);
cause_side_effect({ a: 1 });
cause_side_effect(() => 1);
}
expect: {}
}
// undefined_as_argument: {
// options = {
// toplevel: true,
// unused: true,
// reduce_vars: true,
// side_effects: true,
// }
// input: {
// function cause_side_effect(p = console.log("side effect")) {}
// cause_side_effect(undefined);
// }
// expect: {
// console.log("side effect")
// }
// }
no_false_positives: {
options = {
toplevel: true,
unused: true,
reduce_vars: true,
side_effects: true,
}
input: {
function no_side_effects(
p1,
p2 = "a",
p3 = (() => {})(),
p4 = undefined,
p5 = [p1, p2, p3, p4],
p6,
...rest
) {
// return [p1, p2, p3, p4, p5, p6, ...rest];
}
no_side_effects();
}
expect: {}
}
object_destructuring_getter_side_effect: {
options = {
toplevel: true,
unused: true,
reduce_vars: true,
side_effects: true,

pure_getters: false,
}
input: {
// Evaluating the object in itself is fine, but destructuring it can cause a side effect.
const obj = { get a() { console.log("side effect") } };
function cause_side_effect(
// {a, b} = { a: 1, b: 2 },
{a} = obj,
) {}
cause_side_effect();
}
// expect: {
// console.log("side effect")
// }
expect_stdout: [
"side effect",
]
}
array_destructuring_getter_side_effect: {
options = {
toplevel: true,
unused: true,
reduce_vars: true,
side_effects: true,

pure_getters: false,
}
input: {
const arr = [0, 0, 0];
arr.__defineGetter__(0, function () {
console.log("side effect");
return 1;
})
function cause_side_effect(
// {a, b} = { a: 1, b: 2 },
[a, b] = arr,
) {}
cause_side_effect();
}
// expect: {
// console.log("side effect")
// }
expect_stdout: [
"side effect",
]
}