Introduce transformer support for tsickle #516

tbosch · 2017-06-14T21:58:36Z

This PR introduces transformer support for tsickle.
The first 6 commits are prefactorings, which are used by the last commit which introduces the main API.

Global TAP presubmit is looking good using the new transformer API in G3.

Proposed way of landing this PR:

Review this PR and land it (hopefully mostly as is) on master
Continue to use the old entry point in G3 which is still fully functional
Use the new entry point in G3.

This way, we can have a bit of time between 2. and 3., without blocking other fixes for Tsickle. We are also able to rollback to using the old entry point without creating branches of Tsickle.

tbosch · 2017-06-15T22:03:23Z

Note: This still has some G3 failures, I am working on them. So it's probably better to wait with the review of my PR until I am really sure G3 is green.

For: - getters - for properties with comments. Refactors and simplifies the emit logic. Fixes bug that prevented closure types for variable declarations as soon as the variable statement had a jsdoc comment.

This is required by closure ES6 to not produce an error. Note: In the current version of tsickle, type comments in interpolations are elided because of angular#411. In the upcoming transformer version, this is fixed though.

This change was introduced for the upcoming transformer version, but this is no longer needed. Introduced by angular@711cfa2. (that commit was missing some files though…)

These test cases were reproductions of cases where the upcoming transformer version of tsickle failed in G3.

- emit more values via `Rewriter.writeRange` so that we can later on map these ranges back to nodes. - New Feature flag `FilterTypesInExportStart`: as the generated code does no longer have TypeScript typechecker information and therefore typescript does not elide exports for types. - New Feature flag `SimpleEnum`: as the changed enum declarations also no longer have TypeScript typechecker information and thefore typescript no longer generates `export.` for all property accesses to the exported symbols. - New Feature flag `TypeDefReexportForInterfaces`: as the .d.ts is generated before running tsickle, and therefore does no longer contain the reexports for the interfaces, even if tsickle changes them into functions.

Allow to: - synthesize and change any comment - work around TypeScript bugs when using synthesized nodes (which are created in any transformer that changes things).

API (see `transformer.ts`): The method `emitWithTsickle` allows to emit any program with tsickle. I.e. this is a replacement for using the `TsickleCompilerHost`. Implementation using existing `DecoratorClassVisitor` and `Annotator`: (see `transformer_sourcemap.ts`): Parses the produced sources back into a `ts.SourceFile` and establishes the links to the original nodes via the information given to the `SourceMapper` interface. Tests via goldens: The transformer version of Tsickle emits slightly different JavaScript (see the previous commit). Therefore, some goldens are slightly different. We use patch files to assert the differences to the regular goldens. Tests for SourceMaps: TypeScript automatically produces source maps for all changes that the transformer does. However, some applications (e.g. Angular) rely on the feature of tsickle to merge source maps that were already present in the input TypeScript code. Therefore, the transformer version still has to implement this merging.

evmar · 2017-06-29T17:09:23Z

src/decorator-annotator.ts

