Skip to content

Commit 6f7f3b1

Browse files
authored
Minor fixes to "Convert To Async" refactor (microsoft#45536)
* Minor fixes to convertToAsync * Back out on nested return in inner continuation * Baseline update * Verify type argument for call can be used, add a few more early exit shortcuts
1 parent f9a3d85 commit 6f7f3b1

File tree

43 files changed

+673
-255
lines changed

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

43 files changed

+673
-255
lines changed

src/compiler/checker.ts

+2
Original file line numberDiff line numberDiff line change
@@ -634,6 +634,8 @@ namespace ts {
634634
getESSymbolType: () => esSymbolType,
635635
getNeverType: () => neverType,
636636
getOptionalType: () => optionalType,
637+
getPromiseType: () => getGlobalPromiseType(/*reportErrors*/ false),
638+
getPromiseLikeType: () => getGlobalPromiseLikeType(/*reportErrors*/ false),
637639
isSymbolAccessible,
638640
isArrayType,
639641
isTupleType,

src/compiler/factory/nodeFactory.ts

+9-7
Original file line numberDiff line numberDiff line change
@@ -4939,14 +4939,16 @@ namespace ts {
49394939
}
49404940

49414941
// @api
4942-
function createCatchClause(variableDeclaration: string | VariableDeclaration | undefined, block: Block) {
4942+
function createCatchClause(variableDeclaration: string | BindingName | VariableDeclaration | undefined, block: Block) {
49434943
const node = createBaseNode<CatchClause>(SyntaxKind.CatchClause);
4944-
variableDeclaration = !isString(variableDeclaration) ? variableDeclaration : createVariableDeclaration(
4945-
variableDeclaration,
4946-
/*exclamationToken*/ undefined,
4947-
/*type*/ undefined,
4948-
/*initializer*/ undefined
4949-
);
4944+
if (typeof variableDeclaration === "string" || variableDeclaration && !isVariableDeclaration(variableDeclaration)) {
4945+
variableDeclaration = createVariableDeclaration(
4946+
variableDeclaration,
4947+
/*exclamationToken*/ undefined,
4948+
/*type*/ undefined,
4949+
/*initializer*/ undefined
4950+
);
4951+
}
49504952
node.variableDeclaration = variableDeclaration;
49514953
node.block = block;
49524954
node.transformFlags |=

src/compiler/types.ts

+3-1
Original file line numberDiff line numberDiff line change
@@ -4269,6 +4269,8 @@ namespace ts {
42694269
/* @internal */ createArrayType(elementType: Type): Type;
42704270
/* @internal */ getElementTypeOfArrayType(arrayType: Type): Type | undefined;
42714271
/* @internal */ createPromiseType(type: Type): Type;
4272+
/* @internal */ getPromiseType(): Type;
4273+
/* @internal */ getPromiseLikeType(): Type;
42724274

42734275
/* @internal */ isTypeAssignableTo(source: Type, target: Type): boolean;
42744276
/* @internal */ createAnonymousType(symbol: Symbol | undefined, members: SymbolTable, callSignatures: Signature[], constructSignatures: Signature[], indexInfos: IndexInfo[]): Type;
@@ -7454,7 +7456,7 @@ namespace ts {
74547456
updateDefaultClause(node: DefaultClause, statements: readonly Statement[]): DefaultClause;
74557457
createHeritageClause(token: HeritageClause["token"], types: readonly ExpressionWithTypeArguments[]): HeritageClause;
74567458
updateHeritageClause(node: HeritageClause, types: readonly ExpressionWithTypeArguments[]): HeritageClause;
7457-
createCatchClause(variableDeclaration: string | VariableDeclaration | undefined, block: Block): CatchClause;
7459+
createCatchClause(variableDeclaration: string | BindingName | VariableDeclaration | undefined, block: Block): CatchClause;
74587460
updateCatchClause(node: CatchClause, variableDeclaration: VariableDeclaration | undefined, block: Block): CatchClause;
74597461

74607462
//

src/compiler/utilities.ts

+1-1
Original file line numberDiff line numberDiff line change
@@ -1364,7 +1364,7 @@ namespace ts {
13641364

13651365
// Warning: This has the same semantics as the forEach family of functions,
13661366
// in that traversal terminates in the event that 'visitor' supplies a truthy value.
1367-
export function forEachReturnStatement<T>(body: Block, visitor: (stmt: ReturnStatement) => T): T | undefined {
1367+
export function forEachReturnStatement<T>(body: Block | Statement, visitor: (stmt: ReturnStatement) => T): T | undefined {
13681368

13691369
return traverse(body);
13701370

src/services/codefixes/convertToAsyncFunction.ts

+325-100
Large diffs are not rendered by default.

src/services/suggestionDiagnostics.ts

+23-16
Original file line numberDiff line numberDiff line change
@@ -139,33 +139,40 @@ namespace ts {
139139
// Should be kept up to date with transformExpression in convertToAsyncFunction.ts
140140
export function isFixablePromiseHandler(node: Node, checker: TypeChecker): boolean {
141141
// ensure outermost call exists and is a promise handler
142-
if (!isPromiseHandler(node) || !node.arguments.every(arg => isFixablePromiseArgument(arg, checker))) {
142+
if (!isPromiseHandler(node) || !hasSupportedNumberOfArguments(node) || !node.arguments.every(arg => isFixablePromiseArgument(arg, checker))) {
143143
return false;
144144
}
145145

146146
// ensure all chained calls are valid
147-
let currentNode = node.expression;
147+
let currentNode = node.expression.expression;
148148
while (isPromiseHandler(currentNode) || isPropertyAccessExpression(currentNode)) {
149-
if (isCallExpression(currentNode) && !currentNode.arguments.every(arg => isFixablePromiseArgument(arg, checker))) {
150-
return false;
149+
if (isCallExpression(currentNode)) {
150+
if (!hasSupportedNumberOfArguments(currentNode) || !currentNode.arguments.every(arg => isFixablePromiseArgument(arg, checker))) {
151+
return false;
152+
}
153+
currentNode = currentNode.expression.expression;
154+
}
155+
else {
156+
currentNode = currentNode.expression;
151157
}
152-
currentNode = currentNode.expression;
153158
}
154159
return true;
155160
}
156161

157-
function isPromiseHandler(node: Node): node is CallExpression {
162+
function isPromiseHandler(node: Node): node is CallExpression & { readonly expression: PropertyAccessExpression } {
158163
return isCallExpression(node) && (
159-
hasPropertyAccessExpressionWithName(node, "then") && hasSupportedNumberOfArguments(node) ||
160-
hasPropertyAccessExpressionWithName(node, "catch"));
161-
}
162-
163-
function hasSupportedNumberOfArguments(node: CallExpression) {
164-
if (node.arguments.length > 2) return false;
165-
if (node.arguments.length < 2) return true;
166-
return some(node.arguments, arg => {
167-
return arg.kind === SyntaxKind.NullKeyword ||
168-
isIdentifier(arg) && arg.text === "undefined";
164+
hasPropertyAccessExpressionWithName(node, "then") ||
165+
hasPropertyAccessExpressionWithName(node, "catch") ||
166+
hasPropertyAccessExpressionWithName(node, "finally"));
167+
}
168+
169+
function hasSupportedNumberOfArguments(node: CallExpression & { readonly expression: PropertyAccessExpression }) {
170+
const name = node.expression.name.text;
171+
const maxArguments = name === "then" ? 2 : name === "catch" ? 1 : name === "finally" ? 1 : 0;
172+
if (node.arguments.length > maxArguments) return false;
173+
if (node.arguments.length < maxArguments) return true;
174+
return maxArguments === 1 || some(node.arguments, arg => {
175+
return arg.kind === SyntaxKind.NullKeyword || isIdentifier(arg) && arg.text === "undefined";
169176
});
170177
}
171178

src/testRunner/unittests/services/convertToAsyncFunction.ts

+147-30
Original file line numberDiff line numberDiff line change
@@ -53,6 +53,13 @@ interface Promise<T> {
5353
* @returns A Promise for the completion of the callback.
5454
*/
5555
catch<TResult = never>(onrejected?: ((reason: any) => TResult | PromiseLike<TResult>) | undefined | null): Promise<T | TResult>;
56+
/**
57+
* Attaches a callback that is invoked when the Promise is settled (fulfilled or rejected). The
58+
* resolved value cannot be modified from the callback.
59+
* @param onfinally The callback to execute when the Promise is settled (fulfilled or rejected).
60+
* @returns A Promise for the completion of the callback.
61+
*/
62+
finally(onfinally?: (() => void) | undefined | null): Promise<T>
5663
}
5764
interface PromiseConstructor {
5865
/**
@@ -277,7 +284,30 @@ interface Array<T> {}`
277284
}
278285
}
279286

280-
function testConvertToAsyncFunction(it: Mocha.PendingTestFunction, caption: string, text: string, baselineFolder: string, includeLib?: boolean, includeModule?: boolean, expectFailure = false, onlyProvideAction = false) {
287+
const enum ConvertToAsyncTestFlags {
288+
None,
289+
IncludeLib = 1 << 0,
290+
IncludeModule = 1 << 1,
291+
ExpectSuggestionDiagnostic = 1 << 2,
292+
ExpectNoSuggestionDiagnostic = 1 << 3,
293+
ExpectAction = 1 << 4,
294+
ExpectNoAction = 1 << 5,
295+
296+
ExpectSuccess = ExpectSuggestionDiagnostic | ExpectAction,
297+
ExpectFailed = ExpectNoSuggestionDiagnostic | ExpectNoAction,
298+
}
299+
300+
function testConvertToAsyncFunction(it: Mocha.PendingTestFunction, caption: string, text: string, baselineFolder: string, flags: ConvertToAsyncTestFlags) {
301+
const includeLib = !!(flags & ConvertToAsyncTestFlags.IncludeLib);
302+
const includeModule = !!(flags & ConvertToAsyncTestFlags.IncludeModule);
303+
const expectSuggestionDiagnostic = !!(flags & ConvertToAsyncTestFlags.ExpectSuggestionDiagnostic);
304+
const expectNoSuggestionDiagnostic = !!(flags & ConvertToAsyncTestFlags.ExpectNoSuggestionDiagnostic);
305+
const expectAction = !!(flags & ConvertToAsyncTestFlags.ExpectAction);
306+
const expectNoAction = !!(flags & ConvertToAsyncTestFlags.ExpectNoAction);
307+
const expectFailure = expectNoSuggestionDiagnostic || expectNoAction;
308+
Debug.assert(!(expectSuggestionDiagnostic && expectNoSuggestionDiagnostic), "Cannot combine both 'ExpectSuggestionDiagnostic' and 'ExpectNoSuggestionDiagnostic'");
309+
Debug.assert(!(expectAction && expectNoAction), "Cannot combine both 'ExpectAction' and 'ExpectNoAction'");
310+
281311
const t = extractTest(text);
282312
const selectionRange = t.ranges.get("selection")!;
283313
if (!selectionRange) {
@@ -320,35 +350,47 @@ interface Array<T> {}`
320350
const diagnostics = languageService.getSuggestionDiagnostics(f.path);
321351
const diagnostic = find(diagnostics, diagnostic => diagnostic.messageText === Diagnostics.This_may_be_converted_to_an_async_function.message &&
322352
diagnostic.start === context.span.start && diagnostic.length === context.span.length);
323-
if (expectFailure) {
324-
assert.isUndefined(diagnostic);
325-
}
326-
else {
327-
assert.exists(diagnostic);
328-
}
329-
330353
const actions = codefix.getFixes(context);
331354
const action = find(actions, action => action.description === Diagnostics.Convert_to_async_function.message);
332-
if (expectFailure && !onlyProvideAction) {
333-
assert.isNotTrue(action && action.changes.length > 0);
334-
return;
335-
}
336355

337-
assert.isTrue(action && action.changes.length > 0);
356+
let outputText: string | null;
357+
if (action?.changes.length) {
358+
const data: string[] = [];
359+
data.push(`// ==ORIGINAL==`);
360+
data.push(text.replace("[#|", "/*[#|*/").replace("|]", "/*|]*/"));
361+
const changes = action.changes;
362+
assert.lengthOf(changes, 1);
363+
364+
data.push(`// ==ASYNC FUNCTION::${action.description}==`);
365+
const newText = textChanges.applyChanges(sourceFile.text, changes[0].textChanges);
366+
data.push(newText);
367+
368+
const diagProgram = makeLanguageService({ path, content: newText }, includeLib, includeModule).getProgram()!;
369+
assert.isFalse(hasSyntacticDiagnostics(diagProgram));
370+
outputText = data.join(newLineCharacter);
371+
}
372+
else {
373+
// eslint-disable-next-line no-null/no-null
374+
outputText = null;
375+
}
338376

339-
const data: string[] = [];
340-
data.push(`// ==ORIGINAL==`);
341-
data.push(text.replace("[#|", "/*[#|*/").replace("|]", "/*|]*/"));
342-
const changes = action!.changes;
343-
assert.lengthOf(changes, 1);
377+
Harness.Baseline.runBaseline(`${baselineFolder}/${caption}${extension}`, outputText);
344378

345-
data.push(`// ==ASYNC FUNCTION::${action!.description}==`);
346-
const newText = textChanges.applyChanges(sourceFile.text, changes[0].textChanges);
347-
data.push(newText);
379+
if (expectNoSuggestionDiagnostic) {
380+
assert.isUndefined(diagnostic, "Expected code fix to not provide a suggestion diagnostic");
381+
}
382+
else if (expectSuggestionDiagnostic) {
383+
assert.exists(diagnostic, "Expected code fix to provide a suggestion diagnostic");
384+
}
348385

349-
const diagProgram = makeLanguageService({ path, content: newText }, includeLib, includeModule).getProgram()!;
350-
assert.isFalse(hasSyntacticDiagnostics(diagProgram));
351-
Harness.Baseline.runBaseline(`${baselineFolder}/${caption}${extension}`, data.join(newLineCharacter));
386+
if (expectNoAction) {
387+
assert.isNotTrue(!!action?.changes.length, "Expected code fix to not provide an action");
388+
assert.isNotTrue(typeof outputText === "string", "Expected code fix to not apply changes");
389+
}
390+
else if (expectAction) {
391+
assert.isTrue(!!action?.changes.length, "Expected code fix to provide an action");
392+
assert.isTrue(typeof outputText === "string", "Expected code fix to apply changes");
393+
}
352394
}
353395

354396
function makeLanguageService(file: TestFSWithWatch.File, includeLib?: boolean, includeModule?: boolean) {
@@ -372,19 +414,23 @@ interface Array<T> {}`
372414
}
373415

374416
const _testConvertToAsyncFunction = createTestWrapper((it, caption: string, text: string) => {
375-
testConvertToAsyncFunction(it, caption, text, "convertToAsyncFunction", /*includeLib*/ true);
417+
testConvertToAsyncFunction(it, caption, text, "convertToAsyncFunction", ConvertToAsyncTestFlags.IncludeLib | ConvertToAsyncTestFlags.ExpectSuccess);
376418
});
377419

378420
const _testConvertToAsyncFunctionFailed = createTestWrapper((it, caption: string, text: string) => {
379-
testConvertToAsyncFunction(it, caption, text, "convertToAsyncFunction", /*includeLib*/ true, /*includeModule*/ false, /*expectFailure*/ true);
421+
testConvertToAsyncFunction(it, caption, text, "convertToAsyncFunction", ConvertToAsyncTestFlags.IncludeLib | ConvertToAsyncTestFlags.ExpectFailed);
380422
});
381423

382424
const _testConvertToAsyncFunctionFailedSuggestion = createTestWrapper((it, caption: string, text: string) => {
383-
testConvertToAsyncFunction(it, caption, text, "convertToAsyncFunction", /*includeLib*/ true, /*includeModule*/ false, /*expectFailure*/ true, /*onlyProvideAction*/ true);
425+
testConvertToAsyncFunction(it, caption, text, "convertToAsyncFunction", ConvertToAsyncTestFlags.IncludeLib | ConvertToAsyncTestFlags.ExpectNoSuggestionDiagnostic | ConvertToAsyncTestFlags.ExpectAction);
426+
});
427+
428+
const _testConvertToAsyncFunctionFailedAction = createTestWrapper((it, caption: string, text: string) => {
429+
testConvertToAsyncFunction(it, caption, text, "convertToAsyncFunction", ConvertToAsyncTestFlags.IncludeLib | ConvertToAsyncTestFlags.ExpectSuggestionDiagnostic | ConvertToAsyncTestFlags.ExpectNoAction);
384430
});
385431

386432
const _testConvertToAsyncFunctionWithModule = createTestWrapper((it, caption: string, text: string) => {
387-
testConvertToAsyncFunction(it, caption, text, "convertToAsyncFunction", /*includeLib*/ true, /*includeModule*/ true);
433+
testConvertToAsyncFunction(it, caption, text, "convertToAsyncFunction", ConvertToAsyncTestFlags.IncludeLib | ConvertToAsyncTestFlags.IncludeModule | ConvertToAsyncTestFlags.ExpectSuccess);
388434
});
389435

390436
describe("unittests:: services:: convertToAsyncFunction", () => {
@@ -435,11 +481,11 @@ function [#|f|](): Promise<void>{
435481
function [#|f|]():Promise<void> {
436482
return fetch('https://typescriptlang.org').then(result => { console.log(result); }).catch(err => { console.log(err); });
437483
}`);
438-
_testConvertToAsyncFunction("convertToAsyncFunction_CatchAndRej", `
484+
_testConvertToAsyncFunctionFailed("convertToAsyncFunction_CatchAndRej", `
439485
function [#|f|]():Promise<void> {
440486
return fetch('https://typescriptlang.org').then(result => { console.log(result); }, rejection => { console.log("rejected:", rejection); }).catch(err => { console.log(err) });
441487
}`);
442-
_testConvertToAsyncFunction("convertToAsyncFunction_CatchAndRejRef", `
488+
_testConvertToAsyncFunctionFailed("convertToAsyncFunction_CatchAndRejRef", `
443489
function [#|f|]():Promise<void> {
444490
return fetch('https://typescriptlang.org').then(res, rej).catch(catch_err)
445491
}
@@ -1718,5 +1764,76 @@ function [#|foo|](p: Promise<string[]>) {
17181764
}
17191765
`);
17201766

1767+
_testConvertToAsyncFunction("convertToAsyncFunction_thenNoArguments", `
1768+
declare function foo(): Promise<number>;
1769+
function [#|f|](): Promise<number> {
1770+
return foo().then();
1771+
}`);
1772+
_testConvertToAsyncFunction("convertToAsyncFunction_catchNoArguments", `
1773+
declare function foo(): Promise<number>;
1774+
function [#|f|](): Promise<number> {
1775+
return foo().catch();
1776+
}`);
1777+
_testConvertToAsyncFunction("convertToAsyncFunction_chainedThenCatchThen", `
1778+
declare function foo(): Promise<number>;
1779+
function [#|f|](): Promise<number> {
1780+
return foo().then(x => Promise.resolve(x + 1)).catch(() => 1).then(y => y + 2);
1781+
}`);
1782+
_testConvertToAsyncFunction("convertToAsyncFunction_finally", `
1783+
declare function foo(): Promise<number>;
1784+
function [#|f|](): Promise<number> {
1785+
return foo().finally(() => console.log("done"));
1786+
}`);
1787+
_testConvertToAsyncFunction("convertToAsyncFunction_finallyNoArguments", `
1788+
declare function foo(): Promise<number>;
1789+
function [#|f|](): Promise<number> {
1790+
return foo().finally();
1791+
}`);
1792+
_testConvertToAsyncFunction("convertToAsyncFunction_finallyNull", `
1793+
declare function foo(): Promise<number>;
1794+
function [#|f|](): Promise<number> {
1795+
return foo().finally(null);
1796+
}`);
1797+
_testConvertToAsyncFunction("convertToAsyncFunction_finallyUndefined", `
1798+
declare function foo(): Promise<number>;
1799+
function [#|f|](): Promise<number> {
1800+
return foo().finally(undefined);
1801+
}`);
1802+
_testConvertToAsyncFunction("convertToAsyncFunction_thenFinally", `
1803+
declare function foo(): Promise<number>;
1804+
function [#|f|](): Promise<number> {
1805+
return foo().then(x => x + 1).finally(() => console.log("done"));
1806+
}`);
1807+
_testConvertToAsyncFunction("convertToAsyncFunction_thenFinallyThen", `
1808+
declare function foo(): Promise<number>;
1809+
function [#|f|](): Promise<number> {
1810+
return foo().then(x => Promise.resolve(x + 1)).finally(() => console.log("done")).then(y => y + 2);
1811+
}`);
1812+
_testConvertToAsyncFunctionFailedAction("convertToAsyncFunction_returnInBranch", `
1813+
declare function foo(): Promise<number>;
1814+
function [#|f|](): Promise<number> {
1815+
return foo().then(() => {
1816+
if (Math.random()) {
1817+
return 1;
1818+
}
1819+
return 2;
1820+
}).then(a => {
1821+
return a + 1;
1822+
});
1823+
}
1824+
`);
1825+
_testConvertToAsyncFunctionFailedAction("convertToAsyncFunction_partialReturnInBranch", `
1826+
declare function foo(): Promise<number>;
1827+
function [#|f|](): Promise<number> {
1828+
return foo().then(() => {
1829+
if (Math.random()) {
1830+
return 1;
1831+
}
1832+
console.log("foo");
1833+
}).then(a => {
1834+
return a + 1;
1835+
});
1836+
}
1837+
`);
17211838
});
17221839
}

0 commit comments

Comments
 (0)