rescript-lang / rescript-compiler

The compiler for ReScript.
https://rescript-lang.org
Other
6.74k stars 448 forks source link

ocamlformat fails on bucklescript preprocessor directive #4382

Closed joprice closed 4 years ago

joprice commented 4 years ago

With a file with the following source:

#if DEBUG then
let x = 1
#end

running ocamlformat results in this error:

(master|✚2…) > ocamlformat src/Preprocess.ml
ocamlformat: ignoring "src/Preprocess.ml" (syntax error)
File "src/Preprocess.ml", line 2, characters 1-3:
Error: Syntax error

As a workaround, I can add the file to .ocamlformat-ignore so that my formatting script in ci doesn't fail and not format the rest of the process.

I recognize that this might be less supported since bsrefmt might be more frequently used for bucklescript projects, but there is at least no mention in the preprocessor docs of this incompatibility https://bucklescript.github.io/docs/en/conditional-compilation#docsNav and actually mentions that it is expected to work with exiting tooling: It’s purely functional and type-safe, and cooperates with editor tooling like Merlin. Is this a known issue?

Env:

bs-platform version: 7.2.2
ocamlformat version: 0.13.0
opam switch ocaml-base-compiler.4.07.1
cristianoc commented 4 years ago

@joprice see comment about cppo:

https://github.com/ocaml-ppx/ocamlformat/blob/957ecda41633c0c7d0e14cb11d793ef09263ebfa/README.md#overview

joprice commented 4 years ago

Ah, so Parses without any preprocessing means I shouldn't expect this to be supported unless ocamlformat decided to support bucklescript explicitly? If that's the case, I assume it would lead people not to use conditional compilation at all when already using ocamlformat, or reduce use of conditional compilation to files with very little source that don't benefit from automated formatting in the first place.

Perhaps there could be an alternate syntax for conditional compilation that is defined within comments?

cristianoc commented 4 years ago

That is already true. And cppo is the standard for conditional compilation. Same applies to the reason formatter at the moment. This for example does not format: https://github.com/cristianoc/reanalyze/blob/master/src/Compat.re

And I get annoying errors in the editor.

bobzhang commented 4 years ago

reduce use of conditional compilation to files with very little source that don't benefit from automated formatting in the first place.

This is the suggested practice.

Perhaps there could be an alternate syntax for conditional compilation that is defined within comments?

This is an interesting idea that's worth exploring. But not all formatters will preserve comments faithfully, so it is quite fragile.

There's no actionable items we can do here, so I am going to close it

hhugo commented 4 years ago

Maybe you could reuse the syntax from ppx_optcomp

[%%if ocaml_version < (4, 02, 0)]
let x = 1
[%%else]
let y = 2
[%%endif]
cristianoc commented 4 years ago

@hhugo does that work with ocamlformat?

hhugo commented 4 years ago

Yes. It's standard ocaml syntax (extension points and attributes)

cristianoc commented 4 years ago

Looks a little strange in .re but it reformats:

%if
ocaml_version < (4, 02, 0);
let x = 1;
[%%else];
let y = 2;
[%%endif];

I'll give ppx_optcomp a try.

cristianoc commented 4 years ago

Mmm looks like esy complains about some dependencies. OK never mind then, not going to debug that.

hhugo commented 4 years ago

ocamlformat preserves short/long syntax for extension point. refmt should probably do the same.