Typescript: "Convert to async function" does not handle .then(fn1, fn2) correctly

[mjbvz]

_From @MikeyBurkman on October 8, 2018 21:37_

• VSCode Version: 1.28.0
• OS Version: OSX High Sierra

Steps to Reproduce:

1. The code in the gif on https://code.visualstudio.com/updates/v1_28#_convert-to-async-function is actually wrong
2. When doing .then(fn1, fn2), the second arg fn2 should NOT be called if there's an error in fn1.

Sample code:

'use strict';

const promise = () => new Promise((res) => setTimeout(res, 500));

const fSync = function() {
  return promise().then(
    () => {
      throw new Error('Failure!');
    },
    () => null
  );
};

// This function was created by copying/pasting the above function, and applying the "convert to async function" helper
const fAsync = async function() {
  try {
    await promise();
    throw new Error('Failure!');
  } catch (e) {
    return null;
  }
};

fSync()
  .then(() => console.log('SYNC Sucess?'))
  .catch((err) => console.log('SYNC Got an error', err));

fAsync()
  .then(() => console.log('ASYNC Sucess?'))
  .catch((err) => console.log('ASYNC Got an error', err));

Expected output:

• Both should output "[A]SYNC Got an error Error: Failure!"

Actual output:

• SYNC Got an error Error: Failure!
• ASYNC Success?

_Copied from original issue: Microsoft/vscode#60201_

[rbuckton]

@DanielRosenwasser: We discussed this at one point during the development of this feature, as to whether the refactor should either:

• result in the precise semantics as was originally written, or
• result in easier to read (and further refine) user code.

I believe we opted for the latter, due to the complexity of the refactored output of the former. If we opt to change this, we might want to consider the following as the refactored result instead:

// before
const fSync = function() {
  return promise().then(
    (x) => {
      console.log(x); // note: added to illustrate refactor
      throw new Error('Failure!');
    },
    () => null
  );
};

// after
const fSync = async function() {
  let x;
  try {
    x = await promise();
  } catch (e) {
    return null;
  }
  console.log(x); // note: added to illustrate refactor
  throw new Error('Failure!');
};

[MikeyBurkman]

Granted, having a two-arg then() is pretty rare, and I'll admit that I had a very hard time finding any friends/colleagues that could point out why the refactor was wrong, because having two args is extremely uncommon. But that's the part that worries me.

Anyone who does the two-arg then() probably is aware of what they're doing. But it would be easy enough for someone less skilled (or just not observant) to come in, do the automatic refactor, and move on, not realizing that they just broke the code, and their tests may easily miss it.

I generally assume that when my IDE suggests a refactor, it's going to result in correct output. If a particular refactor would be too complex, or just very difficult, to support, I'd personally much rather have the IDE just not even attempt the refactor than to output incorrect code. I believe it's better to meet 95% of use cases and always be correct, than to do 100% but be sometimes wrong in unpredictable ways.

(Also, having this particular example as the example gif in the release notes was an odd choice. I probably wouldn't have noticed anything had you all chosen a more typical example :P )

[uniqueiniquity]

@MikeyBurkman Thanks for reporting this! I agree with you in that by the end of the development of this feature, our goal was to meet fewer use cases in favor of higher correctness, so I wouldn't call this intentional by any means.

I'll investigate this further as soon as I can!

[uniqueiniquity]

Hi @MikeyBurkman, sorry for taking so long to address this, it's been a busy month.
After spending some time poking at this, I'm inclined to say we should stop offering the refactoring for two-arg then() calls. There doesn't seem to be a user-friendly way to ensure that the refactor's output is 100% correct; that is to say, there's not a clean way to always express that _either_ the onFulfilled callback _or_ the onRejected callback is called without the potential for ballooning conditional usage. I also don't think it's reasonable to try to detect errors that could be thrown by an onFulfilled callback and selectively fail on those.
@DanielRosenwasser what do you think?

[MikeyBurkman]

No worries on the delay. And yeah, after looking at various examples, I unfortunately came to same conclusion as you.

Funny enough, I remember seeing on Twitter a little while ago a gif of someone using this on a particularly nasty JS file in the VSC repo, and being amazed at how condensed the code became. Of course, upon further inspection, I realized that that code used the 2-arg then() quite liberally, so the resultant code would also have issues...

[DanielRosenwasser]

Yeah that was @mjbvz - I wonder if he wants us to remove the 2-arg refactoring 😄

[mjbvz]

We should make the refactorings/suggestions safe so that users can apply them automatically. That example code from the VS Code codebase needs more attention than just a quick fix

[andrewbranch]

I couldn’t figure out why @rbuckton’s suggestion wouldn’t work until after I implemented it. The problem arises only if you chain another .then after the two-parameter .then:

const fSync = function() {
  return promise().then(
    (x) => {
      console.log(x); // note: added to illustrate refactor
      throw new Error('Failure!');
    },
    () => null
-  );
+ ).then(y => {
+   console.log(y);
+ });
};

