codefix convertToAsync does not add await for returned values

[rradczewski]

First of all, thanks a lot for this refactoring, I think it's really useful and will push the adoption of async-await syntax 👍

This is a bug report related to this Twitter conversation: The converted code does not await for Promise.reject, thus the runtime does not throw but returns Promise.reject(3), which does not trigger the catch-clause but makes the function call return a rejected Promise.

I'm not 100% sure what the best conversion would be, but I assume it would be to always await for functions that could possibly return a Promise or expressions that are a Promise.

TypeScript Version: Tested against https://github.com/Microsoft/TypeScript/tree/v3.1.1 and 4ed85b7

Failing testcases:

• On top of master: https://github.com/rradczewski/TypeScript/tree/bug-return-needs-await (master...rradczewski:bug-return-needs-await)
• On top of v3.1.1: https://github.com/rradczewski/TypeScript/tree/bug-v3.1.1-return-needs-await (v3.1.1...rradczewski:bug-v3.1.1-return-needs-await)

These assume that the correct behavior is to add await to a returned expression that yields a Promise.

Search Terms:

• async
• convertToAsync

Code

function testcase() {
  return Promise.resolve(1)
    .then(x => Promise.reject(2))
    .catch(err => console.log(err));
}

Running testcase() code will log 2 and the returned Promise will resolve to undefined.

Expected behavior:

async function testcase() {
    try {
        const x = await Promise.resolve(1);
        return await Promise.reject(2);
    }
    catch (err) {
        return console.log(err);
    }
}

A call to testcase() would print 2 and the returned Promise would resolve to undefined as above.

Actual behavior:

async function testcase() {
    try {
        const x = await Promise.resolve(1);
        return Promise.reject(2);
    }
    catch (err) {
        return console.log(err);
    }
}

A call to testcase() this won't print 2 but node will complain about an unhandled rejected promise.

Related Issues:

• Proposal: Code Fix -- Convert Promise Handlers to Async and Await #25082 - The proposal, while mentioning "changed semantics", gives two conflicting examples:
  • Catch handlers has await console.log(...)
  • Multiple .catch() calls does not add await to console.log
• Possibly in async code fix, handle promise handler callbacks that return a promise #27112 and Async code fix issues concerning underscores and nested promises #27156

[j-oliveras]

The await is not necessary:

An async function always wraps the return value in a Promise. Using return await just adds extra time before the overreaching promise is resolved without changing the semantics.

From no-return-await tslint rule.

[j-oliveras]

Tested with node 8.11.1 and it seems is necessary to await Promise.reject rejections but not resolve at return position.

[j-oliveras]

I found and explanation when is necessary an await: https://stackoverflow.com/a/42750371

With the await, it is catched by the catch block. Without it, returns the promise and then is resolved (or rejected in this case).

@uniqueiniquity maybe, convertToAsync should await the return if it inside a catch block.

[uniqueiniquity]

@rradczewski thanks for reporting, and @j-oliveras thanks for the investigation! I'll tackle this as soon as I can.