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

Fast pipe sugar #1999

Merged
merged 8 commits into from
Jul 5, 2018
Merged

Fast pipe sugar #1999

merged 8 commits into from
Jul 5, 2018

Conversation

IwanKaramazow
Copy link
Contributor

@IwanKaramazow IwanKaramazow commented Jun 10, 2018

TLDR

  • -> refmt into ->
  • |. refmt into ->
  • -> parses into ocaml |.
  • |. parses into ocaml |.

@anmonteiro
Copy link
Member

-> feels awkward to me given that => exists. What is the reasoning behind the move from |. to ->?

@jaredly
Copy link
Contributor

jaredly commented Jun 11, 2018

Also I feel like |. formats better

Something
|. transform
|. another(thing)

feels better than

Something
-> transform
-> another(thing)

|. is similar to |>
-> is very different from =>

@jaredly
Copy link
Contributor

jaredly commented Jun 11, 2018

Adds support for builtin/native "fast pipe" in Reason. I.e. a thread-first operator operating at the syntax level.

And how is this "adds support"? because we already have |.

@hcarty
Copy link
Contributor

hcarty commented Jun 11, 2018

@jaredly I think |. is currently specific to bucklescript.

@hcarty
Copy link
Contributor

hcarty commented Jun 11, 2018

One argument for -> over |. is that |. is a valid infix operator in Reason, at least as of the latest master, while -> is not. So there is no breaking change required for ->.

@chenglou
Copy link
Member

@jaredly it wouldn't have spaces:

Something
->transform
->another(thing)

Like PHP/C++. This evokes OO-style fluent programming without compromises. Imagine under modular implicit:

[1, 2]->toArray->get(0)->forceUnwrap->print

All these functions come from different modules, but are somehow assembled together and this didn't require the list to inherit from anything.

@hcarty BS disables |. as infix already, so no clash there at least.

@jaredly
Copy link
Contributor

jaredly commented Jun 11, 2018

hrmmmmm. my knee jerk reaction is to gag, but that's just my history with php resurfacing 😅 I guess I could get used to it.

@bsansouci
Copy link
Contributor

I prefer keeping |. over ->, which in C/C++ code is semantically totally different from its meaning here (or am I missing something? -> is dereferencing a heap pointer and here it's a "pipe").

@jaredly
Copy link
Contributor

jaredly commented Jun 12, 2018

For us, -> would be more like golang's method call (https://gobyexample.com/methods), and somewhat similar to rusts methods.
e.g. sugar to make things feel more OO.
I think it's a cool idea

@cknitt
Copy link

cknitt commented Jun 12, 2018

Hmmm. As the equivalent of

obj
  .f(x)
  .g(y)
  .h(z);

in JS, I don't think

obj
  ->f(x)
  ->g(y)
  ->h(z);

feels any better than

obj
  |. f(x)
  |. g(y)
  |. h(z);

I think |. was actually quite a good naming choice. | means pipe, and the . signifies (JavaScript-like) method invocation for me, that quite neatly summarizes what the operator does.

Copy link
Member

@chenglou chenglou left a comment

Choose a reason for hiding this comment

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

Should we just parse |. into -> for a brief period of time as a codemod?

@@ -0,0 +1,119 @@
/* tests adapted from https://github.com/BuckleScript/bucklescript/blob/master/jscomp/test/pipe_syntax.ml */
include (
Copy link
Member

Choose a reason for hiding this comment

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

What's with the formatting here lol

}
);

let f = (a, b, c) => a ->(b, c);
Copy link
Member

Choose a reason for hiding this comment

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

Can we remove the space between a and ->?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, on my todolist, still thinking this through, there are some challenges 🐒


let f7 = a => a ->(Some, Some, Some);

/* fast pipe in combination with underscore sugar */
Copy link
Member

Choose a reason for hiding this comment

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

What should the semantics of a->f(_, b) be? x => f(a, x, b) or x => f(x, a, b) or f(a, b)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

f(_, b) desugars to (__x) => f(__x, b)
So a->f(_, b) === ((__x) => f(__x, b))(a)

open Ast_helper

let parseFastPipe e1 e2 =
let attr = (Location.mknoloc "reason.fast_pipe", PStr []) in
Copy link
Member

Choose a reason for hiding this comment

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

This annotation is probably not necessary right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is for pretty printing purposes I'm afraid.
a->f->g is parsed as an ordinary g(f(a)). We need the attribute to give refmt a hint that it actually wasn't g(f(a)), but a->f->g.

@chenglou
Copy link
Member

f->g() currently becomes g(f, ()). Should we turn it into g(f) instead? This seems to be the norm but I actually feel that mingling f->g and f->g() is a bad thing

@jaredly
Copy link
Contributor

jaredly commented Jun 12, 2018

If we turn f->g() into g(f), then let's disallow f->g (e.g. require parens).
I think f->g w/o parens could be confusing, because it would feel like attribute access not a function call

@jaredly
Copy link
Contributor

jaredly commented Jun 12, 2018

Also f->Some() feels pretty weird to me -- it breaks the idea of "pseudo-method call". I think |. is more appropriate for that, and for the fancy tuple shortcut

@chenglou
Copy link
Member

chenglou commented Jun 12, 2018

If we turn f->g() into g(f), then let's disallow f->g (e.g. require parens).

Let's keep f->g() becoming g(f, ()) then.

Which also means you'd use f->Some

@jordwalke
Copy link
Member

jordwalke commented Jun 12, 2018

/* input */
a -> f -> g -> h;
/* parses as */
h(g(f(a)));

I understand why it would work this way, but it's a little weird that -> here is left associative whereas in OCaml -> is right associative and in Reason => is right associative. Arrows have this precedent set that they should be right associative. However, in order to get pipe chaining, I think it has to be left associative.

About the tuple distribution:

a -> (Some, Some, Some);
/* should parse as */
(Some(a), Some(a), Some(a))

I'm not sure about this one. Substitution of a value in an identifier produces completely different meaning:

let ident = (Some, Some, Some);
a -> ident;

Compared to |.:
-> seems much more familiar to people than |. due to the precedent set in other language (especially when formatted as x->b()).

In general, it's not yet clear how -> should be transformed. However, we would currently transform it would be totally different than when we get modular implicits. Modular implicits would likely not work with the fast piping, because we want to let MI do a portion of the resolution and using -> as an infix function would allow it to intercept that dispatch.

I think we should break this up into two pieces:

  1. Let's add support for infix -> as a function identifier that would be left associative. Thankfully, it's not yet a valid function identifier.
  2. Let's let people try out their own ppx approaches which transform -> into fast pipe or whatever they want. We should work towards a clean division of sugary ppx transforms and concrete syntax additions even if some of those are "built-in" transformations. If it's clear that there's exactly one transform that will be used for a long time and would work for all use cases, then we could bake that into the refmt parser as a final stage of preprocessing(we should group all our "stock" ppx transforms into a final processing stage, which would allow others to more easily customize that final stage, in a way that allows refmt to own the entire ppx chain for performance).

"I think f->g w/o parens could be confusing, because it would feel like attribute access not a function call"

With Modular Implicits x->y would actually have a pretty compelling use cases. Just one more thing to consider.

@chenglou
Copy link
Member

The tuple one is just to match BS' |. features so that we can convert |. to -> for now.

The order is important because of side-effects and how people might use it: a->log->print.

The current PR already splits things into a transform and a ppx. We just bundle them together. Also, not too sure they should be overridable for now in userland (likewise => is special and not overridable) because it wrecks the predictability we're trying to build into it. Changing them into modular implicit-compatible thing (which is long term and whatever we're discussing here is obsolete by the time MI lands) is also harder if we allow userland overriding.

@jordwalke
Copy link
Member

This PR does transforms on a selective region of the AST inside of the parser (.mly).

I think it would be better to do the pass on the entire AST at the end, where we do other passes, and where we can chain multiple ppx plugins together.

@IwanKaramazow What's the role that src/reason-parser/reason_ast.ml plays? Is that where you imagine we would consolidate all "stock" ppx plugin logic?

@jordwalke
Copy link
Member

The one about this PR I'm looking to improve is a cleaner separation of concerns so that bs-platform logic doesn't creep into refmt. The tuple distribution feature is something that bs-platform introduced. I don't think the tuple distribution feature belongs in refmt, and it appears to be currently in refmt. I would hope that when using refmt without bs-platform, tuple distribution would result in a type error. However, I can understand how some people might want that tuple distribution feature especially if their existing code uses it in bs-platform.
Here's one idea I mentioned in Discord. What about parsing -> as the infix |. identifier? Then people can continue to rely on bs-platform's built-in ppx's to perform some more opinionated transforms such as tuple distribution. Then people are free to continue to experiment with -> for other use cases before we bake it into refmt directly. Peoples' code would automatically get upgraded from |. to -> when then reformat their files in new releases of Reason with that approach. Then we can keep all the good formatting improvements in this diff etc.

