search

mfp / ocaml-sqlexpr

Minimalistic syntax extension for type-safe, convenient execution of SQL statements.
Other
86 stars 17 forks source link

WIP: hoisting #14

Open j0sh opened 8 years ago

j0sh commented 8 years ago

Not really intended for merging, just a preview of work so far -- I'm still learning ppx too. Feedback welcome.

Firstly, this adds a dependency on Ppx_core for AST folding. The setup for that is quite complex (thanks @Drup for help with this and all my other ppx questions...). The biggest issue is that I think using Ppx_core introduces a transitive dependency on OCaml >= 4.02.3, which is probably undesirable for sqlexpr. I can either try reworking the opam/oasis files to compile ppx only conditionally (hopefully that's possible?), or implement a fold using OCaml's Ast_iterator.

The ppx tests for sqlexpr all pass, but this doesn't yet hoist everything that pa_estring does: handling for Pstr_eval and Pstr_class are missing. The latter simply hasn't been done yet, and the former [1] leads to some very strange interactions with Lwt bindings [2]. (Probably a bug on my end, but I haven't figured it out yet...)

Also there are no tests explicitly for hoisting, but the regular test suite seems to stress the mechanism pretty well, I think. Probably should add some more tests once Pstr_eval and Pstr_class get added in.

Finally, a thought I had while implementing this: it might be more idiomatic to use an annotation alongside the [%sql ...] declaration, eg [@cached] or similar, rather than defining [%sqlc ...] as a separate extension. Since the work for [%sqlc ...] has been done already, probably doesn't matter too much.

[1] https://github.com/j0sh/ocaml-sqlexpr/commit/e828cff3c0889790955a0b7c046bf11ec954ff17 [2] https://gist.github.com/j0sh/434844532f67e87ffb60#file-broken-ml-L31-L42

mfp commented 8 years ago

On Wed, Mar 02, 2016 at 10:38:40PM -0800, Josh Allmann wrote:

Not really intended for merging, just a preview of work so far -- I'm still learning ppx too. Feedback welcome.

Looking at it (but I only know a little more ppx than last week, mostly from figuring out how your code works, so I can't promise I will be of much help).

Firstly, this adds an extension on Ppx_core for AST folding. The setup for that is quite complex (thanks @Drup for help with this and all my other ppx questions...). The biggest issue is that I think using Ppx_core introduces a transitive dependency on OCaml >= 4.02.3, which is probably undesirable for sqlexpr. I can either try reworking the opam/oasis files to compile ppx only conditionally (hopefully that's possible?), or implement a fold using OCaml's Ast_iterator.

I had already assumed it would become difficult to support ppx without breaking the build on older versions, so I've already made a 0.6 branch meant to be buildable with 4.01 (so master with ppx support would be released as 0.7 or, why not, 1.0). Frankly, this represents less work than fighting oasis/opam (I don't foresee more than a couple releases in the 0.6 branch, with some bugfixes from master).

I think it's acceptable to drop ppx support for 4.02.[12]: it's never been released, having been been merged for barely a week, so we're not breaking anybody's code yet.

So unless somebody else comes up with a good reason not to, AFAIAC you can feel free to introduce a >= 4.02.3 dependency if that means less work for you.

The ppx tests for sqlexpr all pass, but this doesn't yet hoist everything that pa_estring does: handling for Pstr_eval and Pstr_class are missing. The latter simply hasn't been done yet, and the former [1] leads to some very strange interactions with Lwt bindings [2]. (Probably a bug on my end, but I haven't figured it out yet...)

Is Pstr_eval of any practical significance? If I understand it correctly, it's for top-level expressions being evaluated right away when the module is initialized(?).

As for Pstr_class, you could argue that you're not very concerned about speed once you're dealing with objects (and failing to hoist sqlexpr statements/expressions should have a very limited impact).

They're minor use cases for a minor optimization, not worth the trouble if they prove difficult.

Finally, a thought I had while implementing this: it might be more idiomatic to use an annotation alongside the [%sql ...] declaration, eg [@cached] or similar, rather than defining [%sqlc ...] as a separate extension. Since the work for [%sqlc ...] has been done already, probably doesn't matter too much.

Cannot really say a lot about idiomatic ppx uses, as a non-ppx-user-yet... If you ask me, [%sqlc ...] is OK and already done :)

Mauricio Fernández