google / closure-compiler

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

Extending Generic Classes in ES6 - No Errors/Warnings #822

Open ghost opened 9 years ago

ghost commented 9 years ago

I'm not sure if I have the JSCompiler Debugger setup correct but the following code doesn't result in any errors or warnings...

/** @template T */
class Foo {
  /** @param {T} value */
  constructor(value) {}
}

/** @extends {Foo.<string>} */
class Doo extends Foo {
  constructor() {
    super(100) // no error or warning for this?
  }
}

As far as I know, TypeScript supports generics so maybe that will be one solution. I imagine the code looking something like this...

class Foo<T> {
  constructor(value:T) {}
}

class Doo extends Foo<string> {
  constructor() {
    super("Hello")
  }
}
MatrixFrog commented 9 years ago

Currently we transpile the ES6 to some (admittedly verbose/ugly) ES5 code, involving Function.apply as I recall, and then we typecheck that ES5 code. So we can either:

1) Do a better job typechecking code that uses Function.apply 2) Transpile to ES5 code that can be typechecked better

I would imagine that the new type inference will do 1) but I'd have to check. Eventually, we'll want the typechecker to understand ES6 directly.

concavelenz commented 9 years ago

When using the Closure Libraries "base" method the call is rewritten into a direct call to the super class prior to type checking and optimizations. This should instead have been written:

Foo.call(this, 100);

Or for arbitrary super expressions "class Doo extends makeBase() {..."

/* @const / var DooBase = makeBase(); var Doo = function() { DooBase.call(this, 100); };

There is no need for this indirection, and it is unnecessarily slow (using both arguments and apply, both of which has a reasonably high cost, compared to a straight "call"). Unlike Closure's goog.base and ".base" methods which exist to make refactoring easier, no one is going to be editing the output of the ES6->ES5 translation.

MatrixFrog commented 9 years ago

We switched this to use Foo.call in 3566f49ce24887662777a04ba87817c200b79b9c but still no type error.

MatrixFrog commented 9 years ago

But we do generally typecheck Function.call (https://closure-compiler-debugger.appspot.com/#input0%3D%252F**%2520%2540param%2520%257Bnumber%257D%2520x%2520*%252F%250Afunction%2520f(x)%2520%257B%257D%250A%250Af.call(null%252C%2520%2522%2522)%253B%26input1%26conformanceConfig%26externs%26refasterjs-template%26includeDefaultExterns%3D1%26CHECK_SYMBOLS%3D1%26CHECK_TYPES%3D1%26LANG_IN_IS_ES6%3D1%26MISSING_PROPERTIES%3D1%26PRETTY_PRINT%3D1%26TRANSPILE%3D1) so I'm not sure why.

MatrixFrog commented 9 years ago

There are actually two problems here.

1) We put {Foo} in the @extends annotation that we generate during transpilation. We should put {Foo<string>} instead. 2) But even if we did that, we wouldn't get any type error: https://closure-compiler-debugger.appspot.com/#input0%3D%252F**%250A%2520*%2520%2540constructor%2520%2540struct%2520%2540template%2520T%250A%2520*%2520%2540param%2520%257BT%257D%2520value%2520*%252F%250Avar%2520Foo%2520%253D%2520function(value)%2520%257B%257D%253B%250A%250A%252F**%2540constructor%2520%2540struct%2520%2540extends%2520%257BFoo%253Cstring%253E%257D%2520*%252F%250Avar%2520Doo%2520%253D%2520function()%2520%257B%250A%2520%2520Foo.call(this%252C%2520100)%253B%250A%257D%253B%250A%250A%26input1%26conformanceConfig%26externs%26refasterjs-template%26includeDefaultExterns%3D1%26CHECK_SYMBOLS%3D1%26CHECK_TYPES%3D1%26LANG_IN_IS_ES6%3D1%26MISSING_PROPERTIES%3D1%26PRETTY_PRINT%3D1%26TRANSPILE%3D1

Perhaps the best solution is to have the typecheck know about super (and run before transpilation) which will probably happen eventually but not in the next few months.

teppeis commented 6 years ago

not fixed in v20180506

brad4d commented 6 years ago

We are actively working to move ES6 feature transpilation after type checking. ETA is July. That effort will enable fixing this issue, and could even end up fixing it along the way without extra effort.