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

Static class functions can be erroneously removed in ADVANCED mode #2763

Open
dfreedm opened this issue Dec 18, 2017 · 7 comments
Open

Static class functions can be erroneously removed in ADVANCED mode #2763

dfreedm opened this issue Dec 18, 2017 · 7 comments
Assignees
Labels
bug internal-issue-created An internal Google issue has been created to track this GitHub issue P2

Comments

@dfreedm
Copy link
Member

dfreedm commented Dec 18, 2017

In a simple example with ADVANCED optimizations enabled

class Foo {
  static print() {
    console.log('f');
  }
  constructor() {
    this.constructor.print();
  }
}

let x = new Foo();

The call to the print function remains, but the implementation is missing.

new function() {
  this.constructor.print();
};

Live Example

@brad4d brad4d self-assigned this Dec 18, 2017
@brad4d
Copy link
Contributor

brad4d commented Dec 18, 2017

What happens is this.

  1. the ES6 class is transpiled into an ES5 class, so we end up with
    function Foo() {
     this.constructor.print();
    }
    Foo.print = function() { console.log('f'); }
    var x = new Foo();
  2. Then CollapseProperties does it's thing to rename Foo.print to Foo$print, but it doesn't realize that this.constructor.print is the same object and fails to update that.
  3. Finally Foo$print appears to be unused, so it gets removed.

@brad4d
Copy link
Contributor

brad4d commented Dec 18, 2017

As a workaround, you can use the @nocollapse annotation to prevent collapsing the print function.

class Foo {
  /** @nocollapse */
  static print() {
    console.log('f');
  }
  constructor() {
    this.constructor.print();
  }
}

let x = new Foo();

@ChadKillingsworth
Copy link
Collaborator

I believe the situation may be a bit worse when the static method is on a super class. @azakus That's why #2286 exists.

@WearyMonkey
Copy link
Contributor

This seems to happen anytime the static method is not called directly from the class.

Example:

class Foo {
   static foo() { 
     console.log('foo'); 
   }
}

function callStaticMethod(Foo) {
  Foo.foo();
}

callStaticMethod(Foo);
(function(){}).a();

The problem doesn't occur when the language_out is ECMASCRIPT_2015 +

$ java -jar compiler.jar --js example.js -O ADVANCED --language_out ECMASCRIPT_2015

'use strict';class a{static a(){console.log("foo")}}(function(b){b.a()})(a);

@theRobinator
Copy link

I'm running into this issue as well, from a slightly different angle. Demo link.
Input:

class Foo {
  static method() {
    return true;
  }
}

const Bar = {
  Foo: Foo
}

console.log(Bar.Foo.method());

Output:

console.log(function(){}.method());

@thebravoman
Copy link

thebravoman commented Oct 23, 2019

A little different but probably connected case

/**
 * @nocollapse
 * @namespace
 */
var NN ={}

NN.Container = class {
  constructor() {
    console.log("some")
  }
  getId() { 
    return new Date()
  }
}
console.log(new NN.Container().getId())

when compiled with

 java -jar $CLOSURE_PATH --js steps3.js --compilation_level ADVANCED_OPTIMIZATIONS --output_wrapper "(function() {%output%}).call(window)" --formatting=pretty_print

results in

(function() {new {}.a;
console.log(new Date);
}).call(window)

Here console.log("some") is missing. The whole NN.Container constructor is missing.

So there should be a '@nocollapse' at the NN.Container level

@kjin
Copy link
Contributor

kjin commented Oct 24, 2019

Created Google internal issue http://b/143308051

@kjin kjin added the internal-issue-created An internal Google issue has been created to track this GitHub issue label Oct 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug internal-issue-created An internal Google issue has been created to track this GitHub issue P2
Projects
None yet
Development

No branches or pull requests

7 participants