The proposed emit doesn’t really have anywhere to go from there:

// after
const fSync = async function() {
  let x;
  try {
    x = await promise();
  } catch (e) {
    return null;
  }
  console.log(x); // note: added to illustrate refactor
  throw new Error('Failure!');
};

We can’t return null in the catch clause, but we also can’t do the two statements after the try/catch if we go through the catch clause. I think we’d have to introduce some kind of state tracking 🙅‍♂

[rbuckton]

The first example assumed there was only one unit of work. The new .then adds more complexity as it introduces a new y variable and continues only on the "fulfilled" rail. As a result you would need even more unrolling to do this correctly.

Essentially, each linear continuation via .then or .catch bifurcates the code path into two rails, the "fulfilled" rail and the "rejected" rail. To illustrate this, I'll break down the then-based code:

const fSync = function() {
  return promise()
    // P1
    .then(
      // F1
      (x) => {
        console.log(x); // (1)
        throw new Error('Failure!'); // (2) rejects P1
      },
      // R1
      () => {
        return null; // (3) fulfills P1
      }
    )
    // P2
    .then(
      // F2
      y => {
        console.log(y); // (4) fulfills P2
      });

This creates a resolution graph that looks like this:

    P0          - await the result of `promise()`
  /    \
 F1    R1
 |      |
 x      |       F1: introduce variable `x`
(1)     |       F1: `console.log(x)`
(2)     |       F1: `throw new Error('Failure!')`
 |      |       F1: reject P1, switch to rejection rail (R2) in P2
 |      |
 |     (3)      F2: `return null`
 |      |       F2: fulfill P1, switch to fulfillment rail (F2) in P2
 |      |
  \    /
    P1
  /    \
 F2    R2
 |      .
 y      .       F2: introduce variable `y`
(4)     .       F2: `console.log(y)`
 |      .       F2: fulfill P2
 |      .
 |      .       R2: not handled, reject P2
  \    .
    P2

To convert this into an async function, we need to break down each continuation into a discrete step and indicate which rail we are on using a boolean variable (ok in the example below).

Using the above example, we would end up with something like this:

const fSync = async function() {
  let ok = true;

  // P0

  let x;
  try {
    if (ok) {
      ok = false;

      // F0
      x = await promise();

      ok = true; // use F1 rail
    } else {
      ok = false;

      // R0

      ok = true;
    }
  } catch {
    ok = false; // use R1 rail
  }

  // P1

  let y;
  try {
    if (ok) {
      ok = false;

      // F1
      console.log(x); // (1)
      throw new Error('Failure!'); // (2)

      ok = true;
    } else {
      ok = false;

      // R1
      y = await null; // (3)

      ok = true;
    }
  } catch {
    ok = false;
  }

  // P2

  try {
    if (ok) {
      ok = false;

      // F2
      console.log(y); // (4)

      ok = true;
    } 
    else {
      ok = false;
      // no R2
    }
  } catch {
    ok = false;
  }
};

However we can simplify this because we can statically analyze our transformation and remove unused/redundant code paths. We can group each run of continuations that have no rejection handler. As soon as we encounter a rejection handler, we need to wrap that run of continuations in a try..catch block (or try..finally for Promise#finally). We can also eliminate if(ok) statements if we have no rejection rails to execute:

const fSync = async function() {
  let ok;

  let x;
  try {
    x = await promise();
    ok = true; // use F1 rail
  } catch {
    ok = false; // use R1 rail
  }

  let y;
  if (ok) {
    console.log(x);
    throw new Error('Failure!');
  }
  y = await null;
  console.log(y);
};

Finally, if we were concerned with variable scoping, we could further refine this by using block scopes:

const fSync = async function() {
  let ok;

  let y;
  {
    let x;
    try {
      x = await promise();
      ok = true; // use F1 rail
    } catch {
      ok = false; // use R1 rail
    }

    if (ok) {
      console.log(x);
      throw new Error('Failure!');
    }
    y = await null;
  }

  console.log(y);
};

[andrewbranch]

Your ok variable was exactly what I meant by “state tracking.” Interesting as it is, it would be miserable to get this out of a refactor. I’ll probably keep the fix I’ve already implemented for the single-then case, and disable the refactor when the two-parameter then isn’t the last in the chain.

[andrewbranch]

Closed in #38152