angular/tsickle

Tsickle — TypeScript to Closure Translator

tsickle's Issues

parseInt has a different type in Closure

Closure defines parseInt as having a required second argument. I think this is to handle the common mistake of doing parseInt(user_input), which does a surprising thing (octal) when user input starts with a leading 0.

For sickle's purposes, we'll need to figure out something. Ideas:

• adjust the TypeScript typings for parseInt
• coerce parseInt to undo the Closure check (which is a little disappointing)

Closure Compiler doesn't understand TypeScript enums

switch untyped mode to emit @suppress {checkTypes} instead of @type {?}.

This means a lot less modifications of source and less opportunities to create invalid TS code though that.

Support array destructuring

eg.

function f([x,y]: number[]) {}

See #96 where I started to do this, but needs more work because number[] must be represented as Array<number> in the closure typedef.

Sickle chokes on decorators (likely due to a bug in TS < 1.8)

Input:

class DecoratorTest {
  @foo
  private x: number;

  constructor() {}
}

causes a crash due to a failed assertion about offsets.

With some debugging, I found that the resulting syntax tree when parsing this input is bogus. Note how the AtToken shows up twice.

| | | | node: PropertyDeclaration
| | | | | node: AtToken
| | | | | node: SyntaxList
| | | | | | node: Decorator
| | | | | | | node: AtToken
| | | | | | | node: Identifier
| | | | | node: SyntaxList
| | | | | | node: PrivateKeyword
| | | | | node: Identifier

Upgrading our TypeScript dep to typescript@beta (1.8) produces

| | | | node: PropertyDeclaration
| | | | | node: SyntaxList
| | | | | | node: Decorator
| | | | | | | node: AtToken
| | | | | | | node: Identifier
| | | | | node: SyntaxList
| | | | | | node: PrivateKeyword
| | | | | node: Identifier

which is a layout I expect.

"static" doesn't interact well with stub decls

class Foo {
  static bar = 3;
}

produces

