Skip to content

fix(39589): await is missing after autofix convert to an async function #39649

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Sep 22, 2020
Merged

fix(39589): await is missing after autofix convert to an async function #39649

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 21 additions & 16 deletions src/services/codefixes/convertToAsyncFunction.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -302,18 +302,15 @@ namespace ts.codefix {
const [onFulfilled, onRejected] = node.arguments;
const onFulfilledArgumentName = getArgBindingName(onFulfilled, transformer);
const transformationBody = getTransformationBody(onFulfilled, prevArgName, onFulfilledArgumentName, node, transformer);

if (onRejected) {
const onRejectedArgumentName = getArgBindingName(onRejected, transformer);
const tryBlock = factory.createBlock(transformExpression(node.expression, transformer, onFulfilledArgumentName).concat(transformationBody));
const transformationBody2 = getTransformationBody(onRejected, prevArgName, onRejectedArgumentName, node, transformer);
const catchArg = onRejectedArgumentName ? isSynthIdentifier(onRejectedArgumentName) ? onRejectedArgumentName.identifier.text : onRejectedArgumentName.bindingPattern : "e";
const catchVariableDeclaration = factory.createVariableDeclaration(catchArg);
const catchClause = factory.createCatchClause(catchVariableDeclaration, factory.createBlock(transformationBody2));

return [factory.createTryStatement(tryBlock, catchClause, /* finallyBlock */ undefined)];
}

return transformExpression(node.expression, transformer, onFulfilledArgumentName).concat(transformationBody);
}

