[WIP] Promises to Async Await Code Fix #24939

elizabethdinella · 2018-06-13T23:06:38Z

Fixes #

elizabethdinella · 2018-06-13T23:07:13Z

mhegazy · 2018-06-13T23:40:58Z

src/services/codefixes/convertPromisesToAwaitAndAsync.ts

+    }
+
+    function getPromiseCall(node:Node): CallExpression{
+        if(node.kind == SyntaxKind.CallExpression){


nit. mind running the formatter on the changed files.

also mind running the linter task, jake lint

mhegazy · 2018-06-13T23:42:12Z

src/services/completions.ts

@@ -1050,6 +1051,12 @@ namespace ts.Completions {
                }
            }

+            if (isMetaProperty(node) && (node.keywordToken === SyntaxKind.NewKeyword || node.keywordToken === SyntaxKind.ImportKeyword)) {


can you merge from master one more time. i think these should already be in master now.

mhegazy · 2018-06-13T23:43:41Z

src/services/suggestionDiagnostics.ts

+                        break;
+                    }
+
+                    const returnType = checker.getReturnTypeOfSignature(checker.getSignatureFromDeclaration(<FunctionDeclaration | FunctionExpression> node))


limit these to non-ambient functions. an easy check is if the function does not have a body, break

what about methods, and arrow functions?

mhegazy · 2018-06-13T23:46:23Z

src/services/suggestionDiagnostics.ts

                    if (isJsFile) {
                        const symbol = node.symbol;
                        if (symbol.members && (symbol.members.size > 0)) {
                            diags.push(createDiagnosticForNode(isVariableDeclaration(node.parent) ? node.parent.name : node, Diagnostics.This_constructor_function_may_be_converted_to_a_class_declaration));
                        }
                    }
+
+                    if(node.modifiers && containsAsync(node.modifiers)){


hasModifier(node, ModifierFlags.Async)

actually you can just call isAsyncFunction

you should also think about async generators.. i would exclude these for now.

mhegazy · 2018-06-13T23:49:39Z

src/services/suggestionDiagnostics.ts

+        return false;
+    }
+
+    function isPromiseType(T:Type):boolean{


a better check is to expose the checker's getPromisedTypeOfPromise, call that, if it returns a type, then it is Promise, if it returns undefined it is not.

mhegazy · 2018-06-13T23:52:27Z

src/services/suggestionDiagnostics.ts

+                        break;
+                    }
+
+                    if(isPromiseType(returnType)){


i am not sure this is sufficient check to be able convert it. e.g.:

function f() { return g(); } async function g() { return 0; }

f returns a Promise, but there is no reason why this should be converted to an async function.

I think a better way to look at this is not the async part but rather the await part. if the function includes a possible use of await. in other words, there a call to .then on a promise in the function.

since we are talking about it should be the use of any of .then, .catch and .finally

DanielRosenwasser · 2018-06-13T23:48:02Z

src/services/suggestionDiagnostics.ts

@@ -104,4 +119,18 @@ namespace ts {
    function getErrorNodeFromCommonJsIndicator(commonJsModuleIndicator: Node): Node {
        return isBinaryExpression(commonJsModuleIndicator) ? commonJsModuleIndicator.left : commonJsModuleIndicator;
    }
+
+    function containsAsync(arr: NodeArray<Modifier>): boolean{


Use the hasModifier helper.

DanielRosenwasser · 2018-06-13T23:54:36Z

src/services/suggestionDiagnostics.ts

+        return false;
+    }
+
+    function isPromiseType(T:Type):boolean{


You need to think about a few things:

There could be multiple types named Promise, and they might not be the ones of interest. So you need some other API to introspect the type.

Usually with async/await, we're less concerned with whether the type is a Promise and more about whether it is "thenable". Yes, that means that converting from async/await changes your types in some cases, but this is probably okay.

DanielRosenwasser · 2018-06-13T23:57:06Z

src/services/codefixes/convertPromisesToAwaitAndAsync.ts

+        return (<PropertyAccessExpression>node.expression).name.text === funcName && isPromiseType(checker.getTypeAtLocation(node));
+    }
+
+    function isPromiseType(T:Type):boolean{


You defined this below

DanielRosenwasser · 2018-06-13T23:57:34Z

src/services/codefixes/convertPromisesToAwaitAndAsync.ts

+
+    function parseThen(node:CallExpression, checker:TypeChecker): Statement[]{
+        let res = node.arguments[0];
+        let rej = node.arguments[1];


let [res, rej] = node.arguments

DanielRosenwasser · 2018-06-14T00:14:19Z

src/services/codefixes/convertPromisesToAwaitAndAsync.ts

+            return node as CallExpression;
+        }
+
+        for( let child of node.getChildren().filter(node => isNode(node)) ){


I think you're looking for the forEachChild in parser.ts. But if not you can also just use

return node.getChildren().find(isNode);

getChildren() is the wrong method to use since it returns the tokens, such as an opening parenthesis.

DanielRosenwasser · 2018-06-14T00:16:30Z

tests/cases/fourslash/codeFixPromiseToAsyncResRef_NoReturnVal.ts

+/// <reference path='fourslash.ts' />
+
+////function f():Promise<any> {
+////    return fetch('http://yahoo.com').then(res);


Just avoid potential trademark issues altogether and find/replace these with typescriptlang.org

ghost · 2018-06-14T15:32:13Z

tests/cases/fourslash/codeFixPromiseToAsync.ts

+    reportsUnnecessary: true,
+}]);
+
+verify.codeFix({


The description here looks wrong.

lbguilherme · 2018-06-14T15:35:19Z

src/compiler/diagnosticMessages.json

+    },
+    "Convert to use async and await":{
+        "category": "Message",
+         "code": 95055


There is an extra space here

I think I'd prefer "Convert to async function", in case anyone else does.

+1 for @bterlson comment. also for the suggestion message as well.

ghost · 2018-06-14T15:33:30Z

tests/cases/fourslash/codeFixPromiseToAsyncResRef.ts

+    index: 0,
+    newFileContent:
+`async function f() {
+   var result = await fetch('http://yahoo.com);


I think we should default to const over var. But a const can't be reassigned, so you'd have to not use the name result twice if there is more than one awaited result.

Actually, the variable is unnecessary here since return res(await fetch()) is closer to what the original code did. So maybe it's never necessary to synthesize a name.

ghost · 2018-06-14T15:35:55Z

tests/cases/fourslash/codeFixPromiseToAsyncRej.ts

+   }catch(rejection){
+       console.log("rejected", rejection);
+   }
+   console.log(result);


If there is an exception then result will be undefined -- don't think the original code had that issue.

ghost · 2018-06-14T15:36:23Z

tests/cases/fourslash/completionImportMeta.ts

@@ -0,0 +1,6 @@
+/// <reference path='fourslash.ts' />
+
+////import./**/


Could this be a separate PR?

mhegazy · 2018-06-14T18:35:49Z

src/services/suggestionDiagnostics.ts

-    function isPromiseType(T:Type):boolean{
-        return T.flags === TypeFlags.Object && T.symbol.name === "Promise";
+    function getReturnStmts(node: Node, retStmts: ReturnStatement[]) {
+        for (const child of node.getChildren().filter(node => isNode(node))) {


no need to getChildren. this is a call that will reiterate the tokens in the tree, and since we do not care about tokens, only return statement nodes, we do not need to do that, and we do not need the .filter either.

also, you can use forEachReturnStatement instead.

mhegazy · 2018-06-14T18:39:15Z

src/services/suggestionDiagnostics.ts

+                    // collect all the return statements
+                    // check that a property access expression exists in there and that it is a handler
+                    const retStmts: ReturnStatement[] = [];
+                    getReturnStmts(node, retStmts);


i would recommend having all this logic in one function, and sharing it between the quick fix and the suggestion code. this way we can use them to report the suggestion, and check in the quick fix if the assumptions are still valid.

this comes up when VSCode, say call us to apply a fix after a code change, so it is possible that the previous code where we reported the suggestion has change.

RyanCavanaugh

Just a few comments

RyanCavanaugh · 2018-08-09T20:36:14Z

src/services/codefixes/convertToAsyncFunction.ts

+        return setOfExpressionsToReturn;
+    }
+
+    function isPromiseReturningExpression(node: Node, checker: TypeChecker, name?: string): boolean {


For any of these predicate-type functions, an explanatory comment showing a 'true' and 'false' case would be really helpful. For this function, for example, it's not obvious to me what name is for

RyanCavanaugh · 2018-08-09T20:38:39Z

src/services/codefixes/convertToAsyncFunction.ts

+                const symbolIdString = getSymbolId(symbol).toString();
+
+                // if the identifier refers to a function we want to add the new synthesized variable for the declaration (ex. blob in let blob = res(arg))
+                // Note - the choice of the first call signature is arbitrary


Given a list of call signatures where you don't know which to pick, it's generally best in TypeScript to pick the last rather than the first.

RyanCavanaugh · 2018-08-09T20:42:39Z

src/services/codefixes/convertToAsyncFunction.ts

+        }
+
+        let nodeType = transformer.checker.getTypeAtLocation(node);
+        if (nodeType && nodeType.flags & 1 && (<IntrinsicType>nodeType).intrinsicName === "error" && isIdentifier(node)) {


RyanCavanaugh · 2018-08-09T20:54:15Z

src/services/codefixes/convertToAsyncFunction.ts

+        const numberOfAssignmentsOriginal = 0, numberOfAssignmentsSynthesized = 0;
+        const types: Type[] = [];
+
+        function getMapEntryIfExists(node: Identifier): SynthIdentifier {


Please have helper functions entirely at the bottom of their containing functions - it's confusing to have imperative lines both above and below them

RyanCavanaugh · 2018-08-09T20:57:08Z

src/services/utilities.ts

@@ -1626,14 +1626,34 @@ namespace ts {
     * WARNING: This is an expensive operation and is only intended to be used in refactorings
     * and code fixes (because those are triggered by explicit user actions).
     */
-    export function getSynthesizedDeepClone<T extends Node | undefined>(node: T, includeTrivia = true): T {
-        const clone = node && getSynthesizedDeepCloneWorker(node as NonNullable<T>);
+    export function getSynthesizedDeepClone<T extends Node | undefined>(node: T, includeTrivia = true, renameMap?: Map<Identifier>, checker?: TypeChecker, callback?: (originalNode: Node, clone: Node) => any): T {


I'd rather this be left as-is and have the extra logic in the refactor code file. I don't want getSynthesizedDeepClone to turn into a Swiss Army Knife of Arbitrary Transforms

I suggested putting it here because it requires a deep clone. Are you okay with the code-fix-specific helper being largely copied from the shared helper?

amcasey · 2018-08-08T22:49:10Z

tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_CatchFollowedByThen.js

+async function f(){
+    let result;
+    try {
+        let result_1 = await fetch("https://typescriptlang.org");


Why not const?

amcasey · 2018-08-08T23:52:18Z

src/services/codefixes/convertToAsyncFunction.ts

+    interface SynthIdentifier {
+        identifier: Identifier;
+        types: Type[];
+        numberOfAssignmentsOriginal: number;


Is numberOfAssignmentsOriginal still useful or can you just use types.length?

amcasey · 2018-08-09T21:04:17Z

src/services/utilities.ts

@@ -1626,14 +1626,34 @@ namespace ts {
     * WARNING: This is an expensive operation and is only intended to be used in refactorings
     * and code fixes (because those are triggered by explicit user actions).
     */
-    export function getSynthesizedDeepClone<T extends Node | undefined>(node: T, includeTrivia = true): T {
-        const clone = node && getSynthesizedDeepCloneWorker(node as NonNullable<T>);
+    export function getSynthesizedDeepClone<T extends Node | undefined>(node: T, includeTrivia = true, renameMap?: Map<Identifier>, checker?: TypeChecker, callback?: (originalNode: Node, clone: Node) => any): T {


I suggested putting it here because it requires a deep clone. Are you okay with the code-fix-specific helper being largely copied from the shared helper?

…code review comments regarding the original types map

amcasey · 2018-08-09T22:20:14Z

src/services/codefixes/convertToAsyncFunction.ts

+        synthNamesMap: Map<SynthIdentifier>; // keys are the symbol id of the identifier
+        allVarNames: SymbolAndIdentifier[];
+        setOfExpressionsToReturn: Map<true>; // keys are the node ids of the expressions
+        context: CodeFixContextBase;


This appears to be unused.

amcasey · 2018-08-09T22:33:27Z

src/services/codefixes/convertToAsyncFunction.ts

+                    if (hasPrevArgName && !shouldReturn) {
+                        const type = transformer.checker.getTypeAtLocation(func);
+                        const callSignatures = type && transformer.checker.getSignaturesOfType(type, SignatureKind.Call);
+                        const returnType = callSignatures && callSignatures[0].getReturnType();


I don't think all overloads have to have the same return type in TS.

amcasey · 2018-08-09T22:35:21Z

src/services/codefixes/convertToAsyncFunction.ts

+    }
+
+    function declaredInFile(symbol: Symbol, sourceFile: SourceFile): boolean {
+        return symbol.valueDeclaration && symbol.valueDeclaration.getSourceFile() === sourceFile;


It doesn't look like symbol.valueDeclaration can be undefined.

amcasey · 2018-08-09T22:41:25Z

src/services/codefixes/convertToAsyncFunction.ts

+
+        return getSynthesizedDeepClone(nodeToRename, /*includeTrivia*/ true, identsToRenameMap, checker, deepCloneCallback);
+
+        function isExpressionOrCallOnTypePromise(child: Node): boolean {


It looks like child: SignatureDeclaration.

amcasey · 2018-08-09T22:43:34Z

src/services/codefixes/convertToAsyncFunction.ts

+    }
+
+    // dispatch function to recursively build the refactoring
+    function transformExpression(node: Expression, transformer: Transformer, outermostParent: CallExpression, prevArgName?: SynthIdentifier): Statement[] {


Should prevArgName be in Transformer?

amcasey · 2018-08-09T22:43:50Z

src/services/codefixes/convertToAsyncFunction.ts

+        }
+
+        let nodeType = transformer.checker.getTypeAtLocation(node);
+        if (nodeType && nodeType.flags & TypeFlags.Any && (<IntrinsicType>nodeType).intrinsicName === "error" && isIdentifier(node)) {


We talked about eliminating the error-type check.

amcasey · 2018-08-09T22:48:42Z

src/services/codefixes/convertToAsyncFunction.ts

+
+    function transformPromiseCall(node: Expression, transformer: Transformer, prevArgName?: SynthIdentifier): Statement[] {
+        const shouldReturn = transformer.setOfExpressionsToReturn.get(getNodeId(node).toString());
+        const hasPrevArgName = prevArgName && prevArgName.identifier.text.length > 0;


Under what circumstances is the identifier empty?

amcasey · 2018-08-09T22:56:05Z

src/services/codefixes/convertToAsyncFunction.ts

+        return createNodeArray([]);
+    }
+
+    function getReturnStatementsWithPromiseHandlersIndices(block: Block): number[] {


Why not just check getReturnStatementsWithPromiseHandlers in the loop above?

amcasey · 2018-08-09T23:04:30Z

src/services/codefixes/convertToAsyncFunction.ts

+                    const temp = transformExpression(node, transformer, node, prevArgName);
+                    innerCbBody = innerCbBody.concat(temp);
+                    if (innerCbBody.length > 0) {
+                        return;


Does this early return have any effect?

amcasey · 2018-08-09T23:06:49Z

src/services/codefixes/convertToAsyncFunction.ts

+
+        if (isFunctionLikeDeclaration(funcNode)) {
+            if (funcNode.parameters.length > 0) {
+                const param = funcNode.parameters[0].name as Identifier;


Why is this cast safe?

amcasey · 2018-08-09T23:08:39Z

src/services/codefixes/convertToAsyncFunction.ts

+        }
+
+        function getSymbol(node: Node): Symbol | undefined {
+            return node.symbol ? node.symbol : transformer.checker.getSymbolAtLocation(node);


Why not check node.symbol in the other places where getSymbolAtLocation is called (or in getSymbolAtLocation itself)? Note: this is a question, not a prompt - I don't understand the difference between the two mechanisms.

amcasey · 2018-08-09T23:09:07Z

src/services/codefixes/convertToAsyncFunction.ts

+        function getMapEntryIfExists(node: Identifier): SynthIdentifier {
+            const originalNode = getOriginalNode(node);
+            const symbol = getSymbol(originalNode);
+            const identifier = node;


Just call the parameter "identifier"?

amcasey · 2018-08-09T23:35:23Z

src/services/suggestionDiagnostics.ts

@@ -109,7 +113,64 @@ namespace ts {
        }
    }

+    function addConvertToAsyncFunctionDiagnostics(node: FunctionLikeDeclaration, checker: TypeChecker, diags: DiagnosticWithLocation[]): void {
+
+        const functionType = node.type ? checker.getTypeFromTypeNode(node.type) : undefined;


This should get the type of node and not consume node.type directly.

if (isAsyncFunction(node) || !node.body) { return; } const functionType = checker.getTypeAtLocation(node); if (!functionType) { return; }

amcasey · 2018-08-09T23:42:32Z

function f() {
	return Promise.resolve().then(() => 1, () => "a"); 
}

Generates catch(), which is an error.

Same thing if the parameter is named _.

amcasey · 2018-08-09T23:47:04Z

function f() {
	return Promise.resolve().then(x => 1).catch(x => "a").then(x => !!x); 
}

Generates

async function f() {
	let x_1: string | number;
    try {
        const x = await Promise.resolve();
        x_1 = 1;
    }
    catch (x_1) {
        x_1 = "a";
    }
    return !!x_1; 
}

There's a collision between the catch parameter and let x_1.

amcasey · 2018-08-10T00:04:21Z

function f() {
	return Promise.resolve().then(function g(x) {
		try {
			return Promise.resolve().then(y => 1);
		}
		catch (e) {
			e;
		}
	}); 
}

The try-catch is eliminated, which seems questionable.

amcasey · 2018-08-10T00:09:23Z

function f() {
	return Promise.resolve().then(f ? (x => 1) : (y => 2));
}

Generates

async function f() {
	await Promise.resolve();
}

billti · 2018-08-10T15:49:43Z

Re the comment above that

"There's a collision between the catch parameter and let x_1"

A catch clause introduces a new lexical scope, and its parameters don't impact outside the scope. (It's a little more nuanced than that - the answer on https://stackoverflow.com/questions/7926237/javascript-catch-clause-scope is a good reference).

amcasey · 2018-08-10T17:14:52Z

@billti My concern was that the assignment in the catch block seemed to update the catch parameter, rather than the local being used for the return value.

amcasey · 2018-08-10T23:05:46Z

Subsumed by #26373

elizabethdinella requested review from DanielRosenwasser, billti, RyanCavanaugh and amcasey June 13, 2018 23:06

mhegazy requested a review from a user June 13, 2018 23:30

mhegazy reviewed Jun 13, 2018

View reviewed changes

DanielRosenwasser requested changes Jun 14, 2018

View reviewed changes

ghost reviewed Jun 14, 2018

View reviewed changes

tests/cases/fourslash/codeFixPromiseToAsync.ts Outdated

reportsUnnecessary: true,

}]);

verify.codeFix({

Copy link

ghost Jun 14, 2018

Choose a reason for hiding this comment

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

The description here looks wrong.

lbguilherme reviewed Jun 14, 2018

View reviewed changes

ghost reviewed Jun 14, 2018

View reviewed changes

mhegazy reviewed Jun 14, 2018

View reviewed changes

elizabethdinella added 12 commits June 15, 2018 15:07

updated build files

f3971df

Initial commit of changing all functions that return a promise to async

6f803f6

adding await to .then() works

b8d8183

Try block is added

69795c8

major refactor - still buggy

6c0a712

Fixed to allow for non inlined functions in a then()

546ee1b

Got catch blocks and onRej functions working

8019335

Fixed try/catch formatting

6e31ff9

Initial commit of recursive function to create cascading catch blocks

469da81

Fixed catch param names

8c8ebe9

First commit of nested trys and cascading catches

635cc04

Fixed the outermost catch to take the correct variable

5099bd1

elizabethdinella added 3 commits August 8, 2018 16:52

Fixed linting errors

39ba3ea

Addressed code review comments and removed extraneous comments

8777657

Added tests and baselines

e6c676e

elizabethdinella force-pushed the promisesToAsyncAwaitCodeFix branch from 0719ad2 to e6c676e Compare August 9, 2018 18:28

RyanCavanaugh requested changes Aug 9, 2018

View reviewed changes

amcasey reviewed Aug 9, 2018

View reviewed changes

Merged with master

25edf4c

elizabethdinella force-pushed the promisesToAsyncAwaitCodeFix branch from 8b17532 to 25edf4c Compare August 9, 2018 21:29

elizabethdinella added 2 commits August 9, 2018 15:03

Addressed code review comments

54b6f2a

Created a rename synthesized deep clone function and addressed other …

e2c02d6

…code review comments regarding the original types map

amcasey reviewed Aug 9, 2018

View reviewed changes

elizabethdinella added 4 commits August 9, 2018 17:19

addressed more code review comments

1e72a46

code review changes

ee994f1

added tests and baselines

8bcac69

Got rid of unnecessary function and fixed linting errors

05cb7bf

Fixed linting error

12bd6fc

elizabethdinella closed this Aug 10, 2018

		@@ -0,0 +1,6 @@
		/// <reference path='fourslash.ts' />

		////import./**/


		return getSynthesizedDeepClone(nodeToRename, /includeTrivia/ true, identsToRenameMap, checker, deepCloneCallback);

		function isExpressionOrCallOnTypePromise(child: Node): boolean {

[WIP] Promises to Async Await Code Fix #24939

[WIP] Promises to Async Await Code Fix #24939

Conversation

elizabethdinella commented Jun 13, 2018

elizabethdinella commented Jun 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost Jun 14, 2018 • edited by ghost Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RyanCavanaugh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amcasey commented Aug 9, 2018 • edited Loading

amcasey commented Aug 9, 2018

amcasey commented Aug 10, 2018

amcasey commented Aug 10, 2018

billti commented Aug 10, 2018

amcasey commented Aug 10, 2018

amcasey commented Aug 10, 2018

ghost Jun 14, 2018 •

edited by ghost

Loading

amcasey commented Aug 9, 2018 •

edited

Loading