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

Add experimental support for ES7 function bind. #1518

Merged
merged 5 commits into from May 14, 2015

Conversation

Projects
None yet
4 participants
@RReverser
Member

RReverser commented May 13, 2015

Please review and let me know about possible issues (syntax is somewhat tricky so I could miss some edge cases). /cc @sebmck @zenparsing

Please note that I implemented it as if tc39/proposal-bind-operator#4 would take place.

Closes #1287.

@RReverser RReverser referenced this pull request May 13, 2015

Closed

ES7 function bind #1287

Show outdated Hide outdated src/babel/transformation/transformers/es7/function-bind.js
stage: 0
};
var CALL = t.identifier("call");

This comment has been minimized.

@kittens

kittens May 13, 2015

Member

The AST is currently considered to be highly mutable so this isn't really safe to do. ie. properties could be added to it that would cause race conditions when ran multiple times.

@kittens

kittens May 13, 2015

Member

The AST is currently considered to be highly mutable so this isn't really safe to do. ie. properties could be added to it that would cause race conditions when ran multiple times.

This comment has been minimized.

@RReverser

RReverser May 13, 2015

Member

Np, will fix now.

@RReverser

RReverser May 13, 2015

Member

Np, will fix now.

Show outdated Hide outdated src/babel/transformation/transformers/es7/function-bind.js
var bindExpr = node.callee;
if (!t.isBindExpression(bindExpr)) return;
var bindCtx = inferBindContext(bindExpr, scope);
node.callee = t.memberExpression(bindExpr.callee, CALL, false);

This comment has been minimized.

@kittens

kittens May 13, 2015

Member

computed property is optional with the Babel node builders 💃

@kittens

kittens May 13, 2015

Member

computed property is optional with the Babel node builders 💃

This comment has been minimized.

@RReverser

RReverser May 13, 2015

Member

Oh, cool - I'm just used to restrictive ast-types too much. Will change.

@RReverser

RReverser May 13, 2015

Member

Oh, cool - I'm just used to restrictive ast-types too much. Will change.

@@ -133,6 +133,12 @@ export function AssignmentExpression(node, print) {
print(node.right);
}
export function BindExpression(node, print) {

This comment has been minimized.

@kittens

kittens May 13, 2015

Member

Missing some generation tests to go along.

@kittens

kittens May 13, 2015

Member

Missing some generation tests to go along.

This comment has been minimized.

@RReverser

RReverser May 13, 2015

Member

Do we really need them for experimental features?
I'll add some now.

@RReverser

RReverser May 13, 2015

Member

Do we really need them for experimental features?
I'll add some now.

This comment has been minimized.

@kittens

kittens May 13, 2015

Member

Actually, there's no way this function could possibly be hit... If the plugin is off then it's a syntax error if it's on then it's transformed away.

@kittens

kittens May 13, 2015

Member

Actually, there's no way this function could possibly be hit... If the plugin is off then it's a syntax error if it's on then it's transformed away.

This comment has been minimized.

@RReverser

RReverser May 13, 2015

Member

Yeah, that's what I thought, too (unless AST is manually passed to Babel's generator).

@RReverser

RReverser May 13, 2015

Member

Yeah, that's what I thought, too (unless AST is manually passed to Babel's generator).

