Reason: Considering removing `Foo.(bar(baz))` module sugar

[chenglou]

As seen by e.g. https://reasonml.chat/t/open-module-syntax-error/920, this particular module sugar has been a recurring pain point. Especially so when newcomers tries to add one more expression in the parens.
This also causes quite a bit of refmt-ing pain, as {open Foo; bar(baz)} sugars back into Foo.(bar(baz)).

I believe the use-case for Foo.(bar(baz)) is thus limited. Removing it solves the above two pain points, plus removes yet another set of parens. We should first refmt them back into {open Foo; bar(baz)}, then remove the sugar in a future update. Foo.bar stays.

cc @Iwankaramazow @anmonteiro

[jaredly]

hrmm it can be quite useful sometimes. Can't we just fix up & merge the PR that fixes refmt?

[chenglou]

What's the use-case?

[zploskey]

Am I understanding this proposal correctly that this would remove formatting from {open Foo; bar(baz)} to Foo.(bar(baz)) but still allow explicitly writing something like Foo.(baz->bar->qux), equivalent to baz->Foo.bar->Foo.qux? I use this a lot when calling a several functions from one module in a single line of code.

[chenglou]

It's proposing to prepare to remove it. You'd add an open Foo to the outer scope.

let whatever = () => {
  open Foo;
  baz->bar->qux;
}

[zploskey]

My personal preference would be to simply remove the formatting conversion from {open Foo; bar(baz)} to Foo.(bar(baz)) and otherwise preserve the way the author wrote it. The sugar makes certain operations quite clean looking.

[anmonteiro]

@zploskey tbh I’m not very fond of removing the Foo.(bar(baz)) sugar either, but I can’t help to dislike your suggestion even more. Because those look the same in the AST, we would have to introduce more complexity to the parser/printer in order to preserve the user formatting.

[hcarty]

The big plus I see for M.(...) is that it makes it easier to limit the scope of an open.

I'm not certain that makes it worth keeping. But it is a significant benefit since it enforces an explicit end to open's scope.

[jaredly]

An example of using it, with bs-css

