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

Compiler doesn't remove unreachable branches #3128

Open
kosik86 opened this issue Oct 20, 2018 · 6 comments
Open

Compiler doesn't remove unreachable branches #3128

kosik86 opened this issue Oct 20, 2018 · 6 comments

Comments

@kosik86
Copy link

kosik86 commented Oct 20, 2018

This can be a limitation of the compiler.
Input:

const flags = {
    "enabled": false
};

if (!flags["enabled"]) {
    console.log("not enabled");
} else {
    console.log("enabled");
}

if (flags["otherOption"]) {
    console.log("other option");
}

Command:

java -jar node_modules/google-closure-compiler/compiler.jar  --entry_point input.js \
  --compilation_level ADVANCED_OPTIMIZATIONS \
  --isolation_mode IIFE input.js 

Output:

(function(){var a={enabled:!1};a.enabled?console.log("enabled"):console.log("not enabled");a.otherOption&&console.log("other option");}).call(this);

I would expect the output to only contain one console.log as other branches are unreachable.
Is this not possible, e.g. is it unable to determine these conditions in compilation time?

@kosik86
Copy link
Author

kosik86 commented Oct 22, 2018

@concavelenz thanks for the response. I preferred bracket notation, especially when these flags come from outside, but I guess this can be rewritten to support dot notation.

I'm a bit surprised that having the otherOption declared as undefined makes that big difference to the compiler. Is this expected behaviour?

@ChadKillingsworth
Copy link
Collaborator

I suspect that otherOption not being defined on the flags namespace causes the compiler to back out of any optimizations on that object since it is unknown.

As for the quoted keys - I'd be pretty wary of CollapseProperties doing anything with quoted properties. That's normally what you do to have the compiler be completely hands off.

@kosik86
Copy link
Author

kosik86 commented Oct 26, 2018

@ChadKillingsworth as you suggested I tried avoiding all quoted keys.
I would really like to enable more aggressive optimisations and have unreachable branch and function removed.

const flags = {
    enabled: false
};
if (!flags.enabled) {
    console.log("not enabled");
} else {
    console.log("enabled");
}

if (flags.enabled) {
    func(flags);
}
function func(flags) {
    console.log(flags);
}

Command

java -jar node_modules/google-closure-compiler/compiler.jar \
--entry_point ./index.js --process_common_js_modules \
--module_resolution NODE --compilation_level ADVANCED_OPTIMIZATIONS \
./index.js \
--assume_function_wrapper \
--isolation_mode IIFE

Example
https://closure-compiler.appspot.com/home#code%3D%252F%252F%2520%253D%253DClosureCompiler%253D%253D%250A%252F%252F%2520%2540compilation_level%2520ADVANCED_OPTIMIZATIONS%250A%252F%252F%2520%2540output_file_name%2520default.js%250A%252F%252F%2520%2540assume_function_wrapper%250A%252F%252F%2520%2540isolation_mode%2520IIFE%250A%252F%252F%2520%253D%253D%252FClosureCompiler%253D%253D%250A%250Aconst%2520flags%2520%253D%2520%257B%250A%2520%2520%2520%2520enabled%253A%2520false%250A%257D%253B%250A%250Aif%2520(!flags.enabled)%2520%257B%250A%250A%2520%2520%2520%2520console.log(%2522not%2520enabled%2522)%253B%250A%250A%257D%2520else%2520%257B%250A%250A%2520%2520%2520%2520console.log(%2522enabled%2522)%253B%250A%250A%257D%250A%250A%250Aif%2520(flags.enabled)%2520%257B%250A%250A%2520%2520%2520%2520func(flags)%253B%250A%250A%257D%250A%250A%250Afunction%2520func(flags)%2520%257B%250A%250A%2520%2520%2520%2520console.log(flags)%253B%250A%250A%257D

@ChadKillingsworth
Copy link
Collaborator

Not sure why the function is causing problems, but you are going to get much better elimination if you aren't depending on CollapseProperties. Variables work much more dependably than object properties for this.

@kosik86
Copy link
Author

kosik86 commented Oct 28, 2018

@ChadKillingsworth this makes sense. I think my example was oversimplified.
I use javascript modules and I was hoping that closure compiler can help removing unused functions in these modules if they all share the same config object.

The structure is going to be very similar, but the function is imported from a module.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants