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

Same-name extern methods preventing code removal for provably side-effect-free code #2132

Open
shicks opened this issue Nov 7, 2016 · 6 comments

Comments

@shicks
Copy link
Member

shicks commented Nov 7, 2016

The compiler should be able to compile the following example to zero:

(function() {
  /** @final */
  class Foo {
    get() {}
    foo() {}
  }

  new Foo().get();
  new Foo().foo();
})();

Instead, it can remove the call to foo(), but it cannot remove the call to get(), presumably because of an extern that's being conflated somewhere.

Debugger link

@concavelenz
Copy link
Contributor

It is definitions on object literals that are causing the problem. In particular, the definition of "Reflect.get" is sufficient to cause the back off in the disambiguation which is unfortunate.

@shicks
Copy link
Member Author

shicks commented Jan 5, 2017

Sure enough: Debugger link

But now I'm confused - I'd thought the compiler distinguished arbitrary object literals from namespaces. So since Reflect is @const, it shouldn't cause backoff, should it? At the very least, we know that the Reflect namespace is never an instance of Foo, so it's impossible that new Foo() could be Reflect.

What would it take to fix this?

@ChadKillingsworth
Copy link
Collaborator

I've been aware of this for a long time. My suggested solution is to recognize any constant object (namespace) in an extern as a distinct type. I just don't have the bandwidth to tackle it right now.

The following should be equivalent:

/** @fileoverview @externs */
const myNamespace = {};
/** @type {number} */
myNamespace.someProp;

vs:

/** @fileoverview @externs */
/** @constructor */
function MyNamespace() {};
/** @type {number} */
MyNamespace.prototype.someProp;

/** @type {!MyNamespace} */
const myNamespace;

The second one gives correct type checking and disambiguation - but the first one is what developers prefer to write.

@shicks
Copy link
Member Author

shicks commented Jun 7, 2017

That makes sense, but doesn't seem to work. Also, static methods on extern'd classes seem to cause problems as well, and I don't think they're amenable to that treatment (i.e. you can't declare Object as both a @constructor and an instanceof ObjectStatics).

@ChadKillingsworth
Copy link
Collaborator

@shicks Something else going on with the debugger there. The code isn't being transpiled (there's a class in the output).

@shicks
Copy link
Member Author

shicks commented Jun 8, 2017

I don't see any class in the output:

(function() {
  /** @final @struct @constructor */ var Foo = function() {
  };
  Foo.prototype.foo = function() {
  };
  Foo.prototype.bar = function() {
  };
  (new Foo).foo();
  (new Foo).bar();
})();

In any case, it looks like I needed to turn on DisambiguateProperties. With that on, it does in fact behave as you say.

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