WIP: Transform for F# Pipeline #1

thiagoarrais · 2019-03-22T16:32:38Z

Q	A
Fixed Issues?
Patch: Bug Fix?
Major: Breaking Change?
Minor: New Feature?	Yes
Tests Added + Pass?	Yes
Documentation PR Link
Any Dependency Changes?	No
License	MIT

This is a transform for the F# proposal of the pipeline operator based on @mAAdhaTTah's parsing code.

It is basically the minimal visitor with support for paramless await. The most relevant lines are fsharpVisitor.js/44-47

TODO:

dedup optimization logic between minimal and fsharp
integration tests for paren-less arrow functions
eliminate temp _ref vars from optimized outputs
the name buildOptmizedSequenceExpression is misleading. it returns call expressions sometimes. fix that.

mAAdhaTTah · 2019-03-22T17:16:06Z

Arrow functions don't require parens, so we def need tests to make sure those are transpiled correctly as well.

packages/babel-plugin-proposal-pipeline-operator/src/fsharpVisitor.js

packages/babel-plugin-proposal-pipeline-operator/test/fixtures/fsharp/await/input.js

thiagoarrais · 2019-03-25T16:22:33Z

Arrow functions don't require parens, so we def need tests to make sure those are transpiled correctly as well.

Yeah, that would be nice. I'll add some tests on that vein for integration testing purposes. But this is more of a parser concern, in my opinion. It would be nice to cover that more thoroughly in the parser tests.

mAAdhaTTah · 2019-03-25T16:44:51Z

It would be nice to cover that more thoroughly in the parser tests.

I think we've got it covered there, but I'd definitely be more comfortable if I saw the full transform here. We can then address that in the parser if we need to.

thiagoarrais · 2019-03-25T17:13:41Z

Trying to write parenless tests. What should g(1) be here? 2 or 4?

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

mAAdhaTTah · 2019-03-25T17:59:23Z

Yeah, Nicolo pointed out the same issue on the parser PR. I think that's a bug I'll need to address; in the last arrow, y should not be in scope.

mAAdhaTTah · 2019-03-25T18:04:47Z

Oh wait, you're writing this with parens... so that's a different issue.

mAAdhaTTah · 2019-03-25T18:06:06Z

Ping me on slack to discuss this, probably easier that way.

thiagoarrais · 2019-03-25T19:38:42Z

Nicolo pointed out the same issue on the parser PR.

Yeah. I stole Nicolo's code from there.

😜

thiagoarrais · 2019-03-25T19:40:32Z

Ping me on slack to discuss this

For anyone following this, we've discussed it and came to the conclusion that g(1) should be 2.

Inludes support for optimizing single-parameter arrow functions

thiagoarrais · 2019-04-17T00:28:41Z

Updated to latest parser and did the initial clean up. I will port this PR to the main babel repo once the parser PR gets merged.

I still plan on deduping the arrow function optimization logic between this and minimal.

I think we've got it covered there, but I'd definitely be more comfortable if I saw the full transform here. We can then address that in the parser if we need to.

Regarding that, do you think the added test is sufficient to address the concerns you raised?

mAAdhaTTah · 2019-04-17T02:58:28Z

Yeah, this looks great! Thank you so much for working on this.

This is finally happeninggg! Makes me really excited for the possibilities of the proposal itself.

thiagoarrais · 2019-04-24T16:23:59Z

Deduped optimization logic between fsharp and minimal. Please let me know what you think.

thiagoarrais · 2019-04-24T16:27:33Z

Oops... Fat-fingered the PR branch. Reopening...

mAAdhaTTah · 2019-04-24T18:25:57Z

LGTM! Looking forward to getting this all in place.

nicolo-ribaudo · 2019-04-26T22:01:06Z

packages/babel-plugin-proposal-pipeline-operator/src/fsharpVisitor.js

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


Nit: these two statements can be merged

Done in ecbb465

nicolo-ribaudo · 2019-04-26T22:07:52Z

packages/babel-plugin-proposal-pipeline-operator/src/fsharpVisitor.js

+      call,
+    ]);
+    path.replaceWith(sequence);
+    maybeOptimizePipelineSequence(path);


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, }) );

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.

nicolo-ribaudo · 2019-04-26T22:09:33Z

...bel-plugin-proposal-pipeline-operator/test/fixtures/fsharp/arrow-functions-parenless/exec.js

+    y + 1
+    |> (z) => z * y);
+    }
+  )


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

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.

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

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 😛

packages/babel-plugin-proposal-pipeline-operator/test/fixtures/fsharp/arrow-functions/input.js

nicolo-ribaudo · 2019-04-26T22:15:01Z

packages/babel-plugin-proposal-pipeline-operator/test/fixtures/fsharp/basic/output.js

