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

Implement f# pipeline in parser #9450

Conversation

@mAAdhaTTah
Copy link
Contributor

commented Feb 3, 2019

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

This is still very much "Work-in-Progress", but since I mostly don't know what I'm doing, I wanted to get this up early so I can get some feedback on alternate approaches, suggestions on the AST, etc. The two main things left are:

  • await parsing
  • arrow body parsing

cc @js-choi @littledan

@mAAdhaTTah mAAdhaTTah force-pushed the mAAdhaTTah:implement-f#-pipeline-in-parser branch from 0ce397e to 2fd61a8 Feb 3, 2019

@babel-bot

This comment has been minimized.

Copy link
Collaborator

commented Feb 3, 2019

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/10691/

@mAAdhaTTah mAAdhaTTah force-pushed the mAAdhaTTah:implement-f#-pipeline-in-parser branch from 2fd61a8 to b08f341 Feb 24, 2019

@mAAdhaTTah

This comment has been minimized.

Copy link
Contributor Author

commented Feb 24, 2019

Just pushed solo await parsing in the pipeline, courtesy of @thiagoarrais. Want to confirm the tests pass, but will also be pushing some tests so Thiago & I can collaborate on them.

@mAAdhaTTah

This comment has been minimized.

Copy link
Contributor Author

commented Feb 24, 2019

Actually may have gotten parenless arrow functions parsing. cc @littledan & @nicolo-ribaudo Definitely interested in suggested tests to add here / what cases I haven't covered.

mAAdhaTTah added 2 commits Feb 24, 2019
Add tests for parens around args to arrow funcs
Remove the lookahead check and just set that a potential arrow function
is there.

@mAAdhaTTah mAAdhaTTah force-pushed the mAAdhaTTah:implement-f#-pipeline-in-parser branch from 2857ab2 to 7b75aa4 Feb 24, 2019

@mAAdhaTTah

This comment has been minimized.

Copy link
Contributor Author

commented Feb 24, 2019

Note that this AST is a modified implementation of what I inquired about here. The idea is to treat the PipelineHead & PipelineBody differently in regards to how they interact with arrow bodies. The operator will will terminate an arrow body when it's within a PipelineBody but not within a PipelineHead, allowing us to write:

x => x |> y => y + 1 |> z => z * 2

