Skip to content

Commit 2dd6e20

Browse files
author
Benjamin Lichtman
committed
Only provide suggestion for outermost async fix
1 parent 4a664d6 commit 2dd6e20

22 files changed

+148
-67
lines changed

‎src/services/codefixes/convertToAsyncFunction.ts‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -60,7 +60,7 @@ namespace ts.codefix {
6060
const setOfExpressionsToReturn = getAllPromiseExpressionsToReturn(functionToConvert, checker);
6161
const functionToConvertRenamed = renameCollidingVarNames(functionToConvert, checker, synthNamesMap, context, setOfExpressionsToReturn, originalTypeMap, allVarNames);
6262
const constIdentifiers = getConstIdentifiers(synthNamesMap);
63-
const returnStatements = functionToConvertRenamed.body && isBlock(functionToConvertRenamed.body) ? getReturnStatementsWithPromiseHandlers(functionToConvertRenamed.body) : emptyArray;
63+
const returnStatements = functionToConvertRenamed.body && isBlock(functionToConvertRenamed.body) ? getReturnStatementsWithPromiseHandlers(functionToConvertRenamed.body, checker) : emptyArray;
6464
const transformer: Transformer = { checker, synthNamesMap, allVarNames, setOfExpressionsToReturn, constIdentifiers, originalTypeMap, isInJSFile: isInJavascript };
6565

6666
if (!returnStatements.length) {
@@ -87,10 +87,10 @@ namespace ts.codefix {
8787
}
8888
}
8989

90-
function getReturnStatementsWithPromiseHandlers(body: Block): ReadonlyArray<ReturnStatement> {
90+
function getReturnStatementsWithPromiseHandlers(body: Block, checker: TypeChecker): ReadonlyArray<ReturnStatement> {
9191
const res: ReturnStatement[] = [];
9292
forEachReturnStatement(body, ret => {
93-
if (isReturnStatementWithFixablePromiseHandler(ret)) res.push(ret);
93+
if (isReturnStatementWithFixablePromiseHandler(ret, checker)) res.push(ret);
9494
});
9595
return res;
9696
}
@@ -450,7 +450,7 @@ namespace ts.codefix {
450450
seenReturnStatement = true;
451451
}
452452

453-
if (isReturnStatementWithFixablePromiseHandler(statement)) {
453+
if (isReturnStatementWithFixablePromiseHandler(statement, transformer.checker)) {
454454
refactoredStmts = refactoredStmts.concat(getInnerTransformationBody(transformer, [statement], prevArgName));
455455
}
456456
else {
@@ -466,7 +466,7 @@ namespace ts.codefix {
466466
seenReturnStatement);
467467
}
468468
else {
469-
const innerRetStmts = isFixablePromiseHandler(funcBody) ? [createReturn(funcBody)] : emptyArray;
469+
const innerRetStmts = isFixablePromiseHandler(funcBody, transformer.checker) ? [createReturn(funcBody)] : emptyArray;
470470
const innerCbBody = getInnerTransformationBody(transformer, innerRetStmts, prevArgName);
471471

472472
if (innerCbBody.length > 0) {

‎src/services/suggestionDiagnostics.ts‎

Lines changed: 30 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,7 @@
11
/* @internal */
22
namespace ts {
3+
const visitedNestedConvertibleFunctions = createMap<true>();
4+
35
export function computeSuggestionDiagnostics(sourceFile: SourceFile, program: Program, cancellationToken: CancellationToken): DiagnosticWithLocation[] {
46
program.getSemanticDiagnostics(sourceFile, cancellationToken);
57
const diags: DiagnosticWithLocation[] = [];
@@ -13,6 +15,7 @@ namespace ts {
1315

1416
const isJsFile = isSourceFileJS(sourceFile);
1517

18+
visitedNestedConvertibleFunctions.clear();
1619
check(sourceFile);
1720

1821
if (getAllowSyntheticDefaultImports(program.getCompilerOptions())) {
@@ -114,17 +117,21 @@ namespace ts {
114117
}
115118

116119
function addConvertToAsyncFunctionDiagnostics(node: FunctionLikeDeclaration, checker: TypeChecker, diags: Push<DiagnosticWithLocation>): void {
117-
if (!isAsyncFunction(node) &&
118-
node.body &&
119-
isBlock(node.body) &&
120-
hasReturnStatementWithPromiseHandler(node.body) &&
121-
returnsPromise(node, checker)) {
120+
if (!visitedNestedConvertibleFunctions.has(getKeyFromNode(node)) && isConvertableFunction(node, checker)) {
122121
diags.push(createDiagnosticForNode(
123122
!node.name && isVariableDeclaration(node.parent) && isIdentifier(node.parent.name) ? node.parent.name : node,
124123
Diagnostics.This_may_be_converted_to_an_async_function));
125124
}
126125
}
127126

127+
function isConvertableFunction(node: FunctionLikeDeclaration, checker: TypeChecker) {
128+
return !isAsyncFunction(node) &&
129+
node.body &&
130+
isBlock(node.body) &&
131+
hasReturnStatementWithPromiseHandler(node.body, checker) &&
132+
returnsPromise(node, checker);
133+
}
134+
128135
function returnsPromise(node: FunctionLikeDeclaration, checker: TypeChecker): boolean {
129136
const functionType = checker.getTypeAtLocation(node);
130137
const callSignatures = checker.getSignaturesOfType(functionType, SignatureKind.Call);
@@ -136,25 +143,25 @@ namespace ts {
136143
return isBinaryExpression(commonJsModuleIndicator) ? commonJsModuleIndicator.left : commonJsModuleIndicator;
137144
}
138145

139-
function hasReturnStatementWithPromiseHandler(body: Block): boolean {
140-
return !!forEachReturnStatement(body, isReturnStatementWithFixablePromiseHandler);
146+
function hasReturnStatementWithPromiseHandler(body: Block, checker: TypeChecker): boolean {
147+
return !!forEachReturnStatement(body, stmt => isReturnStatementWithFixablePromiseHandler(stmt, checker));
141148
}
142149

143-
export function isReturnStatementWithFixablePromiseHandler(node: Node): node is ReturnStatement {
144-
return isReturnStatement(node) && !!node.expression && isFixablePromiseHandler(node.expression);
150+
export function isReturnStatementWithFixablePromiseHandler(node: Node, checker: TypeChecker): node is ReturnStatement {
151+
return isReturnStatement(node) && !!node.expression && isFixablePromiseHandler(node.expression, checker);
145152
}
146153

147154
// Should be kept up to date with transformExpression in convertToAsyncFunction.ts
148-
export function isFixablePromiseHandler(node: Node): boolean {
155+
export function isFixablePromiseHandler(node: Node, checker: TypeChecker): boolean {
149156
// ensure outermost call exists and is a promise handler
150-
if (!isPromiseHandler(node) || !node.arguments.every(isFixablePromiseArgument)) {
157+
if (!isPromiseHandler(node) || !node.arguments.every((arg) => isFixablePromiseArgument(arg, checker))) {
151158
return false;
152159
}
153160

154161
// ensure all chained calls are valid
155162
let currentNode = node.expression;
156163
while (isPromiseHandler(currentNode) || isPropertyAccessExpression(currentNode)) {
157-
if (isCallExpression(currentNode) && !currentNode.arguments.every(isFixablePromiseArgument)) {
164+
if (isCallExpression(currentNode) && !currentNode.arguments.every(arg => isFixablePromiseArgument(arg, checker))) {
158165
return false;
159166
}
160167
currentNode = currentNode.expression;
@@ -167,16 +174,24 @@ namespace ts {
167174
}
168175

169176
// should be kept up to date with getTransformationBody in convertToAsyncFunction.ts
170-
function isFixablePromiseArgument(arg: Expression): boolean {
177+
function isFixablePromiseArgument(arg: Expression, checker: TypeChecker): boolean {
171178
switch (arg.kind) {
172-
case SyntaxKind.NullKeyword:
173-
case SyntaxKind.Identifier: // identifier includes undefined
174179
case SyntaxKind.FunctionDeclaration:
175180
case SyntaxKind.FunctionExpression:
176181
case SyntaxKind.ArrowFunction:
182+
if (isConvertableFunction(arg as FunctionLikeDeclaration, checker)) {
183+
visitedNestedConvertibleFunctions.set(getKeyFromNode(arg as FunctionLikeDeclaration), true);
184+
}
185+
/* falls through */
186+
case SyntaxKind.NullKeyword:
187+
case SyntaxKind.Identifier: // identifier includes undefined
177188
return true;
178189
default:
179190
return false;
180191
}
181192
}
193+
194+
function getKeyFromNode(exp: FunctionLikeDeclaration) {
195+
return `${exp.pos.toString()}:${exp.end.toString()}`;
196+
}
182197
}

‎src/testRunner/unittests/services/convertToAsyncFunction.ts‎

Lines changed: 31 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -255,7 +255,7 @@ interface String { charAt: any; }
255255
interface Array<T> {}`
256256
};
257257

258-
function testConvertToAsyncFunction(caption: string, text: string, baselineFolder: string, includeLib?: boolean, expectFailure = false) {
258+
function testConvertToAsyncFunction(caption: string, text: string, baselineFolder: string, includeLib?: boolean, expectFailure = false, onlyProvideAction = false) {
259259
const t = extractTest(text);
260260
const selectionRange = t.ranges.get("selection")!;
261261
if (!selectionRange) {
@@ -307,7 +307,7 @@ interface Array<T> {}`
307307

308308
const actions = codefix.getFixes(context);
309309
const action = find(actions, action => action.description === Diagnostics.Convert_to_async_function.message);
310-
if (expectFailure) {
310+
if (expectFailure && !onlyProvideAction) {
311311
assert.isNotTrue(action && action.changes.length > 0);
312312
return;
313313
}
@@ -1151,25 +1151,25 @@ function [#|f|]() {
11511151
_testConvertToAsyncFunction("convertToAsyncFunction_simpleFunctionExpression", `
11521152
const [#|foo|] = function () {
11531153
return fetch('https://typescriptlang.org').then(result => { console.log(result) });
1154-
}
1154+
}
11551155
`);
11561156

11571157
_testConvertToAsyncFunction("convertToAsyncFunction_simpleFunctionExpressionWithName", `
11581158
const foo = function [#|f|]() {
11591159
return fetch('https://typescriptlang.org').then(result => { console.log(result) });
1160-
}
1160+
}
11611161
`);
11621162

11631163
_testConvertToAsyncFunction("convertToAsyncFunction_simpleFunctionExpressionAssignedToBindingPattern", `
11641164
const { length } = [#|function|] () {
11651165
return fetch('https://typescriptlang.org').then(result => { console.log(result) });
1166-
}
1166+
}
11671167
`);
11681168

11691169
_testConvertToAsyncFunction("convertToAsyncFunction_catchBlockUniqueParams", `
11701170
function [#|f|]() {
1171-
return Promise.resolve().then(x => 1).catch(x => "a").then(x => !!x);
1172-
}
1171+
return Promise.resolve().then(x => 1).catch(x => "a").then(x => !!x);
1172+
}
11731173
`);
11741174

11751175
_testConvertToAsyncFunction("convertToAsyncFunction_bindingPattern", `
@@ -1178,7 +1178,7 @@ function [#|f|]() {
11781178
}
11791179
function res({ status, trailer }){
11801180
console.log(status);
1181-
}
1181+
}
11821182
`);
11831183

11841184
_testConvertToAsyncFunction("convertToAsyncFunction_bindingPatternNameCollision", `
@@ -1188,7 +1188,7 @@ function [#|f|]() {
11881188
}
11891189
function res({ status, trailer }){
11901190
console.log(status);
1191-
}
1191+
}
11921192
`);
11931193

11941194
_testConvertToAsyncFunctionFailed("convertToAsyncFunction_thenArgumentNotFunction", `
@@ -1209,7 +1209,7 @@ function [#|f|]() {
12091209
}
12101210
function res(result) {
12111211
return Promise.resolve().then(x => console.log(result));
1212-
}
1212+
}
12131213
`);
12141214

12151215
_testConvertToAsyncFunction("convertToAsyncFunction_callbackReturnsPromise", `
@@ -1241,7 +1241,7 @@ function [#|f|]() {
12411241
return Promise.resolve(1)
12421242
.then(x => Promise.reject(x))
12431243
.catch(err => console.log(err));
1244-
}
1244+
}
12451245
`);
12461246

12471247
_testConvertToAsyncFunction("convertToAsyncFunction_nestedPromises", `
@@ -1266,6 +1266,22 @@ _testConvertToAsyncFunction("convertToAsyncFunction_exportModifier", `
12661266
export function [#|foo|]() {
12671267
return fetch('https://typescriptlang.org').then(s => console.log(s));
12681268
}
1269+
`);
1270+
1271+
_testConvertToAsyncFunction("convertToAsyncFunction_OutermostOnlySuccess", `
1272+
function [#|foo|]() {
1273+
return fetch('a').then(() => {
1274+
return fetch('b').then(() => 'c');
1275+
})
1276+
}
1277+
`);
1278+
1279+
_testConvertToAsyncFunctionFailedSuggestion("convertToAsyncFunction_OutermostOnlyFailure", `
1280+
function foo() {
1281+
return fetch('a').then([#|() => {|]
1282+
return fetch('b').then(() => 'c');
1283+
})
1284+
}
12691285
`);
12701286
});
12711287

@@ -1276,4 +1292,8 @@ export function [#|foo|]() {
12761292
function _testConvertToAsyncFunctionFailed(caption: string, text: string) {
12771293
testConvertToAsyncFunction(caption, text, "convertToAsyncFunction", /*includeLib*/ true, /*expectFailure*/ true);
12781294
}
1295+
1296+
function _testConvertToAsyncFunctionFailedSuggestion(caption: string, text: string) {
1297+
testConvertToAsyncFunction(caption, text, "convertToAsyncFunction", /*includeLib*/ true, /*expectFailure*/ true, /*onlyProvideAction*/ true);
1298+
}
12791299
}

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

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,16 @@
1+
// ==ORIGINAL==
2+
3+
function foo() {
4+
return fetch('a').then(/*[#|*/() => {/*|]*/
5+
return fetch('b').then(() => 'c');
6+
})
7+
}
8+
9+
// ==ASYNC FUNCTION::Convert to async function==
10+
11+
function foo() {
12+
return fetch('a').then(async () => {
13+
await fetch('b');
14+
return 'c';
15+
})
16+
}

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

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
1+
// ==ORIGINAL==
2+
3+
function /*[#|*/foo/*|]*/() {
4+
return fetch('a').then(() => {
5+
return fetch('b').then(() => 'c');
6+
})
7+
}
8+
9+
// ==ASYNC FUNCTION::Convert to async function==
10+
11+
async function foo() {
12+
await fetch('a');
13+
await fetch('b');
14+
return 'c';
15+
}

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

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
1+
// ==ORIGINAL==
2+
3+
function /*[#|*/foo/*|]*/() {
4+
return fetch('a').then(() => {
5+
return fetch('b').then(() => 'c');
6+
})
7+
}
8+
9+
// ==ASYNC FUNCTION::Convert to async function==
10+
11+
async function foo() {
12+
await fetch('a');
13+
await fetch('b');
14+
return 'c';
15+
}

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@ function /*[#|*/f/*|]*/() {
55
}
66
function res({ status, trailer }){
77
console.log(status);
8-
}
8+
}
99

1010
// ==ASYNC FUNCTION::Convert to async function==
1111

@@ -15,4 +15,4 @@ async function f() {
1515
}
1616
function res({ status, trailer }){
1717
console.log(status);
18-
}
18+
}

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@ function /*[#|*/f/*|]*/() {
55
}
66
function res({ status, trailer }){
77
console.log(status);
8-
}
8+
}
99

1010
// ==ASYNC FUNCTION::Convert to async function==
1111

@@ -15,4 +15,4 @@ async function f() {
1515
}
1616
function res({ status, trailer }){
1717
console.log(status);
18-
}
18+
}

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@ function /*[#|*/f/*|]*/() {
66
}
77
function res({ status, trailer }){
88
console.log(status);
9-
}
9+
}
1010

1111
// ==ASYNC FUNCTION::Convert to async function==
1212

@@ -17,4 +17,4 @@ async function f() {
1717
}
1818
function res({ status, trailer }){
1919
console.log(status);
20-
}
20+
}

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@ function /*[#|*/f/*|]*/() {
66
}
77
function res({ status, trailer }){
88
console.log(status);
9-
}
9+
}
1010

1111
// ==ASYNC FUNCTION::Convert to async function==
1212

@@ -17,4 +17,4 @@ async function f() {
1717
}
1818
function res({ status, trailer }){
1919
console.log(status);
20-
}
20+
}

0 commit comments

Comments
 (0)