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

add fixAwaitInSyncFunction code fix

626f399

Copy link

ghost commentedJan 8, 2018

Hi@mosho1, I've created a branch at1319519 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.

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 thecodefix-async branch from1319519 to5dfb0c5Compare

January 8, 2018 23:51

Copy link

ContributorAuthor

srolel commentedJan 8, 2018•
edited
Loading

@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 ofreplaceNode or something to do with my editor?

only one codefix

15e916e

ghost reviewed

Jan 9, 2018

View reviewed changes

src/services/codefixes/fixAwaitInSyncFunction.ts Outdated

		});

		functiongetNodeToInsertBefore(sourceFile:SourceFile,pos:number):Node\|undefined{//name
		functiongetNodeToInsertBefore(sourceFile:SourceFile,pos:number):Node\|undefined{//name

Copy link

ghostJan 9, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Whoops, I should have removed the comment before pushing.

remove comment

c945beb

srolel force-pushed thecodefix-async branch from8c9fd0a toc945bebCompare

January 9, 2018 00:24

ghost approved these changes

Jan 9, 2018

View reviewed changes

Copy link

ghost commentedJan 9, 2018•
edited by ghost
Loading

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

mhegazy reviewed

Jan 9, 2018

View reviewed changes

src/compiler/diagnosticMessages.json Outdated

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

Copy link

Contributor

mhegazyJan 9, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

nit.Add async modifier to containing function

Copy link

Contributor

mhegazyJan 9, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

@DanielRosenwasser any better suggestions for the message?

Change explicit return type T to Promise<T>

0cb462a

ghost reviewed

Jan 9, 2018

View reviewed changes

src/services/codefixes/fixAwaitInSyncFunction.ts Outdated

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

Copy link

ghostJan 9, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link

ContributorAuthor

srolelJan 9, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

src/services/codefixes/fixAwaitInSyncFunction.ts Outdated

		functiondoChange(changes:textChanges.ChangeTracker,sourceFile:SourceFile,insertBefore:Node,returnType:TypeNode\|undefined):void{
		if(returnType){
		constentityName=getEntityNameFromTypeNode(returnType);
		if(!entityName\|\|entityName.getText()!=="Promise"){

Copy link

ghostJan 9, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

src/services/codefixes/fixAwaitInSyncFunction.ts Outdated

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

Copy link

ghostJan 9, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Use a functiongetNodes(...): { insertBefore: Node, returnType: Type | undefined } | undefined to avoid repeating this. That would also let you combine the two switch statements fromgetNodeToInsertBefore andgetReturnTypeNode.

Review changes

bbba931

ghost approved these changes

Jan 9, 2018

View reviewed changes

Copy link

ghost left a comment•
edited by ghost
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Change codefix message

752a972

srolel force-pushed thecodefix-async branch from4b80906 to752a972Compare

January 9, 2018 23:23

mhegazy merged commitc0bdd12 intomicrosoft:master

Jan 10, 2018

microsoft locked and limited conversation to collaborators

Jul 3, 2018

Labels

None yet

Movatterモバイル変換

add fixAwaitInSyncFunction code fix#21069

add fixAwaitInSyncFunction code fix#21069

Uh oh!

Conversation

srolel commentedJan 8, 2018• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

ghost commentedJan 8, 2018

Uh oh!

srolel commentedJan 8, 2018• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

ghostJan 9, 2018

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

mhegazyJan 9, 2018

Choose a reason for hiding this comment

Uh oh!

mhegazyJan 9, 2018

Choose a reason for hiding this comment

Uh oh!

ghostJan 9, 2018

Choose a reason for hiding this comment

Uh oh!

srolelJan 9, 2018

Choose a reason for hiding this comment

Uh oh!

ghostJan 9, 2018

Choose a reason for hiding this comment

Uh oh!

ghostJan 9, 2018

Choose a reason for hiding this comment

Uh oh!

ghost left a comment• edited by ghostLoading 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 commentedJan 8, 2018•
edited
Loading

srolel commentedJan 8, 2018•
edited
Loading

ghost commentedJan 9, 2018•
edited by ghost
Loading

ghost left a comment•
edited by ghost
Loading