Ensure that emitter calls callbacks #18284

ghost · 2017-09-06T20:26:25Z

The bug was happening because in formatting.ts, in processChildNodes, we continue scanning until getting to nodes.pos. Since this is a dynamically typed language, nodes.pos may be undefined despite our best efforts to type it, so the loop continued forever, thinking it was inside of the typeParameters of the generated function declaration; which meant it didn't know that it was actually nested several levels deep and should have been indenting more.

The problem was that in textChanges.ts, the function getPos promised to return a number but was implemented with (<any>n)["__pos"], which wasn't always defined. I added an assertion that it was, which brought up more problems.

In textChanges, we rely on callbacks like onBeforeEmitNodeArray to set "__pos". But there were many situations in which the emitter did not call these callbacks, so those all had to be updated; otherwise we don't set "__pos", and then the new nodes have missing or incorrect positions.

ghost · 2017-09-06T20:27:41Z

src/compiler/factory.ts

+        typeParameters: ReadonlyArray<TypeParameterDeclaration> | undefined,
+        parameters: ReadonlyArray<ParameterDeclaration>,
+        type: TypeNode | undefined,
+        equalsGreaterThanTokenOrBody: Token<SyntaxKind.EqualsGreaterThanToken> | ConciseBody,


Need to ensure that we use the => from the new tree, which has a correct position. The => from the old tree has a different position that causes assertion errors in the formatter.

Can we add it at the end instead?

We should always keep elements in the create/update methods in the order they are parsed.

rbuckton · 2017-09-06T21:37:49Z

~~#18091 does not seem related, is that the right bug?~~

Nevermind, your initial paragraph indicated the bug was an infinite loop which is not what #18091 is discussing, but this does address #18091.

rbuckton · 2017-09-06T21:48:14Z

src/compiler/emitter.ts

