Static methods unnecessarily copied to subclass constructor

[rictic]

This code:

class Super {
  /**  
     * @nocollapse 
     */
  static method() {
    return 'foo';
  }
}

class Sub extends Super {}

console.log(Sub.method()); // should log 'foo'
console.log(Sub.hasOwnProperty('method')); // should log 'false' because Sub.method is inherited

Is lowered to ES5 that is essentially:

function Super() {}
Super.method = function() { return 'foo' }

function Sub() {}
Sub.prototype = Object.create(Super.prototype);
Object.setPrototypeOf(Sub, Super);
Sub.method = Super.method;

console.log(Sub.method()); // logs 'foo', correctly
console.log(Sub.hasOwnProperty('method')); // logs 'true', incorrectly

The Sub.method = Super.method; line is unnecessary, because Object.setPrototypeOf(Sub, Super); has already ensured that Sub.method would work correctly, and it also causes Sub.method to be an ownproperty of Sub, which diverges from the original code's semantics.

[rictic]

Repro: https://closure-compiler.appspot.com/home#code%3D%252F%252F%2520%253D%253DClosureCompiler%253D%253D%250A%252F%252F%2520%2540compilation_level%2520SIMPLE_OPTIMIZATIONS%250A%252F%252F%2520%2540output_file_name%2520default.js%250A%252F%252F%2520%253D%253D%252FClosureCompiler%253D%253D%250A%250A%252F%252F%2520ADD%2520YOUR%2520CODE%2520HERE%250Aclass%2520Super%2520%257B%250A%2520%2520%252F**%2520%2520%250A%2520%2520%2520%2520%2520*%2520%2540nocollapse%2520%250A%2520%2520%2520%2520%2520*%252F%250A%2520%2520static%2520method()%2520%257B%250A%2520%2520%2520%2520return%2520'foo'%253B%250A%2520%2520%257D%250A%257D%250A%250Aclass%2520Sub%2520extends%2520Super%2520%257B%257D%250A%250Aconsole.log(Sub.method())%253B%2520%252F%252F%2520should%2520log%2520'foo'%250Aconsole.log(Sub.hasOwnProperty('method'))%253B%2520%252F%252F%2520should%2520log%2520'false'%2520because%2520Sub.method%2520is%2520inherited

[ChadKillingsworth]

@rictic This is only true for browsers that support setPrototypeOf and doesn't nicely line up with any current language out parameter (other than ES6 - which is kinda pointless). IE 11 supports setPrototypeOf, but no earlier versions do. So while IE 9 and 10 support all the ES5 feature set, they do not support setPrototypeOf and therefore require the static methods to be copied.

Copying the static methods is not a perfect transpilation method, but is better than none.

[rictic]

The actual code uses $jscomp.inherits instead of Object.setPrototypeOf. $jscomp.inherits looks like:

$jscomp.inherits = function(a, b) {
  a.prototype = $jscomp.objectCreate(b.prototype);
  a.prototype.constructor = a;
  if ($jscomp.setPrototypeOf) {
    var c = $jscomp.setPrototypeOf;
    c(a, b);
  } else {
    for (c in b) {
      if ("prototype" != c) {
        if (Object.defineProperties) {
          var d = Object.getOwnPropertyDescriptor(b, c);
          d && Object.defineProperty(a, c, d);
        } else {
          a[c] = b[c];
        }
      }
    }
  }
  a.superClass_ = b.prototype;
};

The else block looks like it already loops through the properties of Super and copies them onto Sub. As a result, the Sub.method = Super.method; still seems duplicated to me.

The specific case that this came up in was a project that does not support IE <11, so they can depend on setPrototypeOf existing. If the Sub.method = Super.method; line were removed from the output, the output would be smaller, and the behavior would be more correct on modern browsers.

[ChadKillingsworth]

There's code that copies those methods outside of the setPrototypeOf polyfill?

I understand the extra code for projects that don't depend on <IE11, but this was the best compromise we could achieve.

[brad4d]

Internal issue created b/122077271

[brad4d]

This is working as intended.
On a browser that supports Object.setPrototypeOf() $jscomp.setPrototypeOf will be non-null and will be used to just set the prototype of the child class constructor a to be the super class constructor b.
On older browsers, the properties on b will be copied to a either directly or by using Object.defineProperty

As @ChadKillingsworth said, this was the best compromise we could reach to make sure code that depends on inheritance of static methods would work on both old and new browsers.

[rictic]

We're miscommunicating. I agree that the implementation of $jscomp.inherits is good, and the best known option for modelling inheritance. The issue is that the output also sets inherited class methods from Super onto Sub after calling $jscomp.inherits. i.e. the output of jscompiler is:

$jscomp.inherits(Sub, Super);
Sub.method = Super.method;

