google / closure-compiler

A JavaScript checker and optimizer.
https://developers.google.com/closure/compiler/
Apache License 2.0
7.4k stars 1.15k forks source link

Setter side effects not accounted for in prototype methods #2632

Open Benholio opened 7 years ago

Benholio commented 7 years ago

If I'm only using my property setter internally, it can get eliminated as dead code. Example (formatted for online compiler):

// ==ClosureCompiler==
// @compilation_level ADVANCED_OPTIMIZATIONS
// @formatting pretty_print
// ==/ClosureCompiler==

/** @constructor */
foo = function() {
  this.bar = 'assigned'
};

foo.prototype.__bar_ = 'initial'
foo.prototype.getBar = function() { return this.__bar_ }
foo.prototype.setBar = function(value) { this.__bar_ = value }
foo.prototype.printBar = function() { console.log(this.__bar_) }

Object.defineProperties(foo.prototype, {
   bar: {
    'get': foo.prototype.getBar,
    'set': foo.prototype.setBar
  }
})

var myFoo = new foo()
myFoo.printBar()

Compiles to:

foo = function() {
};
foo.prototype.a = "";
var a = new foo;
console.log(a.a);

The getter/setter has been eliminated, including the assignment. If I use the setter from outside of the class, it is not eliminated, and both the internal assignment and external assignment are preserved:

// ==ClosureCompiler==
// @compilation_level ADVANCED_OPTIMIZATIONS
// @formatting pretty_print
// ==/ClosureCompiler==

/** @constructor */
foo = function() {
  this.bar = 'assigned'
};

foo.prototype.__bar_ = 'initial'
foo.prototype.getBar = function() { return this.__bar_ }
foo.prototype.setBar = function(value) { this.__bar_ = value }
foo.prototype.printBar = function() { console.log(this.__bar_) }

Object.defineProperties(foo.prototype, {
   bar: {
    'get': foo.prototype.getBar,
    'set': foo.prototype.setBar
  }
})

var myFoo = new foo()
myFoo.bar = 'external'
myFoo.printBar()

Result:

foo = function() {
  this.b = "assigned";
};
foo.prototype.a = "initial";
foo.prototype.c = function() {
  return this.a;
};
foo.prototype.f = function(b) {
  this.a = b;
};
Object.defineProperties(foo.prototype, {b:{get:foo.prototype.c, set:foo.prototype.f}});
var a = new foo;
a.b = "external";
console.log(a.a);

Is there a problem with the way I'm using getter/setters here? Or any workaround that would prevent these things from being eliminated?

Benholio commented 7 years ago

For what it's worth, the specific optimization that is eliminating these setters is RemoveUnusedClassProperties.

ChadKillingsworth commented 7 years ago

Actually this is excellent and exactly by design. If something is not used, it should be removed.

If you need the property kept, you'll need to export it:

window['_sinkValue'] = foo.prototype.bar;

You can also use the sinkValue technique from closure-library.

Benholio commented 7 years ago

Thanks for the quick reply! I will definitely investigate those options. I want to make sure I understand your answer, though.

The property setter foo.prototype.setBar is being removed, but it is being used. It is used to assign a value to __bar_ property (this.bar = 'assigned') and that property is used later when myFoo.printBar() is called. The un-optimized code produces console output of 'assigned', and the optimized output produces 'initial'.

For example, if this.bar = 'assigned' is replaced with a direct call to the setter function, this.setBar('assigned'), then the assignment is preserved.

ChadKillingsworth commented 7 years ago

Nope I totally missed that nuance.

More info: http://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%2540formatting%2520pretty_print%250A%252F%252F%2520%2540language_out%2520ES5%250A%252F%252F%2520%253D%253D%252FClosureCompiler%253D%253D%250A%250Aclass%2520Foo%2520%257B%250A%2520%2520constructor()%2520%257B%250A%2520%2520%2520%2520this.baz%2520%253D%2520'assigned'%253B%250A%2520%2520%257D%250A%2520%2520%250A%2520%2520init2()%2520%257B%250A%2520%2520%2520%2520this.baz%2520%253D%2520'assigned'%253B%250A%2520%2520%257D%250A%2520%2520%250A%2520%2520get%2520baz()%2520%257B%250A%2520%2520%2520%2520return%2520this.__baz_%253B%250A%2520%2520%257D%250A%2520%2520set%2520baz(value)%2520%257B%250A%2520%2520%2520%2520this.__baz_%2520%253D%2520value%253B%250A%2520%2520%257D%250A%2520%2520printBaz()%2520%257B%250A%2520%2520%2520%2520console.log(this.__baz_)%253B%250A%2520%2520%257D%250A%257D%250AFoo.prototype.__baz_%2520%253D%2520'initial'%253B%250A%250A%252F**%2520%2540constructor%2520*%252F%250Afoo%2520%253D%2520function()%2520%257B%250A%2520%2520this.bar%2520%253D%2520'assigned'%253B%250A%257D%253B%250A%250Afoo.prototype.__bar_%2520%253D%2520'initial'%250Afoo.prototype.getBar%2520%253D%2520function()%2520%257B%2520return%2520this.__bar_%2520%257D%250Afoo.prototype.setBar%2520%253D%2520function(value)%2520%257B%2520this.__bar_%2520%253D%2520value%2520%257D%250Afoo.prototype.init%2520%253D%2520function()%2520%257B%2520this.bar%2520%253D%2520'assigned'%253B%2520%257D%253B%250Afoo.prototype.printBar%2520%253D%2520function()%2520%257B%2520console.log(this.__bar_)%2520%257D%250A%250AObject.defineProperties(foo.prototype%252C%2520%257B%250A%2520%2520%2520bar%253A%2520%257B%250A%2520%2520%2520%2520get%253A%2520foo.prototype.getBar%252C%250A%2520%2520%2520%2520set%253A%2520foo.prototype.setBar%250A%2520%2520%257D%250A%257D)%250A%250Avar%2520myFoo%2520%253D%2520new%2520foo()%253B%250AmyFoo.init()%253B%250A%252F%252F%2520myFoo.bar%2520%253D%2520'other'%253B%2520%252F%252F%2520uncomment%2520to%2520fix%250AmyFoo.printBar()%250A%250Avar%2520myFoo2%2520%253D%2520new%2520Foo()%253B%250AmyFoo2.init2()%253B%250A%252F%252F%2520myFoo2.baz%2520%253D%2520'other'%253B%2520%252F%252F%2520uncomment%2520to%2520fix%250AmyFoo2.printBaz()%250A