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 precedence #2174

Closed
rickyvetter opened this issue Sep 6, 2018 · 5 comments

Comments

@rickyvetter
Copy link
Member

commented Sep 6, 2018

I think these should always mean exactly the same thing:

foo->bar(baz);
foo->(bar(baz));

This is a really confusing experience and is especially awkward when upgrading the relatively pretty:

foo
|. Map.get("bar")
|. Option.getWithDefault("baz");

to the paren heavy:

foo
->(Map.get("bar"))
->(Option.getWithDefault("baz"));

https://reasonml.github.io/en/try.html?rrjsx=true&reason=GYexFoD4CMEMCcAUcBeBKA3AKC6EACAHwDp84lVMc8pkE71Mg

Apologies if this is a KP or already open somewhere. It's a little tricky to search for operators and I didn't see anything.

@bloodyowl

This comment has been minimized.

Copy link
Contributor

commented Sep 7, 2018

explanation here: #2118 (comment)

from I understand it should safe to just search and replace |. with -> when compiling with BuckleScript (when the fixes in reason are upstreamed to bucklescript).

@IwanKaramazow

This comment has been minimized.

Copy link
Contributor

commented Sep 7, 2018

One of the goals of the fast pipe sugar is to allow certain kinds of code that isn't possible with the infix operator |..
Example:

/* before with infix operator |. */
(location |. streets)[0]

/* after with fast pipe sugar */
location->streets[0]

|. and -> have different precedence. -> has the same precedence as the . in foo.bar.
Fast pipe reads from left to right as you would do with the ., e.g. x.y.z or x->y->z.

To allow easy stubbing of -> on native, let (|.) = (a, f) => f(a), we made the choice to parse the following an ast that doesn't contain function application on the right side of ->.
This is needed to make let (|.) = (a, f) => f(a) work.
Example:

a->f(b);

/* ast sees */
(a->f)(b);
(a |. f)(b);

/* ocaml executes, because of let (|.) = (a, f) => f(a) */ 
f(a)(b);

The downside is that we get the following transformation:

foo
|. Map.get("bar")
|. Option.getWithDefault("baz");

/* precedence of |. results in the following ast */
foo
|. (Map.get("bar"))
|. (Option.getWithDefault("baz"));

/* refmt prints */
foo
->(Map.get("bar"))
->(Option.getWithDefault("baz"));

With the precedence of fast pipe and the fact that it doesn't accept an expression with function application on the right side, the parens are necessary. Also note the ast stays the same here.
If the parens where dropped we would get a total different ast:

foo
->Map.get("bar")
->Option.getWithDefault("baz");

(((foo->Map.get)("bar"))->Option.getWithDefault)("baz");
(((foo |. Map.get)("bar")) |. Option.getWithDefault)("baz");

Refmt should not change the meaning of your program's ast, hence we print the parens to conserve the original ast…

@rickyvetter does this make sense? Do you have a suggestion on how we could solve this?

@rickyvetter

This comment has been minimized.

Copy link
Member Author

commented Sep 7, 2018

Yup. This makes a lot of sense! Thanks for going through the detailed explanation (for the nth time!).

If I understand correctly, the place where meaning differs is when the pipe has a another operator on the right hand side that isn't some form of function application. So foo |. bar.baz and foo |. bar[baz] in particular are common cases where the issue comes up. Why can't the extra parens be removed by the same rule that would remove unnecessary parens in other places in refmt? It removes foo && (bar && baz) correctly. Is this the same issue?

@IwanKaramazow

This comment has been minimized.

Copy link
Contributor

commented Sep 14, 2018

I can investigate if the parens can be removed safely.

@bloodyowl

This comment has been minimized.

Copy link
Contributor

commented Oct 6, 2018

could be great @IwanKaramazow! I'm having lots of cases were the parens move the expression far away, which doesn't really look good. mostly with the a->(b(c)) pattern, where the parens could be removed safely

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.