Ensure that emitter calls callbacks · Pull Request #18284 · microsoft/TypeScript · GitHub

Fixes #18091

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.

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.

#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.

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).

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.

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

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.

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

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.

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

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

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

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.

nit: realign the surrounding comments

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.

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

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

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

Andy (Andrewkraft) (@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

Microsoft Contribution License Agreements (msftclas) added the cla-not-required label Sep 6, 2017

ghost requested a review from Ron Buckton (rbuckton) September 6, 2017 20:26

Deleted user (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

Ron Buckton (rbuckton) suggested 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

Ron Buckton (rbuckton) reviewed Sep 7, 2017

View reviewed changes

Ron Buckton (rbuckton) suggested 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

Ron Buckton (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 (microsoft) locked and limited conversation to collaborators Jun 14, 2018

This pull request was closed.

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

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

                           emitDecorators(node, node.decorators);
                           emitModifiers(node, node.modifiers);
-                          write(node.asteriskToken ? "function* " : "function ");
+                          write("function");

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

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

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

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

                           emitList(parentNode, parameters, ListFormat.IndexSignatureParameters);
                       }
+                      function emitSingleElementList(list: NodeArray<Node>) {

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

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

                                   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),

-                                  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));

                           emitIdentifier(<Identifier>node);
                       }
+                      function pipelineEmitMappedTypeParameter(node: TypeParameterDeclaration): void {

Ensure that emitter calls callbacks · Pull Request #18284 · microsoft/TypeScript · GitHub

Uh oh!

Ensure that emitter calls callbacks - #18284

Ensure that emitter calls callbacks#182849 commits merged intomastermicrosoft/TypeScript:masterfrom extract-method-formatmicrosoft/TypeScript:extract-method-formatCopy head branch name to clipboard

Conversation

Deleted user (ghost) commented Sep 6, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Ron Buckton (rbuckton) commented Sep 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Ron Buckton (rbuckton) Sep 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Deleted user (ghost) Sep 7, 2017 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Deleted user (ghost) commented Sep 7, 2017

Uh oh!

Ron Buckton (rbuckton) left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Mohamed Hegazy (mhegazy) commented Sep 7, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Ensure that emitter calls callbacks#18284
9 commits merged into
masterfrom
extract-method-format

Ron Buckton (rbuckton) commented Sep 6, 2017 •
edited
Loading

Ron Buckton (rbuckton) Sep 6, 2017 •
edited
Loading

Deleted user (ghost) Sep 7, 2017 •
edited by ghost
Loading