New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Function sent #5920

Merged
merged 11 commits into from Jul 25, 2017

Conversation

Projects
None yet
7 participants
@nicolo-ribaudo
Member

nicolo-ribaudo commented Jul 6, 2017

Q A
Patch: Bug Fix? No
Major: Breaking Change? no
Minor: New Feature? yes
Deprecations? no
Spec Compliancy? yes
Tests Added/Pass? yes
Fixed Tickets
License MIT
Doc PR
Dependency Changes no (yes? Just internal dependencies)

This PR introduces two new packages:

  • babel-helper-wrap-function:
    This plugins exports a function which takes a NodePath of a function and an identifier, then it transforms that function into a CallExpression whose callee is the identifier and the parameter is the function.
    It keeps the correct function name (if present) and the correct context (this/arguments).

    e.g.

    (function () {}};
    // Becomes
    _wrapper(function () {}};

    I did not write the code of this plugin, I just cut and pasted it from babel-helper-remap-async-to-generator.
    This plugin is used by babel-helper-remap-async-to-generator and babel-plugin-transform-function-sent.

  • babel-plugin-function-sent
    It transforms

    function* gen() {
      let a = function.sent;
    }

    Roughly to

    let gen = _skipFirstGeneratorNext(function* gen() {
      const _functionSent = yield;
      let a = _functionSentt;
    })

TODO

  • Add to babel-preset-stage-2
  • Update babel/proposals
  • Fix flow errors (I'm currently working with a pc which doesn't support flow; I'll do this in few days)
@mention-bot

This comment has been minimized.

mention-bot commented Jul 6, 2017

@nicolo-ribaudo, thanks for your PR! By analyzing the history of the files in this pull request, we identified @zenparsing, @hzoo and @loganfsmyth to be potential reviewers.

it.next(2);
assert.equal(a, 2);
assert.equal(b, 1);

This comment has been minimized.

@jridgewell

jridgewell Jul 6, 2017

Member

This should be 2. "[function.sent] is the value passed to the generator by the next method that most recently resumed execution of the generator."

class Foo {
gen() {
return _skipFirstGeneratorNext(function* () {

This comment has been minimized.

@jridgewell

jridgewell Jul 6, 2017

Member

This is terrible for performance. We can hoist this out, something like:

let gen;
class Foo {
  gen() {
    if (!gen) {
      gen = _skipFirstGeneratorNext(function* () {
        super.gen() // or whatever
      });
    }
    return gen.apply(this, arguments);
  }
}

This comment has been minimized.

@nicolo-ribaudo

nicolo-ribaudo Jul 6, 2017

Member

Doing so would break code like this: https://github.com/babel/babel/blob/7.0/packages/babel-plugin-transform-async-to-generator/test/fixtures/async-to-generator/object-method-with-super/actual.js

(I'm linking a test of async-to-generator instead of function-sent because it is basically a test for helper-wrap-function, which is the plugin which would host that call)

This comment has been minimized.

@jridgewell

jridgewell Jul 6, 2017

Member

That should still work, we'll need to hoist the function environment.

This comment has been minimized.

@nicolo-ribaudo

nicolo-ribaudo Jul 7, 2017

Member

function _skipFirstGeneratorNext(fn) {
return function () {
let it = fn.apply(this, arguments);
it.next();
return it;
}
}

This comment has been minimized.

@nicolo-ribaudo

nicolo-ribaudo Jul 7, 2017

Member

Copy-pasted the wrong thing 😅

Sorry, can you give me an example? The only way I can think of which works with super.method()/new.target is this:

class A {
  *gen() {
    super.method(function.sent);
  }
}

// Becomes

let _gen;
let superCall_method;
class A {
  gen() {
    if (!_gen) _gen = _skipFirstGeneratorNext(function* () {
      let _functionSent = yield;
      super_callMethod(_functionSent);
    });

    super_callMethod = (...args) => super.method(...args);
    return _gen.apply(this, arguments);
  }
}

But it won't work if A#gen calls A#gen of another instance:

class Base {
  method(x) {
    console.log(x + this.val);
  }
}

class A extends Base {
  constructor(val) {
    super();
    this.val = val;
  }

  *gen(x) {
    x && x.gen().next(7);
    super.method(function.sent);
  }
}

let n = new A(10);
let m = new A(100);
n.gen(m).next(1); // 107 11

// Becomes

class Base {
  method(x) {
    console.log(x + this.val);
  }
}

let _gen, super_callMethod;
class A extends Base {
  constructor(val) {
    super();
    this.val = val;
  }

  gen(x) {
    if (!_gen) _gen = _skipFirstGeneratorNext(function* (x) {
      let _functionSent = yield;
      x && x.gen().next(7);
       super_callMethod(_functionSent);
    });

    super_callMethod = (...args) => super.method(...args);
    return _gen.apply(this, arguments);
  }
}

let n = new A(10);
let m = new A(100);
n.gen(m).next(1); // 107 101

This comment has been minimized.

@jridgewell

jridgewell Jul 7, 2017

Member

I'm not sure I follow, but it's not really important for this PR. We can do it in a follow up.

This comment has been minimized.

@nicolo-ribaudo
path.replaceWith(sentId);
},
Function: {

This comment has been minimized.

@jridgewell

jridgewell Jul 6, 2017

Member

This will hit a lot of unneeded functions. We could just wrap the first time we find a function.sent.

This comment has been minimized.

@nicolo-ribaudo

nicolo-ribaudo Jul 6, 2017

Member

I could not remove this visitor because I needed it to fix the _functionSent assignment (now it is updated every time yield is called).

Now this plugin works like this:

  1. Replace function.sents
  2. Replace yields (this needs a second traversal)
  3. Wrap the function

I could not do 1 and 2 in a single step because if there is yield before function.sent, it is not possible to know if it must be replaced

This comment has been minimized.

@jridgewell

jridgewell Jul 7, 2017

Member

We should be able to with a single pass:

export default function({ types: t }) {
  const yieldVisitor = {
    Function(path) {
      path.skip();
    },

    YieldExpression(path) {
      path.replaceWith(
        t.assignmentExpression("=", this.sentId, path.node),
      );
      path.skip();
    },

    MetaProperty(path) {
      if (!t.isIdentifier(path.node.meta, { name: "function" })) return;
      if (!t.isIdentifier(path.node.property, { name: "sent" })) return;
      path.replaceWith(this.sentId);
    },
  };

  return {
    inherits: syntaxFunctionSent,

    visitor: {
      MetaProperty(path) {
        if (!t.isIdentifier(path.node.meta, { name: "function" })) return;
        if (!t.isIdentifier(path.node.property, { name: "sent" })) return;

        const fnPath = path.getFunctionParent();

        if (!fnPath.node.generator) {
          throw new Error("Parent generator function not found");
        }

        const sentId = path.scope.generateUidIdentifier("function.sent");
        fnPath.traverse(yieldVisitor, { sentId });
        fnPath.get("body").unshiftContainer("body", [
          t.variableDeclaration("let", [
            t.variableDeclarator(sentId, t.yieldExpression()),
          ]),
        ])
        wrapFunction(path, state.addHelper("skipFirstGeneratorNext"));
      },
    },
  };
}

This comment has been minimized.

@nicolo-ribaudo
@@ -0,0 +1,5 @@
(function* () {

This comment has been minimized.

@loganfsmyth

loganfsmyth Jul 6, 2017

Member

It seems like for this case, we don't technically need the _skipFirstGeneratorNext wrapper, because function.sent is never used before the first yield. We don't have much in the way of flow analysis right now, but it could be super nice to only inject the wrapper when it is strictly necessary.

This comment has been minimized.

@nicolo-ribaudo

nicolo-ribaudo Jul 6, 2017

Member

Does Babel have some functions to check if two nodes are in the same flow branch?

Because the fact that yield comes before function.sent isn't enough:

(function* (foo) {
  foo && yield;
  function.sent;
}(false));

This comment has been minimized.

@loganfsmyth

loganfsmyth Jul 6, 2017

Member

I don't think we do have anything like this currently unfortunately.

I guess it may be fine to leave that as a future optimization, as long as the function is only wrapped if function.sent is in use.

},
YieldExpression(path) {
const replaced = t.isAssignmentExpression(path.parent, {

This comment has been minimized.

@jridgewell

jridgewell Jul 7, 2017

Member

We could remove this and add a Path#skip call instead.

This comment has been minimized.

@nicolo-ribaudo

nicolo-ribaudo Jul 7, 2017

Member

We can't: if yield's argument contains function.sent, it needs to be traversed.

@hzoo hzoo added the es-proposal label Jul 20, 2017

@jridgewell

This comment has been minimized.

Member

jridgewell commented Jul 20, 2017

Looks like we need a rebase.

@nicolo-ribaudo

This comment has been minimized.

Member

nicolo-ribaudo commented Jul 21, 2017

Sorry, I won't be able to do it until the 27th

@nicolo-ribaudo

This comment has been minimized.

Member

nicolo-ribaudo commented Jul 21, 2017

Nevermind, I managed to rebase

nicolo-ribaudo and others added some commits Jul 6, 2017

Create "babel-helper-wrap-function"
It contains the logic to wrap a function inside a call expression.
It was part of the "babel-helper-remap-async-to-generator" package, but
it is needed to transpile "function.sent"
Create "babel-transform-function-sent"
It transforms the "function.sent" meta property by replacing it with
"yield" and making the generator ignore the first ".next()" call.

@hzoo hzoo merged commit fb9a752 into babel:7.0 Jul 25, 2017

2 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@hzoo

This comment has been minimized.

Member

hzoo commented Jul 25, 2017

Nice work again @nicolo-ribaudo for finishing this up!

@hzoo hzoo referenced this pull request Jul 25, 2017

Merged

update function.sent #15

@nicolo-ribaudo nicolo-ribaudo deleted the nicolo-ribaudo:function-sent branch Jul 25, 2017

@hax

This comment has been minimized.

hax commented Jul 26, 2017

Glad to see this PR!

@nicolo-ribaudo
Your implementation is simpler than https://github.com/hax/babel-plugin-transform-function-sent , but I hope there would be some useful test fixtures which you can pick up 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment