add fixAwaitInSyncFunction code fix by srolel · Pull Request #21069 · microsoft/TypeScript

srolel · 2018-01-08T18:01:16Z

This is pretty straightforward, replacing function expressions/declarations, method declarations and arrow functions with corresponding ones with the async modifier.

There are 2 issues that I need some help with:

1. Replacing the entire node seems to omit the line break at the end of the replaced node. So that this:

class Foo {
    bar() {
        await Promise.resolve();
    }
}

Becomes:

class Foo {
    async bar() {
        await Promise.resolve();
    }}

This happens both in the unit tests, and in manual e2e testing with vscode.

2. codeFixAwaitInSyncFunction7, the test case of an for-await-of loop, fails -

  tests/cases/fourslash/codeFixAwaitInSyncFunction7.ts
         fourslash test codeFixAwaitInSyncFunction7.ts runs correctly:
     Error: Debug Failure. False expression: Token end is child end
      at processChildNode (src\services\formatting\formatting.ts:704:27)
      at src\services\formatting\formatting.ts:629:21
      at visitNode (src\compiler\parser.ts:38:24)
      at Object.forEachChild (src\compiler\parser.ts:285:24)
      at processNode (src\services\formatting\formatting.ts:626:13)
      at processChildNode (src\services\formatting\formatting.ts:712:17)
      at processChildNodes (src\services\formatting\formatting.ts:767:44)
      at src\services\formatting\formatting.ts:632:21
      at visitNodes (src\compiler\parser.ts:44:24)
      at Object.forEachChild (src\compiler\parser.ts:253:24)
      at processNode (src\services\formatting\formatting.ts:626:13)
      at processChildNode (src\services\formatting\formatting.ts:712:17)
      at src\services\formatting\formatting.ts:629:21
      at visitNode (src\compiler\parser.ts:38:24)
      at Object.forEachChild (src\compiler\parser.ts:160:21)
      at processNode (src\services\formatting\formatting.ts:626:13)
      at formatSpanWorker (src\services\formatting\formatting.ts:416:13)
      at src\services\formatting\formatting.ts:346:108
      at Object.getFormattingScanner (src\services\formatting\formattingScanner.ts:41:21)

I tried to debug this for a while, but nothing conclusive. This does not happen when I use the compiled tsserver.js in vscode to test the code fix.

ghost · 2018-01-08T21:53:30Z

Hi @mosho1, I've created a branch at 1319519 that would just add the keyword to the front of the function instead of replacing the node. That seems to fix the formatting problems since we're only inserting text now.

srolel · 2018-01-08T23:58:25Z

@Andy-MS Thanks, that is a lot better. I picked only the commit with the change, let me know if I should do it differently.

I'm also wondering about those CRs in the tests, I saw them in some tests but not in others. Are they also an outcome of replaceNode or something to do with my editor?

ghost · 2018-01-09T00:15:00Z

src/services/codefixes/fixAwaitInSyncFunction.ts

    });

-    function getNodeToInsertBefore(sourceFile: SourceFile, pos: number): Node | undefined {//name
+    function getNodeToInsertBefore(sourceFile: SourceFile, pos: number): Node | undefined {// name


Whoops, I should have removed the comment before pushing.

ghost · 2018-01-09T16:31:04Z

We could also consider changing an explicit return type T to Promise<T> (if it's not already a promise).

mhegazy · 2018-01-09T17:44:03Z

src/compiler/diagnosticMessages.json

        "category": "Message",
        "code": 90028
    },
+    "Convert to async": {


nit. Add async modifier to containing function

@DanielRosenwasser any better suggestions for the message?

ghost · 2018-01-09T21:09:26Z

src/services/codefixes/fixAwaitInSyncFunction.ts

+                if (isVariableDeclaration(containingFunction.parent) &&
+                    containingFunction.parent.type &&
+                    isFunctionTypeNode(containingFunction.parent.type)) {
+                    return containingFunction.parent.type.type;


Also check for a .type immediately on either of these, i.e. function(): number { ... } or (): number => { ... }.

After adding this change I refactored getReturnType to not use a switch statement at all since with the added condition the two if statements handle all cases.

ghost · 2018-01-09T21:11:40Z

src/services/codefixes/fixAwaitInSyncFunction.ts

+    function doChange(changes: textChanges.ChangeTracker, sourceFile: SourceFile, insertBefore: Node, returnType: TypeNode | undefined): void {
+        if (returnType) {
+            const entityName = getEntityNameFromTypeNode(returnType);
+            if (!entityName || entityName.getText() !== "Promise") {


if (!entityName || entityName.kind !== SyntaxKind.Identifier || entityName.text !== "Promise")

ghost · 2018-01-09T21:12:50Z

src/services/codefixes/fixAwaitInSyncFunction.ts

-        getAllCodeActions: context => codeFixAll(context, errorCodes, (changes, diag) =>
-            doChange(changes, context.sourceFile, getNodeToInsertBefore(diag.file, diag.start!))),
+        getAllCodeActions: context => codeFixAll(context, errorCodes, (changes, diag) => {
+            const token = getTokenAtPosition(diag.file, diag.start!, /*includeJsDocComment*/ false);


Use a function getNodes(...): { insertBefore: Node, returnType: Type | undefined } | undefined to avoid repeating this. That would also let you combine the two switch statements from getNodeToInsertBefore and getReturnTypeNode.

ghost

(but note @mhegazy's comment about the message)

add fixAwaitInSyncFunction code fix

626f399

ghost mentioned this pull request Jan 8, 2018

Ensure visitor visits the awaitModifier of a ForOfStatement #21078

Merged

Just insert the keyword

5dfb0c5

srolel force-pushed the codefix-async branch from 1319519 to 5dfb0c5 Compare January 8, 2018 23:51

only one codefix

15e916e

ghost reviewed Jan 9, 2018

View reviewed changes

remove comment

c945beb

srolel force-pushed the codefix-async branch from 8c9fd0a to c945beb Compare January 9, 2018 00:24

ghost approved these changes Jan 9, 2018

View reviewed changes

mhegazy reviewed Jan 9, 2018

View reviewed changes

Change explicit return type T to Promise<T>

0cb462a

ghost reviewed Jan 9, 2018

View reviewed changes

Review changes

bbba931

ghost approved these changes Jan 9, 2018

View reviewed changes

Change codefix message

752a972

srolel force-pushed the codefix-async branch from 4b80906 to 752a972 Compare January 9, 2018 23:23

mhegazy merged commit c0bdd12 into microsoft:master Jan 10, 2018

microsoft locked and limited conversation to collaborators Jul 3, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add fixAwaitInSyncFunction code fix#21069

add fixAwaitInSyncFunction code fix#21069
mhegazy merged 7 commits intomicrosoft:masterfrom
srolel:codefix-async

srolel commented Jan 8, 2018 •

edited

Loading

Uh oh!

ghost commented Jan 8, 2018

Uh oh!

srolel commented Jan 8, 2018 •

edited

Loading

Uh oh!

ghost Jan 9, 2018

Uh oh!

ghost commented Jan 9, 2018 •

edited by ghost

Loading

Uh oh!

mhegazy Jan 9, 2018

Uh oh!

mhegazy Jan 9, 2018

Uh oh!

ghost Jan 9, 2018

Uh oh!

srolel Jan 9, 2018

Uh oh!

ghost Jan 9, 2018

Uh oh!

ghost Jan 9, 2018

Uh oh!

ghost left a comment •

edited by ghost

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

srolel commented Jan 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Jan 8, 2018

Uh oh!

srolel commented Jan 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost Jan 9, 2018

Choose a reason for hiding this comment

Uh oh!

ghost commented Jan 9, 2018 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mhegazy Jan 9, 2018

Choose a reason for hiding this comment

Uh oh!

mhegazy Jan 9, 2018

Choose a reason for hiding this comment

Uh oh!

ghost Jan 9, 2018

Choose a reason for hiding this comment

Uh oh!

srolel Jan 9, 2018

Choose a reason for hiding this comment

Uh oh!

ghost Jan 9, 2018

Choose a reason for hiding this comment

Uh oh!

ghost Jan 9, 2018

Choose a reason for hiding this comment

Uh oh!

ghost left a comment • 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

srolel commented Jan 8, 2018 •

edited

Loading

srolel commented Jan 8, 2018 •

edited

Loading

ghost commented Jan 9, 2018 •

edited by ghost

Loading

ghost left a comment •

edited by ghost

Loading