@chenglou
Copy link
Member

chenglou commented Jun 13, 2018

Discussed offline. Let's just turn -> into ocaml |. (and vice-versa).

The situation we don't wanna end up in, is for a newcomer to have to learn both -> and |.. This conveniently solves it.

We still need to consider some of the stuff |. doesn't solve currently, like foo |. bar[0] precedence, foo |. f(. bar), and what the precedence of |. vs _ should be. Likewise for 1 |. foo vs 1 |. foo(), but bs' transform distinguishes them (my own preference anyway).

@chenglou
Copy link
Member

Clarification:

  • -> refmt into ->
  • |. refmt into ->
  • -> parses into ocaml |.
  • |. parses into ocaml |.

@jaredly
Copy link
Contributor

jaredly commented Jun 13, 2018

(If we're voting) I have some preference for a->log()->print() over a->log->print.

@hcarty
Copy link
Contributor

hcarty commented Jun 13, 2018

Is the underscore syntax going to stick around? Would it be better to stick with only that sugar since it's more general and avoids the f() vs f ambiguity?

x
|> f(1.0, _)
|> Variant(_, "🐫")
|> g(_)

@IwanKaramazow
Copy link
Contributor Author

@jordwalke the reason_ast.ml module was intended as a safe haven for anything related to ast rewriting/transformation like the underscore sugar or for example something possible things like fast pipe or ppx rewriting etc. The main reason, however, is to break reason_pprint_ast/reason_parser into more manageable pieces. Compilation is getting quite slow these days...

I'll address the feedback.

@gilbert
Copy link

gilbert commented Jun 13, 2018

I was writing a lengthy bikeshed comment on how -> is not as good as |. with code examples, but by the end of it I started disagreeing with myself. +1 from me :)

@IwanKaramazow IwanKaramazow changed the title [Experiment] Native fast pipe Fast pipe sugar Jun 20, 2018
-> parses into ocaml |.
|. parses into ocaml |.
@wiltonlazary
Copy link

vote for: Ligature Request, ReasonML fast pipe: |. tonsky/FiraCode#613

@vezaynk
Copy link

vezaynk commented Jun 21, 2018

Sorry for commenting so late, but what's the point of doing this in the first place? Why do we want to use use 2 operators for the same thing?

-> is a very nice looking operator but seems to be entirely misused here. For one, it will no longer be possible to use it somewhere where it's more appropriate.

Secondly, when I see ->, I do not think "piping". In most languages I have come across -> denotes access (property lookup/method call/etc), not piping.

For example (using @jaredly's example):

a->log()->print()

Pretend you don't know what it does. What would you think it does? I would think it takes a, accesses and calls the log() property which returns a number of properties, one of which is print() and calls it without arguments.

I would immediately then assume that print() uses some sort of private properties to figure out what to actually print as it visibly takes no arguments.

Meanwhile, the |. operator, while still not clear without prior knowledge has a best-case scenario of me recognizing it as a pipe and being logical about it (or looking it up) or not being able to figure it out and looking it up. I am not nudged towards faulty assumptions.

Configuring refmt to convert |. to -> would just make this all so much worse.

@gilbert
Copy link

gilbert commented Jun 21, 2018

Pretend you don't know what it does. What would you think it does?

I would not have any more of an idea than |., but |. is harder to google than "reasonml arrow operator".

I would immediately then assume that print() uses some sort of private properties to figure out what to actually print as it visibly takes no arguments.

I don't know if I would immediately assume private properties, but I do agree the empty parens is misleading. Yet, using |. does not help:

a |. log() |. print()

print() still visibly takes no arguments.

Configuring refmt to convert |. to -> would just make this all so much worse.

How so?

@vezaynk
Copy link

vezaynk commented Jun 22, 2018

Arrow operators have well known meanings. "Fast Piping" is definitely not one of them.

I don't know if I would immediately assume private properties, but I do agree the empty parens is misleading. Yet, using |. does not help

Yes it does. It does not nudge the user into false ideas and makes it evident that something more special than accessing a property is going on. I don't mind being confused, I do very much mind being mislead.

Fundamentally though, what in the world is wrong with |. ? I am not particularly fond of it, but even less so of changing things for no discernible reason. If the proposal would have been something like <| as a way to show that it's the opposite of |>, then I would have been all for it as it would improve clarity. Or at the very least, does not obscure it.

Once again, there is nothing wrong with |. but there is something clearly wrong with -> and I am surprised that I am the only person bringing it up so far.

As a number of people here have already stated, it also doesn't feel right. It doesn't give me the "ReasonML" vibe tbh.

@gilbert
Copy link

gilbert commented Jun 22, 2018

I don't mind being confused, I do very much mind being mislead.

But what's the difference in practice? Either way you don't know what the code does, and the type system prevents you from using it wrong.

In any case, upon further thought I think -> might be too valuable an operator to spend it on basic piping. If the main argument is to make way for modular implicits, then I think it's a premature optimization. -> should stay open for a language extension of some kind.

IMO ReasonML should consider hijacking |> instead. Why have two ways to pipe, after all?

@vezaynk
Copy link

vezaynk commented Jun 22, 2018

The difference in a nutshell is that the process to figure out the functionality is different. People fallback to the familiar. It's getting kind of irrelevant to the discussion so whatever.

Full agreement with second point. It is indeed a beautiful operator and as I said, should be reserved for something more appropriate to an arrow.

I don't understand what you mean about hijacking |>. There's nothing wrong with have 2 ways to pipe (prepend argument and append argument)

@leostera
Copy link

leostera commented Jun 23, 2018

I see a few points being made. As an outsider, I'll try to address them.

@IwanKaramazow: The main reason, however, is to break reason_pprint_ast/reason_parser into more manageable pieces. Compilation is getting quite slow these days...

If the main reason is to split the compilation of these separate modules, why introduce new syntax? Or rather, how does introducing new syntax help you at this point?

User ergonomics should play no role in a performance optimization of the tooling behind the language.

@jaredly: e.g. sugar to make things feel more OO.

Is the intention to attract and appeal to a bigger audience by making the language look more like an imperative/oop counterpart? If that is the case, then I believe it should be crystal clear.

@knyzorg: I don't mind being confused, I do very much mind being mislead.

I could not agree with you more.

One example, for me, is the ? operator in Rust. Once you look it up you understand the behavior but it's easy to fall trap of Looks like something I already know, so must be like something I already know whereas in reality behaves very differently.

I.e, in ruby and lisps, ? is a valid identifier symbol, typically used to indicate a check of some sort that returns a boolean, such as: user.is_admin? == true.

@chenglou

[1, 2]->toArray->get(0)->forceUnwrap->print

All these functions come from different modules, but are somehow assembled together and this didn't require the list to inherit from anything.

While I am certainly awaiting ad-hoc polymorphism, for most people coming from Js or OOP, this is going to be outstandingly misleading.


Overall I feel that this discussion could've been split into

  • the optimization of compilation times by splitting the code, and
  • a completely separate discussion about introducing a new operator and exactly why is it necessary to do so.

I proposed before in Discord to introduce a lightweight RFC process so that impactful, long-term changes can be proposed, discussed, and eventually agreed upon or rejected under a clear, solid, community-driven basis. Like in the Rust community.

That's the sort of attitude that would make me want to invest even more in Reason.

@jordwalke
Copy link
Member

jordwalke commented Jun 27, 2018

@knyzorg:

"Secondly, when I see ->, I do not think "piping". In most languages I have come across -> denotes access (property lookup/method call/etc), not piping."

What's the difference between property/method access and function calling via piping? In many languages, there's this concept of the "implicit" this parameter that is passed. This is true in JS, and some languages even make the this explicit (I think python has self that you pass).

In JS, when you define a method, you do so like:

let obj = {
  count: 100,
  plusN: (n) => this.count + n
};
obj.plusN(20);

But there's an implicit "this" being passed to every function, so it's actually more like this:

let obj = {
  count: 100,
  plusN: (this, n) => this.count + n
};
obj.plusN(obj, 20);

In my view, a non-OO language is not so different, it's just that we tend to maintain a separation of behavior (methods) from the data.

let obj = {
  count: 100
};
module Methods = {
  let plusN = (this, n) => this.count + n
};
Methods.plusN(obj, 20);

Now at this point, there really isn's much difference between the JS OO style, and this form, except the syntax is not really conveying the fact that we have some data and we are applying operations on it. But with the "fast pipe" it then becomes apparent:

obj->Methods.plusN(20);

In my view, this has many benefits over the classical OO patterns where methods are first class attached and inseparable from the object instance. It means you can treat any piece of data like it implements any method that happens to accept that object's type as the first argument. For example, if I have a list [1, 2] I can "call methods on it" from any library that happens to accept lists, including but not limited to the standard library.

[1,2]->List.map(..)
[1,2]->YourLibrary.map(..)

The data itself doesn't have to anticipate all the possible methods that might want to be called with it.

So in my view, "fast pipe" really is not so different from method calls or "access". I believe that people love the typical OO patterns because they have been trained to think in terms of noun->verb() and with IDE autocompletion it kind of makes sense - you often know the data you want to operate on, but can't recall the method name. The x.y or x->y syntax facilitates the autocomplete experience. Well with "fast pipe" we can have that same experience without having to take on some of the complexities of OO in general.

@vezaynk
Copy link

vezaynk commented Jun 27, 2018

A large part of the concept of "this" is that "this" is the current object. You're describing the process of passing "this" between objects which defeats the entire point of it.

Why can't we just let it be |. ?

@jordwalke
Copy link
Member

jordwalke commented Jun 28, 2018

What is "the current object" but a parameter that is (in some languages) implicitly passed?

In JS, the this argument becomes explicit when using the .call mechanism for invoking functions.

thisObj.theMethod.call(thisObject, argOne);

You could even supply a different "current object" this way:

thisObj.theMethod.call(aTotallyDifferentObject, argOne);

In Python, the this is actually explicitly passed:

def open_branch(self):
        if not self.bankrupt:
            print("branch opened")

You may have a valid point, but I'm not seeing it at the moment.

@jordwalke
Copy link
Member

Haxe is a language that has a similar feature (where the . operator is extended in ways similar to fast-pipe based on what you declare you are "using"):

https://haxe.org/manual/lf-static-extension.html

I've found that it's a great feature. You can see that it also uses the convention of the left hand side of the "dot" becoming the first arg of the function. Haxe's approach is more type directed, but the concept is the same. Fast pipe is just more rudimentary in that it does not use type directed disambiguation (when modular implicits come, -> could become type directed).

@chenglou
Copy link
Member

chenglou commented Jul 5, 2018

You folks sure are good at bikeshedding lol!

We'd like to ask you for the benefit of the doubt (just like we've asked the same for the Reason 3 syntax transition, highly controversial but whose benefit has been obvious in retrospect). I'm not gonna rehash the pros and cons from both sides; see above for a nice, heated debate.

