From 044fcc390043435bdd6abc5f5633b31e29647d6d Mon Sep 17 00:00:00 2001 From: WofWca Date: Thu, 17 Feb 2022 18:05:38 +0300 Subject: [PATCH] WIP: fix: don't drop side effects of default param assignments Closes #880 --- lib/compress/index.js | 44 ++++++++- test/compress/test.js | 214 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 257 insertions(+), 1 deletion(-) create mode 100644 test/compress/test.js diff --git a/lib/compress/index.js b/lib/compress/index.js index 8b901a3bf..e89030c40 100644 --- a/lib/compress/index.js +++ b/lib/compress/index.js @@ -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 is too narrow. Consider things like objects, functions, function calls + // 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() + ); + 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); diff --git a/test/compress/test.js b/test/compress/test.js new file mode 100644 index 000000000..6410540f6 --- /dev/null +++ b/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", + ] +}