The explicit set of Sub.method = Super.method; is the part that seems unnecessary to me, given that $jscomp.inherits has already been called, and would have done this if it was needed.

Repro: https://closure-compiler.appspot.com/home#code%3D%252F%252F%2520%253D%253DClosureCompiler%253D%253D%250A%252F%252F%2520%2540compilation_level%2520SIMPLE_OPTIMIZATIONS%250A%252F%252F%2520%2540output_file_name%2520default.js%250A%252F%252F%2520%2540formatting%2520pretty_print%250A%252F%252F%2520%253D%253D%252FClosureCompiler%253D%253D%250A%250A%252F%252F%2520ADD%2520YOUR%2520CODE%2520HERE%250Aclass%2520Super%2520%257B%250A%2520%2520%252F**%250A%2520%2520%2520%2520%2520*%2520%2540nocollapse%250A%2520%2520%2520%2520%2520*%252F%250A%2520%2520static%2520method()%2520%257B%250A%2520%2520%2520%2520return%2520'foo'%253B%250A%2520%2520%257D%250A%257D%250A%250Aclass%2520Sub%2520extends%2520Super%2520%257B%257D%250A%250Aconsole.log(Sub.method())%253B%2520%252F%252F%2520should%2520log%2520'foo'%250Aconsole.log(Sub.hasOwnProperty('method'))%253B%2520%252F%252F%2520should%2520log%2520'false'%2520because%2520Sub.method%2520is%2520inherited

[ChadKillingsworth]

I think that's here: https://github.com/google/closure-compiler/blob/master/src/com/google/javascript/jscomp/Es6ToEs3ClassSideInheritance.java#L41-L42

And it sounds like this entire pass can be removed now that transpilation is after type checking.

[brad4d]

@rictic thanks for the clarification.
@ChadKillingsworth I agree that we want to remove Es6ToEs3ClassSideInheritance pass now,
but I think there's something blocking that, possibly something related to getters and setters.

We may have to just try it and see what breaks.

[lauraharker]

We tried removing this pass a few months ago, but it caused CollapseProperties to break class-side inheritance after transpilation. The pass didn't realize that Sub.method referred to Super.method, so would replace Super.method -> Super$method without changing Sub.method..

It should be possible to merge the pass's logic into property collapsing, though, instead of running it as part of transpilation.

closure-compiler/src/com/google/javascript/jscomp/DefaultPassConfig.java

Line 465 in 18f4faf

// This pass used to be necessary to make the typechecker happy with class-side inheritance.

[justinfagnani]

It looks like that pass is being kept for optimization now under the name ConcretizeStaticInheritanceForInlining.

Could the step that copies statics be removed though? https://github.com/google/closure-compiler/blob/master/src/com/google/javascript/jscomp/ConcretizeStaticInheritanceForInlining.java#L162

[brad4d]

I think the problem that @lauraharker mentioned above still exists, so no.

@justinfagnani what brought this to your attention? Do you have a specific use-case where changing this would make a big difference?

[justinfagnani]

@brad4d we still hit problems where code that works externally breaks in google3 in sometimes vary hard to debug ways, resulting in fixes like lit/lit#4257 where we have to access a static property only via computed property access to work around the bug. In this latest case I'm worries that the workaround has a performance cost since computed property declarations are slower, at least in V8.

[rictic]

A minimal repro of the issue we're seeing now:

class Foo {
  /** 
   * Using export to avoid needing reflect.objectProperty below.
   * @export
   * @nocollapse 
   */
  static foo: string = 'Foo base value';
  
  /** @nocollapse */
  static finalize() {
    if (!this.hasOwnProperty('foo')) {
      this.foo = 'a subclass of Foo';
    }
  }
}

class Bar extends Foo {}

function assertEqual(actual: string, expected: string, name: string) {
  if (actual === expected) {
    return;
  }
  throw new Error(`Expected ${name} to be ${expected} but was ${actual}`);
}

Foo.finalize();
Bar.finalize();
assertEqual(Foo.foo, 'Foo base value', 'Foo.foo');
assertEqual(Bar.foo, 'a subclass of Foo', 'Bar.foo');

When targeting ES5 with advanced optimizations, this throws Error: Expected Bar.foo to be a subclass of Foo but was Foo base value

Ersatz link

The Closure Compiler Playground version times out when compiling with advanced optimizations:

[brad4d]

Thanks for pinging this issue.

We're working on moving transpilation passes to occur after normalization.
After that, I hope to also move them after the InlineAndCollapseProperties pass.

I expect that doing that will allow us to fix this problem along with a lot of other ones.
I've marked the internal copy of this issue as blocked on our efforts to move the transpilation passes, so we won't forget to make sure this gets fixed as part of that effort.