Expand Down Expand Up @@ -395,19 +392,21 @@ namespace ts.codefix {
case SyntaxKind.FunctionExpression:
case SyntaxKind.ArrowFunction: {
const funcBody = (func as FunctionExpression | ArrowFunction).body;
const returnType = getLastCallSignature(transformer.checker.getTypeAtLocation(func), transformer.checker)?.getReturnType();

// Arrow functions with block bodies { } will enter this control flow
if (isBlock(funcBody)) {
let refactoredStmts: Statement[] = [];
let seenReturnStatement = false;

for (const statement of funcBody.statements) {
if (isReturnStatement(statement)) {
seenReturnStatement = true;
if (isReturnStatementWithFixablePromiseHandler(statement)) {
refactoredStmts = refactoredStmts.concat(getInnerTransformationBody(transformer, [statement], prevArgName));
}
else {
refactoredStmts.push(...maybeAnnotateAndReturn(statement.expression, parent.typeArguments?.[0]));
const possiblyAwaitedRightHandSide = returnType && statement.expression ? getPossiblyAwaitedRightHandSide(transformer.checker, returnType, statement.expression) : statement.expression;
refactoredStmts.push(...maybeAnnotateAndReturn(possiblyAwaitedRightHandSide, parent.typeArguments?.[0]));
}
}
else {
Expand All @@ -431,19 +430,21 @@ namespace ts.codefix {
return innerCbBody;
}

const type = transformer.checker.getTypeAtLocation(func);
const returnType = getLastCallSignature(type, transformer.checker)!.getReturnType();
const rightHandSide = getSynthesizedDeepClone(funcBody);
const possiblyAwaitedRightHandSide = !!transformer.checker.getPromisedTypeOfPromise(returnType) ? factory.createAwaitExpression(rightHandSide) : rightHandSide;
if (!shouldReturn(parent, transformer)) {
const transformedStatement = createVariableOrAssignmentOrExpressionStatement(prevArgName, possiblyAwaitedRightHandSide, /*typeAnnotation*/ undefined);
if (prevArgName) {
prevArgName.types.push(returnType);
if (returnType) {
const possiblyAwaitedRightHandSide = getPossiblyAwaitedRightHandSide(transformer.checker, returnType, funcBody);
if (!shouldReturn(parent, transformer)) {
const transformedStatement = createVariableOrAssignmentOrExpressionStatement(prevArgName, possiblyAwaitedRightHandSide, /*typeAnnotation*/ undefined);
if (prevArgName) {
prevArgName.types.push(returnType);
}
return transformedStatement;
}
else {
return maybeAnnotateAndReturn(possiblyAwaitedRightHandSide, parent.typeArguments?.[0]);
}
return transformedStatement;
}
else {
return maybeAnnotateAndReturn(possiblyAwaitedRightHandSide, parent.typeArguments?.[0]);
return silentFail();
}
}
}
Expand All @@ -454,12 +455,16 @@ namespace ts.codefix {
return emptyArray;
}

function getPossiblyAwaitedRightHandSide(checker: TypeChecker, type: Type, expr: Expression): AwaitExpression | Expression {
const rightHandSide = getSynthesizedDeepClone(expr);
return !!checker.getPromisedTypeOfPromise(type) ? factory.createAwaitExpression(rightHandSide) : rightHandSide;
}

function getLastCallSignature(type: Type, checker: TypeChecker): Signature | undefined {
const callSignatures = checker.getSignaturesOfType(type, SignatureKind.Call);
return lastOrUndefined(callSignatures);
}


function removeReturns(stmts: readonly Statement[], prevArgName: SynthBindingName | undefined, transformer: Transformer, seenReturnStatement: boolean): readonly Statement[] {
const ret: Statement[] = [];
for (const stmt of stmts) {
Expand Down
61 changes: 58 additions & 3 deletions src/testRunner/unittests/services/convertToAsyncFunction.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -902,7 +902,7 @@ function [#|f|](): Promise<string> {
}
`
);
_testConvertToAsyncFunction("convertToAsyncFunction_PromiseAllAndThen", `
_testConvertToAsyncFunction("convertToAsyncFunction_PromiseAllAndThen1", `
function [#|f|]() {
return Promise.resolve().then(function () {
return Promise.all([fetch("https://typescriptlang.org"), fetch("https://microsoft.com"), Promise.resolve().then(function () {
Expand All @@ -921,6 +921,26 @@ function [#|f|]() {
})]).then(res => res.toString());
});
}
`
);

_testConvertToAsyncFunction("convertToAsyncFunction_PromiseAllAndThen3", `
function [#|f|]() {
return Promise.resolve().then(() =>
Promise.all([fetch("https://typescriptlang.org"), fetch("https://microsoft.com"), Promise.resolve().then(function () {
return fetch("https://github.com");
}).then(res => res.toString())]));
}
`
);

_testConvertToAsyncFunction("convertToAsyncFunction_PromiseAllAndThen4", `
function [#|f|]() {
return Promise.resolve().then(() =>
Promise.all([fetch("https://typescriptlang.org"), fetch("https://microsoft.com"), Promise.resolve().then(function () {
return fetch("https://github.com");
})]).then(res => res.toString()));
}
`
);
_testConvertToAsyncFunction("convertToAsyncFunction_Scope1", `
Expand Down Expand Up @@ -1124,6 +1144,27 @@ function [#|bar|]<T>(x: T): Promise<T> {
`
);

_testConvertToAsyncFunction("convertToAsyncFunction_Return1", `
function [#|f|](p: Promise<unknown>) {
return p.catch((error: Error) => {
return Promise.reject(error);
});
}`
);

_testConvertToAsyncFunction("convertToAsyncFunction_Return2", `
function [#|f|](p: Promise<unknown>) {
return p.catch((error: Error) => Promise.reject(error));
}`
);

_testConvertToAsyncFunction("convertToAsyncFunction_Return3", `
function [#|f|](p: Promise<unknown>) {
return p.catch(function (error: Error) {
return Promise.reject(error);
});
}`
);

_testConvertToAsyncFunction("convertToAsyncFunction_LocalReturn", `
function [#|f|]() {
Expand Down Expand Up @@ -1352,7 +1393,7 @@ function [#|f|]() {
}
`);

_testConvertToAsyncFunction("convertToAsyncFunction_noArgs", `
_testConvertToAsyncFunction("convertToAsyncFunction_noArgs1", `
function delay(millis: number): Promise<void> {
throw "no"
}
Expand All @@ -1364,7 +1405,21 @@ function [#|main2|]() {
.then(() => { console.log("."); return delay(500); })
.then(() => { console.log("."); return delay(500); })
}
`);
`);

_testConvertToAsyncFunction("convertToAsyncFunction_noArgs2", `
function delay(millis: number): Promise<void> {
throw "no"
}

function [#|main2|]() {
console.log("Please wait. Loading.");
return delay(500)
.then(() => delay(500))
.then(() => delay(500))
.then(() => delay(500))
}
`);

_testConvertToAsyncFunction("convertToAsyncFunction_exportModifier", `
export function [#|foo|]() {
Expand Down
2 changes: 1 addition & 1 deletion ...nvertToAsyncFunction_PromiseAllAndThen.js → ...vertToAsyncFunction_PromiseAllAndThen1.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ function /*[#|*/f/*|]*/() {

async function f() {
await Promise.resolve();
return Promise.all([fetch("https://typescriptlang.org"), fetch("https://microsoft.com"), Promise.resolve().then(function() {
return await Promise.all([fetch("https://typescriptlang.org"), fetch("https://microsoft.com"), Promise.resolve().then(function() {
return fetch("https://github.com");
}).then(res => res.toString())]);
}
2 changes: 1 addition & 1 deletion ...nvertToAsyncFunction_PromiseAllAndThen.ts → ...vertToAsyncFunction_PromiseAllAndThen1.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ function /*[#|*/f/*|]*/() {

async function f() {
await Promise.resolve();
return Promise.all([fetch("https://typescriptlang.org"), fetch("https://microsoft.com"), Promise.resolve().then(function() {
return await Promise.all([fetch("https://typescriptlang.org"), fetch("https://microsoft.com"), Promise.resolve().then(function() {
return fetch("https://github.com");
}).then(res => res.toString())]);
}
17 changes: 17 additions & 0 deletions ...s/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_PromiseAllAndThen3.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
// ==ORIGINAL==

function /*[#|*/f/*|]*/() {
return Promise.resolve().then(() =>
Promise.all([fetch("https://typescriptlang.org"), fetch("https://microsoft.com"), Promise.resolve().then(function () {
return fetch("https://github.com");
}).then(res => res.toString())]));
}

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

async function f() {
await Promise.resolve();
return await Promise.all([fetch("https://typescriptlang.org"), fetch("https://microsoft.com"), Promise.resolve().then(function() {
return fetch("https://github.com");
}).then(res => res.toString())]);
}
17 changes: 17 additions & 0 deletions ...s/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_PromiseAllAndThen3.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
// ==ORIGINAL==

function /*[#|*/f/*|]*/() {
return Promise.resolve().then(() =>
Promise.all([fetch("https://typescriptlang.org"), fetch("https://microsoft.com"), Promise.resolve().then(function () {
return fetch("https://github.com");
}).then(res => res.toString())]));
}

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

async function f() {
await Promise.resolve();
return await Promise.all([fetch("https://typescriptlang.org"), fetch("https://microsoft.com"), Promise.resolve().then(function() {
return fetch("https://github.com");
}).then(res => res.toString())]);
}
18 changes: 18 additions & 0 deletions ...s/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_PromiseAllAndThen4.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
// ==ORIGINAL==

function /*[#|*/f/*|]*/() {
return Promise.resolve().then(() =>
Promise.all([fetch("https://typescriptlang.org"), fetch("https://microsoft.com"), Promise.resolve().then(function () {
return fetch("https://github.com");
})]).then(res => res.toString()));
}

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

async function f() {
await Promise.resolve();
const res = await Promise.all([fetch("https://typescriptlang.org"), fetch("https://microsoft.com"), Promise.resolve().then(function() {
return fetch("https://github.com");
})]);
return res.toString();
}
18 changes: 18 additions & 0 deletions ...s/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_PromiseAllAndThen4.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
// ==ORIGINAL==

function /*[#|*/f/*|]*/() {
return Promise.resolve().then(() =>
Promise.all([fetch("https://typescriptlang.org"), fetch("https://microsoft.com"), Promise.resolve().then(function () {
return fetch("https://github.com");
})]).then(res => res.toString()));
}

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

async function f() {
await Promise.resolve();
const res = await Promise.all([fetch("https://typescriptlang.org"), fetch("https://microsoft.com"), Promise.resolve().then(function() {
return fetch("https://github.com");
})]);
return res.toString();
}
16 changes: 16 additions & 0 deletions tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_Return1.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
// ==ORIGINAL==

function /*[#|*/f/*|]*/(p: Promise<unknown>) {
return p.catch((error: Error) => {
return Promise.reject(error);
});
}
// ==ASYNC FUNCTION::Convert to async function==

async function f(p: Promise<unknown>) {
try {
return p;
} catch (error) {
return await Promise.reject(error);
}
}
14 changes: 14 additions & 0 deletions tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_Return2.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
// ==ORIGINAL==

function /*[#|*/f/*|]*/(p: Promise<unknown>) {
return p.catch((error: Error) => Promise.reject(error));
}
// ==ASYNC FUNCTION::Convert to async function==

async function f(p: Promise<unknown>) {
try {
return p;
} catch (error) {
return await Promise.reject(error);
}
}
16 changes: 16 additions & 0 deletions tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_Return3.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
// ==ORIGINAL==

function /*[#|*/f/*|]*/(p: Promise<unknown>) {
return p.catch(function (error: Error) {
return Promise.reject(error);
});
}
// ==ASYNC FUNCTION::Convert to async function==

async function f(p: Promise<unknown>) {
try {
return p;
} catch (error) {
return await Promise.reject(error);
}
}
5 changes: 3 additions & 2 deletions ...Function/convertToAsyncFunction_noArgs.ts → ...unction/convertToAsyncFunction_noArgs1.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ function /*[#|*/main2/*|]*/() {
.then(() => { console.log("."); return delay(500); })
.then(() => { console.log("."); return delay(500); })
}

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

function delay(millis: number): Promise<void> {
Expand All @@ -26,5 +26,6 @@ async function main2() {
console.log(".");
await delay(500);
console.log(".");
return delay(500);
return await delay(500);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could be wrong, but I believe we only need this change if the statement is inside a try block. Does that seem right, and do you think it would be difficult to limit the change to just those cases?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean for the following code the need to omit await in return statement and add it only if if inside try block?

function main2() {
    console.log("Please wait. Loading.");
    return delay(500)
        .then(() => { console.log("."); return delay(500); })
        .then(() => { console.log("."); return delay(500); })
        .then(() => { console.log("."); return delay(500); })
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, only if the resulting generated code is inside a try. return await is usually regarded as poor style, kind of like returning Promise.resolve(something) in a then. But it has importantly different semantics when inside a try block because it makes the difference between the error being handled in the function implementation vs. propagating the rejection onto the caller.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Example:

async function f1() {
  return await fetch('/'); // `await` is not useful here
}

async function f2() {
  try {
    // `await` is useful here; without it, the catch block would never execute.
    // f2 returns a Promise that never rejects. f1 returns a Promise that resolves
    // or rejects with `fetch('/')`, with or without the `await`.
    return await fetch('/');
  } catch {
    return {};
  }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the clarification.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@andrewbranch Should these examples also omit await in return?

TypeScript/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_nestedPromises.ts

Lines 10 to 12 in 083129f

const x = await fetch('https://typescriptlang.org');
const y = await Promise.resolve(3);
return await Promise.resolve(x.statusText.length + y);

or

TypeScript/tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_callbackReturnsPromiseLastInChain.ts

Lines 9 to 11 in 083129f

async function f() {
const s = await fetch('https://typescriptlang.org');
return await Promise.resolve(s.statusText.length);

I'm thinking about how to handle await inside/outside try statement. Maybe you have some thoughts about that?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, that baseline pointed me to this PR: #27573

I guess this is already a known limitation of the refactor. In that case, I’m ok with your PR as-is.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, I’m a little confused as to why #27573 didn’t already apply to some of the baselines that changed in this PR. I do think that if we start adding a lot more return await, people are going to file issues about it. 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@andrewbranch Thanks for sharing this PR. I didn't know about it.

Copy link
Contributor Author

@a-tarasyuk a-tarasyuk Sep 11, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Basically, this PR fixes a really simple issue, it just handles return statements in {}. For instance, two examples with the same logic behave differently. Example 

function fn(): Promise<void> {
    return Promise.resolve();
}

// Before
function f1() {
    return fn().then(() => fn());
}
// After 
async function f1() {
    await fn();
return await fn(); // await is added
}

// Before
function f2() {
    return fn().then(() => { return fn() });
}
// After 
async function f2() {
    await fn();
return fn(); // await is missing 
}

About for the test, there is a test for {}, however, there is no test for (). I decided to add, to handle both cases. https://github.com/microsoft/TypeScript/pull/39649/files#diff-71c285c7a401ed150a7e8367a2d6681cR1408-R1422

}

28 changes: 28 additions & 0 deletions tests/baselines/reference/convertToAsyncFunction/convertToAsyncFunction_noArgs2.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
// ==ORIGINAL==

function delay(millis: number): Promise<void> {
throw "no"
}

function /*[#|*/main2/*|]*/() {
console.log("Please wait. Loading.");
return delay(500)
.then(() => delay(500))
.then(() => delay(500))
.then(() => delay(500))
}

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

function delay(millis: number): Promise<void> {
throw "no"
}

async function main2() {
console.log("Please wait. Loading.");
await delay(500);
await delay(500);
await delay(500);
return await delay(500);
}