Minor fixes to "Convert To Async" refactor

src/compiler/checker.ts

@@ -634,6 +634,8 @@ namespace ts {
             getESSymbolType: () => esSymbolType,
             getNeverType: () => neverType,
             getOptionalType: () => optionalType,
+            getPromiseType: () => getGlobalPromiseType(/*reportErrors*/ false),
+            getPromiseLikeType: () => getGlobalPromiseLikeType(/*reportErrors*/ false),
             isSymbolAccessible,
             isArrayType,
             isTupleType,

src/compiler/factory/nodeFactory.ts

@@ -4939,14 +4939,16 @@ namespace ts {
         }
 
         // @api
-        function createCatchClause(variableDeclaration: string | VariableDeclaration | undefined, block: Block) {
+        function createCatchClause(variableDeclaration: string | BindingName | VariableDeclaration | undefined, block: Block) {
             const node = createBaseNode<CatchClause>(SyntaxKind.CatchClause);
-            variableDeclaration = !isString(variableDeclaration) ? variableDeclaration : createVariableDeclaration(
-                variableDeclaration,
-                /*exclamationToken*/ undefined,
-                /*type*/ undefined,
-                /*initializer*/ undefined
-            );
+            if (typeof variableDeclaration === "string" || variableDeclaration && !isVariableDeclaration(variableDeclaration)) {
+                variableDeclaration = createVariableDeclaration(
+                    variableDeclaration,
+                    /*exclamationToken*/ undefined,
+                    /*type*/ undefined,
+                    /*initializer*/ undefined
+                );
+            }
             node.variableDeclaration = variableDeclaration;
             node.block = block;
             node.transformFlags |=

src/compiler/types.ts

@@ -4264,6 +4264,8 @@ namespace ts {
         /* @internal */ createArrayType(elementType: Type): Type;
         /* @internal */ getElementTypeOfArrayType(arrayType: Type): Type | undefined;
         /* @internal */ createPromiseType(type: Type): Type;
+        /* @internal */ getPromiseType(): Type;
+        /* @internal */ getPromiseLikeType(): Type;
 
         /* @internal */ isTypeAssignableTo(source: Type, target: Type): boolean;
         /* @internal */ createAnonymousType(symbol: Symbol | undefined, members: SymbolTable, callSignatures: Signature[], constructSignatures: Signature[], indexInfos: IndexInfo[]): Type;
@@ -7447,7 +7449,7 @@ namespace ts {
         updateDefaultClause(node: DefaultClause, statements: readonly Statement[]): DefaultClause;
         createHeritageClause(token: HeritageClause["token"], types: readonly ExpressionWithTypeArguments[]): HeritageClause;
         updateHeritageClause(node: HeritageClause, types: readonly ExpressionWithTypeArguments[]): HeritageClause;
-        createCatchClause(variableDeclaration: string | VariableDeclaration | undefined, block: Block): CatchClause;
+        createCatchClause(variableDeclaration: string | BindingName | VariableDeclaration | undefined, block: Block): CatchClause;
         updateCatchClause(node: CatchClause, variableDeclaration: VariableDeclaration | undefined, block: Block): CatchClause;
 
         //

src/compiler/utilities.ts

@@ -1364,7 +1364,7 @@ namespace ts {
 
     // Warning: This has the same semantics as the forEach family of functions,
     //          in that traversal terminates in the event that 'visitor' supplies a truthy value.
-    export function forEachReturnStatement<T>(body: Block, visitor: (stmt: ReturnStatement) => T): T | undefined {
+    export function forEachReturnStatement<T>(body: Block | Statement, visitor: (stmt: ReturnStatement) => T): T | undefined {
 
         return traverse(body);
 

src/services/suggestionDiagnostics.ts

@@ -139,33 +139,40 @@ namespace ts {
     // Should be kept up to date with transformExpression in convertToAsyncFunction.ts
     export function isFixablePromiseHandler(node: Node, checker: TypeChecker): boolean {
         // ensure outermost call exists and is a promise handler
-        if (!isPromiseHandler(node) || !node.arguments.every(arg => isFixablePromiseArgument(arg, checker))) {
+        if (!isPromiseHandler(node) || !hasSupportedNumberOfArguments(node) || !node.arguments.every(arg => isFixablePromiseArgument(arg, checker))) {
             return false;
         }
 
         // ensure all chained calls are valid
-        let currentNode = node.expression;
+        let currentNode = node.expression.expression;
         while (isPromiseHandler(currentNode) || isPropertyAccessExpression(currentNode)) {
-            if (isCallExpression(currentNode) && !currentNode.arguments.every(arg => isFixablePromiseArgument(arg, checker))) {
-                return false;
+            if (isCallExpression(currentNode)) {
+                if (!hasSupportedNumberOfArguments(currentNode) || !currentNode.arguments.every(arg => isFixablePromiseArgument(arg, checker))) {
+                    return false;
+                }
+                currentNode = currentNode.expression.expression;
+            }
+            else {
+                currentNode = currentNode.expression;
             }
-            currentNode = currentNode.expression;
         }
         return true;
     }
 
-    function isPromiseHandler(node: Node): node is CallExpression {
+    function isPromiseHandler(node: Node): node is CallExpression & { readonly expression: PropertyAccessExpression } {
         return isCallExpression(node) && (
-            hasPropertyAccessExpressionWithName(node, "then") && hasSupportedNumberOfArguments(node) ||
-            hasPropertyAccessExpressionWithName(node, "catch"));
-    }
-
-    function hasSupportedNumberOfArguments(node: CallExpression) {
-        if (node.arguments.length > 2) return false;
-        if (node.arguments.length < 2) return true;
-        return some(node.arguments, arg => {
-            return arg.kind === SyntaxKind.NullKeyword ||
-                isIdentifier(arg) && arg.text === "undefined";
+            hasPropertyAccessExpressionWithName(node, "then") ||
+            hasPropertyAccessExpressionWithName(node, "catch") ||
+            hasPropertyAccessExpressionWithName(node, "finally"));
+    }
+
+    function hasSupportedNumberOfArguments(node: CallExpression & { readonly expression: PropertyAccessExpression }) {
+        const name = node.expression.name.text;
+        const maxArguments = name === "then" ? 2 : name === "catch" ? 1 : name === "finally" ? 1 : 0;
+        if (node.arguments.length > maxArguments) return false;
+        if (node.arguments.length < maxArguments) return true;
+        return maxArguments === 1 || some(node.arguments, arg => {
+            return arg.kind === SyntaxKind.NullKeyword || isIdentifier(arg) && arg.text === "undefined";
         });
     }
 

src/testRunner/unittests/services/convertToAsyncFunction.ts

@@ -53,6 +53,13 @@ interface Promise<T> {
      * @returns A Promise for the completion of the callback.
      */
     catch<TResult = never>(onrejected?: ((reason: any) => TResult | PromiseLike<TResult>) | undefined | null): Promise<T | TResult>;
+    /**
+     * Attaches a callback that is invoked when the Promise is settled (fulfilled or rejected). The
+     * resolved value cannot be modified from the callback.
+     * @param onfinally The callback to execute when the Promise is settled (fulfilled or rejected).
+     * @returns A Promise for the completion of the callback.
+     */
+    finally(onfinally?: (() => void) | undefined | null): Promise<T>
 }
 interface PromiseConstructor {
     /**
@@ -277,7 +284,30 @@ interface Array<T> {}`
         }
     }
 
-    function testConvertToAsyncFunction(it: Mocha.PendingTestFunction, caption: string, text: string, baselineFolder: string, includeLib?: boolean, includeModule?: boolean, expectFailure = false, onlyProvideAction = false) {
+    const enum ConvertToAsyncTestFlags {
+        None,
+        IncludeLib = 1 << 0,
+        IncludeModule = 1 << 1,
+        ExpectSuggestionDiagnostic = 1 << 2,
+        ExpectNoSuggestionDiagnostic = 1 << 3,
+        ExpectAction = 1 << 4,
+        ExpectNoAction = 1 << 5,
+
+        ExpectSuccess = ExpectSuggestionDiagnostic | ExpectAction,
+        ExpectFailed = ExpectNoSuggestionDiagnostic | ExpectNoAction,
+    }
+
+    function testConvertToAsyncFunction(it: Mocha.PendingTestFunction, caption: string, text: string, baselineFolder: string, flags: ConvertToAsyncTestFlags) {
+        const includeLib = !!(flags & ConvertToAsyncTestFlags.IncludeLib);
+        const includeModule = !!(flags & ConvertToAsyncTestFlags.IncludeModule);
+        const expectSuggestionDiagnostic = !!(flags & ConvertToAsyncTestFlags.ExpectSuggestionDiagnostic);
+        const expectNoSuggestionDiagnostic = !!(flags & ConvertToAsyncTestFlags.ExpectNoSuggestionDiagnostic);
+        const expectAction = !!(flags & ConvertToAsyncTestFlags.ExpectAction);
+        const expectNoAction = !!(flags & ConvertToAsyncTestFlags.ExpectNoAction);
+        const expectFailure = expectNoSuggestionDiagnostic || expectNoAction;
+        Debug.assert(!(expectSuggestionDiagnostic && expectNoSuggestionDiagnostic), "Cannot combine both 'ExpectSuggestionDiagnostic' and 'ExpectNoSuggestionDiagnostic'");
+        Debug.assert(!(expectAction && expectNoAction), "Cannot combine both 'ExpectAction' and 'ExpectNoAction'");
+
         const t = extractTest(text);
         const selectionRange = t.ranges.get("selection")!;
         if (!selectionRange) {
@@ -320,35 +350,47 @@ interface Array<T> {}`
             const diagnostics = languageService.getSuggestionDiagnostics(f.path);
             const diagnostic = find(diagnostics, diagnostic => diagnostic.messageText === Diagnostics.This_may_be_converted_to_an_async_function.message &&
                 diagnostic.start === context.span.start && diagnostic.length === context.span.length);
-            if (expectFailure) {
-                assert.isUndefined(diagnostic);
-            }
-            else {
-                assert.exists(diagnostic);
-            }
-
             const actions = codefix.getFixes(context);
             const action = find(actions, action => action.description === Diagnostics.Convert_to_async_function.message);
-            if (expectFailure && !onlyProvideAction) {
-                assert.isNotTrue(action && action.changes.length > 0);
-                return;
-            }
 
-            assert.isTrue(action && action.changes.length > 0);
+            let outputText: string | null;
+            if (action?.changes.length) {
+                const data: string[] = [];
+                data.push(`// ==ORIGINAL==`);
+                data.push(text.replace("[#|", "/*[#|*/").replace("|]", "/*|]*/"));
+                const changes = action.changes;
+                assert.lengthOf(changes, 1);
+
+                data.push(`// ==ASYNC FUNCTION::${action.description}==`);
+                const newText = textChanges.applyChanges(sourceFile.text, changes[0].textChanges);
+                data.push(newText);
+
+                const diagProgram = makeLanguageService({ path, content: newText }, includeLib, includeModule).getProgram()!;
+                assert.isFalse(hasSyntacticDiagnostics(diagProgram));
+                outputText = data.join(newLineCharacter);
+            }
+            else {
+                // eslint-disable-next-line no-null/no-null
+                outputText = null;
+            }
 
-            const data: string[] = [];
-            data.push(`// ==ORIGINAL==`);
-            data.push(text.replace("[#|", "/*[#|*/").replace("|]", "/*|]*/"));
-            const changes = action!.changes;
-            assert.lengthOf(changes, 1);
+            Harness.Baseline.runBaseline(`${baselineFolder}/${caption}${extension}`, outputText);
 
-            data.push(`// ==ASYNC FUNCTION::${action!.description}==`);
-            const newText = textChanges.applyChanges(sourceFile.text, changes[0].textChanges);
-            data.push(newText);
+            if (expectNoSuggestionDiagnostic) {
+                assert.isUndefined(diagnostic, "Expected code fix to not provide a suggestion diagnostic");
+            }
+            else if (expectSuggestionDiagnostic) {
+                assert.exists(diagnostic, "Expected code fix to provide a suggestion diagnostic");
+            }
 
-            const diagProgram = makeLanguageService({ path, content: newText }, includeLib, includeModule).getProgram()!;
-            assert.isFalse(hasSyntacticDiagnostics(diagProgram));
-            Harness.Baseline.runBaseline(`${baselineFolder}/${caption}${extension}`, data.join(newLineCharacter));
+            if (expectNoAction) {
+                assert.isNotTrue(!!action?.changes.length, "Expected code fix to not provide an action");
+                assert.isNotTrue(typeof outputText === "string", "Expected code fix to not apply changes");
+            }
+            else if (expectAction) {
+                assert.isTrue(!!action?.changes.length, "Expected code fix to provide an action");
+                assert.isTrue(typeof outputText === "string", "Expected code fix to apply changes");
+            }
         }
 
         function makeLanguageService(file: TestFSWithWatch.File, includeLib?: boolean, includeModule?: boolean) {
@@ -372,19 +414,23 @@ interface Array<T> {}`
     }
 
     const _testConvertToAsyncFunction = createTestWrapper((it, caption: string, text: string) => {
-        testConvertToAsyncFunction(it, caption, text, "convertToAsyncFunction", /*includeLib*/ true);
+        testConvertToAsyncFunction(it, caption, text, "convertToAsyncFunction", ConvertToAsyncTestFlags.IncludeLib | ConvertToAsyncTestFlags.ExpectSuccess);
     });
 
     const _testConvertToAsyncFunctionFailed = createTestWrapper((it, caption: string, text: string) => {
-        testConvertToAsyncFunction(it, caption, text, "convertToAsyncFunction", /*includeLib*/ true, /*includeModule*/ false, /*expectFailure*/ true);
+        testConvertToAsyncFunction(it, caption, text, "convertToAsyncFunction", ConvertToAsyncTestFlags.IncludeLib | ConvertToAsyncTestFlags.ExpectFailed);
     });
 
     const _testConvertToAsyncFunctionFailedSuggestion = createTestWrapper((it, caption: string, text: string) => {
-        testConvertToAsyncFunction(it, caption, text, "convertToAsyncFunction", /*includeLib*/ true, /*includeModule*/ false, /*expectFailure*/ true, /*onlyProvideAction*/ true);
+        testConvertToAsyncFunction(it, caption, text, "convertToAsyncFunction", ConvertToAsyncTestFlags.IncludeLib | ConvertToAsyncTestFlags.ExpectNoSuggestionDiagnostic | ConvertToAsyncTestFlags.ExpectAction);
+    });
+
+    const _testConvertToAsyncFunctionFailedAction = createTestWrapper((it, caption: string, text: string) => {
+        testConvertToAsyncFunction(it, caption, text, "convertToAsyncFunction", ConvertToAsyncTestFlags.IncludeLib | ConvertToAsyncTestFlags.ExpectSuggestionDiagnostic | ConvertToAsyncTestFlags.ExpectNoAction);
     });
 
     const _testConvertToAsyncFunctionWithModule = createTestWrapper((it, caption: string, text: string) => {
-        testConvertToAsyncFunction(it, caption, text, "convertToAsyncFunction", /*includeLib*/ true, /*includeModule*/ true);
+        testConvertToAsyncFunction(it, caption, text, "convertToAsyncFunction", ConvertToAsyncTestFlags.IncludeLib | ConvertToAsyncTestFlags.IncludeModule | ConvertToAsyncTestFlags.ExpectSuccess);
     });
 
     describe("unittests:: services:: convertToAsyncFunction", () => {
@@ -435,11 +481,11 @@ function [#|f|](): Promise<void>{
 function [#|f|]():Promise<void> {
     return fetch('https://typescriptlang.org').then(result => { console.log(result); }).catch(err => { console.log(err); });
 }`);
-        _testConvertToAsyncFunction("convertToAsyncFunction_CatchAndRej", `
+        _testConvertToAsyncFunctionFailed("convertToAsyncFunction_CatchAndRej", `
 function [#|f|]():Promise<void> {
     return fetch('https://typescriptlang.org').then(result => { console.log(result); }, rejection => { console.log("rejected:", rejection); }).catch(err => { console.log(err) });
 }`);
-        _testConvertToAsyncFunction("convertToAsyncFunction_CatchAndRejRef", `
+        _testConvertToAsyncFunctionFailed("convertToAsyncFunction_CatchAndRejRef", `
 function [#|f|]():Promise<void> {
     return fetch('https://typescriptlang.org').then(res, rej).catch(catch_err)
 }
@@ -1718,5 +1764,76 @@ function [#|foo|](p: Promise<string[]>) {
 }
 `);
 
+        _testConvertToAsyncFunction("convertToAsyncFunction_thenNoArguments", `
+declare function foo(): Promise<number>;
+function [#|f|](): Promise<number> {
+    return foo().then();
+}`);
+        _testConvertToAsyncFunction("convertToAsyncFunction_catchNoArguments", `
+declare function foo(): Promise<number>;
+function [#|f|](): Promise<number> {
+    return foo().catch();
+}`);
+        _testConvertToAsyncFunction("convertToAsyncFunction_chainedThenCatchThen", `
+declare function foo(): Promise<number>;
+function [#|f|](): Promise<number> {
+    return foo().then(x => Promise.resolve(x + 1)).catch(() => 1).then(y => y + 2);
+}`);
+        _testConvertToAsyncFunction("convertToAsyncFunction_finally", `
+declare function foo(): Promise<number>;
+function [#|f|](): Promise<number> {
+    return foo().finally(() => console.log("done"));
+}`);
+        _testConvertToAsyncFunction("convertToAsyncFunction_finallyNoArguments", `
+declare function foo(): Promise<number>;
+function [#|f|](): Promise<number> {
+    return foo().finally();
+}`);
+        _testConvertToAsyncFunction("convertToAsyncFunction_finallyNull", `
+declare function foo(): Promise<number>;
+function [#|f|](): Promise<number> {
+    return foo().finally(null);
+}`);
+        _testConvertToAsyncFunction("convertToAsyncFunction_finallyUndefined", `
+declare function foo(): Promise<number>;
+function [#|f|](): Promise<number> {
+    return foo().finally(undefined);
+}`);
+        _testConvertToAsyncFunction("convertToAsyncFunction_thenFinally", `
+declare function foo(): Promise<number>;
+function [#|f|](): Promise<number> {
+    return foo().then(x => x + 1).finally(() => console.log("done"));
+}`);
+        _testConvertToAsyncFunction("convertToAsyncFunction_thenFinallyThen", `
+declare function foo(): Promise<number>;
+function [#|f|](): Promise<number> {
+    return foo().then(x => Promise.resolve(x + 1)).finally(() => console.log("done")).then(y => y + 2);
+}`);
+        _testConvertToAsyncFunctionFailedAction("convertToAsyncFunction_returnInBranch", `
+declare function foo(): Promise<number>;
+function [#|f|](): Promise<number> {
+    return foo().then(() => {
+        if (Math.random()) {
+            return 1;
+        }
+        return 2;
+    }).then(a => {
+        return a + 1;
+    });
+}
+`);
+        _testConvertToAsyncFunctionFailedAction("convertToAsyncFunction_partialReturnInBranch", `
+declare function foo(): Promise<number>;
+function [#|f|](): Promise<number> {
+    return foo().then(() => {
+        if (Math.random()) {
+            return 1;
+        }
+        console.log("foo");
+    }).then(a => {
+        return a + 1;
+    });
+}
+`);
     });
 }