void and undefined confusion with incremental clutz

[rkirov]

The following snippet of closure:

/** @return {string | undefined} */
MyI.prototype.foo

/** @override */
MyClassThatImplementsI.prototype.foo = function() {
  // no return;
}

When run over the class and interface separately produces:

declare interface I {
  f(): string | undefined;
}

declare class C implements I {
  f(): void;
}

That is an error in TypeScript, because 'void' is not 'undefined' assignable. One answer is to emit 'string | void' for the interface too. Generalizing, if the type is union in a return position it need to use 'void' instead of 'undefined'. Note that as far as I can tell closure does not differentiate void and undefined (they are pure type synonims).

[evmar]

let x = someMyI.foo();

is legal JS code at runtime, and probably the intended use of this API, so I think returning void is not what we want.

Rather, I think this is just another case of inference where we should add an annotation on the subclass.

[rkirov]

If the input is:

/** @interface */
var MyI;

/** @return {string | undefined} */
MyI.prototype.foo = function() {};

/** @constructor */
var MyClassThatImplementsI;

/** @return {undefined} */
MyClassThatImplementsI.prototype.foo = function() {
  // no return;
}

exports.MyI = MyI;
exports.MyClassThatImplementsI = MyClassThatImplementsI;

we still emit:

declare namespace ಠ_ಠ.clutz.module$exports$bare$reexport {
  class MyClassThatImplementsI extends MyClassThatImplementsI_Instance {
  }
  class MyClassThatImplementsI_Instance {
    foo ( ) : void ;
  }
  interface MyI {
    foo ( ) : string | undefined ;
  }
}
declare module 'goog:bare.reexport' {
  import alias = ಠ_ಠ.clutz.module$exports$bare$reexport;
  export = alias;
}

Because, we convert undefined in a return position to void to be more TypeScript-y. Maybe that was a mistake, and we should just emit undefined uniformly :/

[rkirov]

And to answer my question - "why this works in total mode" - it works because with plainly @override closure reports the interface type (not the inferred type).

[LucasSloan]

One way to solve this would be to add void to methods that have type unions that include undefined. Thus

/** @return {string | undefined} */
MyI.prototype.foo

Would be translated to:

declare interface I {
  f(): string | undefined | void;
}

And implementing that interface with a void method would be fine.

[rkirov]

I have reservations because of this situation. Users might already have fn that accept string | undefined and when going through closure they will get assignability errors, that cannot trace back to the original .js source.

declare interface I {
  f(): string | undefined | void;
}

function someOtherF(x: string | undefined) {

}

function f(x: I) {
  someOtherF(x.f());  // type error here
}

[rkirov]

Two other answers here are:

• swallow error in users' .ts code with 'any'
• replace '@override' with an explicit type annotation, because TypeScript does not have anything like @override. If one redefines a method, they have to retype all the types (otherwise they are any).

I am leaning towards the second answer.

[evmar]

Fundamentally if you have the JS equivs of these in different compilation units:

class A { foo(): A|undefined { ... } }

class B { /** @override */ foo() { return undefined; } }

Then there's no way we can rely on inference to pick the proper return type of foo for B. That is an argument for any I guess, separate from the void return problem.

[rkirov]

Sorry, I am not following, isn't the correct type output:

class A {
   foo(): A|undefined {...}
}
class B extends A {
   foo(): undefined;
}

This is valid TS, a subclass can change the return type as long it is a subtype (functions are covariant in the return position).

[evmar]

Ah you are right. It's only if foo has no return statement and we infer void where this will break.

[rkirov]

Yep, which means one answer could be add 'return undefined', which has the same downside that some of these cases are in libraries that oppose changes just so that they are consumed by clutz.