@RReverser RReverser changed the title from Add experimental support for ES7 function bind. (issue #1287) to Add experimental support for ES7 function bind. May 13, 2015

Show outdated Hide outdated src/acorn/src/expression.js
@@ -214,7 +214,12 @@ pp.parseExprSubscripts = function(refShorthandDefaultPos) {
}
pp.parseSubscripts = function(base, start, noCalls) {
if (this.eat(tt.dot)) {
if (this.eat(tt.doubleColon)) {

This comment has been minimized.

@zenparsing

zenparsing May 13, 2015

Contributor

In the following case:

new X::y;

We want (new X)::y rather than new (X::y). Agree?

Similarly, for:

x :: y :: z 

We want (x :: y) :: z rather than x :: (y :: z)

So I think we should only eat :: if noCalls is false.

@zenparsing

zenparsing May 13, 2015

Contributor

In the following case:

new X::y;

We want (new X)::y rather than new (X::y). Agree?

Similarly, for:

x :: y :: z 

We want (x :: y) :: z rather than x :: (y :: z)

So I think we should only eat :: if noCalls is false.

This comment has been minimized.

@RReverser

RReverser May 13, 2015

Member

Thanks Kevin!

As for the first case - makes sense to me.

As for the second one - I don't see any benefits from such construct at all since second .bind anyway doesn't change context of prebound function, so it's kinda convoluted example IMO.

@RReverser

RReverser May 13, 2015

Member

Thanks Kevin!

As for the first case - makes sense to me.

As for the second one - I don't see any benefits from such construct at all since second .bind anyway doesn't change context of prebound function, so it's kinda convoluted example IMO.

This comment has been minimized.

@RReverser

RReverser May 14, 2015

Member

Fixed.

@RReverser
Show outdated Hide outdated test/core/fixtures/transformation/es7.function-bind/call/expected.js
ns.obj.func.call(ctx);
(_context = ns.obj).func.call(_context);
ns.obj1.func.call(ns.obj2);

This comment has been minimized.

@zenparsing

zenparsing May 13, 2015

Contributor

It just occurred to me that in the following case:

ns.obj2.prop::ns.obj1.func()

If we translate to

ns.obj1.func.call(ns.obj2.prop);

we could have some order-of-operations issues where obj2 needs to be looked up before obj1. It might need to translate to something like:

(_context = ns.obj2, ns).obj1.func.call(_context.prop);

esdown gets this wrong at the moment.

@zenparsing

zenparsing May 13, 2015

Contributor

It just occurred to me that in the following case:

ns.obj2.prop::ns.obj1.func()

If we translate to

ns.obj1.func.call(ns.obj2.prop);

we could have some order-of-operations issues where obj2 needs to be looked up before obj1. It might need to translate to something like:

(_context = ns.obj2, ns).obj1.func.call(_context.prop);

esdown gets this wrong at the moment.

This comment has been minimized.

@RReverser

RReverser May 13, 2015

Member

I don't really think this matters in 99% of cases unless someone specifically sets getter, for example, in order to log accesses to functions, do you?

@RReverser

RReverser May 13, 2015

Member

I don't really think this matters in 99% of cases unless someone specifically sets getter, for example, in order to log accesses to functions, do you?

This comment has been minimized.

@kittens

kittens May 13, 2015

Member

It's still very important to get correct semantics. No point going half way. You'll also have to memoise single identifier references that aren't bound (generateMemoisedReference will be of use here) since referencing it may trigger a globally set getter.

@kittens

kittens May 13, 2015

Member

It's still very important to get correct semantics. No point going half way. You'll also have to memoise single identifier references that aren't bound (generateMemoisedReference will be of use here) since referencing it may trigger a globally set getter.

This comment has been minimized.

@RReverser

RReverser May 14, 2015

Member

@zenparsing

It might need to translate to something like:

(_context = ns.obj2, ns).obj1.func.call(_context.prop);

Shouldn't it be

(_context = ns.obj2.prop, ns).obj1.func.call(_context);

instead so that obj2.prop is also accessed earlier than obj1?

@RReverser

RReverser May 14, 2015

Member

@zenparsing

It might need to translate to something like:

(_context = ns.obj2, ns).obj1.func.call(_context.prop);

Shouldn't it be

(_context = ns.obj2.prop, ns).obj1.func.call(_context);

instead so that obj2.prop is also accessed earlier than obj1?

This comment has been minimized.

@RReverser

RReverser May 14, 2015

Member

Updated PR, added execution test to ensure order.

@RReverser

RReverser May 14, 2015

Member

Updated PR, added execution test to ensure order.

This comment has been minimized.

@RReverser

RReverser May 14, 2015

Member

You'll also have to memoise single identifier references that aren't bound (generateMemoisedReference will be of use here) since referencing it may trigger a globally set getter.

@sebmck I just used same memoisation technique as for any other case - scoped identifier is rather specific case optimization that can be done later if needed. For now, it's more interesting to get correct behavior and feedback from users asap so that we could either get this operator in ES7 or not.

@RReverser

RReverser May 14, 2015

Member

You'll also have to memoise single identifier references that aren't bound (generateMemoisedReference will be of use here) since referencing it may trigger a globally set getter.

@sebmck I just used same memoisation technique as for any other case - scoped identifier is rather specific case optimization that can be done later if needed. For now, it's more interesting to get correct behavior and feedback from users asap so that we could either get this operator in ES7 or not.

(_context = ctx, ns.obj.func).call(_context);
(_context = ns.obj).func.call(_context);
(_context = ns.obj2, ns.obj1.func).call(_context);

This comment has been minimized.

@zenparsing

zenparsing May 14, 2015

Contributor

@RReverser You were right, and this looks good to me.

@zenparsing

zenparsing May 14, 2015

Contributor

@RReverser You were right, and this looks good to me.

This comment has been minimized.

@RReverser

RReverser May 14, 2015

Member

@zenparsing Thanks for confirmation and review in general!

@RReverser

RReverser May 14, 2015

Member

@zenparsing Thanks for confirmation and review in general!

@RReverser

This comment has been minimized.

Show comment
Hide comment
@RReverser

RReverser May 14, 2015

Member

@sebmck Can it be merged now?

Member

RReverser commented May 14, 2015

@sebmck Can it be merged now?

stage: 0
};
function getTempId(scope) {

This comment has been minimized.

@kittens

kittens May 14, 2015

Member

I think this UID caching might introduce race conditions with nested binding expressions.

@kittens

kittens May 14, 2015

Member

I think this UID caching might introduce race conditions with nested binding expressions.

This comment has been minimized.

@RReverser

RReverser May 14, 2015

Member

Can you provide any example?

@RReverser

RReverser May 14, 2015

Member

Can you provide any example?

This comment has been minimized.

@RReverser

RReverser May 14, 2015

Member

(I was also worried about that so specifically added complex tests with nested binding expressions - both transformation and execution seem to be fine)

@RReverser

RReverser May 14, 2015

Member

(I was also worried about that so specifically added complex tests with nested binding expressions - both transformation and execution seem to be fine)

This comment has been minimized.

@kittens

kittens May 14, 2015

Member

Yeah, looks like the scoping stuff covers it as the order of evaluation will always be consistent.

@kittens

kittens May 14, 2015

Member

Yeah, looks like the scoping stuff covers it as the order of evaluation will always be consistent.

This comment has been minimized.

@RReverser

RReverser May 14, 2015

Member

Yeah - and sequence of operations is always like:

  1. Store context to _context.
  2. Get .bind / .call property.
  3. Call it with _context as first argument.

So, according to semantics, nothing can happen between storing and using _context.

@RReverser

RReverser May 14, 2015

Member

Yeah - and sequence of operations is always like:

  1. Store context to _context.
  2. Get .bind / .call property.
  3. Call it with _context as first argument.

So, according to semantics, nothing can happen between storing and using _context.

Show outdated Hide outdated src/babel/transformation/transformers/es7/function-bind.js
function inferBindContext(bind, scope) {
var tempId = getTempId(scope);
if (!bind.object) {
bind.callee.object = t.assignmentExpression("=", tempId, bind.callee.object);

This comment has been minimized.

@kittens

kittens May 14, 2015

Member

Flip this from being a negation and this should be GTM.

@kittens

kittens May 14, 2015

Member

Flip this from being a negation and this should be GTM.

This comment has been minimized.

@RReverser

RReverser May 14, 2015

Member

Ok.

@RReverser

This comment has been minimized.

@RReverser

RReverser May 14, 2015

Member

Done.

@RReverser

kittens added a commit that referenced this pull request May 14, 2015

Merge pull request #1518 from babel/es7.functionBind
Add experimental support for ES7 function bind.

@kittens kittens merged commit 7407b37 into master May 14, 2015

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@kittens kittens deleted the es7.functionBind branch May 14, 2015

@kittens

This comment has been minimized.

Show comment
Hide comment
@kittens

kittens May 14, 2015

Member

Awesome, thanks! I've added in handling for "static" contexts in commit 724bf52. The scope API methods will need to be refactored/renamed as they're a bit crap. I'll hopefully push this out later tonight. A short blog post would also be awesome if anyone wants to volunteer.

Member

kittens commented May 14, 2015

Awesome, thanks! I've added in handling for "static" contexts in commit 724bf52. The scope API methods will need to be refactored/renamed as they're a bit crap. I'll hopefully push this out later tonight. A short blog post would also be awesome if anyone wants to volunteer.

@UltCombo

This comment has been minimized.

Show comment
Hide comment
@UltCombo

UltCombo May 14, 2015

Awesome work, thanks @RReverser and @sebmck. 😄

UltCombo commented May 14, 2015

Awesome work, thanks @RReverser and @sebmck. 😄

@RReverser

This comment has been minimized.

Show comment
Hide comment
@RReverser

RReverser May 14, 2015

Member

@sebmck Cool, thanks. I could do the blog post - but never did them before here, so need some pointers (or maybe @thejameskyle or @zenparsing want to do it).

Member

RReverser commented May 14, 2015

@sebmck Cool, thanks. I could do the blog post - but never did them before here, so need some pointers (or maybe @thejameskyle or @zenparsing want to do it).

@RReverser

This comment has been minimized.

Show comment
Hide comment
@RReverser

RReverser May 14, 2015

Member

@sebmck Also, maybe better to release this before writing in the blog so people could actually play with it?

Member

RReverser commented May 14, 2015

@sebmck Also, maybe better to release this before writing in the blog so people could actually play with it?

@kittens

This comment has been minimized.

Show comment
Hide comment
@kittens

kittens May 14, 2015

Member

Released as of 5.4.0 💃

Member

kittens commented May 14, 2015

Released as of 5.4.0 💃

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