From 59e556b5a4eeccfa12b855db34d658fc7ba1a96b Mon Sep 17 00:00:00 2001 From: Andy Hanson Date: Tue, 18 Sep 2018 10:15:50 -0700 Subject: [PATCH 1/3] convertToAsyncFunction: Use ReadonlyArray / ReadonlyMap where possible --- .../codefixes/convertToAsyncFunction.ts | 80 +++++++++---------- 1 file changed, 40 insertions(+), 40 deletions(-) diff --git a/src/services/codefixes/convertToAsyncFunction.ts b/src/services/codefixes/convertToAsyncFunction.ts index 01aaaae65caaa..42d737a9e5fe2 100644 --- a/src/services/codefixes/convertToAsyncFunction.ts +++ b/src/services/codefixes/convertToAsyncFunction.ts @@ -15,24 +15,24 @@ namespace ts.codefix { }); interface SynthIdentifier { - identifier: Identifier; - types: Type[]; + readonly identifier: Identifier; + readonly types: Type[]; numberOfAssignmentsOriginal: number; // number of times the variable should be assigned in the refactor } interface SymbolAndIdentifier { - identifier: Identifier; - symbol: Symbol; + readonly identifier: Identifier; + readonly symbol: Symbol; } interface Transformer { - checker: TypeChecker; - synthNamesMap: Map; // keys are the symbol id of the identifier - allVarNames: SymbolAndIdentifier[]; - setOfExpressionsToReturn: Map; // keys are the node ids of the expressions - constIdentifiers: Identifier[]; - originalTypeMap: Map; // keys are the node id of the identifier - isInJSFile: boolean; + readonly checker: TypeChecker; + readonly synthNamesMap: Map; // keys are the symbol id of the identifier + readonly allVarNames: ReadonlyArray; + readonly setOfExpressionsToReturn: ReadonlyMap; // keys are the node ids of the expressions + readonly constIdentifiers: Identifier[]; + readonly originalTypeMap: ReadonlyMap; // keys are the node id of the identifier + readonly isInJSFile: boolean; } function convertToAsyncFunction(changes: textChanges.ChangeTracker, sourceFile: SourceFile, position: number, checker: TypeChecker, context: CodeFixContextBase): void { @@ -61,7 +61,7 @@ namespace ts.codefix { const functionToConvertRenamed: FunctionLikeDeclaration = renameCollidingVarNames(functionToConvert, checker, synthNamesMap, context, setOfExpressionsToReturn, originalTypeMap, allVarNames); const constIdentifiers = getConstIdentifiers(synthNamesMap); const returnStatements = getReturnStatementsWithPromiseHandlers(functionToConvertRenamed); - const transformer = { checker, synthNamesMap, allVarNames, setOfExpressionsToReturn, constIdentifiers, originalTypeMap, isInJSFile: isInJavascript }; + const transformer: Transformer = { checker, synthNamesMap, allVarNames, setOfExpressionsToReturn, constIdentifiers, originalTypeMap, isInJSFile: isInJavascript }; if (!returnStatements.length) { return; @@ -88,7 +88,7 @@ namespace ts.codefix { } // Returns the identifiers that are never reassigned in the refactor - function getConstIdentifiers(synthNamesMap: Map): Identifier[] { + function getConstIdentifiers(synthNamesMap: ReadonlyMap): Identifier[] { const constIdentifiers: Identifier[] = []; synthNamesMap.forEach((val) => { if (val.numberOfAssignmentsOriginal === 0) { @@ -249,8 +249,8 @@ namespace ts.codefix { } } - function getNewNameIfConflict(name: Identifier, originalNames: Map): SynthIdentifier { - const numVarsSameName = (originalNames.get(name.text) || []).length; + function getNewNameIfConflict(name: Identifier, originalNames: ReadonlyMap): SynthIdentifier { + const numVarsSameName = (originalNames.get(name.text) || emptyArray).length; const numberOfAssignmentsOriginal = 0; const identifier = numVarsSameName === 0 ? name : createIdentifier(name.text + "_" + numVarsSameName); return { identifier, types: [], numberOfAssignmentsOriginal }; @@ -258,9 +258,9 @@ 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, outermostParent: CallExpression, prevArgName?: SynthIdentifier): Statement[] { + function transformExpression(node: Expression, transformer: Transformer, outermostParent: CallExpression, prevArgName?: SynthIdentifier): ReadonlyArray { if (!node) { - return []; + return emptyArray; } const originalType = isIdentifier(node) && transformer.originalTypeMap.get(getNodeId(node).toString()); @@ -280,10 +280,10 @@ namespace ts.codefix { } codeActionSucceeded = false; - return []; + return emptyArray; } - function transformCatch(node: CallExpression, transformer: Transformer, prevArgName?: SynthIdentifier): Statement[] { + function transformCatch(node: CallExpression, transformer: Transformer, prevArgName?: SynthIdentifier): ReadonlyArray { const func = node.arguments[0]; const argName = getArgName(func, transformer); const shouldReturn = transformer.setOfExpressionsToReturn.get(getNodeId(node).toString()); @@ -336,7 +336,7 @@ namespace ts.codefix { return newSynthName; } - function transformThen(node: CallExpression, transformer: Transformer, outermostParent: CallExpression, prevArgName?: SynthIdentifier): Statement[] { + function transformThen(node: CallExpression, transformer: Transformer, outermostParent: CallExpression, prevArgName?: SynthIdentifier): ReadonlyArray { const [res, rej] = node.arguments; if (!res) { @@ -356,18 +356,18 @@ namespace ts.codefix { const catchArg = argNameRej ? argNameRej.identifier.text : "e"; const catchClause = createCatchClause(catchArg, createBlock(transformationBody2)); - return [createTry(tryBlock, catchClause, /* finallyBlock */ undefined) as Statement]; + return [createTry(tryBlock, catchClause, /* finallyBlock */ undefined)]; } return transformExpression(node.expression, transformer, node, argNameRes).concat(transformationBody); } - function getFlagOfIdentifier(node: Identifier, constIdentifiers: Identifier[]): NodeFlags { + function getFlagOfIdentifier(node: Identifier, constIdentifiers: ReadonlyArray): NodeFlags { const inArr: boolean = constIdentifiers.some(elem => elem.text === node.text); return inArr ? NodeFlags.Const : NodeFlags.Let; } - function transformPromiseCall(node: Expression, transformer: Transformer, prevArgName?: SynthIdentifier): Statement[] { + function transformPromiseCall(node: Expression, transformer: Transformer, prevArgName?: SynthIdentifier): ReadonlyArray { const shouldReturn = transformer.setOfExpressionsToReturn.get(getNodeId(node).toString()); // the identifier is empty when the handler (.then()) ignores the argument - In this situation we do not need to save the result of the promise returning call const originalNodeParent = node.original ? node.original.parent : node.parent; @@ -381,23 +381,23 @@ namespace ts.codefix { return [createReturn(getSynthesizedDeepClone(node))]; } - function createTransformedStatement(prevArgName: SynthIdentifier | undefined, rightHandSide: Expression, transformer: Transformer): MutableNodeArray { + function createTransformedStatement(prevArgName: SynthIdentifier | undefined, rightHandSide: Expression, transformer: Transformer): ReadonlyArray { if (!prevArgName || prevArgName.identifier.text.length === 0) { // if there's no argName to assign to, there still might be side effects - return createNodeArray([createStatement(rightHandSide)]); + return [createStatement(rightHandSide)]; } if (prevArgName.types.length < prevArgName.numberOfAssignmentsOriginal) { // if the variable has already been declared, we don't need "let" or "const" - return createNodeArray([createStatement(createAssignment(getSynthesizedDeepClone(prevArgName.identifier), rightHandSide))]); + return [createStatement(createAssignment(getSynthesizedDeepClone(prevArgName.identifier), rightHandSide))]; } - return createNodeArray([createVariableStatement(/*modifiers*/ undefined, - (createVariableDeclarationList([createVariableDeclaration(getSynthesizedDeepClone(prevArgName.identifier), /*type*/ undefined, rightHandSide)], getFlagOfIdentifier(prevArgName.identifier, transformer.constIdentifiers))))]); + return [createVariableStatement(/*modifiers*/ undefined, + (createVariableDeclarationList([createVariableDeclaration(getSynthesizedDeepClone(prevArgName.identifier), /*type*/ undefined, rightHandSide)], getFlagOfIdentifier(prevArgName.identifier, transformer.constIdentifiers))))]; } // should be kept up to date with isFixablePromiseArgument in suggestionDiagnostics.ts - function getTransformationBody(func: Expression, prevArgName: SynthIdentifier | undefined, argName: SynthIdentifier | undefined, parent: CallExpression, transformer: Transformer): NodeArray { + function getTransformationBody(func: Expression, prevArgName: SynthIdentifier | undefined, argName: SynthIdentifier | undefined, parent: CallExpression, transformer: Transformer): ReadonlyArray { const shouldReturn = transformer.setOfExpressionsToReturn.get(getNodeId(parent).toString()); switch (func.kind) { @@ -410,9 +410,9 @@ namespace ts.codefix { break; } - const synthCall = createCall(getSynthesizedDeepClone(func) as Identifier, /*typeArguments*/ undefined, argName ? [argName.identifier] : []); + const synthCall = createCall(getSynthesizedDeepClone(func as Identifier), /*typeArguments*/ undefined, argName ? [argName.identifier] : emptyArray); if (shouldReturn) { - return createNodeArray([createReturn(synthCall)]); + return [createReturn(synthCall)]; } const type = transformer.originalTypeMap.get(getNodeId(func).toString()) || transformer.checker.getTypeAtLocation(func); @@ -450,15 +450,15 @@ namespace ts.codefix { } } - return shouldReturn ? getSynthesizedDeepClones(createNodeArray(refactoredStmts)) : - removeReturns(createNodeArray(refactoredStmts), prevArgName!.identifier, transformer, seenReturnStatement); + return shouldReturn ? refactoredStmts.map(s => getSynthesizedDeepClone(s)) : + removeReturns(refactoredStmts, prevArgName!.identifier, transformer, seenReturnStatement); } else { const innerRetStmts = getReturnStatementsWithPromiseHandlers(createReturn(funcBody)); const innerCbBody = getInnerTransformationBody(transformer, innerRetStmts, prevArgName); if (innerCbBody.length > 0) { - return createNodeArray(innerCbBody); + return innerCbBody; } if (!shouldReturn) { @@ -473,7 +473,7 @@ namespace ts.codefix { return transformedStatement; } else { - return createNodeArray([createReturn(getSynthesizedDeepClone(funcBody))]); + return [createReturn(getSynthesizedDeepClone(funcBody))]; } } } @@ -482,7 +482,7 @@ namespace ts.codefix { codeActionSucceeded = false; break; } - return createNodeArray([]); + return emptyArray; } function getLastCallSignature(type: Type, checker: TypeChecker): Signature | undefined { @@ -491,7 +491,7 @@ namespace ts.codefix { } - function removeReturns(stmts: NodeArray, prevArgName: Identifier, transformer: Transformer, seenReturnStatement: boolean): NodeArray { + function removeReturns(stmts: ReadonlyArray, prevArgName: Identifier, transformer: Transformer, seenReturnStatement: boolean): ReadonlyArray { const ret: Statement[] = []; for (const stmt of stmts) { if (isReturnStatement(stmt)) { @@ -512,15 +512,15 @@ namespace ts.codefix { (createVariableDeclarationList([createVariableDeclaration(prevArgName, /*type*/ undefined, createIdentifier("undefined"))], getFlagOfIdentifier(prevArgName, transformer.constIdentifiers))))); } - return createNodeArray(ret); + return ret; } - function getInnerTransformationBody(transformer: Transformer, innerRetStmts: Node[], prevArgName?: SynthIdentifier) { + function getInnerTransformationBody(transformer: Transformer, innerRetStmts: ReadonlyArray, prevArgName?: SynthIdentifier) { let innerCbBody: Statement[] = []; for (const stmt of innerRetStmts) { - forEachChild(stmt, function visit(node: Node) { + forEachChild(stmt, function visit(node) { if (isCallExpression(node)) { const temp = transformExpression(node, transformer, node, prevArgName); innerCbBody = innerCbBody.concat(temp); From 045c73a5a28da34aab284e53125ede5dd3a532c0 Mon Sep 17 00:00:00 2001 From: Dhruv Rajvanshi Date: Fri, 28 Sep 2018 07:26:37 +0530 Subject: [PATCH 2/3] Issue #27301: Fixed crash when converting function to async (#27396) --- .../codefixes/convertToAsyncFunction.ts | 19 ++++++++---- .../unittests/convertToAsyncFunction.ts | 15 +++++++++- .../convertToAsyncFunction_noArgs.js | 30 +++++++++++++++++++ .../convertToAsyncFunction_noArgs.ts | 30 +++++++++++++++++++ 4 files changed, 88 insertions(+), 6 deletions(-) create mode 100644 tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_noArgs.js create mode 100644 tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_noArgs.ts diff --git a/src/services/codefixes/convertToAsyncFunction.ts b/src/services/codefixes/convertToAsyncFunction.ts index 42d737a9e5fe2..fbaf0178d2f5e 100644 --- a/src/services/codefixes/convertToAsyncFunction.ts +++ b/src/services/codefixes/convertToAsyncFunction.ts @@ -451,7 +451,11 @@ namespace ts.codefix { } return shouldReturn ? refactoredStmts.map(s => getSynthesizedDeepClone(s)) : - removeReturns(refactoredStmts, prevArgName!.identifier, transformer, seenReturnStatement); + removeReturns( + refactoredStmts, + prevArgName === undefined ? undefined : prevArgName.identifier, + transformer, + seenReturnStatement); } else { const innerRetStmts = getReturnStatementsWithPromiseHandlers(createReturn(funcBody)); @@ -491,14 +495,19 @@ namespace ts.codefix { } - function removeReturns(stmts: ReadonlyArray, prevArgName: Identifier, transformer: Transformer, seenReturnStatement: boolean): ReadonlyArray { + function removeReturns(stmts: ReadonlyArray, prevArgName: Identifier | undefined, transformer: Transformer, seenReturnStatement: boolean): ReadonlyArray { const ret: Statement[] = []; for (const stmt of stmts) { if (isReturnStatement(stmt)) { if (stmt.expression) { const possiblyAwaitedExpression = isPromiseReturningExpression(stmt.expression, transformer.checker) ? createAwait(stmt.expression) : stmt.expression; - ret.push(createVariableStatement(/*modifiers*/ undefined, - (createVariableDeclarationList([createVariableDeclaration(prevArgName, /*type*/ undefined, possiblyAwaitedExpression)], getFlagOfIdentifier(prevArgName, transformer.constIdentifiers))))); + if (prevArgName === undefined) { + ret.push(createExpressionStatement(possiblyAwaitedExpression)); + } + else { + ret.push(createVariableStatement(/*modifiers*/ undefined, + (createVariableDeclarationList([createVariableDeclaration(prevArgName, /*type*/ undefined, possiblyAwaitedExpression)], getFlagOfIdentifier(prevArgName, transformer.constIdentifiers))))); + } } } else { @@ -507,7 +516,7 @@ namespace ts.codefix { } // if block has no return statement, need to define prevArgName as undefined to prevent undeclared variables - if (!seenReturnStatement) { + if (!seenReturnStatement && prevArgName !== undefined) { ret.push(createVariableStatement(/*modifiers*/ undefined, (createVariableDeclarationList([createVariableDeclaration(prevArgName, /*type*/ undefined, createIdentifier("undefined"))], getFlagOfIdentifier(prevArgName, transformer.constIdentifiers))))); } diff --git a/src/testRunner/unittests/convertToAsyncFunction.ts b/src/testRunner/unittests/convertToAsyncFunction.ts index c1a08b3319c45..162b35784786e 100644 --- a/src/testRunner/unittests/convertToAsyncFunction.ts +++ b/src/testRunner/unittests/convertToAsyncFunction.ts @@ -1241,6 +1241,19 @@ _testConvertToAsyncFunction("convertToAsyncFunction_nestedPromises", ` function [#|f|]() { return fetch('https://typescriptlang.org').then(x => Promise.resolve(3).then(y => Promise.resolve(x.statusText.length + y))); } +`); +_testConvertToAsyncFunction("convertToAsyncFunction_noArgs", ` +function delay(millis: number): Promise { + throw "no" +} + +function [#|main2|]() { + console.log("Please wait. Loading."); + return delay(500) + .then(() => { console.log("."); return delay(500); }) + .then(() => { console.log("."); return delay(500); }) + .then(() => { console.log("."); return delay(500); }) +} `); }); @@ -1251,4 +1264,4 @@ function [#|f|]() { function _testConvertToAsyncFunctionFailed(caption: string, text: string) { testConvertToAsyncFunction(caption, text, "convertToAsyncFunction", /*includeLib*/ true, /*expectFailure*/ true); } -} \ No newline at end of file +} diff --git a/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_noArgs.js b/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_noArgs.js new file mode 100644 index 0000000000000..8cda2c9dc944f --- /dev/null +++ b/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_noArgs.js @@ -0,0 +1,30 @@ +// ==ORIGINAL== + +function delay(millis) { + throw "no" +} + +function /*[#|*/main2/*|]*/() { + console.log("Please wait. Loading."); + return delay(500) + .then(() => { console.log("."); return delay(500); }) + .then(() => { console.log("."); return delay(500); }) + .then(() => { console.log("."); return delay(500); }) +} + +// ==ASYNC FUNCTION::Convert to async function== + +function delay(millis) { + throw "no" +} + +async function main2() { + console.log("Please wait. Loading."); + await delay(500); + console.log("."); + await delay(500); + console.log("."); + await delay(500); + console.log("."); + return delay(500); +} diff --git a/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_noArgs.ts b/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_noArgs.ts new file mode 100644 index 0000000000000..088bf9f828e76 --- /dev/null +++ b/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_noArgs.ts @@ -0,0 +1,30 @@ +// ==ORIGINAL== + +function delay(millis: number): Promise { + throw "no" +} + +function /*[#|*/main2/*|]*/() { + console.log("Please wait. Loading."); + return delay(500) + .then(() => { console.log("."); return delay(500); }) + .then(() => { console.log("."); return delay(500); }) + .then(() => { console.log("."); return delay(500); }) +} + +// ==ASYNC FUNCTION::Convert to async function== + +function delay(millis: number): Promise { + throw "no" +} + +async function main2() { + console.log("Please wait. Loading."); + await delay(500); + console.log("."); + await delay(500); + console.log("."); + await delay(500); + console.log("."); + return delay(500); +} From 7c6a14e65df2240af135f5a9fdd235ebb28b4d50 Mon Sep 17 00:00:00 2001 From: Benjamin Lichtman Date: Mon, 1 Oct 2018 17:02:43 -0700 Subject: [PATCH 3/3] Insert async keyword as last modifier --- src/services/codefixes/convertToAsyncFunction.ts | 2 +- src/services/textChanges.ts | 10 ++++++++++ src/testRunner/unittests/convertToAsyncFunction.ts | 5 +++++ .../convertToAsyncFunction_exportModifier.js | 12 ++++++++++++ .../convertToAsyncFunction_exportModifier.ts | 12 ++++++++++++ 5 files changed, 40 insertions(+), 1 deletion(-) create mode 100644 tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_exportModifier.js create mode 100644 tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_exportModifier.ts diff --git a/src/services/codefixes/convertToAsyncFunction.ts b/src/services/codefixes/convertToAsyncFunction.ts index fbaf0178d2f5e..c02609e8909ec 100644 --- a/src/services/codefixes/convertToAsyncFunction.ts +++ b/src/services/codefixes/convertToAsyncFunction.ts @@ -68,7 +68,7 @@ namespace ts.codefix { } // add the async keyword - changes.insertModifierBefore(sourceFile, SyntaxKind.AsyncKeyword, functionToConvert); + changes.insertLastModifierBefore(sourceFile, SyntaxKind.AsyncKeyword, functionToConvert); function startTransformation(node: CallExpression, nodeToReplace: Node) { const newNodes = transformExpression(node, transformer, node); diff --git a/src/services/textChanges.ts b/src/services/textChanges.ts index 756d2799a28cb..3b6da401db080 100644 --- a/src/services/textChanges.ts +++ b/src/services/textChanges.ts @@ -315,6 +315,16 @@ namespace ts.textChanges { this.replaceRange(sourceFile, { pos, end: pos }, createToken(modifier), { suffix: " " }); } + public insertLastModifierBefore(sourceFile: SourceFile, modifier: SyntaxKind, before: Node): void { + if (!before.modifiers) { + this.insertModifierBefore(sourceFile, modifier, before); + return; + } + + const pos = before.modifiers.end; + this.replaceRange(sourceFile, { pos, end: pos }, createToken(modifier), { prefix: " " }); + } + public insertCommentBeforeLine(sourceFile: SourceFile, lineNumber: number, position: number, commentText: string): void { const lineStartPosition = getStartPositionOfLine(lineNumber, sourceFile); const startPosition = getFirstNonSpaceCharacterPosition(sourceFile.text, lineStartPosition); diff --git a/src/testRunner/unittests/convertToAsyncFunction.ts b/src/testRunner/unittests/convertToAsyncFunction.ts index 162b35784786e..34316036858ae 100644 --- a/src/testRunner/unittests/convertToAsyncFunction.ts +++ b/src/testRunner/unittests/convertToAsyncFunction.ts @@ -1254,6 +1254,11 @@ function [#|main2|]() { .then(() => { console.log("."); return delay(500); }) .then(() => { console.log("."); return delay(500); }) } +`); +_testConvertToAsyncFunction("convertToAsyncFunction_exportModifier", ` +export function [#|foo|]() { + return fetch('https://typescriptlang.org').then(s => console.log(s)); +} `); }); diff --git a/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_exportModifier.js b/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_exportModifier.js new file mode 100644 index 0000000000000..5ae3876f9e849 --- /dev/null +++ b/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_exportModifier.js @@ -0,0 +1,12 @@ +// ==ORIGINAL== + +export function /*[#|*/foo/*|]*/() { + return fetch('https://typescriptlang.org').then(s => console.log(s)); +} + +// ==ASYNC FUNCTION::Convert to async function== + +export async function foo() { + const s = await fetch('https://typescriptlang.org'); + return console.log(s); +} diff --git a/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_exportModifier.ts b/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_exportModifier.ts new file mode 100644 index 0000000000000..5ae3876f9e849 --- /dev/null +++ b/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_exportModifier.ts @@ -0,0 +1,12 @@ +// ==ORIGINAL== + +export function /*[#|*/foo/*|]*/() { + return fetch('https://typescriptlang.org').then(s => console.log(s)); +} + +// ==ASYNC FUNCTION::Convert to async function== + +export async function foo() { + const s = await fetch('https://typescriptlang.org'); + return console.log(s); +}