Refmt: CRLF is treated differently than LF

[kyldvs]

When typing:

let x = 1;
/* foo */
let y = 2;

With CRLF (windows) line endings you get:

let x = 1; /* foo */

let y = 2;

With LF (unix) line endings you get:

let x = 1;

/* foo */
let y = 2;

[kyldvs]

cc @jordwalke

[jordwalke]

@bryphe do you know where this one could be coming from? I thought it would be with the input channels being marked as binary or not. We should fix this and then get it in with the next release which also has your other fix for binary input channels + merlin

[jordwalke]

Observed behavior on command line: Running on Linux, a file with CRLF were converted purely into LF. I think that is considered correct behavior because the printing is happening in Linux.
Then when we use Nuclide from Windows which uses Nuclide+LSP. Nuclide has the file in Windows with CRLF's, it sends it to Nuclide's LSP client side, then it sends the file contents to the LSP server which is running on Linux and at that point the line endings should be converted to LF, where it runs refmt on them, keeping them LF, and then the result should come back to the LSP client side which should know it's running on windows and convert all LF back to CRLF.

My guess is that refmt is not doing anything wrong and again it's the LSP. Here's how we can test:

1. On pure windows, does refmt someFile.re incorrectly convert CRLF to LF?
2. On pure windows, does cat file.re | refmt incorrectly convert CRLF to LF?
3. What about Reason Language Server? If RLS handles this then maybe we should just switch to RLS.
   (All those above can be tested on our new Reason + esy native windows setup).

[bryphe]

I'll check it out, I haven't dug much into refmterr yet, will be fun to look around

I thought it would be with the input channels being marked as binary or not.

Yes, I suspect this is actually the inverse case - where the channels are being set as binary, but then we're sending / hardcoding /n somewhere. I see that the line endings are being converted from CRLF -> LF when I run refmt, so that would be a problem, and suggests that there is a place we're hardcoding \n.

Something like this could be the culprit:
https://github.com/facebook/reason/blob/d5a26ac4c656f80dcdc93806f6b8c5127da5191d/src/refmttype/reason_format_type.ml#L38

(need to test and see though)

[bryphe]

Oh wow, we replied at the same time! Thanks for the extra details @jordwalke

1. On pure windows, does refmt someFile.re incorrectly convert CRLF to LF?

Yes, this is the case - this is why I think we're hardcoding a \n somewhere when we're emitting the AST.

3. What about Reason Language Server? If RLS handles this then maybe we should just switch to RLS.
   (All those above can be tested on our new Reason + esy native windows setup).

Haven't tried this out, yet. I'm still hitting working through a few hurdles on Windows with this PR: jaredly/reason-language-server#199

[bryphe]

Just to give an update - I did some digging and I believe this is actually a bug with the Format module in OCaml.

Specifically, here:
https://github.com/ocaml/ocaml/blob/0e5cae8dc6ed1eed4dd73e0f502ced0a1781b5a3/stdlib/format.ml#L823

Working on a minimal repro to confirm and submit an issue to core OCaml.

[bryphe]

Alternative fix is to not use a binary channel in this case here in printer_maker.ml

let prepare_output_file = function
    | Some name -> open_out_bin name
    | None -> set_binary_mode_out stdout true; stdout

If we use open_out, then we get CRLF in the output as expected (because in non-binary mode the line endings are converted automatically). However, that's clunky.... because there are other places where we do definitely need binary mode (like when we make the output_value call here):

    output_value output_chan (
      Config.ast_impl_magic_number, filename, ast, comments, parsedAsML, false
    );

I do think a fix makes sense in the ocaml stdlib. However, we probably want to unblock this a bit sooner - options are:

1. Conditionally use open_out or open_out_bin depending on our printing strategy (whether its text, or needs a binary op like output_value)
   OR
2. Stick with binary mode, but do a post-process step on the output text - convert LF -> CRLF.

[jordwalke]

Carrying the discussion on over in the PR, but to summarize my belief about newlines here:

It seems that most libraries (and C libs too?) are written to assume that a single \n characters is a high level declarative token. You manipulate strings with these tokens inside of the strings, but then when presenting the data as a "text file" to the user or system, you convert the text to its "concrete" representation which might replace \n with CRLF. The benefit there is that now all your libraries everywhere don't have to handle the case of searching for either \n or CRLF - they can pretend everyone in the world just used \n characters. The downside is that now you have to think about if your output is going to be presented as a "text file" - vs. a "data file" where "text file" is by definition files that include CRLF wherever a Unix system would have written \n. All because some applications expect CRLF!

The unfortunate thing is that it doesn't seem possible to create a cross platform API that fixes this without that cross platform API always making you say if something is a "text file" or a "data file". That means even when building for Unix you have to use APIs that make you needlessly distinguish just so your app will be compatible on windows.