Skip to content
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

WIP: Transform for F# Pipeline #1

Closed
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
@@ -0,0 +1,27 @@
import { types as t } from "@babel/core";
import maybeOptimizePipelineSequence from "./maybeOptimizePipelineSequence";

const fsharpVisitor = {
BinaryExpression(path) {
const { scope } = path;
const { node } = path;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: these two statements can be merged

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in ecbb465

const { operator, left, right } = node;
if (operator !== "|>") return;

const placeholder = scope.generateUidIdentifierBasedOnNode(left);
scope.push({ id: placeholder });

const call =
right.type === "AwaitExpression"
? t.awaitExpression(t.cloneNode(placeholder))
: t.callExpression(right, [t.cloneNode(placeholder)]);
const sequence = t.sequenceExpression([
t.assignmentExpression("=", t.cloneNode(placeholder), left),
call,
]);
path.replaceWith(sequence);
maybeOptimizePipelineSequence(path);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of creating a temporary path for the sequence expression and then optimizing it away, would it be possible to do something like this?

path.replaceWith(
  buildOptimizedSequenceExpression({
    assign: t.assignmentExpression("=", t.cloneNode(placeholder), left),
    call,
    path,
  })
);

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I started with something similar to this but ended up passing the path in because I needed it for the scope.rename call. I think your approach is doable and would be cleaner. I'll give it a try.

},
};

export default fsharpVisitor;
2 changes: 2 additions & 0 deletions packages/babel-plugin-proposal-pipeline-operator/src/index.js
Expand Up @@ -2,10 +2,12 @@ import { declare } from "@babel/helper-plugin-utils";
import syntaxPipelineOperator from "@babel/plugin-syntax-pipeline-operator";
import minimalVisitor from "./minimalVisitor";
import smartVisitor from "./smartVisitor";
import fsharpVisitor from "./fsharpVisitor";

const visitorsPerProposal = {
minimal: minimalVisitor,
smart: smartVisitor,
fsharp: fsharpVisitor,
};

