Begin cleaning up the PPX

[aantron]

This commit deprecates a lot of little-used PPX features, which are probably also somewhat ill-conceived. This includes all PPX options and several bits of syntax:

• -no-debug: There is no point in supporting this (PPX: remove -no-debug option #528). In opam, it is used only in sqlexpr (cc @mfp).
• -no-sequence, -no-strict-sequence: We are removing >> anyway (Remove >> syntax from the PPX #495), so we should remove these options. They have no users in opam.
• -log, -no-log: We are deprecating Lwt_log, so it's probably worthwhile to remove logging support from the PPX (Delete logging support from the PPX #520). These options have no users in opam.
• [%lwt ...] syntax expanding to Lwt.catch .... This is causing problems ([easy-ish] PPX: replace [%lwt ...] by [%lwt.catch ...] or remove it #527), so we don't want to support it. It is used in eliom and sqlexpr (cc @mfp @balat @jrochel @vouillon @vasilisp).
• [%finally ...] should be replaced by [%lwt.finally ...]. The bare [%finally ...] is used in devkit, gdb, gdbprofiler, sqlexpr (cc @cyberhuman @ygrek @copy @mfp).

Some more notes:

• While editing the PPX, I noticed an undocumented feature: a let%lwt ... structure item becomes a call to Lwt_main.run. It might be impossible to support this on Node.js in a reasonable way. I'm not sure if we want to deprecate it yet, but it's good that it is not documented.
• -no-sequence is the most legitimate of the PPX options. Users that want >> as a definable operator may be using it to suppress Lwt's >>. I'm not sure if the warning will be too noisy for them.
• This PR doesn't check for [%lwt let ...], just [%lwt ...] when it would expand to Lwt.catch .... We should probably disable [%lwt let ...] and [%lwt ...] around all other constructs, by checking the location of the %lwt extension point, as is being done in Ppx: add support for begin%lwt e1; e2; end in ppx extension #525 for ;. This is still being discussed there, so left for future work.
• @kandu, this PR doesn't include your change about inserting spaces into command line argument descriptions. Whichever order this PR and Ppx: add support for begin%lwt e1; e2; end in ppx extension #525 get merged in, I'll be sure to port/cherry-pick that commit and you will get credit for it :)

Some time after deprecating all this, maybe in lwt_ppx 2.0.0, we can delete support for all these features from the PPX, making it much simpler than it is now. A side effect of factoring the PPX out into lwt_ppx is that it can have its own release cycle, so we could do this without a major release of the main package lwt.

[aantron]

Also, I believe most of this stuff was included in the PPX to make it more obvious how to convert from the Camlp4 syntax. However, these particular features turned out to be not needed by most users.

[copy]

[%finally ...] should be replaced by [%lwt.finally ...]

May I ask what the rationale is behind this change? I don't mind updating my software, I just prefer having a less noisy version. A common pattern in ppxs to avoid name clashes is to provide both a prefixed and a non-prefixed version. For example, from ppx_deriving:

The [%derive.x:] syntax can be shortened to [%x:], given that the deriver x exists and the payload is a type. If these conditions are not satisfied, the extension node will be left uninterpreted to minimize potential conflicts with other rewriters.

[aantron]

Just to avoid crowding the namespace. I got the impression that the main reason it exists in unprefixed form is that the Lwt Camlp4 syntax had a finally keyword.

I wasn't aware of this being a common PPX pattern, if that's so we can remove that commit. It's also the one I'm least convinced about deprecating.

[aantron]

I checked the reverse dependences for lwt.finally as well. It's used in ocurl and otetris. Interestingly, it's also used in devkit, and it appears the commit that introduced lwt.finally is more recent than the one that introduced finally. Anyway, it's not clear that the community prefers either form, largely because the community doesn't seem to use this syntax.

Actually, in all cases except otetris, either syntax seems to have been the result of conversion from Camlp4. I was hoping to see if there is a preference either way for writing new code, but the sample is pretty small.

Of course, that only covers code in opam, so I don't really know.

[ygrek]

Here is some quick data point :

$ ag --stats-only %lwt.finally
80 matches
41 files contained matches
35910 files searched
$ ag --stats-only %finally
83 matches
31 files contained matches
35910 files searched

[ygrek]

PS I hope -no-sequence stays as long as >> is supported so that we can ensure no new >> is introduced in the code accidentally.

[Drup]

I think --no-debug should stay around, mostly because it's very useful when you are doing dev on the ppx itself.

I agree with @copy on %finally. Having both a qualified and unqualified version is a common idiom, and I think it's a useful one.

I mostly agree with the rest.

[aantron]

@Drup, can you say how -no-debug helps with developing the PPX? I don't see what the benefit is.

I find the existence of -no-debug to be a hindrance to developing the PPX, as conditional code is needed because of it.

[aantron]

Also I believe @kandu mentioned that it's easier to work on the PPX with -no-debug.

[aantron]

Okay, I think I will merge this without the commit about deprecating [%finally].

@Drup: -no-debug will be deprecated, but it will remain for now. Hopefully, we will (have time to) figure out what's happening with stack traces soon, then we can decide what exactly to do with it. If we can't get backtraces to work using the PPX, we won't be emitting backtrace_ functions anyway, and the output will be simple too.

@ygrek: Indeed, we won't remove -no-sequence until >> is removed, and it will stick around for a while after that, too, as a no-effect option, to avoid breaking builds.

[Drup]

Well, -no-debug makes the code produced by -dsource/-dparsetree readable. :]

Is the conditional code such an issue ? It's fairly well localized and not extremely complicated.

[aantron]

I don't think either the conditional code nor complex -dsource output is a big deal, no. I'd like to remove -no-debug from the docs and discourage anyone from passing it "in production" for now.

[Drup]

I'd like to remove -no-debug from the docs and discourage anyone from passing it "in production" for now.

That's totally fair. :)