Minor fixes to "Convert To Async" refactor

[rbuckton]

This fixes some corner cases in the "Convert to Async" refactor:

• Correctly awaiting return value inside of a catch
• Eliding empty .then(), .then(null), .then(undefined), etc.
• Eliding empty .catch(), .catch(null), etc.
• Eliding catch variable when unused (i.e., .catch(() => { })).
• Support .finally() using try { } finally { }.
• Use awaited type in Promise.resolve(1).then(x => Promise.resolve(x + 1)).then(y => { ... }) (type of y is now number instead of number | Promise<number>).

Fixes #35192

[rbuckton]

I found a small issue I need to investigate, as one of my fixes is a bit over aggressive. Essentially I was trying to fix this case:

function f2() {
    return foo().then(() => {
        if (!0) {
            return 1;
        }
        console.log("other");
    }).then(a => {
        return a;
    });
}

Which currently gets refactored to this:

async function f2() {
    await foo();
    if (!0) {
        return 1;
    }
    console.log("other");
    const a = undefined;
    return a;
}

Which is incorrect, because the if branch shouldn't exit the function. To resolve this I started injecting labeled blocks:

async function f2() {
    await foo();
    let a;
    block_1: {
        if (!0) {
            a = 1;
            break block_1;
        }
        console.log("other");
    }
    return a + 1;
}

However it is too aggressive in cases where branches are exhaustive.

[andrewbranch]

@rbuckton where we’ve landed in discussions about Convert To Async before is that if we can't produce code that a reasonable human would write, the refactor should be unavailable. Labeled blocks would be a hard dealbreaker for me as a user of this refactor.

[rbuckton]

@rbuckton where we’ve landed in discussions about Convert To Async before is that if we can't produce code that a reasonable human would write, the refactor should be unavailable. Labeled blocks would be a hard dealbreaker for me as a user of this refactor.

While I can understand and agree with this sentiment (and will happily remove that part), unfortunately I still have to solve the underlying problem, unless we want to also make the refactor unavailable for a common scenario like this:

function f() {
  return foo().then(a => {
    if (a) {
      return 1;
    }
    else {
      return 2;
    }
  }).then(b => {
    return b;
  });
}

This could be refactored without labeled blocks, but also fails to refactor, producing this currently:

async function f() {
    const a = await foo();
    if (a) {
        return 1;
    }
    else {
        return 2;
    }
    const b = undefined;
    return b;
}

When the result should be:

async function f() {
    const a = await foo();
    let b: number;
    if (a) {
        b = 1;
    }
    else {
        b = 2;
    }
    return b;
}

[andrewbranch]

Makes sense. Your expected behavior there looks reasonable enough, but if it’s wildly difficult to get it working for some reason, I’m not too concerned about targeted disabling of the refactor.

[rbuckton]

Handling all the cases for conditional branching in a continuation is growing more and more complex. For now, I've changed it to just reject the codefix if there's a nested return in any callback that isn't the final continuation, so

function f() {
  return foo().then(a => {
    if (a) {
      return 1;
    }
    else {
      return 2;
    }
  }).then(b => {
    return b;
  });
}

will fail to refactor since there's a trailing continuation, yet

function f() {
  return foo().then(a => {
    if (a) {
      return 1;
    }
    else {
      return 2;
    }
  })
}

will refactor fine since the nested return is in the final continuation.

[rbuckton]

@amcasey, @andrewbranch can you take another look with these last changes?

[amcasey]

Baselines LGTM

[amcasey]

What was wrong here? Does this belong outside the try-catch?

[rbuckton]

In #38152 we removed support for applying the code fix if both an onfulfilled and an onrejected handler are present for the same call to .then, however we never removed some of the code for that refactor and it was inadvertently being called. If you look at both of these tests in https://github.com/microsoft/TypeScript/pull/45536/files/da1d25c4f7460212ba6e40b47ccebb07703f2d63#diff-8eb4fd66983b9bab0f035c7dabf674c0d05e08266a0fd1b0f09dde1ab1a8a840 you'll see that they both have both handlers present.

[amcasey]

What was wrong here?

[rbuckton]

See #45536 (comment)

[amcasey]

Not returning was just a bug?

[rbuckton]

Correct. Passing null or undefined to the onfulfilled handler is essentially the same as passing v => { return v; }, the value still flows through. Similarly, passing null or undefined to an onrejected handler is essentially the same as passing e => { throw e; }.