Fix hoisting of arguments

[danez]

In the case that the generator or the async function is initially an arrow function and uses only a rest param, the babel es2015 transforms will replace the arrow with a function and the rest param with arguments. regenerator-transform will later also replace and move the arguments. In the end babel will hoist the arguments back up, but to the wrong level.

Thats a little bit complicated to describe, maybe better I show it with code:

1. initial

function test(fn) {
  return async (...args) => {
    return fn(...args);
  };
}

2. arrow transform

function test(fn) {
  return async function (...args) {
    return fn(...args);
  };
}

3. rest transform

function test(fn) {
  return async function () {
    return fn.apply(this, arguments);
  };
}

4. regenerator transform (this is what it should be in the end)

function test(fn) {
  var _this = this;
  return function () {
    var _ref = _asyncToGenerator(regeneratorRuntime.mark(function _callee() {
      var _args = arguments;
      return regeneratorRuntime.wrap(function _callee$(_context) {
        while (1) {
          switch (_context.prev = _context.next) {
            case 0:
              return _context.abrupt("return", fn.apply(undefined, _args));

            case 1:
            case "end":
              return _context.stop();
          }
        }
      }, _callee, _this);
    }));

    return function () {
      return _ref.apply(this, arguments);
    };
  }();
}

5. babel hoist (not exactly sure what is triggering this, maybe another plugin, or babel itself)

Note that arguments gets hoisted incorrectly

function test(fn) {
  var _arguments = arguments,
      _this = this;

  return function () {
    var _ref = _asyncToGenerator(regeneratorRuntime.mark(function _callee() {
      var _args = _arguments;
      return regeneratorRuntime.wrap(function _callee$(_context) {
        while (1) {
          switch (_context.prev = _context.next) {
            case 0:
              return _context.abrupt("return", fn.apply(undefined, _args));

            case 1:
            case "end":
              return _context.stop();
          }
        }
      }, _callee, _this);
    }));

    return function () {
      return _ref.apply(this, arguments);
    };
  }();
}

This change sets a hint on the arguments-identifier to which function it belongs to, and it will only get hoisted up to this level.
We also do this in babel in certain places: https://github.com/babel/babel/blob/7.0/packages/babel-plugin-transform-es2015-parameters/src/rest.js#L212

I tried creating an test, but couldn't find exactly how to do it.

//cc @hzoo @existentialism @loganfsmyth

Fixes: babel/babel#5485 babel/babel#5330 babel/babel#4219

[benjamn]

I'm not an expert on how hoisting works in Babel, though I have encountered it before. If anyone else has immediate ideas, please do chime in!

[benjamn]

This explanation of _shadowedFunctionLiteral seems consistent with the intent of these changes.

[Jessidhia]

Is this possibly related to gaearon/react-hot-loader#391 / babel/babel#5078 ?

[danez]

I was thinking the same that there is probably the same issue with this. But this PR is only for arguments. Maybe I can figure out if we can do the same with this.

@benjamn yeah it is weird that we have to use a private flag, but not sure what else we should do in this case, maybe we can disable the hoisting in a better way, I will investigate a little more.

[danez]

@Kovensky The this issues in 5078 seems unrelated, and is just an issue with copying the the nodes.
I tried different variants with thisand regenerator and they all work.

Another idea I had was turning the function into a non shadowed function by doing delete node.shadow, but this seems more risky to break other stuff.

Anyone has a different idea?

[benjamn]

@danez Care to add a test for this behavior, just in case the _shadowedFunctionLiteral "API" ever changes without warning? I'm happy with this implementation if it has a test.

[danez]

If you tell me how. All the tests seem to require native support in node for the features being tested, which might be hard here for async functions. And the bug only appears when babel is doing the transform.

[benjamn]

Sure, the test/async.js tests should run only in translation: https://github.com/facebook/regenerator/blob/master/test/async.js

[danez]

Thanks for the help, hope it is okay they way I setup the test.