Fix locations in generated lwt ppx

[copy]

This fixes two problems in the generation of let%lwt expressions:

1. The location of the generated body (i.e. Lwt.bind t (fun v -> body)) didn't contain the actual body (first commit)
2. The problem mentioned here, i.e. the right hand side of the let binding spanning the whole expression (second commit)

For reference, the diff between the broken parse tree and the fixed one: https://gist.github.com/copy/97aafc69a2467fa390e3f02f5aa97f09 By manual inspection, the locations look correct now.

This fixes #337.

[Armael]

I found the same fixes independently (but @copy was faster :-)), so I believe this PR is correct.

[Khady]

Thanks. I will try to check it on my code this weekend. Does it affect only let or match, try and finally too?

Is there a way to add tests for this BTW?

[aantron]

Tests are interesting. Would we maybe run a merlin query on every character of the file, and expect some result of correct locations from it? Of course, I think such a setup shouldn't be part of this PR or the issue it closes.

[Khady]

It looks good from what I checked. I'm just waiting for a last confirmation from @Engil

[Khady]

@aantron sorry the the late confirmation. Good for me.

@copy thank you very much. I will unlock the bounty. I think you need to claim it.

[aantron]

Thanks to all involved!

@Khady No worries, we're all busy :)

@copy I believe you need to claim the bounty on Bountysource, I guess the proper link for that is here: https://www.bountysource.com/issues/43966981-lwt-ppx-reports-incorrect-locations

And here is some background info linked by @Khady: http://blog.bountysource.com/post/59038181176/weve-improved-how-bounties-are-awarded

[copy]

@Khady @aantron Thanks for the bounty!

[Drup]

Well, there are two things to test for locations in pxxs: merlin is one, but another one is to look at the locations of type error messages returned by the compiler, which is fairly easy to automate using the toplevel (didn't we had that in lwt at some point? At least we do in jsoo and tyxml).

@copy Did you tested that? In particular, I'm not convinced by the 3rd commit.

[copy]

I tested the changes manually using MerlinTypeOf on any expression. I also inspected the parse trees.

@Drup What are your doubts about the third commit? It does use ghost locations, however only for the catch-all pattern exn -> Lwt.fail exn. See: https://gist.github.com/copy/33f28e1071698b429f80ba16a347288e

[Drup]

@copy What I meant is that you should also test by writing some ill-typed code using the ppx and make sure that the typecheck returns an error that is well-located. This is also very easy to do automatically, by using a toplevel and/or topexpect.

[copy]

I wrote some expect tests using the toplevel as @Drup suggested. I wasn't sure how to integrate them into lwt's build system, so the tests are in a separate repository for now: https://github.com/copy/lwt-ppx-expect-tests.

The results look good (in my view), but I also lacked creativity to create type errors (usually I fix them ;-)), so a review would be welcome.

cc @aantron

[aantron]

@copy you can see Bisect_ppx as a reference. Its entire test suite is basically expect tests fit into the OUnit model, though it's much more elaborate that what's necessary for Lwt (due to the number and diversity of tests, and some other factors).

It's all centered on calling Unix.system with redirections. I'm not sure how that will interact with Windows, but Windows does have at least > for redirections IIRC. See https://github.com/aantron/bisect_ppx/blob/f1b1899609cfa1f77cdd5f0e32d614ed0d6d9db4/test/src/test_helpers.ml#L38-L47.