From 711e570241f3f47d2bb21edcefd45808426615d2 Mon Sep 17 00:00:00 2001 From: Ron Buckton Date: Sat, 21 Aug 2021 16:46:21 -0700 Subject: [PATCH 1/4] Minor fixes to convertToAsync --- .../codefixes/convertToAsyncFunction.ts | 151 ++++++++++++++---- src/services/suggestionDiagnostics.ts | 3 +- .../services/convertToAsyncFunction.ts | 53 ++++++ ...tion_CatchFollowedByThenMismatchTypes03.ts | 2 +- .../convertToAsyncFunction_LocalReturn.js | 2 +- .../convertToAsyncFunction_LocalReturn.ts | 2 +- .../convertToAsyncFunction_NoCatchHandler.js | 6 +- .../convertToAsyncFunction_NoCatchHandler.ts | 6 +- .../convertToAsyncFunction_NoRes.ts | 2 +- .../convertToAsyncFunction_NoRes2.ts | 2 +- .../convertToAsyncFunction_NoRes3.ts | 2 +- .../convertToAsyncFunction_NoRes4.js | 2 +- .../convertToAsyncFunction_NoRes4.ts | 2 +- .../convertToAsyncFunction_Param2.ts | 2 +- ...convertToAsyncFunction_PromiseCallInner.js | 2 +- ...convertToAsyncFunction_PromiseCallInner.ts | 2 +- .../convertToAsyncFunction_Return1.ts | 2 +- .../convertToAsyncFunction_Return2.ts | 2 +- .../convertToAsyncFunction_Return3.ts | 2 +- ...on_catchBlockUniqueParamsBindingPattern.js | 2 +- ...on_catchBlockUniqueParamsBindingPattern.ts | 2 +- ...convertToAsyncFunction_catchNoArguments.ts | 12 ++ ...nvertToAsyncFunction_catchTypeArgument1.ts | 4 +- ...ertToAsyncFunction_chainedThenCatchThen.ts | 19 +++ .../convertToAsyncFunction_emptyCatch1.js | 4 +- .../convertToAsyncFunction_emptyCatch1.ts | 4 +- .../convertToAsyncFunction_emptyCatch2.js | 6 +- .../convertToAsyncFunction_emptyCatch2.ts | 6 +- .../convertToAsyncFunction_finally.ts | 16 ++ ...nvertToAsyncFunction_finallyNoArguments.ts | 12 ++ .../convertToAsyncFunction_finallyNull.ts | 12 ++ ...convertToAsyncFunction_finallyUndefined.ts | 12 ++ .../convertToAsyncFunction_thenFinally.ts | 17 ++ .../convertToAsyncFunction_thenFinallyThen.ts | 19 +++ .../convertToAsyncFunction_thenNoArguments.ts | 12 ++ 35 files changed, 335 insertions(+), 71 deletions(-) create mode 100644 tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_catchNoArguments.ts create mode 100644 tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_chainedThenCatchThen.ts create mode 100644 tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_finally.ts create mode 100644 tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_finallyNoArguments.ts create mode 100644 tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_finallyNull.ts create mode 100644 tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_finallyUndefined.ts create mode 100644 tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_thenFinally.ts create mode 100644 tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_thenFinallyThen.ts create mode 100644 tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_thenNoArguments.ts diff --git a/src/services/codefixes/convertToAsyncFunction.ts b/src/services/codefixes/convertToAsyncFunction.ts index 5445ad590f5ad..62b119bde224a 100644 --- a/src/services/codefixes/convertToAsyncFunction.ts +++ b/src/services/codefixes/convertToAsyncFunction.ts @@ -43,6 +43,12 @@ namespace ts.codefix { readonly isInJSFile: boolean; } + interface PromiseReturningCallExpression extends CallExpression { + readonly expression: PropertyAccessExpression & { + readonly escapedText: Name; + }; + } + function convertToAsyncFunction(changes: textChanges.ChangeTracker, sourceFile: SourceFile, position: number, checker: TypeChecker): void { // get the function declaration - returns a promise const tokenAtPosition = getTokenAtPosition(sourceFile, position); @@ -84,7 +90,7 @@ namespace ts.codefix { for (const returnStatement of returnStatements) { forEachChild(returnStatement, function visit(node) { if (isCallExpression(node)) { - const newNodes = transformExpression(node, transformer); + const newNodes = transformExpression(node, node, transformer); changes.replaceNodeWithNodes(sourceFile, returnStatement, newNodes); } else if (!isFunctionLike(node)) { @@ -121,7 +127,13 @@ namespace ts.codefix { // if .catch() is the last call in the chain, move leftward in the chain until we hit something else that should be returned forEachChild(node, visit); } + else if (isPromiseReturningCallExpression(node, checker, "finally")) { + setOfExpressionsToReturn.add(getNodeId(node)); + // if .finally() is the last call in the chain, move leftward in the chain until we hit something else that should be returned + forEachChild(node, visit); + } else if (isPromiseTypedExpression(node, checker)) { + // TODO(rbuckton): Do we need this if we have `returnContextNode`? setOfExpressionsToReturn.add(getNodeId(node)); // don't recurse here, since we won't refactor any children or arguments of the expression } @@ -133,7 +145,7 @@ namespace ts.codefix { return setOfExpressionsToReturn; } - function isPromiseReturningCallExpression(node: Node, checker: TypeChecker, name: string): node is CallExpression { + function isPromiseReturningCallExpression(node: Node, checker: TypeChecker, name: Name): node is PromiseReturningCallExpression { if (!isCallExpression(node)) return false; const isExpressionOfName = hasPropertyAccessExpressionWithName(node, name); const nodeType = isExpressionOfName && checker.getTypeAtLocation(node); @@ -236,30 +248,103 @@ namespace ts.codefix { // dispatch function to recursively build the refactoring // should be kept up to date with isFixablePromiseHandler in suggestionDiagnostics.ts - function transformExpression(node: Expression, transformer: Transformer, prevArgName?: SynthBindingName): readonly Statement[] { + function transformExpression(returnContextNode: Expression, node: Expression, transformer: Transformer, prevArgName?: SynthBindingName): readonly Statement[] { if (isPromiseReturningCallExpression(node, transformer.checker, "then")) { - if (node.arguments.length === 0) return silentFail(); return transformThen(node, transformer, prevArgName); } if (isPromiseReturningCallExpression(node, transformer.checker, "catch")) { return transformCatch(node, transformer, prevArgName); } + if (isPromiseReturningCallExpression(node, transformer.checker, "finally")) { + return transformFinally(node, transformer, prevArgName); + } + // TODO(rbuckton): Do we need this if we have `returnContextNode`? if (isPropertyAccessExpression(node)) { - return transformExpression(node.expression, transformer, prevArgName); + return transformExpression(returnContextNode, node.expression, transformer, prevArgName); } const nodeType = transformer.checker.getTypeAtLocation(node); if (nodeType && transformer.checker.getPromisedTypeOfPromise(nodeType)) { - Debug.assertNode(node.original!.parent, isPropertyAccessExpression); - return transformPromiseExpressionOfPropertyAccess(node, transformer, prevArgName); + Debug.assertNode(getOriginalNode(node).parent, isPropertyAccessExpression); + return transformPromiseExpressionOfPropertyAccess(returnContextNode, node, transformer, prevArgName); } return silentFail(); } - function transformCatch(node: CallExpression, transformer: Transformer, prevArgName?: SynthBindingName): readonly Statement[] { - const func = singleOrUndefined(node.arguments); - const argName = func ? getArgBindingName(func, transformer) : undefined; + function isNullOrUndefinedOrMissing({ checker }: Transformer, node: Expression) { + if (node.kind === SyntaxKind.NullKeyword) return true; + if (isIdentifier(node) && !isGeneratedIdentifier(node) && idText(node) === "undefined") { + const symbol = checker.getSymbolAtLocation(node); + return !symbol || checker.isUndefinedSymbol(symbol); + } + return false; + } + + function createUniqueSynthName(prevArgName: SynthIdentifier): SynthIdentifier { + const renamedPrevArg = factory.createUniqueName(prevArgName.identifier.text, GeneratedIdentifierFlags.Optimistic); + return createSynthIdentifier(renamedPrevArg); + } + + function transformFinally(node: PromiseReturningCallExpression<"finally">, transformer: Transformer, prevArgName?: SynthBindingName): readonly Statement[] { + const callback = node.arguments.length > 0 ? node.arguments[0] : undefined; + if (!callback || isNullOrUndefinedOrMissing(transformer, callback)) { + return transformExpression(/* returnContextNode */ node, node.expression.expression, transformer, prevArgName); + } + + let possibleNameForVarDecl: SynthIdentifier | undefined; + + // If there is another call in the chain after the .catch() we are transforming, we will need to save the result of both paths (try block and catch block) + // To do this, we will need to synthesize a variable that we were not aware of while we were adding identifiers to the synthNamesMap + // We will use the prevArgName and then update the synthNamesMap with a new variable name for the next transformation step + if (prevArgName && !shouldReturn(node, transformer)) { + if (isSynthIdentifier(prevArgName)) { + possibleNameForVarDecl = prevArgName; + transformer.synthNamesMap.forEach((val, key) => { + if (val.identifier.text === prevArgName.identifier.text) { + const newSynthName = createUniqueSynthName(prevArgName); + transformer.synthNamesMap.set(key, newSynthName); + } + }); + } + else { + possibleNameForVarDecl = createSynthIdentifier(factory.createUniqueName("result", GeneratedIdentifierFlags.Optimistic), prevArgName.types); + } + + // We are about to write a 'let' variable declaration, but `transformExpression` for both + // the try block and catch block will assign to this name. Setting this flag indicates + // that future assignments should be written as `name = value` instead of `const name = value`. + possibleNameForVarDecl.hasBeenDeclared = true; + } + + const tryBlock = factory.createBlock(transformExpression(/*returnContextNode*/ node, node.expression.expression, transformer, possibleNameForVarDecl)); + const transformationBody = callback ? getTransformationBody(callback, /*prevArgName*/ undefined, /*argName*/ undefined, node, transformer) : emptyArray; + const finallyBlock = factory.createBlock(transformationBody); + const tryStatement = factory.createTryStatement(tryBlock, /*catchClause*/ undefined, finallyBlock); + + // In order to avoid an implicit any, we will synthesize a type for the declaration using the unions of the types of both paths (try block and catch block) + let varDeclList: VariableStatement | undefined; + let varDeclIdentifier: Identifier | undefined; + if (possibleNameForVarDecl && !shouldReturn(node, transformer)) { + varDeclIdentifier = getSynthesizedDeepClone(possibleNameForVarDecl.identifier); + const typeArray: Type[] = possibleNameForVarDecl.types; + const unionType = transformer.checker.getUnionType(typeArray, UnionReduction.Subtype); + const unionTypeNode = transformer.isInJSFile ? undefined : transformer.checker.typeToTypeNode(unionType, /*enclosingDeclaration*/ undefined, /*flags*/ undefined); + const varDecl = [factory.createVariableDeclaration(varDeclIdentifier, /*exclamationToken*/ undefined, unionTypeNode)]; + varDeclList = factory.createVariableStatement(/*modifiers*/ undefined, factory.createVariableDeclarationList(varDecl, NodeFlags.Let)); + } + + const destructuredResult = prevArgName && varDeclIdentifier && isSynthBindingPattern(prevArgName) + && factory.createVariableStatement(/*modifiers*/ undefined, factory.createVariableDeclarationList([factory.createVariableDeclaration(getSynthesizedDeepClone(prevArgName.bindingPattern), /*exclamationToken*/ undefined, /*type*/ undefined, varDeclIdentifier)], NodeFlags.Const)); + return compact([varDeclList, tryStatement, destructuredResult]); + } + + function transformCatchWorker(node: PromiseReturningCallExpression<"catch" | "then">, callback: Expression | undefined, transformer: Transformer, prevArgName?: SynthBindingName): readonly Statement[] { + if (!callback || isNullOrUndefinedOrMissing(transformer, callback)) { + return transformExpression(/* returnContextNode */ node, node.expression.expression, transformer, prevArgName); + } + + const argName = callback ? getArgBindingName(callback, transformer) : undefined; let possibleNameForVarDecl: SynthIdentifier | undefined; // If there is another call in the chain after the .catch() we are transforming, we will need to save the result of both paths (try block and catch block) @@ -285,10 +370,10 @@ namespace ts.codefix { possibleNameForVarDecl.hasBeenDeclared = true; } - const tryBlock = factory.createBlock(transformExpression(node.expression, transformer, possibleNameForVarDecl)); - const transformationBody = func ? getTransformationBody(func, possibleNameForVarDecl, argName, node, transformer) : emptyArray; - const catchArg = argName ? isSynthIdentifier(argName) ? argName.identifier.text : argName.bindingPattern : "e"; - const catchVariableDeclaration = factory.createVariableDeclaration(catchArg); + const tryBlock = factory.createBlock(transformExpression(/*returnContextNode*/ node, node.expression.expression, transformer, possibleNameForVarDecl)); + const transformationBody = callback ? getTransformationBody(callback, possibleNameForVarDecl, argName, node, transformer) : emptyArray; + const catchArg = argName ? isSynthIdentifier(argName) ? argName.identifier.text : argName.bindingPattern : undefined; + const catchVariableDeclaration = catchArg !== undefined ? factory.createVariableDeclaration(catchArg) : undefined; const catchClause = factory.createCatchClause(catchVariableDeclaration, factory.createBlock(transformationBody)); // In order to avoid an implicit any, we will synthesize a type for the declaration using the unions of the types of both paths (try block and catch block) @@ -309,33 +394,39 @@ namespace ts.codefix { return compact([varDeclList, tryStatement, destructuredResult]); } - function createUniqueSynthName(prevArgName: SynthIdentifier): SynthIdentifier { - const renamedPrevArg = factory.createUniqueName(prevArgName.identifier.text, GeneratedIdentifierFlags.Optimistic); - return createSynthIdentifier(renamedPrevArg); + function transformCatch(node: PromiseReturningCallExpression<"catch">, transformer: Transformer, prevArgName?: SynthBindingName): readonly Statement[] { + const func = singleOrUndefined(node.arguments); + return transformCatchWorker(node, func, transformer, prevArgName); } - function transformThen(node: CallExpression, transformer: Transformer, prevArgName?: SynthBindingName): readonly Statement[] { - const [onFulfilled, onRejected] = node.arguments; + function transformThen(node: PromiseReturningCallExpression<"then">, transformer: Transformer, prevArgName?: SynthBindingName): readonly Statement[] { + const onFulfilled = node.arguments.length > 0 ? node.arguments[0] : undefined; + const onRejected = node.arguments.length > 1 ? node.arguments[1] : undefined; + if (!onFulfilled || isNullOrUndefinedOrMissing(transformer, onFulfilled)) { + return transformCatchWorker(node, onRejected, transformer, prevArgName); + } + const onFulfilledArgumentName = getArgBindingName(onFulfilled, transformer); const transformationBody = getTransformationBody(onFulfilled, prevArgName, onFulfilledArgumentName, node, transformer); - if (onRejected) { + if (onRejected && !isNullOrUndefinedOrMissing(transformer, onRejected)) { const onRejectedArgumentName = getArgBindingName(onRejected, transformer); - const tryBlock = factory.createBlock(transformExpression(node.expression, transformer, onFulfilledArgumentName).concat(transformationBody)); + const tryBlock = factory.createBlock(transformExpression(node.expression, node.expression, transformer, onFulfilledArgumentName).concat(transformationBody)); const transformationBody2 = getTransformationBody(onRejected, prevArgName, onRejectedArgumentName, node, transformer); - const catchArg = onRejectedArgumentName ? isSynthIdentifier(onRejectedArgumentName) ? onRejectedArgumentName.identifier.text : onRejectedArgumentName.bindingPattern : "e"; - const catchVariableDeclaration = factory.createVariableDeclaration(catchArg); + const catchArg = onRejectedArgumentName ? isSynthIdentifier(onRejectedArgumentName) ? onRejectedArgumentName.identifier.text : onRejectedArgumentName.bindingPattern : undefined; + const catchVariableDeclaration = catchArg !== undefined ? factory.createVariableDeclaration(catchArg) : undefined; const catchClause = factory.createCatchClause(catchVariableDeclaration, factory.createBlock(transformationBody2)); return [factory.createTryStatement(tryBlock, catchClause, /* finallyBlock */ undefined)]; } - return transformExpression(node.expression, transformer, onFulfilledArgumentName).concat(transformationBody); + + return transformExpression(node.expression.expression, node.expression.expression, transformer, onFulfilledArgumentName).concat(transformationBody); } /** * Transforms the 'x' part of `x.then(...)`, or the 'y()' part of `y().catch(...)`, where 'x' and 'y()' are Promises. */ - function transformPromiseExpressionOfPropertyAccess(node: Expression, transformer: Transformer, prevArgName?: SynthBindingName): readonly Statement[] { - if (shouldReturn(node, transformer)) { - return [factory.createReturnStatement(getSynthesizedDeepClone(node))]; + function transformPromiseExpressionOfPropertyAccess(returnContextNode: Expression, node: Expression, transformer: Transformer, prevArgName?: SynthBindingName): readonly Statement[] { + if (shouldReturn(returnContextNode, transformer)) { + return [factory.createReturnStatement(factory.createAwaitExpression(getSynthesizedDeepClone(node)))]; } return createVariableOrAssignmentOrExpressionStatement(prevArgName, factory.createAwaitExpression(node), /*typeAnnotation*/ undefined); @@ -402,7 +493,7 @@ namespace ts.codefix { const returnType = callSignatures[0].getReturnType(); const varDeclOrAssignment = createVariableOrAssignmentOrExpressionStatement(prevArgName, factory.createAwaitExpression(synthCall), parent.typeArguments?.[0]); if (prevArgName) { - prevArgName.types.push(returnType); + prevArgName.types.push(transformer.checker.getAwaitedType(returnType) || returnType); } return varDeclOrAssignment; @@ -452,7 +543,7 @@ namespace ts.codefix { if (!shouldReturn(parent, transformer)) { const transformedStatement = createVariableOrAssignmentOrExpressionStatement(prevArgName, possiblyAwaitedRightHandSide, /*typeAnnotation*/ undefined); if (prevArgName) { - prevArgName.types.push(returnType); + prevArgName.types.push(transformer.checker.getAwaitedType(returnType) || returnType); } return transformedStatement; } @@ -517,7 +608,7 @@ namespace ts.codefix { for (const stmt of innerRetStmts) { forEachChild(stmt, function visit(node) { if (isCallExpression(node)) { - const temp = transformExpression(node, transformer, prevArgName); + const temp = transformExpression(node, node, transformer, prevArgName); innerCbBody = innerCbBody.concat(temp); if (innerCbBody.length > 0) { return; diff --git a/src/services/suggestionDiagnostics.ts b/src/services/suggestionDiagnostics.ts index 09740f8f2eef4..d42b4a01c6c5f 100644 --- a/src/services/suggestionDiagnostics.ts +++ b/src/services/suggestionDiagnostics.ts @@ -157,7 +157,8 @@ namespace ts { function isPromiseHandler(node: Node): node is CallExpression { return isCallExpression(node) && ( hasPropertyAccessExpressionWithName(node, "then") && hasSupportedNumberOfArguments(node) || - hasPropertyAccessExpressionWithName(node, "catch")); + hasPropertyAccessExpressionWithName(node, "catch") || + hasPropertyAccessExpressionWithName(node, "finally")); } function hasSupportedNumberOfArguments(node: CallExpression) { diff --git a/src/testRunner/unittests/services/convertToAsyncFunction.ts b/src/testRunner/unittests/services/convertToAsyncFunction.ts index d73877b259f59..78197e59e2413 100644 --- a/src/testRunner/unittests/services/convertToAsyncFunction.ts +++ b/src/testRunner/unittests/services/convertToAsyncFunction.ts @@ -53,6 +53,13 @@ interface Promise { * @returns A Promise for the completion of the callback. */ catch(onrejected?: ((reason: any) => TResult | PromiseLike) | undefined | null): Promise; + /** + * 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 } interface PromiseConstructor { /** @@ -1718,5 +1725,51 @@ function [#|foo|](p: Promise) { } `); + _testConvertToAsyncFunction("convertToAsyncFunction_thenNoArguments", ` +declare function foo(): Promise; +function [#|f|](): Promise { + return foo().then(); +}`); + _testConvertToAsyncFunction("convertToAsyncFunction_catchNoArguments", ` +declare function foo(): Promise; +function [#|f|](): Promise { + return foo().catch(); +}`); + _testConvertToAsyncFunction("convertToAsyncFunction_chainedThenCatchThen", ` +declare function foo(): Promise; +function [#|f|](): Promise { + return foo().then(x => Promise.resolve(x + 1)).catch(() => 1).then(y => y + 2); +}`); + _testConvertToAsyncFunction("convertToAsyncFunction_finally", ` +declare function foo(): Promise; +function [#|f|](): Promise { + return foo().finally(() => console.log("done")); +}`); + _testConvertToAsyncFunction("convertToAsyncFunction_finallyNoArguments", ` +declare function foo(): Promise; +function [#|f|](): Promise { + return foo().finally(); +}`); + _testConvertToAsyncFunction("convertToAsyncFunction_finallyNull", ` +declare function foo(): Promise; +function [#|f|](): Promise { + return foo().finally(null); +}`); + _testConvertToAsyncFunction("convertToAsyncFunction_finallyUndefined", ` +declare function foo(): Promise; +function [#|f|](): Promise { + return foo().finally(undefined); +}`); + _testConvertToAsyncFunction("convertToAsyncFunction_thenFinally", ` +declare function foo(): Promise; +function [#|f|](): Promise { + return foo().then(x => x + 1).finally(() => console.log("done")); +}`); + _testConvertToAsyncFunction("convertToAsyncFunction_thenFinallyThen", ` +declare function foo(): Promise; +function [#|f|](): Promise { + return foo().then(x => Promise.resolve(x + 1)).finally(() => console.log("done")).then(y => y + 2); +}`); + }); } diff --git a/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_CatchFollowedByThenMismatchTypes03.ts b/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_CatchFollowedByThenMismatchTypes03.ts index 23e446282fca7..d0de85630e58a 100644 --- a/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_CatchFollowedByThenMismatchTypes03.ts +++ b/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_CatchFollowedByThenMismatchTypes03.ts @@ -15,7 +15,7 @@ function rej(reject){ // ==ASYNC FUNCTION::Convert to async function== async function f(){ - let result: number | Promise; + let result: number; try { const result_1 = await fetch("https://typescriptlang.org"); result = await res(result_1); diff --git a/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_LocalReturn.js b/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_LocalReturn.js index f90801ad9e1d1..7943f9cd8a11e 100644 --- a/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_LocalReturn.js +++ b/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_LocalReturn.js @@ -11,7 +11,7 @@ function /*[#|*/f/*|]*/() { async function f() { let x = fetch("https://typescriptlang.org").then(res => console.log(res)); try { - return x; + return await x; } catch (err) { return console.log("Error!", err); } diff --git a/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_LocalReturn.ts b/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_LocalReturn.ts index f90801ad9e1d1..7943f9cd8a11e 100644 --- a/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_LocalReturn.ts +++ b/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_LocalReturn.ts @@ -11,7 +11,7 @@ function /*[#|*/f/*|]*/() { async function f() { let x = fetch("https://typescriptlang.org").then(res => console.log(res)); try { - return x; + return await x; } catch (err) { return console.log("Error!", err); } diff --git a/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_NoCatchHandler.js b/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_NoCatchHandler.js index 01b8c5fa18a8a..bc3d31abb8f73 100644 --- a/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_NoCatchHandler.js +++ b/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_NoCatchHandler.js @@ -7,8 +7,6 @@ function /*[#|*/f/*|]*/() { // ==ASYNC FUNCTION::Convert to async function== async function f() { - try { - const x = await fetch('https://typescriptlang.org'); - return x.statusText; - } catch (e) { } + const x = await fetch('https://typescriptlang.org'); + return x.statusText; } diff --git a/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_NoCatchHandler.ts b/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_NoCatchHandler.ts index 01b8c5fa18a8a..bc3d31abb8f73 100644 --- a/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_NoCatchHandler.ts +++ b/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_NoCatchHandler.ts @@ -7,8 +7,6 @@ function /*[#|*/f/*|]*/() { // ==ASYNC FUNCTION::Convert to async function== async function f() { - try { - const x = await fetch('https://typescriptlang.org'); - return x.statusText; - } catch (e) { } + const x = await fetch('https://typescriptlang.org'); + return x.statusText; } diff --git a/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_NoRes.ts b/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_NoRes.ts index 3bc8dbf5f5aeb..c8695ce84bcd7 100644 --- a/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_NoRes.ts +++ b/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_NoRes.ts @@ -8,7 +8,7 @@ function /*[#|*/f/*|]*/():Promise { async function f():Promise { try { - await fetch('https://typescriptlang.org'); + return await fetch('https://typescriptlang.org'); } catch (rejection) { return console.log("rejected:", rejection); } diff --git a/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_NoRes2.ts b/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_NoRes2.ts index ada11e4260127..5f2b295e22e74 100644 --- a/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_NoRes2.ts +++ b/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_NoRes2.ts @@ -8,7 +8,7 @@ function /*[#|*/f/*|]*/():Promise { async function f():Promise { try { - await fetch('https://typescriptlang.org'); + return await fetch('https://typescriptlang.org'); } catch (rej) { return console.log(rej); } diff --git a/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_NoRes3.ts b/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_NoRes3.ts index d088aba90155f..059d058d0378e 100644 --- a/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_NoRes3.ts +++ b/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_NoRes3.ts @@ -8,7 +8,7 @@ function /*[#|*/f/*|]*/():Promise { async function f():Promise { try { - return fetch('https://typescriptlang.org'); + return await fetch('https://typescriptlang.org'); } catch (rej) { return console.log(rej); } diff --git a/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_NoRes4.js b/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_NoRes4.js index 61f93fa11e2c2..41a8e1a601f78 100644 --- a/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_NoRes4.js +++ b/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_NoRes4.js @@ -8,7 +8,7 @@ function /*[#|*/f/*|]*/() { async function f() { try { - await fetch('https://typescriptlang.org'); + return await fetch('https://typescriptlang.org'); } catch (rejection) { return console.log("rejected:", rejection); } diff --git a/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_NoRes4.ts b/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_NoRes4.ts index 61f93fa11e2c2..41a8e1a601f78 100644 --- a/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_NoRes4.ts +++ b/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_NoRes4.ts @@ -8,7 +8,7 @@ function /*[#|*/f/*|]*/() { async function f() { try { - await fetch('https://typescriptlang.org'); + return await fetch('https://typescriptlang.org'); } catch (rejection) { return console.log("rejected:", rejection); } diff --git a/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_Param2.ts b/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_Param2.ts index f40c9bf5befc3..c0c7b41324ba6 100644 --- a/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_Param2.ts +++ b/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_Param2.ts @@ -16,7 +16,7 @@ function my_print (resp): Promise { async function f() { try { - return my_print(fetch("https://typescriptlang.org").then(res => console.log(res))); + return await my_print(fetch("https://typescriptlang.org").then(res => console.log(res))); } catch (err) { return console.log("Error!", err); } diff --git a/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_PromiseCallInner.js b/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_PromiseCallInner.js index 621612a256ae9..45d0ba4e665ea 100644 --- a/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_PromiseCallInner.js +++ b/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_PromiseCallInner.js @@ -9,7 +9,7 @@ function /*[#|*/f/*|]*/() { async function f() { try { - return fetch(Promise.resolve(1).then(res => "https://typescriptlang.org")); + return await fetch(Promise.resolve(1).then(res => "https://typescriptlang.org")); } catch (err) { return console.log(err); } diff --git a/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_PromiseCallInner.ts b/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_PromiseCallInner.ts index 621612a256ae9..45d0ba4e665ea 100644 --- a/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_PromiseCallInner.ts +++ b/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_PromiseCallInner.ts @@ -9,7 +9,7 @@ function /*[#|*/f/*|]*/() { async function f() { try { - return fetch(Promise.resolve(1).then(res => "https://typescriptlang.org")); + return await fetch(Promise.resolve(1).then(res => "https://typescriptlang.org")); } catch (err) { return console.log(err); } diff --git a/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_Return1.ts b/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_Return1.ts index 75e1d23902e86..9f170402cf709 100644 --- a/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_Return1.ts +++ b/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_Return1.ts @@ -9,7 +9,7 @@ function /*[#|*/f/*|]*/(p: Promise) { async function f(p: Promise) { try { - return p; + return await p; } catch (error) { return await Promise.reject(error); } diff --git a/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_Return2.ts b/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_Return2.ts index 01fea0275c6f8..e0cfb4d43d471 100644 --- a/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_Return2.ts +++ b/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_Return2.ts @@ -7,7 +7,7 @@ function /*[#|*/f/*|]*/(p: Promise) { async function f(p: Promise) { try { - return p; + return await p; } catch (error) { return await Promise.reject(error); } diff --git a/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_Return3.ts b/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_Return3.ts index 43cbe7e36b4e9..06d06cc105e93 100644 --- a/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_Return3.ts +++ b/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_Return3.ts @@ -9,7 +9,7 @@ function /*[#|*/f/*|]*/(p: Promise) { async function f(p: Promise) { try { - return p; + return await p; } catch (error) { return await Promise.reject(error); } diff --git a/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_catchBlockUniqueParamsBindingPattern.js b/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_catchBlockUniqueParamsBindingPattern.js index 845c27d45c92b..0318c204f5500 100644 --- a/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_catchBlockUniqueParamsBindingPattern.js +++ b/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_catchBlockUniqueParamsBindingPattern.js @@ -11,7 +11,7 @@ async function f() { try { await Promise.resolve(); result = ({ x: 3 }); - } catch (e) { + } catch { result = ({ x: "a" }); } const { x } = result; diff --git a/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_catchBlockUniqueParamsBindingPattern.ts b/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_catchBlockUniqueParamsBindingPattern.ts index 9bc5e4b178b4f..a913668facc8c 100644 --- a/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_catchBlockUniqueParamsBindingPattern.ts +++ b/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_catchBlockUniqueParamsBindingPattern.ts @@ -11,7 +11,7 @@ async function f() { try { await Promise.resolve(); result = ({ x: 3 }); - } catch (e) { + } catch { result = ({ x: "a" }); } const { x } = result; diff --git a/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_catchNoArguments.ts b/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_catchNoArguments.ts new file mode 100644 index 0000000000000..a8a361df9809f --- /dev/null +++ b/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_catchNoArguments.ts @@ -0,0 +1,12 @@ +// ==ORIGINAL== + +declare function foo(): Promise; +function /*[#|*/f/*|]*/(): Promise { + return foo().catch(); +} +// ==ASYNC FUNCTION::Convert to async function== + +declare function foo(): Promise; +async function f(): Promise { + return await foo(); +} \ No newline at end of file diff --git a/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_catchTypeArgument1.ts b/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_catchTypeArgument1.ts index 728f09dc4d647..69be7b60e7b85 100644 --- a/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_catchTypeArgument1.ts +++ b/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_catchTypeArgument1.ts @@ -14,9 +14,9 @@ type APIResponse = { success: true, data: T } | { success: false }; async function get() { try { - return Promise + return await Promise .resolve>({ success: true, data: { email: "" } }); - } catch (e) { + } catch { const result: APIResponse<{ email: string; }> = ({ success: false }); return result; } diff --git a/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_chainedThenCatchThen.ts b/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_chainedThenCatchThen.ts new file mode 100644 index 0000000000000..6b4a3e848743a --- /dev/null +++ b/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_chainedThenCatchThen.ts @@ -0,0 +1,19 @@ +// ==ORIGINAL== + +declare function foo(): Promise; +function /*[#|*/f/*|]*/(): Promise { + return foo().then(x => Promise.resolve(x + 1)).catch(() => 1).then(y => y + 2); +} +// ==ASYNC FUNCTION::Convert to async function== + +declare function foo(): Promise; +async function f(): Promise { + let y: number; + try { + const x = await foo(); + y = await Promise.resolve(x + 1); + } catch { + y = 1; + } + return y + 2; +} \ No newline at end of file diff --git a/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_emptyCatch1.js b/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_emptyCatch1.js index 20fa127d9908d..a0fedb4b2b9a5 100644 --- a/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_emptyCatch1.js +++ b/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_emptyCatch1.js @@ -7,7 +7,5 @@ function /*[#|*/f/*|]*/() { // ==ASYNC FUNCTION::Convert to async function== async function f() { - try { - return Promise.resolve(); - } catch (e) { } + return await Promise.resolve(); } diff --git a/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_emptyCatch1.ts b/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_emptyCatch1.ts index 20fa127d9908d..a0fedb4b2b9a5 100644 --- a/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_emptyCatch1.ts +++ b/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_emptyCatch1.ts @@ -7,7 +7,5 @@ function /*[#|*/f/*|]*/() { // ==ASYNC FUNCTION::Convert to async function== async function f() { - try { - return Promise.resolve(); - } catch (e) { } + return await Promise.resolve(); } diff --git a/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_emptyCatch2.js b/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_emptyCatch2.js index aa7e9f1bb8db3..3dd3969df5991 100644 --- a/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_emptyCatch2.js +++ b/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_emptyCatch2.js @@ -7,8 +7,6 @@ function /*[#|*/f/*|]*/() { // ==ASYNC FUNCTION::Convert to async function== async function f() { - try { - const x = await Promise.resolve(0); - return x; - } catch (e) { } + const x = await Promise.resolve(0); + return x; } diff --git a/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_emptyCatch2.ts b/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_emptyCatch2.ts index aa7e9f1bb8db3..3dd3969df5991 100644 --- a/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_emptyCatch2.ts +++ b/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_emptyCatch2.ts @@ -7,8 +7,6 @@ function /*[#|*/f/*|]*/() { // ==ASYNC FUNCTION::Convert to async function== async function f() { - try { - const x = await Promise.resolve(0); - return x; - } catch (e) { } + const x = await Promise.resolve(0); + return x; } diff --git a/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_finally.ts b/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_finally.ts new file mode 100644 index 0000000000000..cf622bd6079a5 --- /dev/null +++ b/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_finally.ts @@ -0,0 +1,16 @@ +// ==ORIGINAL== + +declare function foo(): Promise; +function /*[#|*/f/*|]*/(): Promise { + return foo().finally(() => console.log("done")); +} +// ==ASYNC FUNCTION::Convert to async function== + +declare function foo(): Promise; +async function f(): Promise { + try { + return await foo(); + } finally { + return console.log("done"); + } +} \ No newline at end of file diff --git a/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_finallyNoArguments.ts b/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_finallyNoArguments.ts new file mode 100644 index 0000000000000..d2464518f5d6b --- /dev/null +++ b/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_finallyNoArguments.ts @@ -0,0 +1,12 @@ +// ==ORIGINAL== + +declare function foo(): Promise; +function /*[#|*/f/*|]*/(): Promise { + return foo().finally(); +} +// ==ASYNC FUNCTION::Convert to async function== + +declare function foo(): Promise; +async function f(): Promise { + return await foo(); +} \ No newline at end of file diff --git a/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_finallyNull.ts b/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_finallyNull.ts new file mode 100644 index 0000000000000..f69afd06ca876 --- /dev/null +++ b/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_finallyNull.ts @@ -0,0 +1,12 @@ +// ==ORIGINAL== + +declare function foo(): Promise; +function /*[#|*/f/*|]*/(): Promise { + return foo().finally(null); +} +// ==ASYNC FUNCTION::Convert to async function== + +declare function foo(): Promise; +async function f(): Promise { + return await foo(); +} \ No newline at end of file diff --git a/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_finallyUndefined.ts b/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_finallyUndefined.ts new file mode 100644 index 0000000000000..8d979bdcb646d --- /dev/null +++ b/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_finallyUndefined.ts @@ -0,0 +1,12 @@ +// ==ORIGINAL== + +declare function foo(): Promise; +function /*[#|*/f/*|]*/(): Promise { + return foo().finally(undefined); +} +// ==ASYNC FUNCTION::Convert to async function== + +declare function foo(): Promise; +async function f(): Promise { + return await foo(); +} \ No newline at end of file diff --git a/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_thenFinally.ts b/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_thenFinally.ts new file mode 100644 index 0000000000000..b1e83fadacb67 --- /dev/null +++ b/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_thenFinally.ts @@ -0,0 +1,17 @@ +// ==ORIGINAL== + +declare function foo(): Promise; +function /*[#|*/f/*|]*/(): Promise { + return foo().then(x => x + 1).finally(() => console.log("done")); +} +// ==ASYNC FUNCTION::Convert to async function== + +declare function foo(): Promise; +async function f(): Promise { + try { + const x = await foo(); + return x + 1; + } finally { + return console.log("done"); + } +} \ No newline at end of file diff --git a/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_thenFinallyThen.ts b/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_thenFinallyThen.ts new file mode 100644 index 0000000000000..d1d8eea561195 --- /dev/null +++ b/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_thenFinallyThen.ts @@ -0,0 +1,19 @@ +// ==ORIGINAL== + +declare function foo(): Promise; +function /*[#|*/f/*|]*/(): Promise { + return foo().then(x => Promise.resolve(x + 1)).finally(() => console.log("done")).then(y => y + 2); +} +// ==ASYNC FUNCTION::Convert to async function== + +declare function foo(): Promise; +async function f(): Promise { + let y: number; + try { + const x = await foo(); + y = await Promise.resolve(x + 1); + } finally { + console.log("done"); + } + return y + 2; +} \ No newline at end of file diff --git a/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_thenNoArguments.ts b/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_thenNoArguments.ts new file mode 100644 index 0000000000000..916db17d4bd35 --- /dev/null +++ b/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_thenNoArguments.ts @@ -0,0 +1,12 @@ +// ==ORIGINAL== + +declare function foo(): Promise; +function /*[#|*/f/*|]*/(): Promise { + return foo().then(); +} +// ==ASYNC FUNCTION::Convert to async function== + +declare function foo(): Promise; +async function f(): Promise { + return await foo(); +} \ No newline at end of file From 7dfc3c52ca227c8d72fe99d247088b64c64817db Mon Sep 17 00:00:00 2001 From: Ron Buckton Date: Thu, 26 Aug 2021 16:48:19 -0700 Subject: [PATCH 2/4] Back out on nested return in inner continuation --- src/compiler/factory/nodeFactory.ts | 16 +- src/compiler/types.ts | 2 +- src/compiler/utilities.ts | 2 +- .../codefixes/convertToAsyncFunction.ts | 388 +++++++++++------- src/services/suggestionDiagnostics.ts | 32 +- .../services/convertToAsyncFunction.ts | 126 ++++-- .../reference/api/tsserverlibrary.d.ts | 4 +- tests/baselines/reference/api/typescript.d.ts | 4 +- .../convertToAsyncFunction_CatchAndRej.ts | 19 - .../convertToAsyncFunction_CatchAndRejRef.ts | 37 -- 10 files changed, 370 insertions(+), 260 deletions(-) delete mode 100644 tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_CatchAndRej.ts delete mode 100644 tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_CatchAndRejRef.ts diff --git a/src/compiler/factory/nodeFactory.ts b/src/compiler/factory/nodeFactory.ts index f39576f3e4acf..c697a04860b71 100644 --- a/src/compiler/factory/nodeFactory.ts +++ b/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(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 |= diff --git a/src/compiler/types.ts b/src/compiler/types.ts index d055b91a93a5e..4cd8fb97e315d 100644 --- a/src/compiler/types.ts +++ b/src/compiler/types.ts @@ -7445,7 +7445,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; // diff --git a/src/compiler/utilities.ts b/src/compiler/utilities.ts index 9ba15712f6630..f5dfe4e00c2d4 100644 --- a/src/compiler/utilities.ts +++ b/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(body: Block, visitor: (stmt: ReturnStatement) => T): T | undefined { + export function forEachReturnStatement(body: Block | Statement, visitor: (stmt: ReturnStatement) => T): T | undefined { return traverse(body); diff --git a/src/services/codefixes/convertToAsyncFunction.ts b/src/services/codefixes/convertToAsyncFunction.ts index 62b119bde224a..008c65883a784 100644 --- a/src/services/codefixes/convertToAsyncFunction.ts +++ b/src/services/codefixes/convertToAsyncFunction.ts @@ -34,6 +34,7 @@ namespace ts.codefix { readonly types: Type[]; /** A declaration for this identifier has already been generated */ hasBeenDeclared: boolean; + hasBeenReferenced: boolean; } interface Transformer { @@ -90,7 +91,7 @@ namespace ts.codefix { for (const returnStatement of returnStatements) { forEachChild(returnStatement, function visit(node) { if (isCallExpression(node)) { - const newNodes = transformExpression(node, node, transformer); + const newNodes = transformExpression(node, node, transformer, /*hasContinuation*/ false); changes.replaceNodeWithNodes(sourceFile, returnStatement, newNodes); } else if (!isFunctionLike(node)) { @@ -133,7 +134,6 @@ namespace ts.codefix { forEachChild(node, visit); } else if (isPromiseTypedExpression(node, checker)) { - // TODO(rbuckton): Do we need this if we have `returnContextNode`? setOfExpressionsToReturn.add(getNodeId(node)); // don't recurse here, since we won't refactor any children or arguments of the expression } @@ -241,6 +241,10 @@ namespace ts.codefix { return createSynthIdentifier(identifier); } + function hasFailed() { + return !codeActionSucceeded; + } + function silentFail() { codeActionSucceeded = false; return emptyArray; @@ -248,31 +252,34 @@ namespace ts.codefix { // dispatch function to recursively build the refactoring // should be kept up to date with isFixablePromiseHandler in suggestionDiagnostics.ts - function transformExpression(returnContextNode: Expression, node: Expression, transformer: Transformer, prevArgName?: SynthBindingName): readonly Statement[] { + /** + * @param hasContinuation Whether another `then`, `catch`, or `finally` continuation follows the continuation to which this expression belongs. + * @param continuationArgName The argument name for the continuation that follows this call. + */ + function transformExpression(returnContextNode: Expression, node: Expression, transformer: Transformer, hasContinuation: boolean, continuationArgName?: SynthBindingName): readonly Statement[] { if (isPromiseReturningCallExpression(node, transformer.checker, "then")) { - return transformThen(node, transformer, prevArgName); + return transformThen(node, elementAt(node.arguments, 0), elementAt(node.arguments, 1), transformer, hasContinuation, continuationArgName); } if (isPromiseReturningCallExpression(node, transformer.checker, "catch")) { - return transformCatch(node, transformer, prevArgName); + return transformCatch(node, elementAt(node.arguments, 0), transformer, hasContinuation, continuationArgName); } if (isPromiseReturningCallExpression(node, transformer.checker, "finally")) { - return transformFinally(node, transformer, prevArgName); + return transformFinally(node, elementAt(node.arguments, 0), transformer, hasContinuation, continuationArgName); } - // TODO(rbuckton): Do we need this if we have `returnContextNode`? if (isPropertyAccessExpression(node)) { - return transformExpression(returnContextNode, node.expression, transformer, prevArgName); + return transformExpression(returnContextNode, node.expression, transformer, hasContinuation, continuationArgName); } const nodeType = transformer.checker.getTypeAtLocation(node); if (nodeType && transformer.checker.getPromisedTypeOfPromise(nodeType)) { Debug.assertNode(getOriginalNode(node).parent, isPropertyAccessExpression); - return transformPromiseExpressionOfPropertyAccess(returnContextNode, node, transformer, prevArgName); + return transformPromiseExpressionOfPropertyAccess(returnContextNode, node, transformer, continuationArgName); } return silentFail(); } - function isNullOrUndefinedOrMissing({ checker }: Transformer, node: Expression) { + function isNullOrUndefined({ checker }: Transformer, node: Expression) { if (node.kind === SyntaxKind.NullKeyword) return true; if (isIdentifier(node) && !isGeneratedIdentifier(node) && idText(node) === "undefined") { const symbol = checker.getSymbolAtLocation(node); @@ -286,150 +293,164 @@ namespace ts.codefix { return createSynthIdentifier(renamedPrevArg); } - function transformFinally(node: PromiseReturningCallExpression<"finally">, transformer: Transformer, prevArgName?: SynthBindingName): readonly Statement[] { - const callback = node.arguments.length > 0 ? node.arguments[0] : undefined; - if (!callback || isNullOrUndefinedOrMissing(transformer, callback)) { - return transformExpression(/* returnContextNode */ node, node.expression.expression, transformer, prevArgName); - } - + function getPossibleNameForVarDecl(node: PromiseReturningCallExpression<"then" | "catch" | "finally">, transformer: Transformer, continuationArgName?: SynthBindingName) { let possibleNameForVarDecl: SynthIdentifier | undefined; - // If there is another call in the chain after the .catch() we are transforming, we will need to save the result of both paths (try block and catch block) - // To do this, we will need to synthesize a variable that we were not aware of while we were adding identifiers to the synthNamesMap - // We will use the prevArgName and then update the synthNamesMap with a new variable name for the next transformation step - if (prevArgName && !shouldReturn(node, transformer)) { - if (isSynthIdentifier(prevArgName)) { - possibleNameForVarDecl = prevArgName; + // If there is another call in the chain after the .catch() or .finally() we are transforming, we will need to save the result of both paths + // (try block and catch/finally block). To do this, we will need to synthesize a variable that we were not aware of while we were adding + // identifiers to the synthNamesMap. We will use the continuationArgName and then update the synthNamesMap with a new variable name for + // the next transformation step + + if (continuationArgName && !shouldReturn(node, transformer)) { + if (isSynthIdentifier(continuationArgName)) { + possibleNameForVarDecl = continuationArgName; transformer.synthNamesMap.forEach((val, key) => { - if (val.identifier.text === prevArgName.identifier.text) { - const newSynthName = createUniqueSynthName(prevArgName); + if (val.identifier.text === continuationArgName.identifier.text) { + const newSynthName = createUniqueSynthName(continuationArgName); transformer.synthNamesMap.set(key, newSynthName); } }); } else { - possibleNameForVarDecl = createSynthIdentifier(factory.createUniqueName("result", GeneratedIdentifierFlags.Optimistic), prevArgName.types); + possibleNameForVarDecl = createSynthIdentifier(factory.createUniqueName("result", GeneratedIdentifierFlags.Optimistic), continuationArgName.types); } // We are about to write a 'let' variable declaration, but `transformExpression` for both - // the try block and catch block will assign to this name. Setting this flag indicates + // the try block and catch/finally block will assign to this name. Setting this flag indicates // that future assignments should be written as `name = value` instead of `const name = value`. - possibleNameForVarDecl.hasBeenDeclared = true; + declareSynthIdentifier(possibleNameForVarDecl); } - const tryBlock = factory.createBlock(transformExpression(/*returnContextNode*/ node, node.expression.expression, transformer, possibleNameForVarDecl)); - const transformationBody = callback ? getTransformationBody(callback, /*prevArgName*/ undefined, /*argName*/ undefined, node, transformer) : emptyArray; - const finallyBlock = factory.createBlock(transformationBody); - const tryStatement = factory.createTryStatement(tryBlock, /*catchClause*/ undefined, finallyBlock); + return possibleNameForVarDecl; + } + + function finishCatchOrFinallyTransform(node: PromiseReturningCallExpression<"then" | "catch" | "finally">, transformer: Transformer, tryStatement: TryStatement, possibleNameForVarDecl: SynthIdentifier | undefined, continuationArgName?: SynthBindingName) { + const statements: Statement[] = []; // In order to avoid an implicit any, we will synthesize a type for the declaration using the unions of the types of both paths (try block and catch block) - let varDeclList: VariableStatement | undefined; let varDeclIdentifier: Identifier | undefined; + if (possibleNameForVarDecl && !shouldReturn(node, transformer)) { - varDeclIdentifier = getSynthesizedDeepClone(possibleNameForVarDecl.identifier); + varDeclIdentifier = getSynthesizedDeepClone(declareSynthIdentifier(possibleNameForVarDecl)); const typeArray: Type[] = possibleNameForVarDecl.types; const unionType = transformer.checker.getUnionType(typeArray, UnionReduction.Subtype); const unionTypeNode = transformer.isInJSFile ? undefined : transformer.checker.typeToTypeNode(unionType, /*enclosingDeclaration*/ undefined, /*flags*/ undefined); const varDecl = [factory.createVariableDeclaration(varDeclIdentifier, /*exclamationToken*/ undefined, unionTypeNode)]; - varDeclList = factory.createVariableStatement(/*modifiers*/ undefined, factory.createVariableDeclarationList(varDecl, NodeFlags.Let)); + const varDeclList = factory.createVariableStatement(/*modifiers*/ undefined, factory.createVariableDeclarationList(varDecl, NodeFlags.Let)); + statements.push(varDeclList); } - const destructuredResult = prevArgName && varDeclIdentifier && isSynthBindingPattern(prevArgName) - && factory.createVariableStatement(/*modifiers*/ undefined, factory.createVariableDeclarationList([factory.createVariableDeclaration(getSynthesizedDeepClone(prevArgName.bindingPattern), /*exclamationToken*/ undefined, /*type*/ undefined, varDeclIdentifier)], NodeFlags.Const)); - return compact([varDeclList, tryStatement, destructuredResult]); + statements.push(tryStatement); + + if (continuationArgName && varDeclIdentifier && isSynthBindingPattern(continuationArgName)) { + statements.push(factory.createVariableStatement( + /*modifiers*/ undefined, + factory.createVariableDeclarationList([ + factory.createVariableDeclaration( + getSynthesizedDeepClone(declareSynthBindingPattern(continuationArgName)), + /*exclamationToken*/ undefined, + /*type*/ undefined, + varDeclIdentifier + )], + NodeFlags.Const))); + } + + return statements; } - function transformCatchWorker(node: PromiseReturningCallExpression<"catch" | "then">, callback: Expression | undefined, transformer: Transformer, prevArgName?: SynthBindingName): readonly Statement[] { - if (!callback || isNullOrUndefinedOrMissing(transformer, callback)) { - return transformExpression(/* returnContextNode */ node, node.expression.expression, transformer, prevArgName); + /** + * @param hasContinuation Whether another `then`, `catch`, or `finally` continuation follows this continuation. + * @param continuationArgName The argument name for the continuation that follows this call. + */ + function transformFinally(node: PromiseReturningCallExpression<"finally">, onFinally: Expression | undefined, transformer: Transformer, hasContinuation: boolean, continuationArgName?: SynthBindingName): readonly Statement[] { + if (!onFinally || isNullOrUndefined(transformer, onFinally)) { + // Ignore this call as it has no effect on the result + return transformExpression(/* returnContextNode */ node, node.expression.expression, transformer, hasContinuation, continuationArgName); } - const argName = callback ? getArgBindingName(callback, transformer) : undefined; - let possibleNameForVarDecl: SynthIdentifier | undefined; + const possibleNameForVarDecl = getPossibleNameForVarDecl(node, transformer, continuationArgName); - // If there is another call in the chain after the .catch() we are transforming, we will need to save the result of both paths (try block and catch block) - // To do this, we will need to synthesize a variable that we were not aware of while we were adding identifiers to the synthNamesMap - // We will use the prevArgName and then update the synthNamesMap with a new variable name for the next transformation step - if (prevArgName && !shouldReturn(node, transformer)) { - if (isSynthIdentifier(prevArgName)) { - possibleNameForVarDecl = prevArgName; - transformer.synthNamesMap.forEach((val, key) => { - if (val.identifier.text === prevArgName.identifier.text) { - const newSynthName = createUniqueSynthName(prevArgName); - transformer.synthNamesMap.set(key, newSynthName); - } - }); - } - else { - possibleNameForVarDecl = createSynthIdentifier(factory.createUniqueName("result", GeneratedIdentifierFlags.Optimistic), prevArgName.types); - } + // Transform the left-hand-side of `.finally` into an array of inlined statements. We pass `true` for hasContinuation as `node` is the outer continuation. + const inlinedLeftHandSide = transformExpression(/*returnContextNode*/ node, node.expression.expression, transformer, /*hasContinuation*/ true, possibleNameForVarDecl); + if (hasFailed()) return silentFail(); // shortcut out of more work - // We are about to write a 'let' variable declaration, but `transformExpression` for both - // the try block and catch block will assign to this name. Setting this flag indicates - // that future assignments should be written as `name = value` instead of `const name = value`. - possibleNameForVarDecl.hasBeenDeclared = true; - } + // Transform the callback argument into an array of inlined statements. We pass whether we have an outer continuation here + // as that indicates whether `return` is valid. + const inlinedCallback = transformCallbackArgument(onFinally, hasContinuation, /*continuationArgName*/ undefined, /*argName*/ undefined, node, transformer); + if (hasFailed()) return silentFail(); // shortcut out of more work - const tryBlock = factory.createBlock(transformExpression(/*returnContextNode*/ node, node.expression.expression, transformer, possibleNameForVarDecl)); - const transformationBody = callback ? getTransformationBody(callback, possibleNameForVarDecl, argName, node, transformer) : emptyArray; - const catchArg = argName ? isSynthIdentifier(argName) ? argName.identifier.text : argName.bindingPattern : undefined; - const catchVariableDeclaration = catchArg !== undefined ? factory.createVariableDeclaration(catchArg) : undefined; - const catchClause = factory.createCatchClause(catchVariableDeclaration, factory.createBlock(transformationBody)); + const tryBlock = factory.createBlock(inlinedLeftHandSide); + const finallyBlock = factory.createBlock(inlinedCallback); + const tryStatement = factory.createTryStatement(tryBlock, /*catchClause*/ undefined, finallyBlock); + return finishCatchOrFinallyTransform(node, transformer, tryStatement, possibleNameForVarDecl, continuationArgName); + } - // In order to avoid an implicit any, we will synthesize a type for the declaration using the unions of the types of both paths (try block and catch block) - let varDeclList: VariableStatement | undefined; - let varDeclIdentifier: Identifier | undefined; - if (possibleNameForVarDecl && !shouldReturn(node, transformer)) { - varDeclIdentifier = getSynthesizedDeepClone(possibleNameForVarDecl.identifier); - const typeArray: Type[] = possibleNameForVarDecl.types; - const unionType = transformer.checker.getUnionType(typeArray, UnionReduction.Subtype); - const unionTypeNode = transformer.isInJSFile ? undefined : transformer.checker.typeToTypeNode(unionType, /*enclosingDeclaration*/ undefined, /*flags*/ undefined); - const varDecl = [factory.createVariableDeclaration(varDeclIdentifier, /*exclamationToken*/ undefined, unionTypeNode)]; - varDeclList = factory.createVariableStatement(/*modifiers*/ undefined, factory.createVariableDeclarationList(varDecl, NodeFlags.Let)); + /** + * @param hasContinuation Whether another `then`, `catch`, or `finally` continuation follows this continuation. + * @param continuationArgName The argument name for the continuation that follows this call. + */ + function transformCatch(node: PromiseReturningCallExpression<"then" | "catch">, onRejected: Expression | undefined, transformer: Transformer, hasContinuation: boolean, continuationArgName?: SynthBindingName): readonly Statement[] { + if (!onRejected || isNullOrUndefined(transformer, onRejected)) { + // Ignore this call as it has no effect on the result + return transformExpression(/* returnContextNode */ node, node.expression.expression, transformer, hasContinuation, continuationArgName); } - const tryStatement = factory.createTryStatement(tryBlock, catchClause, /*finallyBlock*/ undefined); - const destructuredResult = prevArgName && varDeclIdentifier && isSynthBindingPattern(prevArgName) - && factory.createVariableStatement(/*modifiers*/ undefined, factory.createVariableDeclarationList([factory.createVariableDeclaration(getSynthesizedDeepClone(prevArgName.bindingPattern), /*exclamationToken*/ undefined, /*type*/ undefined, varDeclIdentifier)], NodeFlags.Const)); - return compact([varDeclList, tryStatement, destructuredResult]); - } + const inputArgName = getArgBindingName(onRejected, transformer); + const possibleNameForVarDecl = getPossibleNameForVarDecl(node, transformer, continuationArgName); + + // Transform the left-hand-side of `.then`/`.catch` into an array of inlined statements. We pass `true` for hasContinuation as `node` is the outer continuation. + const inlinedLeftHandSide = transformExpression(/*returnContextNode*/ node, node.expression.expression, transformer, /*hasContinuation*/ true, possibleNameForVarDecl); + if (hasFailed()) return silentFail(); // shortcut out of more work - function transformCatch(node: PromiseReturningCallExpression<"catch">, transformer: Transformer, prevArgName?: SynthBindingName): readonly Statement[] { - const func = singleOrUndefined(node.arguments); - return transformCatchWorker(node, func, transformer, prevArgName); + // Transform the callback argument into an array of inlined statements. We pass whether we have an outer continuation here + // as that indicates whether `return` is valid. + const inlinedCallback = transformCallbackArgument(onRejected, hasContinuation, possibleNameForVarDecl, inputArgName, node, transformer); + if (hasFailed()) return silentFail(); // shortcut out of more work + + const tryBlock = factory.createBlock(inlinedLeftHandSide); + const catchClause = factory.createCatchClause(inputArgName && getSynthesizedDeepClone(declareSynthBindingName(inputArgName)), factory.createBlock(inlinedCallback)); + const tryStatement = factory.createTryStatement(tryBlock, catchClause, /*finallyBlock*/ undefined); + return finishCatchOrFinallyTransform(node, transformer, tryStatement, possibleNameForVarDecl, continuationArgName); } - function transformThen(node: PromiseReturningCallExpression<"then">, transformer: Transformer, prevArgName?: SynthBindingName): readonly Statement[] { - const onFulfilled = node.arguments.length > 0 ? node.arguments[0] : undefined; - const onRejected = node.arguments.length > 1 ? node.arguments[1] : undefined; - if (!onFulfilled || isNullOrUndefinedOrMissing(transformer, onFulfilled)) { - return transformCatchWorker(node, onRejected, transformer, prevArgName); + /** + * @param hasContinuation Whether another `then`, `catch`, or `finally` continuation follows this continuation. + * @param continuationArgName The argument name for the continuation that follows this call. + */ + function transformThen(node: PromiseReturningCallExpression<"then">, onFulfilled: Expression | undefined, onRejected: Expression | undefined, transformer: Transformer, hasContinuation: boolean, continuationArgName?: SynthBindingName): readonly Statement[] { + if (!onFulfilled || isNullOrUndefined(transformer, onFulfilled)) { + // If we don't have an `onfulfilled` callback, try treating this as a `.catch`. + return transformCatch(node, onRejected, transformer, hasContinuation, continuationArgName); } - const onFulfilledArgumentName = getArgBindingName(onFulfilled, transformer); - const transformationBody = getTransformationBody(onFulfilled, prevArgName, onFulfilledArgumentName, node, transformer); - if (onRejected && !isNullOrUndefinedOrMissing(transformer, onRejected)) { - const onRejectedArgumentName = getArgBindingName(onRejected, transformer); - const tryBlock = factory.createBlock(transformExpression(node.expression, node.expression, transformer, onFulfilledArgumentName).concat(transformationBody)); - const transformationBody2 = getTransformationBody(onRejected, prevArgName, onRejectedArgumentName, node, transformer); - const catchArg = onRejectedArgumentName ? isSynthIdentifier(onRejectedArgumentName) ? onRejectedArgumentName.identifier.text : onRejectedArgumentName.bindingPattern : undefined; - const catchVariableDeclaration = catchArg !== undefined ? factory.createVariableDeclaration(catchArg) : undefined; - const catchClause = factory.createCatchClause(catchVariableDeclaration, factory.createBlock(transformationBody2)); - return [factory.createTryStatement(tryBlock, catchClause, /* finallyBlock */ undefined)]; + // We don't currently support transforming a `.then` with both onfulfilled and onrejected handlers. + if (onRejected && !isNullOrUndefined(transformer, onRejected)) { + return silentFail(); } - return transformExpression(node.expression.expression, node.expression.expression, transformer, onFulfilledArgumentName).concat(transformationBody); + const inputArgName = getArgBindingName(onFulfilled, transformer); + + // Transform the left-hand-side of `.then` into an array of inlined statements. We pass `true` for hasContinuation as `node` is the outer continuation. + const inlinedLeftHandSide = transformExpression(node.expression.expression, node.expression.expression, transformer, /*hasContinuation*/ true, inputArgName); + if (hasFailed()) return silentFail(); // shortcut out of more work + + // Transform the callback argument into an array of inlined statements. We pass whether we have an outer continuation here + // as that indicates whether `return` is valid. + const inlinedCallback = transformCallbackArgument(onFulfilled, hasContinuation, continuationArgName, inputArgName, node, transformer); + if (hasFailed()) return silentFail(); // shortcut out of more work + + return concatenate(inlinedLeftHandSide, inlinedCallback); } /** * Transforms the 'x' part of `x.then(...)`, or the 'y()' part of `y().catch(...)`, where 'x' and 'y()' are Promises. */ - function transformPromiseExpressionOfPropertyAccess(returnContextNode: Expression, node: Expression, transformer: Transformer, prevArgName?: SynthBindingName): readonly Statement[] { + function transformPromiseExpressionOfPropertyAccess(returnContextNode: Expression, node: Expression, transformer: Transformer, continuationArgName?: SynthBindingName): readonly Statement[] { if (shouldReturn(returnContextNode, transformer)) { return [factory.createReturnStatement(factory.createAwaitExpression(getSynthesizedDeepClone(node)))]; } - return createVariableOrAssignmentOrExpressionStatement(prevArgName, factory.createAwaitExpression(node), /*typeAnnotation*/ undefined); + return createVariableOrAssignmentOrExpressionStatement(continuationArgName, factory.createAwaitExpression(node), /*typeAnnotation*/ undefined); } function createVariableOrAssignmentOrExpressionStatement(variableName: SynthBindingName | undefined, rightHandSide: Expression, typeAnnotation: TypeNode | undefined): readonly Statement[] { @@ -440,7 +461,7 @@ namespace ts.codefix { if (isSynthIdentifier(variableName) && variableName.hasBeenDeclared) { // if the variable has already been declared, we don't need "let" or "const" - return [factory.createExpressionStatement(factory.createAssignment(getSynthesizedDeepClone(variableName.identifier), rightHandSide))]; + return [factory.createExpressionStatement(factory.createAssignment(getSynthesizedDeepClone(referenceSynthIdentifier(variableName)), rightHandSide))]; } return [ @@ -448,14 +469,14 @@ namespace ts.codefix { /*modifiers*/ undefined, factory.createVariableDeclarationList([ factory.createVariableDeclaration( - getSynthesizedDeepClone(getNode(variableName)), + getSynthesizedDeepClone(declareSynthBindingName(variableName)), /*exclamationToken*/ undefined, typeAnnotation, rightHandSide)], NodeFlags.Const))]; } - function maybeAnnotateAndReturn(expressionToReturn: Expression | undefined, typeAnnotation: TypeNode | undefined): readonly Statement[] { + function maybeAnnotateAndReturn(expressionToReturn: Expression | undefined, typeAnnotation: TypeNode | undefined): Statement[] { if (typeAnnotation && expressionToReturn) { const name = factory.createUniqueName("result", GeneratedIdentifierFlags.Optimistic); return [ @@ -467,21 +488,28 @@ namespace ts.codefix { } // should be kept up to date with isFixablePromiseArgument in suggestionDiagnostics.ts - function getTransformationBody(func: Expression, prevArgName: SynthBindingName | undefined, argName: SynthBindingName | undefined, parent: CallExpression, transformer: Transformer): readonly Statement[] { + /** + * @param hasContinuation Whether another `then`, `catch`, or `finally` continuation follows the continuation to which this callback belongs. + * @param continuationArgName The argument name for the continuation that follows this call. + * @param inputArgName The argument name provided to this call + */ + function transformCallbackArgument(func: Expression, hasContinuation: boolean, continuationArgName: SynthBindingName | undefined, inputArgName: SynthBindingName | undefined, parent: CallExpression, transformer: Transformer): readonly Statement[] { switch (func.kind) { case SyntaxKind.NullKeyword: // do not produce a transformed statement for a null argument break; case SyntaxKind.PropertyAccessExpression: case SyntaxKind.Identifier: // identifier includes undefined - if (!argName) { + if (!inputArgName) { // undefined was argument passed to promise handler break; } - const synthCall = factory.createCallExpression(getSynthesizedDeepClone(func as Identifier | PropertyAccessExpression), /*typeArguments*/ undefined, isSynthIdentifier(argName) ? [argName.identifier] : []); + const synthCall = factory.createCallExpression(getSynthesizedDeepClone(func as Identifier | PropertyAccessExpression), /*typeArguments*/ undefined, isSynthIdentifier(inputArgName) ? [referenceSynthIdentifier(inputArgName)] : []); + if (shouldReturn(parent, transformer)) { - return maybeAnnotateAndReturn(synthCall, parent.typeArguments?.[0]); + // TODO(rbuckton): `parent.typeArguments[0]` is wrong here if we support Promise-like types that might have different type argument lists than `Promise`. + return maybeAnnotateAndReturn(synthCall, elementAt(parent.typeArguments, 0)); } const type = transformer.checker.getTypeAtLocation(func); @@ -491,9 +519,10 @@ namespace ts.codefix { return silentFail(); } const returnType = callSignatures[0].getReturnType(); - const varDeclOrAssignment = createVariableOrAssignmentOrExpressionStatement(prevArgName, factory.createAwaitExpression(synthCall), parent.typeArguments?.[0]); - if (prevArgName) { - prevArgName.types.push(transformer.checker.getAwaitedType(returnType) || returnType); + // TODO(rbuckton): `parent.typeArguments[0]` is wrong here if we support Promise-like types that might have different type argument lists than `Promise`. + const varDeclOrAssignment = createVariableOrAssignmentOrExpressionStatement(continuationArgName, factory.createAwaitExpression(synthCall), elementAt(parent.typeArguments, 0)); + if (continuationArgName) { + continuationArgName.types.push(transformer.checker.getAwaitedType(returnType) || returnType); } return varDeclOrAssignment; @@ -510,13 +539,54 @@ namespace ts.codefix { if (isReturnStatement(statement)) { seenReturnStatement = true; if (isReturnStatementWithFixablePromiseHandler(statement, transformer.checker)) { - refactoredStmts = refactoredStmts.concat(getInnerTransformationBody(transformer, [statement], prevArgName)); + refactoredStmts = refactoredStmts.concat(transformReturnStatementWithFixablePromiseHandler(transformer, statement, hasContinuation, continuationArgName)); } else { const possiblyAwaitedRightHandSide = returnType && statement.expression ? getPossiblyAwaitedRightHandSide(transformer.checker, returnType, statement.expression) : statement.expression; - refactoredStmts.push(...maybeAnnotateAndReturn(possiblyAwaitedRightHandSide, parent.typeArguments?.[0])); + // TODO(rbuckton): `parent.typeArguments[0]` is wrong here if we support Promise-like types that might have different type argument lists than `Promise`. + refactoredStmts.push(...maybeAnnotateAndReturn(possiblyAwaitedRightHandSide, elementAt(parent.typeArguments, 0))); } } + else if (hasContinuation && forEachReturnStatement(statement, returnTrue)) { + // If there is a nested `return` in a callback that has a trailing continuation, we don't transform it as the resulting complexity is too great. For example: + // + // source | result + // -------------------------------------| --------------------------------------- + // function f(): Promise { | async function f9(): Promise { + // return foo().then(() => { | await foo(); + // if (Math.random()) { | if (Math.random()) { + // return 1; | return 1; // incorrect early return + // } | } + // return 2; | return 2; // incorrect early return + // }).then(a => { | const a = undefined; + // return a + 1; | return a + 1; + // }); | } + // } | + // + // However, branching returns in the outermost continuation are acceptable as no other continuation follows it: + // + // source | result + //--------------------------------------|--------------------------------------- + // function f() { | async function f() { + // return foo().then(res => { | const res = await foo(); + // if (res.ok) { | if (res.ok) { + // return 1; | return 1; + // } | } + // else { | else { + // if (res.buffer.length > 5) { | if (res.buffer.length > 5) { + // return 2; | return 2; + // } | } + // else { | else { + // return 3; | return 3; + // } | } + // } | } + // }); | } + // } | + // + // We may improve this in the future, but for now the heuristics are too complex + + return silentFail(); + } else { refactoredStmts.push(statement); } @@ -526,29 +596,32 @@ namespace ts.codefix { ? refactoredStmts.map(s => getSynthesizedDeepClone(s)) : removeReturns( refactoredStmts, - prevArgName, + continuationArgName, transformer, seenReturnStatement); } else { - const innerRetStmts = isFixablePromiseHandler(funcBody, transformer.checker) ? [factory.createReturnStatement(funcBody)] : emptyArray; - const innerCbBody = getInnerTransformationBody(transformer, innerRetStmts, prevArgName); + const inlinedStatements = isFixablePromiseHandler(funcBody, transformer.checker) ? + transformReturnStatementWithFixablePromiseHandler(transformer, factory.createReturnStatement(funcBody), hasContinuation, continuationArgName) : + emptyArray; - if (innerCbBody.length > 0) { - return innerCbBody; + if (inlinedStatements.length > 0) { + return inlinedStatements; } if (returnType) { const possiblyAwaitedRightHandSide = getPossiblyAwaitedRightHandSide(transformer.checker, returnType, funcBody); + if (!shouldReturn(parent, transformer)) { - const transformedStatement = createVariableOrAssignmentOrExpressionStatement(prevArgName, possiblyAwaitedRightHandSide, /*typeAnnotation*/ undefined); - if (prevArgName) { - prevArgName.types.push(transformer.checker.getAwaitedType(returnType) || returnType); + const transformedStatement = createVariableOrAssignmentOrExpressionStatement(continuationArgName, possiblyAwaitedRightHandSide, /*typeAnnotation*/ undefined); + if (continuationArgName) { + continuationArgName.types.push(transformer.checker.getAwaitedType(returnType) || returnType); } return transformedStatement; } else { - return maybeAnnotateAndReturn(possiblyAwaitedRightHandSide, parent.typeArguments?.[0]); + // TODO(rbuckton): `parent.typeArguments[0]` is wrong here if we support Promise-like types that might have different type argument lists than `Promise`. + return maybeAnnotateAndReturn(possiblyAwaitedRightHandSide, elementAt(parent.typeArguments, 0)); } } else { @@ -582,9 +655,12 @@ namespace ts.codefix { if (prevArgName === undefined) { ret.push(factory.createExpressionStatement(possiblyAwaitedExpression)); } + else if (isSynthIdentifier(prevArgName) && prevArgName.hasBeenDeclared) { + ret.push(factory.createExpressionStatement(factory.createAssignment(referenceSynthIdentifier(prevArgName), possiblyAwaitedExpression))); + } else { ret.push(factory.createVariableStatement(/*modifiers*/ undefined, - (factory.createVariableDeclarationList([factory.createVariableDeclaration(getNode(prevArgName), /*exclamationToken*/ undefined, /*type*/ undefined, possiblyAwaitedExpression)], NodeFlags.Const)))); + (factory.createVariableDeclarationList([factory.createVariableDeclaration(declareSynthBindingName(prevArgName), /*exclamationToken*/ undefined, /*type*/ undefined, possiblyAwaitedExpression)], NodeFlags.Const)))); } } } @@ -596,29 +672,30 @@ namespace ts.codefix { // if block has no return statement, need to define prevArgName as undefined to prevent undeclared variables if (!seenReturnStatement && prevArgName !== undefined) { ret.push(factory.createVariableStatement(/*modifiers*/ undefined, - (factory.createVariableDeclarationList([factory.createVariableDeclaration(getNode(prevArgName), /*exclamationToken*/ undefined, /*type*/ undefined, factory.createIdentifier("undefined"))], NodeFlags.Const)))); + (factory.createVariableDeclarationList([factory.createVariableDeclaration(declareSynthBindingName(prevArgName), /*exclamationToken*/ undefined, /*type*/ undefined, factory.createIdentifier("undefined"))], NodeFlags.Const)))); } return ret; } - - function getInnerTransformationBody(transformer: Transformer, innerRetStmts: readonly Node[], prevArgName?: SynthBindingName) { + /** + * @param hasContinuation Whether another `then`, `catch`, or `finally` continuation follows the continuation to which this statement belongs. + * @param continuationArgName The argument name for the continuation that follows this call. + */ + function transformReturnStatementWithFixablePromiseHandler(transformer: Transformer, innerRetStmt: ReturnStatement, hasContinuation: boolean, continuationArgName?: SynthBindingName) { let innerCbBody: Statement[] = []; - for (const stmt of innerRetStmts) { - forEachChild(stmt, function visit(node) { - if (isCallExpression(node)) { - const temp = transformExpression(node, node, transformer, prevArgName); - innerCbBody = innerCbBody.concat(temp); - if (innerCbBody.length > 0) { - return; - } + forEachChild(innerRetStmt, function visit(node) { + if (isCallExpression(node)) { + const temp = transformExpression(node, node, transformer, hasContinuation, continuationArgName); + innerCbBody = innerCbBody.concat(temp); + if (innerCbBody.length > 0) { + return; } - else if (!isFunctionLike(node)) { - forEachChild(node, visit); - } - }); - } + } + else if (!isFunctionLike(node)) { + forEachChild(node, visit); + } + }); return innerCbBody; } @@ -688,18 +765,35 @@ namespace ts.codefix { return every(bindingName.elements, isEmptyBindingName); } - function getNode(bindingName: SynthBindingName) { - return isSynthIdentifier(bindingName) ? bindingName.identifier : bindingName.bindingPattern; - } - function createSynthIdentifier(identifier: Identifier, types: Type[] = []): SynthIdentifier { - return { kind: SynthBindingNameKind.Identifier, identifier, types, hasBeenDeclared: false }; + return { kind: SynthBindingNameKind.Identifier, identifier, types, hasBeenDeclared: false, hasBeenReferenced: false }; } function createSynthBindingPattern(bindingPattern: BindingPattern, elements: readonly SynthBindingName[] = emptyArray, types: Type[] = []): SynthBindingPattern { return { kind: SynthBindingNameKind.BindingPattern, bindingPattern, elements, types }; } + function referenceSynthIdentifier(synthId: SynthIdentifier) { + synthId.hasBeenReferenced = true; + return synthId.identifier; + } + + function declareSynthBindingName(synthName: SynthBindingName) { + return isSynthIdentifier(synthName) ? declareSynthIdentifier(synthName) : declareSynthBindingPattern(synthName); + } + + function declareSynthBindingPattern(synthPattern: SynthBindingPattern) { + for (const element of synthPattern.elements) { + declareSynthBindingName(element); + } + return synthPattern.bindingPattern; + } + + function declareSynthIdentifier(synthId: SynthIdentifier) { + synthId.hasBeenDeclared = true; + return synthId.identifier; + } + function isSynthIdentifier(bindingName: SynthBindingName): bindingName is SynthIdentifier { return bindingName.kind === SynthBindingNameKind.Identifier; } diff --git a/src/services/suggestionDiagnostics.ts b/src/services/suggestionDiagnostics.ts index d42b4a01c6c5f..4e8392e837d09 100644 --- a/src/services/suggestionDiagnostics.ts +++ b/src/services/suggestionDiagnostics.ts @@ -139,34 +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, "then") || hasPropertyAccessExpressionWithName(node, "catch") || hasPropertyAccessExpressionWithName(node, "finally")); } - 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"; + 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"; }); } diff --git a/src/testRunner/unittests/services/convertToAsyncFunction.ts b/src/testRunner/unittests/services/convertToAsyncFunction.ts index 78197e59e2413..a8bd6e6d520c0 100644 --- a/src/testRunner/unittests/services/convertToAsyncFunction.ts +++ b/src/testRunner/unittests/services/convertToAsyncFunction.ts @@ -284,7 +284,30 @@ interface Array {}` } } - 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) { @@ -327,35 +350,47 @@ interface Array {}` 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) { @@ -379,19 +414,23 @@ interface Array {}` } 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", () => { @@ -442,11 +481,11 @@ function [#|f|](): Promise{ function [#|f|]():Promise { return fetch('https://typescriptlang.org').then(result => { console.log(result); }).catch(err => { console.log(err); }); }`); - _testConvertToAsyncFunction("convertToAsyncFunction_CatchAndRej", ` + _testConvertToAsyncFunctionFailed("convertToAsyncFunction_CatchAndRej", ` function [#|f|]():Promise { 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 { return fetch('https://typescriptlang.org').then(res, rej).catch(catch_err) } @@ -1770,6 +1809,31 @@ declare function foo(): Promise; function [#|f|](): Promise { return foo().then(x => Promise.resolve(x + 1)).finally(() => console.log("done")).then(y => y + 2); }`); - + _testConvertToAsyncFunctionFailedAction("convertToAsyncFunction_returnInBranch", ` +declare function foo(): Promise; +function [#|f|](): Promise { + return foo().then(() => { + if (Math.random()) { + return 1; + } + return 2; + }).then(a => { + return a + 1; + }); +} +`); + _testConvertToAsyncFunctionFailedAction("convertToAsyncFunction_partialReturnInBranch", ` +declare function foo(): Promise; +function [#|f|](): Promise { + return foo().then(() => { + if (Math.random()) { + return 1; + } + console.log("foo"); + }).then(a => { + return a + 1; + }); +} +`); }); } diff --git a/tests/baselines/reference/api/tsserverlibrary.d.ts b/tests/baselines/reference/api/tsserverlibrary.d.ts index 14de9571dc63a..846fa5805d9b6 100644 --- a/tests/baselines/reference/api/tsserverlibrary.d.ts +++ b/tests/baselines/reference/api/tsserverlibrary.d.ts @@ -3620,7 +3620,7 @@ declare 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; createPropertyAssignment(name: string | PropertyName, initializer: Expression): PropertyAssignment; updatePropertyAssignment(node: PropertyAssignment, name: PropertyName, initializer: Expression): PropertyAssignment; @@ -11076,7 +11076,7 @@ declare namespace ts { /** @deprecated Use `factory.updateHeritageClause` or the factory supplied by your transformation context instead. */ const updateHeritageClause: (node: HeritageClause, types: readonly ExpressionWithTypeArguments[]) => HeritageClause; /** @deprecated Use `factory.createCatchClause` or the factory supplied by your transformation context instead. */ - const createCatchClause: (variableDeclaration: string | VariableDeclaration | undefined, block: Block) => CatchClause; + const createCatchClause: (variableDeclaration: string | VariableDeclaration | BindingName | undefined, block: Block) => CatchClause; /** @deprecated Use `factory.updateCatchClause` or the factory supplied by your transformation context instead. */ const updateCatchClause: (node: CatchClause, variableDeclaration: VariableDeclaration | undefined, block: Block) => CatchClause; /** @deprecated Use `factory.createPropertyAssignment` or the factory supplied by your transformation context instead. */ diff --git a/tests/baselines/reference/api/typescript.d.ts b/tests/baselines/reference/api/typescript.d.ts index a83b83826341f..285b9b3447e4a 100644 --- a/tests/baselines/reference/api/typescript.d.ts +++ b/tests/baselines/reference/api/typescript.d.ts @@ -3620,7 +3620,7 @@ declare 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; createPropertyAssignment(name: string | PropertyName, initializer: Expression): PropertyAssignment; updatePropertyAssignment(node: PropertyAssignment, name: PropertyName, initializer: Expression): PropertyAssignment; @@ -7272,7 +7272,7 @@ declare namespace ts { /** @deprecated Use `factory.updateHeritageClause` or the factory supplied by your transformation context instead. */ const updateHeritageClause: (node: HeritageClause, types: readonly ExpressionWithTypeArguments[]) => HeritageClause; /** @deprecated Use `factory.createCatchClause` or the factory supplied by your transformation context instead. */ - const createCatchClause: (variableDeclaration: string | VariableDeclaration | undefined, block: Block) => CatchClause; + const createCatchClause: (variableDeclaration: string | VariableDeclaration | BindingName | undefined, block: Block) => CatchClause; /** @deprecated Use `factory.updateCatchClause` or the factory supplied by your transformation context instead. */ const updateCatchClause: (node: CatchClause, variableDeclaration: VariableDeclaration | undefined, block: Block) => CatchClause; /** @deprecated Use `factory.createPropertyAssignment` or the factory supplied by your transformation context instead. */ diff --git a/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_CatchAndRej.ts b/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_CatchAndRej.ts deleted file mode 100644 index 6b5260baeda57..0000000000000 --- a/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_CatchAndRej.ts +++ /dev/null @@ -1,19 +0,0 @@ -// ==ORIGINAL== - -function /*[#|*/f/*|]*/():Promise { - return fetch('https://typescriptlang.org').then(result => { console.log(result); }, rejection => { console.log("rejected:", rejection); }).catch(err => { console.log(err) }); -} -// ==ASYNC FUNCTION::Convert to async function== - -async function f():Promise { - try { - try { - const result = await fetch('https://typescriptlang.org'); - console.log(result); - } catch (rejection) { - console.log("rejected:", rejection); - } - } catch (err) { - console.log(err); - } -} \ No newline at end of file diff --git a/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_CatchAndRejRef.ts b/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_CatchAndRejRef.ts deleted file mode 100644 index 190ae9d18a253..0000000000000 --- a/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_CatchAndRejRef.ts +++ /dev/null @@ -1,37 +0,0 @@ -// ==ORIGINAL== - -function /*[#|*/f/*|]*/():Promise { - return fetch('https://typescriptlang.org').then(res, rej).catch(catch_err) -} -function res(result){ - console.log(result); -} -function rej(rejection){ - return rejection.ok; -} -function catch_err(err){ - console.log(err); -} -// ==ASYNC FUNCTION::Convert to async function== - -async function f():Promise { - try { - try { - const result = await fetch('https://typescriptlang.org'); - return res(result); - } catch (rejection) { - return rej(rejection); - } - } catch (err) { - return catch_err(err); - } -} -function res(result){ - console.log(result); -} -function rej(rejection){ - return rejection.ok; -} -function catch_err(err){ - console.log(err); -} \ No newline at end of file From 4ca84d572649ea892918a5aef4a8df721fc4ce40 Mon Sep 17 00:00:00 2001 From: Ron Buckton Date: Fri, 27 Aug 2021 12:38:57 -0700 Subject: [PATCH 3/4] Baseline update --- .../baselines/reference/completionEntryForUnionMethod.baseline | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/baselines/reference/completionEntryForUnionMethod.baseline b/tests/baselines/reference/completionEntryForUnionMethod.baseline index beda7838c3895..8bfd544e98a6d 100644 --- a/tests/baselines/reference/completionEntryForUnionMethod.baseline +++ b/tests/baselines/reference/completionEntryForUnionMethod.baseline @@ -1722,7 +1722,7 @@ "kind": "space" }, { - "text": "Function used to determine the order of the elements. It is expected to return\r\na negative value if first argument is less than second argument, zero if they're equal and a positive\r\nvalue otherwise. If omitted, the elements are sorted in ascending, ASCII character order.\r\n```ts\r\n[11,2,22,1].sort((a, b) => a - b)\r\n```", + "text": "Function used to determine the order of the elements. It is expected to return\r\na negative value if the first argument is less than the second argument, zero if they're equal, and a positive\r\nvalue otherwise. If omitted, the elements are sorted in ascending, ASCII character order.\r\n```ts\r\n[11,2,22,1].sort((a, b) => a - b)\r\n```", "kind": "text" } ] From da1d25c4f7460212ba6e40b47ccebb07703f2d63 Mon Sep 17 00:00:00 2001 From: Ron Buckton Date: Fri, 27 Aug 2021 16:11:58 -0700 Subject: [PATCH 4/4] Verify type argument for call can be used, add a few more early exit shortcuts --- src/compiler/checker.ts | 2 + src/compiler/types.ts | 2 + .../codefixes/convertToAsyncFunction.ts | 80 ++++++++++++++----- ...convertToAsyncFunction_catchNoArguments.ts | 2 +- .../convertToAsyncFunction_emptyCatch1.js | 2 +- .../convertToAsyncFunction_emptyCatch1.ts | 2 +- ...nvertToAsyncFunction_finallyNoArguments.ts | 2 +- .../convertToAsyncFunction_finallyNull.ts | 2 +- ...convertToAsyncFunction_finallyUndefined.ts | 2 +- .../convertToAsyncFunction_thenNoArguments.ts | 2 +- 10 files changed, 71 insertions(+), 27 deletions(-) diff --git a/src/compiler/checker.ts b/src/compiler/checker.ts index 8ab4f678f4e87..f4adc54e438a8 100644 --- a/src/compiler/checker.ts +++ b/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, diff --git a/src/compiler/types.ts b/src/compiler/types.ts index e1311af2aa7a0..d120c3e61c67f 100644 --- a/src/compiler/types.ts +++ b/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; diff --git a/src/services/codefixes/convertToAsyncFunction.ts b/src/services/codefixes/convertToAsyncFunction.ts index 008c65883a784..3c6045fdb6c78 100644 --- a/src/services/codefixes/convertToAsyncFunction.ts +++ b/src/services/codefixes/convertToAsyncFunction.ts @@ -92,12 +92,21 @@ namespace ts.codefix { forEachChild(returnStatement, function visit(node) { if (isCallExpression(node)) { const newNodes = transformExpression(node, node, transformer, /*hasContinuation*/ false); + if (hasFailed()) { + return true; // return something truthy to shortcut out of more work + } changes.replaceNodeWithNodes(sourceFile, returnStatement, newNodes); } else if (!isFunctionLike(node)) { forEachChild(node, visit); + if (hasFailed()) { + return true; // return something truthy to shortcut out of more work + } } }); + if (hasFailed()) { + return; // shortcut out of more work + } } } @@ -123,14 +132,10 @@ namespace ts.codefix { setOfExpressionsToReturn.add(getNodeId(node)); forEach(node.arguments, visit); } - else if (isPromiseReturningCallExpression(node, checker, "catch")) { - setOfExpressionsToReturn.add(getNodeId(node)); - // if .catch() is the last call in the chain, move leftward in the chain until we hit something else that should be returned - forEachChild(node, visit); - } - else if (isPromiseReturningCallExpression(node, checker, "finally")) { + else if (isPromiseReturningCallExpression(node, checker, "catch") || + isPromiseReturningCallExpression(node, checker, "finally")) { setOfExpressionsToReturn.add(getNodeId(node)); - // if .finally() is the last call in the chain, move leftward in the chain until we hit something else that should be returned + // if .catch() or .finally() is the last call in the chain, move leftward in the chain until we hit something else that should be returned forEachChild(node, visit); } else if (isPromiseTypedExpression(node, checker)) { @@ -152,6 +157,41 @@ namespace ts.codefix { return !!(nodeType && checker.getPromisedTypeOfPromise(nodeType)); } + // NOTE: this is a mostly copy of `isReferenceToType` from checker.ts. While this violates DRY, it keeps + // `isReferenceToType` in checker local to the checker to avoid the cost of a property lookup on `ts`. + function isReferenceToType(type: Type, target: Type) { + return (getObjectFlags(type) & ObjectFlags.Reference) !== 0 + && (type as TypeReference).target === target; + } + + function getExplicitPromisedTypeOfPromiseReturningCallExpression(node: PromiseReturningCallExpression<"then" | "catch" | "finally">, callback: Expression, checker: TypeChecker) { + if (node.expression.name.escapedText === "finally") { + // for a `finally`, there's no type argument + return undefined; + } + + // If the call to `then` or `catch` comes from the global `Promise` or `PromiseLike` type, we can safely use the + // type argument supplied for the callback. For other promise types we would need a more complex heuristic to determine + // which type argument is safe to use as an annotation. + const promiseType = checker.getTypeAtLocation(node.expression.expression); + if (isReferenceToType(promiseType, checker.getPromiseType()) || + isReferenceToType(promiseType, checker.getPromiseLikeType())) { + if (node.expression.name.escapedText === "then") { + if (callback === elementAt(node.arguments, 0)) { + // for the `onfulfilled` callback, use the first type argument + return elementAt(node.typeArguments, 0); + } + else if (callback === elementAt(node.arguments, 1)) { + // for the `onrejected` callback, use the second type argument + return elementAt(node.typeArguments, 1); + } + } + else { + return elementAt(node.typeArguments, 0); + } + } + } + function isPromiseTypedExpression(node: Node, checker: TypeChecker): node is Expression { if (!isExpression(node)) return false; return !!checker.getPromisedTypeOfPromise(checker.getTypeAtLocation(node)); @@ -273,7 +313,7 @@ namespace ts.codefix { const nodeType = transformer.checker.getTypeAtLocation(node); if (nodeType && transformer.checker.getPromisedTypeOfPromise(nodeType)) { Debug.assertNode(getOriginalNode(node).parent, isPropertyAccessExpression); - return transformPromiseExpressionOfPropertyAccess(returnContextNode, node, transformer, continuationArgName); + return transformPromiseExpressionOfPropertyAccess(returnContextNode, node, transformer, hasContinuation, continuationArgName); } return silentFail(); @@ -423,7 +463,7 @@ namespace ts.codefix { return transformCatch(node, onRejected, transformer, hasContinuation, continuationArgName); } - // We don't currently support transforming a `.then` with both onfulfilled and onrejected handlers. + // We don't currently support transforming a `.then` with both onfulfilled and onrejected handlers, per GH#38152. if (onRejected && !isNullOrUndefined(transformer, onRejected)) { return silentFail(); } @@ -445,9 +485,13 @@ namespace ts.codefix { /** * Transforms the 'x' part of `x.then(...)`, or the 'y()' part of `y().catch(...)`, where 'x' and 'y()' are Promises. */ - function transformPromiseExpressionOfPropertyAccess(returnContextNode: Expression, node: Expression, transformer: Transformer, continuationArgName?: SynthBindingName): readonly Statement[] { + function transformPromiseExpressionOfPropertyAccess(returnContextNode: Expression, node: Expression, transformer: Transformer, hasContinuation: boolean, continuationArgName?: SynthBindingName): readonly Statement[] { if (shouldReturn(returnContextNode, transformer)) { - return [factory.createReturnStatement(factory.createAwaitExpression(getSynthesizedDeepClone(node)))]; + let returnValue = getSynthesizedDeepClone(node); + if (hasContinuation) { + returnValue = factory.createAwaitExpression(returnValue); + } + return [factory.createReturnStatement(returnValue)]; } return createVariableOrAssignmentOrExpressionStatement(continuationArgName, factory.createAwaitExpression(node), /*typeAnnotation*/ undefined); @@ -493,7 +537,7 @@ namespace ts.codefix { * @param continuationArgName The argument name for the continuation that follows this call. * @param inputArgName The argument name provided to this call */ - function transformCallbackArgument(func: Expression, hasContinuation: boolean, continuationArgName: SynthBindingName | undefined, inputArgName: SynthBindingName | undefined, parent: CallExpression, transformer: Transformer): readonly Statement[] { + function transformCallbackArgument(func: Expression, hasContinuation: boolean, continuationArgName: SynthBindingName | undefined, inputArgName: SynthBindingName | undefined, parent: PromiseReturningCallExpression<"then" | "catch" | "finally">, transformer: Transformer): readonly Statement[] { switch (func.kind) { case SyntaxKind.NullKeyword: // do not produce a transformed statement for a null argument @@ -508,8 +552,7 @@ namespace ts.codefix { const synthCall = factory.createCallExpression(getSynthesizedDeepClone(func as Identifier | PropertyAccessExpression), /*typeArguments*/ undefined, isSynthIdentifier(inputArgName) ? [referenceSynthIdentifier(inputArgName)] : []); if (shouldReturn(parent, transformer)) { - // TODO(rbuckton): `parent.typeArguments[0]` is wrong here if we support Promise-like types that might have different type argument lists than `Promise`. - return maybeAnnotateAndReturn(synthCall, elementAt(parent.typeArguments, 0)); + return maybeAnnotateAndReturn(synthCall, getExplicitPromisedTypeOfPromiseReturningCallExpression(parent, func, transformer.checker)); } const type = transformer.checker.getTypeAtLocation(func); @@ -519,8 +562,7 @@ namespace ts.codefix { return silentFail(); } const returnType = callSignatures[0].getReturnType(); - // TODO(rbuckton): `parent.typeArguments[0]` is wrong here if we support Promise-like types that might have different type argument lists than `Promise`. - const varDeclOrAssignment = createVariableOrAssignmentOrExpressionStatement(continuationArgName, factory.createAwaitExpression(synthCall), elementAt(parent.typeArguments, 0)); + const varDeclOrAssignment = createVariableOrAssignmentOrExpressionStatement(continuationArgName, factory.createAwaitExpression(synthCall), getExplicitPromisedTypeOfPromiseReturningCallExpression(parent, func, transformer.checker)); if (continuationArgName) { continuationArgName.types.push(transformer.checker.getAwaitedType(returnType) || returnType); } @@ -543,8 +585,7 @@ namespace ts.codefix { } else { const possiblyAwaitedRightHandSide = returnType && statement.expression ? getPossiblyAwaitedRightHandSide(transformer.checker, returnType, statement.expression) : statement.expression; - // TODO(rbuckton): `parent.typeArguments[0]` is wrong here if we support Promise-like types that might have different type argument lists than `Promise`. - refactoredStmts.push(...maybeAnnotateAndReturn(possiblyAwaitedRightHandSide, elementAt(parent.typeArguments, 0))); + refactoredStmts.push(...maybeAnnotateAndReturn(possiblyAwaitedRightHandSide, getExplicitPromisedTypeOfPromiseReturningCallExpression(parent, func, transformer.checker))); } } else if (hasContinuation && forEachReturnStatement(statement, returnTrue)) { @@ -620,8 +661,7 @@ namespace ts.codefix { return transformedStatement; } else { - // TODO(rbuckton): `parent.typeArguments[0]` is wrong here if we support Promise-like types that might have different type argument lists than `Promise`. - return maybeAnnotateAndReturn(possiblyAwaitedRightHandSide, elementAt(parent.typeArguments, 0)); + return maybeAnnotateAndReturn(possiblyAwaitedRightHandSide, getExplicitPromisedTypeOfPromiseReturningCallExpression(parent, func, transformer.checker)); } } else { diff --git a/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_catchNoArguments.ts b/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_catchNoArguments.ts index a8a361df9809f..bad8bb56800c5 100644 --- a/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_catchNoArguments.ts +++ b/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_catchNoArguments.ts @@ -8,5 +8,5 @@ function /*[#|*/f/*|]*/(): Promise { declare function foo(): Promise; async function f(): Promise { - return await foo(); + return foo(); } \ No newline at end of file diff --git a/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_emptyCatch1.js b/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_emptyCatch1.js index a0fedb4b2b9a5..72aebf371ad0d 100644 --- a/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_emptyCatch1.js +++ b/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_emptyCatch1.js @@ -7,5 +7,5 @@ function /*[#|*/f/*|]*/() { // ==ASYNC FUNCTION::Convert to async function== async function f() { - return await Promise.resolve(); + return Promise.resolve(); } diff --git a/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_emptyCatch1.ts b/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_emptyCatch1.ts index a0fedb4b2b9a5..72aebf371ad0d 100644 --- a/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_emptyCatch1.ts +++ b/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_emptyCatch1.ts @@ -7,5 +7,5 @@ function /*[#|*/f/*|]*/() { // ==ASYNC FUNCTION::Convert to async function== async function f() { - return await Promise.resolve(); + return Promise.resolve(); } diff --git a/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_finallyNoArguments.ts b/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_finallyNoArguments.ts index d2464518f5d6b..d75b571875ac4 100644 --- a/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_finallyNoArguments.ts +++ b/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_finallyNoArguments.ts @@ -8,5 +8,5 @@ function /*[#|*/f/*|]*/(): Promise { declare function foo(): Promise; async function f(): Promise { - return await foo(); + return foo(); } \ No newline at end of file diff --git a/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_finallyNull.ts b/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_finallyNull.ts index f69afd06ca876..07f734ac51826 100644 --- a/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_finallyNull.ts +++ b/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_finallyNull.ts @@ -8,5 +8,5 @@ function /*[#|*/f/*|]*/(): Promise { declare function foo(): Promise; async function f(): Promise { - return await foo(); + return foo(); } \ No newline at end of file diff --git a/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_finallyUndefined.ts b/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_finallyUndefined.ts index 8d979bdcb646d..4cabfe12e70af 100644 --- a/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_finallyUndefined.ts +++ b/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_finallyUndefined.ts @@ -8,5 +8,5 @@ function /*[#|*/f/*|]*/(): Promise { declare function foo(): Promise; async function f(): Promise { - return await foo(); + return foo(); } \ No newline at end of file diff --git a/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_thenNoArguments.ts b/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_thenNoArguments.ts index 916db17d4bd35..66f5799bdc460 100644 --- a/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_thenNoArguments.ts +++ b/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_thenNoArguments.ts @@ -8,5 +8,5 @@ function /*[#|*/f/*|]*/(): Promise { declare function foo(): Promise; async function f(): Promise { - return await foo(); + return foo(); } \ No newline at end of file