-    }
-    pos = classDecl.getLastToken().getFullStart();
+  if (rewriter.file.text[classDecl.getEnd() - 1] !== '}') {
+    rewriter.error(classDecl, 'unexpected class terminator');


Should we "return" here to avoid the rest of the function?

evmar · 2017-06-29T17:11:32Z

src/rewriter.ts

@@ -84,13 +84,19 @@ export abstract class Rewriter {
    this.writeNodeFrom(node, pos);
  }

-  writeNodeFrom(node: ts.Node, pos: number) {
+  writeNodeFrom(node: ts.Node, pos: number, end = Number.MAX_SAFE_INTEGER) {


Can write:

writeNodeFrom(node: ts.Node, pos: number, end = node.getEnd()) {

and then drop the Math.min() below as well as calling node.getEnd().

evmar · 2017-06-29T17:11:49Z

src/tsickle.ts

@@ -659,7 +660,11 @@ class Annotator extends ClosureRewriter {
        // Only emit a type annotation when it's a plain variable and
        // not a binding pattern, as Closure doesn't(?) have a syntax
        // for annotating binding patterns.  See issue #128.
-        if (varDecl.name.kind === ts.SyntaxKind.Identifier) {
+        // Don't emit type annotation when the variable statement is a @polymerBehavior,
+        // as otherweise the polymer closure checker will fail.


typo: otherwise

evmar · 2017-06-29T17:11:56Z

src/tsickle.ts

-        if (varDecl.name.kind === ts.SyntaxKind.Identifier) {
+        // Don't emit type annotation when the variable statement is a @polymerBehavior,
+        // as otherweise the polymer closure checker will fail.
+        // TODO(tbosch): file an issue for this!


Fix this with a link to the issue?

I think for now I will keep the comment as filing an issue takes some time...

Actually, I think I should file an issue against tsickle to create issues for typescript. This way we don't forget to do this.

Created #529 to track this and linked in the code.

evmar · 2017-06-29T17:14:05Z

src/tsickle.ts

+        // See comment above.
+        if (this.templateSpanStackCount > 0) {
+          this.emit('(');
+        }
        this.emitJSDocType(nnexpr, undefined, type);
        // See comment above.
        this.emit('((');


I vaguely suspect that this double-paren is trying to work around the same(?) issue you are. It's pretty weird that we emit three layers of parens here. It's maybe not worth trying to undo in your change, but maybe you could add a TODO?

We still need the double paren thing for same cases sadly (I saw it via the golden tests) :-(

Updated the docs there.

evmar · 2017-06-29T17:16:41Z

src/decorator-annotator.ts

@@ -130,6 +133,26 @@ export class DecoratorClassVisitor {
    this.propDecorators.set(name, decorators);
  }

+  private getValueIdentifierForType(typeSymbol: ts.Symbol): ts.Identifier|null {


Can you add docs on this? What is a value identifier (or value declaration)? Something about literal types?

For lowering decorators, we need to refer to constructor types. So we start off with the identifiers that represent these types. However, we want to emit them in a value position. TypeScript does not allow to use the same ts.Identifier here. Instead, this method looks for the place where the value that is associated to the type is defined and returns that identifier instead.

evmar · 2017-06-29T17:17:28Z

src/tsickle.ts

+   */
+  FilterTypesInExportStart = 1 << 1,
+  /**
+   * emit enums as


Can you comment here why you'd want to pass this flag or not? I recall one of these is a temporary thing that we intend to remove. I see below you have the "transformers need: ..." block, but is the intent that we remove all of these flags?

This is not temporary.

TSC works as this:

parse and type check / create symbol table

run transformers

emit

If we are expanding * into concrete identifiers, TypeScript has no information about them, and therefore cannot elide types from them. So we have to do this.
In the version of tsickle where we create a new pogram, TypeScript will recreate the symbol table and therefore know which types to elide.

So for transformers, we always want to do this.
Without transformers, we must not do this, as we rely on the fact that we produce functions for interfaces, which should not be elided.

Added all information about the flags directly to the enum members, and documented that these flags will be removed and on by default once the non transformer version is dropped.

evmar · 2017-06-29T17:18:58Z

src/tsickle.ts

      return true;
    } else {
      this.errorUnimplementedKind(decl, 'unexpected kind of import');
      return false;  // Use default processing.
    }
  }

+  private collectImportNames(decl: ts.ImportDeclaration) {


Can you document this?

evmar · 2017-06-29T17:20:10Z

src/transformer_util.ts

+/**
+ * Adjusts the given CustomTransformers with additional transformers
+ * to fix bugs in TypeScript.
+ * @param given A


XXX finish comment here

evmar · 2017-06-29T17:20:46Z

src/transformer_util.ts

+}
+
+/**
+ * And extended version of the TransformationContext that stores the FileContext as well.


evmar · 2017-06-29T17:22:51Z

src/transformer_util.ts

+/**
+ * Transform that needs to be executed right before TypeScript's transform.
+ *
+ * This prepares the node tree to workaround some bugs in the TypeScript emitter.


Is it worth filing the bugs upstream? I'm also worried they'll fix the bugs and then break our workarounds, but I'm not sure what we can do in that case.

Created #528 to track filing these bugs.

evmar · 2017-06-29T17:55:17Z

src/transformer_util.ts

+ * Transform that needs to be executed after TypeScript's transform.
+ *
+ * This fixes places where the TypeScript transformer does not
+ * emit synthetic comments.


Are these bugs? Should we file them?

Yes. I think I will leave TODOs everywhere for now where I need to file a bug, as that takes a bit (with small reproduction, ...)

Created #528 to track filing these bugs.

evmar · 2017-06-29T17:55:39Z

src/transformer_util.ts

+        } else if (
+            parent3 && parent3.kind === ts.SyntaxKind.VariableStatement &&
+            tsickle.hasModifierFlag(parent3, ts.ModifierFlags.Export)) {
+          // TypeScript ignore synthetic comments on exported variables.


typo: ignores

evmar · 2017-06-29T17:55:42Z

src/transformer_util.ts

+          }
+        }
+      }
+      // TypeScript ignore synthetic comments on reexport / import statements.


typo: ignores

evmar · 2017-06-29T17:56:22Z

src/transformer_util.ts

+
+/**
+ * Synthesizes the comments before and after a node
+ * befor calling a callback.


typo: before

I don't follow what this means, what does it mean to "synthesize a comment"

By default, comments are just text ranges (i.e. from - to). To synthesize means to create a node that is independent of text ranges and contains the information in itself.

For comments, we go from ts.CommentRange to ts.SynthesizedComment. The latter contains a field text to store the actual comment.

Added a comment.

evmar · 2017-06-29T18:07:43Z

src/transformer_util.ts

+  return -1;
+}
+
+function visitNodeStatementsWithSynthesizedComments<T extends ts.Node>(


evmar · 2017-06-29T18:07:54Z

src/transformer_util.ts

+  return visitor(node, statements);
+}
+
+function synthesizeDetachedLeadingComments(


docs (here and in the rest of the file)

evmar · 2017-06-29T18:10:28Z

src/transformer.ts

+  // All diagnostics (including warnings) are treated as errors.
+  // If we've decided to ignore them, just discard them.
+  // Warnings include stuff like "don't use @type in your jsdoc"; tsickle
+  // warns and then fixes up the code to be Closure-compatible anyway.


The rule here is that we error on google3 code, and don't error on third-party code. Is that still preserved here?

Yes, via the host.shouldIgnoreWarningsForPath.

Updated the comment.

evmar · 2017-06-29T18:11:40Z

src/transformer_sourcemap.ts

+  };
+}
+
+class NodeSourceMapper implements SourceMapper {


docs on here and the rest of this file

I think maybe this one could use a @fileOverview, because this is the one that does the clever "map node offsets to nodes" trick

evmar · 2017-06-29T18:13:19Z

test/tsickle_transformer_test.ts

+  }
+  const patchPath = calcPatchPath(path);
+  if (fs.existsSync(patchPath)) {
+    // Note: the typings for `diff.applyPath` are wrong in that the function


typo: patch

mprobst · 2017-06-30T11:59:52Z

src/tsickle.ts

+      const namedImports = importClause.namedBindings as ts.NamedImports;
+      names.push(...namedImports.elements.map(e => e.name));
+    }
+    names.forEach(name => {


for (const name of names) ?

mprobst · 2017-06-30T12:00:04Z

src/tsickle.ts

+      }
+      const declarationNames: ts.Identifier[] = [];
+      if (symbol.declarations) {
+        symbol.declarations.forEach(d => {


mprobst · 2017-06-30T12:07:30Z

src/transformer_util.ts

+/**
+ * Transform to be used as first transform to reset some state.
+ */
+function firstTransform(context: ts.TransformationContext) {


can you give this semantic names, e.g. addFileContexts?

mprobst · 2017-06-30T12:09:49Z

src/transformer_util.ts

+ */
+function firstTransform(context: ts.TransformationContext) {
+  return (sourceFile: ts.SourceFile) => {
+    (context as TransformationContext).fileContext = new FileContext(sourceFile);


Document that all transformers run in sequence for each file before visiting the next file, which makes this operation (caching the source file here) valid.

mprobst · 2017-06-30T12:10:43Z

src/transformer_util.ts

+/**
+ * A context that stores information per file to e.g. allow communication
+ * between transformers.
+ * Note that the ts.TransformationContext stores context about all files


maybe "There is one ts.TransformationContext per emit, but because files are handled sequentially by all transformers, we can store blah here..."

mprobst · 2017-06-30T12:12:27Z

src/transformer_util.ts

+ */
+class FileContext {
+  syntheticNodeParents = new Map<ts.Node, ts.Node|undefined>();
+  importOrReexportDeclarations: Array<ts.Node&{moduleSpecifier: ts.StringLiteral}> = [];


Is this a ts.ExportDeclaration|ts.ImportDeclaration?

mprobst · 2017-06-30T12:13:07Z

src/transformer_util.ts

+ *
+ * This prepares the node tree to workaround some bugs in the TypeScript emitter.
+ */
+function preTypeScriptTransform(context: ts.TransformationContext) {


semantic name

mprobst · 2017-06-30T12:14:32Z

src/transformer_util.ts

+      const originalNode = ts.getOriginalNode(node);
+      // Needed so that e.g. `module { ... }` prints the variable statement
+      // before the closure.
+      // tslint:disable-next-line:no-any


can you add a comment why you need the any?

mprobst · 2017-06-30T12:17:29Z

src/transformer_util.ts

+ * This fixes places where the TypeScript transformer does not
+ * emit synthetic comments.
+ */
+function postTypescriptTransform(context: ts.TransformationContext) {


semantic name

mprobst · 2017-06-30T12:28:46Z

src/transformer_util.ts

+}
+
+function synthesizeLeadingComments(
+    sourceFile: ts.SourceFile, node: ts.Node, lastCommentEnd: number): number {


this seems to always return -1 - possible a bug? Missing a test?

mprobst · 2017-06-30T12:36:31Z

src/transformer.ts

+  const emittedFiles: string[] = [];
+  const externs: {[fileName: string]: string} = {};
+  const modulesManifest = new ModulesManifest();
+  emitResults.forEach(er => {


mprobst · 2017-06-30T12:44:54Z

src/transformer_sourcemap.ts

+      ts.setTextRange(node, originalNode ? originalNode : {pos: -1, end: -1});
+      ts.setOriginalNode(node, originalNode);
+
+      // tslint:disable-next-line:no-any


@evmar

From review feedback from @evmar and @mprobst

b-strauss · 2017-07-02T00:46:26Z

FYI: this introduced 71 test failures on Windows. 😞

mprobst · 2017-07-04T15:15:46Z

What kind? At 71 failures, it's most likely something trivial to fix?

b-strauss · 2017-07-04T19:19:11Z

Pretty much every comment is missing. Maybe I can find a simple solution.

Example:

./test_files/type/type.js with ./test_files/type/type.js.transform.patch
      + expected - actual

      -goog.module('test_files.type.type');var module = module || {id: 'test_files/type/type.js'};let typeAny;
      -let typeArr;
      -let typeArr2;
      -let typeNestedArr;
      -let typeObject = { a: 3, b: 'b' };
      -let typeObjectIndexable;
      -let typeObjectMixedIndexProperty;
      -let typeObjectEmpty;
      -let typeTuple = [1, 2];
      -let typeComplexTuple = ['', true];
      -let typeTupleTuple = [[1, 2]];
      -let typeTupleTuple2 = [[1, 2], ''];
      -let typeUnion = Math.random() > 0.5 ? false : '';
      -let typeUnion2 = Math.random() > 0.5 ? false : '';
      -let typeOptionalField = {};
      -let typeOptionalUnion = {};
      -let typeFunc = function () { };
      -let typeFunc2 = function (a, b) { return ''; };
      -let typeFunc3 = function (x, cb) { return ''; };
      -// TODO: let typeFunc4: (a: number, ...args: number[]) => void;
      +goog.module('test_files.type.type');var module = module || {id: 'test_files/type/type.js'};/**
      + * @fileoverview added by tsickle
      + * @suppress {checkTypes} checked by tsc
      + */
      +let /** @type {?} */ typeAny;
      +let /** @type {!Array<?>} */ typeArr;
      +let /** @type {!Array<?>} */ typeArr2;
      +let /** @type {!Array<!Array<{a: ?}>>} */ typeNestedArr;
      +let /** @type {{a: number, b: string}} */ typeObject = { a: 3, b: 'b' };
      +let /** @type {!Object<string,number>} */ typeObjectIndexable;
      +let /** @type {?} */ typeObjectMixedIndexProperty;
      +let /** @type {!Object} */ typeObjectEmpty;
      +let /** @type {!Array<?>} */ typeTuple = [1, 2];
      +let /** @type {!Array<?>} */ typeComplexTuple = ['', true];
      +let /** @type {!Array<?>} */ typeTupleTuple = [[1, 2]];
      +let /** @type {!Array<?>} */ typeTupleTuple2 = [[1, 2], ''];
      +let /** @type {(string|boolean)} */ typeUnion = Math.random() > 0.5 ? false : '';
      +let /** @type {(string|boolean)} */ typeUnion2 = Math.random() > 0.5 ? false : '';
      +let /** @type {{optional: (undefined|boolean)}} */ typeOptionalField = {};
      +let /** @type {{optional: (undefined|string|boolean)}} */ typeOptionalUnion = {};
      +let /** @type {function(): void} */ typeFunc = function () { };
      +let /** @type {function(number, ?): string} */ typeFunc2 = function (a, b) { return ''; };
      +let /** @type {function(number, function(number): string): string} */ typeFunc3 = function (x, cb) { return ''; };
      +/**
      + * @param {function(number): number} callback
      + * @return {void}
      + */
       function typeCallback(callback) { }
       typeCallback(val => val + 1);
      +/**
      + * @template T
      + * @param {function(T): T} callback
      + * @return {void}
      + */
       function typeGenericCallback(callback) { }
       typeGenericCallback(val => val);

googlebot added the cla: yes label Jun 14, 2017

tbosch mentioned this pull request Jun 14, 2017

Proof of concept: typescript transformer support for tsickle #507

Closed

4 tasks

tbosch requested a review from evmar June 14, 2017 21:59

tbosch mentioned this pull request Jun 15, 2017

TypeScript / tsc drops comments, removes Closure-style casts #411

Closed

tbosch force-pushed the transform_final branch from 5b8f7a0 to 298d656 Compare June 16, 2017 22:00

tbosch force-pushed the transform_final branch from 298d656 to b16f42b Compare June 26, 2017 09:59

tbosch changed the title ~~Introduce transformer support for tsickle~~ WIP: Introduce transformer support for tsickle Jun 26, 2017

tbosch force-pushed the transform_final branch 2 times, most recently from 5988baf to 80be1c0 Compare June 29, 2017 13:51

Correctly emit downleveled decorators for combined pass

6434d0c

For: - getters - for properties with comments. Refactors and simplifies the emit logic. Fixes bug that prevented closure types for variable declarations as soon as the variable statement had a jsdoc comment.

tbosch mentioned this pull request Jun 29, 2017

Externs aren't generated for type aliases of records #510

Open

tbosch added 2 commits June 29, 2017 07:47

Emit additional parenthesis for casts in template expressions

2101930

This is required by closure ES6 to not produce an error. Note: In the current version of tsickle, type comments in interpolations are elided because of angular#411. In the upcoming transformer version, this is fixed though.

Revert: Don’t add a line break after null for ctorParameters.

a5b35e4

This change was introduced for the upcoming transformer version, but this is no longer needed. Introduced by angular@711cfa2. (that commit was missing some files though…)

tbosch force-pushed the transform_final branch from 80be1c0 to facaeaa Compare June 29, 2017 15:21

tbosch changed the title ~~WIP: Introduce transformer support for tsickle~~ Introduce transformer support for tsickle Jun 29, 2017

tbosch added 4 commits June 29, 2017 08:22

Add more test cases via goldens

0fb5f21

These test cases were reproductions of cases where the upcoming transformer version of tsickle failed in G3.

Introduce general transformer utils

6ff41ce

Allow to: - synthesize and change any comment - work around TypeScript bugs when using synthesized nodes (which are created in any transformer that changes things).

tbosch force-pushed the transform_final branch from facaeaa to 296e760 Compare June 29, 2017 15:23

evmar reviewed Jun 29, 2017

View reviewed changes

mprobst reviewed Jun 30, 2017

View reviewed changes

mprobst approved these changes Jun 30, 2017

View reviewed changes

Add more docs and minor cleanup

928ebca

From review feedback from @evmar and @mprobst

tbosch force-pushed the transform_final branch from d58950f to 928ebca Compare June 30, 2017 13:07

tbosch merged commit 672c9bf into angular:master Jun 30, 2017

tbosch deleted the transform_final branch July 31, 2017 14:30

Introduce transformer support for tsickle #516

Introduce transformer support for tsickle #516

Conversation

tbosch commented Jun 14, 2017 • edited Loading

tbosch commented Jun 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tbosch commented Jun 14, 2017 •

edited

Loading