class Foo {
  static bar = 3;constructor() {


// Sickle: begin stub declarations.


this.bar;

And the TypeScript compile dies due to no this.bar.

"declare module" crashes

declare module Foo { enum Bar { ...

after sickle we produce the TypeScript code

declare module Foo {
  [...]
/** @type {Bar} */
(<any>Bar).BAZ =  1;
}

and you can't have statements in a declared module. I think the workaround is to skip them in this case but I don't really know what the original code is trying to do...

Generate structurally typed interfaces for classes (?)

TypeScript uses structural type compatibility throughout, e.g.:

class Foo { bar: string; }
class Baz { bam: string; }
var f: Foo = new Baz(); // legal as all properties match

In Closure, that's only allowed for structural interfaces. While undocumented, according to the code this should be triggered by:

/** @interface @record */
my.StructuralInterface = function() {};

It should be possible to translate all TypeScript types to structural interfaces (for interface) or pairs of interface and class for classes:

interface MyIf {}
class MyClass {}

would generate:

/** @interface @record */
function MyIf() {};
/** @interface @record */
function MyClass_StructuralInterface() {};
/** @constructor @implements {MyClass_StructuralInterface} */
function MyClass() {};

Whether it's more convenient/safer/easier to implement to either invent a new name for the interface, or to invent a new name for the class, and either change the name in all type-positions, or change the name in all value-positions, is unclear at this point, both would probably work.

Translate "declare" and d.ts files as externs.js

We should generate Closure externs automatically from their TypeScript equivalents.

Annotations on parameter properties show up in wrong place

See "parameter properties" in this:
http://www.typescriptlang.org/Handbook#classes-privatepublic-modifiers

Code like

class Foo {
  constructor(/** @export */ public bar: string) {}
}

needs to have us move the @export declaration onto the stub decl and remove it from the parameter list.

Annotate class methods

class Foo {
  bar(x: string, y?: string) {}
}

generates

class Foo {
  bar(x: string, y?: string) {}
// Sickle: begin synthetic ctor.
constructor() {
[... more elided...]

We don't annotate the function for some reason? We need to do so for the optional argument.

De-reference all properties in `export * from ...`

When compiling export * from 'foo'; to CommonJS module format, we end up with code like this:

function __export(m) { for (var k in m) exports[k] = m; }
__export(require('somemodule'));

Now surprisingly this works in Closure Compiler when replacing require with goog.require. It simply patches the exports to be the module name (which is a global symbol) and then the right properties end up on the right objects (yay!).

However when importing a property that was re-exported like this, Closure Compiler complains about a missing property. It's possible to shut that off (--jscomp_off=missingProperties), but that seems overall dangerous - for example, dead code elimination might kill that property.

If sickle was to "de-reference" and re-export the particular symbols, this would avoid the problem for the time being, and allow us to continue using the hacky CommonJS --> goog.require transformation without having to rely on ES6 modules.

An alternative, possibly cleaner, solution would be to convert all module imports to goog.module and goog.require within sickle (including the * dereferencing). This would break the second compilation pass - TypeScript would no longer understand all those imports. However we have already type checked, so at that point we could just ignore all errors and emit.

WDYT? @martine @rkirov

Enum written with incorrect reverse map

from angular2/github/modules/angular2/src/compiler/url_resolver.ts

enum _ComponentIndex {
  Scheme = 1,
  UserInfo,
  Domain,
  Port,
  Path,
  QueryData,
  Fragment
}

TS/sickle produces this ES6 code:

var _ComponentIndex;
(function (_ComponentIndex) {
    _ComponentIndex[_ComponentIndex["Scheme"] = 1] = "Scheme";
    _ComponentIndex[_ComponentIndex["UserInfo"] = 2] = "UserInfo";
    _ComponentIndex[_ComponentIndex["Domain"] = 3] = "Domain";
    _ComponentIndex[_ComponentIndex["Port"] = 4] = "Port";
    _ComponentIndex[_ComponentIndex["Path"] = 5] = "Path";
    _ComponentIndex[_ComponentIndex["QueryData"] = 6] = "QueryData";
    _ComponentIndex[_ComponentIndex["Fragment"] = 7] = "Fragment";
})(_ComponentIndex || (_ComponentIndex = {}));
_ComponentIndex.Scheme = 1;
_ComponentIndex.UserInfo = 0;
_ComponentIndex.Domain = 1;
_ComponentIndex.Port = 2;
_ComponentIndex.Path = 3;
_ComponentIndex.QueryData = 4;
_ComponentIndex.Fragment = 5;

Causes a runtime failure, we have
["components/youtube_app.html", undefined, undefined, undefined, undefined, "components/youtube_app.html", undefined, undefined]
and then parts[_ComponentIndex.Path][0] == '/'
fails because _ComponentIndex.Path is 3 rather than 5.

Reproducible Setup to run Sickle on Angular2

... followed by Closure Compiler to verify the result.

This is mostly as a guinea pig to have a way to verify fixes and generate new issues.

Transpiling causes confusing line numbers in output

When using sickle, the compiler error messages you get are relative to the sickle output, which means that file offsets are wrong.

One idea is to ensure that sickle output has output lines in the same place as input lines, which means generating code that (for example) doesn't use newlines. That at least makes line numbers line up.

But in cases where we generate a synthetic constructor there's not a particular line in the source we are modifying, so we may be forced to add a new to the source. But maybe even then we could do a rule like "stuff all synthetic code in one line in the very bottom of the class, right before the closing curly brace".

add goldens for different steps of the pipeline

We have the following pipeline:

.ts -> (sickle) -> .ts with annotations -> (ts compiler) -> .js with annotations -> (closure compiler) -> minified .js

We currently have goldens only for ".js with annotations". It would be helpful to catch bugs earlier with goldens for ".ts with annotations" and maybe also goldens for "minified .js".

enums in modules can't be typed

module Foo { enum Bar { ...

eventually produces something like

(function (Foo) {
    /** @typedef {number} */
    var Bar;

But you apparently can't (?) put a @typedef within a scope like that, it has to be top level (?).

returning an arrow function triggers ASI

function foo() {
    return (x: string) => 3;
}

produces

/**
 */
function foo() {
    return 
/**
 * @param { string} x
 */
(x: string) => 3;
}

Because we inserted the newline after the return, it triggers ASI and you get

error TS7027: Unreachable code detected.

Don't remove typedefs

When we visit a type alias, e.g. https://github.com/angular/sickle/blob/master/src/sickle.ts#L146

We still need to emit the TypeScript code, otherwise the type is lost. At the time I wrote that code I forgot that we're generating TypeScript, not plain JS.

Stub decls must appear after "super" call

class Foo {
  constructor(public x: string) {
    super(3);
  }
}

becomes

class Foo {
  constructor(public x: string) {

// Sickle: begin stub declarations.

 /** @type { string} */
this.x;
// Sickle: end stub declarations.

    super(3);
  }
}

It's illegal in TS to have code before the super call apparently.

Synthetic ctor needs to call base class ctor with proper arguments

class SuperTestBase {
  constructor(public x: number) {}
}
class SuperTestDerivedNoCTor extends SuperTestBase {
  foobar: number = 3;
}

Sickle generates a constructor for the derived class here, to have a place to declare the type of foobar.

But the existence of a constructor means that we need to call the constructor of the base class, which means we need to be able to reconstruct the constructor parameters as well as the parameters to super.

I think this means we might need to store a map of classes to the expected parameters of the ctor, which might even mean needing to do cross-file analysis... :~(

I tried a hack like this but it didn't go. I might be able to turn off some type checking and force it though.

constructor(...args: any[]) {
  super(...args);

Support command-line build workflows

Users other than Google will probably want some sort of sickle executable that they can apply to their source to get Closure output.

Perhaps Sickle should even go as far as being responsible for executing Closure on their behalf -- so you just take your TypeScript output, give it to Sickle, and you get a minified binary out.

emitting declared symbols from modules into a global externs is wrong

currently, we collect all declared namespace and emit them as externs.

// in a.ts
export var a = 0; // so this is indeed a typescript module.
declare var window: {};
declare namespace angular { ... }

sickle outputs var window and var angular in externs.js. However, TS declarations in modules are scoped to the module similarly to plain old JS variable declarations (var foo). So it would be valid TS to have again:

// in b.ts 
export var a = 0; // so this is indeed a typescript module.
declare var window: {};
declare namespace angular { ... }

but sickle output would be wrong. I can't come up with a clean solution here, we are trying to merge multiple scopes (every module is one), into one, and the only solution is renaming, which is going to be PITA.

@martine , @mprobst Ideas?

fully support untyped mode

As per our conversation with JS compiler folks, most advanced optimizations do not need types (only disambiguate/ambiguate props do). Sickle can fully support an advanced optimization consumer once we have the following:

• turn off closure warnings/errors on valid code that is minification safe. #59
• support a driver that does typechecking first, then sickle run, finally an emit (no typechecking).
• detect minification unsafe code that passes the TS typechecker. #23

/cc @martine

JSDoc comments need an extra newline

class Foo {
  /** js-doc comment with text in it */
  frotz: number;
}

becomes

class Foo {
  constructor() {
        /**  js-doc comment with text in it @type { number} */
        this.frotz;
    }
}

which is invalid Closure syntax -- we need a newline before that @type bit.

Unnamed object parameters

index (1) must be less than size (1)
  Node(PARAM_LIST): /javascript/angular2/github/modules/angular2/src/core/di/provider.js:69:15
    constructor(token, { useClass, useValue, useExisting, useFactory, deps, multi }) {
  Parent(FUNCTION ):/javascript/angular2/github/modules/angular2/src/core/di/provider.js:69:4
    constructor(token, { useClass, useValue, useExisting, useFactory, deps, multi }) {

    at com.google.common.base.Preconditions.checkElementIndex(Preconditions.java:1295)
    at com.google.common.base.Preconditions.checkElementIndex(Preconditions.java:1277)
    at com.google.common.collect.SingletonImmutableList.get(SingletonImmutableList.java:41)

javascript/angular2/github/modules/angular2/src/core/di/provider.js looks like

/**
     * @param token
     * @param { ?} {useClass, useValue, useExisting, useFactory, deps, multi}
     */
    constructor(token, { useClass, useValue, useExisting, useFactory, deps, multi }) {

I don't think that name for parameter 2 is legal.

This is the next blocker for compiling angular2 with closure compiler.

cannot annotate rxjs

fails here:
https://github.com/ReactiveX/rxjs/blob/1104af1c9996662eba51d9f0209d4924b5ae6ed4/src/operator/combineLatest.ts#L26

with compiler should have enforced array type for rest param

reorganize closureExternsBlacklist

from @rkirov

This list needs to be more structured (we can keep hacking on it for now). Here is what I did in clutz:

fix which files are considered "platform apis" on both sides:

lib.d.ts for TS (TSsymbols)
closure/externs:COMMON (ClosureSymbols)
modify lib.d.ts to be a strict subset of closure externs

enumerate symbols in missingPlatformSymbols = ClosureSymbols \ TSSymbols ( \ is set difference)
clutz now emits everything not in closure externs + missingPlatformSymbols.

Beyond "platform externs", I don't think the tool should be doing any white/black listing, as we can't have any visibility into what those symbols are. It won't scale to keep adding them here.

Don't create redundant param declarations when original source already has jsdoc

Input:

/**
 * @param x
 */
function foo(x: string) {}

currently generates

/**
 * * @param x
 * @param { string} x
 */
function foo(x: string) {}

which fails with an error like "Bad type annotation. duplicate variable name".

Use ES6 sets and maps

Would let us drop all the hasOwnProperty stuff.

Depends on what version of node we depend on.

Non-exported enums not accepted by Closure

The first enum below is exported, the second is not.

Closure rejects the second one with
ERROR - The alias Foo is assigned a value more than once.

goog.module('foo');
(function (ErrorType) {
    ErrorType[ErrorType["NONE"] = 0] = "NONE";
    ErrorType[ErrorType["PERMANENT"] = 1] = "PERMANENT";
    ErrorType[ErrorType["RETRYABLE"] = 2] = "RETRYABLE";
    ErrorType[ErrorType["NOT_FOUND"] = 3] = "NOT_FOUND";
})(exports.ErrorType || (exports.ErrorType = {}));
var ErrorType = exports.ErrorType;
ErrorType.NONE = 0;
ErrorType.PERMANENT = 1;
ErrorType.RETRYABLE = 2;
ErrorType.NOT_FOUND = 3;



var Foo;
(function (Foo) {
    Foo[Foo["BAR"] = 0] = "BAR";
})(Foo || (Foo = {}));
Foo.BAR = 0;

John Lenz says: "The goog.module support is being rewritten and the behavior here is likely to change. As is this is simply violation of the current restriction on goog.module top level declarations"

Sickle eats JSX angle brackets

let x = <div>
  <FooComponent/>

becomes

let x = <div>
  <<FooComponent/>

Constructable types

var ctorType: {new(a: number): Foo};

should translate to:

var /** function(new:Foo, number) */ ctorType;

However that format does not support additional properties on ctorType. Apparently this is supported by Closure's internal representation of the type, but there's no syntax for it.

Translate TypeScript Object Types (aka type literals) to Closure structs

E.g.

var litType: {someField: number};

Should become:

var /** {{someField: number}} */ litType;

Handle abstract methods

abstract class Base {
  abstract foo(): void;
  bar() { foo(); }
}

TS drops the decl of foo when generating ES6, but we need to keep it around to make Closure happy.

Preserve TypeScript type coercions in Closure code

Input:

function foo(str: string) {}
foo(<string>JSON.parse('"foo"'));

Outputs:

// @param {string} str
function foo(str) { }
foo(JSON.parse('"foo"'));

We drop the <string> so Closure doesn't know about the type cast, producing a warning.

Default param arguments need to be annotated as optional

function foo(x: number, y: string = 'hi')

should produce something like

function foo(/** number */ x, /** string= */ y = 'hi')

Note the equals sign on the 'y' arg. This is because it's optional for the caller to provide it.

Interface should accept a ts.SourceFile, not a ts.Program

We don't want to worry about the compile flags etc. that a ts.Program needs for construction.

Merge existing closure annotations with output ones

For example for angular1 templates, we can have a field with export annotation:

class Ctrl {
  /** @export */
  foo: string;
}

sickle needs to output

/** @export @type {string} */
this.foo;

Optional arguments need to propagate into Closure type

function OptionalArgument(x: number, y?: string) {

currently does

function OptionalArgument(/** number */ x, /** string */ y) {

but we need y to be string=.

Interfaces disappear in generated code

Two issues:

• interfaces are not generated at all
• no implements clauses

Externs: support "declare class" followed by "declare namespace"

es6-promise does this:
https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/es6-promise/es6-promise.d.ts

Note that they must be in this order; the compiler enforces it.

That means to emit we just have to avoid stepping on the class when emitting the module.

"export enum" with comment is pretty hard to annotate

We try to emit a @typedef for enums when we hit them.
Below is the tree of the various nodes and where the emits happen when parsing something like

// comment here
export enum ...

Note that we emit the @typedef before we hit the ExportKeyword, which means it'll be hard to make the @typedef appear directly adjacent to the enum in the output. I'm beginning to think stripping comments by default is a better strategy...

| | node: EnumDeclaration
| | | emit "/** @typedef {number} */\n"
| | | node: SyntaxList
| | | | node: ExportKeyword
| | | | | emit "\n\n// This additional exported enum is here to exercise the fix for issue #51.\nexport"
| | | node: EnumKeyword
| | | | emit " enum"

Export of enum typedef doesn't pass e2e test

If we generate code like the following, it fails to compile in Closure:

/** @typedef {number} */
export var EnumTest2;
(function (EnumTest2) {
    EnumTest2[EnumTest2["XYZ"] = 0] = "XYZ";
    EnumTest2[EnumTest2["PI"] = 3.14159] = "PI";
})(EnumTest2 || (EnumTest2 = {}));
/** @type {EnumTest2} */
EnumTest2.XYZ = 0;  // <- ***fails here***
/** @type {EnumTest2} */
EnumTest2.PI = 3.14159;

The error message:

[...] ERROR - Bad type annotation. Unknown type EnumTest2
/** @type {EnumTest2} */
           ^

Something about export and typedef interacting poorly maybe?

The other weird thing is that I can't reproduce this with the Google-internal JS compiler, which makes me wonder if it's a bug.

sickle should strip all comments

in file_comments.ts we emit jsdoc before existing comment:

/** @return { string} */ // This test verifies that initial comments don't confuse offsets.
 function foo() {
     return 'foo';
 }

we should strip all comments, since we already don't emit comments in properties (see comments.ts golden).

Propagate rest (aka ...) params into types

Even in untyped mode, a function like

function foo(...args: string[]) 

becomes

/**
 * @param { ?} args
 * @return { ?}
 */
function foo(...args: string[]) 

which Closure doesn't accept: "WARNING - Missing "..." in type annotation for rest parameter."

Warn when properties are accessed through ['literal']

Typescript allows property access with square brackets and string literal to access regular properties (ref https://github.com/Microsoft/TypeScript/blob/master/doc/spec.md#4.13)

so the following is valid TS

class A {
  foo: string;
}

var a: A;
var b = a['foo'];

but this will fail spectacularly post minification with closure.

Support untyped mode in enums

When emitting an enum in untyped mode, we still want to emit some of the glue code we currently emit, but we shouldn't emit any of the types.

See this test output:
https://github.com/angular/sickle/blob/b4c9b8e32c2922626c83a69dbfbef5718272aeeb/test_files/sickle/enum.ts

We should preserve the (EnumTest1).XYZ = 0; bit, but drop all the closure annotations.

Update project description on Github, still says "Tickle"

It looks like maybe this project used to be named Tickle - and it still says that on the Github project description shown prominently. "Tickle - TypeScript to Closure Annotator"

This issue is a reminder to eventually adjust that.

TypeScript ES6 decorator emit leaves class in a position closure doesn't re-write

eg
blaze-bin/third_party/javascript/angular2/github/modules/angular2/src/web_workers/shared/serializer.js
contains

let Serializer = class {
...
};
Serializer = __decorate([
    di_1.Injectable(),
    __metadata('design:paramtypes', [render_store_1.RenderStore])
], Serializer);
exports.Serializer = Serializer;

With language_out=ES5, closure still leaves class keywords in the final JS.

//third_party/javascript/angular2/github/modules/angular2/src/web_workers/shared/serializer.js
goog.loadModule(function(exports) {'use strict';goog.module('angular2$src$web__workers$shared$serializer');
...
let Serializer = class {
...

This breaks us, even in Chrome where class is supported, because it disallows using call or apply on the constructor function of a class.