add fixAwaitInSyncFunction code fix - #21069
Conversation
Sorry, something went wrong.
|
Hi MOSHOOD OJUKO (@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. |
Sorry, something went wrong.
|
Andy (Andrewkraft) (@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? |
Sorry, something went wrong.
| }); | ||
|
|
||
| function getNodeToInsertBefore(sourceFile: SourceFile, pos: number): Node | undefined {//name | ||
| function getNodeToInsertBefore(sourceFile: SourceFile, pos: number): Node | undefined {// name |
There was a problem hiding this comment.
Whoops, I should have removed the comment before pushing.
Sorry, something went wrong.
|
We could also consider changing an explicit return type T to Promise<T> (if it's not already a promise). |
Sorry, something went wrong.
| "category": "Message", | ||
| "code": 90028 | ||
| }, | ||
| "Convert to async": { |
There was a problem hiding this comment.
nit. Add async modifier to containing function
Sorry, something went wrong.
There was a problem hiding this comment.
Daniel Rosenwasser (@DanielRosenwasser) any better suggestions for the message?
Sorry, something went wrong.
| if (isVariableDeclaration(containingFunction.parent) && | ||
| containingFunction.parent.type && | ||
| isFunctionTypeNode(containingFunction.parent.type)) { | ||
| return containingFunction.parent.type.type; |
There was a problem hiding this comment.
Also check for a .type immediately on either of these, i.e. function(): number { ... } or (): number => { ... }.
Sorry, something went wrong.
There was a problem hiding this comment.
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.
Sorry, something went wrong.
| function doChange(changes: textChanges.ChangeTracker, sourceFile: SourceFile, insertBefore: Node, returnType: TypeNode | undefined): void { | ||
| if (returnType) { | ||
| const entityName = getEntityNameFromTypeNode(returnType); | ||
| if (!entityName || entityName.getText() !== "Promise") { |
There was a problem hiding this comment.
if (!entityName || entityName.kind !== SyntaxKind.Identifier || entityName.text !== "Promise")
Sorry, something went wrong.
| 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); |
There was a problem hiding this comment.
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.
Sorry, something went wrong.
There was a problem hiding this comment.
(but note Mohamed Hegazy (@mhegazy)'s comment about the message)
Sorry, something went wrong.
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.