There's one thing I'd like folks to remember though: if it doesn't work out, we have the infra in place to undo the change without too much churn (including auto formatting, as always). So we have a little bit more leeway to experiment here and a slightly bigger safety net. Yes, the churn of needing to learn an extra operator's still there, but that's the cost of such investments. We'll try to document this well at least.

@chenglou chenglou merged commit f763db6 into reasonml:master Jul 5, 2018
@vezaynk
Copy link

vezaynk commented Jul 5, 2018

What are the metrics to decide if it "works out"?

@alex35mil
Copy link

alex35mil commented Jul 6, 2018

Controversial decision to remove spaces. . and > have quite different visual noise level. We don’t do a+b, right? And it seems odd in general to try to couple it with OOP . semantics. It looks and feels clumsy.

@alex35mil
Copy link

This looks odd as well:

foo
|> DataLast.bar
->DataFirst.bar
|> DataLast.baz
->DataFirst.baz

/* vs */

foo
|> DataLast.bar
-> DataFirst.bar
|> DataLast.baz
-> DataFirst.baz

@IwanKaramazow IwanKaramazow deleted the NativeFastPipe branch July 7, 2018 06:13
@IwanKaramazow
Copy link
Contributor Author

You can think of -> belonging to the category of "access".
Imagine foo.bar.baz:

foo
.bar
.baz

/* VS */
foo
. bar
. baz

@alex35mil
Copy link

@IwanKaramazow Yeah, I got idea from the thread. I don’t understand why in this case spaces were removed but we still have them in case of +, |> etc etc

@gilbert
Copy link

gilbert commented Jul 7, 2018

Considering the intent behind changing |. to ->, I would expect @alexfedoseev's example to parse this way:

foo
|> (DataLast.bar->DataFirst.bar)
|> (DataLast.baz->DataFirst.baz)

@texastoland
Copy link

texastoland commented Aug 27, 2018

Not that you need another dissenting voice but this feels like starting to jump the shark. I understand the OOP-like motive but it distances Reason further from JS. What's more is I'd rather just have classes that compiled sanely like Fable for the nicer autocomplete. If you really want it to be Pythonic why not just use the .. I refactored my code just so refmt wouldn't insert it.

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

Successfully merging this pull request may close these issues.