+
+var inc = x => x + 1;
+
+expect((_ = 10, inc(_))).toBe(11);


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

Agreed. Will do.

This also applies to minimal. Right?

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.

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

thiagoarrais · 2019-05-15T14:23:05Z

This was moved to the main repo as babel#9984

mAAdhaTTah reviewed Mar 22, 2019

View reviewed changes

packages/babel-plugin-proposal-pipeline-operator/src/fsharpVisitor.js Outdated Show resolved Hide resolved

mAAdhaTTah reviewed Mar 22, 2019

View reviewed changes

packages/babel-plugin-proposal-pipeline-operator/test/fixtures/fsharp/await/input.js Show resolved Hide resolved

thiagoarrais force-pushed the transform-f#-pipeline branch from 6738d16 to 523e18c Compare April 15, 2019 03:07

Transform for F#-style await

bbb8a14

Inludes support for optimizing single-parameter arrow functions

thiagoarrais force-pushed the transform-f#-pipeline branch from 523e18c to bbb8a14 Compare April 17, 2019 00:20

thiagoarrais added 2 commits April 23, 2019 13:20

Extracts arrow function optimization to its own function

573fe40

Share optimizing code between fsharp and minimal

34c4d70

thiagoarrais closed this Apr 24, 2019

thiagoarrais deleted the transform-f#-pipeline branch April 24, 2019 16:26

thiagoarrais restored the transform-f#-pipeline branch April 24, 2019 16:26

thiagoarrais reopened this Apr 24, 2019

nicolo-ribaudo reviewed Apr 26, 2019

View reviewed changes

thiagoarrais added 4 commits April 29, 2019 13:35

Better indentation

4811f08

merge object patterns

ecbb465

simpler optimization function as per nicolo's suggestion

f50bd7a

Optimize simple function calls

63b7c04

thiagoarrais changed the title ~~Transform for F# Pipeline~~ WIP: Transform for F# Pipeline May 9, 2019

thiagoarrais mentioned this pull request May 15, 2019

Transform for F# Pipeline babel/babel#9984

Merged

thiagoarrais closed this May 15, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: Transform for F# Pipeline #1

WIP: Transform for F# Pipeline #1

thiagoarrais commented Mar 22, 2019 •

edited

mAAdhaTTah commented Mar 22, 2019

thiagoarrais commented Mar 25, 2019

mAAdhaTTah commented Mar 25, 2019

thiagoarrais commented Mar 25, 2019 •

edited

mAAdhaTTah commented Mar 25, 2019

mAAdhaTTah commented Mar 25, 2019

mAAdhaTTah commented Mar 25, 2019

thiagoarrais commented Mar 25, 2019

thiagoarrais commented Mar 25, 2019

thiagoarrais commented Apr 17, 2019

mAAdhaTTah commented Apr 17, 2019

thiagoarrais commented Apr 24, 2019

thiagoarrais commented Apr 24, 2019

mAAdhaTTah commented Apr 24, 2019

nicolo-ribaudo Apr 26, 2019

thiagoarrais Apr 29, 2019

nicolo-ribaudo Apr 26, 2019

thiagoarrais Apr 29, 2019

nicolo-ribaudo Apr 26, 2019

thiagoarrais Apr 29, 2019

thiagoarrais Apr 29, 2019

nicolo-ribaudo Apr 29, 2019 •

edited

nicolo-ribaudo Apr 26, 2019

thiagoarrais Apr 29, 2019

thiagoarrais May 3, 2019

thiagoarrais May 9, 2019

nicolo-ribaudo May 13, 2019

thiagoarrais commented May 15, 2019

WIP: Transform for F# Pipeline #1

WIP: Transform for F# Pipeline #1

Conversation

thiagoarrais commented Mar 22, 2019 • edited

mAAdhaTTah commented Mar 22, 2019

thiagoarrais commented Mar 25, 2019

mAAdhaTTah commented Mar 25, 2019

thiagoarrais commented Mar 25, 2019 • edited

mAAdhaTTah commented Mar 25, 2019

mAAdhaTTah commented Mar 25, 2019

mAAdhaTTah commented Mar 25, 2019

thiagoarrais commented Mar 25, 2019

thiagoarrais commented Mar 25, 2019

thiagoarrais commented Apr 17, 2019

mAAdhaTTah commented Apr 17, 2019

thiagoarrais commented Apr 24, 2019

thiagoarrais commented Apr 24, 2019

mAAdhaTTah commented Apr 24, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolo-ribaudo Apr 29, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thiagoarrais commented May 15, 2019

thiagoarrais commented Mar 22, 2019 •

edited

thiagoarrais commented Mar 25, 2019 •

edited

nicolo-ribaudo Apr 29, 2019 •

edited