reasonml / reason

Simple, fast & type safe code that leverages the JavaScript & OCaml ecosystems
http://reasonml.github.io
MIT License
10.13k stars 430 forks source link

Problem with merlin and MerlinTypeOf when using ppx #2143

Closed jordwalke closed 6 years ago

jordwalke commented 6 years ago

This repo repos the issue:

https://github.com/andreypopp/reason-ppx-let-merlin

Thank you for reporting @andreypopp

zindel commented 6 years ago

Wanted to second this one. Fastpack makes heavy use of the lwt.ppx and now all the MerlinTypeOf and MerlinLocate are broken

jordwalke commented 6 years ago

Yes, we need to fix that issue with MerlinTypeOf. I filed an issue and the main ppx tools that JaneStreet makes will filter out the Reason attributes we put on strings. In the mean time there's a fix on our end we can/should do:

When the output is --print binary (which is the default when used in a compiler toolchain) we should strip extra ppx attributes that we added to the AST. In this case, it's the extra unicode string attributes that we add to strings that are messing up this ppx plugin (it should have also been resilient to this, but that's not the case). Those style based annotations aren't needed when actually being sent to a compiler (--print binary) and so should be stripped when --print binary. This along with any other "style preservation" attributes we place just to annotate the AST for the sake of printing heuristics.

Heads up to @IwanKaramazow @jaredly

As we put more style data in the AST in the form of ppx attributes, they should be stripped when used in an actual compiler (as opposed to when only running refmt for the sake of printing in editor).

jordwalke commented 6 years ago

Actually, the inline string is the reason why ppx-inline-test has trouble, so it would be good to fix that: https://github.com/janestreet/ppx_inline_test/issues/15

But any ppx_let issues might be different.

IwanKaramazow commented 6 years ago

Fixing later today, found the problem.

IwanKaramazow commented 6 years ago

Ok, confirmed, the locations on the ast nodes aren't correct. Fixing this atm.