-            emit(node.typeParameter.constraint);
-            write("]");
+
+            if (onEmitNode) {


We really should avoid calling onEmitNode directly, and should be calling emit instead, except in this case we need to emit a type parameter in a different fashion. For anyone writing a custom transform, they won't know whether their onEmitNode is being called against a normal TypeParameter or the TypeParameter of a MappedType. That is the reason we have EmitHint, as it allows us to provide a hint as to the context in which we are emitting a node.

I would recommend we add MappedTypeParameter to the EmitHint enum and add a branch to pipelineEmitWithHint for that branch that calls emitMappedTypeParameter. Then we can replace this code with a call to pipelineEmitWithNotification(EmitHint.MappedTypeParameter, node.typeParameter).

rbuckton · 2017-09-06T21:50:06Z

src/compiler/emitter.ts

@@ -1114,6 +1115,14 @@ namespace ts {
            write("}");
        }

+        function emitMappedTypeParameter(_hint: EmitHint, node: TypeParameterDeclaration) {
+            write("[");


We parse the opening and closing brackets in parseMappedType, so their tokens do not belong to the type parameter. Writing the [ and ] tokens should happen in emitMappedType.

rbuckton · 2017-09-06T21:52:27Z

src/compiler/emitter.ts

@@ -1382,7 +1381,10 @@ namespace ts {
        }

        function emitYieldExpression(node: YieldExpression) {
-            write(node.asteriskToken ? "yield*" : "yield");
+            write("yield");
+            if (node.asteriskToken) {


No need to guard against undefined, as that is already handled in emit

rbuckton · 2017-09-06T21:52:45Z

src/compiler/emitter.ts

@@ -1662,7 +1664,11 @@ namespace ts {
        function emitFunctionDeclarationOrExpression(node: FunctionDeclaration | FunctionExpression) {
            emitDecorators(node, node.decorators);
            emitModifiers(node, node.modifiers);
-            write(node.asteriskToken ? "function* " : "function ");
+            write("function");


~~No need to guard against undefined as that is already handled in emit~~

Edit: I was incorrect. Apparently emit does not have this guard.

I get a test failure in the test PrinterAPI printFile removeComments if I try to emit the asterisk unconditionally. emit doesn't seem to check for an undefined node.

rbuckton · 2017-09-06T21:55:36Z

src/compiler/emitter.ts

@@ -2395,6 +2399,17 @@ namespace ts {
            emitList(parentNode, parameters, ListFormat.IndexSignatureParameters);
        }

+        function emitSingleElementList(list: NodeArray<Node>) {


Can you craft suitable ListFormat bitmasks to use instead and just call emitList where applicable instead of creating a special cased list emit function?

rbuckton · 2017-09-06T22:01:33Z

src/compiler/emitter.ts

@@ -2580,16 +2595,20 @@ namespace ts {
                : writeTokenText(token, pos);
        }

-        function writeTokenNode(node: Node) {
+        function writeTokenAndCallCallbacks(node: Node, text: string) {


Maybe we can simplify this by replacing all calls to writeIfPresent with emit, as emit already guards against undefined, will in turn eventually call writeTokenNode, and may give us better comment preservation and source-maps.

rbuckton · 2017-09-06T22:06:20Z

src/compiler/utilities.ts

-        return array.hasOwnProperty("pos")
-            && array.hasOwnProperty("end");
+        const res = array.hasOwnProperty("pos") && array.hasOwnProperty("end");
+        if (res) {


We possibly call this a lot between visitor and factory. Do the added checks have any perf impact?

rbuckton · 2017-09-07T19:13:27Z

src/compiler/factory.ts

+        condition: Expression,
+        whenTrue: Expression,
+        whenFalse: Expression,
+        questionToken: Token<SyntaxKind.QuestionToken> = node.questionToken,


We should always keep these arguments in the same order they are parsed.

rbuckton · 2017-09-07T19:14:16Z

src/compiler/factory.ts

+        typeParameters: ReadonlyArray<TypeParameterDeclaration> | undefined,
+        parameters: ReadonlyArray<ParameterDeclaration>,
+        type: TypeNode | undefined,
+        equalsGreaterThanTokenOrBody: Token<SyntaxKind.EqualsGreaterThanToken> | ConciseBody,


We should always keep elements in the create/update methods in the order they are parsed.

rbuckton · 2017-09-07T19:15:03Z

src/compiler/factory.ts

-        }
+        body: ConciseBody,
+        // Optional for backwards-compatibility only -- should always provide this.
+        equalsGreaterThanToken: Token<SyntaxKind.EqualsGreaterThanToken> = node.equalsGreaterThanToken): ArrowFunction {


As complex as it was, the previous version was better. We should always keep these arguments in the same order as they would be parsed/visited.

rbuckton · 2017-09-07T19:16:09Z

src/compiler/types.ts

@@ -4261,6 +4261,7 @@ namespace ts {
        SourceFile,         // Emitting a SourceFile
        Expression,         // Emitting an Expression
        IdentifierName,     // Emitting an IdentifierName
+        MappedTypeParameter, // Emitting a TypeParameterDeclaration inside of a MappedTypeNode


nit: realign the surrounding comments

rbuckton · 2017-09-07T19:18:04Z

src/compiler/visitor.ts

@@ -525,7 +525,9 @@ namespace ts {
                return updateConditional(<ConditionalExpression>node,
                    visitNode((<ConditionalExpression>node).condition, visitor, isExpression),
                    visitNode((<ConditionalExpression>node).whenTrue, visitor, isExpression),
-                    visitNode((<ConditionalExpression>node).whenFalse, visitor, isExpression));
+                    visitNode((<ConditionalExpression>node).whenFalse, visitor, isExpression),
+                    visitNode((<ConditionalExpression>node).questionToken, visitor, n => n.kind === SyntaxKind.QuestionToken),


I would just use isToken rather than be overly concerned about the specific token. No need to allocate a new function object every time this is called.

rbuckton · 2017-09-07T19:18:10Z

src/compiler/visitor.ts

-                    visitNode((<ConditionalExpression>node).whenFalse, visitor, isExpression));
+                    visitNode((<ConditionalExpression>node).whenFalse, visitor, isExpression),
+                    visitNode((<ConditionalExpression>node).questionToken, visitor, n => n.kind === SyntaxKind.QuestionToken),
+                    visitNode((<ConditionalExpression>node).colonToken, visitor, n => n.kind === SyntaxKind.ColonToken));


I would just use isToken rather than be overly concerned about the specific token. No need to allocate a new function object every time this is called.

rbuckton · 2017-09-07T19:18:16Z

src/compiler/visitor.ts

-                    visitNode((<ArrowFunction>node).equalsGreaterThanToken, visitor, n => n.kind === SyntaxKind.EqualsGreaterThanToken),
-                    visitFunctionBody((<ArrowFunction>node).body, visitor, context));
+                    visitFunctionBody((<ArrowFunction>node).body, visitor, context),
+                    visitNode((<ArrowFunction>node).equalsGreaterThanToken, visitor, n => n.kind === SyntaxKind.EqualsGreaterThanToken));


I would just use isToken rather than be overly concerned about the specific token. No need to allocate a new function object every time this is called.

ghost · 2017-09-07T21:02:49Z

Ran the perf tests and saw no slowdown due to this PR.

rbuckton

I'd still rather not have emitSingleElementList if possible, but we can remove it later.

rbuckton · 2017-09-07T21:03:45Z

src/compiler/emitter.ts

@@ -465,6 +474,12 @@ namespace ts {
            emitIdentifier(<Identifier>node);
        }

+        function pipelineEmitMappedTypeParameter(node: TypeParameterDeclaration): void {


Just call this emitMappedTypeParameter.

* Ensure that emitter calls calbacks * Move new parameter to end of parameters * Fix for ConditionalExpression * Make suggested changes to emitter * Fix parameter ordering * Respond to minor comments * Remove potentially expensive assertion * More emitter cleanup

mhegazy · 2017-09-07T22:17:44Z

@Andy-MS we will need to port this to release-2.5

* Ensure that emitter calls calbacks * Move new parameter to end of parameters * Fix for ConditionalExpression * Make suggested changes to emitter * Fix parameter ordering * Respond to minor comments * Remove potentially expensive assertion * More emitter cleanup

Ensure that emitter calls calbacks

9815359

msftclas added the cla-not-required label Sep 6, 2017

ghost requested a review from rbuckton September 6, 2017 20:26

ghost commented Sep 6, 2017

View reviewed changes

ghost changed the title ~~Ensure that emitter calls calbacks~~ Ensure that emitter calls callbacks Sep 6, 2017

rbuckton requested changes Sep 6, 2017

View reviewed changes

Move new parameter to end of parameters

7d931bb

ghost force-pushed the extract-method-format branch 2 times, most recently from 495f1a2 to 7d931bb Compare September 7, 2017 18:14

Andy Hanson added 2 commits September 7, 2017 11:14

Merge branch 'master' into extract-method-format

7d56deb

Fix for ConditionalExpression

5b44de1

ghost force-pushed the extract-method-format branch from 49e21b1 to 5b44de1 Compare September 7, 2017 18:44

Make suggested changes to emitter

aae877d

rbuckton reviewed Sep 7, 2017

View reviewed changes

rbuckton requested changes Sep 7, 2017

View reviewed changes

Andy Hanson added 3 commits September 7, 2017 12:57

Fix parameter ordering

8f775dc

Respond to minor comments

bca3b09

Remove potentially expensive assertion

15e2663

rbuckton approved these changes Sep 7, 2017

View reviewed changes

More emitter cleanup

b33dbec

ghost merged commit ed4e2e6 into master Sep 7, 2017

ghost deleted the extract-method-format branch September 7, 2017 21:30

ghost mentioned this pull request Sep 7, 2017

Ensure that emitter calls callbacks () #18325

Merged

This was referenced Sep 14, 2017

Fixes to emit / format for codeFix #18484

Merged

Ensure that emitter calls callbacks for empty blocks #18547

Merged

ghost mentioned this pull request Sep 22, 2017

getEditsForRefactor debug failure in js file #18671

Closed

This was referenced Jan 6, 2018

Should this just be emit? imdone/TypeScript#90

Open

Should this just be emit? imdone/TypeScript#127

Open

Should this just be emit? imdone/TypeScript#192

Open

Should this just be emit? imdone/TypeScript#714

Open

ghost mentioned this pull request Feb 8, 2018

Fix bug: Don't skip emitting JSX attributes #21777

Merged

microsoft locked and limited conversation to collaborators Jun 14, 2018

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ensure that emitter calls callbacks #18284

Ensure that emitter calls callbacks #18284

ghost commented Sep 6, 2017

ghost Sep 6, 2017

mhegazy Sep 6, 2017

rbuckton Sep 7, 2017

rbuckton commented Sep 6, 2017 •

edited

Loading

rbuckton Sep 6, 2017

rbuckton Sep 6, 2017

rbuckton Sep 6, 2017

rbuckton Sep 6, 2017 •

edited

Loading

ghost Sep 7, 2017 •

edited by ghost

Loading

rbuckton Sep 6, 2017

rbuckton Sep 6, 2017

rbuckton Sep 6, 2017

rbuckton Sep 7, 2017

rbuckton Sep 7, 2017

rbuckton Sep 7, 2017

rbuckton Sep 7, 2017

rbuckton Sep 7, 2017

rbuckton Sep 7, 2017

rbuckton Sep 7, 2017

ghost commented Sep 7, 2017

rbuckton left a comment

rbuckton Sep 7, 2017

mhegazy commented Sep 7, 2017

Ensure that emitter calls callbacks #18284

Ensure that emitter calls callbacks #18284

Conversation

ghost commented Sep 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rbuckton commented Sep 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rbuckton Sep 6, 2017 • edited Loading

Choose a reason for hiding this comment

ghost Sep 7, 2017 • edited by ghost Loading

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

ghost commented Sep 7, 2017

rbuckton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhegazy commented Sep 7, 2017

rbuckton commented Sep 6, 2017 •

edited

Loading

rbuckton Sep 6, 2017 •

edited

Loading

ghost Sep 7, 2017 •

edited by ghost

Loading