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
Partial application plugin #9474
Partial application plugin #9474
Conversation
Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/10466/ |
(I'm marking this as Low priority until the parser PR is merged - don't worry! 😉 ) |
t.variableDeclaration("const", [ | ||
t.variableDeclarator( | ||
receiverLVal, | ||
t.identifier(receiverRVal(node)), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can use path.scope.push
to generate those vars and avoid the iife:
const g = o.f(?, x, 1);
// becomes
var _receiver, _func, _param, _param2; // generated by scope.push
const g = (_receiver = o, _func = o.f, _param = x, _param2 = 1, _arg => _func.call(_receiver, _arg, _param, _param2);
Btw, there is probably some scope/types helper function that says that 1
is a pure constant value and can thus can be transpiled as
const g = (_receiver = o, _func = o.f, _param = x, _arg => _func.call(_receiver, _arg, _param, 1);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would like to ask for a bit of clarification:
- what is the advantage of not using the iife?
- could you explain how pushing to
scope
creates the declarations for me?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a left a few minor comments that might make some parts of the implementation more compact. Great job man! 👍
} | ||
|
||
/** | ||
* a recursive function that unfolds MemberExpressions within MemberExpression |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe a rewording to a recursive function that unfolds nested MemberExpressions
return true; | ||
} | ||
} | ||
return false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe refactor to something like:
function hasArgumentPlaceholder(node) {
return node.arguments.some(t.isArgumentPlaceholder)
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is*
functions can accept two arguments, so you'll need to wrap it in an arrow to only use one parameter (or use partial application lol)
function receiverRVal(node) { | ||
let rVal = unfold(node).split("."); | ||
rVal.pop(); | ||
rVal = rVal.join("."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can return immediately rVal.join(".");
* @returns {Array<Expression>} | ||
*/ | ||
function unwrapArguments(node) { | ||
const nonPlaceholder = node.arguments.filter( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can also return immediately here.
*/ | ||
function unwrapAllArguments(node, scope) { | ||
const clone = t.cloneNode(node); | ||
clone.arguments.forEach(argument => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe refactor into a .map
*/ | ||
function argsToVarDeclarator(inits, scope) { | ||
let declarator = []; | ||
declarator = inits.map(expr => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could also return immediately here return inits.map...
* @param {Array<Arguments>} args | ||
*/ | ||
function mapNonPlaceholderToLVal(nonPlaceholderDecl, allArgsList) { | ||
const clone = Array.from(allArgsList); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Array.from(allArgsList)
.map(cl => ...);
@nicolo-ribaudo @danielcaldas thank you for all the reviews, I'll try to address them as soon as possible. Meanwhile, I have a question:
What do you think about this solution:
Instead we do this:
or maybe we can get the |
Can we use normal functions? const add1 = add(1, ?);
// ->
var _func, _param;
const add1 = (_func = add, _param = 1, function add(_placeholder) { return _func(_param, _placeholder) }); |
It works. |
t.variableDeclaration("const", [ | ||
t.variableDeclarator( | ||
receiverLVal, | ||
t.identifier(receiverRVal(node)), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand what receiverRVal
(which, btw, doesn't generate a valid identifier since the result can contain .
) is needed for: isn't using node.callee
(and node.callee.property
for functionLVal
) enough?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, it doesn't work:
a.b.fn().c["d"].e(?);
// ->
(() => {
const _receiver = a.b.fn.c.undefined;
const _func = a.b.fn.c.undefined.e;
return _argPlaceholder => _func.call(_receiver, _argPlaceholder);
})();
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, and instead of using newFuncLVal
/newReceiverLVal
you could use path.scope.generateUidIdentifierBasedOnNode(node.callee)
and path.scope.generateUidIdentifierBasedOnNode(node.callee.object)
. It's up to you to decide.
object.get().fn(?);
// ->
(() => {
const _object$get = object.get;
const _object$get$fn = object.get.fn;
return _argPlaceholder => _object$get$fn.call(_object$get, _argPlaceholder);
})();
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, and the previous example should be something like this, otuherwise we might execute some accessors twice:
(() => {
const _object$get = object.get;
const _object$get$fn = _object$get.fn;
return _argPlaceholder => _object$get$fn.call(_object$get, _argPlaceholder);
})();
const h = (_p = p, _p$b = p.b, _param2 = y, _param3 = x, function b(_argPlaceholder2) { | ||
return _p$b.call(_p, 1, _param2, _param3, 2, _argPlaceholder2); | ||
}); | ||
const j = (_a$b$c$d$e = a.b.c.d.e, _a$b$c$d$e$foo = a.b.c.d.e.foo, function foo(_argPlaceholder3) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be
const j = (_a$b$c$d$e = a.b.c.d.e, _a$b$c$d$e$foo = _a$b$c$d$e.foo, ...
because a.b.c.d.e
could be getters which must be accessed exactly once.
* @param {Array<Node>} args | ||
*/ | ||
function mapNonPlaceholderToLVal(nonPlaceholderArgs, allArgsList) { | ||
const clonedArgs = Array.from(allArgsList); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you need to clone this array? You are only cloning the array, not its elements.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you are correct, this is useless. I removed it.
*/ | ||
function mapNonPlaceholderToLVal(nonPlaceholderArgs, allArgsList) { | ||
const clonedArgs = Array.from(allArgsList); | ||
clonedArgs.map(arg => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: you can use forEach
let cloneList = []; | ||
allArgsList.forEach(item => { | ||
if (item.name && item.name.includes("_argPlaceholder")) { | ||
cloneList = cloneList.concat(item); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cloneList.push(item)
node.callee, | ||
); | ||
const receiverLVal = path.scope.generateUidIdentifierBasedOnNode( | ||
node.callee.object, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should only be generated when node.callee
is a member expression.
Does foo(a, ...b, ?); Since the spec doesn't define how it should behave (or I can't find it), we can just throw an error when we find it. Or maybe it uses one of the following semantics? (cc @rbuckton) var _foo, _a, _b;
_foo = foo, _a = a, _b = b, function foo(_arg) { return _foo(_a, ..._b, _arg) }
// or maybe it calls [Symbol.iterator] earlier
_foo = foo, _a = a, _b = [...b], function foo(_arg) { return _foo(_a, ..._b, _arg) } |
@nicolo-ribaudo: Anything usable in a normal function call should be usable here. All evaluation should be eager, so the second example is correct. |
(From the original post)
Don't they already work? |
I added a few tests for 7 and 8. Regarding 5, I think, I sort of understand what "lexical this" means. |
Ignoring partial application for a second, when you call 6 basically means that when you do |
@rbuckton Thank you for the explanation. I think 6 is also addressed. |
remove unnecessary error message and hasPartial function from parseNewArguments add types for PartialExpression Update the tests rename PartialExpression to Partial move Partial from expressions to types and rename to ArgumentPlaceholder add tests for ArgumentPlaceholder in babel-generator rename Partial to ArgumentPlaceholder update the tests remove alias from the type and undo changes in generated folder adds a nice error message better definition for the type auto-generated files update the conditional for allowPlaceholder message and tests update CallExpression definition to accept ArgumentPlaceholder change description clean up indent ArgumentPlaceholder entry and revert unwanted changes
e4b72a9
to
9196d67
Compare
@nicolo-ribaudo one of the builds for travis times out. Would you retrigger it? |
It seems that after the rebase the plugin disappeared? If you need it, you can restore from https://github.com/nicolo-ribaudo/babel/tree/pr/byara/9474-backup (it is 6 days old so commits after #9474 (review) are not included). If you prefer, I suggest using |
9196d67
to
bb7068a
Compare
if (t.isArgumentPlaceholder(arg)) { | ||
const id = scope.generateUid("_argPlaceholder"); | ||
placeholders.push(t.identifier(id)); | ||
args.push(t.cloneNode(t.identifier(id))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to cloneNode
here, t.identifier
returns a new node.
The failing tests are fixed by #9558 |
{ | ||
"name": "@babel/plugin-proposal-partial-application", | ||
"version": "7.2.0", | ||
"description": "Transform pipeline operator into call expressions", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to be updated to match the readme.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎉
Could you rebase (or merge master) this PR, since the parser PR has been merged? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work.
Does this work with awaiting async functions? Can we add a test for this? Thinking about it more, I guess that the partial application does not really touch with async functions as await foo(?)
is nonsense. await foo(?)(1)
could work, but doesn't make much sense. So not sure if a test makes sense.
Also it would be nice to add a test where the result from the call is not assigned but directly chained: foo(?, a)(b).then(()=>{});
Doesn't make much sense either, but I guess it should work.
@@ -0,0 +1,3 @@ | |||
{ | |||
"plugins": ["proposal-partial-application"] | |||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: The options.json
files are the same for all tests as I can see, so the config file could be moved on level up into the general
folder. Then all the fixtures in general
would share the config.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@danez Thanks for the feedback.
I moved options.json
one level up and added a new test.
compare(a, b) { | ||
if (a > b) { | ||
return a; | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit, but this semi seems unnecessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep, unnecessary, I'll remove it.
false, | ||
), | ||
]); | ||
path.replaceWith(finalExpression); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not big deal by any means, but since both of these end with replacing the path with a sequenceExpression
, we could do:
const sequenceParts = [];
if (node.callee.type === "MemberExpression") {
sequenceParts.push(
/* stuff */
);
} else {
sequenceParts.push(
/* stuff */
);
}
path.replaceWith(t.sequenceExpression(sequenceParts));
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good idea! made the necessary changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Benefit of coming late to the party is the code looks awesome already :)
Nice work!
Awesome work! |
* master: (58 commits) Remove dependency on home-or-tmp package (babel#9678) [proposal-object-rest-spread] fix templateLiteral in extractNormalizedKeys (babel#9628) Partial application plugin (babel#9474) Private Static Class Methods (Stage 3) (babel#9446) gulp-uglify@3.0.2 rollup@1.6.0 eslint@5.15.1 jest@24.5.0 regexpu-core@4.5.4 Remove input and length from state (babel#9646) Switch from rollup-stream to rollup and update deps (babel#9640) System modules - Hoist classes like other variables (babel#9639) fix: Don't transpile ES2018 symbol properties (babel#9650) Add WarningsToErrorsPlugin to webpack to avoid missing build problems on CI (babel#9647) Update regexpu-core dependency (babel#9642) Add placeholders support to @babel/types and @babel/generator (babel#9542) Generate plugins file Make babel-standalone an ESModule and enable flow (babel#9025) Reorganize token types and use a map for them (babel#9645) [TS] Allow context type annotation on getters/setters (babel#9641) ...
This PR addresses the proposal regarding Partial Application plugin.
The syntax rules that was suggested in the proposal to be covered:
I'm not 100% sure about 5, 6, 7 and 8 and how to address them.