Support export * from '..' pattern from declaration files

[theseanl]

As a first step in full module d.ts support, I attempted to provide support for export * from statements in externs generation. I would appreciate any feedbacks.

Changes

When there are export * from '...' statements in a file, generate externs on a different namespace, similar to what is done for export = '...' statements. The original namespace is declared as having a union type of each of namespaces on which exported members of re-exported modules are defined. For example,

/// A.d.ts
export * from 'B';
export * from 'C';

would generate:

/** @const */
var mangled$A_ = {};
/** @type {(typeof mangled$B|typeof mangled$C|typeof mangled$A_)} */
var mangled$A;

Considerations

As I understand, the original namespace's type would be more like an intersection type than a union type, but since closure type system does not have intersection types, and according to https://github.com/google/closure-compiler/wiki/Types-in-the-Closure-Type-System#union-types,

a type union is considered to have a property x if any of the types in the union have such a property.

so I suppose the compiler will be able to find all the property on such union types, as it would find on intersection types.

A change made is that now generateExterns depends on the full program, in order to check whether other files that it reexports actually have generated namespaces.

An edge case that might be an issue is when multiple reexported module declares the same name. As I understand, this is not forbidden in ES module specification, and the one exported later overrides the former. I do not know how closure compiler would behave in such cases. But I guess no one would intentionally write such code and it'd be possible to fix it from .d.ts side, so it would not be a big issue.

Another way I've considered is to make such mangled namespace object implement some interfaces, which as I understand is a preferred way of writing externs according to google/closure-compiler#2516 (comment). Then the type of the root namespace could be a union type of such interfaces.

[evmar]

Can you describe how you're verifying whether this works? Another way of asking that is, what problem is this fixing for you, and how do you evaluate whether this fix fixes that problem?

In some other place we implement export * by traversing all the exports of the module and writing aliases for them. Would an approach like that work here? It seems it would need less magic.

[theseanl]

I'm having compiling some user code using tensorflow/tfjs as an ultimate test in mind, but there are other issues that is preventing it, so I'm currently just trying with simple toy cases with hand-written externs, and checking whether closure compiling it does not mangle properties defined in externs and does not produce warnings. I've added one such case as a golden test, I'll check if I could make it more robust.

The main reason I chose this way was that it seems to be requiring the least changes to the existing code which I do not know well. I see that traversing re-exported module would work well too. But that way would produce much larger externs file when there are multiple export * from '...' chains (which do occur in tensorflow/tfjs), I wonder if it could make transpilation time significantly worse.

[theseanl]

Regarding this approach's magic, I think if tsickle changes the way it generates externs to generate one big interface per file and declare namespace objects as having type of the interface, I suppose it can express intersection types in more straightforward way via multiple @extends.

[theseanl]

I've tried to create a clear-cut example which this patch would fix, but it turned out to be hard.

It's partly because what tsickle depends on undocumented behavior of closure compiler. See the above mentioned issue google/closure-compiler#2132. Because this pattern is what tsickle generates, even if tsickle does not properly communicate type information to closure compiler, broken externs still prevents renaming of some properties altogether.

I hope this situation is improved, and in this regards, I think tsickle should generate externs in a form recommended in the issue above (a big interface for each files), for better minification result. And once this is done, the current state of skipping export * from '...' may cause visible effect (or when closure compiler fixes the issue 2132).

[evmar]

I tried tinkering with the big namespace per file idea, and created files like

/** @externs */

const ns1 = {};
/** @record */
ns1.X = function() {}
/** @type {string} */
ns1.X.prototype.foo;

const ns2 = {};
/** @record */
ns2.Y = function() {}
/** @type {string} */
ns2.Y.prototype.bar;

/** @type {typeof ns1|typeof ns2} */
let ns;

goog.module('demo');

ns.X = 3;
ns.Q = 3;

/**
 * @param {ns.X} x
 * @param {ns.Y} y
 */
function f(x, y) {
  x = y;
}

But the latter file seems to let me assign anything to fields of ns (like the nonexistent Q) and doesn't think ns.X or ns.Y exist. What did I do wrong?

[theseanl]

What I was suggesting is a big interface per file.

[edit: removed a comment which turned out to be irrelevant]

[evmar]

I am not too great on the details here (I don't think Closure thinks about interfaces and namespaces the same way as TS does) but I am not sure if a big interface per file can have interfaces as fields. That is, in my above code I was attempting to simulate what should happen when you merge a library with an "X" export with another with a "Y" export.
(I tried adding @const and it didn't seem to change the example, but it was a good idea...)

[theseanl]

I am not sure if a big interface per file can have interfaces as fields

I hope there is a way to do that.

I don't understand what your example is testing -- is it about this PR or about the new idea of interfaces per file?

[evmar]

Both, I thought? I was trying to understand what this PR does by making a small example of the code pattern is relies upon, but I couldn't seem to make it work. I think maybe I misunderstand what this is doing.

[theseanl]

I now understand your example. You are right that ns.X and ns.Y are not recognized as types, thank you. Maybe typeof ns only holds values defined on ns, not types.

However, I think that such a code pattern cannot be generated from tsickle. When tsickle generates types, it seems to be always resolving to the actual place where a symbol is defined, so it will be

/**
 * @param {ns1.X} x
 * @param {ns2.Y} y
 */

, I guess this is how ts API works.

Regarding ns.Q = 3, I thought this isn't meant to be caught? I tried replacing it with ns1.Q = 3 and it still does not cause warnings. But if I instead do ns.Q, it correctly triggers a warning that the property is never defined.

[theseanl]

(Edit: moved to #1223)

[theseanl]

Hi, upon further consideration I've noticed that the aforementioned premise

it seems to be always resolving to the actual place where a symbol is defined

is not abided in externs generation. Fortunately it seems to be working on normal .ts files transformation.

So I fixed it from externs.ts, by modifying addImportAlias to use the similar logic used for normal .ts file transformation. (Modifying addImportAlias seems to be necessary anyway for further works to fix other module dts problems) The fix was quite simple. I've also modified golden tests to check how such parameter types are translated. Could you check? @evmar

Now there is a subtle problem. As I've checked, this (typeof ns) approach only seems to be working when externs are concatenated in a suitable order.

/** @const {typeof B} */    var A;
/** @const {typeof C} */    var B;
/** @const */               var C = {};
/** @type {number} */       C.C;

produces a compiler warning,

/** @const */               var C = {};
/** @type {number} */       C.C;
/** @const {typeof C} */    var B;
/** @const {typeof B} */    var A;

works, as expected.
But surprisingly the following works too.

/** @const {typeof B} */    var A;
/** @const */               var C = {};
/** @type {number} */       C.C;
/** @const {typeof C} */    var B;

It would only be reliable when externs are concatenated in some topological order. This is provably possible when there's no circular dependencies among export *s, which is not unexpected, as tsickle already does not support circular imports in normal .ts files. This is a not a problem for tscc, also if there is not much export * being used in other places where tsickle is being used, such a restriction shouldn't cause much issues. I made the test pass based on a behavior of node-glob returning paths in a lexicographic order.

[theseanl]

Is there anything that I can do to help the review process? I intend to provide more patches for other module-dts issues (visible ones) building on top of this and #1221.