Skip to content

Commit e40ce24

Browse files
Merge pull request #27156 from uniqueiniquity/promisesAndUnderscores
Async code fix issues concerning underscores and nested promises
2 parents 83fe1ea + 0cb9fd6 commit e40ce24

19 files changed

+322
-53
lines changed

src/services/codefixes/convertToAsyncFunction.ts

Lines changed: 49 additions & 47 deletions
Original file line numberDiff line numberDiff line change
@@ -14,17 +14,10 @@ namespace ts.codefix {
1414
getAllCodeActions: context => codeFixAll(context, errorCodes, (changes, err) => convertToAsyncFunction(changes, err.file, err.start, context.program.getTypeChecker(), context)),
1515
});
1616

17-
18-
/*
19-
custom type to encapsulate information for variable declarations synthesized in the refactor
20-
numberOfUsesOriginal - number of times the variable should be assigned in the refactor
21-
numberOfUsesSynthesized - count of how many times the variable has been assigned so far
22-
At the end of the refactor, numberOfUsesOriginal should === numberOfUsesSynthesized
23-
*/
2417
interface SynthIdentifier {
2518
identifier: Identifier;
2619
types: Type[];
27-
numberOfAssignmentsOriginal: number;
20+
numberOfAssignmentsOriginal: number; // number of times the variable should be assigned in the refactor
2821
}
2922