<div className=Css.(style([backgroundColor(`red), padding(`px(20))])>
</div>

[IwanKaramazow]

There are two problems:

• the local open syntax is not so intuitive for newcomers (I struggled with it/had no idea what it meant)
• refmt printing suboptimal

I'm not 100% sure what the best way forward is. Refmt can be fixed with a bit of work. On the other hand the local open sugar is very convenient for "powerusers". (people who understand its meaning). Not sure if we should discard it for newcomers. (Is there such a thing as going too far?)
There's also the aesthetics of M.(...), it looks clean compared to the open M; ....

What if we allow parsing of:

Hashtbl.(
  add(abc, “b1”, “b1v”);
  abc->abc(“b1”, “b1v”);
);

and have refmt insert the braces?
We could put some effort in "over"-parsing a lot of cases with local open, to safeguard a more intuitive/forgivable syntax?

[chenglou]

An example of using it, with bs-css

<div className=Css.(style([backgroundColor(`red), padding(`px(20))])>
</div>

Yeah but the moment you have more than one style usage (roughly, if you have more than one or two tags), you can either alias it or open it locally in render anyway. Unless you repeat Css.(...) for every tag.

[IwanKaramazow]

What about:

let packageJson = Path.(dir / "package.json");

vs

let packageJson = {
  open Path;
  dir / "package.json"
};

[chenglou]

Yes, this would be the compromise. For infix in particular, we'd like to de-incentivize them.

[IwanKaramazow]

Does anyone have (real) examples of scope limitation with open sugar?

[hcarty]

It can also be useful for code like this:

let y = Float.(x * of_int64(Int64.(a * b));

which doesn't translate well to open:

let y = {
  open Float;
  x * of_int64({ open Int64; a * b })
};

The more compact Float.(...) version isn't perfect but I think it's far more readable than the open version.

[chenglou]

Btw, I appreciate folks coming up with counterexamples, but I think @IwanKaramazow's right. We need real repos for these kind of changes, otherwise every syntax change seems to elicit a thread of folks helpfully but ironically trying hard to find use-cases to _justify the non-change_ of said syntax. If we try hard enough, there will always be counterexamples.

[hcarty]

My float/int64 example isn't lifted from literal code but it is a pattern I used a lot in my previous job where I was doing more number crunching. That general usage was part of the discussion that lead to M.(...) syntax originally landing in OCaml.

[chenglou]

Ah, thanks for providing the background.
I'm wondering if there's not another way of solving this. Finger crossed for modular implicit lol

[hcarty]

In 20xx we'll all be laughing about these discussions with our modular implicit-enabled ecosystem 😄

For a tiny bit more history on this feature, it was available as a camlp4 extension before M.(...) was supported. It really made a big difference for the readability of numeric code.

A strong argument could be made for factoring out the Int64 part of the example I gave into its own let ab = { open Int64; a * b }; What works best in practice depends heavily on context though. I don't have access to the body of code using this heavily anymore to provide more concrete examples.

[bloodyowl]

I use it in a few places and having X.(expression) looks nicer and more intuitive once you know how it works, but having it converted to {open X; expression} wouldn't bother me a lot.

[andreypopp]

I use this extensively in esy in two occasions:

1. Limit the scope of infix operators, example:

   let packageJson = Path.(dir / "package.json");
   let copyCommand = Cmd.("cp" % p(src) % p(dst));
   

   and this is super common in esy (a lot of path manipulation) and looks
   clean & nice (subjectively). Also I'm using established OCaml libraries (fpath and bos).

   With this proposal it would look like:

   let packageJson = {
    open Path;
    dir / "package.json";
   }
   let copyCommand = {
    open Cmd;
    "cp" % p(src) % p(dst);
   }
   

2. Limit the scope of combinator based API:

   let pp = Fmt.(list(~sep=unit(", "), pair(string, string)))
   

   which can be rewritten as

   let pp = {
    open Fmt;
    list(~sep=unit(", "), pair(string, string))
   }
   

   or

   module F = Fmt;
   let pp = F.list(~sep=F.unit(", "), F.pair(F.string, F.string)))
   

   Other examples of combinator based APIs which are pretty common - angstrom,
   tyre, cmdliner, JSON decoders/encoders (esy-lib/Json) - so I think this use case is
   important.

The alternatives look less clean to me:

• With aliased module binding:

  module F = Fmt
  

  Code becomes noisy in my opinion and it forces you to pick a name for an alias (not a super big problem as I'd pick a short name but still).

• The reformatted open:

  let pp = {
  open Fmt;
  list(~sep=unit(", "), pair(string, string))
  }
  

  Looks to me maybe even more confusing than M.(...) as the connection between
  open statement and code which use it is less apparent on the first sight.

  But I think it poses a more serious problem (apart from being too verbose): I
  can see how people (especially those who new to the codebase) might be tempted
  to reuse this code block to add more code there and thus exposing it to the
  scope of open'ed module.

  Thus I agree with the comment by @hcarty that M.(...) syntax limits the
  scope of open which is very useful feature in my opinion.

I think I'd want to see {open M; ...} go away instead of removing M.(..).

Also maybe M.(..) can be made less confusing by making it look less like an
function application and more like a "decorated code block":

let pp = Fmt.{ list(~sep=unit(", "), pair(string, string)) }

I know this conflicts with an open for a record literal but maybe we should
remove it too!

[chenglou]

We're gonna discourage custom infix; the above use-case might suffer a bit, but that seems to be it

[hcarty]

I think at least two of the explicit usecases stated above (numeric code and combinator libraries) benefit significantly from both custom infix and scoped open. And both are pretty important classes of code.

I agree that both features should be used rather sparingly as they can lead to impossible to read code. But that's true of almost any feature. Anonymous functions are often poisonous to code readability - that (hopefully) doesn't mean we should discard them from the language!

[jordwalke]

I also have found them very helpful in a real world problem I'm solving right now. I have a few different versions of an AST. The different versions are about 80% the same, but the differences represent extensions of some base AST. In the 80% case, all the variant leaves have the same names. I am defining conversion functions back and forth between these trees.

In some files that mostly deal withAstA, I want AstA to be the default scope for variant labels. But within that file, I occasionally want to make AstB be the default scope for the overlap in labels, but only for some parts of expressions.

This point has already been explained in other responses, but just thought I'd explain how I find the expression level open syntax helpful in a real world thing I'm working on right now: when data structures have many overlap in variable label names.

open AstA;            /* At top of file */
...
let convertToAstB = (astA) => {
  let result = convertAndAppend(
    EFun(EObject([])),    /* By default AstA for whole file */
    AstB.(EObject([EUndefined, EUndefined, EUndefined]))
  )
  ...  
};

There's other ways to write it but involves prefixing lots of module paths or opening more widely than I'd like, or breaking up the function into more bindings than I would like.

[ostera]

I'm curious as to how many more times have people stumbled upon this problem — changing an established syntax for a single post seems extreme.

[Lupus]

I've just wrote the following piece of code in my work project (reason native with jane street core):

    let now_with_delta =
      Time.(add(now(), Span.of_sec(config.max_clock_delta)))
      |> (x => Time_ns.(x |> of_time |> to_int_ns_since_epoch));

And I don't want it to look like this:

    let now_with_delta = {
      open Time;
      add(now(), Span.of_sec(config.max_clock_delta))
      |> x => {
        open Time_ns;
        x |> of_time |> to_int_ns_since_epoch;
      }

All of my codebase uses open sugar in various places, akin to what other posts describe above.

I've shared this issue's proposal with one of my colleagues, who works with Reason React in JS world, and he was terrified with local open sugar disappearing, as he aggressively uses this sugar in his code.

Another example of clean code with open sugar can be found in async ssl bindings. I've leveraged that style in my own bindings to couple C libraries in Reason, looks like this:

      let write =
        foreign(
          "rados_aio_write",
          Ctypes.(
            Io_ctx.t
            @-> string
            @-> Completion.t
            @-> ptr(char)
            @-> size_t
            @-> uint64_t
            @-> returning(int)
          ),
        );

We're gonna discourage custom infix

@chenglou could you please elaborate on this? Infix operators are an essential part of OCaml and are also widely used. A lot of Lwt-based projects use operators from Lwt.Infix.

Attempts to drop widely-adopted syntax features sound quite alerting to me. I've decided for ReasonML for the tech stack of my team at work, because it's still OCaml, time-proven language with rich feature set. I've read somewhere (can't find it right now) that ReasonML syntax was designed to address top 10 problems identified in OCaml syntax by seasoned OCaml developers. That sounds like a great goal. I understand that building community is essential for ReasonML ecosystem to prosper, but discarding features based on newcomer's feedback looks like a way to end up in local maximum of this syntax optimization process. Parser simplification is always good, but it should'be the deciding factor for dropping established syntax features either.

[jordwalke]

@Lupus Hear you loud and clear. Thank you for the feedback. Since this issue was created, there have been improvements to refmt for preserving some additional formatting, which eliminates some of the original complaints of this sugar (though not all).

Side Note:
Coincidentally, I've recently discovered a newfound appreciation for the X.(..) local open syntax for creating DSLs that are very similar to the one you showed with ctypes. What is great about it, is that you can pretty seamlessly let various modules define the meaning of an "arrow" like @->. For example:

Module.(
  foo
  @-> fooFoo
  @-> Bar.(
    bar
    @-> barBar
  )
);

Where each module gets to determine the meaning of @->.

[jordwalke]

@Lupus' comment makes me think that this issue being open is giving the impression that this is somehow on the roadmap, but it's moreso just a place of (good) discussion. I'll close it, but everyone feel free to comment if you have any more thoughts.

[Lupus]

Thank you @jordwalke ! Now I can code in piece! :smile:

this issue being open is giving the impression that this is somehow on the roadmap

This was exactly my impression, when I stumbled upon this issue while googling for something else, read it through, and assumed that it does not have any resolution.

DSL idea sounds really great btw :+1: