Skip to content

Commit bce34ad

Browse files
uniqueiniquityuniqueiniquity
authored
Merge pull request #27031 from uniqueiniquity/asyncCatchUniqueNames
Ensure async code fix renaming can do more than one rename
2 parents e547cdf + 0e985eb commit bce34ad

12 files changed

Lines changed: 210 additions & 54 deletions

‎src/services/codefixes/convertToAsyncFunction.ts‎

Lines changed: 57 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -83,19 +83,14 @@ namespace ts.codefix {
8383
}
8484

8585
for (const statement of returnStatements) {
86-
if (isCallExpression(statement)) {
87-
startTransformation(statement, statement);
88-
}
89-
else {
90-
forEachChild(statement, function visit(node: Node) {
91-
if (isCallExpression(node)) {
92-
startTransformation(node, statement);
93-
}
94-
else if (!isFunctionLike(node)) {
95-
forEachChild(node, visit);
96-
}
97-
});
98-
}
86+
forEachChild(statement, function visit(node) {
87+
if (isCallExpression(node)) {
88+
startTransformation(node, statement);
89+
}
90+
else if (!isFunctionLike(node)) {
91+
forEachChild(node, visit);
92+
}
93+
});
9994
}
10095
}
10196

@@ -167,6 +162,7 @@ namespace ts.codefix {
167162
function renameCollidingVarNames(nodeToRename: FunctionLikeDeclaration, checker: TypeChecker, synthNamesMap: Map<SynthIdentifier>, context: CodeFixContextBase, setOfAllExpressionsToReturn: Map<true>, originalType: Map<Type>, allVarNames: SymbolAndIdentifier[]): FunctionLikeDeclaration {
168163

169164
const identsToRenameMap: Map<Identifier> = createMap(); // key is the symbol id
165+
const collidingSymbolMap: Map<Symbol[]> = createMap();
170166
forEachChild(nodeToRename, function visit(node: Node) {
171167
if (!isIdentifier(node)) {
172168
forEachChild(node, visit);
@@ -184,26 +180,33 @@ namespace ts.codefix {
184180
// 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))
185181
// Note - the choice of the last call signature is arbitrary
186182
if (lastCallSignature && lastCallSignature.parameters.length && !synthNamesMap.has(symbolIdString)) {
187-
const synthName = getNewNameIfConflict(createIdentifier(lastCallSignature.parameters[0].name), allVarNames);
183+
const firstParameter = lastCallSignature.parameters[0];
184+
const ident = isParameter(firstParameter.valueDeclaration) && tryCast(firstParameter.valueDeclaration.name, isIdentifier) || createOptimisticUniqueName("result");
185+
const synthName = getNewNameIfConflict(ident, collidingSymbolMap);
188186
synthNamesMap.set(symbolIdString, synthName);
189187
allVarNames.push({ identifier: synthName.identifier, symbol });
188+
addNameToFrequencyMap(collidingSymbolMap, ident.text, symbol);
190189
}
191190
// we only care about identifiers that are parameters and declarations (don't care about other uses)
192191
else if (node.parent && (isParameter(node.parent) || isVariableDeclaration(node.parent))) {
192+
const originalName = node.text;
193+
const collidingSymbols = collidingSymbolMap.get(originalName);
193194

194195
// if the identifier name conflicts with a different identifier that we've already seen
195-
if (allVarNames.some(ident => ident.identifier.text === node.text && ident.symbol !== symbol)) {
196-
const newName = getNewNameIfConflict(node, allVarNames);
196+
if (collidingSymbols && collidingSymbols.some(prevSymbol => prevSymbol !== symbol)) {
197+
const newName = getNewNameIfConflict(node, collidingSymbolMap);
197198
identsToRenameMap.set(symbolIdString, newName.identifier);
198199
synthNamesMap.set(symbolIdString, newName);
199200
allVarNames.push({ identifier: newName.identifier, symbol });
201+
addNameToFrequencyMap(collidingSymbolMap, originalName, symbol);
200202
}
201203
else {
202204
const identifier = getSynthesizedDeepClone(node);
203205
identsToRenameMap.set(symbolIdString, identifier);
204206
synthNamesMap.set(symbolIdString, { identifier, types: [], numberOfAssignmentsOriginal: allVarNames.filter(elem => elem.identifier.text === node.text).length/*, numberOfAssignmentsSynthesized: 0*/ });
205207
if ((isParameter(node.parent) && isExpressionOrCallOnTypePromise(node.parent.parent)) || isVariableDeclaration(node.parent)) {
206208
allVarNames.push({ identifier, symbol });
209+
addNameToFrequencyMap(collidingSymbolMap, originalName, symbol);
207210
}
208211
}
209212
}
@@ -246,8 +249,17 @@ namespace ts.codefix {
246249

247250
}
248251

249-
function getNewNameIfConflict(name: Identifier, allVarNames: SymbolAndIdentifier[]): SynthIdentifier {
250-
const numVarsSameName = allVarNames.filter(elem => elem.identifier.text === name.text).length;
252+
function addNameToFrequencyMap(renamedVarNameFrequencyMap: Map<Symbol[]>, originalName: string, symbol: Symbol) {
253+
if (renamedVarNameFrequencyMap.has(originalName)) {
254+
renamedVarNameFrequencyMap.get(originalName)!.push(symbol);
255+
}
256+
else {
257+
renamedVarNameFrequencyMap.set(originalName, [symbol]);
258+
}
259+
}
260+
261+
function getNewNameIfConflict(name: Identifier, originalNames: Map<Symbol[]>): SynthIdentifier {
262+
const numVarsSameName = (originalNames.get(name.text) || []).length;
251263
const numberOfAssignmentsOriginal = 0;
252264
const identifier = numVarsSameName === 0 ? name : createIdentifier(name.text + "_" + numVarsSameName);
253265
return { identifier, types: [], numberOfAssignmentsOriginal };
@@ -294,13 +306,14 @@ namespace ts.codefix {
294306
prevArgName.numberOfAssignmentsOriginal = 2; // Try block and catch block
295307
transformer.synthNamesMap.forEach((val, key) => {
296308
if (val.identifier.text === prevArgName.identifier.text) {
297-
transformer.synthNamesMap.set(key, getNewNameIfConflict(prevArgName.identifier, transformer.allVarNames));
309+
const newSynthName = createUniqueSynthName(prevArgName);
310+
transformer.synthNamesMap.set(key, newSynthName);
298311
}
299312
});
300313

301314
// update the constIdentifiers list
302315
if (transformer.constIdentifiers.some(elem => elem.text === prevArgName.identifier.text)) {
303-
transformer.constIdentifiers.push(getNewNameIfConflict(prevArgName.identifier, transformer.allVarNames).identifier);
316+
transformer.constIdentifiers.push(createUniqueSynthName(prevArgName).identifier);
304317
}
305318
}
306319

@@ -326,6 +339,12 @@ namespace ts.codefix {
326339
return varDeclList ? [varDeclList, tryStatement] : [tryStatement];
327340
}
328341

342+
function createUniqueSynthName(prevArgName: SynthIdentifier) {
343+
const renamedPrevArg = createOptimisticUniqueName(prevArgName.identifier.text);
344+
const newSynthName = { identifier: renamedPrevArg, types: [], numberOfAssignmentsOriginal: 0 };
345+
return newSynthName;
346+
}
347+
329348
function transformThen(node: CallExpression, transformer: Transformer, outermostParent: CallExpression, prevArgName?: SynthIdentifier): Statement[] {
330349
const [res, rej] = node.arguments;
331350

@@ -348,11 +367,8 @@ namespace ts.codefix {
348367

349368
return [createTry(tryBlock, catchClause, /* finallyBlock */ undefined) as Statement];
350369
}
351-
else {
352-
return transformExpression(node.expression, transformer, node, argNameRes).concat(transformationBody);
353-
}
354370

355-
return [];
371+
return transformExpression(node.expression, transformer, node, argNameRes).concat(transformationBody);
356372
}
357373

358374
function getFlagOfIdentifier(node: Identifier, constIdentifiers: Identifier[]): NodeFlags {
@@ -419,8 +435,13 @@ namespace ts.codefix {
419435
// Arrow functions with block bodies { } will enter this control flow
420436
if (isFunctionLikeDeclaration(func) && func.body && isBlock(func.body) && func.body.statements) {
421437
let refactoredStmts: Statement[] = [];
438+
let seenReturnStatement = false;
422439

423440
for (const statement of func.body.statements) {
441+
if (isReturnStatement(statement)) {
442+
seenReturnStatement = true;
443+
}
444+
424445
if (getReturnStatementsWithPromiseHandlers(statement).length) {
425446
refactoredStmts = refactoredStmts.concat(getInnerTransformationBody(transformer, [statement], prevArgName));
426447
}
@@ -430,7 +451,7 @@ namespace ts.codefix {
430451
}
431452

432453
return shouldReturn ? getSynthesizedDeepClones(createNodeArray(refactoredStmts)) :
433-
removeReturns(createNodeArray(refactoredStmts), prevArgName!.identifier, transformer.constIdentifiers);
454+
removeReturns(createNodeArray(refactoredStmts), prevArgName!.identifier, transformer.constIdentifiers, seenReturnStatement);
434455
}
435456
else {
436457
const funcBody = (<ArrowFunction>func).body;
@@ -443,7 +464,7 @@ namespace ts.codefix {
443464

444465
if (hasPrevArgName && !shouldReturn) {
445466
const type = transformer.checker.getTypeAtLocation(func);
446-
const returnType = getLastCallSignature(type, transformer.checker).getReturnType();
467+
const returnType = getLastCallSignature(type, transformer.checker)!.getReturnType();
447468
const varDeclOrAssignment = createVariableDeclarationOrAssignment(prevArgName!, getSynthesizedDeepClone(funcBody) as Expression, transformer);
448469
prevArgName!.types.push(returnType);
449470
return varDeclOrAssignment;
@@ -460,13 +481,13 @@ namespace ts.codefix {
460481
return createNodeArray([]);
461482
}
462483

463-
function getLastCallSignature(type: Type, checker: TypeChecker): Signature {
464-
const callSignatures = type && checker.getSignaturesOfType(type, SignatureKind.Call);
465-
return callSignatures && callSignatures[callSignatures.length - 1];
484+
function getLastCallSignature(type: Type, checker: TypeChecker): Signature | undefined {
485+
const callSignatures = checker.getSignaturesOfType(type, SignatureKind.Call);
486+
return lastOrUndefined(callSignatures);
466487
}
467488

468489

469-
function removeReturns(stmts: NodeArray<Statement>, prevArgName: Identifier, constIdentifiers: Identifier[]): NodeArray<Statement> {
490+
function removeReturns(stmts: NodeArray<Statement>, prevArgName: Identifier, constIdentifiers: Identifier[], seenReturnStatement: boolean): NodeArray<Statement> {
470491
const ret: Statement[] = [];
471492
for (const stmt of stmts) {
472493
if (isReturnStatement(stmt)) {
@@ -480,6 +501,12 @@ namespace ts.codefix {
480501
}
481502
}
482503

504+
// if block has no return statement, need to define prevArgName as undefined to prevent undeclared variables
505+
if (!seenReturnStatement) {
506+
ret.push(createVariableStatement(/*modifiers*/ undefined,
507+
(createVariableDeclarationList([createVariableDeclaration(prevArgName, /*type*/ undefined, createIdentifier("undefined"))], getFlagOfIdentifier(prevArgName, constIdentifiers)))));
508+
}
509+
483510
return createNodeArray(ret);
484511
}
485512

@@ -517,9 +544,6 @@ namespace ts.codefix {
517544
name = getMapEntryIfExists(param);
518545
}
519546
}
520-
else if (isCallExpression(funcNode) && funcNode.arguments.length > 0 && isIdentifier(funcNode.arguments[0])) {
521-
name = { identifier: funcNode.arguments[0] as Identifier, types, numberOfAssignmentsOriginal };
522-
}
523547
else if (isIdentifier(funcNode)) {
524548
name = getMapEntryIfExists(funcNode);
525549
}

‎src/services/suggestionDiagnostics.ts‎

Lines changed: 4 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -141,8 +141,8 @@ namespace ts {
141141
}
142142

143143
/** @internal */
144-
export function getReturnStatementsWithPromiseHandlers(node: Node): Node[] {
145-
const returnStatements: Node[] = [];
144+
export function getReturnStatementsWithPromiseHandlers(node: Node): ReturnStatement[] {
145+
const returnStatements: ReturnStatement[] = [];
146146
if (isFunctionLike(node)) {
147147
forEachChild(node, visit);
148148
}
@@ -155,14 +155,8 @@ namespace ts {
155155
return;
156156
}
157157

158-
if (isReturnStatement(child)) {
159-
forEachChild(child, addHandlers);
160-
}
161-
162-
function addHandlers(returnChild: Node) {
163-
if (isFixablePromiseHandler(returnChild)) {
164-
returnStatements.push(child as ReturnStatement);
165-
}
158+
if (isReturnStatement(child) && child.expression && isFixablePromiseHandler(child.expression)) {
159+
returnStatements.push(child);
166160
}
167161

168162
forEachChild(child, visit);

‎src/services/utilities.ts‎

Lines changed: 5 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1664,11 +1664,10 @@ namespace ts {
16641664
return clone;
16651665
}
16661666

1667-
export function getSynthesizedDeepCloneWithRenames<T extends Node | undefined>(node: T, includeTrivia = true, renameMap?: Map<Identifier>, checker?: TypeChecker, callback?: (originalNode: Node, clone: Node) => any): T {
1668-
1667+
export function getSynthesizedDeepCloneWithRenames<T extends Node>(node: T, includeTrivia = true, renameMap?: Map<Identifier>, checker?: TypeChecker, callback?: (originalNode: Node, clone: Node) => any): T {
16691668
let clone;
1670-
if (node && isIdentifier(node!) && renameMap && checker) {
1671-
const symbol = checker.getSymbolAtLocation(node!);
1669+
if (isIdentifier(node) && renameMap && checker) {
1670+
const symbol = checker.getSymbolAtLocation(node);
16721671
const renameInfo = symbol && renameMap.get(String(getSymbolId(symbol)));
16731672

16741673
if (renameInfo) {
@@ -1677,11 +1676,11 @@ namespace ts {
16771676
}
16781677

16791678
if (!clone) {
1680-
clone = node && getSynthesizedDeepCloneWorker(node as NonNullable<T>, renameMap, checker, callback);
1679+
clone = getSynthesizedDeepCloneWorker(node as NonNullable<T>, renameMap, checker, callback);
16811680
}
16821681

16831682
if (clone && !includeTrivia) suppressLeadingAndTrailingTrivia(clone);
1684-
if (callback && node) callback(node!, clone);
1683+
if (callback && clone) callback(node, clone);
16851684

16861685
return clone as T;
16871686
}

‎src/testRunner/unittests/convertToAsyncFunction.ts‎

Lines changed: 26 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -751,7 +751,7 @@ function [#|f|](): Promise<void> {
751751
}
752752
return x.then(resp => {
753753
var blob = resp.blob().then(blob => blob.byteOffset).catch(err => 'Error');
754-
return fetch("https://micorosft.com").then(res => console.log("Another one!"));
754+
return fetch("https://microsoft.com").then(res => console.log("Another one!"));
755755
});
756756
}
757757
`
@@ -1132,6 +1132,31 @@ function [#|f|]() {
11321132
const [#|foo|] = function () {
11331133
return fetch('https://typescriptlang.org').then(result => { console.log(result) });
11341134
}
1135+
`);
1136+
1137+
_testConvertToAsyncFunction("convertToAsyncFunction_catchBlockUniqueParams", `
1138+
function [#|f|]() {
1139+
return Promise.resolve().then(x => 1).catch(x => "a").then(x => !!x);
1140+
}
1141+
`);
1142+
1143+
_testConvertToAsyncFunction("convertToAsyncFunction_bindingPattern", `
1144+
function [#|f|]() {
1145+
return fetch('https://typescriptlang.org').then(res);
1146+
}
1147+
function res({ status, trailer }){
1148+
console.log(status);
1149+
}
1150+
`);
1151+
1152+
_testConvertToAsyncFunction("convertToAsyncFunction_bindingPatternNameCollision", `
1153+
function [#|f|]() {
1154+
const result = 'https://typescriptlang.org';
1155+
return fetch(result).then(res);
1156+
}
1157+
function res({ status, trailer }){
1158+
console.log(status);
1159+
}
11351160
`);
11361161

11371162
_testConvertToAsyncFunctionFailed("convertToAsyncFunction_thenArgumentNotFunction", `
@@ -1146,7 +1171,6 @@ function [#|f|]() {
11461171
}
11471172
`);
11481173

1149-
11501174
});
11511175

11521176
function _testConvertToAsyncFunction(caption: string, text: string) {

‎tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_InnerVarNameConflict.ts‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,5 +13,6 @@ function /*[#|*/f/*|]*/(): Promise<string> {
1313
async function f(): Promise<string> {
1414
const resp = await fetch("https://typescriptlang.org");
1515
var blob = resp.blob().then(blob_1 => blob_1.byteOffset).catch(err => 'Error');
16-
return blob_1.toString();
16+
const blob_2 = undefined;
17+
return blob_2.toString();
1718
}

‎tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_MultipleReturns2.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@ function /*[#|*/f/*|]*/(): Promise<void> {
77
}
88
return x.then(resp => {
99
var blob = resp.blob().then(blob => blob.byteOffset).catch(err => 'Error');
10-
return fetch("https://micorosft.com").then(res => console.log("Another one!"));
10+
return fetch("https://microsoft.com").then(res => console.log("Another one!"));
1111
});
1212
}
1313

@@ -21,6 +21,6 @@ async function f(): Promise<void> {
2121
}
2222
const resp = await x;
2323
var blob = resp.blob().then(blob_1 => blob_1.byteOffset).catch(err => 'Error');
24-
const res_1 = await fetch("https://micorosft.com");
24+
const res_2 = await fetch("https://microsoft.com");
2525
return console.log("Another one!");
2626
}

‎tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_bindingPattern.js‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,18 @@
1+
// ==ORIGINAL==
2+
3+
function /*[#|*/f/*|]*/() {
4+
return fetch('https://typescriptlang.org').then(res);
5+
}
6+
function res({ status, trailer }){
7+
console.log(status);
8+
}
9+
10+
// ==ASYNC FUNCTION::Convert to async function==
11+
12+
async function f() {
13+
const result = await fetch('https://typescriptlang.org');
14+
return res(result);
15+
}
16+
function res({ status, trailer }){
17+
console.log(status);
18+
}

‎tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_bindingPattern.ts‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,18 @@
1+
// ==ORIGINAL==
2+
3+
function /*[#|*/f/*|]*/() {
4+
return fetch('https://typescriptlang.org').then(res);
5+
}
6+
function res({ status, trailer }){
7+
console.log(status);
8+
}
9+
10+
// ==ASYNC FUNCTION::Convert to async function==
11+
12+
async function f() {
13+
const result = await fetch('https://typescriptlang.org');
14+
return res(result);
15+
}
16+
function res({ status, trailer }){
17+
console.log(status);
18+
}

‎tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_bindingPatternNameCollision.js‎

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,20 @@
1+
// ==ORIGINAL==
2+
3+
function /*[#|*/f/*|]*/() {
4+
const result = 'https://typescriptlang.org';
5+
return fetch(result).then(res);
6+
}
7+
function res({ status, trailer }){
8+
console.log(status);
9+
}
10+
11+
// ==ASYNC FUNCTION::Convert to async function==
12+
13+
async function f() {
14+
const result = 'https://typescriptlang.org';
15+
const result_1 = await fetch(result);
16+
return res(result_1);
17+
}
18+
function res({ status, trailer }){
19+
console.log(status);
20+
}

0 commit comments

Comments
 (0)