3023
interface SymbolAndIdentifier {
@@ -179,9 +172,9 @@ namespace ts.codefix {
179172

180173
// 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))
181174
// Note - the choice of the last call signature is arbitrary
182-
if (lastCallSignature && lastCallSignature.parameters.length && !synthNamesMap.has(symbolIdString)) {
183-
const firstParameter = lastCallSignature.parameters[0];
184-
const ident = isParameter(firstParameter.valueDeclaration) && tryCast(firstParameter.valueDeclaration.name, isIdentifier) || createOptimisticUniqueName("result");
175+
if (lastCallSignature && !isFunctionLikeDeclaration(node.parent) && !synthNamesMap.has(symbolIdString)) {
176+
const firstParameter = firstOrUndefined(lastCallSignature.parameters);
177+
const ident = firstParameter && isParameter(firstParameter.valueDeclaration) && tryCast(firstParameter.valueDeclaration.name, isIdentifier) || createOptimisticUniqueName("result");
185178
const synthName = getNewNameIfConflict(ident, collidingSymbolMap);
186179
synthNamesMap.set(symbolIdString, synthName);
187180
allVarNames.push({ identifier: synthName.identifier, symbol });
@@ -234,9 +227,7 @@ namespace ts.codefix {
234227

235228
if (renameInfo) {
236229
const type = checker.getTypeAtLocation(node);
237-
if (type) {
238-
originalType.set(getNodeId(clone).toString(), type);
239-
}
230+
originalType.set(getNodeId(clone).toString(), type);
240231
}
241232
}
242233

@@ -320,7 +311,7 @@ namespace ts.codefix {
320311
const tryBlock = createBlock(transformExpression(node.expression, transformer, node, prevArgName));
321312

322313
const transformationBody = getTransformationBody(func, prevArgName, argName, node, transformer);
323-
const catchArg = argName.identifier.text.length > 0 ? argName.identifier.text : "e";
314+
const catchArg = argName ? argName.identifier.text : "e";
324315
const catchClause = createCatchClause(catchArg, createBlock(transformationBody));
325316

326317
/*
@@ -362,7 +353,7 @@ namespace ts.codefix {
362353

363354
const transformationBody2 = getTransformationBody(rej, prevArgName, argNameRej, node, transformer);
364355

365-
const catchArg = argNameRej.identifier.text.length > 0 ? argNameRej.identifier.text : "e";
356+
const catchArg = argNameRej ? argNameRej.identifier.text : "e";
366357
const catchClause = createCatchClause(catchArg, createBlock(transformationBody2));
367358

368359
return [createTry(tryBlock, catchClause, /* finallyBlock */ undefined) as Statement];
@@ -379,21 +370,25 @@ namespace ts.codefix {
379370
function transformPromiseCall(node: Expression, transformer: Transformer, prevArgName?: SynthIdentifier): Statement[] {
380371
const shouldReturn = transformer.setOfExpressionsToReturn.get(getNodeId(node).toString());
381372
// 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
382-
const hasPrevArgName = prevArgName && prevArgName.identifier.text.length > 0;
383373
const originalNodeParent = node.original ? node.original.parent : node.parent;
384-
if (hasPrevArgName && !shouldReturn && (!originalNodeParent || isPropertyAccessExpression(originalNodeParent))) {
385-
return createVariableDeclarationOrAssignment(prevArgName!, createAwait(node), transformer).concat(); // hack to make the types match
374+
if (prevArgName && !shouldReturn && (!originalNodeParent || isPropertyAccessExpression(originalNodeParent))) {
375+
return createTransformedStatement(prevArgName, createAwait(node), transformer).concat(); // hack to make the types match
386376
}
387-
else if (!hasPrevArgName && !shouldReturn && (!originalNodeParent || isPropertyAccessExpression(originalNodeParent))) {
377+
else if (!prevArgName && !shouldReturn && (!originalNodeParent || isPropertyAccessExpression(originalNodeParent))) {
388378
return [createStatement(createAwait(node))];
389379
}
390380

391381
return [createReturn(getSynthesizedDeepClone(node))];
392382
}
393383

394-
function createVariableDeclarationOrAssignment(prevArgName: SynthIdentifier, rightHandSide: Expression, transformer: Transformer): NodeArray<Statement> {
384+
function createTransformedStatement(prevArgName: SynthIdentifier | undefined, rightHandSide: Expression, transformer: Transformer): NodeArray<Statement> {
385+
if (!prevArgName || prevArgName.identifier.text.length === 0) {
386+
// if there's no argName to assign to, there still might be side effects
387+
return createNodeArray([createStatement(rightHandSide)]);
388+
}
395389

396390
if (prevArgName.types.length < prevArgName.numberOfAssignmentsOriginal) {
391+
// if the variable has already been declared, we don't need "let" or "const"
397392
return createNodeArray([createStatement(createAssignment(getSynthesizedDeepClone(prevArgName.identifier), rightHandSide))]);
398393
}
399394

@@ -402,31 +397,33 @@ namespace ts.codefix {
402397
}
403398

404399
// should be kept up to date with isFixablePromiseArgument in suggestionDiagnostics.ts
405-
function getTransformationBody(func: Expression, prevArgName: SynthIdentifier | undefined, argName: SynthIdentifier, parent: CallExpression, transformer: Transformer): NodeArray<Statement> {
400+
function getTransformationBody(func: Expression, prevArgName: SynthIdentifier | undefined, argName: SynthIdentifier | undefined, parent: CallExpression, transformer: Transformer): NodeArray<Statement> {
406401

407-
const hasPrevArgName = prevArgName && prevArgName.identifier.text.length > 0;
408-
const hasArgName = argName && argName.identifier.text.length > 0;
409402
const shouldReturn = transformer.setOfExpressionsToReturn.get(getNodeId(parent).toString());
410403
switch (func.kind) {
411404
case SyntaxKind.NullKeyword:
412405
// do not produce a transformed statement for a null argument
413406
break;
414-
case SyntaxKind.Identifier:
415-
// identifier includes undefined
416-
if (!hasArgName) break;
407+
case SyntaxKind.Identifier: // identifier includes undefined
408+
if (!argName) break;
417409

418-
const synthCall = createCall(getSynthesizedDeepClone(func) as Identifier, /*typeArguments*/ undefined, [argName.identifier]);
410+
const synthCall = createCall(getSynthesizedDeepClone(func) as Identifier, /*typeArguments*/ undefined, argName ? [argName.identifier] : []);
419411
if (shouldReturn) {
420412
return createNodeArray([createReturn(synthCall)]);
421413
}
422414

423-
if (!hasPrevArgName) break;
424-
425-
const type = transformer.originalTypeMap.get(getNodeId(func).toString());
426-
const callSignatures = type && transformer.checker.getSignaturesOfType(type, SignatureKind.Call);
427-
const returnType = callSignatures && callSignatures[0].getReturnType();
428-
const varDeclOrAssignment = createVariableDeclarationOrAssignment(prevArgName!, createAwait(synthCall), transformer);
429-
prevArgName!.types.push(returnType!);
415+
const type = transformer.originalTypeMap.get(getNodeId(func).toString()) || transformer.checker.getTypeAtLocation(func);
416+
const callSignatures = transformer.checker.getSignaturesOfType(type, SignatureKind.Call);
417+
if (!callSignatures.length) {
418+
// if identifier in handler has no call signatures, it's invalid
419+
codeActionSucceeded = false;
420+
break;
421+
}
422+
const returnType = callSignatures[0].getReturnType();
423+
const varDeclOrAssignment = createTransformedStatement(prevArgName, createAwait(synthCall), transformer);
424+
if (prevArgName) {
425+
prevArgName.types.push(returnType);
426+
}
430427
return varDeclOrAssignment;
431428

432429
case SyntaxKind.FunctionExpression:
@@ -451,7 +448,7 @@ namespace ts.codefix {
451448
}
452449

453450
return shouldReturn ? getSynthesizedDeepClones(createNodeArray(refactoredStmts)) :
454-
removeReturns(createNodeArray(refactoredStmts), prevArgName!.identifier, transformer.constIdentifiers, seenReturnStatement);
451+
removeReturns(createNodeArray(refactoredStmts), prevArgName!.identifier, transformer, seenReturnStatement);
455452
}
456453
else {
457454
const innerRetStmts = getReturnStatementsWithPromiseHandlers(createReturn(funcBody));
@@ -461,20 +458,24 @@ namespace ts.codefix {
461458
return createNodeArray(innerCbBody);
462459
}
463460

464-
if (hasPrevArgName && !shouldReturn) {
461+
if (!shouldReturn) {
465462
const type = transformer.checker.getTypeAtLocation(func);
466463
const returnType = getLastCallSignature(type, transformer.checker)!.getReturnType();
467-
const varDeclOrAssignment = createVariableDeclarationOrAssignment(prevArgName!, getSynthesizedDeepClone(funcBody), transformer);
468-
prevArgName!.types.push(returnType);
469-
return varDeclOrAssignment;
464+
const rightHandSide = getSynthesizedDeepClone(funcBody);
465+
const possiblyAwaitedRightHandSide = !!transformer.checker.getPromisedTypeOfPromise(returnType) ? createAwait(rightHandSide) : rightHandSide;
466+
const transformedStatement = createTransformedStatement(prevArgName, possiblyAwaitedRightHandSide, transformer);
467+
if (prevArgName) {
468+
prevArgName.types.push(returnType);
469+
}
470+
return transformedStatement;
470471
}
471472
else {
472473
return createNodeArray([createReturn(getSynthesizedDeepClone(funcBody))]);
473474
}
474475
}
475476
}
476477
default:
477-
// We've found a transformation body we don't know how to handle, so the refactoring should no-op to avoid deleting code.
478+
// If no cases apply, we've found a transformation body we don't know how to handle, so the refactoring should no-op to avoid deleting code.
478479
codeActionSucceeded = false;
479480
break;
480481
}
@@ -487,13 +488,14 @@ namespace ts.codefix {
487488
}
488489

489490

490-
function removeReturns(stmts: NodeArray<Statement>, prevArgName: Identifier, constIdentifiers: Identifier[], seenReturnStatement: boolean): NodeArray<Statement> {
491+
function removeReturns(stmts: NodeArray<Statement>, prevArgName: Identifier, transformer: Transformer, seenReturnStatement: boolean): NodeArray<Statement> {
491492
const ret: Statement[] = [];
492493
for (const stmt of stmts) {
493494
if (isReturnStatement(stmt)) {
494495
if (stmt.expression) {
496+
const possiblyAwaitedExpression = isPromiseReturningExpression(stmt.expression, transformer.checker) ? createAwait(stmt.expression) : stmt.expression;
495497
ret.push(createVariableStatement(/*modifiers*/ undefined,
496-
(createVariableDeclarationList([createVariableDeclaration(prevArgName, /*type*/ undefined, stmt.expression)], getFlagOfIdentifier(prevArgName, constIdentifiers)))));
498+
(createVariableDeclarationList([createVariableDeclaration(prevArgName, /*type*/ undefined, possiblyAwaitedExpression)], getFlagOfIdentifier(prevArgName, transformer.constIdentifiers)))));
497499
}
498500
}
499501
else {
@@ -504,7 +506,7 @@ namespace ts.codefix {
504506
// if block has no return statement, need to define prevArgName as undefined to prevent undeclared variables
505507
if (!seenReturnStatement) {
506508
ret.push(createVariableStatement(/*modifiers*/ undefined,
507-
(createVariableDeclarationList([createVariableDeclaration(prevArgName, /*type*/ undefined, createIdentifier("undefined"))], getFlagOfIdentifier(prevArgName, constIdentifiers)))));
509+
(createVariableDeclarationList([createVariableDeclaration(prevArgName, /*type*/ undefined, createIdentifier("undefined"))], getFlagOfIdentifier(prevArgName, transformer.constIdentifiers)))));
508510
}
509511

510512
return createNodeArray(ret);
@@ -531,7 +533,7 @@ namespace ts.codefix {
531533
return innerCbBody;
532534
}
533535

534-
function getArgName(funcNode: Node, transformer: Transformer): SynthIdentifier {
536+
function getArgName(funcNode: Node, transformer: Transformer): SynthIdentifier | undefined {
535537

536538
const numberOfAssignmentsOriginal = 0;
537539
const types: Type[] = [];
@@ -548,8 +550,8 @@ namespace ts.codefix {
548550
name = getMapEntryIfExists(funcNode);
549551
}
550552

551-
if (!name || name.identifier === undefined || name.identifier.text === "_" || name.identifier.text === "undefined") {
552-
return { identifier: createIdentifier(""), types, numberOfAssignmentsOriginal };
553+
if (!name || name.identifier === undefined || name.identifier.text === "undefined") {
554+
return undefined;
553555
}
554556

555557
return name;

src/testRunner/unittests/convertToAsyncFunction.ts

Lines changed: 54 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -416,6 +416,20 @@ function [#|f|](): Promise<void> {
416416
return fetch('https://typescriptlang.org').then( () => console.log("done") );
417417
}`
418418
);
419+
_testConvertToAsyncFunction("convertToAsyncFunction_IgnoreArgs3", `
420+
function [#|f|](): Promise<void> {
421+
return fetch('https://typescriptlang.org').then( () => console.log("almost done") ).then( () => console.log("done") );
422+
}`
423+
);
424+
_testConvertToAsyncFunction("convertToAsyncFunction_IgnoreArgs4", `
425+
function [#|f|]() {
426+
return fetch('https://typescriptlang.org').then(res);
427+
}
428+
function res(){
429+
console.log("done");
430+
}`
431+
);
432+
419433
_testConvertToAsyncFunction("convertToAsyncFunction_Method", `
420434
class Parser {
421435
[#|f|]():Promise<void> {
@@ -817,7 +831,7 @@ function [#|f|]() {
817831
);
818832
_testConvertToAsyncFunction("convertToAsyncFunction_Scope1", `
819833
function [#|f|]() {
820-
var var1:Promise<Response>, var2;
834+
var var1: Response, var2;
821835
return fetch('https://typescriptlang.org').then( _ =>
822836
Promise.resolve().then( res => {
823837
var2 = "test";
@@ -1183,6 +1197,45 @@ function [#|f|]() {
11831197
}
11841198
`);
11851199

1200+
_testConvertToAsyncFunction("convertToAsyncFunction_runEffectfulContinuation", `
1201+
function [#|f|]() {
1202+
return fetch('https://typescriptlang.org').then(res).then(_ => console.log("done"));
1203+
}
1204+
function res(result) {
1205+
return Promise.resolve().then(x => console.log(result));
1206+
}
1207+
`);
1208+
1209+
_testConvertToAsyncFunction("convertToAsyncFunction_callbackReturnsPromise", `
1210+
function [#|f|]() {
1211+
return fetch('https://typescriptlang.org').then(s => Promise.resolve(s.statusText.length)).then(x => console.log(x + 5));
1212+
}
1213+
`);
1214+
1215+
_testConvertToAsyncFunction("convertToAsyncFunction_callbackReturnsPromiseInBlock", `
1216+
function [#|f|]() {
1217+
return fetch('https://typescriptlang.org').then(s => { return Promise.resolve(s.statusText.length) }).then(x => x + 5);
1218+
}
1219+
`);
1220+
1221+
_testConvertToAsyncFunction("convertToAsyncFunction_callbackReturnsFixablePromise", `
1222+
function [#|f|]() {
1223+
return fetch('https://typescriptlang.org').then(s => Promise.resolve(s.statusText).then(st => st.length)).then(x => console.log(x + 5));
1224+
}
1225+
`);
1226+
1227+
_testConvertToAsyncFunction("convertToAsyncFunction_callbackReturnsPromiseLastInChain", `
1228+
function [#|f|]() {
1229+
return fetch('https://typescriptlang.org').then(s => Promise.resolve(s.statusText.length));
1230+
}
1231+
`);
1232+
1233+
1234+
_testConvertToAsyncFunction("convertToAsyncFunction_nestedPromises", `
1235+
function [#|f|]() {
1236+
return fetch('https://typescriptlang.org').then(x => Promise.resolve(3).then(y => Promise.resolve(x.statusText.length + y)));
1237+
}
1238+
`);
11861239
});
11871240

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

tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_IgnoreArgs1.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,6 @@ function /*[#|*/f/*|]*/(): Promise<void> {
66
// ==ASYNC FUNCTION::Convert to async function==
77

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

tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_Scope1.ts

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
// ==ORIGINAL==
22

33
function /*[#|*/f/*|]*/() {
4-
var var1:Promise<Response>, var2;
4+
var var1: Response, var2;
55
return fetch('https://typescriptlang.org').then( _ =>
66
Promise.resolve().then( res => {
77
var2 = "test";
@@ -18,11 +18,11 @@ function /*[#|*/f/*|]*/() {
1818
// ==ASYNC FUNCTION::Convert to async function==
1919

2020
async function f() {
21-
var var1:Promise<Response>, var2;
22-
await fetch('https://typescriptlang.org');
21+
var var1: Response, var2;
22+
const _ = await fetch('https://typescriptlang.org');
2323
const res = await Promise.resolve();
2424
var2 = "test";
25-
const res_1 = fetch("https://microsoft.com");
25+
const res_1 = await fetch("https://microsoft.com");
2626
const response = var1 === res_1;
2727
return res(response);
2828
}

0 commit comments

Comments
 (0)