which parses as (fully paren'd):

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

I don't know yet if the fact that it's feasible in babel means it's feasible from a spec perspective. Will need those with more expertise to weigh in.

Also note that accepting F# w/ parenless arrows may conflict with the movements toward making Smart Pipelines a superset of F# Pipelines.

@mAAdhaTTah mAAdhaTTah changed the title [WIP] Implement f# pipeline in parser Implement f# pipeline in parser Feb 24, 2019

Ban await f in F# pipelines
Needs to be solo or wrapped in parens. Also update function name to
test in `await` tests so we can use `await f` in the ban test name.

@nicolo-ribaudo nicolo-ribaudo self-requested a review Feb 25, 2019

@nicolo-ribaudo
Copy link
Member

left a comment

Question: why do we need the PipelineHead and PipelineBody nodes?

Can't a |> b just be BinaryOperator(op: |>, left: Identifier, right: Identifier)? They can be parsed differently even if the nodes aren't present in the AST.

@mAAdhaTTah

This comment has been minimized.

Copy link
Contributor Author

commented Feb 26, 2019

Question: why do we need the PipelineHead and PipelineBody nodes?

My understanding of the spec was we needed the different nodes to provide different precedence values related to the arrow function, depending on whether it's in the Head or Body. But this is not my strong suit, so I would need someone else to weigh in here.

@babel babel deleted a comment from buildsize bot Feb 26, 2019

@nicolo-ribaudo nicolo-ribaudo self-requested a review Feb 26, 2019

@nicolo-ribaudo

This comment has been minimized.

Copy link
Member

commented Feb 26, 2019

I think that we should remove those nodes if (and only if) it is possible to understand if a node is the equivalent of a PipelineHead only by checking it's parent: this is how we disambiguate those cases in babel or babel/generator.

To my understanding, if t.isBinaryExpression(node, { operator: "|>" }) && !t.isBinaryExpression(node.left, { operator: "|>" }), then node.left is a pipeline head. Is it correct?

@mAAdhaTTah

This comment has been minimized.

Copy link
Contributor Author

commented Feb 26, 2019

@nicolo-ribaudo Yeah, that's correct. I can remove those nodes, but we do want to keep babel's produced AST in line with the spec, correct? If so, and we decide the spec requires them, would changing the AST be a breaking change? I'm not entirely clear on the spec needs here, and my understanding is Babel's AST should match the spec's AST, so I want to make sure we keep them in alignment before we remove anything. That said, I could easily be wrong about the spec's needs here, so I want to keep an eye on potential paths forward as we settle on the F# parenless AST.

Dismissing approval because the test I commented asking to add a comment in the code was actually wrong

Fix parsing of pipeline operator in arrow bodies
This should resolve the scoping issues we're seeing in the transform.

@mAAdhaTTah mAAdhaTTah force-pushed the mAAdhaTTah:implement-f#-pipeline-in-parser branch from a0a2ece to 7e17e56 Apr 13, 2019

@buildsize

This comment has been minimized.

Copy link

commented Apr 13, 2019

File name Previous Size New Size Change
babel-preset-env.js 2.27 MB 2.05 MB -232.03 KB (10%)
babel-preset-env.min.js 1.3 MB 1.1 MB -211.58 KB (16%)
babel.js 2.8 MB 2.78 MB -27.89 KB (1%)
babel.min.js 1.56 MB 1.55 MB -13.02 KB (1%)
mAAdhaTTah added 2 commits Apr 14, 2019
Add more tests for pipeline in object / array
This also needs to check, when there's a pipeline operator within
the arrow or object, that it parses correctly.
Handle assignment with pipeline in pipeline
More nested pipeline logic.
@mAAdhaTTah

This comment has been minimized.

Copy link
Contributor Author

commented Apr 14, 2019

@nicolo-ribaudo Addressed your comments / questions. I think we still need PipelineBody, but can update that if you think we don't.

@nicolo-ribaudo
Copy link
Member

left a comment

#9450 (comment)

I think so. The Smart proposal has a PipelineBody node as well.

Actually it looks like it's defined in types.js but it is never created in the parser. The Smart proposals has nodes like PipelineBareFunction, but it's there because it's different from a simple identifier/member expression.

Also, in the Smart proposal makes sense to have different nodes since, for example, a PipelineBareFunction and a PipelineTopicExpression have different meanings, while in the F# proposal it would always only be PipelineBody

Remove PipelineBody node
This isn't strictly need, as it's just an extra layer of nesting and
provides no additional information.
@mAAdhaTTah

This comment has been minimized.

Copy link
Contributor Author

commented Apr 15, 2019

Removed the PipelineBody node from the AST.

@nicolo-ribaudo nicolo-ribaudo referenced this pull request Apr 30, 2019
2 of 2 tasks complete
@hzoo
hzoo approved these changes May 13, 2019
Copy link
Member

left a comment

Nice work!

Was wondering (not specific to this PR) about how we would go about removing the different options in the future (once a proposal moves forward). I guess with the new state it doesn't seem that bad to remove (wish it was easier but unlikely that is possible to do easily). But would love to write more docs on the overall process we are going to take moving forward regarding multiple proposals and also the lifecycle of this process (implementation, deprecation, removal, acceptance, etc)

@nicolo-ribaudo nicolo-ribaudo changed the base branch from master to feature-7.5.0/fsharp-pipeline May 14, 2019

@nicolo-ribaudo nicolo-ribaudo merged commit d6c39ee into babel:feature-7.5.0/fsharp-pipeline May 14, 2019

5 checks passed

babel/repl REPL preview is available
Details
buildsize Significant change of babel-preset-env.js down by 13.02 KB (0.16%)
Details
ci/circleci Your tests passed on CircleCI!
Details
codecov/project 87.17% (target 80%)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

Pipeline Operator automation moved this from In progress to Done May 14, 2019

MichaelFoss added a commit to valtech-nyc/babel that referenced this pull request May 14, 2019
nicolo-ribaudo added a commit that referenced this pull request May 25, 2019
@goodmind goodmind referenced this pull request Jul 7, 2019
3 of 3 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.