Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add fixAwaitInSyncFunction code fix #21069

Merged
merged 7 commits into from
Jan 10, 2018
Merged

Conversation

srolel
Copy link
Contributor

@srolel srolel commented Jan 8, 2018

Fixes #21034

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
Copy link

ghost commented Jan 8, 2018

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
Copy link
Contributor Author

srolel commented Jan 8, 2018

@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?

@@ -19,7 +19,7 @@ namespace ts.codefix {
doChange(changes, context.sourceFile, getNodeToInsertBefore(diag.file, diag.start!))),
});

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

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.

@ghost
Copy link

ghost commented Jan 9, 2018

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

@@ -3843,6 +3843,10 @@
"category": "Message",
"code": 90028
},
"Convert to async": {
Copy link
Contributor

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

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?

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

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
Contributor Author

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 refactored getReturnType to not use a switch statement at all since with the added condition the two if statements handle all cases.

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

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

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

Choose a reason for hiding this comment

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

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.

Copy link

@ghost ghost left a comment

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)

@mhegazy mhegazy merged commit c0bdd12 into microsoft:master Jan 10, 2018
@microsoft microsoft locked and limited conversation to collaborators Jul 3, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
2 participants