export default declare((api, options) => {
Expand Down
@@ -0,0 +1,51 @@
import { types as t } from "@babel/core";

// tries to optimize sequence expressions in the format
// (a = b, ((c) => d + e)(a))
// to
// (a = b, a + e)
const maybeOptimizePipelineSequence = path => {
const [assignNode, callNode] = path.node.expressions;
const { left: placeholderNode, right: pipelineLeft } = assignNode;
const { callee: calledExpression } = callNode;

let optimizeArrow =
t.isArrowFunctionExpression(calledExpression) &&
t.isExpression(calledExpression.body) &&
!calledExpression.async &&
!calledExpression.generator;
let param;

if (optimizeArrow) {
const { params } = calledExpression;
if (params.length === 1 && t.isIdentifier(params[0])) {
param = params[0];
} else if (params.length > 0) {
optimizeArrow = false;
}
} else if (t.isIdentifier(calledExpression, { name: "eval" })) {
const evalSequence = t.sequenceExpression([
t.numericLiteral(0),
calledExpression,
]);

path.get("expressions.1.callee").replaceWith(evalSequence);
}

if (optimizeArrow && !param) {
// Arrow function with 0 arguments
path.replaceWith(
t.sequenceExpression([pipelineLeft, calledExpression.body]),
);
return;
}

if (param) {
path
.get("expressions.1.callee.body")
.scope.rename(param.name, placeholderNode.name);
path.get("expressions.1").replaceWith(calledExpression.body);
}
};

export default maybeOptimizePipelineSequence;
@@ -1,52 +1,24 @@
import { types as t } from "@babel/core";
import maybeOptimizePipelineSequence from "./maybeOptimizePipelineSequence";

const minimalVisitor = {
BinaryExpression(path) {
const { scope } = path;
const { node } = path;
const { operator, left } = node;
let { right } = node;
const { operator, left, right } = node;
if (operator !== "|>") return;

let optimizeArrow =
t.isArrowFunctionExpression(right) &&
t.isExpression(right.body) &&
!right.async &&
!right.generator;
let param;

if (optimizeArrow) {
const { params } = right;
if (params.length === 1 && t.isIdentifier(params[0])) {
param = params[0];
} else if (params.length > 0) {
optimizeArrow = false;
}
} else if (t.isIdentifier(right, { name: "eval" })) {
right = t.sequenceExpression([t.numericLiteral(0), right]);
}

if (optimizeArrow && !param) {
// Arrow function with 0 arguments
path.replaceWith(t.sequenceExpression([left, right.body]));
return;
}

const placeholder = scope.generateUidIdentifierBasedOnNode(param || left);
const placeholder = scope.generateUidIdentifierBasedOnNode(left);
scope.push({ id: placeholder });
if (param) {
path.get("right").scope.rename(param.name, placeholder.name);
}

const call = optimizeArrow
? right.body
: t.callExpression(right, [t.cloneNode(placeholder)]);
const call = t.callExpression(right, [t.cloneNode(placeholder)]);
path.replaceWith(
t.sequenceExpression([
t.assignmentExpression("=", t.cloneNode(placeholder), left),
call,
]),
);
maybeOptimizePipelineSequence(path);
},
};

Expand Down
@@ -0,0 +1,34 @@
const y = 2;

const f = (x) => (x
|> (y) => y + 1)
|> (z) => z * y

const _f = (x) => x
|> (y) => y + 1
|> (z) => z * y

const g = (x) => x
|> (y) => (
y + 1
|> (z) => z * y
)

const _g = (x) => x
|> (y => (
y + 1
|> (z) => z * y)
)

const __g = (x) => x
|> (y => { return (
y + 1
|> (z) => z * y);
}
)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All this random indentation makes it really hard to review this file 😛

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is hard for me too :-p

I'm still getting the hang of how to indent pipelines. I'll see what I can do.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please see 4811f08 and let me know what you think.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah they look better. As a general rule, I think that if the pipeline body is multiline, ) should aligned with the | of |> and there should be a line break after (. SImilar to

return (
  x => y
);

Let's hope Prettier implements them soon 😛


expect( f(1)).toBe(4);
expect( _f(1)).toBe(4);
expect( g(1)).toBe(2);
expect( _g(1)).toBe(2);
expect(__g(1)).toBe(2);
@@ -0,0 +1,14 @@
var result = [5,10]
|> (_ => _.map(x => x * 2))
|> (_ => _.reduce( (a,b) => a + b ))
|> (sum => sum + 1)

expect(result).toBe(31);


var inc = (x) => x + 1;
var double = (x) => x * 2;

var result2 = [4, 9].map( x => x |> inc |> double )

expect(result2).toEqual([10, 20]);
@@ -0,0 +1,14 @@
var result = [5,10]
|> (_ => _.map(x => x * 2))
|> (_ => _.reduce( (a,b) => a + b ))
|> (sum => sum + 1)

expect(result).toBe(31);
thiagoarrais marked this conversation as resolved.
Show resolved Hide resolved


var inc = (x) => x + 1;
var double = (x) => x * 2;

var result2 = [4, 9].map( x => x |> inc |> double )

expect(result2).toEqual([10, 20]);
@@ -0,0 +1,15 @@
var _ref, _ref2, _ref3;

var result = (_ref = (_ref2 = (_ref3 = [5, 10], _ref3.map(x => x * 2)), _ref2.reduce((a, b) => a + b)), _ref + 1);
expect(result).toBe(31);

var inc = x => x + 1;

var double = x => x * 2;

var result2 = [4, 9].map(x => {
var _ref4, _x;

return _ref4 = (_x = x, inc(_x)), double(_ref4);
});
expect(result2).toEqual([10, 20]);
@@ -0,0 +1,13 @@
const triple = (x) => x * 3;

async function myFunction(n) {
return n
|> Math.abs
|> Promise.resolve
|> await
|> triple;
}

return myFunction(-7).then(result => {
expect(result).toBe(21);
});
@@ -0,0 +1,6 @@
async function myFunction(n) {
return n
|> Math.abs
|> Promise.resolve
|> await;
}
thiagoarrais marked this conversation as resolved.
Show resolved Hide resolved
@@ -0,0 +1,9 @@
{
"plugins": [
["proposal-pipeline-operator", { "proposal": "fsharp" }]
],
"parserOpts": {
"allowReturnOutsideFunction": true
},
"minNodeVersion": "8.0.0"
}
@@ -0,0 +1,5 @@
async function myFunction(n) {
var _ref, _ref2, _n;

return _ref = (_ref2 = (_n = n, Math.abs(_n)), Promise.resolve(_ref2)), await _ref;
}
@@ -0,0 +1,3 @@
var inc = (x) => x + 1

expect(10 |> inc).toBe(11);
@@ -0,0 +1,3 @@
var inc = (x) => x + 1

expect(10 |> inc).toBe(11);
@@ -0,0 +1,5 @@
var _;

var inc = x => x + 1;

expect((_ = 10, inc(_))).toBe(11);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If at least one of the following conditions is true, we could optimize it to inc(10):

  • the right expression is an identifier and it is declared
  • the left expression is immutable

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. Will do.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This also applies to minimal. Right?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added this in 63b7c04. I'm marking the PR as WIP, though. I'm uncomfortable with buildOptimizedSequenceExpression now no longer returning a sequence expression. And it also does some AST deconstructing that I would like to avoid.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you prefer, we can work on this optimization in a separate PR, and keep the current one as it was.

@@ -0,0 +1,7 @@
(function() {
'use strict';
var result = '(function() { return this; })()'
|> eval;

expect(result).not.toBeUndefined();
})();
@@ -0,0 +1,7 @@
(function() {
'use strict';
var result = '(function() { return this; })()'
|> eval;

expect(result).not.toBeUndefined();
})();
@@ -0,0 +1,8 @@
(function () {
'use strict';

var _functionReturn;

var result = (_functionReturn = '(function() { return this; })()', (0, eval)(_functionReturn));
expect(result).not.toBeUndefined();
})();
@@ -0,0 +1,5 @@
var array = [10,20,30];

var last = array |> (a => a[a.length-1]);

expect(last).toBe(30);
@@ -0,0 +1,5 @@
var _array;

var array = [10, 20, 30];
var last = (_array = array, _array[_array.length - 1]);
expect(last).toBe(30);
@@ -0,0 +1,8 @@
var a = 1,
b = 2,
c = 3;
var result = a
|> (() => b)
|> (() => c);

expect(result).toBe(c);
@@ -0,0 +1,8 @@
var a = 1,
b = 2,
c = 3;
var result = a
|> (() => b)
|> (() => c);

expect(result).toBe(c);
@@ -0,0 +1,7 @@
var _ref, _a;

var a = 1,
b = 2,
c = 3;
var result = ((a, b), c);
expect(result).toBe(c);
@@ -0,0 +1,8 @@
{
"plugins": [
["proposal-pipeline-operator", { "proposal": "fsharp" }]
],
"parserOpts": {
"allowReturnOutsideFunction": true
}
}
@@ -0,0 +1,20 @@
const y = 2;
const f = (x) => x
|> (y => y + 1)
|> (z => z * y)

const g = (x) => x
|> (y =>
y + 1
|> (z => z * y)
)

const h = (x) => x
|> (y => (
y + 1
|> (z => z * y)
))

expect(f(1)).toBe(4);
expect(g(1)).toBe(2);
expect(h(1)).toBe(2);
@@ -0,0 +1,20 @@
const y = 2;
const f = (x) => x
|> (y => y + 1)
|> (z => z * y)

const g = (x) => x
|> (y =>
y + 1
|> (z => z * y)
)

const h = (x) => x
|> (y => (
y + 1
|> (z => z * y)
))

expect(f(1)).toBe(4);
expect(g(1)).toBe(2);
expect(h(1)).toBe(2);