Missing `=>` infix operator escape #1944

anmonteiro · 2018-05-20T01:47:07Z

This PR applies the same solution that we use for /\* and makes =\> work.

Because we reuse the solution that is already in place, this is still parsed as => in the AST and printed as => in the OCaml side.

fixes #1941

anmonteiro · 2018-05-20T01:53:50Z

src/reason-parser/reason_syntax_util.ml

@@ -393,10 +393,12 @@ let identifier_mapper f super =
 (** escape_stars_slashes_mapper escapes all stars and slases in an AST *)
 let escape_stars_slashes_mapper =
  let escape_stars_slashes str =
-    if String.contains str '/' then
+    if (String.contains str '/') ||
+       ((String.contains str '=') && (String.contains str '>')) then


maybe this can be a simple str = "=>" since we're special casing the fat arrow. let me know

This probably should be str = "=>" or something equivalent to avoid catching operators which don't need escaping like >= or ==>.

@hcarty that a better reason to do what I was suggesting, will fix.

done in d4e841e

jordwalke · 2018-05-23T20:05:19Z

Great improvement, @anmonteiro! One alternative approach would be to swap -> and =>. Thoughts?

hcarty · 2018-05-23T20:12:52Z

One downside to swapping is that it breaks with how other conflicts are handled between syntaxes.

anmonteiro · 2018-05-23T20:28:30Z

@hcarty what do you mean?

edit: sorry I hadn't seen @jordwalke's comment.

anmonteiro · 2018-05-23T20:32:55Z

@jordwalke I personally don't mind it, here are my thoughts on the tradeoffs:

=\> is uglier (subjective) but feels natural given /\* for example
=\> could probably just be listed in this comparison table without any further explanation, whereas -> would maybe need its own little explanation paragraph
- Granted, this is an edge case that wouldn't come up to often, but I'm generally in favor of what feels more natural.

Thoughts?

hcarty · 2018-05-24T18:20:30Z

It might be nice to save -> for Reason's core syntax later on. It's not a valid infix operator currently.

The previously cases of swapping for keywords caused confusion before. Infix operators are not likely to be as confusing when swapped - that's already done for string concatenation for example - but it's still nice to have conversion rules be consistent. Is => used often enough to be worth adding an exception?

anmonteiro · 2018-05-28T20:05:54Z

@jordwalke do you agree with the above? should we go ahead and keep =\> then?

jordwalke · 2018-06-13T00:08:35Z

My original idea of swapping -> and => didn't take into account that people will likely want to implement custom behavior for -> via ppx. So for that reason, we wouldn't want -> to be represented by a super common infix operator like =>(in ocaml). Instead parsing -> into the more obscure |. identifier makes it a better candidate for fancy / opinionated transforms.

jordwalke · 2018-06-13T00:49:00Z

The reasons for using such a complicated escaping convention for * and / had more to do with avoiding lexing ambiguity with comments. Normally, we apply swaps or mappings. What would such a solution look like here?

One approach:
ML's => could be printed in Reason as ==>.
ML's ==> could be printed in Reason as ===>.
ML's ===> could be printed in Reason as ====>.

