Handle function overloads, perhaps by unifying signatures

[evmar]

Tsickle currently errors when generating externs from a declaration class with multiple constructors.
However, these exist in d.ts files. We need to figure out how to support it.

Example:
https://github.com/DefinitelyTyped/DefinitelyTyped/blob/56024034ccc426af2840e90fe94043c2213dd6b4/google.visualization/google.visualization.d.ts#L238

[mprobst]

When declaring the implementation of an overloaded function/method/ctor, you need to provide an implementation that takes the union of all parameters, and returns the union of all return types (and then TS trusts you that you return the right type for the right parameter set). Sounds like we might have to do this merge ourselves?

[evmar]

Some more instances of this problem:

• https://github.com/DefinitelyTyped/DefinitelyTyped/blob/56024034ccc426af2840e90fe94043c2213dd6b4/threejs/three.d.ts#L976
• https://github.com/Reactive-Extensions/RxJS/blob/29f85627e4ab71b050480f26dcc5d737f1b840f4/ts/rx.lite.d.ts#L254
• https://github.com/DefinitelyTyped/DefinitelyTyped/blob/56024034ccc426af2840e90fe94043c2213dd6b4/pixi.js/pixi.js.d.ts#L394

Doing the merge looks like it'd generate some pretty crazy types, especially in that first case.
But I don't see any way around it.

[mprobst]

Yes, the types are crazy. But OTOH, that really is the runtime type of the method without overloads, which is probably what Closure should see, right?

[evmar]

Under the rationale of "TypeScript has already type-checked", I might just do something like

/**
 * @constructor
 * @param {...*} args
 */
function Foo(args) {}

for now.

[evmar]

See for example
https://github.com/Microsoft/monaco-editor/blob/master/website/playground/monaco.d.ts.txt
which defines more than ten overloads for Promise.then.

[hess-g]

From my whiteboard notes on what the generator would need to cover:

• Maximum number of args would be the count of the function(s) with the largest count.
• All args would be "_opt" at and beyond the index greater than the minimum count across all functions.
• name "x_or_y" if different names at same index.
• {Tx | Ty} if different types at same index

And a few open questions:

• Some special naming convention for variadic?
• How to handle mix of nullable and not nullable at same ordinal index.

As for "TypesScript has already type-checked", that doesn't help in the "I'm still doing native JS dev" scenario, so as much fidelity as could be accomplished would be beneficial.

[evmar]

If you're interested in approaching this, I think the best place to start is to just take some of the examples we have in real code (linked in this bug) and add them to the test suite. To start with, we at least shouldn't generate invalid code.

[hess-g]

Unless someone else has a fix in flight, I'm going to look at this over the coming week. Agree that first step is to write a reasonably comprehensive set of valid ts --> expected extern test data.

[evmar]

