"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!