The downside to that approach is that operators are extra long etc (even ones like ==> which don't have an ambiguity must be written as ===> in Reason).

Another approach is that we can swap the operator => and \=>. Reason parses \=> into the AST as \=>. We could make it so \=> gets "swapped" with => where all the other swapping happens. Benefits:

All the swapping works more or less the same - no lexing changes necessary.
The leading backslash could be used in many places to represent these compatibility parsings. You never have to question how to escape an operator because it's always the first character.

anmonteiro · 2018-07-02T21:06:21Z

@jordwalke I really like your 2nd idea. I'll try to prototype something in this PR over the next couple of days.

I think you said in Discord that backslash prefixed operators are not valid in OCaml, which would allow Reason to fix all these compatibility parsings in one go.

hcarty · 2018-07-02T21:20:32Z

A user pointed out that ++ is not currently handled properly when converting from OCaml to Reason (see https://reasonml.chat/t/access-conflicting-infix-operator/805). Could that be captured in this fix as well?

anmonteiro · 2018-07-02T21:34:05Z

@hcarty the idea I have in mind (and I think it's what Jordan implied) is that \ could be a prefix for all such transformations between OCaml and Reason. Therefore, we'd make it work using \++.

That solves the problem, right?

hcarty · 2018-07-02T21:38:37Z

@anmonteiro I think so, at least for infix operators.

This PR applies the same solution that we use for `/\*` and makes `=\>` work fixes reasonml#1941

anmonteiro · 2018-10-11T15:33:42Z

@jordwalke I think this is ready for another look.

I implemented your suggestion by having any operator preceded with a backslash (\) be the same operator on the OCaml side without the backslash.

This enables things like:

let (\++) = ...

let (\=>) = ...

The way I made it generic for all backslash-prefixed operators is by still having e.g. \=> in the AST but printing to OCaml as =>. This way we don't need to enumerate all the possible operators that need swapping.

The downside of this approach is that converting OCaml code that has e.g. => to Reason will look like let (=>) = ... which is invalid Reason code (but I think that's fine?).

hcarty · 2018-10-11T15:41:07Z

This seems like a nice simplification, with the downside that keeping the \-prefix form in the AST makes conversion to/from OCaml syntax harder.

anmonteiro · 2018-10-11T15:46:24Z

@hcarty The problem is only about OCaml -> Reason, not the other way around.

Some examples:

# Reason -> Reason
$ echo 'let (\=>) = (a, b) => a + b' | ./_build/install/default/bin/refmt
let (\=>) = (a, b) => a + b;
$ echo 'let (\++) = (a, b) => a + b' | ./_build/install/default/bin/refmt
let (\++) = (a, b) => a + b;
# Reason -> OCaml
$ echo 'let (\=>) = (a, b) => a + b' | ./_build/install/default/bin/refmt --print ml
let (=>) a b = a + b
$ echo 'let (\++) = (a, b) => a + b' | ./_build/install/default/bin/refmt --print ml
let (++) a b = a + b
# OCaml -> Reason (where the problem is)
$ echo 'let (=>) a b = a + b' | ./_build/install/default/bin/refmt --parse ml
let (=>) = (a, b) => a + b;
$ echo 'let (++) a b = a + b' | ./_build/install/default/bin/refmt --parse ml
let (++) = (a, b) => a + b;

hcarty · 2018-10-11T16:11:25Z

@anmonteiro Sorry, I was thinking of Reason -> OCaml through ocamlformat since refmt doesn't keep comments. I don't think ocamlformat is perfect in that regard either, to be fair. And ocamlformat's Reason -> OCaml conversion currently has some pretty tight version constraints.

IwanKaramazow · 2018-10-16T16:42:53Z

@hcarty If you refmt Reason -> Ocaml with refmt and then use ocamlformat, would that work in your case?

hcarty · 2018-10-16T17:15:33Z

@IwanKaramazow Unfortunately not. Going .re -> refmt -> ocamlformat translates the syntax but last I checked refmt drops comments when outputting OCaml syntax.

jwhitley · 2019-06-02T16:52:55Z

Thanks for the work here @anmonteiro! It'd be nice to get this PR approved and merged, since it blocks interoperability with some fairly basic OCaml infix operators, esp. => and libraries that use them.

anmonteiro · 2019-06-02T16:56:06Z

The PR isn't done yet, and I haven't found the time to squeeze it in.

akdor1154 · 2022-04-11T02:12:06Z

A nasty one I just come across is the (//) operator used in Fpath - it might need special treatment on top of this :(

facebook-github-bot added CLA Signed labels May 20, 2018

anmonteiro commented May 20, 2018

View reviewed changes

anmonteiro force-pushed the gh-1941 branch from 7541680 to 6543b61 Compare May 28, 2018 20:02

anmonteiro force-pushed the gh-1941 branch 2 times, most recently from 79fef91 to f9fc39a Compare June 3, 2018 18:44

anmonteiro added 4 commits October 11, 2018 14:30

Missing => infix operator escape

79ff78d

This PR applies the same solution that we use for `/\*` and makes `=\>` work fixes reasonml#1941

fix warning

fa307a2

only escape fat arrow

1d5e115

better / more tests

9d25a58

anmonteiro force-pushed the gh-1941 branch from f9fc39a to 9d25a58 Compare October 11, 2018 13:32

Implement Jordan's suggestion

69fb82a

anmonteiro mentioned this pull request Feb 16, 2019

(++) can't be used as an operator since it turns into (^) #2309

Open

anmonteiro mentioned this pull request Jun 1, 2019

Incorrect conversion of custom fat arrow operator #2416

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Missing `=>` infix operator escape #1944

Missing `=>` infix operator escape #1944

anmonteiro commented May 20, 2018 •

edited

Loading

anmonteiro May 20, 2018

hcarty May 20, 2018

anmonteiro May 20, 2018

anmonteiro May 20, 2018

jordwalke commented May 23, 2018

hcarty commented May 23, 2018

anmonteiro commented May 23, 2018 •

edited

Loading

anmonteiro commented May 23, 2018

hcarty commented May 24, 2018

anmonteiro commented May 28, 2018 •

edited

Loading

jordwalke commented Jun 13, 2018

jordwalke commented Jun 13, 2018

anmonteiro commented Jul 2, 2018

hcarty commented Jul 2, 2018

anmonteiro commented Jul 2, 2018

hcarty commented Jul 2, 2018

anmonteiro commented Oct 11, 2018

hcarty commented Oct 11, 2018

anmonteiro commented Oct 11, 2018

hcarty commented Oct 11, 2018

IwanKaramazow commented Oct 16, 2018 •

edited

Loading

hcarty commented Oct 16, 2018

jwhitley commented Jun 2, 2019

anmonteiro commented Jun 2, 2019

akdor1154 commented Apr 11, 2022

Missing => infix operator escape #1944

Are you sure you want to change the base?

Missing => infix operator escape #1944

Conversation

anmonteiro commented May 20, 2018 • edited Loading

anmonteiro May 20, 2018

Choose a reason for hiding this comment

hcarty May 20, 2018

Choose a reason for hiding this comment

anmonteiro May 20, 2018

Choose a reason for hiding this comment

anmonteiro May 20, 2018

Choose a reason for hiding this comment

jordwalke commented May 23, 2018

hcarty commented May 23, 2018

anmonteiro commented May 23, 2018 • edited Loading

anmonteiro commented May 23, 2018

hcarty commented May 24, 2018

anmonteiro commented May 28, 2018 • edited Loading

jordwalke commented Jun 13, 2018

jordwalke commented Jun 13, 2018

anmonteiro commented Jul 2, 2018

hcarty commented Jul 2, 2018

anmonteiro commented Jul 2, 2018

hcarty commented Jul 2, 2018

anmonteiro commented Oct 11, 2018

hcarty commented Oct 11, 2018

anmonteiro commented Oct 11, 2018

hcarty commented Oct 11, 2018

IwanKaramazow commented Oct 16, 2018 • edited Loading

hcarty commented Oct 16, 2018

jwhitley commented Jun 2, 2019

anmonteiro commented Jun 2, 2019

akdor1154 commented Apr 11, 2022

Missing `=>` infix operator escape #1944

Missing `=>` infix operator escape #1944

anmonteiro commented May 20, 2018 •

edited

Loading

anmonteiro commented May 23, 2018 •

edited

Loading

anmonteiro commented May 28, 2018 •

edited

Loading

IwanKaramazow commented Oct 16, 2018 •

edited

Loading