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

[a-tarasyuk]

Fixes #39589

[andrewbranch]

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?

[a-tarasyuk]

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); })
}

[andrewbranch]

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.

[andrewbranch]

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 {};
  }
}

[a-tarasyuk]

Thanks for the clarification.

[a-tarasyuk]

@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?

[andrewbranch]

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.

[andrewbranch]

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. 🤔

[a-tarasyuk]

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

[a-tarasyuk]

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

[andrewbranch]

I’m a little nervous that people will notice more unnecessary return await and complain, but this does solve a pretty bad correctness bug, and it sounds like we have an exact precedent for adding return await in exchange for correctness. We’ll let user feedback drive whether we need to address return await more broadly in the future.

[andrewbranch]

Thanks @a-tarasyuk!