From 8c9e8666ed6f02b5f7c29430e475cbe4a4ad4444 Mon Sep 17 00:00:00 2001 From: Benjamin Lichtman Date: Thu, 6 Sep 2018 15:53:05 -0700 Subject: [PATCH 01/12] Miscellaneous cleanup --- .../codefixes/convertToAsyncFunction.ts | 27 +++++++------------ src/services/suggestionDiagnostics.ts | 14 +++------- src/services/utilities.ts | 2 +- 3 files changed, 15 insertions(+), 28 deletions(-) diff --git a/src/services/codefixes/convertToAsyncFunction.ts b/src/services/codefixes/convertToAsyncFunction.ts index 76cdf471466..9d4d6f74be4 100644 --- a/src/services/codefixes/convertToAsyncFunction.ts +++ b/src/services/codefixes/convertToAsyncFunction.ts @@ -81,19 +81,14 @@ namespace ts.codefix { } for (const statement of returnStatements) { - if (isCallExpression(statement)) { - startTransformation(statement, statement); - } - else { - forEachChild(statement, function visit(node: Node) { - if (isCallExpression(node)) { - startTransformation(node, statement); - } - else if (!isFunctionLike(node)) { - forEachChild(node, visit); - } - }); - } + forEachChild(statement, function visit(node: Node) { + if (isCallExpression(node)) { + startTransformation(node, statement); + } + else if (!isFunctionLike(node)) { + forEachChild(node, visit); + } + }); } } @@ -344,11 +339,8 @@ namespace ts.codefix { return [createTry(tryBlock, catchClause, /* finallyBlock */ undefined) as Statement]; } - else { - return transformExpression(node.expression, transformer, node, argNameRes).concat(transformationBody); - } - return []; + return transformExpression(node.expression, transformer, node, argNameRes).concat(transformationBody); } function getFlagOfIdentifier(node: Identifier, constIdentifiers: Identifier[]): NodeFlags { @@ -513,6 +505,7 @@ namespace ts.codefix { name = getMapEntryIfExists(param); } } + // currently not relevant, since we don't produce a valid transformation if the argument to a promise operation is a CallExpression else if (isCallExpression(funcNode) && funcNode.arguments.length > 0 && isIdentifier(funcNode.arguments[0])) { name = { identifier: funcNode.arguments[0] as Identifier, types, numberOfAssignmentsOriginal }; } diff --git a/src/services/suggestionDiagnostics.ts b/src/services/suggestionDiagnostics.ts index 167bcb6bbac..c1af35eefa8 100644 --- a/src/services/suggestionDiagnostics.ts +++ b/src/services/suggestionDiagnostics.ts @@ -141,8 +141,8 @@ namespace ts { } /** @internal */ - export function getReturnStatementsWithPromiseHandlers(node: Node): Node[] { - const returnStatements: Node[] = []; + export function getReturnStatementsWithPromiseHandlers(node: Node): ReturnStatement[] { + const returnStatements: ReturnStatement[] = []; if (isFunctionLike(node)) { forEachChild(node, visit); } @@ -155,14 +155,8 @@ namespace ts { return; } - if (isReturnStatement(child)) { - forEachChild(child, addHandlers); - } - - function addHandlers(returnChild: Node) { - if (isPromiseHandler(returnChild)) { - returnStatements.push(child as ReturnStatement); - } + if (isReturnStatement(child) && child.expression && isPromiseHandler(child.expression)) { + returnStatements.push(child); } forEachChild(child, visit); diff --git a/src/services/utilities.ts b/src/services/utilities.ts index 1c10bc983fd..4559a88881e 100644 --- a/src/services/utilities.ts +++ b/src/services/utilities.ts @@ -1671,7 +1671,7 @@ namespace ts { } if (clone && !includeTrivia) suppressLeadingAndTrailingTrivia(clone); - if (callback && node) callback(node!, clone); + if (callback && node && clone) callback(node!, clone); return clone as T; } From 7466ac1cd58b102547833f2a7c0b9037c2315d2b Mon Sep 17 00:00:00 2001 From: Benjamin Lichtman Date: Thu, 6 Sep 2018 15:53:13 -0700 Subject: [PATCH 02/12] [WIP] add test --- src/testRunner/unittests/convertToAsyncFunction.ts | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/testRunner/unittests/convertToAsyncFunction.ts b/src/testRunner/unittests/convertToAsyncFunction.ts index 99788e1310e..d1655824216 100644 --- a/src/testRunner/unittests/convertToAsyncFunction.ts +++ b/src/testRunner/unittests/convertToAsyncFunction.ts @@ -1194,6 +1194,11 @@ const [#|foo|] = function () { } `); + _testConvertToAsyncFunction("convertToAsyncFunction_catchBlockUniqueParams", ` +function [#|f|]() { + return Promise.resolve().then(x => 1).catch(x => "a").then(x => !!x); +} +`); }); From a4c87df821259ef03fd92b9993b93e10aee7fab8 Mon Sep 17 00:00:00 2001 From: Benjamin Lichtman Date: Fri, 7 Sep 2018 16:28:47 -0700 Subject: [PATCH 03/12] [WIP] Use original identifier name to count up from when renaming collisions --- src/services/codefixes/convertToAsyncFunction.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/services/codefixes/convertToAsyncFunction.ts b/src/services/codefixes/convertToAsyncFunction.ts index 9d4d6f74be4..97658fd3e2f 100644 --- a/src/services/codefixes/convertToAsyncFunction.ts +++ b/src/services/codefixes/convertToAsyncFunction.ts @@ -240,7 +240,7 @@ namespace ts.codefix { } function getNewNameIfConflict(name: Identifier, allVarNames: SymbolAndIdentifier[]): SynthIdentifier { - const numVarsSameName = allVarNames.filter(elem => elem.identifier.text === name.text).length; + const numVarsSameName = allVarNames.filter(elem => elem.symbol.name === name.text).length; const numberOfAssignmentsOriginal = 0; const identifier = numVarsSameName === 0 ? name : createIdentifier(name.text + "_" + numVarsSameName); return { identifier, types: [], numberOfAssignmentsOriginal }; @@ -426,7 +426,7 @@ namespace ts.codefix { if (hasPrevArgName && !shouldReturn) { const type = transformer.checker.getTypeAtLocation(func); - const returnType = getLastCallSignature(type, transformer.checker).getReturnType(); + const returnType = getLastCallSignature(type, transformer.checker)!.getReturnType(); const varDeclOrAssignment = createVariableDeclarationOrAssignment(prevArgName!, getSynthesizedDeepClone(funcBody) as Expression, transformer); prevArgName!.types.push(returnType); return varDeclOrAssignment; @@ -440,7 +440,7 @@ namespace ts.codefix { return createNodeArray([]); } - function getLastCallSignature(type: Type, checker: TypeChecker): Signature { + function getLastCallSignature(type: Type, checker: TypeChecker): Signature | undefined { const callSignatures = type && checker.getSignaturesOfType(type, SignatureKind.Call); return callSignatures && callSignatures[callSignatures.length - 1]; } From 92edc2db56693186d7acf405c279c2e1898fecb5 Mon Sep 17 00:00:00 2001 From: Benjamin Lichtman Date: Fri, 7 Sep 2018 17:04:34 -0700 Subject: [PATCH 04/12] [WIP] Record original name of renamed variable --- .../codefixes/convertToAsyncFunction.ts | 22 ++++++++++--------- ...tToAsyncFunction_catchBlockUniqueParams.js | 19 ++++++++++++++++ ...tToAsyncFunction_catchBlockUniqueParams.ts | 19 ++++++++++++++++ 3 files changed, 50 insertions(+), 10 deletions(-) create mode 100644 tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_catchBlockUniqueParams.js create mode 100644 tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_catchBlockUniqueParams.ts diff --git a/src/services/codefixes/convertToAsyncFunction.ts b/src/services/codefixes/convertToAsyncFunction.ts index 97658fd3e2f..d8d6fa05d1f 100644 --- a/src/services/codefixes/convertToAsyncFunction.ts +++ b/src/services/codefixes/convertToAsyncFunction.ts @@ -25,15 +25,16 @@ namespace ts.codefix { numberOfAssignmentsOriginal: number; } - interface SymbolAndIdentifier { + interface SymbolAndIdentifierAndOriginalName { identifier: Identifier; symbol: Symbol; + originalName: string; } interface Transformer { checker: TypeChecker; synthNamesMap: Map; // keys are the symbol id of the identifier - allVarNames: SymbolAndIdentifier[]; + allVarNames: SymbolAndIdentifierAndOriginalName[]; setOfExpressionsToReturn: Map; // keys are the node ids of the expressions constIdentifiers: Identifier[]; originalTypeMap: Map; // keys are the node id of the identifier @@ -60,7 +61,7 @@ namespace ts.codefix { const synthNamesMap: Map = createMap(); const originalTypeMap: Map = createMap(); - const allVarNames: SymbolAndIdentifier[] = []; + const allVarNames: SymbolAndIdentifierAndOriginalName[] = []; const isInJSFile = isInJavaScriptFile(functionToConvert); const setOfExpressionsToReturn = getAllPromiseExpressionsToReturn(functionToConvert, checker); const functionToConvertRenamed: FunctionLikeDeclaration = renameCollidingVarNames(functionToConvert, checker, synthNamesMap, context, setOfExpressionsToReturn, originalTypeMap, allVarNames); @@ -157,7 +158,7 @@ namespace ts.codefix { This function collects all existing identifier names and names of identifiers that will be created in the refactor. It then checks for any collisions and renames them through getSynthesizedDeepClone */ - function renameCollidingVarNames(nodeToRename: FunctionLikeDeclaration, checker: TypeChecker, synthNamesMap: Map, context: CodeFixContextBase, setOfAllExpressionsToReturn: Map, originalType: Map, allVarNames: SymbolAndIdentifier[]): FunctionLikeDeclaration { + function renameCollidingVarNames(nodeToRename: FunctionLikeDeclaration, checker: TypeChecker, synthNamesMap: Map, context: CodeFixContextBase, setOfAllExpressionsToReturn: Map, originalType: Map, allVarNames: SymbolAndIdentifierAndOriginalName[]): FunctionLikeDeclaration { const identsToRenameMap: Map = createMap(); // key is the symbol id forEachChild(nodeToRename, function visit(node: Node) { @@ -177,26 +178,27 @@ namespace ts.codefix { // if the identifier refers to a function we want to add the new synthesized variable for the declaration (ex. blob in let blob = res(arg)) // Note - the choice of the last call signature is arbitrary if (lastCallSignature && lastCallSignature.parameters.length && !synthNamesMap.has(symbolIdString)) { + const name = lastCallSignature.parameters[0].name; const synthName = getNewNameIfConflict(createIdentifier(lastCallSignature.parameters[0].name), allVarNames); synthNamesMap.set(symbolIdString, synthName); - allVarNames.push({ identifier: synthName.identifier, symbol }); + allVarNames.push({ identifier: synthName.identifier, symbol, originalName: name }); } // we only care about identifiers that are parameters and declarations (don't care about other uses) else if (node.parent && (isParameter(node.parent) || isVariableDeclaration(node.parent))) { // if the identifier name conflicts with a different identifier that we've already seen - if (allVarNames.some(ident => ident.identifier.text === node.text && ident.symbol !== symbol)) { + if (allVarNames.some(ident => ident.originalName === node.text && ident.symbol !== symbol)) { const newName = getNewNameIfConflict(node, allVarNames); identsToRenameMap.set(symbolIdString, newName.identifier); synthNamesMap.set(symbolIdString, newName); - allVarNames.push({ identifier: newName.identifier, symbol }); + allVarNames.push({ identifier: newName.identifier, symbol, originalName: node.text }); } else { const identifier = getSynthesizedDeepClone(node); identsToRenameMap.set(symbolIdString, identifier); synthNamesMap.set(symbolIdString, { identifier, types: [], numberOfAssignmentsOriginal: allVarNames.filter(elem => elem.identifier.text === node.text).length/*, numberOfAssignmentsSynthesized: 0*/ }); if ((isParameter(node.parent) && isExpressionOrCallOnTypePromise(node.parent.parent)) || isVariableDeclaration(node.parent)) { - allVarNames.push({ identifier, symbol }); + allVarNames.push({ identifier, symbol, originalName: node.text }); } } } @@ -239,8 +241,8 @@ namespace ts.codefix { } - function getNewNameIfConflict(name: Identifier, allVarNames: SymbolAndIdentifier[]): SynthIdentifier { - const numVarsSameName = allVarNames.filter(elem => elem.symbol.name === name.text).length; + function getNewNameIfConflict(name: Identifier, allVarNames: SymbolAndIdentifierAndOriginalName[]): SynthIdentifier { + const numVarsSameName = allVarNames.filter(elem => elem.originalName === name.text).length; const numberOfAssignmentsOriginal = 0; const identifier = numVarsSameName === 0 ? name : createIdentifier(name.text + "_" + numVarsSameName); return { identifier, types: [], numberOfAssignmentsOriginal }; diff --git a/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_catchBlockUniqueParams.js b/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_catchBlockUniqueParams.js new file mode 100644 index 00000000000..2600adec16b --- /dev/null +++ b/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_catchBlockUniqueParams.js @@ -0,0 +1,19 @@ +// ==ORIGINAL== + +function /*[#|*/f/*|]*/() { + return Promise.resolve().then(x => 1).catch(x => "a").then(x => !!x); +} + +// ==ASYNC FUNCTION::Convert to async function== + +async function f() { + let x_2; + try { + const x = await Promise.resolve(); + x_2 = 1; + } + catch (x_1) { + x_2 = "a"; + } + return !!x_2; +} diff --git a/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_catchBlockUniqueParams.ts b/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_catchBlockUniqueParams.ts new file mode 100644 index 00000000000..5c4daf076a0 --- /dev/null +++ b/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_catchBlockUniqueParams.ts @@ -0,0 +1,19 @@ +// ==ORIGINAL== + +function /*[#|*/f/*|]*/() { + return Promise.resolve().then(x => 1).catch(x => "a").then(x => !!x); +} + +// ==ASYNC FUNCTION::Convert to async function== + +async function f() { + let x_2: string | number; + try { + const x = await Promise.resolve(); + x_2 = 1; + } + catch (x_1) { + x_2 = "a"; + } + return !!x_2; +} From 9079df1a4d3f6c67990674545079a1fa13eafc67 Mon Sep 17 00:00:00 2001 From: Benjamin Lichtman Date: Tue, 11 Sep 2018 11:09:31 -0700 Subject: [PATCH 05/12] Update baselines --- .../convertToAsyncFunction_InnerVarNameConflict.ts | 2 +- .../convertToAsyncFunction_MultipleReturns2.ts | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_InnerVarNameConflict.ts b/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_InnerVarNameConflict.ts index 3570a90a0b1..119d9d408bb 100644 --- a/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_InnerVarNameConflict.ts +++ b/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_InnerVarNameConflict.ts @@ -13,5 +13,5 @@ function /*[#|*/f/*|]*/(): Promise { async function f(): Promise { const resp = await fetch("https://typescriptlang.org"); var blob = resp.blob().then(blob_1 => blob_1.byteOffset).catch(err => 'Error'); - return blob_1.toString(); + return blob_2.toString(); } diff --git a/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_MultipleReturns2.ts b/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_MultipleReturns2.ts index 6569c1fb0ef..389faf61891 100644 --- a/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_MultipleReturns2.ts +++ b/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_MultipleReturns2.ts @@ -21,6 +21,6 @@ async function f(): Promise { } const resp = await x; var blob = resp.blob().then(blob_1 => blob_1.byteOffset).catch(err => 'Error'); - const res_1 = await fetch("https://micorosft.com"); + const res_2 = await fetch("https://micorosft.com"); return console.log("Another one!"); } From 906fbae37b2310dcb3b5d1bc3b36c763cd957846 Mon Sep 17 00:00:00 2001 From: Benjamin Lichtman Date: Wed, 12 Sep 2018 14:47:06 -0700 Subject: [PATCH 06/12] Handle promise handler block bodies with no return and other cleanup --- .../codefixes/convertToAsyncFunction.ts | 21 ++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-) diff --git a/src/services/codefixes/convertToAsyncFunction.ts b/src/services/codefixes/convertToAsyncFunction.ts index d8d6fa05d1f..235f50226eb 100644 --- a/src/services/codefixes/convertToAsyncFunction.ts +++ b/src/services/codefixes/convertToAsyncFunction.ts @@ -82,7 +82,7 @@ namespace ts.codefix { } for (const statement of returnStatements) { - forEachChild(statement, function visit(node: Node) { + forEachChild(statement, function visit(node) { if (isCallExpression(node)) { startTransformation(node, statement); } @@ -179,7 +179,7 @@ namespace ts.codefix { // Note - the choice of the last call signature is arbitrary if (lastCallSignature && lastCallSignature.parameters.length && !synthNamesMap.has(symbolIdString)) { const name = lastCallSignature.parameters[0].name; - const synthName = getNewNameIfConflict(createIdentifier(lastCallSignature.parameters[0].name), allVarNames); + const synthName = getNewNameIfConflict(createIdentifier(name), allVarNames); synthNamesMap.set(symbolIdString, synthName); allVarNames.push({ identifier: synthName.identifier, symbol, originalName: name }); } @@ -404,8 +404,13 @@ namespace ts.codefix { // Arrow functions with block bodies { } will enter this control flow if (isFunctionLikeDeclaration(func) && func.body && isBlock(func.body) && func.body.statements) { let refactoredStmts: Statement[] = []; + let seenReturnStatement = false; for (const statement of func.body.statements) { + if (isReturnStatement(statement)) { + seenReturnStatement = true; + } + if (getReturnStatementsWithPromiseHandlers(statement).length) { refactoredStmts = refactoredStmts.concat(getInnerTransformationBody(transformer, [statement], prevArgName)); } @@ -415,7 +420,7 @@ namespace ts.codefix { } return shouldReturn ? getSynthesizedDeepClones(createNodeArray(refactoredStmts)) : - removeReturns(createNodeArray(refactoredStmts), prevArgName!.identifier, transformer.constIdentifiers); + removeReturns(createNodeArray(refactoredStmts), prevArgName!.identifier, transformer.constIdentifiers, seenReturnStatement); } else { const funcBody = (func).body; @@ -443,12 +448,12 @@ namespace ts.codefix { } function getLastCallSignature(type: Type, checker: TypeChecker): Signature | undefined { - const callSignatures = type && checker.getSignaturesOfType(type, SignatureKind.Call); + const callSignatures = checker.getSignaturesOfType(type, SignatureKind.Call); return callSignatures && callSignatures[callSignatures.length - 1]; } - function removeReturns(stmts: NodeArray, prevArgName: Identifier, constIdentifiers: Identifier[]): NodeArray { + function removeReturns(stmts: NodeArray, prevArgName: Identifier, constIdentifiers: Identifier[], seenReturnStatement: boolean): NodeArray { const ret: Statement[] = []; for (const stmt of stmts) { if (isReturnStatement(stmt)) { @@ -462,6 +467,12 @@ namespace ts.codefix { } } + // if block has no return statement, need to define prevArgName as undefined to prevent undeclared variables + if (!seenReturnStatement) { + ret.push(createVariableStatement(/*modifiers*/ undefined, + (createVariableDeclarationList([createVariableDeclaration(prevArgName, /*type*/ undefined, createIdentifier("undefined"))], getFlagOfIdentifier(prevArgName, constIdentifiers))))); + } + return createNodeArray(ret); } From 95e5f7d55a5a75581093024b5c8419625db2ff9b Mon Sep 17 00:00:00 2001 From: Benjamin Lichtman Date: Wed, 12 Sep 2018 14:47:13 -0700 Subject: [PATCH 07/12] Add and update tests --- .../unittests/convertToAsyncFunction.ts | 9 +++++++++ ...vertToAsyncFunction_InnerVarNameConflict.ts | 1 + .../convertToAsyncFunction_bindingPattern.ts | 18 ++++++++++++++++++ 3 files changed, 28 insertions(+) create mode 100644 tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_bindingPattern.ts diff --git a/src/testRunner/unittests/convertToAsyncFunction.ts b/src/testRunner/unittests/convertToAsyncFunction.ts index d1655824216..f774c58de94 100644 --- a/src/testRunner/unittests/convertToAsyncFunction.ts +++ b/src/testRunner/unittests/convertToAsyncFunction.ts @@ -1198,6 +1198,15 @@ const [#|foo|] = function () { function [#|f|]() { return Promise.resolve().then(x => 1).catch(x => "a").then(x => !!x); } +`); + + _testConvertToAsyncFunction("convertToAsyncFunction_bindingPattern", ` +function [#|f|]():Promise { + return fetch('https://typescriptlang.org').then(res); +} +function res({ status, trailer }){ + console.log(status); +} `); }); diff --git a/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_InnerVarNameConflict.ts b/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_InnerVarNameConflict.ts index 119d9d408bb..72e4a66fb55 100644 --- a/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_InnerVarNameConflict.ts +++ b/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_InnerVarNameConflict.ts @@ -13,5 +13,6 @@ function /*[#|*/f/*|]*/(): Promise { async function f(): Promise { const resp = await fetch("https://typescriptlang.org"); var blob = resp.blob().then(blob_1 => blob_1.byteOffset).catch(err => 'Error'); + const blob_2 = undefined; return blob_2.toString(); } diff --git a/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_bindingPattern.ts b/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_bindingPattern.ts new file mode 100644 index 00000000000..f7d26faa980 --- /dev/null +++ b/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_bindingPattern.ts @@ -0,0 +1,18 @@ +// ==ORIGINAL== + +function /*[#|*/f/*|]*/():Promise { + return fetch('https://typescriptlang.org').then(res); +} +function res({ status, trailer }){ + console.log(status); +} + +// ==ASYNC FUNCTION::Convert to async function== + +async function f():Promise { + const __0 = await fetch('https://typescriptlang.org'); + return res(__0); +} +function res({ status, trailer }){ + console.log(status); +} From 905578cf371ad287bf3b975ecdb8c4a43c20270b Mon Sep 17 00:00:00 2001 From: Benjamin Lichtman Date: Thu, 13 Sep 2018 09:02:02 -0700 Subject: [PATCH 08/12] Use existing identifier when possible for renaming functions --- src/services/codefixes/convertToAsyncFunction.ts | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/services/codefixes/convertToAsyncFunction.ts b/src/services/codefixes/convertToAsyncFunction.ts index 235f50226eb..51511c2207c 100644 --- a/src/services/codefixes/convertToAsyncFunction.ts +++ b/src/services/codefixes/convertToAsyncFunction.ts @@ -178,10 +178,11 @@ namespace ts.codefix { // if the identifier refers to a function we want to add the new synthesized variable for the declaration (ex. blob in let blob = res(arg)) // Note - the choice of the last call signature is arbitrary if (lastCallSignature && lastCallSignature.parameters.length && !synthNamesMap.has(symbolIdString)) { - const name = lastCallSignature.parameters[0].name; - const synthName = getNewNameIfConflict(createIdentifier(name), allVarNames); + const firstParameter = lastCallSignature.parameters[0]; + const ident = isParameter(firstParameter.valueDeclaration) && tryCast(firstParameter.valueDeclaration.name, isIdentifier) || createOptimisticUniqueName("result"); + const synthName = getNewNameIfConflict(ident, allVarNames); synthNamesMap.set(symbolIdString, synthName); - allVarNames.push({ identifier: synthName.identifier, symbol, originalName: name }); + allVarNames.push({ identifier: synthName.identifier, symbol, originalName: ident.text }); } // we only care about identifiers that are parameters and declarations (don't care about other uses) else if (node.parent && (isParameter(node.parent) || isVariableDeclaration(node.parent))) { @@ -449,7 +450,7 @@ namespace ts.codefix { function getLastCallSignature(type: Type, checker: TypeChecker): Signature | undefined { const callSignatures = checker.getSignaturesOfType(type, SignatureKind.Call); - return callSignatures && callSignatures[callSignatures.length - 1]; + return lastOrUndefined(callSignatures); } From 504b5f298542236b902892d6af66b4eab2cc966c Mon Sep 17 00:00:00 2001 From: Benjamin Lichtman Date: Thu, 13 Sep 2018 09:04:52 -0700 Subject: [PATCH 09/12] Add and update tests --- .../unittests/convertToAsyncFunction.ts | 10 ++++++++++ .../convertToAsyncFunction_bindingPattern.ts | 4 ++-- ...yncFunction_bindingPatternNameCollision.ts | 20 +++++++++++++++++++ 3 files changed, 32 insertions(+), 2 deletions(-) create mode 100644 tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_bindingPatternNameCollision.ts diff --git a/src/testRunner/unittests/convertToAsyncFunction.ts b/src/testRunner/unittests/convertToAsyncFunction.ts index f774c58de94..05df4f297e8 100644 --- a/src/testRunner/unittests/convertToAsyncFunction.ts +++ b/src/testRunner/unittests/convertToAsyncFunction.ts @@ -1207,6 +1207,16 @@ function [#|f|]():Promise { function res({ status, trailer }){ console.log(status); } +`); + + _testConvertToAsyncFunction("convertToAsyncFunction_bindingPatternNameCollision", ` +function [#|f|]():Promise { + const result = 'https://typescriptlang.org'; + return fetch(result).then(res); +} +function res({ status, trailer }){ + console.log(status); +} `); }); diff --git a/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_bindingPattern.ts b/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_bindingPattern.ts index f7d26faa980..97c68d57260 100644 --- a/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_bindingPattern.ts +++ b/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_bindingPattern.ts @@ -10,8 +10,8 @@ function res({ status, trailer }){ // ==ASYNC FUNCTION::Convert to async function== async function f():Promise { - const __0 = await fetch('https://typescriptlang.org'); - return res(__0); + const result = await fetch('https://typescriptlang.org'); + return res(result); } function res({ status, trailer }){ console.log(status); diff --git a/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_bindingPatternNameCollision.ts b/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_bindingPatternNameCollision.ts new file mode 100644 index 00000000000..db0c63535c7 --- /dev/null +++ b/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_bindingPatternNameCollision.ts @@ -0,0 +1,20 @@ +// ==ORIGINAL== + +function /*[#|*/f/*|]*/():Promise { + const result = 'https://typescriptlang.org'; + return fetch(result).then(res); +} +function res({ status, trailer }){ + console.log(status); +} + +// ==ASYNC FUNCTION::Convert to async function== + +async function f():Promise { + const result = 'https://typescriptlang.org'; + const result_1 = await fetch(result); + return res(result_1); +} +function res({ status, trailer }){ + console.log(status); +} From d12110d3e5fe2682fa9f89718c033d975924d306 Mon Sep 17 00:00:00 2001 From: Benjamin Lichtman Date: Thu, 13 Sep 2018 09:32:38 -0700 Subject: [PATCH 10/12] Respond to CR --- .../codefixes/convertToAsyncFunction.ts | 5 +++-- src/services/utilities.ts | 11 +++++----- .../unittests/convertToAsyncFunction.ts | 6 +++--- ...convertToAsyncFunction_MultipleReturns2.ts | 4 ++-- .../convertToAsyncFunction_bindingPattern.js | 18 +++++++++++++++++ .../convertToAsyncFunction_bindingPattern.ts | 4 ++-- ...yncFunction_bindingPatternNameCollision.js | 20 +++++++++++++++++++ ...yncFunction_bindingPatternNameCollision.ts | 4 ++-- 8 files changed, 55 insertions(+), 17 deletions(-) create mode 100644 tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_bindingPattern.js create mode 100644 tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_bindingPatternNameCollision.js diff --git a/src/services/codefixes/convertToAsyncFunction.ts b/src/services/codefixes/convertToAsyncFunction.ts index 51511c2207c..e698218baf9 100644 --- a/src/services/codefixes/convertToAsyncFunction.ts +++ b/src/services/codefixes/convertToAsyncFunction.ts @@ -186,20 +186,21 @@ namespace ts.codefix { } // we only care about identifiers that are parameters and declarations (don't care about other uses) else if (node.parent && (isParameter(node.parent) || isVariableDeclaration(node.parent))) { + const originalName = node.text; // if the identifier name conflicts with a different identifier that we've already seen if (allVarNames.some(ident => ident.originalName === node.text && ident.symbol !== symbol)) { const newName = getNewNameIfConflict(node, allVarNames); identsToRenameMap.set(symbolIdString, newName.identifier); synthNamesMap.set(symbolIdString, newName); - allVarNames.push({ identifier: newName.identifier, symbol, originalName: node.text }); + allVarNames.push({ identifier: newName.identifier, symbol, originalName }); } else { const identifier = getSynthesizedDeepClone(node); identsToRenameMap.set(symbolIdString, identifier); synthNamesMap.set(symbolIdString, { identifier, types: [], numberOfAssignmentsOriginal: allVarNames.filter(elem => elem.identifier.text === node.text).length/*, numberOfAssignmentsSynthesized: 0*/ }); if ((isParameter(node.parent) && isExpressionOrCallOnTypePromise(node.parent.parent)) || isVariableDeclaration(node.parent)) { - allVarNames.push({ identifier, symbol, originalName: node.text }); + allVarNames.push({ identifier, symbol, originalName }); } } } diff --git a/src/services/utilities.ts b/src/services/utilities.ts index 4559a88881e..01613a23b0d 100644 --- a/src/services/utilities.ts +++ b/src/services/utilities.ts @@ -1654,11 +1654,10 @@ namespace ts { return clone; } - export function getSynthesizedDeepCloneWithRenames(node: T, includeTrivia = true, renameMap?: Map, checker?: TypeChecker, callback?: (originalNode: Node, clone: Node) => any): T { - + export function getSynthesizedDeepCloneWithRenames(node: T, includeTrivia = true, renameMap?: Map, checker?: TypeChecker, callback?: (originalNode: Node, clone: Node) => any): T { let clone; - if (node && isIdentifier(node!) && renameMap && checker) { - const symbol = checker.getSymbolAtLocation(node!); + if (isIdentifier(node) && renameMap && checker) { + const symbol = checker.getSymbolAtLocation(node); const renameInfo = symbol && renameMap.get(String(getSymbolId(symbol))); if (renameInfo) { @@ -1667,11 +1666,11 @@ namespace ts { } if (!clone) { - clone = node && getSynthesizedDeepCloneWorker(node as NonNullable, renameMap, checker, callback); + clone = getSynthesizedDeepCloneWorker(node as NonNullable, renameMap, checker, callback); } if (clone && !includeTrivia) suppressLeadingAndTrailingTrivia(clone); - if (callback && node && clone) callback(node!, clone); + if (callback && clone) callback(node, clone); return clone as T; } diff --git a/src/testRunner/unittests/convertToAsyncFunction.ts b/src/testRunner/unittests/convertToAsyncFunction.ts index 05df4f297e8..047df5ffadc 100644 --- a/src/testRunner/unittests/convertToAsyncFunction.ts +++ b/src/testRunner/unittests/convertToAsyncFunction.ts @@ -823,7 +823,7 @@ function [#|f|](): Promise { } return x.then(resp => { var blob = resp.blob().then(blob => blob.byteOffset).catch(err => 'Error'); - return fetch("https://micorosft.com").then(res => console.log("Another one!")); + return fetch("https://microsoft.com").then(res => console.log("Another one!")); }); } ` @@ -1201,7 +1201,7 @@ function [#|f|]() { `); _testConvertToAsyncFunction("convertToAsyncFunction_bindingPattern", ` -function [#|f|]():Promise { +function [#|f|]() { return fetch('https://typescriptlang.org').then(res); } function res({ status, trailer }){ @@ -1210,7 +1210,7 @@ function res({ status, trailer }){ `); _testConvertToAsyncFunction("convertToAsyncFunction_bindingPatternNameCollision", ` -function [#|f|]():Promise { +function [#|f|]() { const result = 'https://typescriptlang.org'; return fetch(result).then(res); } diff --git a/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_MultipleReturns2.ts b/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_MultipleReturns2.ts index 389faf61891..59a02875d84 100644 --- a/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_MultipleReturns2.ts +++ b/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_MultipleReturns2.ts @@ -7,7 +7,7 @@ function /*[#|*/f/*|]*/(): Promise { } return x.then(resp => { var blob = resp.blob().then(blob => blob.byteOffset).catch(err => 'Error'); - return fetch("https://micorosft.com").then(res => console.log("Another one!")); + return fetch("https://microsoft.com").then(res => console.log("Another one!")); }); } @@ -21,6 +21,6 @@ async function f(): Promise { } const resp = await x; var blob = resp.blob().then(blob_1 => blob_1.byteOffset).catch(err => 'Error'); - const res_2 = await fetch("https://micorosft.com"); + const res_2 = await fetch("https://microsoft.com"); return console.log("Another one!"); } diff --git a/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_bindingPattern.js b/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_bindingPattern.js new file mode 100644 index 00000000000..f06ce44e78b --- /dev/null +++ b/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_bindingPattern.js @@ -0,0 +1,18 @@ +// ==ORIGINAL== + +function /*[#|*/f/*|]*/() { + return fetch('https://typescriptlang.org').then(res); +} +function res({ status, trailer }){ + console.log(status); +} + +// ==ASYNC FUNCTION::Convert to async function== + +async function f() { + const result = await fetch('https://typescriptlang.org'); + return res(result); +} +function res({ status, trailer }){ + console.log(status); +} diff --git a/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_bindingPattern.ts b/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_bindingPattern.ts index 97c68d57260..f06ce44e78b 100644 --- a/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_bindingPattern.ts +++ b/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_bindingPattern.ts @@ -1,6 +1,6 @@ // ==ORIGINAL== -function /*[#|*/f/*|]*/():Promise { +function /*[#|*/f/*|]*/() { return fetch('https://typescriptlang.org').then(res); } function res({ status, trailer }){ @@ -9,7 +9,7 @@ function res({ status, trailer }){ // ==ASYNC FUNCTION::Convert to async function== -async function f():Promise { +async function f() { const result = await fetch('https://typescriptlang.org'); return res(result); } diff --git a/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_bindingPatternNameCollision.js b/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_bindingPatternNameCollision.js new file mode 100644 index 00000000000..6813472966a --- /dev/null +++ b/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_bindingPatternNameCollision.js @@ -0,0 +1,20 @@ +// ==ORIGINAL== + +function /*[#|*/f/*|]*/() { + const result = 'https://typescriptlang.org'; + return fetch(result).then(res); +} +function res({ status, trailer }){ + console.log(status); +} + +// ==ASYNC FUNCTION::Convert to async function== + +async function f() { + const result = 'https://typescriptlang.org'; + const result_1 = await fetch(result); + return res(result_1); +} +function res({ status, trailer }){ + console.log(status); +} diff --git a/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_bindingPatternNameCollision.ts b/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_bindingPatternNameCollision.ts index db0c63535c7..6813472966a 100644 --- a/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_bindingPatternNameCollision.ts +++ b/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_bindingPatternNameCollision.ts @@ -1,6 +1,6 @@ // ==ORIGINAL== -function /*[#|*/f/*|]*/():Promise { +function /*[#|*/f/*|]*/() { const result = 'https://typescriptlang.org'; return fetch(result).then(res); } @@ -10,7 +10,7 @@ function res({ status, trailer }){ // ==ASYNC FUNCTION::Convert to async function== -async function f():Promise { +async function f() { const result = 'https://typescriptlang.org'; const result_1 = await fetch(result); return res(result_1); From e700022cef4dbee7b10d44f91d0320d2a89d8922 Mon Sep 17 00:00:00 2001 From: Benjamin Lichtman Date: Thu, 13 Sep 2018 09:46:40 -0700 Subject: [PATCH 11/12] Remove unnecessary case --- src/services/codefixes/convertToAsyncFunction.ts | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/services/codefixes/convertToAsyncFunction.ts b/src/services/codefixes/convertToAsyncFunction.ts index e698218baf9..bceee6811bb 100644 --- a/src/services/codefixes/convertToAsyncFunction.ts +++ b/src/services/codefixes/convertToAsyncFunction.ts @@ -520,10 +520,6 @@ namespace ts.codefix { name = getMapEntryIfExists(param); } } - // currently not relevant, since we don't produce a valid transformation if the argument to a promise operation is a CallExpression - else if (isCallExpression(funcNode) && funcNode.arguments.length > 0 && isIdentifier(funcNode.arguments[0])) { - name = { identifier: funcNode.arguments[0] as Identifier, types, numberOfAssignmentsOriginal }; - } else if (isIdentifier(funcNode)) { name = getMapEntryIfExists(funcNode); } From 98055ad54089faae5ed7f00747dfb985679d8b42 Mon Sep 17 00:00:00 2001 From: Benjamin Lichtman Date: Fri, 14 Sep 2018 09:46:58 -0700 Subject: [PATCH 12/12] Use separate map with smaller scope to track renames --- .../codefixes/convertToAsyncFunction.ts | 50 +++++++++++++------ 1 file changed, 35 insertions(+), 15 deletions(-) diff --git a/src/services/codefixes/convertToAsyncFunction.ts b/src/services/codefixes/convertToAsyncFunction.ts index bceee6811bb..94b311a1ea6 100644 --- a/src/services/codefixes/convertToAsyncFunction.ts +++ b/src/services/codefixes/convertToAsyncFunction.ts @@ -25,16 +25,15 @@ namespace ts.codefix { numberOfAssignmentsOriginal: number; } - interface SymbolAndIdentifierAndOriginalName { + interface SymbolAndIdentifier { identifier: Identifier; symbol: Symbol; - originalName: string; } interface Transformer { checker: TypeChecker; synthNamesMap: Map; // keys are the symbol id of the identifier - allVarNames: SymbolAndIdentifierAndOriginalName[]; + allVarNames: SymbolAndIdentifier[]; setOfExpressionsToReturn: Map; // keys are the node ids of the expressions constIdentifiers: Identifier[]; originalTypeMap: Map; // keys are the node id of the identifier @@ -61,7 +60,7 @@ namespace ts.codefix { const synthNamesMap: Map = createMap(); const originalTypeMap: Map = createMap(); - const allVarNames: SymbolAndIdentifierAndOriginalName[] = []; + const allVarNames: SymbolAndIdentifier[] = []; const isInJSFile = isInJavaScriptFile(functionToConvert); const setOfExpressionsToReturn = getAllPromiseExpressionsToReturn(functionToConvert, checker); const functionToConvertRenamed: FunctionLikeDeclaration = renameCollidingVarNames(functionToConvert, checker, synthNamesMap, context, setOfExpressionsToReturn, originalTypeMap, allVarNames); @@ -158,9 +157,10 @@ namespace ts.codefix { This function collects all existing identifier names and names of identifiers that will be created in the refactor. It then checks for any collisions and renames them through getSynthesizedDeepClone */ - function renameCollidingVarNames(nodeToRename: FunctionLikeDeclaration, checker: TypeChecker, synthNamesMap: Map, context: CodeFixContextBase, setOfAllExpressionsToReturn: Map, originalType: Map, allVarNames: SymbolAndIdentifierAndOriginalName[]): FunctionLikeDeclaration { + function renameCollidingVarNames(nodeToRename: FunctionLikeDeclaration, checker: TypeChecker, synthNamesMap: Map, context: CodeFixContextBase, setOfAllExpressionsToReturn: Map, originalType: Map, allVarNames: SymbolAndIdentifier[]): FunctionLikeDeclaration { const identsToRenameMap: Map = createMap(); // key is the symbol id + const collidingSymbolMap: Map = createMap(); forEachChild(nodeToRename, function visit(node: Node) { if (!isIdentifier(node)) { forEachChild(node, visit); @@ -180,27 +180,31 @@ namespace ts.codefix { if (lastCallSignature && lastCallSignature.parameters.length && !synthNamesMap.has(symbolIdString)) { const firstParameter = lastCallSignature.parameters[0]; const ident = isParameter(firstParameter.valueDeclaration) && tryCast(firstParameter.valueDeclaration.name, isIdentifier) || createOptimisticUniqueName("result"); - const synthName = getNewNameIfConflict(ident, allVarNames); + const synthName = getNewNameIfConflict(ident, collidingSymbolMap); synthNamesMap.set(symbolIdString, synthName); - allVarNames.push({ identifier: synthName.identifier, symbol, originalName: ident.text }); + allVarNames.push({ identifier: synthName.identifier, symbol }); + addNameToFrequencyMap(collidingSymbolMap, ident.text, symbol); } // we only care about identifiers that are parameters and declarations (don't care about other uses) else if (node.parent && (isParameter(node.parent) || isVariableDeclaration(node.parent))) { const originalName = node.text; + const collidingSymbols = collidingSymbolMap.get(originalName); // if the identifier name conflicts with a different identifier that we've already seen - if (allVarNames.some(ident => ident.originalName === node.text && ident.symbol !== symbol)) { - const newName = getNewNameIfConflict(node, allVarNames); + if (collidingSymbols && collidingSymbols.some(prevSymbol => prevSymbol !== symbol)) { + const newName = getNewNameIfConflict(node, collidingSymbolMap); identsToRenameMap.set(symbolIdString, newName.identifier); synthNamesMap.set(symbolIdString, newName); - allVarNames.push({ identifier: newName.identifier, symbol, originalName }); + allVarNames.push({ identifier: newName.identifier, symbol }); + addNameToFrequencyMap(collidingSymbolMap, originalName, symbol); } else { const identifier = getSynthesizedDeepClone(node); identsToRenameMap.set(symbolIdString, identifier); synthNamesMap.set(symbolIdString, { identifier, types: [], numberOfAssignmentsOriginal: allVarNames.filter(elem => elem.identifier.text === node.text).length/*, numberOfAssignmentsSynthesized: 0*/ }); if ((isParameter(node.parent) && isExpressionOrCallOnTypePromise(node.parent.parent)) || isVariableDeclaration(node.parent)) { - allVarNames.push({ identifier, symbol, originalName }); + allVarNames.push({ identifier, symbol }); + addNameToFrequencyMap(collidingSymbolMap, originalName, symbol); } } } @@ -243,8 +247,17 @@ namespace ts.codefix { } - function getNewNameIfConflict(name: Identifier, allVarNames: SymbolAndIdentifierAndOriginalName[]): SynthIdentifier { - const numVarsSameName = allVarNames.filter(elem => elem.originalName === name.text).length; + function addNameToFrequencyMap(renamedVarNameFrequencyMap: Map, originalName: string, symbol: Symbol) { + if (renamedVarNameFrequencyMap.has(originalName)) { + renamedVarNameFrequencyMap.get(originalName)!.push(symbol); + } + else { + renamedVarNameFrequencyMap.set(originalName, [symbol]); + } + } + + function getNewNameIfConflict(name: Identifier, originalNames: Map): SynthIdentifier { + const numVarsSameName = (originalNames.get(name.text) || []).length; const numberOfAssignmentsOriginal = 0; const identifier = numVarsSameName === 0 ? name : createIdentifier(name.text + "_" + numVarsSameName); return { identifier, types: [], numberOfAssignmentsOriginal }; @@ -289,13 +302,14 @@ namespace ts.codefix { prevArgName.numberOfAssignmentsOriginal = 2; // Try block and catch block transformer.synthNamesMap.forEach((val, key) => { if (val.identifier.text === prevArgName.identifier.text) { - transformer.synthNamesMap.set(key, getNewNameIfConflict(prevArgName.identifier, transformer.allVarNames)); + const newSynthName = createUniqueSynthName(prevArgName); + transformer.synthNamesMap.set(key, newSynthName); } }); // update the constIdentifiers list if (transformer.constIdentifiers.some(elem => elem.text === prevArgName.identifier.text)) { - transformer.constIdentifiers.push(getNewNameIfConflict(prevArgName.identifier, transformer.allVarNames).identifier); + transformer.constIdentifiers.push(createUniqueSynthName(prevArgName).identifier); } } @@ -321,6 +335,12 @@ namespace ts.codefix { return varDeclList ? [varDeclList, tryStatement] : [tryStatement]; } + function createUniqueSynthName(prevArgName: SynthIdentifier) { + const renamedPrevArg = createOptimisticUniqueName(prevArgName.identifier.text); + const newSynthName = { identifier: renamedPrevArg, types: [], numberOfAssignmentsOriginal: 0 }; + return newSynthName; + } + function transformThen(node: CallExpression, transformer: Transformer, outermostParent: CallExpression, prevArgName?: SynthIdentifier): Statement[] { const [res, rej] = node.arguments;