The way the test suite works is you create a dir in test_files/ with some .ts files, and then run `UPDATE_GOLDENS=1 gulp watch' and it generates the outputs alongside the input. From there you can examine those outputs and adjust the code until it produces what you'd like. We can even check in goldens with "bad" output, just to capture the current state, so that your fixes show up as diffs to the golden output.

[evmar]

Another interesting one. Note the differing return types.

    /**
    * Loads the client library interface to a particular API. If a callback is not provided, a promise is returned.
    * @param name The name of the API to load.
    * @param version The version of the API to load.
    * @return promise The promise that get's resolved after the request is finished.
    */
    export function load(name: string, version: string): Promise<void>

    /**
    * Loads the client library interface to a particular API. The new API interface will be in the form gapi.client.api.collection.method.
    * @param name The name of the API to load.
    * @param version The version of the API to load
    * @param callback the function that is called once the API interface is loaded
    * @param url optional, the url of your app - if using Google's APIs, don't set it
    */
    export function load(name: string, version: string, callback: () => any, url?: string): void;

From https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/gapi/gapi.d.ts

[evmar]

Another ugly case: this project has a local typings file that defines a third signature for the above "load" function. At first glance it seems there is no way to make that work, unless we have a global view of all d.ts before generating any externs.

[hess-g]

Evan - good references. Per your comments on a separate thread, I'm going to create the golden set of tests, with a reasonable coverage from bland to ugly, and submit that as a pr later this week. (I'm caught up in some "real" work time-sensitive stuff today/tomorrow). I'll also post some shorthand pseudocode for the alg changes I'm going to make, for comment before I get into the weeds.

I agree that first and foremost this will deal with one class/interface in one file, not attempt to merge across multiple files. That can perhaps come later.

On the overloaded return value example: In that particular case, since closure's standards are to not annotate a void response, would you propose {Promise, undefined}, {Promise, void}, or something else? {?Promise} doesn't seem right, since the response is actually "undefined" not null.

[evmar]

I think @mprobst might take a shot at this on his flight on Thursday, be sure to sync with him.

[hess-g]

Question on instantiating type metadata at runtime: In approaching the overloads problem, I should be able to use the existing ClosureRewriter#emitFunctionType if I can either:

1. Create a new instance of tsSignatureDeclaration, via a builder of some sort; this new instance would have the merged set of parameter names, types and return types.
2. Create a text representation of the TS version of the merged function, then use some factory or compiler interface to generate the metadata.

If there is no easy way at runtime to dynamically build an instance of the declaration, then I'll have to perform some surgery on emitFunctionType, so as to separate the creation of the text for the tag elements (for the singular vs >1 overload variants) from the building of the tags themselves.

[evmar]

I haven't yet tried creating TS AST nodes (like SignatureDeclaration). I worry it might be hard because they have offsets into the text of the source file and other parts have symbols mapped into the typechecker, but maybe you can avoid that.

I think you'll likely need to take emitFunctionType apart, perhaps to accept an array of decls to merge, or perhaps to accept some intermediate data type (e.g. interface Function { name: string; retType: string; params: ... }) that you invent.

[hess-g]

That's where I'm headed.

[butterflyhug]

Is it a strict requirement that all functions must be annotated with @param and @return instead of @type {function()}? I know the latter is less readable, but I think it would be a more precise translation of the Typescript for function overloads.

For example, I'm wondering if we could translate the gapi example as follows:

/**
 * Loads the client library interface to a particular API. If a callback is not provided, a promise is returned.
 * or
 * Loads the client library interface to a particular API. The new API interface will be in the form gapi.client.api.collection.method.
 * @param name The name of the API to load.
 * @param version The version of the API to load.
 * @param opt_callback the function that is called once the API interface is loaded or undefined
 * @param opt_url optional, the url of your app - if using Google's APIs, don't set it or undefined
 * @return promise The promise that get's resolved after the request is finished. or undefined
 * @type {function(string, string):Promise<undefined>|function(string, string, function()=, string=):undefined}
 */
function load(name, version, opt_callback, opt_url) {}

In other words, we'd still follow all the rules outlined above except for "{Tx | Ty} if different types at same index", which would be replaced by the @type-style annotation that I'm demonstrating here.

[evmar]

@butterflyhug is that a valid Closure annotation? I've never seen it done this way.

[butterflyhug]

The compiler team's documentation lists the function type. It gets used all the time for callbacks and, less frequently, for type casts. I assumed that it would be fully supported in type unions, even though I haven't seen that usage either.

I just tested it. The compiler doesn't give any errors, but there's still two issues using my example with the Closure Compiler:

1. The compiler warns about incompatible annotations if we mix @param / @return with @type in the same JsDoc, even if @type is the only annotation with type information. We'd need to strip those JsDoc tags, which is another step away from standard style, but not the end of the world.
2. More importantly, the compiler doesn't recognize the @type annotation on a function declaration at all. It does recognize the annotation when assigning a function expression to a constant, but even then it's not type checking the function parameters correctly, so I suspect the annotation is getting silently dropped even then.

So, you're right, this isn't currently an option. Bummer.

[evmar]

That is too bad. The other thing I'd not expect to work is when you have some f of type function(...)|function(...) and you then use it f(arg1, arg2). I would think the compiler wouldn't know it should figure out of any of the union types match.

[evmar]

By the way, I think this bug is mostly done. It might be worth closing it and opening bugs on any specific remaining issues.

[evmar]

Calling this done.