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

inlining of the function definitions that have conflicting arguments #908

Draft
wants to merge 8 commits into
base: master
Choose a base branch
from
Draft

inlining of the function definitions that have conflicting arguments #908

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
09b0a8c
test: assign_and_inline
aminya Jan 12, 2021
4ade54a
test: inline_different_name
aminya Jan 12, 2021
208e584
test: rename_and_inline
aminya Jan 12, 2021
79c1c43
prep: add rename_arg
aminya Apr 26, 2021
a7e207c
feat: rename function arguments for inlining
aminya May 4, 2021
1be33ba
test: use the arguments of the parent function for transparency
aminya May 7, 2021
58e62f9
test: set pure_getters to false and add TODO
aminya May 7, 2021
8a42c82
Merge branch 'master' into inline-if-args-injectable
aminya May 8, 2021
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
24 changes: 22 additions & 2 deletions lib/compress/index.js
View file
Open in desktop
Expand Up @@ -5603,15 +5603,35 @@ def_optimize(AST_Call, function(self, compressor) {
if (has_flag(arg, UNUSED)) continue;
if (!safe_to_inject
|| block_scoped.has(arg.name)
|| identifier_atom.has(arg.name)
|| scope.conflicting_def(arg.name)) {
|| identifier_atom.has(arg.name)) {
return false;
}
if (scope.conflicting_def(arg.name)) {
rename_arg(arg);
}
if (in_loop) in_loop.push(arg.definition());
}
return true;
}

function rename_arg(arg) {
const newArgName = fn.create_symbol(AST_SymbolFunarg, {
source: fn,
scope: scope,
tentative_name: "argument_" + fn.argnames.length,
});

const cb = (node) => {
if (node instanceof AST_SymbolRef && node.thedef.name === arg.name) {
node.thedef.name = newArgName;
return node;
} else {
return walk(node, cb);
}
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

In lib/scope.js is an easy example of how to rename a variable. The definition object (obtained through node.thedef or node.definition()) can help us iterate on references and definitions alike.

It also deal with shadowing (in case fn here contains another scope which redefines a variable of the same name)

def.name = name
def.orig.forEach(function(sym) {      
    sym.name = name;                  
});                                   
def.references.forEach(function(sym) {
    sym.name = name;                  
});

Copy link
Collaborator

Choose a reason for hiding this comment

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

Otherwise this looks great, and the tests are sound.

Copy link
Author

@aminya aminya May 4, 2021 •
edited

Choose a reason for hiding this comment

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

I edited the renaming code. However, I don't know why the original function node doesn't seem to update. The last test should fail as I have written it. The inlined argument should be named argument_1. Or maybe I am understanding this wrong.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe the cause is the fact that the "defaults" option is enabled, causing more transforms to occur.

If you remove defaults and add reduce_vars, inline and unused, it should showcase what you did a bit better. Even if the code in the "expect" section isn't truly optimally minified, which isn't so important for a test.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I should have a deeper review of this PR later today. My cursory look says it's good to merge, but I'll check it out better after work.

Copy link
Author

@aminya aminya May 7, 2021 •
edited

Choose a reason for hiding this comment

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

After spending more time on this, if I am correct, I actually think this is the expected behavior! I wanted to rename the variables of the pure function so they can be inlined by the compressor (like the second test). The compressor correctly substituted the new name we created with the argument passed in!

walk(fn.body[0], cb);
}

function can_inject_vars(block_scoped, safe_to_inject) {
var len = fn.body.length;
for (var i = 0; i < len; i++) {
Expand Down
70 changes: 70 additions & 0 deletions test/compress/toplevel-pure-method.js
View file
Open in desktop
@@ -0,0 +1,70 @@
assign_and_inline: {
options = {
toplevel: true,
defaults: true,
}
input: {
class MyClass {
method(a1) {
myPureFunction(a1)
}
}

function myPureFunction (a) {
console.log(a)
}

new MyClass();
}
expect: {
new class{method(a1) {var a;a=a1,console.log(a)}};
}
}

inline_different_name: {
options = {
toplevel: true,
defaults: true,
pure_getters: true
}
input: {
class MyClass {
method(a1) {
myPureFunction(a1)
}
}

function myPureFunction (a) {
console.log(a)
}

new MyClass();
}
expect: {
new class{method(a1) {console.log(a1)}};
}
}

rename_and_inline: {
options = {
toplevel: true,
defaults: true,
pure_getters: true
}
input: {
class MyClass {
method(a) {
myPureFunction(a)
}
}

function myPureFunction (a) {
console.log(a)
}

new MyClass();
}
expect: {
new class{method(a) {console.log(a)}};
}
}